Noticed v2 Update Instructions / Sample Code

[krsyoung]

Work in progress based on experience moving to the new 2.0 💪

What version of Noticed are you using?

v2.0.1

What version of Rails are you using?

v7.1.3

Describe your issue

Just working through the move to v2 and wanted to capture a couple of notes. I'm not through the migration yet on a small - medium app but did hit a couple of issues. Please consider this WIP.

Findings:

1. ~[UPGRADE.md] Syntax error with - an extra closing bracket attributes[:type] = attributes[:type].sub("Notification", "Notifier")) [here]~(https://github.com/excid3/noticed/blob/a519631d867cb36daf3d88ed59359bab361e7fa3/UPGRADE.md?plain=1#L32)
2. ~[UPGRADE.md] Sample code includes a mix of symbols and strings here (i.e. "type" and :type)~
3. ~[UPGRADE.md] Unknown attribute interacted_at (should be read_at? and seen_at should be nil?) here~
4. [BUG?] After running the migrations the type for Noticed::Notification is always empty (which seems to break many things).
5. ~[?] Am I crazy or did there used to be a deliver_later which we seem to have used exclusively?~
6. ~[?] Unable to create new notifications due to PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "noticed_events_pkey DETAIL: Key (id)=(1) already exists.. Really weird, when entering via debugger every time I call deliver on the notification I have the conflict increment by 1.~
7. ~[UPGRADE.md] Since we removed deliver_by :database, what is the approach to conditionally deliver?~
8. ~Slack notifications, json is now a required method (used to use text)~
9. ~Email notifications, method is now a required method (used to default to the same name as the notifier in the mailer class)~

# noticed.rake
# frozen_string_literal: true

namespace :noticed do
  task migrate: :environment do
    # Create a mapping of your notifier class name to the dominant parameter that should
    # be considered the subject / record for the notification
    DOMINANT_PARAM = {
      "ActivityNotifier" => :activity,
      "SlackNotifier" => nil,
      "ThanksNotifier" => :thanks
    }

    # Dummy class to access the old table
    class Notification < ActiveRecord::Base
      self.inheritance_column = nil
    end

    # Migrate each record to the new tables
    Notification.find_each do |notification|
      attributes = {}.with_indifferent_access
      attributes[:type] = notification.attributes["type"].sub("Notification", "Notifier")
      attributes[:params] = Noticed::Coder.load(notification.params)
      attributes[:params] = {} if attributes[:params].try(:has_key?, "noticed_error") # Skip invalid records
      attributes[:notifications_attributes] = [{
        type: "#{attributes[:type]}::Notification",
        recipient_type: notification.recipient_type,
        recipient_id: notification.recipient_id,
        read_at: notification.read_at,
        seen_at: notification.read_at
      }]

      # use the constant map above to define the parameter that should be used as the record
      # for the new event (if one is present)
      if DOMINANT_PARAM.key?(attributes["type"]) && DOMINANT_PARAM[attributes["type"]].present?
        begin
          attributes["record_type"] = attributes["params"][DOMINANT_PARAM[attributes["type"]]].class.name
          attributes["record_id"] = attributes["params"][DOMINANT_PARAM[attributes["type"]]].id
        rescue => e
          # something bad happened
          puts attributes
        end
      end

      Noticed::Event.create!(attributes)
    end
  end
end

Example notifier class:

class ChecklistAssignmentNotifier < Noticed::Event
  # Add required params
  required_param :checklist_assignment

  notification_methods do
    def message
      "New Checklist Assignment #{params[:checklist_assignment].title} due #{params[:checklist_assignment].due_at.strftime("%b %d")}"
    end

    def url
      checklist_assignment_path(params[:checklist_assignment])
    end
  end
end

[excid3]

Thanks for this @krsyoung 👍

1. Fixed.
2. Fixed.
3. Forgot about that, thanks!
4. Not sure on that one. Need some more details.
5. Everything is delivered later now, so that has been removed.
6. Need more details on that.
7. We always deliver to the database now by creating a Noticed::Event. Recipients are optional. Not sure what you mean by conditionally deliver? You mean if: options on delivery methods?

[krsyoung]

If I had actually been helpful I would have put a PR together! Thanks for knocking those off @excid3 , and thanks for 2.0. The interface feels great!

I'm happy to split these into separate issues if you want to avoid a dumpster thread. Just let me know.

❌ 4. Noticed::Notification "type" column always nil

My observation here is that after running the migration to move to the new tables, the "noticed_notifications.type" column is always nil. To compensate for this I added that second loop, which looks to the event association to figure out the class name and appends "::Notification" to it:

Noticed::Notification.find_each do |notification|
  klass_name = "#{notification.event.type}::Notification"
  notification.update(type: klass_name)
end

This fixes one of my initial problems of the helper methods not working in views (i.e. message or url).

My brain tells me that shouldn't be needed - that creating the Noticed::Event should set the "type" in the resulting Noticed::Notfications. I haven't been able to confirm it with a new notification yet because of my other strange problem.

Here is an example of the issue after running the migration code (without the block above):

# nil
irb(main):002> Noticed::Notification.where(type: nil).count
  Noticed::Notification Count (1.6ms)  SELECT COUNT(*) FROM "noticed_notifications" WHERE "noticed_notifications"."type" IS NULL
=> 3814

# not nil
irb(main):003> Noticed::Notification.where.not(type: nil).count
  Noticed::Notification Count (1.4ms)  SELECT COUNT(*) FROM "noticed_notifications" WHERE "noticed_notifications"."type" IS NOT NULL
=> 0

# not nil
irb(main):006> Noticed::Event.where.not(type: nil).count
  Noticed::Event Count (1.5ms)  SELECT COUNT(*) FROM "noticed_events" WHERE "noticed_events"."type" IS NOT NULL
=> 3814

# nil
irb(main):005> Noticed::Event.where(type: nil).count
  Noticed::Event Count (1.6ms)  SELECT COUNT(*) FROM "noticed_events" WHERE "noticed_events"."type" IS NULL
=> 0

Thought: Might be related to this logic and it not being a part of the Noticed::Event.create path / deliver never being called? https://github.com/excid3/noticed/blob/00767eef58d157c54f5f62d34cb3bb752dc23b2c/app/models/concerns/noticed/deliverable.rb#L91

✅ 6. Violation of noticed_events_pkey

I'll dig into this one more (it smells like a postgres issue), however, I didn't change anything in the code base except for the noticed 2.0.1 upgrade. I tried after removing the old notifications table and had the same problem.

More to follow.

I have no good explanation for this other than it has always worked and now doesn't work. Seems like something else changed. The problem is fixed by resetting the primary key sequence (which I have never done in my life). Go figure! (thanks to SO.

ActiveRecord::Base.connection.tables.each do |t|
  ActiveRecord::Base.connection.reset_pk_sequence!(t)
end

✅ 7. Conditional database delivery

This might just be a code smell on our part. One example of a notifier where we use this is to send meeting invite notifications (6 users, 1 is the organizer so they don't really need to know about it). We've used the deliver_by :database, unless: :organizer? style previously. The result being the organizer doesn't get a database notification but the other 5 participants do.

I could buy the argument that the caller should just not pass in the organizer if that is the thinking.

Will beef up the above with some examples over the next ~1h.

[excid3]

Regarding 7, yeah in that case I would just leave out the organizer in the recipients list.

[excid3]

I updated the data migration code in the UPGRADE guide to include the type for Noticed::Notification records and also included a fallback for params that reference missing data.

type is something we have to set manually in order to add the notification_methods feature. I just introduced it on Monday and forgot to update the data migration for it. 🙈

# Temporarily define the Notification model to access the old table
class Notification < ActiveRecord::Base
  self.inheritance_column = nil
end

# Migrate each record to the new tables
Notification.find_each do |notification|
  attributes = notification.attributes.slice("id", "type").with_indifferent_access
  attributes[:type] = attributes[:type].sub("Notification", "Notifier")
  attributes[:params] = Noticed::Coder.load(notification.params)
  attributes[:params] = {} if attributes[:params].try(:has_key?, "noticed_error") # Skip invalid records
  attributes[:notifications_attributes] = [{
    type: "#{attributes[:type]}::Notification",
    recipient_type: notification.recipient_type,
    recipient_id: notification.recipient_id,
    read_at: notification.read_at,
    seen_at: notification.read_at
  }]
  Noticed::Event.create!(attributes)
end

[excid3]

Also made notes for Delivery Method changes in the UPGRADE guide: https://github.com/excid3/noticed/blob/main/UPGRADE.md#delivery-method-changes

[mikepond]

In the upgrade guide,

in the line:

Delete the Notification model from app/models/notifications.rb.

The file name should be app/models/notification.rb without the "s"

And, actually, shouldn't the instruction be to delete the file altogether rather than to delete the model from the file?

[excid3]

@mikepond Fixed 👍

[krsyoung]

I'll re-run the migration tonight and plough through some tests. I did notice that the interfaces to some of the delivery methods have changes and the docs might be lagging (or I might be short on coffee).

@mikepond great point, I ended up also just deleting the notification.rb file.

Very exciting, thanks again @excid3!

[excid3]

I spent a little time earlier updating docs/delivery_methods folder for the new options and added them to the UPGRADE.md docs as well. Let me know if you find anything else that needs updating. Since this was a total rewrite of the gem, almost everything ended up changing (but I think it'll be worth it in the long run).

[krsyoung]

Fantastic! Doc updates are super helpful New migration code is perfect.

I'm having some weird issue with emails not delivering, but I'm thinking that is a "me" problem. Once I squash that, I think we're in good shape to promote to prod over the weekend and we can fully enjoy noticed 2.0 🎆

I'll close this out. Thanks for all of the work @excid3! 🙇

[excid3]

Let me know what you find out regarding emails. Maybe your workers aren't processing the mailer queue?

[krsyoung]

1. Happy Friday 🎆
2. The _pk issue is back
3. I'm now suspicious of the assignment of attributes["id"] via the slice in the migration. Will fiddle around with it!

.. then onto the mail going MIA.

[excid3]

We can probably remove that. There's really no reason to preserve IDs when migrating to the new tables.

[krsyoung]

Beauty, that makes a lot of sense. Can confirm that works!

As for the mail delivery, I had all sorts of wrong in some unless statements (procs / symbols / instance / class etc..). I believe I have that sorted now and all is good!

  deliver_by :email do |config|
    config.mailer = "MeetingMailer"
    config.method = "meeting_invite_notification"
    config.unless = -> { gcal? }
  end

  notification_methods do
    def gcal?
      params[:meeting].gcal_id.present?
    end
  end

Again, as per above, it might be a better pattern to avoid this in the controller and skip the unless in the notifier.

💪

[excid3]

Yeah, if it's a Proc, it will be evaluated in context of the Notification. If it's a symbol, it's evaluated on the Notifier and passes the delivery method as the argument. I am not sure if the delivery method or the notification should be the argument though.

It looks like I did not test this in the test suite so it's still open to changing. I don't see much reason for this to pass in the DeliveryMethod instance and probably should be a Notification.

class CommentNotifier < Noticed::Event
  deliver_by :email do |config|
    config.if = :gcal?
  end

  def gcal?(notification)
    notification.params[:meeting].gcal_id.present?
  end
end

[krsyoung]

I like the looks of that and it makes sense with the notification as the param. I was running through debugger and spaced on the self param in evaluate_option method for the symbol clause.

I'm sure as the community adopts there will be some creative ideas one way or the other. For us, this upgrade has been fantastic and there is no better Friday than sending to prod :shipit:

[Halvanhelv]

Hi! @excid3 I have a problem with the migration to v2, I would be grateful for your help

I use Rails 7.1.2, ruby 3.2.2, noticed 2.0.1

in app/notifiers I have for example class below::

class PostCommentedNotifier < Noticed::Event
  required_params :post_id, :post_title, :comment_id, :comment_body
end 

I migrated the data from the old notifications table using the code from the upgrade.md instructions and also renamed app/notifications to app/notifiers and renamed classes and changed the parent of all classes to Noticed::Event

For the User model, I added has_many :notifications, as: :recipient, dependent: :destroy, class_name: "Noticed::Notification"

I get an error when I try to call the notification collection

_The single-table inheritance mechanism failed to locate the subclass: 'PostCommentedNotifier::Notifier'. This error is raised because the column 'type' is reserved for storing the class in case of inheritance. Please rename this column if you didn't intend it to be used for storing the inheritance class or overwrite Noticed::Notification.inheritance_column to use another column for that information._

In the table noticed_notifications

in the type field is the value PostCommentedNotifier::Notifier which appeared when I executed the code from upgrade.md

Can you please tell me how I can get this fixed?

[excid3]

PostCommentedNotifier::Notifier should be PostCommentedNotifier::Notification

[Halvanhelv]

@excid3 I also noticed that deliver_by :database was removed and how to deal with this for example if I had conditions for delivery by database? It was useful to me for example that users do not spam with the same actions, for example notifications about votes

And to do that, I did the following deliver_by :database, unless: :was_voted?, format: :custom_attributes

  def was_voted?
    Notification.exists?(spam_protect_unique_key: unique_key)
  end

  def unique_key
    "post_upvoted_post_id_#{params[:post_id]}_actor_account_id_#{params[:actor_account_id]}"
  end

[excid3]

Notifications always deliver to the database now. You can choose to include the recipient or not before delivery.

[Halvanhelv]

Notifications always deliver to the database now. You can choose to include the recipient or not before delivery.

got it, thanks for the help

[phil-6]

Hey @excid3 I'm just working on a plan for our upgrade to V2, haven't done a deep dive into the code yet, so apologies if I've missed something. Instead of creating a new issue I thought it make sense to tag onto the end of this thread to keep all the knowledge in one place - would you prefer if I opened this a a new issue or asked these sort of questions elsewhere?

Notification Model Methods

We have a notification index that allows the user to filter, and search their notification history by using different scopes we added to the previous notification model. We also used counter culture to cache notifications counts and had a few other methods that we added to the model.

Where should these live in a V2.0 world?

# app/models/notification.rb #v1.6
class Notification < ApplicationRecord
  include Noticed::Model
  belongs_to :recipient, polymorphic: true

  scope :filter_by_type, ->(type) { where(type:) }
  scope :filter_by_org, ->(organisation_id) { where(organisation_id:) }
  scope :exclude_type, ->(type) { where.not(type:) }

  counter_culture :recipient, column_name: "notifications_count"
  counter_culture :recipient, column_name: proc { |a| 'unread_notifications_count' if a.read_at.nil? },
                              column_names: { Notification.unread => :unread_notifications_count }
   def organisation
    Organisation.find_by(id: organisation_id)
  end
end

Recipient IDs as UUIDs

We use UUIDs as primary keys throughout out app - is this as simple as modifying the migrations to add type: :uuid?

Deliver Later

Previously we made fairly extensive use of .delivery_later to offload the processing of notifications to a background job. What is the suggested approach for background processing for large numbers of individual notifications?

Thanks!

[excid3]

1. You'd add those to the Noticed::Notification model with a to_prepare block and a concern.

module NotificationExtensions
  extend ActiveSupport::Concern

  included do
    scope :filter_by_type, ->(type) { where(type:) }
    scope :filter_by_org, ->(organisation_id) { where(organisation_id:) }
    scope :exclude_type, ->(type) { where.not(type:) }

    counter_culture :recipient, column_name: "notifications_count"
    counter_culture :recipient, column_name: proc { |a| 'unread_notifications_count' if a.read_at.nil? },
                              column_names: { Notification.unread => :unread_notifications_count }
  end

  def organisation
    Organisation.find_by(id: organisation_id)
  end
end

Rails.application.config.to_prepare do
  Noticed::Notification.include NotificationExtensions
end

2. Yep.
3. Notifications are always delivered later now.

[phil-6]

Thanks Chris, super helpful as always.

Next questions (sorry!):

Previously we were able to define a format_for_database on the database option to allow us to define custom database fields.

What's the V2 approach to this?

v1.6 example:

# app/notifications/message_notification.rb
class NewChatNotification < Noticed::Base
  include IosNotification
  include AndroidNotification
  deliver_by :database, format: :format_for_database

  def title
    "New Chat Created by #{params[:creator].full_name}"
  end

  def body
    "You've been added to a new chat."
  end

  def url
    chat_path(params[:chat])
  end

  def organisation_id
    params[:chat].organisation_id
  end

  def organisation_name
    params[:chat].organisation.name
  end

  def format_for_database
    {
      type: self.class.name,
      title:,
      body:,
      url:,
      organisation_id:,
      organisation_name:,
      params:
    }
  end
end

With regards to the initial #7 from the list at the start of this thread. We have quite a few notifications that we want to deliver to a specific user via APNS or FCM but not show up in the notification index, appear in any of the counts. They also don't need to be "read".

It doesn't make sense to me in this case to not pass the recipient as you suggested here. I guess we could pass the recipient in the params, but that doesn't feel great either.

I guess we could scope the database to exclude notifications of these types

[excid3]

Depends on where you want to add the extra fields. If it's on the Noticed::Event (shared for all recipients), just add them to the table and use

NewChatNotification.new(
  params: {},
  organisation_id: "",
  organisation_name: ""
  ...
)

What's nice is the Notifiers are just ActiveRecord models, so you can instantiate them with new to set any attributes. with is just a shortcut to match the Rails params style from ActionMailer.

For Noticed::Notifications (unique for each user), you can override the recipient_attributes_for(recipient) method: https://github.com/excid3/noticed/blob/main/app/models/concerns/noticed/deliverable.rb#L91-L97

The database records are required to process the job, so you'll want to filter out those in the UI I think. Could be as simple as a scope that does not return Notifications of certain types:

scope :notifications, ->{ includes(:events).where.not(events: {type: EXCLUDED_TYPES}) }, as: :recipient, dependent: :destroy

[phil-6]

Thanks Chris.

Using .new is working nicely. NewMessageNotifier is my test

 NewMessageNotifier.new(record: self, params: { sender: user }, organisation: chat.organisation)
                        .deliver(chat.users.excluding(user))

for the scope - that was the direction I was heading towards.

I've ended up adding it as part of the Notification Extensions, as type is delegated to event already I can just use

scope :for_index_and_counts, -> { where.not(type: EXCLUDED_TYPES) }

Still not certain on the naming of that scope though!

[phil-6]

@excid3 Okay, I really hope this is the last one!

Previously we had some callbacks to fire TurboStream broadcasts on the notification model.

We have one after_create which prepended the notification index with a partial, and several after_commit that update various counts, in the navbar and menu. We also have a couple of counter_culture methods which run as callbacks.

Including these as an extensions via to_prepare doesn't seem to be working.

The after_create turbo_stream might make sense to have as a custom TurboStream delivery method, but do you have any suggestions as to why the after_commit callbacks might not be working or another suggestion on how to steam an update to a count after notification has been marked as read?

(StackOverflow, Reddit and the GPTs haven't helped)

Edit: just read that v2.0 uses insert to create the Notifications. That explains why the after create and counter_cultures don't work.

Any suggestions appreciated

[chasebeck]

@phil-6 I had the same issue. I ended up using a custom delivery method (DeliveryMethods::TurboStream) which is mentioned in the README but only after you expand the "Turbo Stream Custom Delivery Method Example" section 👎. I was then able to reuse the original Notification after_create callback broadcast_* methods. Hope that helps!

[marckohlbrugge]

@phil-6 Your comment saved me a lot of time 🙌

I've been debugging why my after_createcallback didn't work, and then I saw your tweet mentioning the same problem. I ended up "fixing" it by using a custom delivery method.