Thoughts on using MODEL_id instead of MODEL as param

[henryaj]

Seems like noticed serializes the entire param in the Notification record. I have some pretty chunky models; I don't really want to save a whole copy of them within each Notification.

In some parts of my codebase, I've taken to using e.g. params :lender_id instead of params :lender, and then just doing a Lender.find on the param when I need to get the Lender record back. This seems like it would be fine in quite a lot of circumstances.

Is there any thought on which is preferred and why? I don't think has_noticed_notification works if using MODEL_id, but it seems like it would be a small change for it to be supported (which I could potentially make).

[excid3]

I'm fine with adding it as long as it's not a breaking change. Any idea how you envision that working?

[henryaj]

Something like has_noticed_notifications, reference: true or similar?

[excid3]

Hmm, I feel like we could be more descriptive. by_id: true or something?

Or maybe it needs to be a new method?

has_noticed_notifications_by_id

I'm not sure.

[dwightwatson]

Would it make sense to use the model's Global ID instead - that way there could still be some magic in converting that data back to an ActiveRecord instance if required?

[excid3]

@dwightwatson That's already how it works using the ActiveJob coder.

I don't think GlobalID would help for this stuff because the key is already going to denote the model name.

[marcoadkins]

I made this work in my project by adding a polymorphic subject field to the notifications table. Feels like it could be upstreamed. Improves the AR has_many_notifications logic a bit and removes the need to do a query on a jsonb field to get your associated notifications.

create_table :notifications do |t|
  t.references :recipient, polymorphic: true, null: false
  t.references :subject, polymorphic: true
  t.string :type, null: false
  t.jsonb :params
  t.datetime :read_at

  t.timestamps
end
    
class Notification < ApplicationRecord
  include Noticed::Model
  belongs_to :recipient, polymorphic: true
  belongs_to :subject, polymorphic: true
end

class MessageNotification < Noticed::Base
  deliver_by :database, format: :format_database
  
  def format_database
    {
      type: self.class.name,
      params: params,
      subject: params[:message]
    }
  end
end

class Message < ApplicationRecord
  has_many :notifications, as: :subject, dependent: :destroy
end

[ziaulrehman40]

We are trying to use it with a multi-tenant system, with ros_apartment gem, and it errors out when deserializing user with globalid, i am not fully sure how globalids work in rails/ruby, and if it makes DB request(if it does, it will fail obv and tenant needs to be set to find anything in User table, which can only set after job is running(using serializer or anything, not when params deserialisation is going on).

Any quick solution for our scenario?

[ziaulrehman40]

I think we just HAVE to avoid globalids and play on plain ids, we will try some monkey patching for now

[ziaulrehman40]

UPDATE: We are going en-route to send in ids instead of objects in recipients and overriding methods needed(mainly in the delivery_methods we are using, so far database and email had to be overriden)

[AndersGM]

I have the same issue, and wondered if a very simple solution could an option to specify where to get the value from, e.g.:

has_noticed_notifications param_name: "model_id", param_method: :id

Will happily create a PR for this :-)

[MrKirat]

+1

[MrKirat]

Hello, @excid3 ! Happy to see a new version of this gem!
Could you please share if the suggested functionality from this issue has been implemented since I can't find anything about it in the new gem's documentation? Thank you!

[excid3]

Yep, see the part about the record param as special and gets saved as the record:belongs_to{polymorphic} association on the Noticed::Event.

[marcoadkins]

@excid3 Glad the polymorphic idea panned out. Been working well for us for over a year now.