Tidy up BulkUpdateService specs

1. Don't use instance variables, use `let` instead.
2. Add descriptions for all specs.
3. Share variables where possible.
4. Give labels more vivid names than 1, 2, and 3.
5. Remove deprecation warnings by passing issue IDs as '1,2,3' instead
   of an array, as that's how they're passed by the front-end. (The
   deprecation warning is for passing a nested array, which is what
   happens if an actual array is passed, as:
     `[1, 2, 3].split(',') == [[1, 2, 3]]`

1 files changed, 97 insertions, 103 deletions

diff --git a/spec/services/issues/bulk_update_service_spec.rb b/spec/services/issues/bulk_update_service_spec.rb
index 09a1415abf0..91d08636951 100644
--- a/spec/services/issues/bulk_update_service_spec.rb
+++ b/spec/services/issues/bulk_update_service_spec.rb
@@ -1,36 +1,28 @@
 require 'spec_helper'
 
 describe Issues::BulkUpdateService, services: true do
-  let(:issue) { create(:issue, project: @project) }
-
-  before do
-    @user = create :user
-    opts = {
-      name: "GitLab",
-      namespace: @user.namespace
-    }
-    @project = Projects::CreateService.new(@user, opts).execute
-  end
+  let(:user) { create(:user) }
+  let(:project) { Projects::CreateService.new(user, namespace: user.namespace, name: 'test').execute }
 
-  describe :close_issue do
+  let!(:result) { Issues::BulkUpdateService.new(project, user, params).execute }
 
     before do
       @issues = create_list(:issue, 5, project: @project)
       @params = {
         state_event: 'close',
-        issues_ids: @issues.map(&:id).join(",")
+        issues_ids: issues.map(&:id).join(',')
       }
     end
 
-    it do
-      result = Issues::BulkUpdateService.new(@project, @user, @params).execute
+    it 'succeeds and returns the correct number of issues updated' do
       expect(result[:success]).to be_truthy
-      expect(result[:count]).to eq(@issues.count)
-
-      expect(@project.issues.opened).to be_empty
-      expect(@project.issues.closed).not_to be_empty
+      expect(result[:count]).to eq(issues.count)
     end
 
+    it 'closes all the issues passed' do
+      expect(project.issues.opened).to be_empty
+      expect(project.issues.closed).not_to be_empty
+    end
   end
 
   describe :reopen_issues do
@@ -38,95 +30,99 @@ describe Issues::BulkUpdateService, services: true do
       @issues = create_list(:closed_issue, 5, project: @project)
       @params = {
         state_event: 'reopen',
-        issues_ids: @issues.map(&:id).join(",")
+        issues_ids: issues.map(&:id).join(',')
       }
     end
 
-    it do
-      result = Issues::BulkUpdateService.new(@project, @user, @params).execute
+    it 'succeeds and returns the correct number of issues updated' do
       expect(result[:success]).to be_truthy
-      expect(result[:count]).to eq(@issues.count)
-
-      expect(@project.issues.closed).to be_empty
-      expect(@project.issues.opened).not_to be_empty
+      expect(result[:count]).to eq(issues.count)
     end
 
+    it 'reopens all the issues passed' do
+      expect(project.issues.closed).to be_empty
+      expect(project.issues.opened).not_to be_empty
+    end
   end
 
-  describe :update_assignee do
+  describe 'updating assignee' do
+    let(:issue) do
+      create(:issue, project: project) { |issue| issue.update_attributes(assignee: user) }
+    end
 
-    before do
-      @new_assignee = create :user
-      @params = {
-        issues_ids: issue.id.to_s,
-        assignee_id: @new_assignee.id
+    let(:params) do
+      {
+        assignee_id: assignee_id,
+        issues_ids: issue.id.to_s
       }
     end
 
-    it do
-      result = Issues::BulkUpdateService.new(@project, @user, @params).execute
-      expect(result[:success]).to be_truthy
-      expect(result[:count]).to eq(1)
+    context 'when the new assignee ID is a valid user' do
+      let(:new_assignee) { create(:user) }
+      let(:assignee_id) { new_assignee.id }
 
-      expect(@project.issues.first.assignee).to eq(@new_assignee)
-    end
+      it 'succeeds' do
+        expect(result[:success]).to be_truthy
+        expect(result[:count]).to eq(1)
+      end
 
-    it 'allows mass-unassigning' do
-      @project.issues.first.update_attribute(:assignee, @new_assignee)
-      expect(@project.issues.first.assignee).not_to be_nil
+      it 'updates the assignee to the use ID passed' do
+        expect(issue.reload.assignee).to eq(new_assignee)
+      end
+    end
 
-      @params[:assignee_id] = -1
+    context 'when the new assignee ID is -1' do
+      let(:assignee_id) { -1 }
 
-      Issues::BulkUpdateService.new(@project, @user, @params).execute
-      expect(@project.issues.first.assignee).to be_nil
+      it 'unassigns the issues' do
+        expect(issue.reload.assignee).to be_nil
+      end
     end
 
-    it 'does not unassign when assignee_id is not present' do
-      @project.issues.first.update_attribute(:assignee, @new_assignee)
-      expect(@project.issues.first.assignee).not_to be_nil
-
-      @params[:assignee_id] = ''
+    context 'when the new assignee ID is not present', focus: true do
+      let(:assignee_id) { nil }
 
-      Issues::BulkUpdateService.new(@project, @user, @params).execute
-      expect(@project.issues.first.assignee).not_to be_nil
+      it 'does not unassign' do
+        expect(issue.reload.assignee).to eq(user)
+      end
     end
   end
 
-  describe :update_milestone do
+  describe 'updating milestones' do
+    let(:issue) { create(:issue, project: project) }
+    let(:milestone) { create(:milestone, project: project) }
 
-    before do
-      @milestone = create(:milestone, project: @project)
-      @params = {
+    let(:params) do
+      {
         issues_ids: issue.id.to_s,
-        milestone_id: @milestone.id
+        milestone_id: milestone.id
       }
     end
 
-    it do
-      result = Issues::BulkUpdateService.new(@project, @user, @params).execute
+    it 'succeeds' do
       expect(result[:success]).to be_truthy
       expect(result[:count]).to eq(1)
+    end
 
-      expect(@project.issues.first.milestone).to eq(@milestone)
+    it 'updates the issue milestone' do
+      expect(project.issues.first.milestone).to eq(milestone)
     end
   end
 
   describe 'updating labels' do
     def create_issue_with_labels(labels)
-      create(:issue, project: project) { |issue| issue.labels = labels }
+      create(:issue, project: project) { |issue| issue.update_attributes(labels: labels) }
     end
 
-    let(:user) { create(:user) }
-    let(:project) { Projects::CreateService.new(user, namespace: user.namespace, name: 'test').execute }
-    let(:label_1) { create(:label, project: project) }
-    let(:label_2) { create(:label, project: project) }
-    let(:label_3) { create(:label, project: project) }
+    let(:bug) { create(:label, project: project) }
+    let(:regression) { create(:label, project: project) }
+    let(:merge_requests) { create(:label, project: project) }
 
-    let(:issue_all_labels) { create_issue_with_labels([label_1, label_2, label_3]) }
-    let(:issue_labels_1_and_2) { create_issue_with_labels([label_1, label_2]) }
-    let(:issue_labels_1_and_3) { create_issue_with_labels([label_1, label_3]) }
+    let(:issue_all_labels) { create_issue_with_labels([bug, regression, merge_requests]) }
+    let(:issue_bug_and_regression) { create_issue_with_labels([bug, regression]) }
+    let(:issue_bug_and_merge_requests) { create_issue_with_labels([bug, merge_requests]) }
     let(:issue_no_labels) { create(:issue, project: project) }
-    let(:issues) { [issue_all_labels, issue_labels_1_and_2, issue_labels_1_and_3, issue_no_labels] }
+    let(:issues) { [issue_all_labels, issue_bug_and_regression, issue_bug_and_merge_requests, issue_no_labels] }
 
     let(:labels) { [] }
     let(:add_labels) { [] }
@@ -141,76 +137,74 @@ describe Issues::BulkUpdateService, services: true do
       }
     end
 
-    before { Issues::BulkUpdateService.new(project, user, params).execute }
-
     context 'when label_ids are passed' do
       let(:issues) { [issue_all_labels, issue_no_labels] }
-      let(:labels) { [label_1, label_2] }
+      let(:labels) { [bug, regression] }
 
       it 'updates the labels of all issues passed to the labels passed' do
         expect(issues.map(&:reload).map(&:label_ids)).to all(eq(labels.map(&:id)))
       end
 
       it 'does not update issues not passed in' do
-        expect(issue_labels_1_and_2.label_ids).to contain_exactly(label_1.id, label_2.id)
+        expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id)
       end
     end
 
     context 'when add_label_ids are passed' do
-      let(:issues) { [issue_all_labels, issue_labels_1_and_3, issue_no_labels] }
-      let(:add_labels) { [label_1, label_2, label_3] }
+      let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] }
+      let(:add_labels) { [bug, regression, merge_requests] }
 
       it 'adds those label IDs to all issues passed' do
         expect(issues.map(&:reload).map(&:label_ids)).to all(include(*add_labels.map(&:id)))
       end
 
       it 'does not update issues not passed in' do
