Merge branch 'fix_unassign_slash_command' into 'master'

Fix '/unassign' slash command

Closes #2470 and #2516

See merge request !1986

app/services/slash_commands/interpret_service.rb

@@ -92,48 +92,77 @@ module SlashCommands
 
     desc 'Assign'
     explanation do |users|
-      "Assigns #{users.map(&:to_reference).to_sentence}." if users.any?
+      users = issuable.is_a?(Issue) ? users : users.take(1)
+      "Assigns #{users.map(&:to_reference).to_sentence}."
+    end
+    params do
+      issuable.is_a?(Issue) ? '@user1 @user2' : '@user'
     end
-    params '@user'
     condition do
       current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
     parse_params do |assignee_param|
-      users = extract_references(assignee_param, :user)
-
-      if users.empty?
-        users = User.where(username: assignee_param.split(' ').map(&:strip))
-      end
-
-      users
+      extract_users(assignee_param)
     end
     command :assign do |users|
       next if users.empty?
 
       if issuable.is_a?(Issue)
-        @updates[:assignee_ids] = users.map(&:id)
+        # EE specific. In CE we should replace one assignee with another
+        @updates[:assignee_ids] = issuable.assignees.pluck(:id) + users.map(&:id)
       else
         @updates[:assignee_id] = users.last.id
       end
     end
 
-    desc 'Remove assignee'
+    desc do
+      if issuable.is_a?(Issue)
+        'Remove all or specific assignee(s)'
+      else
+        'Remove assignee'
+      end
+    end
     explanation do
-      "Removes assignee #{issuable.assignees.first.to_reference}."
+      "Removes #{'assignee'.pluralize(issuable.assignees.size)} #{issuable.assignees.map(&:to_reference).to_sentence}"
+    end
+    params do
+      issuable.is_a?(Issue) ? '@user1 @user2' : ''
     end
     condition do
       issuable.persisted? &&
         issuable.assignees.any? &&
         current_user.can?(:"admin_#{issuable.to_ability_name}", project)
     end
-    command :unassign do
+    command :unassign do |unassign_param = nil|
+      users = extract_users(unassign_param)
+
       if issuable.is_a?(Issue)
-        @updates[:assignee_ids] = []
+        @updates[:assignee_ids] =
+          if users.any?
+            issuable.assignees.pluck(:id) - users.map(&:id)
+          else
+            []
+          end
       else
         @updates[:assignee_id] = nil
       end
     end
 
+    desc 'Change assignee(s)'
+    explanation do
+      'Change assignee(s)'
+    end
+    params '@user1 @user2'
+    condition do
+      issuable.is_a?(Issue) &&
+        issuable.persisted? &&
+        issuable.assignees.any? &&
+        current_user.can?(:"admin_#{issuable.to_ability_name}", project)
+    end
+    command :reassign do |unassign_param|
+      @updates[:assignee_ids] = extract_users(unassign_param).map(&:id)
+    end
+
     desc 'Set milestone'
     explanation do |milestone|
       "Sets the milestone to #{milestone.to_reference}." if milestone
@@ -487,6 +516,18 @@ module SlashCommands
       end
     end
 
+    def extract_users(params)
+      return [] if params.nil?
+
+      users = extract_references(params, :user)
+
+      if users.empty?
+        users = User.where(username: params.split(' ').map(&:strip))
+      end
+
+      users
+    end
+
     def find_labels(labels_param)
       extract_references(labels_param, :label) |
         LabelsFinder.new(current_user, project_id: project.id, name: labels_param.split).execute

changelogs/unreleased-ee/fix_unassign_slash_command.yml

+---
+title: 'Fix: /unassign by default unassigns everyone. Implement /reassign command'
+merge_request:
+author:

doc/user/project/slash_commands.md

@@ -16,8 +16,9 @@ do.
 | `/reopen`                  | Reopen the issue or merge request |
 | `/merge`                   | Merge (when pipeline succeeds) |
 | `/title <New title>`       | Change title |
-| `/assign @username`        | Assign |
-| `/unassign`                | Remove assignee |
+| `/assign @user1 @user2 `   | Add assignee(s) |
+| `/reassign @user1 @user2 ` | Change assignee(s) |
+| `/unassign @user1 @user2`  | Remove all or specific assignee(s) |
 | `/milestone %milestone`    | Set milestone |
 | `/remove_milestone`        | Remove milestone |
 | `/label ~foo ~"bar baz"`   | Add label(s) |
@@ -39,3 +40,6 @@ do.
 | `/weight <1-9>` | Set the weight of the issue |
 | `/clear_weight` | Clears the issue weight |
 | `/board_move ~column`      | Move issue to column on the board |
+
+Note: In GitLab EES every issue can have more than one assignee, so commands `/assign`, `/unassign` and `/reassign`
+support multiple assignees.

