Merge branch 'fix-bulk-assign' into 'master'

Fix unwanted label unassignment

## What does this MR do?
- When updating the milestone
  - [x] Do not remove labels when assigning a milestone
  - [x] Do not remove labels when unassigning a milestone
  - [x] Do not remove labels when assigning a milestone and adding another label

- When toggling selected issues labels should be kept
  - [x] Select an issue with an assigned label -> pick another label from dropdown-> unselect the issue -> select the issue again -> submit the form: Existing label should not be removed.

## Are there points in the code the reviewer needs to double check?
Labels should not be added or removed to issues when doing bulk actions unless we explicitly select a label from the dropdown

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [ ] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !4863

CHANGELOG

@@ -25,6 +25,7 @@ v 8.9.1 (unreleased)
   - Use jQuery objects in ref dropdown. !4850
   - Fix GitLab project import issues related to notes and builds. !4855
   - Restrict header logo to 36px so it doesn't overflow. !4861
+  - Fix unwanted label unassignment. !4863
   - Fix merge requests project settings help link anchor. !4873
   - Fix 404 when accessing pipelines as guest user on public projects. !4881
 

app/assets/javascripts/issuable.js.coffee

@@ -68,12 +68,15 @@ issuable_created = false
     Turbolinks.visit(issuesUrl);
 
   initChecks: ->
+    @issuableBulkActions = $('.bulk-update').data('bulkActions')
+
     $('.check_all_issues').off('click').on('click', ->
       $('.selected_issue').prop('checked', @checked)
       Issuable.checkChanged()
     )
 
-    $('.selected_issue').off('change').on('change', Issuable.checkChanged)
+    $('.selected_issue').off('change').on('change', Issuable.checkChanged.bind(@))
+
 
   checkChanged: ->
     checked_issues = $('.selected_issue:checked')
@@ -88,3 +91,6 @@ issuable_created = false
       $('#update_issues_ids').val []
       $('.issues_bulk_update').hide()
       $('.issues-other-filters').show()
+      @issuableBulkActions.willUpdateLabels = false
+
+    return true

app/assets/javascripts/issues-bulk-assignment.js.coffee

@@ -7,6 +7,11 @@ class @IssuableBulkActions
       @issues = @getElement('.issues-list .issue')
     } = opts
 
+    # Save instance
+    @form.data 'bulkActions', @
+
+    @willUpdateLabels = false
+
     @bindEvents()
 
     # Fixes bulk-assign not working when navigating through pages
@@ -87,6 +92,7 @@ class @IssuableBulkActions
         add_label_ids     : []
         remove_label_ids  : []
 
+    if @willUpdateLabels
       @getLabelsToApply().map (id) ->
         formData.update.add_label_ids.push id
 

app/assets/javascripts/labels_select.js.coffee

@@ -319,6 +319,8 @@ class @LabelsSelect
 
         multiSelect: $dropdown.hasClass 'js-multiselect'
         clicked: (label) ->
+          _this.enableBulkLabelDropdown()
+
           if $dropdown.hasClass('js-filter-bulk-update')
             return
 
@@ -377,3 +379,8 @@ class @LabelsSelect
       label_ids.push $("#issue_#{issue_id}").data('labels')
 
     _.intersection.apply _, label_ids
+
+  enableBulkLabelDropdown: ->
+    if $('.selected_issue:checked').length
+      issuableBulkActions = $('.bulk-update').data('bulkActions')
+      issuableBulkActions.willUpdateLabels = true

spec/features/issues/bulk_assignment_labels_spec.rb

@@ -10,7 +10,7 @@ feature 'Issues > Labels bulk assignment', feature: true do
   let!(:bug)      { create(:label, project: project, title: 'bug') }
   let!(:feature)  { create(:label, project: project, title: 'feature') }
 
-  context 'as a allowed user', js: true do
+  context 'as an allowed user', js: true do
     before do
       project.team << [user, :master]
 
@@ -164,6 +164,133 @@ feature 'Issues > Labels bulk assignment', feature: true do
         end
       end
     end
