Merge branch '195989-quick-action-add-dup-assignee-error' into 'master'

Fixes error when assigning an existing assignee

See merge request gitlab-org/gitlab!23416

app/models/issue_assignee.rb

@@ -3,6 +3,8 @@
 class IssueAssignee < ApplicationRecord
   belongs_to :issue
   belongs_to :assignee, class_name: "User", foreign_key: :user_id
+
+  validates :assignee, uniqueness: { scope: :issue_id }
 end
 
 IssueAssignee.prepend_if_ee('EE::IssueAssignee')

app/models/merge_request_assignee.rb

@@ -3,4 +3,6 @@
 class MergeRequestAssignee < ApplicationRecord
   belongs_to :merge_request
   belongs_to :assignee, class_name: "User", foreign_key: :user_id
+
+  validates :assignee, uniqueness: { scope: :merge_request_id }
 end

changelogs/unreleased/195989-quick-action-add-dup-assignee-error.yml

+---
+title: Fix error when assigning an existing asignee
+merge_request: 23416
+author:
+type: fixed

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

@@ -5,6 +5,7 @@ describe Notes::QuickActionsService do
   let(:group)   { create(:group) }
   let(:project) { create(:project, group: group) }
   let(:user) { create(:user) }
+  let(:assignee) { create(:user) }
   let(:issue) { create(:issue, project: project) }
   let(:epic) { create(:epic, group: group)}
 
@@ -232,29 +233,82 @@ describe Notes::QuickActionsService do
     end
   end
 
-  context 'Issue assignees' do
   describe '/assign' do
