[Multiple issue assignees] address review comments

app/finders/issuable_finder.rb

@@ -231,6 +231,17 @@ class IssuableFinder
     klass.all
   end
 
+  def by_scope(items)
+    case params[:scope]
+    when 'created-by-me', 'authored'
+      items.where(author_id: current_user.id)
+    when 'assigned-to-me'
+      items.assigned_to(current_user)
+    else
+      items
+    end
+  end
+
   def by_state(items)
     case params[:state].to_s
     when 'closed'

app/finders/issues_finder.rb

@@ -38,17 +38,6 @@ class IssuesFinder < IssuableFinder
     items
   end
 
-  def by_scope(items)
-    case params[:scope]
-    when 'created-by-me', 'authored'
-      items.where(author_id: current_user.id)
-    when 'assigned-to-me'
-      items.where("issue_assignees.user_id = ?", current_user.id)
-    else
-      items
-    end
-  end
-
   def self.not_restricted_by_confidentiality(user)
     issues = Issue.with_assignees
 

app/finders/merge_requests_finder.rb

@@ -23,17 +23,6 @@ class MergeRequestsFinder < IssuableFinder
 
   private
 
-  def by_scope(items)
-    case params[:scope]
-    when 'created-by-me', 'authored'
-      items.where(author_id: current_user.id)
-    when 'assigned-to-me'
-      items.where(assignee_id: current_user.id)
-    else
-      items
-    end
-  end
-
   def item_project_ids(items)
     items&.reorder(nil)&.select(:target_project_id)
   end

app/services/issues/base_service.rb

@@ -22,7 +22,7 @@ module Issues
     end
 
     def filter_assignee(issuable)
-      return if params[:assignee_ids].to_a.empty?
+      return if params[:assignee_ids].blank?
 
       assignee_ids = params[:assignee_ids].select{ |assignee_id| assignee_can_read?(issuable, assignee_id)}
 

app/services/system_note_service.rb

@@ -54,7 +54,7 @@ module SystemNoteService
   # issue - Issue object
   # project  - Project owning noteable
   # author   - User performing the change
-  # assignees - User being assigned, or nil
+  # assignees - Users being assigned, or nil
   #
   # Example Note text:
   #

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -1329,7 +1329,7 @@ describe Projects::MergeRequestsController do
     end
 
     it 'correctly pluralizes flash message on success' do
-      issue2.update!(assignees: [user])
+      issue2.assignees = [user]
 
       post_assign_issues
 

spec/features/atom/dashboard_issues_spec.rb

@@ -41,7 +41,7 @@ describe "Dashboard Issues Feed", feature: true  do
 
           expect(entry).to be_present
           expect(entry).to have_selector('author email', text: issue2.author_public_email)
-          expect(entry).to have_selector('assignees email', text: issue2.assignees.first.public_email)
+          expect(entry).to have_selector('assignees email', text: assignee.public_email)
           expect(entry).not_to have_selector('labels')
           expect(entry).not_to have_selector('milestone')
           expect(entry).to have_selector('description', text: issue2.description)
@@ -64,7 +64,7 @@ describe "Dashboard Issues Feed", feature: true  do
 
           expect(entry).to be_present
           expect(entry).to have_selector('author email', text: issue1.author_public_email)
-          expect(entry).to have_selector('assignees email', text: issue1.assignees.first.public_email)
+          expect(entry).to have_selector('assignees email', text: assignee.public_email)
           expect(entry).to have_selector('labels label', text: label1.title)
           expect(entry).to have_selector('milestone', text: milestone1.title)
           expect(entry).not_to have_selector('description')

spec/models/concerns/elastic/issue_spec.rb

@@ -31,13 +31,14 @@ describe Issue, elastic: true do
   end
 
   it "returns json with all needed elements" do
-    issue = create :issue, project: project
+    assignee = create(:user)
+    issue = create :issue, project: project, assignees: [assignee]
 
     expected_hash = issue.attributes.extract!('id', 'iid', 'title', 'description', 'created_at',
                                                 'updated_at', 'state', 'project_id', 'author_id',
                                                 'confidential')
 
-    expected_hash['assignee_id'] = []
+    expected_hash['assignee_id'] = [assignee.id]
 
     expect(issue.as_indexed_json).to eq(expected_hash)
   end

spec/models/concerns/issuable_spec.rb

@@ -56,84 +56,6 @@ describe Issue, "Issuable" do
     end
   end
 
