From 0f4a1fcdd3214fc7e86929e292f6269a52207d8a Mon Sep 17 00:00:00 2001 From: Gabriel Grubba <70247653+Grubba27@users.noreply.github.com> Date: Thu, 12 Dec 2024 11:28:11 -0300 Subject: [PATCH] FIX: Not send notifications when it should never notify assignment (#620) * FIX: Not send notifications when it should never notify assignment - Added silenced_assignments table and migration to store silenced assignments - Added tests for silenced assignments * DEV: Add annotation to model * DEV: add empty line after early return * DEV: lint file --- app/jobs/regular/assign_notification.rb | 2 ++ app/models/silenced_assignment.rb | 19 +++++++++++++++++++ ...41212113000_create_silenced_assignments.rb | 14 ++++++++++++++ lib/assigner.rb | 4 ++++ spec/jobs/regular/assign_notification_spec.rb | 6 ++++++ spec/lib/assigner_spec.rb | 4 ++++ spec/requests/assign_controller_spec.rb | 3 +++ 7 files changed, 52 insertions(+) create mode 100644 app/models/silenced_assignment.rb create mode 100644 db/migrate/20241212113000_create_silenced_assignments.rb diff --git a/app/jobs/regular/assign_notification.rb b/app/jobs/regular/assign_notification.rb index 2bae14d..8f0eed9 100644 --- a/app/jobs/regular/assign_notification.rb +++ b/app/jobs/regular/assign_notification.rb @@ -4,6 +4,8 @@ module Jobs class AssignNotification < ::Jobs::Base def execute(args) raise Discourse::InvalidParameters.new(:assignment_id) if args[:assignment_id].nil? + return if SilencedAssignment.exists?(assignment_id: args[:assignment_id]) + Assignment.find(args[:assignment_id]).create_missing_notifications! end end diff --git a/app/models/silenced_assignment.rb b/app/models/silenced_assignment.rb new file mode 100644 index 0000000..5389d9f --- /dev/null +++ b/app/models/silenced_assignment.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class SilencedAssignment < ActiveRecord::Base + belongs_to :assignment +end + +# == Schema Information +# +# Table name: silenced_assignments +# +# id :bigint not null, primary key +# assignment_id :bigint not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_silenced_assignments_on_assignment_id (assignment_id) UNIQUE +# diff --git a/db/migrate/20241212113000_create_silenced_assignments.rb b/db/migrate/20241212113000_create_silenced_assignments.rb new file mode 100644 index 0000000..8c0129e --- /dev/null +++ b/db/migrate/20241212113000_create_silenced_assignments.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true +class CreateSilencedAssignments < ActiveRecord::Migration[7.2] + def up + create_table :silenced_assignments do |t| + t.bigint :assignment_id, null: false + t.timestamps + end + add_index :silenced_assignments, :assignment_id, unique: true + end + + def down + drop_table :silenced_assignments + end +end diff --git a/lib/assigner.rb b/lib/assigner.rb index 33cbbd1..a0e1786 100644 --- a/lib/assigner.rb +++ b/lib/assigner.rb @@ -322,6 +322,10 @@ class ::Assigner first_post.publish_change_to_clients!(:revised, reload_topic: true) queue_notification(assignment) if should_notify + + # This assignment should never be notified + SilencedAssignment.create!(assignment_id: assignment.id) if !should_notify + publish_assignment(assignment, assign_to, note, status) if assignment.assigned_to_user? diff --git a/spec/jobs/regular/assign_notification_spec.rb b/spec/jobs/regular/assign_notification_spec.rb index 4ecbd57..8231cbd 100644 --- a/spec/jobs/regular/assign_notification_spec.rb +++ b/spec/jobs/regular/assign_notification_spec.rb @@ -26,6 +26,12 @@ RSpec.describe Jobs::AssignNotification do assignment.expects(:create_missing_notifications!) execute_job end + + it "does not create notifications if assignment is silenced" do + SilencedAssignment.stubs(:exists?).with(assignment_id: assignment_id).returns(true) + assignment.expects(:create_missing_notifications!).never + execute_job + end end end end diff --git a/spec/lib/assigner_spec.rb b/spec/lib/assigner_spec.rb index b1cb4ae..63b43d2 100644 --- a/spec/lib/assigner_spec.rb +++ b/spec/lib/assigner_spec.rb @@ -794,6 +794,10 @@ RSpec.describe Assigner do assigner.assign(group, should_notify: false) expect(topic.allowed_groups).to include(group) expect(Notification.count).to eq(0) + expect(SilencedAssignment.count).to eq(1) + + group.add(Fabricate(:user)) + expect(Notification.count).to eq(0) # no one is ever notified about this assignment end it "doesn't invite group if all members have access to the PM already" do diff --git a/spec/requests/assign_controller_spec.rb b/spec/requests/assign_controller_spec.rb index 3b889e7..bd1aa31 100644 --- a/spec/requests/assign_controller_spec.rb +++ b/spec/requests/assign_controller_spec.rb @@ -262,6 +262,7 @@ RSpec.describe DiscourseAssign::AssignController do it "notifies the assignee when the topic is assigned to a group" do admins = Group[:admins] admins.messageable_level = Group::ALIAS_LEVELS[:everyone] + admins.assignable_level = Group::ALIAS_LEVELS[:everyone] admins.save! SiteSetting.invite_on_assign = true @@ -290,6 +291,7 @@ RSpec.describe DiscourseAssign::AssignController do it "does not notify the assignee when the topic is assigned to a group if should_notify option is set to false" do admins = Group[:admins] admins.messageable_level = Group::ALIAS_LEVELS[:everyone] + admins.assignable_level = Group::ALIAS_LEVELS[:everyone] admins.save! SiteSetting.invite_on_assign = true @@ -313,6 +315,7 @@ RSpec.describe DiscourseAssign::AssignController do should_notify: false, } expect(Notification.count).to eq(0) + expect(SilencedAssignment.count).to eq(1) end it "fails with a specific error message if the topic is not a PM and the assignee can not see it" do