Ephemeral Notifiers

[bvogel]

WRT to #382

I know that 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.

so before (v1) we had

class SomeEventNotification < Noticed::Base
  deliver_by :database, format: :save_associations_outside_jsonb, if: :database_deliver?
  deliver_by :fcm, format: :firebase_notification, credentials: :firebase_credentials, if: :push_deliver?
  ...

with database_deliver? and push_deliver? checking each users settings for each notification type.

Now I have

class SomeEventNotifier < Noticed::Event
  deliver_by :fcm do |config|
    config.credentials = :firebase_credentials
    config.device_tokens = :fcm_device_tokens
    config.json = :firebase_notification
    config.if = :push_deliver?
  end
  ...

caller was something like this:

      users.find_each do |user|
        SomeEventNotifier.with(
          attribute: @event_log.target_record.foo,
          subject: @event_log.target_record,
          version: 1
        ).deliver(user)
      end

now I'd need to do:

      users.find_each do |user|
        next unless database_deliver?(user) || push_deliver?(user)
        if database_deliver?
          SomeEventNotifier.with(
            attribute: @event_log.target_record.foo,
            subject: @event_log.target_record,
            version: 1
          ).deliver(user)
        else
          SomeEphemeralEventNotifier.with(
            attribute: @event_log.target_record.foo,
            subject: @event_log.target_record,
            version: 1
          ).deliver(user)       
      end

with SomeEphemeralEventNotifier being a 1:1 copy of the original one just with a different base class?

[excid3]

You should just choose the correct class, not duplicate the code if all the params are the same:

      users.find_each do |user|
        next unless database_deliver?(user) || push_deliver?(user)
        notifier = database_deliver? ? SomeEventNotifier : SomeEphemeralEventNotifier
        notifier.with(
            attribute: @event_log.target_record.foo,
            subject: @event_log.target_record,
            version: 1
          ).deliver(user)

And you can use a Concern to share the delivery methods between the two types of notifiers.

class SomeEventNotifier < Noticed::Event
  include SomeEvent::DeliveryMethods
end

class SomeEventEphemeralNotifier < Noticed::Event
  include SomeEvent::DeliveryMethods
end

module SomeEvent::DeliveryMethods
  extend ActiveSupport::Concern

  included do
    deliver_by :fcm do |config|
      config.credentials = :firebase_credentials
      config.device_tokens = :fcm_device_tokens
      config.json = :firebase_notification
      config.if = :push_deliver?
    end
  end
end

I haven't seen a good argument for why you'd want to sometimes deliver a notification to the database and sometimes not.

[bvogel]

Our argument is the freedom of choice for our users, so if they do want to get push notifications about new content we publish, but do not want to see notifications about the same content on the website (when they are already there and most likely already have seen that content) and that's what they prefer, I can't blame them. But it may be that we have an error in the way we use noticed, however this seemed to have worked just fine with v1 and now with v2 it's much more complex. I'd see if I can make your suggestion with concerns and slim classes work, even though that still means having three classes instead of one.

(I think you meant to say a different base class, right?)

class SomeEventEphemeralNotifier < Noticed::Ephemeral
  include SomeEvent::DeliveryMethods
end

[excid3]

You can also keep using v1 if it better suits your needs.

[bvogel]

Yes we could, however it seems as if all future development will be focused on v2, so I feel we'd be better of using that. I just was reacting to you statement that you hadn't come across a use-case where DB wasn't required, laying out ours. However I see having a non-standard use-case will require non-standard solutions.

[excid3]

It still seems a bit odd to me. You can still write the records to the database and just filter them out when querying on the website.

[taras-viiatyk-yalantis]

Hi everyone!

We have the same issue on our project. We want to be able to save notifications into DB conditionally.

@bvogel How did you deal with conditional saving to DB after migration to V2?

[excid3]

You can have 2 notifiers, one ephemeral and one database backed, and choose between them when sending.

[taras-viiatyk-yalantis]

Probably yes, but this ruins the clean abstraction around notifications that we had in version 1. In the previous version, the notifier itself was responsible for conditional saving, and the logic was encapsulated within the notifier. Now, this condition needs to be defined one layer higher, which feels less elegant. In version 1, everything was nice and clean. This is probably the most significant backward compatibility issue introduced in version 2.

[taras-viiatyk-yalantis]

@excid3
I'm not very familiar with the source code yet, so I would like to ask if it's possible to achieve conditional saving to the database in version 2? If so, I’d be happy to explore this further and possibly work on a PR with the necessary changes.

[BooVeMan]

As I was asked: we abstracted almost all code into a concern and then had two notifiers one inheriting from the Noticed::Event and a secondary inheriting from Noticed::Ephemeral, and then the job delivering the notifications will use this (pseudo-) code

      notifier = DatabaseNotifier.with(**attributes.dup)
      if notifier.database_deliver?(user)
        notifier.deliver(user)
      elsif notifier.push_deliver?(user)
        PushNotifier.with(**attributes).deliver(user)
      end