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 <asooomaasoooma90@gmail.com>

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) }

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

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)

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

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