Merge branch '352418-quick-action-assign-groups' into 'master'

Ignore group members in user quick actions

See merge request gitlab-org/gitlab!80324

app/services/quick_actions/interpret_service.rb

@@ -69,27 +69,24 @@ module QuickActions
       Gitlab::QuickActions::Extractor.new(self.class.command_definitions)
     end
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def extract_users(params)
-      return [] if params.nil?
-
-      args = params.split(' ').uniq
-      users = extract_references(params, :user)
+      return [] if params.blank?
 
-      if users.empty?
-        users =
-          if params.strip == 'me'
-            [current_user]
-          else
-            User.where(username: args)
-          end
-      end
+      # We are using the a simple User.by_username query here rather than a ReferenceExtractor
+      # because the needs here are much simpler: we only deal in usernames, and
+      # want to also handle bare usernames. The ReferenceExtractor also has
+      # different behaviour, and will return all group members for groups named
+      # using a user-style reference, which is not in scope here.
+      args        = params.split(/\s|,/).select(&:present?).uniq - ['and']
+      usernames   = (args - ['me']).map { _1.delete_prefix('@') }
+      found       = User.by_username(usernames).to_a.select { can?(:read_user_profile, _1) }
+      found_names = found.map(&:username).to_set
+      missing     = args.reject { |arg| arg == 'me' || found_names.include?(arg.delete_prefix('@')) }.map { "'#{_1}'" }
 
-      failed_parse(format(_("Failed to find users for '%{params}'"), params: params)) unless users.size == args.size
+      failed_parse(format(_("Failed to find users for %{missing}"), missing: missing.to_sentence)) if missing.present?
 
-      users
+      found + [current_user].select { args.include?('me') }
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     def find_milestones(project, params = {})
       group_ids = project.group.self_and_ancestors.select(:id) if project.group
@@ -194,6 +191,10 @@ module QuickActions
         user: current_user
       )
     end
+
+    def can?(ability, object)
+      Ability.allowed?(current_user, ability, object)
+    end
   end
 end
 

ee/spec/services/ee/notes/quick_actions_service_spec.rb

@@ -273,8 +273,8 @@ RSpec.describe Notes::QuickActionsService do
 
     let(:multiline_assign_reviewer_text) do
       <<~HEREDOC
-      /assign_reviewer @#{user.username}
-      /assign_reviewer @#{reviewer.username})
+      /assign_reviewer #{user.to_reference}
+      /assign_reviewer #{reviewer.to_reference}
       HEREDOC
     end
 

ee/spec/services/quick_actions/interpret_service_spec.rb

@@ -132,6 +132,25 @@ RSpec.describe QuickActions::InterpretService do
           expect(updates[:assignee_ids]).to match_array([user.id, user2.id])
         end
 
+        context 'assign command with a group of users' do
+          let(:group) { create(:group) }
+          let(:project) { create(:project, group: group) }
+          let(:group_members) { create_list(:user, 3) }
+          let(:command) { "/assign #{group.to_reference}" }
+
+          before do
+            group_members.each { group.add_developer(_1) }
+          end
+
+          it 'ignores group members' do
+            merge_request.update!(assignee_ids: [user.id])
+
+            _, updates = service.execute(command, merge_request)
+
+            expect(updates[:assignee_ids]).to be_nil
+          end
+        end
+
         context 'assign command with multiple assignees' do
           it 'fetches assignee and populates assignee_ids if content contains /assign' do
             merge_request.update!(assignee_ids: [user.id])

locale/gitlab.pot

@@ -14921,7 +14921,7 @@ msgstr ""
 msgid "Failed to find import label for Jira import."
 msgstr ""
 
-msgid "Failed to find users for '%{params}'"
+msgid "Failed to find users for %{missing}"
 msgstr ""
 
 msgid "Failed to generate export, please try again later."

spec/services/quick_actions/interpret_service_spec.rb

@@ -11,13 +11,14 @@ RSpec.describe QuickActions::InterpretService do
   let_it_be(:developer2) { create(:user) }
   let_it_be(:developer3) { create(:user) }
   let_it_be_with_reload(:issue) { create(:issue, project: project) }
-  let(:milestone) { create(:milestone, project: project, title: '9.10') }
-  let(:commit) { create(:commit, project: project) }
   let_it_be(:inprogress) { create(:label, project: project, title: 'In Progress') }
   let_it_be(:helmchart) { create(:label, project: project, title: 'Helm Chart Registry') }
   let_it_be(:bug) { create(:label, project: project, title: 'Bug') }
 
-  let(:service) { described_class.new(project, developer) }
+  let(:milestone) { create(:milestone, project: project, title: '9.10') }
+  let(:commit) { create(:commit, project: project) }
+
+  subject(:service) { described_class.new(project, developer) }
 
   before_all do
     public_project.add_developer(developer)
@@ -970,6 +971,32 @@ RSpec.describe QuickActions::InterpretService do
         it_behaves_like 'assign_reviewer command'
       end
 
+      context 'with a private user' do
+        let(:ref) { create(:user, :unconfirmed).to_reference }
+        let(:content) { "/assign_reviewer #{ref}" }
+
+        it_behaves_like 'failed command', 'a parse error' do
+          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '#{ref}'." }
+        end
+      end
+
+      context 'with a private user, bare username' do
+        let(:ref) { create(:user, :unconfirmed).username }
+        let(:content) { "/assign_reviewer #{ref}" }
+
+        it_behaves_like 'failed command', 'a parse error' do
+          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '#{ref}'." }
+        end
+      end
+
+      context 'with @all' do
+        let(:content) { "/assign_reviewer @all" }
+
+        it_behaves_like 'failed command', 'a parse error' do
+          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '@all'." }
+        end
+      end
+
       context 'with an incorrect user' do
         let(:content) { '/assign_reviewer @abcd1234' }
 
@@ -997,11 +1024,10 @@ RSpec.describe QuickActions::InterpretService do
       end
 
       context 'with extra text' do
-        let(:arg) { "@#{developer.username} do it!" }
-        let(:content) { "/assign_reviewer #{arg}" }
+        let(:content) { "/assign_reviewer #{developer.to_reference} do it!" }
 
         it_behaves_like 'failed command', 'a parse error' do
-          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '#{arg}'." }
+          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for 'do' and 'it!'." }
         end
       end
     end
@@ -2451,7 +2477,8 @@ RSpec.describe QuickActions::InterpretService do
         it 'tells us why we cannot do that' do
           _, explanations = service.explain(content, merge_request)
 
-          expect(explanations).to eq ["Problem with assign command: Failed to find users for '#{arg}'."]
+          expect(explanations)
+            .to contain_exactly "Problem with assign command: Failed to find users for 'to', 'this', and 'issue'."
         end
       end
     end