Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: Add should_notify option to Assigner#assign #604

Merged
merged 3 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion app/controllers/discourse_assign/assign_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
(
Expand All @@ -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
Expand Down
29 changes: 21 additions & 8 deletions lib/assigner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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 =
Expand All @@ -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 = {}
Expand Down Expand Up @@ -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?
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
30 changes: 29 additions & 1 deletion spec/lib/assigner_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -756,16 +762,38 @@ 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,
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)
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
Expand Down
56 changes: 56 additions & 0 deletions spec/requests/assign_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down