Notification without recipient

[pavelbonda]

Hi!

I'm using this this gem in production app and recently I've met a case when I need to send notification without specific recipient (slack notification + email to support).

Currently gem requires recipient object to be passed to #deliver or #deliver_later. I think that for some cases this is not necessary.

What do you think about making recipient not required?

I achieved this with simple monkey patch, but I'm not sure that it will play well with database notifications:

  def deliver(recipient = :default_recipient)
    super
  end

  def deliver_later(recipient = :default_recipient)
    super
  end

[excid3]

Hmm, hadn't considered that before. I don't see why we couldn't.

Have you tried doing deliver(nil) and deliver_later(nil)? Does that work without a monkey patch?

[pavelbonda]

It will not work with nil because of:

def deliver(recipients)     
  validate!
  run_callbacks :deliver do        
    Array.wrap(recipients).uniq.each do |recipient|
      run_delivery(recipient, enqueue: false)
    end 
  end
end

run_delivery method will not be executed, since: Array.wrap(nil).uniq => []

[excid3]

Ah yes. 👍

From a quick glance, I think only the database delivery method will choke (since it tries to save it as the recipient polymorphic column).

I guess another question is, do we want to change the method arguments. We could just document passing in a symbol for notifications without recipients. For example:

CommentNotification.deliver(:system)

I kind of like requiring an argument still.

[pavelbonda]

Agree with you. Do you think that adding a validation of recipient class to database and action_cable delivery methods (they both consider recipient to be a model) is a good idea?

[excid3]

I don't think it will break anything with ActionCable delivery methods since you can pass strings along as the channel name. We'd need to confirm that, but I think it should be alright.

It would for database though, so at least that might need to raise an error if it doesn't inherit from ActiveRecord::Base

[multiplegeorges]

Just chiming in here, I also need this functionality. It's especially useful for Slack notifications.

As you mentioned, only the database delivery fails, but it's quite handy to use the database along with the others to have a record of what got sent out when.

I like the explicit argument making it clear this is a "general" notification. I'd lean to adding a keyword argument like CommentNotification.deliver(no_recipients: true) to make the intention explicit.

Thanks for this gem, it's great!

[SirRawlins]

I have my own similar use case for this, however I'm not sure allowing delivery without a recipient makes sense.

If you don't have a recipient then where is the notification to be delivered?

I think passing a recipient is fine, you can just create a stub / placeholder recipient for system notifications.

EventNotification.deliver_later(SystemRecioient.new)

[scottjacobsen]

:+1: to this - I'm currently just doing Notification.deliver("") for slack, and that works fine, but making the param optional would be ideal. I considered passing the slack channel names as recipients, but if I ever want to extend the notification to also send emails I would want to use the recipient arg for that.