-      let(:project) { create(:project) }
-      let(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } }
-      let(:assignee) { create(:user) }
-      let(:service) { described_class.new(project, maintainer) }
-      let(:note) { create(:note_on_issue, note: note_text, project: project) }
-
-      let(:note_text) do
-        %(/assign @#{assignee.username} @#{maintainer.username}\n")
-      end
+    let(:note_text) { %(/assign @#{user.username} @#{assignee.username}\n) }
+    let(:multiline_assign_note_text) { %(/assign @#{user.username}\n/assign @#{assignee.username}) }
 
     before do
-        project.add_maintainer(maintainer)
       project.add_maintainer(assignee)
+      project.add_maintainer(user)
     end
 
+    context 'Issue assignees' do
+      let(:note) { create(:note_on_issue, note: note_text, project: project) }
+
       it 'adds multiple assignees from the list' do
-        _, update_params = service.execute(note)
+        _, update_params, message = service.execute(note)
         service.apply_updates(update_params, note)
 
+        expect(message).to eq("Assigned @#{assignee.username} and @#{user.username}.")
         expect(note.noteable.assignees.count).to eq(2)
       end
+
+      it_behaves_like 'assigning an already assigned user', false do
+        let(:target) { note.noteable }
+      end
+
+      it_behaves_like 'assigning an already assigned user', true do
+        let(:note) { create(:note_on_issue, note: multiline_assign_note_text, project: project) }
+        let(:target) { note.noteable }
+      end
+    end
+
+    context 'MergeRequest' do
+      let(:note) { create(:note_on_merge_request, note: note_text, project: project) }
+
+      it_behaves_like 'assigning an already assigned user', false do
+        let(:target) { note.noteable }
+      end
+
+      it_behaves_like 'assigning an already assigned user', true do
+        let(:note) { create(:note_on_merge_request, note: multiline_assign_note_text, project: project) }
+        let(:target) { note.noteable }
+      end
+    end
+  end
+
+  describe '/unassign' do
+    let(:note_text) { %(/unassign @#{assignee.username} @#{user.username}\n) }
+    let(:multiline_unassign_note_text) { %(/unassign @#{assignee.username}\n/unassign @#{user.username}) }
+
+    before do
+      project.add_maintainer(user)
+    end
+
+    context 'Issue assignees' do
+      let(:note) { create(:note_on_issue, note: note_text, project: project) }
+
+      it_behaves_like 'unassigning a not assigned user', false do
+        let(:target) { note.noteable }
+      end
+
+      it_behaves_like 'unassigning a not assigned user', true do
+        let(:note) { create(:note_on_issue, note: multiline_unassign_note_text, project: project) }
+        let(:target) { note.noteable }
+      end
+    end
+
+    context 'MergeRequest' do
+      let(:note) { create(:note_on_merge_request, note: note_text, project: project) }
+
+      it_behaves_like 'unassigning a not assigned user', false do
+        let(:target) { note.noteable }
+      end
+
+      it_behaves_like 'unassigning a not assigned user', true do
+        let(:note) { create(:note_on_merge_request, note: multiline_unassign_note_text, project: project) }
+        let(:target) { note.noteable }
+      end
     end
   end
 

ee/spec/services/quick_actions/interpret_service_spec.rb

@@ -964,17 +964,6 @@ describe QuickActions::InterpretService do
       end
     end
 
-    describe 'unassign command with non-existent assignee reference' do
-      let(:content) { "/unassign @#{user.username} @#{user3.username}" }
-      let(:issue) { create(:issue, project: project, assignees: [user, user2]) }
-
-      it 'ignores non-existent assignee references' do
-        _, explanations = service.explain(content, issue)
-
-        expect(explanations).to eq(["Removes assignee @#{user.username}."])
-      end
-    end
-
     describe 'weight command' do
       let(:content) { '/weight 4' }
 

ee/spec/support/shared_examples/quick_actions/issuable/assign_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'assigning an already assigned user' do |is_multiline|
+  before do
+    target.assignees = [assignee]
+  end
+
+  it 'adds multiple assignees from the list' do
+    _, update_params, message = service.execute(note)
+
+    expected_message = is_multiline ? "Assigned @#{user.username}. Assigned @#{assignee.username}." : "Assigned @#{assignee.username} and @#{user.username}."
+    expect(message).to eq(expected_message)
+    expect { service.apply_updates(update_params, note) }.not_to raise_error
+  end
+end

ee/spec/support/shared_examples/quick_actions/issuable/unassign_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'unassigning a not assigned user' do |is_multiline|
+  before do
+    target.assignees = [assignee]
+  end
+
+  it 'adds multiple assignees from the list' do
+    _, update_params, message = service.execute(note)
+
+    expected_message = is_multiline ? "Removed assignee @#{assignee.username}. Removed assignee @#{user.username}." : "Removed assignees @#{user.username} and @#{assignee.username}."
+
+    expect(message).to eq(expected_message)
+    expect { service.apply_updates(update_params, note) }.not_to raise_error
+  end
+end

lib/gitlab/quick_actions/issue_and_merge_request_actions.rb

@@ -12,6 +12,15 @@ module Gitlab
         explanation do |users|
           _('Assigns %{assignee_users_sentence}.') % { assignee_users_sentence: assignee_users_sentence(users) }
         end
+        execution_message do |users = nil|
+          if users.blank?
+            _("Failed to assign a user because no user was found.")
+          else
+            users = [users.first] unless quick_action_target.allows_multiple_assignees?
+
+            _('Assigned %{assignee_users_sentence}.') % { assignee_users_sentence: assignee_users_sentence(users) }
+          end
+        end
         params do
           quick_action_target.allows_multiple_assignees? ? '@user1 @user2' : '@user'
         end
@@ -23,19 +32,14 @@ module Gitlab
           extract_users(assignee_param)
         end
         command :assign do |users|
-          if users.empty?
-            @execution_message[:assign] = _("Failed to assign a user because no user was found.")
-            next
-          end
+          next if users.empty?
 
           if quick_action_target.allows_multiple_assignees?
             @updates[:assignee_ids] ||= quick_action_target.assignees.map(&:id)
-            @updates[:assignee_ids] += users.map(&:id)
+            @updates[:assignee_ids] |= users.map(&:id)
           else
             @updates[:assignee_ids] = [users.first.id]
           end
-
-          @execution_message[:assign] = _('Assigned %{assignee_users_sentence}.') % { assignee_users_sentence: assignee_users_sentence(users) }
         end
 
         desc do
@@ -249,7 +253,7 @@ module Gitlab
         def assignees_for_removal(users)
           assignees = quick_action_target.assignees
           if users.present? && quick_action_target.allows_multiple_assignees?
-            assignees & users
+            users
           else
             assignees
           end

spec/models/issue_assignee_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe IssueAssignee do
+  let(:issue) { create(:issue) }
+
+  subject { issue.issue_assignees.build(assignee: create(:user)) }
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:issue).class_name('Issue') }
+    it { is_expected.to belong_to(:assignee).class_name('User') }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_uniqueness_of(:assignee).scoped_to(:issue_id) }
+  end
+end

spec/models/merge_request_assignee_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequestAssignee do
+  let(:merge_request) { create(:merge_request) }
+
+  subject { merge_request.merge_request_assignees.build(assignee: create(:user)) }
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:merge_request).class_name('MergeRequest') }
+    it { is_expected.to belong_to(:assignee).class_name('User') }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_uniqueness_of(:assignee).scoped_to(:merge_request_id) }
+  end
+end

spec/services/quick_actions/interpret_service_spec.rb

@@ -804,7 +804,7 @@ describe QuickActions::InterpretService do
       let(:issuable) { issue }
     end
 
-    it_behaves_like 'empty command' do
+    it_behaves_like 'empty command', "Failed to assign a user because no user was found." do
       let(:content) { '/assign' }
       let(:issuable) { issue }
     end