Re-Implement Callbacks

[avanrielly]

Pull Request

Summary:
Adding ActiveModel::Callbacks back into Noticed that were removed in 2.0.

Related Issue:

Description:
This PR adds ActiveModel::Callbacks back into the Noticed gem. This is a feature that was broken when 2.0 was released. The code supports the DeliveryMethod callbacks, but it does not include event level callbacks.

Callbacks supported:

• DeliveryMethod#deliver

Testing:
I have added unit tests for each type of callback to ensure they are called appropriately.
I have integrated it with our app to test each kind of callback to make sure it works.

Screenshots (if applicable):

Checklist:

• Code follows the project's coding standards
• Tests have been added or updated to cover the changes
• Documentation has been updated (if applicable)
• All existing tests pass
• Conforms to the contributing guidelines

Additional Notes:

[excid3]

I'm not entirely sure how valuable these callbacks are since everything runs in jobs so I decided to leave them out in v2. They're also very confusing as to when they will run.

Does anyone have a valid use case that justifies the complexity?

[avanrielly]

If we want to leave the Event level callbacks out, I am fine with that. We do use the delivery method callbacks, which do run inside the job. These allow us to run metrics on each delivery attempt.

If you are ok with leaving the delivery method callbacks in, I can remove the event level ones.

[delynn]

With v1 we use Delivery callbacks to create records that allow us to keep track of the status of deliveries.

For example, if a notification is sent via Email or SMS, we save off the response ID that is returned when calling Postmark or ClickSend. We then leverage Webhooks from those vendors to update the status of the notification for delivery success/failure and open/read tracking so that we can display that info to administrators so that they can troubleshoot when one of their users complains that they didn't get a notification.

[excid3]

Great example @delynn. 👍

Now that Notifiers are ActiveRecord models, it's pretty easy to extend them so I'm not sure the callbacks are as useful there since you can do anything you want. You could add an after_create callback or a method that wraps deliver:

class MyNotifier < Noticed::Event
  def deliver_with_action(recipients)
     deliver(recipients)
     # any post-save actions
  end
end

It's also a bit weird to have callbacks around enqueuing delivery methods? We won't have a callback that runs after all the deliveries are complete since they're run independently so that just feels confusing to me.

I think the callbacks around deliveries make the most sense.
An alternative could be adding a standard config.after_delivery = ->(recipient) or config.after_delivery = :method_name that you could define?

There's no good way to define the after_deliver callback without overriding the delivery method, so this could be a good option. We could pass it the return value(s) from the Deliver and we could run it on the Notification so you can record the Mail ID for example:

class MyNotifier < Noticed::Event
  deliver_by email do |config|
    config.after_deliver = ->(mail) {
      # Record message ID on the Notification
    }
  end
end

[avanrielly]

I think that both callbacks and the config.after_deliver approach and the DeliveryMethod#callbacks are great features for two different use cases. My proposal would be to add both as separate features and not to consider one a superior solution over the other.

The callbacks will be used best in the case of encapsulating functionality for any custom delivery methods an app might have. This would be done by having an ApplicationDeliveryMethod that contains any callback logic required for all custom delivery methods. This reduces duplication and complexity.

The config.after_deliver approach would allow for a hook into the default Noticed delivery methods. This would reduce the need for custom delivery methods. The downside is that the method/proc would have to be added for every deliver_by block, even if each implementation is the same.

My one concern is that the evaluate_option method does not have the concept of any parameters. How would you suggest passing in the results of a delivery without mixing too many concerns?

[delynn]

@excid3 I'm selfishly only interested in callbacks on `Noticed::DeliveryMethod' since we only use the callback functionality on our custom delivery methods. 😄

That said, I agree with @avanrielly that something like config.after_delivery would allow for hooks into the results of the built in delivery methods if that's something you think people might also need.

[excid3]

Got some examples to share the pros and cons of callbacks?

For example, I feel like the deliver method should be the one handling most things, in which case a callback doesn't really improve much, especially if it didn't have access to the return value of deliver.

Say you want to deliver an email and store the mail ID:

class MyDelivery < Noticed::DeliveryMethod
  def deliver
    mail = send_email
    save_activity mail
  end
end

Examples always help, so please share!

[delynn]

For me, I think the pro of Delivery callbacks is to dry up tracking code for custom delivery methods. In your example any custom delivery method needs to know how to save off the activity. This isn't terrible, but in our case with v1, we were able to abstract this out to something like the following:

class ApplicationDeliveryMethod < Noticed::DeliveryMethod
  attr_accessor :client_response

  after_deliver :save_client_response

  private

  def save_client_response
    if client_response
      # Shared code to keep track of responses in a generic way that webhooks can then update
    end
  end
end

[excid3]

What does your implemented delivery method(s) look like? I don't see much value added here with callbacks and it feels a lot more indirect than something like:

class ApplicationDeliveryMethod
  private

  def save_client_response(response)
    return unless response.present?
    # Shared code
  end
end

class MyApiDelivery < ApplicationDeliveryMethod
  def deliver
    save_client_response(make_request)
  end
end

This does make me want to implement an ApplicationNotifier and ApplicationDeliveryMethod in the generators too. 👍

[phil-6]

We had two use-cases for callbacks.

One was to update the web UI via Turbo Streams - but this could easily be a custom delivery method.
The other was to update counter caches for the user's number of unread and total notifications.

[delynn]

@excid3 I saw you merged this, but also wanted to get back to you. I think your solution to our use case of callbacks would have worked. I think we just went with callbacks since v1 had the functionality. Thank you for bringing this functionality back. It'll mean a smaller upgrade/refactor lift for us when we move to v2.

I like your idea of adding a generator for ApplicationNotifier and ApplicationDeliveryMethod.

[excid3]

I think the Application classes are a good example of where the callbacks make sense. Added the Application classes last night btw. 👍