Documentation improvement: user params.except(:post) instead of params.delete(:post)

[sjieg]

In the documentation using a notifiable relation the following is suggested to use:

def format_for_database
    {
      notifiable: params.delete(:post),
      type: self.class.name,
      params: params
    }
  end

In my case, when sending a notification to more than 1 user, with the second users params[:post] did not exist anymore, because that parameter was deleted on the previous iteration.

This caused quite a strange bug, as I was creating the Notification from a delayed job, and the error caused the job to repeat. So this caused the first user to receive multiple messages, while the second user didn't receive anything.

Anyway, I'd like to recommend the following documentation to prevent these kind of problems:

  def format_for_database
    {
      notifiable: params[:post],
      type: self.class.name,
      params: params.except(:post)
    }
  end

If you like the following idea, I can make a new ticket for it:

Add a parameter to database to set the notifiable target:

deliver_by :database, notifiable: :post

Which will check params for a :post value and set it.

WDYT?

[excid3]

+1 for the documentation update. We should definitely do that.

As for the notifiable: :post I think it's a good idea, but maybe needs a better name. I'm trying to think of a more descriptive name for it, but that's a tough one. polymorphic_param_name: :post or something descriptive?

[sjieg]

notification_source
notify_from
notification_target
notifiable_param
notifiable_relation

Is the given value always a param or could it also be a referring function like so?

deliver_by :database, notifiable_relation: :get_relation

param :reply

def get_relation
  params[:reply].post
end

Now it got me thinking, the current solution allows me to name notifiable however I want. Perhaps that dynamic should be kept by doing something like:
belongs_to_{polymorphic_relation_name}: :post

[excid3]

Yeah, I guess since we don't dictate the name of the relationship that makes it tougher to do this automatically.

At the moment, I'd rather keep it simple and just improve the docs and we can keep discussing ideas for polymorphic notifications.

[rafaelpivato]

If you like the following idea, I can make a new ticket for it:
Add a parameter to database to set the notifiable target:

deliver_by :database, notifiable: :post

Which will check params for a :post value and set it.

This idea sounds great. I saw some examples on README explaining how to query notifications based on "post" or a notifiable. To me it sounds as important having that as having a recipient. To adhere to a simpler terminology akin to "recipient", the term "subject" could be used instead of "notifiable".

Anyway, I truly believe we need that other polymorphic as part of the ActiveRecord model.

Also, I don't think that should be an option to database but a broader approach to the whole notification class, the same way recipient is. Maybe one of the following calls would do the job.

PostCreatedNotification.about(post).deliver(user)
PostCreatedNotification.with(user_agent: request.headers['User-Agent']).about(post).deliver(user)
PostCreatedNotification.deliver(user, subject: post)

# Not advisable as the subject would not be an actual param
PostCreatedNotification.with(subject: post).deliver(user)

[sjieg]

Created separate ticket for tracking the idea's around the polymorphic relation: #96

This ticket can be closed as the updated documentation has been merged and does not need a release.