From 4c68ff08bb2c48f85a3f3a79db14ec7402a8ba69 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 18 Aug 2017 13:36:51 -0700 Subject: [PATCH 1/6] add a spec for a participant with a :custom setting where that custom setting is not enabled. --- spec/services/notification_service_spec.rb | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 44b2d28d1d4..5223152ce66 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -130,7 +130,18 @@ describe NotificationService, :mailer do project.add_master(issue.author) project.add_master(assignee) project.add_master(note.author) - create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@subscribed_participant cc this guy') + + @u_custom_off = create_user_with_notification(:custom, 'custom_off') + project.add_guest(@u_custom_off) + + create( + :note_on_issue, + author: @u_custom_off, + noteable: issue, + project_id: issue.project_id, + note: 'i think @subscribed_participant should see this' + ) + update_custom_notification(:new_note, @u_guest_custom, resource: project) update_custom_notification(:new_note, @u_custom_global) end @@ -141,7 +152,7 @@ describe NotificationService, :mailer do reset_delivered_emails! # Ensure create SentNotification by noteable = issue 6 times, not noteable = note - expect(SentNotification).to receive(:record).with(issue, any_args).exactly(8).times + expect(SentNotification).to receive(:record).with(issue, any_args).exactly(9).times notification.new_note(note) @@ -153,6 +164,7 @@ describe NotificationService, :mailer do should_email(@subscriber) should_email(@watcher_and_subscriber) should_email(@subscribed_participant) + should_email(@u_custom_off) should_not_email(@u_guest_custom) should_not_email(@u_guest_watcher) should_not_email(note.author) From 7059af29800da478edbe04f4edf63fd7698909f5 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 18 Aug 2017 13:39:12 -0700 Subject: [PATCH 2/6] actually use the :participating notification type --- app/services/notification_recipient_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 21c9c314a2a..c9f07c140f7 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -95,7 +95,7 @@ module NotificationRecipientService def add_participants(user) return unless target.respond_to?(:participants) - self << [target.participants(user), :watch] + self << [target.participants(user), :participating] end # Get project/group users with CUSTOM notification level From 3676275a5a07492bb24a8b1925f7221c375a2f42 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 18 Aug 2017 13:40:09 -0700 Subject: [PATCH 3/6] don't rely on order of notification levels factor out #suitable_notification_level? and check manually by notification level. this makes the notification logic clear and actually reflect what is in the documentation as to what should happen with each setting. --- app/models/notification_recipient.rb | 53 ++++++++++++++-------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb index dc862565a71..183e098d819 100644 --- a/app/models/notification_recipient.rb +++ b/app/models/notification_recipient.rb @@ -27,46 +27,45 @@ class NotificationRecipient @notification_setting ||= find_notification_setting end - def raw_notification_level - notification_setting&.level&.to_sym - end - def notification_level - # custom is treated the same as watch if it's enabled - otherwise it's - # set to :custom, meaning to send exactly when our type is :participating - # or :mention. - @notification_level ||= - case raw_notification_level - when :custom - if @custom_action && notification_setting&.event_enabled?(@custom_action) - :watch - else - :custom - end - else - raw_notification_level - end + @notification_level ||= notification_setting&.level&.to_sym end def notifiable? return false unless has_access? return false if own_activity? - return true if @type == :subscription + # even users with :disabled notifications receive manual subscriptions + return !unsubscribed? if @type == :subscription - return false if notification_level.nil? || notification_level == :disabled - - return %i[participating mention].include?(@type) if notification_level == :custom - - return false if %i[watch participating].include?(notification_level) && excluded_watcher_action? - - return false unless NotificationSetting.levels[notification_level] <= NotificationSetting.levels[@type] + return false unless suitable_notification_level? + # check this last because it's expensive + # nobody should receive notifications if they've specifically unsubscribed return false if unsubscribed? true end + def suitable_notification_level? + case notification_level + when :disabled, nil + false + when :custom + custom_enabled? || %i[participating mention].include?(@type) + when :watch, :participating + !excluded_watcher_action? + when :mention + @type == :mention + else + false + end + end + + def custom_enabled? + @custom_action && notification_setting&.event_enabled?(@custom_action) + end + def unsubscribed? return false unless @target return false unless @target.respond_to?(:subscriptions) @@ -98,7 +97,7 @@ class NotificationRecipient def excluded_watcher_action? return false unless @custom_action - return false if raw_notification_level == :custom + return false if notification_level == :custom NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(@custom_action) end From 57fb2d4f8f0c9f5dc563bdb0eb6935748674a299 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Mon, 21 Aug 2017 13:47:15 -0700 Subject: [PATCH 4/6] rm a comment that is a lie --- spec/services/notification_service_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 5223152ce66..5b349017c8b 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -151,7 +151,6 @@ describe NotificationService, :mailer do add_users_with_subscription(note.project, issue) reset_delivered_emails! - # Ensure create SentNotification by noteable = issue 6 times, not noteable = note expect(SentNotification).to receive(:record).with(issue, any_args).exactly(9).times notification.new_note(note) From bf7f23531d78c02836decae88895609b14a2980f Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Mon, 21 Aug 2017 14:19:53 -0700 Subject: [PATCH 5/6] add a changelog --- changelogs/unreleased/bugfix-notify-custom-participants.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/bugfix-notify-custom-participants.yml diff --git a/changelogs/unreleased/bugfix-notify-custom-participants.yml b/changelogs/unreleased/bugfix-notify-custom-participants.yml new file mode 100644 index 00000000000..32ba1076b09 --- /dev/null +++ b/changelogs/unreleased/bugfix-notify-custom-participants.yml @@ -0,0 +1,5 @@ +--- +title: 13680-notify-custom-participants +merge_request: 13680 +author: jneen +type: fixed From 77c01b22d7e21cf20592e3d6d6f05a7beefc118c Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Tue, 22 Aug 2017 08:39:31 -0700 Subject: [PATCH 6/6] add a description to the changelog --- changelogs/unreleased/bugfix-notify-custom-participants.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/bugfix-notify-custom-participants.yml b/changelogs/unreleased/bugfix-notify-custom-participants.yml index 32ba1076b09..04fcb95e18a 100644 --- a/changelogs/unreleased/bugfix-notify-custom-participants.yml +++ b/changelogs/unreleased/bugfix-notify-custom-participants.yml @@ -1,5 +1,5 @@ --- -title: 13680-notify-custom-participants +title: Fixed: Notifications weren't sending to participating users with a `Custom` notification setting. merge_request: 13680 author: jneen type: fixed