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
This commit is contained in:
parent
215e1e3ab6
commit
0f4a1fcdd3
|
@ -4,6 +4,8 @@ module Jobs
|
||||||
class AssignNotification < ::Jobs::Base
|
class AssignNotification < ::Jobs::Base
|
||||||
def execute(args)
|
def execute(args)
|
||||||
raise Discourse::InvalidParameters.new(:assignment_id) if args[:assignment_id].nil?
|
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!
|
Assignment.find(args[:assignment_id]).create_missing_notifications!
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -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
|
||||||
|
#
|
|
@ -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
|
|
@ -322,6 +322,10 @@ class ::Assigner
|
||||||
|
|
||||||
first_post.publish_change_to_clients!(:revised, reload_topic: true)
|
first_post.publish_change_to_clients!(:revised, reload_topic: true)
|
||||||
queue_notification(assignment) if should_notify
|
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)
|
publish_assignment(assignment, assign_to, note, status)
|
||||||
|
|
||||||
if assignment.assigned_to_user?
|
if assignment.assigned_to_user?
|
||||||
|
|
|
@ -26,6 +26,12 @@ RSpec.describe Jobs::AssignNotification do
|
||||||
assignment.expects(:create_missing_notifications!)
|
assignment.expects(:create_missing_notifications!)
|
||||||
execute_job
|
execute_job
|
||||||
end
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -794,6 +794,10 @@ RSpec.describe Assigner do
|
||||||
assigner.assign(group, should_notify: false)
|
assigner.assign(group, should_notify: false)
|
||||||
expect(topic.allowed_groups).to include(group)
|
expect(topic.allowed_groups).to include(group)
|
||||||
expect(Notification.count).to eq(0)
|
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
|
end
|
||||||
|
|
||||||
it "doesn't invite group if all members have access to the PM already" do
|
it "doesn't invite group if all members have access to the PM already" do
|
||||||
|
|
|
@ -262,6 +262,7 @@ RSpec.describe DiscourseAssign::AssignController do
|
||||||
it "notifies the assignee when the topic is assigned to a group" do
|
it "notifies the assignee when the topic is assigned to a group" do
|
||||||
admins = Group[:admins]
|
admins = Group[:admins]
|
||||||
admins.messageable_level = Group::ALIAS_LEVELS[:everyone]
|
admins.messageable_level = Group::ALIAS_LEVELS[:everyone]
|
||||||
|
admins.assignable_level = Group::ALIAS_LEVELS[:everyone]
|
||||||
admins.save!
|
admins.save!
|
||||||
|
|
||||||
SiteSetting.invite_on_assign = true
|
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
|
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 = Group[:admins]
|
||||||
admins.messageable_level = Group::ALIAS_LEVELS[:everyone]
|
admins.messageable_level = Group::ALIAS_LEVELS[:everyone]
|
||||||
|
admins.assignable_level = Group::ALIAS_LEVELS[:everyone]
|
||||||
admins.save!
|
admins.save!
|
||||||
|
|
||||||
SiteSetting.invite_on_assign = true
|
SiteSetting.invite_on_assign = true
|
||||||
|
@ -313,6 +315,7 @@ RSpec.describe DiscourseAssign::AssignController do
|
||||||
should_notify: false,
|
should_notify: false,
|
||||||
}
|
}
|
||||||
expect(Notification.count).to eq(0)
|
expect(Notification.count).to eq(0)
|
||||||
|
expect(SilencedAssignment.count).to eq(1)
|
||||||
end
|
end
|
||||||
|
|
||||||
it "fails with a specific error message if the topic is not a PM and the assignee can not see it" do
|
it "fails with a specific error message if the topic is not a PM and the assignee can not see it" do
|
||||||
|
|
Loading…
Reference in New Issue