From 6101ecdba54497cc19ca85bc6278543d01eab490 Mon Sep 17 00:00:00 2001 From: Gabriel Grubba <70247653+Grubba27@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:01:02 -0300 Subject: [PATCH] FEATURE: Add `should_notify` option to `Assigner#assign` (#604) * FEATURE: Add `should_notify` option to `Assigner#assign` This option let's you control whether the added user within a group assignment should be notified or not. * DEV: stree assign_controller file * Update app/controllers/discourse_assign/assign_controller.rb Co-authored-by: David Taylor --------- Co-authored-by: David Taylor --- .../discourse_assign/assign_controller.rb | 10 +++- lib/assigner.rb | 29 +++++++--- spec/lib/assigner_spec.rb | 30 +++++++++- spec/requests/assign_controller_spec.rb | 56 +++++++++++++++++++ 4 files changed, 115 insertions(+), 10 deletions(-) diff --git a/app/controllers/discourse_assign/assign_controller.rb b/app/controllers/discourse_assign/assign_controller.rb index 63926568..31a776b3 100644 --- a/app/controllers/discourse_assign/assign_controller.rb +++ b/app/controllers/discourse_assign/assign_controller.rb @@ -43,6 +43,8 @@ def assign group_name = params.permit(:group_name)["group_name"] note = params.permit(:note)["note"].presence status = params.permit(:status)["status"].presence + should_notify = params.permit(:should_notify)["should_notify"] + should_notify = (should_notify.present? ? should_notify.to_s == "true" : true) assign_to = ( @@ -58,7 +60,13 @@ def assign target = target_type.constantize.where(id: target_id).first raise Discourse::NotFound unless target - assign = Assigner.new(target, current_user).assign(assign_to, note: note, status: status) + assign = + Assigner.new(target, current_user).assign( + assign_to, + note: note, + status: status, + should_notify: should_notify, + ) if assign[:success] render json: success_json diff --git a/lib/assigner.rb b/lib/assigner.rb index a0343849..33cbbd19 100644 --- a/lib/assigner.rb +++ b/lib/assigner.rb @@ -221,7 +221,7 @@ def forbidden_reasons(assign_to:, type:, note:, status:, allow_self_reassign:) end end - def update_details(assign_to, note, status, skip_small_action_post: false) + def update_details(assign_to, note, status, skip_small_action_post: false, should_notify: true) case when note.present? && status.present? && @target.assignment.note != note && @target.assignment.status != status @@ -240,7 +240,7 @@ def update_details(assign_to, note, status, skip_small_action_post: false) end @target.assignment.update!(note: note, status: status) - queue_notification(@target.assignment) + queue_notification(@target.assignment) if should_notify publish_assignment(@target.assignment, assign_to, note, status) # email is skipped, for now @@ -258,12 +258,17 @@ def assign( note: nil, skip_small_action_post: false, status: nil, - allow_self_reassign: false + allow_self_reassign: false, + should_notify: true ) assigned_to_type = assign_to.is_a?(User) ? "User" : "Group" if topic.private_message? && SiteSetting.invite_on_assign - assigned_to_type == "Group" ? invite_group(assign_to) : invite_user(assign_to) + if assigned_to_type == "Group" + invite_group(assign_to, should_notify) + else + invite_user(assign_to) + end end forbidden_reason = @@ -277,7 +282,15 @@ def assign( return { success: false, reason: forbidden_reason } if forbidden_reason if no_assignee_change?(assign_to) && details_change?(note, status) - return update_details(assign_to, note, status, skip_small_action_post: skip_small_action_post) + return( + update_details( + assign_to, + note, + status, + skip_small_action_post: skip_small_action_post, + should_notify: should_notify, + ) + ) end action_code = {} @@ -308,7 +321,7 @@ def assign( ) first_post.publish_change_to_clients!(:revised, reload_topic: true) - queue_notification(assignment) + queue_notification(assignment) if should_notify publish_assignment(assignment, assign_to, note, status) if assignment.assigned_to_user? @@ -452,7 +465,7 @@ def invite_user(user) topic.invite(@assigned_by, user.username) end - def invite_group(group) + def invite_group(group, should_notify) return if topic.topic_allowed_groups.exists?(group_id: group.id) if topic .all_allowed_users @@ -463,7 +476,7 @@ def invite_group(group) end guardian.ensure_can_invite_group_to_private_message!(group, topic) - topic.invite_group(@assigned_by, group) + topic.invite_group(@assigned_by, group, should_notify: should_notify) end def guardian diff --git a/spec/lib/assigner_spec.rb b/spec/lib/assigner_spec.rb index cb2db1e5..b1cb4ae3 100644 --- a/spec/lib/assigner_spec.rb +++ b/spec/lib/assigner_spec.rb @@ -465,11 +465,17 @@ def assigned_to?(assignee) it "queues notification" do assigner.assign(moderator) + expect(job_enqueued?(job: :assign_notification)).to eq(true) expect_enqueued_with(job: :assign_notification) do assigner.assign(moderator, status: "Done") end end + it "does not queue notification if should_notify is set to false" do + assigner.assign(moderator, status: "Done", should_notify: false) + expect(job_enqueued?(job: :assign_notification)).to eq(false) + end + it "publishes topic assignment with note" do assigner.assign(moderator) @@ -756,7 +762,7 @@ def assigned_to?(assignee) expect(topic.allowed_users).not_to include(user) end - it "invites group to the PM" do + it "invites group to the PM and notifies users" do group = Fabricate( :group, @@ -764,8 +770,30 @@ def assigned_to?(assignee) messageable_level: Group::ALIAS_LEVELS[:only_admins], ) group.add(Fabricate(:user)) + + Notification.delete_all + Jobs.run_immediately! + assigner.assign(group) expect(topic.allowed_groups).to include(group) + expect(Notification.count).to be > 0 + end + + it "invites group to the PM and does not notifies users if should_notify is false" do + group = + Fabricate( + :group, + assignable_level: Group::ALIAS_LEVELS[:only_admins], + messageable_level: Group::ALIAS_LEVELS[:only_admins], + ) + group.add(Fabricate(:user)) + + Notification.delete_all + Jobs.run_immediately! + + assigner.assign(group, should_notify: false) + expect(topic.allowed_groups).to include(group) + expect(Notification.count).to eq(0) 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 d7704414..3b889e7b 100644 --- a/spec/requests/assign_controller_spec.rb +++ b/spec/requests/assign_controller_spec.rb @@ -259,6 +259,62 @@ def assign_user_to_post ) end + it "notifies the assignee when the topic is assigned to a group" do + admins = Group[:admins] + admins.messageable_level = Group::ALIAS_LEVELS[:everyone] + admins.save! + + SiteSetting.invite_on_assign = true + pm = Fabricate(:private_message_post, user: admin).topic + + another_user = Fabricate(:user) + admins.add(another_user) + admins + .group_users + .find_by(user_id: another_user.id) + .update!(notification_level: NotificationLevels.all[:watching]) + + Notification.delete_all + Jobs.run_immediately! + + put "/assign/assign.json", + params: { + target_id: pm.id, + target_type: "Topic", + group_name: admins.name, + } + + expect(Notification.count).to be > 0 + end + + 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.save! + + SiteSetting.invite_on_assign = true + pm = Fabricate(:private_message_post, user: admin).topic + + another_user = Fabricate(:user) + admins.add(another_user) + admins + .group_users + .find_by(user_id: another_user.id) + .update!(notification_level: NotificationLevels.all[:watching]) + + Notification.delete_all + Jobs.run_immediately! + + put "/assign/assign.json", + params: { + target_id: pm.id, + target_type: "Topic", + group_name: admins.name, + should_notify: false, + } + expect(Notification.count).to eq(0) + end + it "fails with a specific error message if the topic is not a PM and the assignee can not see it" do topic = Fabricate(:topic, category: Fabricate(:private_category, group: Fabricate(:group))) another_user = Fabricate(:user)