-        expect(issue_labels_1_and_2.label_ids).to contain_exactly(label_1.id, label_2.id)
+        expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id)
       end
     end
 
     context 'when remove_label_ids are passed' do
-      let(:issues) { [issue_all_labels, issue_labels_1_and_3, issue_no_labels] }
-      let(:remove_labels) { [label_1, label_2, label_3] }
+      let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] }
+      let(:remove_labels) { [bug, regression, merge_requests] }
 
       it 'removes those label IDs from all issues passed' do
         expect(issues.map(&:reload).map(&:label_ids)).to all(be_empty)
       end
 
       it 'does not update issues not passed in' do
-        expect(issue_labels_1_and_2.label_ids).to contain_exactly(label_1.id, label_2.id)
+        expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id)
       end
     end
 
     context 'when add_label_ids and remove_label_ids are passed' do
-      let(:issues) { [issue_all_labels, issue_labels_1_and_3, issue_no_labels] }
-      let(:add_labels) { [label_1] }
-      let(:remove_labels) { [label_3] }
+      let(:issues) { [issue_all_labels, issue_bug_and_merge_requests, issue_no_labels] }
+      let(:add_labels) { [bug] }
+      let(:remove_labels) { [merge_requests] }
 
       it 'adds the label IDs to all issues passed' do
