diff --git a/CHANGELOG.md b/CHANGELOG.md index 7082dd4c..a1e61652 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ ### Unreleased +* [Bugfix] Action Push Native's push notification class is configured with `config.notification_class` (`config.class` is reserved for overriding the delivery method class) +* `deliver_by` with a duplicate name raises `ArgumentError` instead of `NameError`; `recipients :unknown_method` raises `NoMethodError` instead of silently delivering to nobody +* Slack and Webhook delivery logic is shared between individual and bulk delivery methods via `Noticed::SlackDelivery` and `Noticed::WebhookDelivery` * `Noticed::EventJob` enqueues delivery jobs in batches with `ActiveJob.perform_all_later` (Rails 7.1+) and loads notifications with `find_in_batches`. Note that `perform_all_later` does not run ActiveJob enqueue callbacks. * [Bugfix] iOS delivery opens a connection per delivery instead of a shared pool, so `error_handler` runs for the notification actually being delivered and notifiers with different APNs credentials no longer share a connection. The `pool_size` option is removed. * [Bugfix] FCM only treats a 400 as an invalid token when the error refers to the registration token, so malformed payloads no longer delete valid tokens. The access token is fetched once per delivery. diff --git a/README.md b/README.md index 1bc36603..37732d76 100644 --- a/README.md +++ b/README.md @@ -374,6 +374,20 @@ Each of these options are available for every delivery method (individual or bul * `config.if` — Intended for a lambda or method; runs after the `wait` if configured; cancels the delivery method if returns falsey * `config.unless` — Intended for a lambda or method; runs after the `wait` if configured; cancels the delivery method if returns truthy +Lambdas for `if` and `unless` are evaluated in the context of the Notification. A symbol calls a method on the Notifier and passes the notification: + +```ruby +class MessageNotifier < Noticed::Event + deliver_by :email do |config| + config.if = :email_enabled? + end + + def email_enabled?(notification) + notification.recipient.email_notifications? + end +end +``` + The following are evaluated in the context of the Notification so it can be customize to the recipient: * `config.wait` — (Should yield an `ActiveSupport::Duration`) Delays the job that runs this delivery method for the given duration of time diff --git a/app/models/concerns/noticed/deliverable.rb b/app/models/concerns/noticed/deliverable.rb index 588f572f..ba8211eb 100644 --- a/app/models/concerns/noticed/deliverable.rb +++ b/app/models/concerns/noticed/deliverable.rb @@ -18,7 +18,7 @@ def inherited(base) end def bulk_deliver_by(name, options = {}) - raise NameError, "#{name} has already been used for this Notifier." if bulk_delivery_methods.has_key?(name) + raise ArgumentError, "#{name} has already been used for this Notifier." if bulk_delivery_methods.has_key?(name) config = ActiveSupport::OrderedOptions.new.merge(options) yield config if block_given? @@ -26,7 +26,7 @@ def bulk_deliver_by(name, options = {}) end def deliver_by(name, options = {}) - raise NameError, "#{name} has already been used for this Notifier." if delivery_methods.has_key?(name) + raise ArgumentError, "#{name} has already been used for this Notifier." if delivery_methods.has_key?(name) if name == :database Noticed.deprecator.warn <<-WARNING.squish @@ -116,7 +116,7 @@ def evaluate_recipients if _recipients.respond_to?(:call, true) instance_exec(&_recipients) - elsif _recipients.is_a?(Symbol) && respond_to?(_recipients, true) + else send(_recipients) end end diff --git a/app/models/noticed/event.rb b/app/models/noticed/event.rb index 63301a8d..6c3fae9e 100644 --- a/app/models/noticed/event.rb +++ b/app/models/noticed/event.rb @@ -8,20 +8,15 @@ class Event < ApplicationRecord belongs_to :record, polymorphic: true, optional: true has_many :notifications, dependent: :delete_all - accepts_nested_attributes_for :notifications - scope :newest_first, -> { order(created_at: :desc) } attribute :params, :json, default: {} - # Ephemeral notifiers cannot serialize params since they aren't ActiveRecord backed - if respond_to? :serialize - # The coder: keyword was added in Rails 7.1 - if Rails.gem_version >= Gem::Version.new("7.1.0.alpha") - serialize :params, coder: Coder - else - serialize :params, Coder - end + # The coder: keyword was added in Rails 7.1 + if Rails.gem_version >= Gem::Version.new("7.1.0.alpha") + serialize :params, coder: Coder + else + serialize :params, Coder end end end diff --git a/docs/delivery_methods/action_push_native.md b/docs/delivery_methods/action_push_native.md index ea27e41d..bb9ec0d5 100644 --- a/docs/delivery_methods/action_push_native.md +++ b/docs/delivery_methods/action_push_native.md @@ -53,3 +53,7 @@ end * `silent` Should return a `Boolean` if notification should be silent + +* `notification_class` - *Optional* + + The `ActionPushNative::Notification` subclass to deliver with. Defaults to `ApplicationPushNotification`. diff --git a/lib/generators/noticed/templates/bulk_delivery_method.rb.tt b/lib/generators/noticed/templates/bulk_delivery_method.rb.tt index 16597e71..66556a8d 100644 --- a/lib/generators/noticed/templates/bulk_delivery_method.rb.tt +++ b/lib/generators/noticed/templates/bulk_delivery_method.rb.tt @@ -2,7 +2,7 @@ class BulkDeliveryMethods::<%= class_name %> < ApplicationBulkDeliveryMethod # To use this delivery method, specify the class option in your notifier. # # class MyNotifer < ApplicationNotifier - # bulk_deliver_by :<%= file_path %>, class: "<%= class_name %>" + # bulk_deliver_by :<%= file_name %>, class: "BulkDeliveryMethods::<%= class_name %>" # end # Specify required options for the deliver_by config block diff --git a/lib/generators/noticed/templates/delivery_method.rb.tt b/lib/generators/noticed/templates/delivery_method.rb.tt index f7f3f97b..093ab031 100644 --- a/lib/generators/noticed/templates/delivery_method.rb.tt +++ b/lib/generators/noticed/templates/delivery_method.rb.tt @@ -2,7 +2,7 @@ class DeliveryMethods::<%= class_name %> < ApplicationDeliveryMethod # To use this delivery method, specify the class option in your notifier. # # class MyNotifer < ApplicationNotifier - # deliver_by :<%= file_path %>, class: "<%= class_name %>" + # deliver_by :<%= file_name %>, class: "DeliveryMethods::<%= class_name %>" # end # Specify required options for the deliver_by config block diff --git a/lib/noticed/bulk_delivery_methods/slack.rb b/lib/noticed/bulk_delivery_methods/slack.rb index 42c2a0b4..21cceeae 100644 --- a/lib/noticed/bulk_delivery_methods/slack.rb +++ b/lib/noticed/bulk_delivery_methods/slack.rb @@ -1,39 +1,7 @@ module Noticed module BulkDeliveryMethods class Slack < BulkDeliveryMethod - DEFAULT_URL = "https://slack.com/api/chat.postMessage" - - required_options :json - - def deliver - headers = evaluate_option(:headers) - json = evaluate_option(:json) - response = post_request url, headers: headers, json: json - - if raise_if_not_ok? && !success?(response) - raise ResponseUnsuccessful.new(response, url, {headers: headers, json: json}) - end - - response - end - - def url - evaluate_option(:url) || DEFAULT_URL - end - - def raise_if_not_ok? - value = evaluate_option(:raise_if_not_ok) - value.nil? || value - end - - def success?(response) - if response.content_type == "application/json" - JSON.parse(response.body).dig("ok") - else - # https://api.slack.com/changelog/2016-05-17-changes-to-errors-for-incoming-webhooks - response.is_a?(Net::HTTPSuccess) - end - end + include SlackDelivery end end end diff --git a/lib/noticed/bulk_delivery_methods/webhook.rb b/lib/noticed/bulk_delivery_methods/webhook.rb index 42445bd5..c7d821ce 100644 --- a/lib/noticed/bulk_delivery_methods/webhook.rb +++ b/lib/noticed/bulk_delivery_methods/webhook.rb @@ -1,18 +1,7 @@ module Noticed module BulkDeliveryMethods class Webhook < BulkDeliveryMethod - required_options :url - - def deliver - post_request( - evaluate_option(:url), - basic_auth: evaluate_option(:basic_auth), - headers: evaluate_option(:headers), - json: evaluate_option(:json), - form: evaluate_option(:form), - body: evaluate_option(:body) - ) - end + include WebhookDelivery end end end diff --git a/lib/noticed/delivery_methods/action_push_native.rb b/lib/noticed/delivery_methods/action_push_native.rb index 0e0299dc..2b38b296 100644 --- a/lib/noticed/delivery_methods/action_push_native.rb +++ b/lib/noticed/delivery_methods/action_push_native.rb @@ -4,7 +4,7 @@ class ActionPushNative < DeliveryMethod required_options :devices, :format def deliver - notification = (!!evaluate_option(:silent)) ? notification_class.silent : notification_class + notification = evaluate_option(:silent) ? notification_class.silent : notification_class notification .with_apple(evaluate_option(:with_apple)) @@ -14,8 +14,9 @@ def deliver .deliver_later_to(evaluate_option(:devices)) end + # `notification_class` rather than `class` since `config.class` is reserved for overriding the delivery method def notification_class - fetch_constant(:class) || ApplicationPushNotification + fetch_constant(:notification_class) || ApplicationPushNotification end end end diff --git a/lib/noticed/delivery_methods/slack.rb b/lib/noticed/delivery_methods/slack.rb index 0c9fd4b9..355e6c86 100644 --- a/lib/noticed/delivery_methods/slack.rb +++ b/lib/noticed/delivery_methods/slack.rb @@ -1,39 +1,7 @@ module Noticed module DeliveryMethods class Slack < DeliveryMethod - DEFAULT_URL = "https://slack.com/api/chat.postMessage" - - required_options :json - - def deliver - headers = evaluate_option(:headers) - json = evaluate_option(:json) - response = post_request url, headers: headers, json: json - - if raise_if_not_ok? && !success?(response) - raise ResponseUnsuccessful.new(response, url, {headers: headers, json: json}) - end - - response - end - - def url - evaluate_option(:url) || DEFAULT_URL - end - - def raise_if_not_ok? - value = evaluate_option(:raise_if_not_ok) - value.nil? || value - end - - def success?(response) - if response.content_type == "application/json" - JSON.parse(response.body).dig("ok") - else - # https://api.slack.com/changelog/2016-05-17-changes-to-errors-for-incoming-webhooks - response.is_a?(Net::HTTPSuccess) - end - end + include SlackDelivery end end end diff --git a/lib/noticed/delivery_methods/webhook.rb b/lib/noticed/delivery_methods/webhook.rb index 6bb35e60..ab706d28 100644 --- a/lib/noticed/delivery_methods/webhook.rb +++ b/lib/noticed/delivery_methods/webhook.rb @@ -1,18 +1,7 @@ module Noticed module DeliveryMethods class Webhook < DeliveryMethod - required_options :url - - def deliver - post_request( - evaluate_option(:url), - basic_auth: evaluate_option(:basic_auth), - headers: evaluate_option(:headers), - json: evaluate_option(:json), - form: evaluate_option(:form), - body: evaluate_option(:body) - ) - end + include WebhookDelivery end end end diff --git a/lib/noticed/slack_delivery.rb b/lib/noticed/slack_delivery.rb new file mode 100644 index 00000000..b9b07f60 --- /dev/null +++ b/lib/noticed/slack_delivery.rb @@ -0,0 +1,42 @@ +module Noticed + # Shared between the individual and bulk Slack delivery methods + module SlackDelivery + extend ActiveSupport::Concern + + DEFAULT_URL = "https://slack.com/api/chat.postMessage" + + included do + required_options :json + end + + def deliver + headers = evaluate_option(:headers) + json = evaluate_option(:json) + response = post_request url, headers: headers, json: json + + if raise_if_not_ok? && !success?(response) + raise ResponseUnsuccessful.new(response, url, {headers: headers, json: json}) + end + + response + end + + def url + evaluate_option(:url) || DEFAULT_URL + end + + def raise_if_not_ok? + value = evaluate_option(:raise_if_not_ok) + value.nil? || value + end + + def success?(response) + if response.content_type == "application/json" + JSON.parse(response.body).dig("ok") + else + # https://api.slack.com/changelog/2016-05-17-changes-to-errors-for-incoming-webhooks + response.is_a?(Net::HTTPSuccess) + end + end + end +end diff --git a/lib/noticed/webhook_delivery.rb b/lib/noticed/webhook_delivery.rb new file mode 100644 index 00000000..a27735a2 --- /dev/null +++ b/lib/noticed/webhook_delivery.rb @@ -0,0 +1,21 @@ +module Noticed + # Shared between the individual and bulk Webhook delivery methods + module WebhookDelivery + extend ActiveSupport::Concern + + included do + required_options :url + end + + def deliver + post_request( + evaluate_option(:url), + basic_auth: evaluate_option(:basic_auth), + headers: evaluate_option(:headers), + json: evaluate_option(:json), + form: evaluate_option(:form), + body: evaluate_option(:body) + ) + end + end +end diff --git a/test/delivery_methods/action_push_native_test.rb b/test/delivery_methods/action_push_native_test.rb new file mode 100644 index 00000000..fc9c6064 --- /dev/null +++ b/test/delivery_methods/action_push_native_test.rb @@ -0,0 +1,107 @@ +require "test_helper" + +class ActionPushNativeTest < ActiveSupport::TestCase + # Mimics the ActionPushNative::Notification builder API + class FakePushNotification + class_attribute :deliveries, default: [] + + class Builder + attr_reader :options + + def initialize(options = {}) + @options = options + end + + def with_apple(value) + tap { options[:apple] = value } + end + + def with_google(value) + tap { options[:google] = value } + end + + def with_data(value) + tap { options[:data] = value } + end + + def new(**attributes) + FakePushNotification.new(options, attributes) + end + end + + def self.silent + Builder.new(silent: true) + end + + def self.with_apple(value) + Builder.new.with_apple(value) + end + + attr_reader :options, :attributes, :devices + + def initialize(options, attributes) + @options, @attributes = options, attributes + end + + def deliver_later_to(devices) + @devices = devices + deliveries << self + end + end + + setup do + FakePushNotification.deliveries = [] + @delivery_method = Noticed::DeliveryMethods::ActionPushNative.new + @delivery_method.instance_variable_set :@notification, noticed_notifications(:one) + end + + test "delivers to devices with format and platform options" do + set_config( + notification_class: "ActionPushNativeTest::FakePushNotification", + devices: [:device], + format: {title: "Hello", body: "World"}, + with_apple: {category: "observable"}, + with_google: {priority: "high"}, + with_data: {url: "/"} + ) + + @delivery_method.deliver + + delivery = FakePushNotification.deliveries.sole + assert_equal [:device], delivery.devices + assert_equal({title: "Hello", body: "World"}, delivery.attributes) + assert_equal({apple: {category: "observable"}, google: {priority: "high"}, data: {url: "/"}}, delivery.options) + end + + test "silent notifications" do + set_config( + notification_class: "ActionPushNativeTest::FakePushNotification", + devices: [:device], + format: {title: "Hello"}, + silent: true + ) + + @delivery_method.deliver + + assert FakePushNotification.deliveries.sole.options[:silent] + end + + test "notification_class can be evaluated from a lambda" do + set_config( + notification_class: -> { ActionPushNativeTest::FakePushNotification }, + devices: [:device], + format: {title: "Hello"} + ) + + assert_difference "FakePushNotification.deliveries.size" do + @delivery_method.deliver + end + end + + private + + def set_config(config) + # Real config is OrderedOptions, which leaves nested hashes alone (HashWithIndifferentAccess would stringify their keys) + @delivery_method.instance_variable_set :@config, ActiveSupport::OrderedOptions.new.merge(config) + end +end diff --git a/test/notifier_test.rb b/test/notifier_test.rb index 8c595ca1..1455c411 100644 --- a/test/notifier_test.rb +++ b/test/notifier_test.rb @@ -236,6 +236,20 @@ class RecipientsLambdaEphemeral < Noticed::Ephemeral assert SimpleNotifier::Notification.new.respond_to?(:inherited_method) end + test "recipients with an unknown method name raises" do + notifier = Class.new(Noticed::Event) { recipients :nope } + assert_raises(NoMethodError) { notifier.new.evaluate_recipients } + end + + test "duplicate delivery method names raise" do + assert_raises(ArgumentError) do + Class.new(Noticed::Event) do + deliver_by :test + deliver_by :test + end + end + end + test "with does not mutate the params hash" do params = {record: users(:one), message: "test"} RecordNotifier.with(params)