-  describe "before_save" do
-    describe "#update_cache_counts when an issue is reassigned" do
-      context "when previous assignee exists" do
-        before do
-          assignee = create(:user)
-          issue.project.team << [assignee, :developer]
-          issue.assignees << assignee
-        end
-
-        it "updates cache counts for new assignee" do
-          user = create(:user)
-
-          expect(user).to receive(:update_cache_counts)
-
-          issue.assignees << user
-        end
-
-        it "updates cache counts for previous assignee" do
-          old_assignee = issue.assignees.first
-
-          expect_any_instance_of(User).to receive(:update_cache_counts)
-
-          issue.assignees.destroy_all
-        end
-      end
-
-      context "when previous assignee does not exist" do
-        before{ issue.assignees = [] }
-
-        it "updates cache count for the new assignee" do
-          expect_any_instance_of(User).to receive(:update_cache_counts)
-
-          issue.assignees << user
-        end
-      end
-    end
-
-    describe "#update_cache_counts when a merge request is reassigned" do
-      let(:project) { create :project }
-      let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
-
-      context "when previous assignee exists" do
-        before do
-          assignee = create(:user)
-          project.team << [assignee, :developer]
-          merge_request.update(assignee: assignee)
-        end
-
-        it "updates cache counts for new assignee" do
-          user = create(:user)
-
-          expect(user).to receive(:update_cache_counts)
-
-          merge_request.update(assignee: user)
-        end
-
-        it "updates cache counts for previous assignee" do
-          old_assignee = merge_request.assignee
-          allow(User).to receive(:find_by_id).with(old_assignee.id).and_return(old_assignee)
-
-          expect(old_assignee).to receive(:update_cache_counts)
-
-          merge_request.update(assignee: nil)
-        end
-      end
-
-      context "when previous assignee does not exist" do
-        before { merge_request.update(assignee: nil) }
-
-        it "updates cache count for the new assignee" do
-          expect_any_instance_of(User).to receive(:update_cache_counts)
-
-          merge_request.update(assignee: user)
-        end
-      end
-    end
-  end
-
   describe ".search" do
     let!(:searchable_issue) { create(:issue, title: "Searchable issue") }
 

spec/models/issue_spec.rb

@@ -38,6 +38,46 @@ describe Issue, models: true do
     end
   end
 
+  describe "before_save" do
+    describe "#update_cache_counts when an issue is reassigned" do
+      let(:issue) { create(:issue) }
+      let(:assignee) { create(:user) }
+
+      context "when previous assignee exists" do
+        before do
+          issue.project.team << [assignee, :developer]
+          issue.assignees << assignee
+        end
+
+        it "updates cache counts for new assignee" do
+          user = create(:user)
+
+          expect(user).to receive(:update_cache_counts)
+
+          issue.assignees << user
+        end
+
+        it "updates cache counts for previous assignee" do
+          issue.assignees.first
+
+          expect_any_instance_of(User).to receive(:update_cache_counts)
+
+          issue.assignees.destroy_all
+        end
+      end
+
+      context "when previous assignee does not exist" do
+        it "updates cache count for the new assignee" do
+          issue.assignees = []
+
+          expect_any_instance_of(User).to receive(:update_cache_counts)
+
+          issue.assignees << assignee
+        end
+      end
+    end
+  end
+
   describe '#card_attributes' do
     it 'includes the author name' do
       allow(subject).to receive(:author).and_return(double(name: 'Robert'))

spec/models/merge_request_spec.rb

@@ -88,6 +88,48 @@ describe MergeRequest, models: true do
     end
   end
 
+  describe "before_save" do
+    describe "#update_cache_counts when a merge request is reassigned" do
+      let(:project) { create :project }
+      let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+      let(:assignee) { create :user }
+
+      context "when previous assignee exists" do
+        before do
+          project.team << [assignee, :developer]
+          merge_request.update(assignee: assignee)
+        end
+
+        it "updates cache counts for new assignee" do
+          user = create(:user)
+
+          expect(user).to receive(:update_cache_counts)
+
+          merge_request.update(assignee: user)
+        end
+
+        it "updates cache counts for previous assignee" do
+          old_assignee = merge_request.assignee
+          allow(User).to receive(:find_by_id).with(old_assignee.id).and_return(old_assignee)
+
+          expect(old_assignee).to receive(:update_cache_counts)
+
+          merge_request.update(assignee: nil)
+        end
+      end
+
+      context "when previous assignee does not exist" do
+        it "updates cache count for the new assignee" do
+          merge_request.update(assignee: nil)
+
+          expect_any_instance_of(User).to receive(:update_cache_counts)
+
+          merge_request.update(assignee: assignee)
+        end
+      end
+    end
+  end
+
   describe '#card_attributes' do
     it 'includes the author name' do
       allow(subject).to receive(:author).and_return(double(name: 'Robert'))