[Multiple issue assignees] Fix a few specs

app/services/notification_recipient_service.rb

@@ -8,7 +8,6 @@ class NotificationRecipientService
     @project = project
   end
 
-  # TODO: refactor this: previous_assignee argument can be a user object or an array which is not really nice
   def build_recipients(target, current_user, action: nil, previous_assignee: nil, skip_current_user: true)
     custom_action = build_custom_key(action, target)
 
@@ -29,7 +28,8 @@ class NotificationRecipientService
       recipients << previous_assignee if previous_assignee
       recipients << target.assignee
     when :reassign_issue
-      recipients.concat(previous_assignee) if previous_assignee.any?
+      previous_assignees = Array(previous_assignee)
+      recipients.concat(previous_assignees) if previous_assignees.any?
       recipients.concat(target.assignees)
     end
 

app/services/system_note_service.rb

@@ -68,13 +68,16 @@ module SystemNoteService
   #
   # Returns the created Note object
   def change_issue_assignees(issue, project, author, old_assignees)
-    # TODO: basic implementation, should be improved before merging the MR
     body =
       if issue.assignees.any? && old_assignees.any?
         unassigned_users = old_assignees - issue.assignees
         added_users = issue.assignees.to_a - old_assignees
 
-        "assigned to #{added_users.map(&:to_reference).to_sentence} and unassigned #{unassigned_users.map(&:to_reference).to_sentence}"
+        text_parts = []
+        text_parts << "assigned to #{added_users.map(&:to_reference).to_sentence}" if added_users.any?
+        text_parts << "unassigned #{unassigned_users.map(&:to_reference).to_sentence}" if unassigned_users.any?
+
+        text_parts.join(' and ')
       elsif old_assignees.any?
         "removed all assignees"
       elsif issue.assignees.any?

lib/gitlab/github_import/issue_formatter.rb

@@ -10,7 +10,7 @@ module Gitlab
           description: description,
           state: state,
           author_id: author_id,
-          assignee_ids: [assignee_id],
+          assignee_ids: Array(assignee_id),
           created_at: raw_data.created_at,
           updated_at: raw_data.updated_at
         }

spec/lib/gitlab/github_import/issue_formatter_spec.rb

@@ -43,7 +43,7 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
           description: "*Created by: octocat*\n\nI'm having a problem with this.",
           state: 'opened',
           author_id: project.creator_id,
-          assignee_id: nil,
+          assignee_ids: [],
           created_at: created_at,
           updated_at: updated_at
         }
@@ -64,7 +64,7 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
           description: "*Created by: octocat*\n\nI'm having a problem with this.",
           state: 'closed',
           author_id: project.creator_id,
-          assignee_id: nil,
+          assignee_ids: [],
           created_at: created_at,
           updated_at: updated_at
         }
@@ -77,19 +77,19 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
       let(:raw_data) { double(base_data.merge(assignee: octocat)) }
 
       it 'returns nil as assignee_id when is not a GitLab user' do
-        expect(issue.attributes.fetch(:assignee_id)).to be_nil
+        expect(issue.attributes.fetch(:assignee_ids)).to be_empty
       end
 
       it 'returns GitLab user id associated with GitHub id as assignee_id' do
         gl_user = create(:omniauth_user, extern_uid: octocat.id, provider: 'github')
 
-        expect(issue.attributes.fetch(:assignee_id)).to eq gl_user.id
+        expect(issue.attributes.fetch(:assignee_ids)).to eq [gl_user.id]
       end
 
       it 'returns GitLab user id associated with GitHub email as assignee_id' do
         gl_user = create(:user, email: octocat.email)
 
-        expect(issue.attributes.fetch(:assignee_id)).to eq gl_user.id
+        expect(issue.attributes.fetch(:assignee_ids)).to eq [gl_user.id]
       end
     end
 

spec/services/system_note_service_spec.rb

@@ -6,6 +6,7 @@ describe SystemNoteService, services: true do
   let(:project)  { create(:project) }
   let(:author)   { create(:user) }
   let(:noteable) { create(:issue, project: project) }
+  let(:issue)    { noteable }
 
   shared_examples_for 'a system note' do
     it 'is valid' do
@@ -133,6 +134,50 @@ describe SystemNoteService, services: true do
     end
   end
 
+  describe '.change_issue_assignees' do
+    subject { described_class.change_issue_assignees(noteable, project, author, [assignee]) }
+
+    let(:assignee) { create(:user) }
+    let(:assignee1) { create(:user) }
+    let(:assignee2) { create(:user) }
+    let(:assignee3) { create(:user) }
+
+    it_behaves_like 'a system note'
+
+    def build_note(old_assignees, new_assignees)
+      issue.assignees = new_assignees
+      described_class.change_issue_assignees(issue, project, author, old_assignees).note
+    end
+
+    it 'builds a correct phrase when an assignee is added to a non-assigned issue' do
+      expect(build_note([], [assignee1])).to eq "assigned to @#{assignee1.username}"
+    end
+
+    it 'builds a correct phrase when assignee removed' do
+      expect(build_note([assignee1], [])).to eq 'removed all assignees'
+    end
+
+    it 'builds a correct phrase when assignees changed' do
+      expect(build_note([assignee1], [assignee2])).to eq \
+        "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}"
+    end
+
+    it 'builds a correct phrase when three assignees removed and one added' do
+      expect(build_note([assignee, assignee1, assignee2], [assignee3])).to eq \
+        "assigned to @#{assignee3.username} and unassigned @#{assignee.username}, @#{assignee1.username}, and @#{assignee2.username}"
+    end
+
+    it 'builds a correct phrase when one assignee changed from a set' do
+      expect(build_note([assignee, assignee1], [assignee, assignee2])).to eq \
+        "assigned to @#{assignee2.username} and unassigned @#{assignee1.username}"
+    end
+
+    it 'builds a correct phrase when one assignee removed from a set' do
+      expect(build_note([assignee, assignee1, assignee2], [assignee, assignee1])).to eq \
+        "unassigned @#{assignee2.username}"
+    end
+  end
+
   describe '.change_label' do
     subject { described_class.change_label(noteable, project, author, added, removed) }