-        expect(issues.map(&:reload).map(&:label_ids)).to all(include(label_1.id))
+        expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id))
       end
 
       it 'removes the label IDs from all issues passed' do
-        expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(label_3.id)
+        expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(merge_requests.id)
       end
 
       it 'does not update issues not passed in' do
-        expect(issue_labels_1_and_2.label_ids).to contain_exactly(label_1.id, label_2.id)
+        expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id)
       end
     end
 
     context 'when add_label_ids and label_ids are passed' do
-      let(:issues) { [issue_all_labels, issue_labels_1_and_2, issue_labels_1_and_3] }
-      let(:labels) { [label_3] }
-      let(:add_labels) { [label_2] }
+      let(:issues) { [issue_all_labels, issue_bug_and_regression, issue_bug_and_merge_requests] }
+      let(:labels) { [merge_requests] }
+      let(:add_labels) { [regression] }
 
       it 'adds the label IDs to all issues passed' do
-        expect(issues.map(&:reload).map(&:label_ids)).to all(include(label_2.id))
+        expect(issues.map(&:reload).map(&:label_ids)).to all(include(regression.id))
       end
 
       it 'ignores the label IDs parameter' do
-        expect(issues.map(&:reload).map(&:label_ids)).to all(include(label_1.id))
+        expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id))
       end
 
       it 'does not update issues not passed in' do
@@ -219,43 +213,43 @@ describe Issues::BulkUpdateService, services: true do
     end
 
     context 'when remove_label_ids and label_ids are passed' do
-      let(:issues) { [issue_no_labels, issue_labels_1_and_2] }
-      let(:labels) { [label_3] }
-      let(:remove_labels) { [label_2] }
+      let(:issues) { [issue_no_labels, issue_bug_and_regression] }
+      let(:labels) { [merge_requests] }
+      let(:remove_labels) { [regression] }
 
       it 'remove the label IDs from all issues passed' do
-        expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(label_2.id)
+        expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(regression.id)
       end
 
       it 'ignores the label IDs parameter' do
-        expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(label_3.id)
+        expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(merge_requests.id)
       end
 
       it 'does not update issues not passed in' do
-        expect(issue_all_labels.label_ids).to contain_exactly(label_1.id, label_2.id, label_3.id)
+        expect(issue_all_labels.label_ids).to contain_exactly(bug.id, regression.id, merge_requests.id)
       end
     end
 
     context 'when add_label_ids, remove_label_ids, and label_ids are passed' do
-      let(:issues) { [issue_labels_1_and_3, issue_no_labels] }
-      let(:labels) { [label_2] }
-      let(:add_labels) { [label_1] }
-      let(:remove_labels) { [label_3] }
+      let(:issues) { [issue_bug_and_merge_requests, issue_no_labels] }
+      let(:labels) { [regression] }
+      let(:add_labels) { [bug] }
+      let(:remove_labels) { [merge_requests] }
 
       it 'adds the label IDs to all issues passed' do
-        expect(issues.map(&:reload).map(&:label_ids)).to all(include(label_1.id))
+        expect(issues.map(&:reload).map(&:label_ids)).to all(include(bug.id))
       end
 
       it 'removes the label IDs from all issues passed' do
-        expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(label_3.id)
+        expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(merge_requests.id)
       end
 
       it 'ignores the label IDs parameter' do
-        expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(label_2.id)
+        expect(issues.map(&:reload).map(&:label_ids).flatten).not_to include(regression.id)
       end
 
       it 'does not update issues not passed in' do
-        expect(issue_labels_1_and_2.label_ids).to contain_exactly(label_1.id, label_2.id)
+        expect(issue_bug_and_regression.label_ids).to contain_exactly(bug.id, regression.id)
       end
     end
   end