excid3
Coupling of a Notification with Delivery Methods - Cannot Track Delivery Method Status
https://github.com/excid3/noticed/blob/1020679d06ef2393851119ffda561281d24d037b/lib/noticed/base.rb#L71
The way this is implemented, there is a tight coupling between the notification model and each "channel" delivery method. There is no way to keep track of the deliverability of each delivery method and what the provider sends back as a result.
For example, I also want to be able to track the status of each delivery method (ex: Email & SMS). Many providers will allow callbacks to your application as to the the status of an email (sent, delivered, bounced, read, clicked, ect) or SMS (sent, delivered, failed, ect)...
Maybe a solution is to have another model NotificationDelivery that can store the state of each individual notification (and its corresponding third party id, this is be the unique identifier the Email Provider, SMS provider sends back). Then each notification can has_many NotificationDelivery.
Seems like this would be a big addition to the project and might (or might not) exceed the scope of what the project is trying to accomplish. Open to ideas on if this should be implemented in the project
I have thought about this and my thinking was to just store the delivery details in a json column. Any reason that would be problematic?
I think the risk with a json column is you could start to get some really bloated table rows, especially when you have 4-6 delivery methods for each notification.
Things that (I believe) we should store at the minimum:
- Third Party Id
- Status (sent/delivered/read/failed ect.)
But other things that might be important to store are provider status, provider status code, provider error code, ect... However, maybe for this reason a json column is good. At a minimum, we need to have some sort of standardized data model for the NotificationDelivery. Each provider has their own data model. For consistency, we need some sort of data model that consistent.
It would required (by anyone who wants to track the status of each) to have some sort of index on the provider id as well. (So that you could update the status of the channel notification on a webhook callback). I am not sure the performance impacts of having an index with 4-6 entries for each row. Especially when its likely you are dealing with a table that will likely be millions of rows.
Not sure how hard the metaprogramming on this would be (just psuedo-code below)
# For example in a Twilio Controller, they sent back status of delivered
notifcation = Notification.find_by_delivery_method_[delivery-method-name](thirdparty_id)
notification.delivery_method_[delivery-method-name]_mark_as_delivered!(twilio_data.timestamp)
The above syntax looks really gross, I guess is would seem nicer
notification_delivery = NotificationDelivery.find_by(thirdparty_id: thirdparty_id)
notification_delivery.status = :delivered
notification_delivery.provider_details << request.body
notification_delivery.save
# or
notification_delivery = NotificationDelivery.find_by(thirdparty_id: thirdparty_id)
notification_delivery.mark_as_delivered!(twilio_data.timestamp)
A lot of this is thinking out loud. I really like the syntax of this library and think it needs to stay simple as it already is. Maybe you (or someone else) has ideas on a syntax that could marry performance, readability, and simplicity...?
I guess the thing we should figure out first is what kind of information do we want to track in Noticed (and if there is anything we may not want to or leave up to you to track).
Delivery method responses:
- Database: N/A
- ActionCable: Success true/false
- Email: Success true/false
- Microsoft Teams: Success true/false, HTTP response code and body
- Slack: Success true/false, HTTP response code and body
- Twilio: Success true/false, HTTP response code and body
- Vonage: Success true/false, HTTP response code and body
So I'm kind of thinking: Notification::Delivery
- belongs_to :notification
- delivery_method:string
- success:boolean
- external_id:string
- data:json
- http_code
- http_body
Do Teams / Slack / Twilio / Vonage all return an ID for the message in the response?
Yes - In my experience, they all return an id of some sort (and yes, keep it a string, Slack has some interesting message ids). So to add to yr delivery method responses above we should also add and id. An error_code or error_message might also be helpful (or just raise a wrapped exception).
I'm going to make a big proposal here, but I do it because I think we are getting delivery channels mixed with delivery providers. A channel should be higher level: Email, SMS, Push, Voice, Chatbot. A delivery provider would be Twilio/Vonage, Amazon SES / Postmark / Mailgun... This would mimic the interchangeability like so many other rails things like ActiveJob & ActiveMailer.
It's also important that we make a composite id constraint. This probably should be a unique constraint on delivery_channel + delivery_provider + external_id There's nothing that guarantees uniqueness across vendors. And in practice I've even seen vendors use the same ids for different models (ex: sms and voice). As you could imagine this caused some uniqueness errors.
In the callback action, we would likely do something like below
NotificationDelivery.find_by(vendor: :twilio, channel: :sms, id: external_id)
At the point of callback, we should know those 3 pieces of info.
The notifme library in NodeJS is a great example that lent itself really well to the Strategy Pattern.
Some of their design could save a lot of brain work on our part.
Has there been any update to this? I am currently struggling to find a way to add an error callback on delivery_methods so I can mark them inactive.
just stumbled on this. bit nervous sharing code in case it's rubbish .. but anyway, this is a way I track each individual delivery method, in a single parent Notification, marking them complete or failed.
Introduce a separate DeliveryAuditLog object.
class DeliveryAuditLog < ApplicationRecord
include AASM
belongs_to :notification, required: false
validates_presence_of :delivery_method
aasm do
state :sending, initial: true
state :succeeded, :failed
event :complete
event :fail
end
end
# Updated Notification class
class Notification < ApplicationRecord
has_many :delivery_audit_logs, class_name: 'DeliveryAuditLog', dependent: :destroy
def update_audit_log(delivery_method:, event:)
delivery_audit_log = delivery_audit_logs.find_by(delivery_method: delivery_method)
delivery_audit_log.aasm.fire!(event) if delivery_audit_log
end
def completed?
delivery_audit_logs.all? { |d| d.succeeded? }
end
def failed?
delivery_audit_logs.any? { |d| d.failed? }
end
def audit_log_delivery_methods
delivery_audit_logs.collect(&:delivery_method)
end
The metaprogramming :
module NotificationConfig
# Ensures all delivery methods of a Notification are wrapped in Audit log creation/management
# so we can track the status of downstream deliveries
# The :database method is not tracked as it creates it's own DB record anyway
#
def self.generate_audit_log_callbacks
delivery_methods.each do |d|
next if d[:options][:class].blank?
klass = d[:options][:class]
delivery_klass = klass.is_a?(String) ? klass.constantize : klass.class
# Add the hook
delivery_klass.before_deliver :add_delivery_audit_log
# Define the hook implementation to create the AuditLog
delivery_klass.define_method(:add_delivery_audit_log) do
DeliveryAuditLog.create!(delivery_method: delivery_klass, notification: notification.record)
end
end
end
So to add audit trail, just include generate_audit_log_callbacks
PulseCore::ApplicationNotification < Noticed::Base
include NotificationConfig
.....
generate_audit_log_callbacks
And in your delivery method base classes add the methods to update the audit log - I just needed success/fail but can amend audit state engine events, as needed
ApplicationDeliveryMethod < Noticed::DeliveryMethods::Base
def complete!
return unless notification.record # database_notification
notification.record.update_audit_log(delivery_method: self.class.name, event: :complete)
end
def fail!
return unless notification.record # database_notification
notification.record.update_audit_log(delivery_method: self.class.name, event: :fail)
end
In your delivery method update the audit log
class DeliveryMethods:SomeRequest < ApplicationDeliveryMethod
def deliver
....
blah.errors? ? fail! : complete!
end
What led me here was I'm struggling with replay/recovery - how to target only one specific delivery mechanism that failed, from a notification with multiple delivery mechanisms.
