FIX: Improve errors when invite to topic fails (#11133)

It used to simply say "not allowed" without giving any hint what the
problem could be. This commit refactors the code and tries to improve
readability.

app/models/topic.rb

@@ -1031,12 +1031,40 @@ class Topic < ActiveRecord::Base
         raise UserExists.new(I18n.t("topic_invite.user_exists"))
       end
 
-      if invite_existing_muted?(target_user, invited_by)
+      if MutedUser
-        raise NotAllowed.new(I18n.t("topic_invite.not_allowed"))
+          .where(user: target_user, muted_user: invited_by)
+          .joins(:muted_user)
+          .where('NOT admin AND NOT moderator')
+          .exists?
+        raise NotAllowed.new(I18n.t("topic_invite.muted_invitee"))
       end
 
-      if !allowed_pm_user?(target_user, invited_by)
+      if TopicUser
-        raise NotAllowed.new(I18n.t("topic_invite.not_allowed"))
+          .where(topic: self,
+                 user: target_user,
+                 notification_level: TopicUser.notification_levels[:muted])
+          .exists?
+        raise NotAllowed.new(I18n.t("topic_invite.muted_topic"))
+      end
+
+      if !target_user.staff? &&
+         target_user&.user_option&.enable_allowed_pm_users &&
+         !AllowedPmUser.where(user: target_user, allowed_pm_user: invited_by).exists?
+        raise NotAllowed.new(I18n.t("topic_invite.receiver_does_not_allow_pm"))
+      end
+
+      if !target_user.staff? &&
+         invited_by&.user_option&.enable_allowed_pm_users &&
+         !AllowedPmUser.where(user: invited_by, allowed_pm_user: target_user).exists?
+        raise NotAllowed.new(I18n.t("topic_invite.sender_does_not_allow_pm"))
+      end
+
+      if !target_user.staff? && target_user&.user_option&.enable_allowed_pm_users
+        topic_users = self.topic_allowed_users.pluck(:user_id)
+        allowed_users = AllowedPmUser.where(user: target_user.id, allowed_pm_user_id: topic_users)
+        if (allowed_users - topic_users).size > 0
+          raise NotAllowed.new(I18n.t("topic_invite.receiver_does_not_allow_other_user_pm"))
+        end
       end
 
       if private_message?
@@ -1051,63 +1079,6 @@ class Topic < ActiveRecord::Base
     end
   end
 
-  def invite_existing_muted?(target_user, invited_by)
-    if invited_by.id &&
-       MutedUser.where(user_id: target_user.id, muted_user_id: invited_by.id)
-           .joins(:muted_user)
-           .where('NOT admin AND NOT moderator')
-           .exists?
-      return true
-    end
-
-    if TopicUser.where(
-         topic: self,
-         user: target_user,
-         notification_level: TopicUser.notification_levels[:muted]
-        ).exists?
-      return true
-    end
-
-    false
-  end
-
-  def allowed_pm_user?(target_user, invited_by)
-    return true if target_user.staff?
-    users = TopicAllowedUser.where(topic: self).pluck(:user_id)
-    users << target_user.id << invited_by.id
-    users.uniq
-
-    users_with_allowed_pms = allowed_pms_enabled(users).pluck(:id).uniq
-
-    if users_with_allowed_pms.any?
-      users_sender_can_pm = allowed_pms_enabled(users)
-        .where("allowed_pm_users.allowed_pm_user_id" => invited_by.id)
-        .pluck(:id).uniq
-
-      return false unless users_sender_can_pm.include? target_user.id
-
-      can_users_receive_from_sender = allowed_pms_enabled([invited_by.id])
-        .where("allowed_pm_users.allowed_pm_user_id IN (:user_ids)", user_ids: users.delete(invited_by.id))
-        .pluck(:id).uniq
-
-      users_not_allowed = users_with_allowed_pms - users_sender_can_pm - can_users_receive_from_sender
-      return false if users_not_allowed.any?
-    end
-
-    true
-  end
-
-  def allowed_pms_enabled(user_ids)
-    User
-      .joins("LEFT JOIN user_options ON user_options.user_id = users.id")
-      .joins("LEFT JOIN allowed_pm_users ON allowed_pm_users.user_id = users.id")
-      .where("
-        user_options.user_id IS NOT NULL AND
-        user_options.user_id IN (:user_ids) AND
-        user_options.enable_allowed_pm_users
-      ", user_ids: user_ids)
-  end
-
   def email_already_exists_for?(invite)
     invite.email_already_exists && private_message?
   end

config/locales/server.en.yml

@@ -238,7 +238,11 @@ en:
   topic_invite:
     failed_to_invite: "The user cannot be invited into this topic without a group membership in either one of the following groups: %{group_names}."
     user_exists: "Sorry, that user has already been invited. You may only invite a user to a topic once."
-    not_allowed: "Sorry, that user can't be invited."
+    muted_invitee: "Sorry, that user muted you."
+    muted_topic: "Sorry, that user muted this topic."
+    receiver_does_not_allow_pm: "Sorry, that user does not allow you to send them private messages."
+    sender_does_not_allow_pm: "Sorry, you do not allow that user to send you private messages."
+    receiver_does_not_allow_other_user_pm: "Sorry, that user does not allow an existing participant to send them private messages."
 
   backup:
     operation_already_running: "An operation is currently running. Can't start a new job right now."

spec/models/topic_spec.rb

@@ -694,6 +694,7 @@ describe Topic do
           it 'fails with an error message' do
             expect { topic.invite(user, another_user.username) }
               .to raise_error(Topic::NotAllowed)
+              .with_message(I18n.t("topic_invite.muted_invitee"))
             expect(topic.allowed_users).to_not include(another_user)
             expect(Post.last).to be_blank
             expect(Notification.last).to be_blank
@@ -729,6 +730,7 @@ describe Topic do
             AllowedPmUser.create!(user: another_user, allowed_pm_user: user2)
             expect { topic.invite(user, another_user.username) }
               .to raise_error(Topic::NotAllowed)
+              .with_message(I18n.t("topic_invite.receiver_does_not_allow_pm"))
           end
 
           it 'should succeed for staff even when not allowed' do
@@ -743,14 +745,16 @@ describe Topic do
             AllowedPmUser.create!(user: another_user, allowed_pm_user: user)
             expect { topic.invite(user, another_user.username) }
               .to raise_error(Topic::NotAllowed)
+              .with_message(I18n.t("topic_invite.sender_does_not_allow_pm"))
           end
 
           it 'should raise error if target_user has not allowed any of the other participants' do
-            user2.user_option.update!(enable_allowed_pm_users: true)
+            another_user.user_option.update!(enable_allowed_pm_users: true)
-            AllowedPmUser.create!(user: user2, allowed_pm_user: user)
+            AllowedPmUser.create!(user: another_user, allowed_pm_user: user)
 
             expect { pm.invite(user, another_user.username) }
               .to raise_error(Topic::NotAllowed)
+              .with_message(I18n.t("topic_invite.receiver_does_not_allow_other_user_pm"))
           end
         end
       end