Code review cleanups

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.

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

app/models/concerns/noticed/deliverable.rb

@@ -18,15 +18,15 @@ 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?
         bulk_delivery_methods[name] = DeliverBy.new(name, config, bulk: true)
       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

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

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`.

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

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

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

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

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

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

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

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

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