Improve issuable's bulk assignment implementation

This fixes the case when the user wants to add a label. The user has to use the dropdown’s filter input to look for a label and click it in order to see the bug.

Step to reproduce

- Select at least two issues, one label should be present in all issues, other label should be present in at least one.

- On the label dropdown: Deselect label that is present in all issues, look for another issue using the filter input and click it.

- Click on `Update issues`

Before: Unmarked label were kept on selected issues.
Now: Unmarked label is removed from selected issues

app/assets/javascripts/dispatcher.js.es6

@@ -36,7 +36,9 @@
         case 'projects:merge_requests:index':
         case 'projects:issues:index':
           Issuable.init();
-          new gl.IssuableBulkActions();
+          new gl.IssuableBulkActions({
+            page
+          });
           shortcut_handler = new ShortcutsNavigation();
           break;
         case 'projects:issues:show':

app/assets/javascripts/gl_dropdown.js

@@ -20,7 +20,6 @@
       this.filterInputBlur = (ref = this.options.filterInputBlur) != null ? ref : true;
       $inputContainer = this.input.parent();
       $clearButton = $inputContainer.find('.js-dropdown-input-clear');
-      this.indeterminateIds = [];
       $clearButton.on('click', (function(_this) {
         // Clear click
         return function(e) {
@@ -345,12 +344,12 @@
           $el = $(this);
           selected = self.rowClicked($el);
           if (self.options.clicked) {
-            self.options.clicked(selected, $el, e);
+            self.options.clicked(selected[0], $el, e, selected[1]);
           }
 
           // Update label right after all modifications in dropdown has been done
           if (self.options.toggleLabel) {
-            self.updateLabel(selected, $el, self);
+            self.updateLabel(selected[0], $el, self);
           }
 
           $el.trigger('blur');
@@ -441,12 +440,6 @@
       this.resetRows();
       this.addArrowKeyEvent();
 
-      if (this.options.setIndeterminateIds) {
-        this.options.setIndeterminateIds.call(this);
-      }
-      if (this.options.setActiveIds) {
-        this.options.setActiveIds.call(this);
-      }
       // Makes indeterminate items effective
       if (this.fullData && this.dropdown.find('.dropdown-menu-toggle').hasClass('js-filter-bulk-update')) {
         this.parseData(this.fullData);
@@ -480,11 +473,6 @@
       if (this.options.filterable) {
         $input.blur().val("");
       }
-      // Triggering 'keyup' will re-render the dropdown which is not always required
-      // specially if we want to keep the state of the dropdown needed for bulk-assignment
-      if (!this.options.persistWhenHide) {
-        $input.trigger("input");
-      }
       if (this.dropdown.find(".dropdown-toggle-page").length) {
         $('.dropdown-menu', this.dropdown).removeClass(PAGE_TWO_CLASS);
       }
@@ -617,7 +605,8 @@
     };
 
     GitLabDropdown.prototype.rowClicked = function(el) {
-      var field, fieldName, groupName, isInput, selectedIndex, selectedObject, value;
+      var field, fieldName, groupName, isInput, selectedIndex, selectedObject, value, markedIds, unmarkedIds, i, isMarking;
+
       fieldName = this.options.fieldName;
       isInput = $(this.el).is('input');
       if (this.renderedData) {
@@ -638,7 +627,7 @@
           el.addClass(ACTIVE_CLASS);
         }
 
-        return selectedObject;
+        return [selectedObject];
       }
 
       field = [];
@@ -656,6 +645,7 @@
       }
 
       if (el.hasClass(ACTIVE_CLASS)) {
+        isMarking = false;
         el.removeClass(ACTIVE_CLASS);
         if (field && field.length) {
           if (isInput) {
@@ -665,6 +655,7 @@
           }
         }
       } else if (el.hasClass(INDETERMINATE_CLASS)) {
+        isMarking = true;
         el.addClass(ACTIVE_CLASS);
         el.removeClass(INDETERMINATE_CLASS);
         if (field && field.length && value == null) {
@@ -674,6 +665,7 @@
           this.addInput(fieldName, value, selectedObject);
         }
       } else {
+        isMarking = true;
         if (!this.options.multiSelect || el.hasClass('dropdown-clear-active')) {
           this.dropdown.find("." + ACTIVE_CLASS).removeClass(ACTIVE_CLASS);
           if (!isInput) {
@@ -694,7 +686,7 @@
         }
       }
 
-      return selectedObject;
+      return [selectedObject, isMarking];
     };
 
     GitLabDropdown.prototype.focusTextInput = function() {

app/assets/javascripts/issuable.js.es6

@@ -141,6 +141,9 @@
       const $issuesOtherFilters = $('.issues-other-filters');
       const $issuesBulkUpdate = $('.issues_bulk_update');
 
+      this.issuableBulkActions.willUpdateLabels = false;
+      this.issuableBulkActions.setOriginalDropdownData();
+
       if ($checkedIssues.length > 0) {
         let ids = $.map($checkedIssues, function(value) {
           return $(value).data('id');
@@ -152,7 +155,6 @@
         $updateIssuesIds.val([]);
         $issuesBulkUpdate.hide();
         $issuesOtherFilters.show();
-        this.issuableBulkActions.willUpdateLabels = false;
       }
       return true;
     },

app/assets/javascripts/issues_bulk_assignment.js.es6

@@ -2,9 +2,10 @@
 ((global) => {
 
   class IssuableBulkActions {
-    constructor({ container, form, issues } = {}) {
-      this.container = container || $('.content'),
+    constructor({ container, form, issues, page } = {}) {
+      this.prefixId = page === 'projects:merge_requests:index' ? 'merge_request_' : 'issue_';
       this.form = form || this.getElement('.bulk-update');
+      this.$labelDropdown = this.form.find('.js-label-select');
       this.issues = issues || this.getElement('.issues-list .issue');
       this.form.data('bulkActions', this);
       this.willUpdateLabels = false;
@@ -13,10 +14,6 @@
       Issuable.initChecks();
     }
 
-    getElement(selector) {
-      return this.container.find(selector);
-    }
-
     bindEvents() {
       return this.form.off('submit').on('submit', this.onFormSubmit.bind(this));
     }
@@ -70,10 +67,7 @@
 
     getUnmarkedIndeterminedLabels() {
       const result = [];
-      const labelsToKeep = [];
-
-      this.getElement('.labels-filter .is-indeterminate')
-        .each((i, el) => labelsToKeep.push($(el).data('labelId')));
+      const labelsToKeep = this.$labelDropdown.data('indeterminate');
 
       this.getLabelsFromSelection().forEach((id) => {
         if (labelsToKeep.indexOf(id) === -1) {
@@ -103,45 +97,63 @@
         }
       };
       if (this.willUpdateLabels) {
-        this.getLabelsToApply().map(function(id) {
-          return formData.update.add_label_ids.push(id);
-        });
-        this.getLabelsToRemove().map(function(id) {
-          return formData.update.remove_label_ids.push(id);
-        });
+        formData.update.add_label_ids = this.$labelDropdown.data('marked');
+        formData.update.remove_label_ids = this.$labelDropdown.data('unmarked');
       }
       return formData;
     }
 
-    getLabelsToApply() {
-      const labelIds = [];
-      const $labels = this.form.find('.labels-filter input[name="update[label_ids][]"]');
-      $labels.each(function(k, label) {
-        if (label) {
-          return labelIds.push(parseInt($(label).val()));
-        }
-      });
-      return labelIds;
+    setOriginalDropdownData() {
+      $('.bulk-update .js-label-select').data('common', this.getOriginalCommonIds());
+      $('.bulk-update .js-label-select').data('marked', this.getOriginalMarkedIds());
+      $('.bulk-update .js-label-select').data('indeterminate', this.getOriginalIndeterminateIds());
     }
 
+    // From issuable's initial bulk selection
+    getOriginalCommonIds() {
+      let labelIds = [];
 
-    /**
-     * Returns Label IDs that will be removed from issue selection
-     * @return {Array} Array of labels IDs
-     */
+      this.getElement('.selected_issue:checked').each((i, el) => {
+        labelIds.push(this.getElement(`#${this.prefixId}${el.dataset.id}`).data('labels'));
+      });
+      return _.intersection.apply(this, labelIds);
+    }
 
-    getLabelsToRemove() {
-      const result = [];
-      const indeterminatedLabels = this.getUnmarkedIndeterminedLabels();
-      const labelsToApply = this.getLabelsToApply();
-      indeterminatedLabels.map(function(id) {
-        // We need to exclude label IDs that will be applied
-        // By not doing this will cause issues from selection to not add labels at all
-        if (labelsToApply.indexOf(id) === -1) {
-          return result.push(id);
-        }
+    // From issuable's initial bulk selection
+    getOriginalMarkedIds() {
+      var labelIds = [];
+      this.getElement('.selected_issue:checked').each((i, el) => {
+        labelIds.push(this.getElement(`#${this.prefixId}${el.dataset.id}`).data('labels'));
       });
-      return result;
+      return _.intersection.apply(_, labelIds);
+    }
+
+    // From issuable's initial bulk selection
+    getOriginalIndeterminateIds() {
+      let uniqueIds = [];
+      let labelIds = [];
+      let issuableLabels = [];
+
+      // Collect unique label IDs for all checked issues
+      this.getElement('.selected_issue:checked').each((i, el) => {
+        issuableLabels = this.getElement(`#${this.prefixId}${el.dataset.id}`).data('labels');
+        issuableLabels.forEach((labelId) => {
+          // Store unique IDs
+          if (uniqueIds.indexOf(labelId) === -1) {
+            uniqueIds.push(labelId);
+          }
+        });
+        // Store array of IDs per issuable
+        labelIds.push(issuableLabels);
+      });
+      // Add uniqueIds to add it as argument for _.intersection
+      labelIds.unshift(uniqueIds);
+      // Return IDs that are present but not in all selected issueables
+      return _.difference(uniqueIds, _.intersection.apply(this, labelIds));
+    }
+
+    getElement(selector) {
+      return $('.content').find(selector);
     }
   }
 

app/assets/javascripts/labels_select.js

@@ -5,8 +5,9 @@
       var _this;
       _this = this;
       $('.js-label-select').each(function(i, dropdown) {
-        var $block, $colorPreview, $dropdown, $form, $loading, $selectbox, $sidebarCollapsedValue, $value, abilityName, defaultLabel, enableLabelCreateButton, issueURLSplit, issueUpdateURL, labelHTMLTemplate, labelNoneHTMLTemplate, labelUrl, namespacePath, projectPath, saveLabelData, selectedLabel, showAny, showNo, $sidebarLabelTooltip, initialSelected, $toggleText, fieldName, useId, propertyName, showMenuAbove;
+        var $block, $colorPreview, $dropdown, $form, $loading, $selectbox, $sidebarCollapsedValue, $value, abilityName, defaultLabel, enableLabelCreateButton, issueURLSplit, issueUpdateURL, labelHTMLTemplate, labelNoneHTMLTemplate, labelUrl, namespacePath, projectPath, saveLabelData, selectedLabel, showAny, showNo, $sidebarLabelTooltip, initialSelected, $toggleText, fieldName, useId, propertyName, showMenuAbove, $container;
         $dropdown = $(dropdown);
+        $dropdownContainer = $dropdown.closest('.labels-filter')
         $toggleText = $dropdown.find('.dropdown-toggle-text');
         namespacePath = $dropdown.data('namespace-path');
         projectPath = $dropdown.data('project-path');
@@ -122,7 +123,7 @@
             });
           });
         };
-        return $dropdown.glDropdown({
+        $dropdown.glDropdown({
           showMenuAbove: showMenuAbove,
           data: function(term, callback) {
             return $.ajax({
@@ -169,33 +170,35 @@
             });
           },
           renderRow: function(label, instance) {
-            var $a, $li, active, color, colorEl, indeterminate, removesAll, selectedClass, spacing;
+            var $a, $li, color, colorEl, indeterminate, removesAll, selectedClass, spacing, i, marked;
             $li = $('<li>');
             $a = $('<a href="#">');
             selectedClass = [];
             removesAll = label.id <= 0 || (label.id == null);
             if ($dropdown.hasClass('js-filter-bulk-update')) {
-              indeterminate = instance.indeterminateIds;
-              active = instance.activeIds;
+              indeterminate = $dropdown.data('indeterminate') || [];
+              marked = $dropdown.data('marked') || [];
+
               if (indeterminate.indexOf(label.id) !== -1) {
                 selectedClass.push('is-indeterminate');
               }
-              if (active.indexOf(label.id) !== -1) {
+
+              if (marked.indexOf(label.id) !== -1) {
                 // Remove is-indeterminate class if the item will be marked as active
                 i = selectedClass.indexOf('is-indeterminate');
                 if (i !== -1) {
                   selectedClass.splice(i, 1);
                 }
                 selectedClass.push('is-active');
-                // Add input manually
-                instance.addInput(this.fieldName, label.id);
               }
-            }
-            if (this.id(label) && $form.find("input[type='hidden'][name='" + ($dropdown.data('fieldName')) + "'][value='" + this.id(label).toString().replace(/'/g, '\\\'') + "']").length) {
-              selectedClass.push('is-active');
-            }
-            if ($dropdown.hasClass('js-multiselect') && removesAll) {
-              selectedClass.push('dropdown-clear-active');
+            } else {
+              if (this.id(label) && $form.find("input[type='hidden'][name='" + ($dropdown.data('fieldName')) + "'][value='" + this.id(label).toString().replace(/'/g, '\\\'') + "']").length) {
+                selectedClass.push('is-active');
+              }
+
+              if ($dropdown.hasClass('js-multiselect') && removesAll) {
+                selectedClass.push('dropdown-clear-active');
+              }
             }
             if (label.duplicate) {
               spacing = 100 / label.color.length;
@@ -231,7 +234,6 @@
             // Return generated html
             return $li.html($a).prop('outerHTML');
           },
-          persistWhenHide: $dropdown.data('persistWhenHide'),
           search: {
             fields: ['title']
           },
@@ -310,18 +312,15 @@
                 }
               }
             }
-            if ($dropdown.hasClass('js-filter-bulk-update')) {
-              // If we are persisting state we need the classes
-              if (!this.options.persistWhenHide) {
-                return $dropdown.parent().find('.is-active, .is-indeterminate').removeClass();
-              }
-            }
           },
           multiSelect: $dropdown.hasClass('js-multiselect'),
           vue: $dropdown.hasClass('js-issue-board-sidebar'),
-          clicked: function(label, $el, e) {
+          clicked: function(label, $el, e, isMarking) {
             var isIssueIndex, isMRIndex, page;
-            _this.enableBulkLabelDropdown();
+
+            page = $('body').data('page');
+            isIssueIndex = page === 'projects:issues:index';
+            isMRIndex = page === 'projects:merge_requests:index';
 
             if ($dropdown.parent().find('.is-active:not(.dropdown-clear-active)').length) {
               $dropdown.parent()
@@ -330,12 +329,11 @@
             }
 
             if ($dropdown.hasClass('js-filter-bulk-update') || $dropdown.hasClass('js-issuable-form-dropdown')) {
+              _this.enableBulkLabelDropdown();
+              _this.setDropdownData($dropdown, isMarking, this.id(label));
               return;
             }
 
-            page = $('body').data('page');
-            isIssueIndex = page === 'projects:issues:index';
-            isMRIndex = page === 'projects:merge_requests:index';
             if ($('html').hasClass('issue-boards-page') && !$dropdown.hasClass('js-issue-board-sidebar')) {
               if (label.isAny) {
                 gl.issueBoards.BoardsStore.state.filters['label_name'] = [];
@@ -397,17 +395,10 @@
               }
             }
           },
-          setIndeterminateIds: function() {
-            if (this.dropdown.find('.dropdown-menu-toggle').hasClass('js-filter-bulk-update')) {
-              return this.indeterminateIds = _this.getIndeterminateIds();
-            }
-          },
-          setActiveIds: function() {
-            if (this.dropdown.find('.dropdown-menu-toggle').hasClass('js-filter-bulk-update')) {
-              return this.activeIds = _this.getActiveIds();
-            }
-          }
         });
+
+        // Set dropdown data
+        _this.setOriginalDropdownData($dropdownContainer, $dropdown);
       });
       this.bindEvents();
     }
@@ -420,34 +411,9 @@
       if ($('.selected_issue:checked').length) {
         return;
       }
-      // Remove inputs
-      $('.issues_bulk_update .labels-filter input[type="hidden"]').remove();
-      // Also restore button text
       return $('.issues_bulk_update .labels-filter .dropdown-toggle-text').text('Label');
     };
 
-    LabelsSelect.prototype.getIndeterminateIds = function() {
-      var label_ids;
-      label_ids = [];
-      $('.selected_issue:checked').each(function(i, el) {
-        var issue_id;
-        issue_id = $(el).data('id');
-        return label_ids.push($("#issue_" + issue_id).data('labels'));
-      });
-      return _.flatten(label_ids);
-    };
-
-    LabelsSelect.prototype.getActiveIds = function() {
-      var label_ids;
-      label_ids = [];
-      $('.selected_issue:checked').each(function(i, el) {
-        var issue_id;
-        issue_id = $(el).data('id');
-        return label_ids.push($("#issue_" + issue_id).data('labels'));
-      });
-      return _.intersection.apply(_, label_ids);
-    };
-
     LabelsSelect.prototype.enableBulkLabelDropdown = function() {
       var issuableBulkActions;
       if ($('.selected_issue:checked').length) {
@@ -456,8 +422,56 @@
       }
     };
 
-    return LabelsSelect;
+    LabelsSelect.prototype.setDropdownData = function($dropdown, isMarking, value) {
+      var issuableBulkActions = $('.bulk-update').data('bulkActions');
 
+      markedIds = $dropdown.data('marked') || [];
+      unmarkedIds = $dropdown.data('unmarked') || [];
+      indeterminateIds = $dropdown.data('indeterminate') || [];
+
+      if (isMarking) {
+        markedIds.push(value);
+
+        i = indeterminateIds.indexOf(value);
+        if (i > -1) {
+          indeterminateIds.splice(i, 1);
+        }
+
+        i = unmarkedIds.indexOf(value);
+        if (i > -1) {
+          unmarkedIds.splice(i, 1);
+        }
+      } else {
+        // If marked item (not common) is unmarked
+        i = markedIds.indexOf(value);
+        if (i > -1) {
+          markedIds.splice(i, 1);
+        }
+
+        // If an indeterminate item is being unmarked
+        if (issuableBulkActions.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 (issuableBulkActions.getOriginalCommonIds().indexOf(value) > -1) {
+          unmarkedIds.push(value);
+        }
+      }
+
+      $dropdown.data('marked', markedIds);
+      $dropdown.data('unmarked', unmarkedIds);
+      $dropdown.data('indeterminate', indeterminateIds);
+    };
+
+    LabelsSelect.prototype.setOriginalDropdownData = function($container, $dropdown) {
+      var labels = [];
+      $container.find('[name="label_name[]"]').map(function() { return labels.push(this.value); });
+      $dropdown.data('marked', labels);
+    };
+
+    return LabelsSelect;
   })();
 
 }).call(this);

app/views/projects/merge_requests/_merge_request.html.haml

-%li{ class: mr_css_classes(merge_request) }
+%li{ id: dom_id(merge_request), class: mr_css_classes(merge_request), data: { labels: merge_request.label_ids, id: merge_request.id } }
   - if @bulk_edit
     .issue-check
       = check_box_tag dom_id(merge_request, "selected"), nil, false, 'data-id' => merge_request.id, class: "selected_issue"

spec/features/issues/bulk_assignment_labels_spec.rb

@@ -9,6 +9,7 @@ feature 'Issues > Labels bulk assignment', feature: true do
   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!(:wontfix)  { create(:label, project: project, title: 'wontfix') }
 
   context 'as an allowed user', js: true do
     before do
@@ -291,6 +292,45 @@ feature 'Issues > Labels bulk assignment', feature: true do
         expect(find("#issue_#{issue1.id}")).not_to have_content 'feature'
       end
     end
+
+    # Special case https://gitlab.com/gitlab-org/gitlab-ce/issues/24877
+    context 'unmarking common label', focus: true do
+      before do
+        issue1.labels << bug
+        issue1.labels << feature
+        issue2.labels << bug
+
+        visit namespace_project_issues_path(project.namespace, project)
+      end
+
+      it 'applies label from filtered results' do
+        check 'check_all_issues'
+
+        page.within('.issues_bulk_update') do
+          click_button 'Labels'
+          wait_for_ajax
+
+          expect(find('.dropdown-menu-labels li', text: 'bug')).to have_css('.is-active')
+          expect(find('.dropdown-menu-labels li', text: 'feature')).to have_css('.is-indeterminate')
+
+          click_link 'bug'
+          find('.dropdown-input-field', visible: true).set('wontfix');
+          click_link 'wontfix'
+        end
+
+        update_issues
+
+        page.within '.issues-holder' do
+          expect(find("#issue_#{issue1.id}")).not_to have_content 'bug'
+          expect(find("#issue_#{issue1.id}")).to have_content 'feature'
+          expect(find("#issue_#{issue1.id}")).to have_content 'wontfix'
+
+          expect(find("#issue_#{issue2.id}")).not_to have_content 'bug'
+          expect(find("#issue_#{issue2.id}")).not_to have_content 'feature'
+          expect(find("#issue_#{issue2.id}")).to have_content 'wontfix'
+        end
+      end
+    end
   end
 
   context 'as a guest' do
@@ -320,7 +360,7 @@ feature 'Issues > Labels bulk assignment', feature: true do
 
   def open_labels_dropdown(items = [], unmark = false)
     page.within('.issues_bulk_update') do
-      click_button 'Label'
+      click_button 'Labels'
       wait_for_ajax
       items.map do |item|
         click_link item