[Multiple issue assignees] Fix Issues::UpdateService and IssuesController

app/controllers/projects/issues_controller.rb

@@ -269,8 +269,8 @@ class Projects::IssuesController < Projects::ApplicationController
 
   def issue_params
     params.require(:issue).permit(
-      :title, :assignee_id, :position, :description, :confidential, :weight,
-      :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: []
+      :title, :position, :description, :confidential, :weight,
+      :milestone_id, :due_date, :state_event, :task_num, :lock_version, label_ids: [], assignee_ids: [],
     )
   end
 end

app/services/issues/base_service.rb

@@ -22,15 +22,9 @@ module Issues
     end
 
     def filter_assignee(issuable)
-      return if params[:assignee_ids].blank?
+      return if params[:assignee_ids].to_a.empty?
 
-      assignee_ids = params[:assignee_ids].split(',').map(&:strip)
-
-      if assignee_ids == [ IssuableFinder::NONE ]
-        params[:assignee_ids] = ""
-      else
-        params.delete(:assignee_ids) unless assignee_ids.all?{ |assignee_id| assignee_can_read?(issuable, assignee_id)}
-      end
+      params[:assignee_ids] = params[:assignee_ids].select{ |assignee_id| assignee_can_read?(issuable, assignee_id)}
     end
   end
 end

app/services/issues/update_service.rb

@@ -22,7 +22,7 @@ module Issues
       end
 
       if issue.previous_changes.include?('title') ||
-          issue.previous_changes.include?('description')
+        issue.previous_changes.include?('description')
         todo_service.update_issue(issue, current_user)
       end
 
@@ -30,7 +30,7 @@ module Issues
         create_milestone_note(issue)
       end
 
-      if issue.previous_changes.include?('assignee_ids')
+      if issue.assignees != old_assignees
         create_assignee_note(issue)
         notification_service.reassigned_issue(issue, current_user, old_assignees)
         todo_service.reassigned_issue(issue, current_user)

spec/controllers/projects/issues_controller_spec.rb

@@ -161,11 +161,11 @@ describe Projects::IssuesController do
           namespace_id: project.namespace.to_param,
           project_id: project,
           id: issue.iid,
-          issue: { assignee_ids: assignee.id.to_s },
+          issue: { assignee_ids: [assignee.id] },
           format: :json
         body = JSON.parse(response.body)
 
-        expect(body['assignee'].keys)
+        expect(body['assignees'].first.keys)
           .to match_array(%w(name username avatar_url))
       end
     end

spec/services/issues/update_service_spec.rb

@@ -39,7 +39,7 @@ describe Issues::UpdateService, services: true do
         {
           title: 'New title',
           description: 'Also please fix',
-          assignee_ids: "#{user2.id}, #{user3.id}",
+          assignee_ids: [user2.id, user3.id],
           state_event: 'close',
           label_ids: [label.id],
           due_date: Date.tomorrow
@@ -52,7 +52,7 @@ describe Issues::UpdateService, services: true do
         expect(issue).to be_valid
         expect(issue.title).to eq 'New title'
         expect(issue.description).to eq 'Also please fix'
-        expect(issue.assignees).to eq [user2, user3]
+        expect(issue.assignees).to match_array([user2, user3])
         expect(issue).to be_closed
         expect(issue.labels).to match_array [label]
         expect(issue.due_date).to eq Date.tomorrow
@@ -86,7 +86,7 @@ describe Issues::UpdateService, services: true do
           expect(issue).to be_valid
           expect(issue.title).to eq 'New title'
           expect(issue.description).to eq 'Also please fix'
-          expect(issue.assignees).to eq [user3]
+          expect(issue.assignees).to match_array [user3]
           expect(issue.labels).to be_empty
           expect(issue.milestone).to be_nil
           expect(issue.due_date).to be_nil
@@ -399,6 +399,41 @@ describe Issues::UpdateService, services: true do
       end
     end
 
+    context 'updating asssignee_id' do
+      it 'does not update assignee when assignee_id is invalid' do
+        update_issue(assignee_ids: [-1])
+
+        expect(issue.reload.assignees).to eq([user3])
+      end
+
+      it 'unassigns assignee when user id is 0' do
+        update_issue(assignee_ids: [0])
+
+        expect(issue.reload.assignees).to be_empty
+      end
+
+      it 'does not update assignee_id when user cannot read issue' do
+        update_issue(assignee_ids: [create(:user).id])
+
+        expect(issue.reload.assignees).to eq([user3])
+      end
+
+      context "when issuable feature is private" do
+        levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC]
+
+        levels.each do |level|
+          it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
+            assignee = create(:user)
+            project.update(visibility_level: level)
+            feature_visibility_attr = :"#{issue.model_name.plural}_access_level"
+            project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE)
+
+            expect{ update_issue(assignee_ids: [assignee.id]) }.not_to change{ issue.assignees }
+          end
+        end
+      end
+    end
+
     context 'updating mentions' do
       let(:mentionable) { issue }
       include_examples 'updating mentions', Issues::UpdateService

