From 17056ef8f321be0a39dfad7ceb822f99a2e944f0 Mon Sep 17 00:00:00 2001 From: adamsosterics Date: Wed, 11 Mar 2026 17:52:47 +0100 Subject: [PATCH 1/2] Check for subscriptions earlier This refactor will help us to add targets who are only subscribed for a specific notifiable instance. --- .../apis/notification_api.rb | 20 ++++++++++++------- .../models/concerns/target.rb | 5 ++++- spec/concerns/models/target_spec.rb | 2 +- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/lib/activity_notification/apis/notification_api.rb b/lib/activity_notification/apis/notification_api.rb index af91febb..1cdd2eef 100644 --- a/lib/activity_notification/apis/notification_api.rb +++ b/lib/activity_notification/apis/notification_api.rb @@ -230,10 +230,19 @@ def notify(target_type, notifiable, options = {}) if options[:notify_later] notify_later(target_type, notifiable, options) else - targets = notifiable.notification_targets(target_type, options[:pass_full_options] ? options : options[:key]) + potential_targets = notifiable.notification_targets(target_type, options[:pass_full_options] ? options : options[:key]) + subscribed_targets = [] + key = options[:key] || notifiable.default_notification_key + if potential_targets.respond_to?(:find_each) + potential_targets.find_each do |target| + subscribed_targets << target if target.subscribes_to_notification?(key) + end + else + subscribed_targets = potential_targets.select { |target| target.subscribes_to_notification?(key) } + end # Optimize blank check to avoid loading all records for ActiveRecord relations - unless targets_empty?(targets) - notify_all(targets, notifiable, options) + unless targets_empty?(subscribed_targets) + notify_all(subscribed_targets, notifiable, options) end end end @@ -413,10 +422,7 @@ def notify_later_to(target, notifiable, options = {}) # @option options [Hash] :parameters ({}) Additional parameters of the notifications def generate_notification(target, notifiable, options = {}) key = options[:key] || notifiable.default_notification_key - if target.subscribes_to_notification?(key) - # Store notification - notification = store_notification(target, notifiable, key, options) - end + store_notification(target, notifiable, key, options) end # Opens all notifications of the target. diff --git a/lib/activity_notification/models/concerns/target.rb b/lib/activity_notification/models/concerns/target.rb index 90302bfe..e035a6f3 100644 --- a/lib/activity_notification/models/concerns/target.rb +++ b/lib/activity_notification/models/concerns/target.rb @@ -377,7 +377,10 @@ def opened_notification_index(options = {}) # @option options [Hash] :optional_targets ({}) Options for optional targets, keys are optional target name (:amazon_sns or :slack etc) and values are options # @return [Notification] Generated notification instance def receive_notification_of(notifiable, options = {}) - Notification.notify_to(self, notifiable, options) + key = options[:key] || notifiable.default_notification_key + if self.subscribes_to_notification?(key) + Notification.notify_to(self, notifiable, options) + end end alias_method :receive_notification_now_of, :receive_notification_of diff --git a/spec/concerns/models/target_spec.rb b/spec/concerns/models/target_spec.rb index 34ecd522..2819a009 100644 --- a/spec/concerns/models/target_spec.rb +++ b/spec/concerns/models/target_spec.rb @@ -917,7 +917,7 @@ def custom_printable_name describe "#receive_notification_of" do it "is an alias of ActivityNotification::Notification.notify_to" do expect(ActivityNotification::Notification).to receive(:notify_to) - test_instance.receive_notification_of create(:user) + test_instance.receive_notification_of test_notifiable end end From 9bf78ee6735c0e72705e68f69d11fc471f3f90e1 Mon Sep 17 00:00:00 2001 From: adamsosterics Date: Fri, 13 Mar 2026 17:39:33 +0100 Subject: [PATCH 2/2] Proof-of-concept for notifiable instance subscriptions This is just a quick proof-of-concept for adding subscriptions to notifiable instances. --- lib/activity_notification/apis/notification_api.rb | 1 + lib/activity_notification/models/concerns/notifiable.rb | 4 ++++ lib/activity_notification/models/concerns/subscriber.rb | 2 +- lib/generators/templates/migrations/migration.rb | 2 ++ .../20181209000000_create_activity_notification_tables.rb | 1 + 5 files changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/activity_notification/apis/notification_api.rb b/lib/activity_notification/apis/notification_api.rb index 1cdd2eef..fbc35520 100644 --- a/lib/activity_notification/apis/notification_api.rb +++ b/lib/activity_notification/apis/notification_api.rb @@ -240,6 +240,7 @@ def notify(target_type, notifiable, options = {}) else subscribed_targets = potential_targets.select { |target| target.subscribes_to_notification?(key) } end + subscribed_targets.append(*notifiable.instance_subscription_targets(target_type)) # Optimize blank check to avoid loading all records for ActiveRecord relations unless targets_empty?(subscribed_targets) notify_all(subscribed_targets, notifiable, options) diff --git a/lib/activity_notification/models/concerns/notifiable.rb b/lib/activity_notification/models/concerns/notifiable.rb index 1e5a6e55..8e059a6a 100644 --- a/lib/activity_notification/models/concerns/notifiable.rb +++ b/lib/activity_notification/models/concerns/notifiable.rb @@ -86,6 +86,10 @@ def notification_targets(target_type, options = {}) resolved_parameter end + def instance_subscription_targets(target_type) + Subscription.where(target_type: target_type, notifiable_type: self.class.name, notifiable_id: self.id).map(&:target) + end + # Returns group unit of the notifications from configured field or overridden method. # This method is able to be overridden. # diff --git a/lib/activity_notification/models/concerns/subscriber.rb b/lib/activity_notification/models/concerns/subscriber.rb index c152057f..6b528dd9 100644 --- a/lib/activity_notification/models/concerns/subscriber.rb +++ b/lib/activity_notification/models/concerns/subscriber.rb @@ -149,7 +149,7 @@ def notification_keys(options = {}) # @param [Boolean] subscribe_as_default Default subscription value to use when the subscription record does not configured # @return [Boolean] If the target subscribes to the notification def _subscribes_to_notification?(key, subscribe_as_default = ActivityNotification.config.subscribe_as_default) - evaluate_subscription(subscriptions.where(key: key).first, :subscribing?, subscribe_as_default) + evaluate_subscription(subscriptions.where(key: key, notifiable_type: nil).first, :subscribing?, subscribe_as_default) end # Returns if the target subscribes to the notification email. diff --git a/lib/generators/templates/migrations/migration.rb b/lib/generators/templates/migrations/migration.rb index 27a54163..36079c3d 100644 --- a/lib/generators/templates/migrations/migration.rb +++ b/lib/generators/templates/migrations/migration.rb @@ -28,6 +28,7 @@ def change <% if @migration_tables.include?('subscriptions') %>create_table :subscriptions do |t| t.belongs_to :target, polymorphic: true, index: true, null: false + t.belongs_to :notifiable, polymorphic: true, index: true t.string :key, index: true, null: false t.boolean :subscribing, null: false, default: true t.boolean :subscribing_to_email, null: false, default: true @@ -41,6 +42,7 @@ def change end add_index :subscriptions, [:target_type, :target_id, :key], unique: true<% else %># create_table :subscriptions do |t| # t.belongs_to :target, polymorphic: true, index: true, null: false + # t.belongs_to :notifiable, polymorphic: true, index: true # t.string :key, index: true, null: false # t.boolean :subscribing, null: false, default: true # t.boolean :subscribing_to_email, null: false, default: true diff --git a/spec/rails_app/db/migrate/20181209000000_create_activity_notification_tables.rb b/spec/rails_app/db/migrate/20181209000000_create_activity_notification_tables.rb index ac83337d..23fa526c 100644 --- a/spec/rails_app/db/migrate/20181209000000_create_activity_notification_tables.rb +++ b/spec/rails_app/db/migrate/20181209000000_create_activity_notification_tables.rb @@ -17,6 +17,7 @@ def change create_table :subscriptions do |t| t.belongs_to :target, polymorphic: true, index: true, null: false + t.belongs_to :notifiable, polymorphic: true, index: true t.string :key, index: true, null: false t.boolean :subscribing, null: false, default: true t.boolean :subscribing_to_email, null: false, default: true