From c5b549b8878ad53727f17d976d52b9ad538730b7 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Wed, 27 Jan 2021 14:57:41 +0300 Subject: [PATCH] FIX: Improve error message when assigning a PM to a user who doesn't have access to the PM (#137) Currently if you try to assign a PM to a user who can't see it, you get an error message that says "@{user} can't be assigned since they don't belong to assigned allowed groups". This commit improves the message and now it tells you the user doesn't have access to the PM and that you need to invite the user before you can assign them. Signed-off-by: OsamaSayegh --- .../discourse_assign/assign_controller.rb | 4 +++ config/locales/server.en.yml | 16 +++++++----- lib/topic_assigner.rb | 11 ++++++-- spec/lib/topic_assigner_spec.rb | 4 +-- spec/requests/assign_controller_spec.rb | 26 +++++++++++++++++++ 5 files changed, 50 insertions(+), 11 deletions(-) diff --git a/app/controllers/discourse_assign/assign_controller.rb b/app/controllers/discourse_assign/assign_controller.rb index faf9fa8..9e9521c 100644 --- a/app/controllers/discourse_assign/assign_controller.rb +++ b/app/controllers/discourse_assign/assign_controller.rb @@ -167,6 +167,10 @@ module DiscourseAssign { error: I18n.t('discourse_assign.already_assigned', username: user.username) } when :forbidden_assign_to { error: I18n.t('discourse_assign.forbidden_assign_to', username: user.username) } + when :forbidden_assignee_not_pm_participant + { error: I18n.t('discourse_assign.forbidden_assignee_not_pm_participant', username: user.username) } + when :forbidden_assignee_cant_see_topic + { error: I18n.t('discourse_assign.forbidden_assignee_cant_see_topic', username: user.username) } else max = SiteSetting.max_assigned_topics { error: I18n.t('discourse_assign.too_many_assigns', username: user.username, max: max) } diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index beec70c..170acec 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -18,18 +18,20 @@ en: assigned_to: "Topic assigned to @%{username}" unassigned: "Topic was unassigned" already_claimed: "That topic has already been claimed." - already_assigned: 'Topic is already assigned to @%{username}' + already_assigned: "Topic is already assigned to @%{username}" too_many_assigns: "@%{username} has already reached the maximum number of assigned topics (%{max})." forbidden_assign_to: "@%{username} can't be assigned since they don't belong to assigned allowed groups." + forbidden_assignee_not_pm_participant: "@%{username} can't be assigned because they don't have access to this PM. You can grant @%{username} access by inviting them to this PM." + forbidden_assignee_cant_see_topic: "@%{username} can't be assigned because they don't have access to this topic." flag_assigned: "Sorry, that flag's topic is assigned to another user" flag_unclaimed: "You must claim that topic before acting on the flag" topic_assigned_excerpt: "assigned you the topic '%{title}'" reminders_frequency: - never: 'never' - daily: 'daily' - weekly: 'weekly' - monthly: 'monthly' - quarterly: 'quarterly' + never: "never" + daily: "daily" + weekly: "weekly" + monthly: "monthly" + quarterly: "quarterly" assign_mailer: title: "Assign Mailer" subject_template: "[%{email_prefix}] %{assignee_name} assigned you to '%{topic_title}'!" @@ -48,7 +50,7 @@ en: %{newest_assignments} %{oldest_assignments} - + This reminder will be sent %{frequency} if you have more than one assigned topic. newest: | ### Newest diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb index 4183403..6d9fb05 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -135,11 +135,18 @@ class ::TopicAssigner end def can_be_assigned?(assign_to) - return false unless allowed_user_ids.include?(assign_to.id) - Guardian.new(assign_to).can_see_topic?(@topic) + allowed_user_ids.include?(assign_to.id) + end + + def can_assignee_see_topic?(assignee) + Guardian.new(assignee).can_see_topic?(@topic) end def assign(assign_to, silent: false) + if !can_assignee_see_topic?(assign_to) + reason = @topic.private_message? ? :forbidden_assignee_not_pm_participant : :forbidden_assignee_cant_see_topic + return { success: false, reason: reason } + end return { success: false, reason: :forbidden_assign_to } unless can_be_assigned?(assign_to) return { success: false, reason: :already_assigned } if @topic.custom_fields && @topic.custom_fields[ASSIGNED_TO_ID] == assign_to.id.to_s return { success: false, reason: :too_many_assigns } unless can_assign_to?(assign_to) diff --git a/spec/lib/topic_assigner_spec.rb b/spec/lib/topic_assigner_spec.rb index bf72dd1..638f59e 100644 --- a/spec/lib/topic_assigner_spec.rb +++ b/spec/lib/topic_assigner_spec.rb @@ -195,14 +195,14 @@ RSpec.describe TopicAssigner do assign = TopicAssigner.new(pm, admin).assign(moderator) expect(assign[:success]).to eq(false) - expect(assign[:reason]).to eq(:forbidden_assign_to) + expect(assign[:reason]).to eq(:forbidden_assignee_not_pm_participant) end it 'fails to assign when the assigned user cannot view the topic' do assign = TopicAssigner.new(secure_topic, admin).assign(moderator) expect(assign[:success]).to eq(false) - expect(assign[:reason]).to eq(:forbidden_assign_to) + expect(assign[:reason]).to eq(:forbidden_assignee_cant_see_topic) end it "assigns the PM to the moderator when it's included in the list of allowed users" do diff --git a/spec/requests/assign_controller_spec.rb b/spec/requests/assign_controller_spec.rb index b040bc6..62bf77b 100644 --- a/spec/requests/assign_controller_spec.rb +++ b/spec/requests/assign_controller_spec.rb @@ -143,6 +143,32 @@ RSpec.describe DiscourseAssign::AssignController do I18n.t('discourse_assign.too_many_assigns', username: another_user.username, max: max_assigns) ) end + + it 'fails with a specific error message if the topic is a PM and the assignee can not see it' do + pm = Fabricate(:private_message_post, user: user).topic + another_user = Fabricate(:user) + add_to_assign_allowed_group(another_user) + put '/assign/assign.json', params: { + topic_id: pm.id, username: another_user.username + } + + expect(response.parsed_body['error']).to eq( + I18n.t('discourse_assign.forbidden_assignee_not_pm_participant', username: another_user.username) + ) + 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) + add_to_assign_allowed_group(another_user) + put '/assign/assign.json', params: { + topic_id: topic.id, username: another_user.username + } + + expect(response.parsed_body['error']).to eq( + I18n.t('discourse_assign.forbidden_assignee_cant_see_topic', username: another_user.username) + ) + end end context '#assigned' do