spec/services/merge_requests/update_service_spec.rb

@@ -466,6 +466,54 @@ describe MergeRequests::UpdateService, services: true do
       end
     end
 
+    context 'updating asssignee_id' do
+      it 'does not update assignee when assignee_id is invalid' do
+        merge_request.update(assignee_id: user.id)
+
+        update_merge_request(assignee_id: -1)
+
+        expect(merge_request.reload.assignee).to eq(user)
+      end
+
+      it 'unassigns assignee when user id is 0' do
+        merge_request.update(assignee_id: user.id)
+
+        update_merge_request(assignee_id: 0)
+
+        expect(merge_request.assignee_id).to be_nil
+      end
+
+      it 'saves assignee when user id is valid' do
+        update_merge_request(assignee_id: user.id)
+
+        expect(merge_request.assignee_id).to eq(user.id)
+      end
+
+      it 'does not update assignee_id when user cannot read issue' do
+        non_member        = create(:user)
+        original_assignee = merge_request.assignee
+
+        update_merge_request(assignee_id: non_member.id)
+
+        expect(merge_request.assignee_id).to eq(original_assignee.id)
+      end
+
+      context "when issuable feature is private" do
+        levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC]
+
+        levels.each do |level|
+          it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
+            assignee = create(:user)
+            project.update(visibility_level: level)
+            feature_visibility_attr = :"#{merge_request.model_name.plural}_access_level"
+            project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE)
+
+            expect{ update_merge_request(assignee_id: assignee) }.not_to change{ merge_request.assignee }
+          end
+        end
+      end
+    end
+
     include_examples 'issuable update service' do
       let(:open_issuable) { merge_request }
       let(:closed_issuable) { create(:closed_merge_request, source_project: project) }

spec/support/services/issuable_update_service_shared_examples.rb

@@ -18,52 +18,4 @@ shared_examples 'issuable update service' do
       end
     end
   end
-
-  context 'asssignee_id' do
-    it 'does not update assignee when assignee_id is invalid' do
-      open_issuable.update(assignee_id: user.id)
-
-      update_issuable(assignee_id: -1)
-
-      expect(open_issuable.reload.assignee).to eq(user)
-    end
-
-    it 'unassigns assignee when user id is 0' do
-      open_issuable.update(assignee_id: user.id)
-
-      update_issuable(assignee_id: 0)
-
-      expect(open_issuable.assignee_id).to be_nil
-    end
-
-    it 'saves assignee when user id is valid' do
-      update_issuable(assignee_id: user.id)
-
-      expect(open_issuable.assignee_id).to eq(user.id)
-    end
-
-    it 'does not update assignee_id when user cannot read issue' do
-      non_member        = create(:user)
-      original_assignee = open_issuable.assignee
-
-      update_issuable(assignee_id: non_member.id)
-
-      expect(open_issuable.assignee_id).to eq(original_assignee.id)
-    end
-
-    context "when issuable feature is private" do
-      levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC]
-
-      levels.each do |level|
-        it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
-          assignee = create(:user)
-          project.update(visibility_level: level)
-          feature_visibility_attr = :"#{open_issuable.model_name.plural}_access_level"
-          project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE)
-
-          expect{ update_issuable(assignee_id: assignee) }.not_to change{ open_issuable.assignee }
-        end
-      end
-    end
-  end
 end