+
+    context 'toggling a milestone' do
+      let!(:milestone) { create(:milestone, project: project, title: 'First Release') }
+
+      context 'setting a milestone' do
+        before do
+          issue1.labels << bug
+          issue2.labels << feature
+          visit namespace_project_issues_path(project.namespace, project)
+        end
+
+        it 'labels are kept' do
+          expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+          expect(find("#issue_#{issue2.id}")).to have_content 'feature'
+
+          check 'check_all_issues'
+          open_milestone_dropdown(['First Release'])
+          update_issues
+
+          expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+          expect(find("#issue_#{issue1.id}")).to have_content 'First Release'
+          expect(find("#issue_#{issue2.id}")).to have_content 'feature'
+          expect(find("#issue_#{issue2.id}")).to have_content 'First Release'
+        end
+      end
+
+      context 'setting a milestone and adding another label' do
+        before do
+          issue1.labels << bug
+
+          visit namespace_project_issues_path(project.namespace, project)
+        end
+
+        it 'existing label is kept and new label is present' do
+          expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+
+          check 'check_all_issues'
+          open_milestone_dropdown ['First Release']
+          open_labels_dropdown ['feature']
+          update_issues
+
+          expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+          expect(find("#issue_#{issue1.id}")).to have_content 'feature'
+          expect(find("#issue_#{issue1.id}")).to have_content 'First Release'
+          expect(find("#issue_#{issue2.id}")).to have_content 'feature'
+          expect(find("#issue_#{issue2.id}")).to have_content 'First Release'
+        end
+      end
+
+      context 'setting a milestone and removing existing label' do
+        before do
+          issue1.labels << bug
+          issue1.labels << feature
+          issue2.labels << feature
+
+          visit namespace_project_issues_path(project.namespace, project)
+        end
+
+        it 'existing label is kept and new label is present' do
+          expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+          expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+          expect(find("#issue_#{issue2.id}")).to have_content 'feature'
+
+          check 'check_all_issues'
+          open_milestone_dropdown ['First Release']
+          unmark_labels_in_dropdown ['feature']
+          update_issues
+
+          expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+          expect(find("#issue_#{issue1.id}")).not_to have_content 'feature'
+          expect(find("#issue_#{issue1.id}")).to have_content 'First Release'
+          expect(find("#issue_#{issue2.id}")).not_to have_content 'feature'
+          expect(find("#issue_#{issue2.id}")).to have_content 'First Release'
+        end
+      end
+
+      context 'unsetting a milestone' do
+        before do
+          issue1.milestone = milestone
+          issue2.milestone = milestone
+          issue1.save
+          issue2.save
+          issue1.labels << bug
+          issue2.labels << feature
+
+          visit namespace_project_issues_path(project.namespace, project)
+        end
+
+        it 'labels are kept' do
+          expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+          expect(find("#issue_#{issue1.id}")).to have_content 'First Release'
+          expect(find("#issue_#{issue2.id}")).to have_content 'feature'
+          expect(find("#issue_#{issue2.id}")).to have_content 'First Release'
+
+          check 'check_all_issues'
+          open_milestone_dropdown(['No Milestone'])
+          update_issues
+
+          expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+          expect(find("#issue_#{issue1.id}")).not_to have_content 'First Release'
+          expect(find("#issue_#{issue2.id}")).to have_content 'feature'
+          expect(find("#issue_#{issue2.id}")).not_to have_content 'First Release'
+        end
+      end
+    end
+
+    context 'toggling checked issues' do
+      before do
+        issue1.labels << bug
+
+        visit namespace_project_issues_path(project.namespace, project)
+      end
+
+      it do
+        expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+
+        check_issue issue1
+        open_labels_dropdown ['feature']
+        uncheck_issue issue1
+        check_issue issue1
+        update_issues
+        sleep 1 # needed
+
+        expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+        expect(find("#issue_#{issue1.id}")).not_to have_content 'feature'
+      end
+    end
   end
 
   context 'as a guest' do
@@ -181,6 +308,16 @@ feature 'Issues > Labels bulk assignment', feature: true do
     end
   end
 
+  def open_milestone_dropdown(items = [])
+    page.within('.issues_bulk_update') do
+      click_button 'Milestone'
+      wait_for_ajax
+      items.map do |item|
+        click_link item
+      end
+    end
+  end
+
   def open_labels_dropdown(items = [], unmark = false)
     page.within('.issues_bulk_update') do
       click_button 'Label'
@@ -201,11 +338,19 @@ feature 'Issues > Labels bulk assignment', feature: true do
     open_labels_dropdown(items, true)
   end
 
-  def check_issue(issue)
+  def check_issue(issue, uncheck = false)
     page.within('.issues-list') do
+      if uncheck
+        uncheck "selected_issue_#{issue.id}"
+      else
         check "selected_issue_#{issue.id}"
       end
     end
+  end
+
+  def uncheck_issue(issue)
+    check_issue(issue, true)
+  end
 
   def update_issues
     click_button 'Update issues'