diff options
author | Alfredo Sumaran <alfredo@gitlab.com> | 2016-11-29 02:47:02 -0500 |
---|---|---|
committer | Alfredo Sumaran <alfredo@gitlab.com> | 2016-12-14 10:05:00 -0500 |
commit | 262fc28a7d1015b0e1349d665d5527ccee29592f (patch) | |
tree | 497ff1b98a60f0cfa5e09499924d4f64ea84fb2b | |
parent | 278baa5b618c5595045415a55d863daf9a15d792 (diff) | |
download | gitlab-ce-262fc28a7d1015b0e1349d665d5527ccee29592f.tar.gz |
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
-rw-r--r-- | app/assets/javascripts/dispatcher.js.es6 | 4 | ||||
-rw-r--r-- | app/assets/javascripts/gl_dropdown.js | 26 | ||||
-rw-r--r-- | app/assets/javascripts/issuable.js.es6 | 4 | ||||
-rw-r--r-- | app/assets/javascripts/issues_bulk_assignment.js.es6 | 92 | ||||
-rw-r--r-- | app/assets/javascripts/labels_select.js | 138 | ||||
-rw-r--r-- | app/views/projects/merge_requests/_merge_request.html.haml | 2 | ||||
-rw-r--r-- | spec/features/issues/bulk_assignment_labels_spec.rb | 42 |
7 files changed, 185 insertions, 123 deletions
diff --git a/app/assets/javascripts/dispatcher.js.es6 b/app/assets/javascripts/dispatcher.js.es6 index 413117c2226..b2ad3d763f6 100644 --- a/app/assets/javascripts/dispatcher.js.es6 +++ b/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': diff --git a/app/assets/javascripts/gl_dropdown.js b/app/assets/javascripts/gl_dropdown.js index 9a91018a8e4..c35e8e93d72 100644 --- a/app/assets/javascripts/gl_dropdown.js +++ b/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() { diff --git a/app/assets/javascripts/issuable.js.es6 b/app/assets/javascripts/issuable.js.es6 index 46503c290ae..5f6c1bbdc90 100644 --- a/app/assets/javascripts/issuable.js.es6 +++ b/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; }, diff --git a/app/assets/javascripts/issues_bulk_assignment.js.es6 b/app/assets/javascripts/issues_bulk_assignment.js.es6 index 9697fb33566..8c0eaac8924 100644 --- a/app/assets/javascripts/issues_bulk_assignment.js.es6 +++ b/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); } } diff --git a/app/assets/javascripts/labels_select.js b/app/assets/javascripts/labels_select.js index f334f35594d..1d79807785f 100644 --- a/app/assets/javascripts/labels_select.js +++ b/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); diff --git a/app/views/projects/merge_requests/_merge_request.html.haml b/app/views/projects/merge_requests/_merge_request.html.haml index fa189ae62d8..53ba124b65e 100644 --- a/app/views/projects/merge_requests/_merge_request.html.haml +++ b/app/views/projects/merge_requests/_merge_request.html.haml @@ -1,4 +1,4 @@ -%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" diff --git a/spec/features/issues/bulk_assignment_labels_spec.rb b/spec/features/issues/bulk_assignment_labels_spec.rb index bc2c087c9b9..01844f9140a 100644 --- a/spec/features/issues/bulk_assignment_labels_spec.rb +++ b/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 |