lib/gitlab/slash_commands/command_definition.rb

@@ -48,17 +48,23 @@ module Gitlab
       end
 
       def to_h(opts)
+        context = OpenStruct.new(opts)
+
         desc = description
         if desc.respond_to?(:call)
-          context = OpenStruct.new(opts)
           desc = context.instance_exec(&desc) rescue ''
         end
 
+        prms = params
+        if prms.respond_to?(:call)
+          prms = Array(context.instance_exec(&prms)) rescue params
+        end
+
         {
           name: name,
           aliases: aliases,
           description: desc,
-          params: params
+          params: prms
         }
       end
 

lib/gitlab/slash_commands/dsl.rb

@@ -40,8 +40,8 @@ module Gitlab
         #   command :command_key do |arguments|
         #     # Awesome code block
         #   end
-        def params(*params)
-          @params = params
+        def params(*params, &block)
+          @params = block_given? ? block : params
         end
 
         # Allows to give an explanation of what the command will do when

spec/services/issuable/bulk_update_service_spec.rb

@@ -72,7 +72,7 @@ describe Issuable::BulkUpdateService, services: true do
     end
 
     context "when the new assignee ID is #{IssuableFinder::NONE}" do
-      it "unassigns the issues" do
+      it 'unassigns the issues' do
         expect { bulk_update(merge_request, assignee_id: IssuableFinder::NONE) }
           .to change { merge_request.reload.assignee }.to(nil)
       end

spec/services/notes/slash_commands_service_spec.rb

@@ -221,7 +221,7 @@ describe Notes::SlashCommandsService, services: true do
     end
   end
 
-  context 'CE restriction for issue assignees' do
+  context 'Issue assignees' do
     describe '/assign' do
       let(:project) { create(:empty_project) }
       let(:master) { create(:user).tap { |u| project.team << [u, :master] } }

spec/services/slash_commands/interpret_service_spec.rb

@@ -399,15 +399,21 @@ describe SlashCommands::InterpretService, services: true do
       let(:content) { "/assign @#{developer.username}" }
 
       context 'Issue' do
-        it 'fetches assignee and populates assignee_id if content contains /assign' do
+        it 'fetches assignees and populates them if content contains /assign' do
+          user = create(:user)
+          issue.assignees << user
+
           _, updates = service.execute(content, issue)
 
-          expect(updates).to eq(assignee_ids: [developer.id])
+          expect(updates[:assignee_ids]).to match_array([developer.id, user.id])
         end
       end
 
       context 'Merge Request' do
         it 'fetches assignee and populates assignee_id if content contains /assign' do
+          user = create(:user)
+          merge_request.update(assignee: user)
+
           _, updates = service.execute(content, merge_request)
 
           expect(updates).to eq(assignee_id: developer.id)
@@ -451,11 +457,46 @@ describe SlashCommands::InterpretService, services: true do
       let(:content) { '/unassign' }
 
       context 'Issue' do
-        it 'populates assignee_ids: [] if content contains /unassign' do
-          issue.update(assignee_ids: [developer.id])
-          _, updates = service.execute(content, issue)
+        it 'unassigns user if content contains /unassign @user' do
+          issue.update(assignee_ids: [developer.id, developer2.id])
+
+          _, updates = service.execute("/unassign @#{developer2.username}", issue)
+
+          expect(updates).to eq(assignee_ids: [developer.id])
+        end
+
+        it 'unassigns both users if content contains /unassign @user @user1' do
+          user = create(:user)
+
+          issue.update(assignee_ids: [developer.id, developer2.id, user.id])
+
+          _, updates = service.execute("/unassign @#{developer2.username} @#{developer.username}", issue)
+
+          expect(updates).to eq(assignee_ids: [user.id])
+        end
+
+        it 'unassigns all the users if content contains /unassign' do
+          issue.update(assignee_ids: [developer.id, developer2.id])
+
+          _, updates = service.execute('/unassign', issue)
+
+          expect(updates[:assignee_ids]).to be_empty
+        end
+      end
+
+      context 'reassign command' do
+        let(:content) { '/reassign' }
+
+        context 'Issue' do
+          it 'reassigns user if content contains /reassign @user' do
+            user = create(:user)
+
+            issue.update(assignee_ids: [developer.id, developer2.id])
+
+            _, updates = service.execute("/reassign @#{user.username}", issue)
 
-          expect(updates).to eq(assignee_ids: [])
+            expect(updates).to eq(assignee_ids: [user.id])
+          end
         end
       end
 
@@ -929,7 +970,7 @@ describe SlashCommands::InterpretService, services: true do
       it 'includes current assignee reference' do
         _, explanations = service.explain(content, issue)
 
-        expect(explanations).to eq(["Removes assignee @#{developer.username}."])
+        expect(explanations).to eq(["Removes assignee #{developer.to_reference}"])
       end
     end