Fix bulk editing labels bug

Fix bug where labels were being bulk added that the user did not
explicitly select

app/assets/javascripts/issuable_bulk_update_actions.js

-/* eslint-disable consistent-return, func-names, array-callback-return */
-
 import $ from 'jquery';
-import { intersection } from 'lodash';
+import { difference, intersection, union } from 'lodash';
 import axios from './lib/utils/axios_utils';
 import Flash from './flash';
 import { __ } from './locale';
@@ -36,43 +34,6 @@ export default {
     return new Flash(__('Issue update failed'));
   },
 
-  getSelectedIssues() {
-    return this.issues.has('.selected-issuable:checked');
-  },
-
-  getLabelsFromSelection() {
-    const labels = [];
-    this.getSelectedIssues().map(function() {
-      const labelsData = $(this).data('labels');
-      if (labelsData) {
-        return labelsData.map(labelId => {
-          if (labels.indexOf(labelId) === -1) {
-            return labels.push(labelId);
-          }
-        });
-      }
-    });
-    return labels;
-  },
-
-  /**
-   * Will return only labels that were marked previously and the user has unmarked
-   * @return {Array} Label IDs
-   */
-
-  getUnmarkedIndeterminedLabels() {
-    const result = [];
-    const labelsToKeep = this.$labelDropdown.data('indeterminate');
-
-    this.getLabelsFromSelection().forEach(id => {
-      if (labelsToKeep.indexOf(id) === -1) {
-        result.push(id);
-      }
-    });
-
-    return result;
-  },
-
   /**
    * Simple form serialization, it will return just what we need
    * Returns key/value pairs from form data
@@ -93,35 +54,37 @@ export default {
       },
     };
     if (this.willUpdateLabels) {
-      formData.update.add_label_ids = this.$labelDropdown.data('marked');
-      formData.update.remove_label_ids = this.$labelDropdown.data('unmarked');
+      formData.update.add_label_ids = this.$labelDropdown.data('user-checked');
+      formData.update.remove_label_ids = this.$labelDropdown.data('user-unchecked');
     }
     return formData;
   },
 
   setOriginalDropdownData() {
     const $labelSelect = $('.bulk-update .js-label-select');
-    const dirtyLabelIds = $labelSelect.data('marked') || [];
-    const chosenLabelIds = [...this.getOriginalMarkedIds(), ...dirtyLabelIds];
-
-    $labelSelect.data('common', this.getOriginalCommonIds());
-    $labelSelect.data('marked', chosenLabelIds);
-    $labelSelect.data('indeterminate', this.getOriginalIndeterminateIds());
+    const userCheckedIds = $labelSelect.data('user-checked') || [];
+    const userUncheckedIds = $labelSelect.data('user-unchecked') || [];
+
+    // Common labels plus user checked labels minus user unchecked labels
+    const checkedIdsToShow = difference(
+      union(this.getOriginalCommonIds(), userCheckedIds),
+      userUncheckedIds,
+    );
+
+    // Indeterminate labels minus user checked labels minus user unchecked labels
+    const indeterminateIdsToShow = difference(
+      this.getOriginalIndeterminateIds(),
+      userCheckedIds,
+      userUncheckedIds,
+    );
+
+    $labelSelect.data('marked', checkedIdsToShow);
+    $labelSelect.data('indeterminate', indeterminateIdsToShow);
   },
 
   // From issuable's initial bulk selection
   getOriginalCommonIds() {
     const labelIds = [];
-
-    this.getElement('.selected-issuable:checked').each((i, el) => {
-      labelIds.push(this.getElement(`#${this.prefixId}${el.dataset.id}`).data('labels'));
-    });
-    return intersection.apply(this, labelIds);
-  },
-
-  // From issuable's initial bulk selection
-  getOriginalMarkedIds() {
-    const labelIds = [];
     this.getElement('.selected-issuable:checked').each((i, el) => {
       labelIds.push(this.getElement(`#${this.prefixId}${el.dataset.id}`).data('labels'));
     });

app/assets/javascripts/labels_select.js

@@ -3,7 +3,7 @@
 /* global ListLabel */
 
 import $ from 'jquery';
-import { isEqual, escape, sortBy, template } from 'lodash';
+import { difference, isEqual, escape, sortBy, template } from 'lodash';
 import { sprintf, s__, __ } from './locale';
 import axios from './lib/utils/axios_utils';
 import IssuableBulkUpdateActions from './issuable_bulk_update_actions';
@@ -560,45 +560,20 @@ export default class LabelsSelect {
     IssuableBulkUpdateActions.willUpdateLabels = true;
   }
   // eslint-disable-next-line class-methods-use-this
-  setDropdownData($dropdown, isMarking, value) {
-    const markedIds = $dropdown.data('marked') || [];
-    const unmarkedIds = $dropdown.data('unmarked') || [];
-    const indeterminateIds = $dropdown.data('indeterminate') || [];
+  setDropdownData($dropdown, isChecking, labelId) {
+    let userCheckedIds = $dropdown.data('user-checked') || [];
+    let userUncheckedIds = $dropdown.data('user-unchecked') || [];
 
-    if (isMarking) {
-      markedIds.push(value);
-
-      let i = indeterminateIds.indexOf(value);
-      if (i > -1) {
-        indeterminateIds.splice(i, 1);
-      }
-
-      i = unmarkedIds.indexOf(value);
-      if (i > -1) {
-        unmarkedIds.splice(i, 1);
-      }
+    if (isChecking) {
+      userCheckedIds = userCheckedIds.concat(labelId);
+      userUncheckedIds = difference(userUncheckedIds, [labelId]);
     } else {
-      // If marked item (not common) is unmarked
-      const i = markedIds.indexOf(value);
-      if (i > -1) {
-        markedIds.splice(i, 1);
-      }
-
-      // If an indeterminate item is being unmarked
-      if (IssuableBulkUpdateActions.getOriginalIndeterminateIds().indexOf(value) > -1) {
-        unmarkedIds.push(value);
-      }
-
-      // If a marked item is being unmarked
-      // (a marked item could also be a label that is present in all selection)
-      if (IssuableBulkUpdateActions.getOriginalCommonIds().indexOf(value) > -1) {
-        unmarkedIds.push(value);
-      }
+      userUncheckedIds = userUncheckedIds.concat(labelId);
+      userCheckedIds = difference(userCheckedIds, [labelId]);
     }
 
-    $dropdown.data('marked', markedIds);
-    $dropdown.data('unmarked', unmarkedIds);
-    $dropdown.data('indeterminate', indeterminateIds);
+    $dropdown.data('user-checked', userCheckedIds);
+    $dropdown.data('user-unchecked', userUncheckedIds);
   }
   // eslint-disable-next-line class-methods-use-this
   setOriginalDropdownData($container, $dropdown) {

changelogs/unreleased/cngo-labels-bug.yml

+---
+title: Fix bulk editing labels bug
+merge_request: 36981
+author:
+type: fixed

spec/features/issues/bulk_assignment_labels_spec.rb

@@ -5,11 +5,12 @@ require 'spec_helper'
 RSpec.describe 'Issues > Labels bulk assignment' do
   let(:user)      { create(:user) }
   let!(:project)  { create(:project) }
-  let!(:issue1)   { create(:issue, project: project, title: "Issue 1") }
-  let!(:issue2)   { create(:issue, project: project, title: "Issue 2") }
   let!(:bug)      { create(:label, project: project, title: 'bug') }
   let!(:feature)  { create(:label, project: project, title: 'feature') }
+  let!(:frontend) { create(:label, project: project, title: 'frontend') }
   let!(:wontfix)  { create(:label, project: project, title: 'wontfix') }
+  let!(:issue1)   { create(:issue, project: project, title: "Issue 1", labels: [frontend]) }
+  let!(:issue2)   { create(:issue, project: project, title: "Issue 2") }
 
   context 'as an allowed user', :js do
     before do
@@ -52,19 +53,50 @@ RSpec.describe 'Issues > Labels bulk assignment' do
           it do
             expect(find("#issue_#{issue1.id}")).to have_content 'bug'
             expect(find("#issue_#{issue2.id}")).to have_content 'bug'
+            expect(find("#issue_#{issue2.id}")).not_to have_content 'frontend'
           end
         end
 
-        context 'to a issue' do
+        context 'to some issues' do
           before do
             check "selected_issue_#{issue1.id}"
+            check "selected_issue_#{issue2.id}"
             open_labels_dropdown ['bug']
             update_issues
           end
 
+          it do
+            expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+            expect(find("#issue_#{issue2.id}")).to have_content 'bug'
+            expect(find("#issue_#{issue2.id}")).not_to have_content 'frontend'
+          end
+        end
+
+        context 'to an issue' do
+          before do
+            check "selected_issue_#{issue1.id}"
+            open_labels_dropdown ['bug']
+            update_issues
+          end
+
+          it do
+            expect(find("#issue_#{issue1.id}")).to have_content 'bug'
+            expect(find("#issue_#{issue2.id}")).not_to have_content 'bug'
+            expect(find("#issue_#{issue2.id}")).not_to have_content 'frontend'
+          end
+        end
+
+        context 'to an issue by selecting the label first' do
+          before do
+            open_labels_dropdown ['bug']
+            check "selected_issue_#{issue1.id}"
+            update_issues
+          end
+
           it do
             expect(find("#issue_#{issue1.id}")).to have_content 'bug'
             expect(find("#issue_#{issue2.id}")).not_to have_content 'bug'
+            expect(find("#issue_#{issue2.id}")).not_to have_content 'frontend'
           end
         end
       end