Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ephemeral notifiers #382

Merged
merged 11 commits into from
Jan 29, 2024
Merged

Add ephemeral notifiers #382

merged 11 commits into from
Jan 29, 2024

Conversation

excid3
Copy link
Owner

@excid3 excid3 commented Jan 23, 2024

This adds an Ephemeral class for sending notifications. This was previously a feature of Noticed v1 but was removed to make solve for bulk notifications. The way things are architected, we can simulate the Notifier & Notification classes for databaseless / ephemeral notifications that should not be saved in the database.

Ephemeral notifiers basically just pass the params and recipients to the delivery method jobs directly. The Notifier and Notification use ActiveModel to set up the same basic attributes as the database models in order to achieve compatibility. It seems to work pretty well.

This is an experimental idea to see if it would be a reasonable solution. There are limitations to this in that config options must be serializable by ActiveJob (meaning Procs and Lambdas cannot be used for delivery method configs, etc).

See #375

@excid3 excid3 added the enhancement New feature or request label Jan 23, 2024
@excid3 excid3 self-assigned this Jan 23, 2024
@excid3
Copy link
Owner Author

excid3 commented Jan 25, 2024

@viktor-shmigol have you had a chance to test this?

@viktor-shmigol
Copy link

viktor-shmigol commented Jan 26, 2024

@excid3 I'm sorry for the long response. I was a little bit busy.
I've tested it and I found that I'm not able to call any notifier method from the Noticed::DeliveryMethod instance.
For example:

class NewMessageNotifier < Noticed::Ephemeral
  deliver_by :internal_notification, class: 'DeliveryMethods::InternalNotification'

  required_params :message

  def channel
    "user_#{recipient.id}"
  end

  def payload
    { body: }
  end

  def body
    I18n.t('notifications.new_message', author: params[:message].author_name)
  end
end


class DeliveryMethods::InternalNotification < Noticed::DeliveryMethod
  delegate :event, to: :notification

  def deliver
    ActionCable.server.broadcast(event.channel, event.payload)
  end
end

NewMessageNotifier.new(params: {message: Message.last}).deliver(User.last)

I'm getting this error:

Error performing DeliveryMethods::InternalNotification (Job ID: 0833436a-bbcd-4ee3-811f-ce31362c6d98) from Async(default) in 9.52ms:
NoMethodError (undefined method `channel' for #<Noticed::Ephemeral:0x0000ffff8149cb50 @attributes=#<ActiveModel::AttributeSet:0x0000ffff8149c8a8 @attributes={"params"...

If I wrap those methods in notification_methods:

class NewMessageNotifier < Noticed::Ephemeral
  deliver_by :internal_notification, class: 'DeliveryMethods::InternalNotification'

  required_params :message

  notification_methods do
    def channel
      "user_#{recipient.id}"
    end

    def payload
      { body: }
    end

    def body
      I18n.t('notifications.new_message', author: params[:message].author_name)
    end
  end
end

I'm getting this error:

NoMethodError: undefined method `notification_methods' for NewMessageNotifier:Class

Also, it would be good to have a way to deliver notifications in foreground mode using perform_now.
E.g:
NewMessageNotifier.with({message: Message.last}).deliver_now(User.last)

@excid3
Copy link
Owner Author

excid3 commented Jan 26, 2024

@viktor-shmigol Give this a try again. It was retrieving Noticed::Ephemeral::Notification constant instead of NewMessageNotifier::Notification which is what caused the method missing error.

@excid3 excid3 merged commit bc18d21 into main Jan 29, 2024
45 checks passed
@excid3 excid3 deleted the ephemeral-notifiers branch January 29, 2024 22:11
@bvogel
Copy link

bvogel commented Jul 23, 2024

I know this is an old PR, but I seem to be missing something:
we have user options that allow our users to determine which kind of notifications they will receive. With v1 there was a setting for web/database and a setting for push/fcm. So the combinations are both (web/push), only web (web/-), only push (-/push) and none at all (-/-) With the v2 options I can cover easily "both", "only web" and "none" - however for "only push" I'd need an ephemeral notifier - does that mean I need to double the code for all notifiers (currently 14 different ones) and select either the regular or the ephemeral based on the users setting? Is that how this is supposed to work? TBH the previous if: for the database method was much more serviceable.

@excid3
Copy link
Owner Author

excid3 commented Jul 23, 2024

@bvogel Please open up a discussion with an example to showcase what you're trying to do.

@bvogel
Copy link

bvogel commented Jul 23, 2024

thanks: #472

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants