Merge branch '271459-fix-remove-label-inconsistency' into 'master'

Fix remove label inconsistency

See merge request gitlab-org/gitlab!46805

app/assets/javascripts/graphql_shared/utils.js

@@ -9,4 +9,8 @@
 export const getIdFromGraphQLId = (gid = '') =>
   parseInt((gid || '').replace(/gid:\/\/gitlab\/.*\//g, ''), 10) || null;
 
-export default {};
+export const MutationOperationMode = {
+  Append: 'APPEND',
+  Remove: 'REMOVE',
+  Replace: 'REPLACE',
+};

app/assets/javascripts/sidebar/components/labels/sidebar_labels.vue

 <script>
 import $ from 'jquery';
-import { difference, union } from 'lodash';
-import flash from '~/flash';
-import axios from '~/lib/utils/axios_utils';
+import { camelCase, difference, union } from 'lodash';
+import updateIssueLabelsMutation from '~/boards/queries/issue_set_labels.mutation.graphql';
+import createFlash from '~/flash';
+import { IssuableType } from '~/issue_show/constants';
 import { __ } from '~/locale';
+import updateMergeRequestLabelsMutation from '~/sidebar/queries/update_merge_request_labels.mutation.graphql';
+import { toLabelGid } from '~/sidebar/utils';
 import { DropdownVariant } from '~/vue_shared/components/sidebar/labels_select_vue/constants';
 import LabelsSelect from '~/vue_shared/components/sidebar/labels_select_vue/labels_select_root.vue';
+import { getIdFromGraphQLId, MutationOperationMode } from '~/graphql_shared/utils';
+
+const mutationMap = {
+  [IssuableType.Issue]: {
+    mutation: updateIssueLabelsMutation,
+    mutationName: 'updateIssue',
+  },
+  [IssuableType.MergeRequest]: {
+    mutation: updateMergeRequestLabelsMutation,
+    mutationName: 'mergeRequestSetLabels',
+  },
+};
 
 export default {
   components: {
@@ -21,7 +36,6 @@ export default {
     'issuableType',
     'labelsFetchPath',
     'labelsManagePath',
-    'labelsUpdatePath',
     'projectIssuesPath',
     'projectPath',
   ],
@@ -35,37 +49,79 @@ export default {
     handleDropdownClose() {
       $(this.$el).trigger('hidden.gl.dropdown');
     },
-    handleUpdateSelectedLabels(dropdownLabels) {
+    getUpdateVariables(dropdownLabels) {
       const currentLabelIds = this.selectedLabels.map(label => label.id);
       const userAddedLabelIds = dropdownLabels.filter(label => label.set).map(label => label.id);
       const userRemovedLabelIds = dropdownLabels.filter(label => !label.set).map(label => label.id);
 
       const labelIds = difference(union(currentLabelIds, userAddedLabelIds), userRemovedLabelIds);
 
-      this.updateSelectedLabels(labelIds);
+      switch (this.issuableType) {
+        case IssuableType.Issue:
+          return {
+            addLabelIds: userAddedLabelIds,
+            iid: this.iid,
+            projectPath: this.projectPath,
+            removeLabelIds: userRemovedLabelIds,
+          };
+        case IssuableType.MergeRequest:
+          return {
+            iid: this.iid,
+            labelIds: labelIds.map(toLabelGid),
+            operationMode: MutationOperationMode.Replace,
+            projectPath: this.projectPath,
+          };
+        default:
+          return {};
+      }
+    },
+    handleUpdateSelectedLabels(dropdownLabels) {
+      this.updateSelectedLabels(this.getUpdateVariables(dropdownLabels));
+    },
+    getRemoveVariables(labelId) {
+      switch (this.issuableType) {
+        case IssuableType.Issue:
+          return {
+            iid: this.iid,
+            projectPath: this.projectPath,
+            removeLabelIds: [labelId],
+          };
+        case IssuableType.MergeRequest:
+          return {
+            iid: this.iid,
+            labelIds: [toLabelGid(labelId)],
+            operationMode: MutationOperationMode.Remove,
+            projectPath: this.projectPath,
+          };
+        default:
+          return {};
+      }
     },
     handleLabelRemove(labelId) {
-      const currentLabelIds = this.selectedLabels.map(label => label.id);
-      const labelIds = difference(currentLabelIds, [labelId]);
-
-      this.updateSelectedLabels(labelIds);
+      this.updateSelectedLabels(this.getRemoveVariables(labelId));
     },
-    updateSelectedLabels(labelIds) {
+    updateSelectedLabels(inputVariables) {
       this.isLabelsSelectInProgress = true;
 
-      axios({
-        data: {
-          [this.issuableType]: {
-            label_ids: labelIds,
-          },
-        },
-        method: 'put',
-        url: this.labelsUpdatePath,
+      this.$apollo
+        .mutate({
+          mutation: mutationMap[this.issuableType].mutation,
+          variables: { input: inputVariables },
         })
         .then(({ data }) => {
-          this.selectedLabels = data.labels;
+          const { mutationName } = mutationMap[this.issuableType];
+
+          if (data[mutationName]?.errors?.length) {
+            throw new Error();
+          }
+
+          const issuableType = camelCase(this.issuableType);
+          this.selectedLabels = data[mutationName]?.[issuableType]?.labels?.nodes?.map(label => ({
+            ...label,
+            id: getIdFromGraphQLId(label.id),
+          }));
         })
-        .catch(() => flash(__('An error occurred while updating labels.')))
+        .catch(() => createFlash({ message: __('An error occurred while updating labels.') }))
         .finally(() => {
           this.isLabelsSelectInProgress = false;
         });

app/assets/javascripts/sidebar/mount_sidebar.js

@@ -91,8 +91,13 @@ export function mountSidebarLabels() {
     return false;
   }
 
+  const apolloProvider = new VueApollo({
+    defaultClient: createDefaultClient(),
+  });
+
   return new Vue({
     el,
+    apolloProvider,
     provide: {
       ...el.dataset,
       allowLabelCreate: parseBoolean(el.dataset.allowLabelCreate),

app/assets/javascripts/sidebar/queries/update_merge_request_labels.mutation.graphql

+mutation mergeRequestSetLabels($input: MergeRequestSetLabelsInput!) {
+  mergeRequestSetLabels(input: $input) {
+    errors
+    mergeRequest {
+      labels {
+        nodes {
+          color
+          description
+          id
+          title
+        }
+      }
+    }
+  }
+}

app/assets/javascripts/sidebar/utils.js

+export const toLabelGid = id => `gid://gitlab/Label/${id}`;

app/helpers/issuables_helper.rb

@@ -492,6 +492,21 @@ module IssuablesHelper
     }
   end
 
+  def sidebar_labels_data(issuable_sidebar, project)
+    {
+      allow_label_create: issuable_sidebar.dig(:current_user, :can_admin_label).to_s,
+      allow_scoped_labels: issuable_sidebar[:scoped_labels_available].to_s,
+      can_edit: issuable_sidebar.dig(:current_user, :can_edit).to_s,
+      iid: issuable_sidebar[:iid],
+      issuable_type: issuable_sidebar[:type],
+      labels_fetch_path: issuable_sidebar[:project_labels_path],
+      labels_manage_path: project_labels_path(project),
+      project_issues_path: issuable_sidebar[:project_issuables_path],
+      project_path: project.full_path,
+      selected_labels: issuable_sidebar[:labels].to_json
+    }
+  end
+
   def parent
     @project || @group
   end

app/views/shared/issuable/_sidebar.html.haml

@@ -103,17 +103,7 @@
                     .js-due-date-calendar
 
 
-      .js-sidebar-labels{ data: { allow_label_create: issuable_sidebar.dig(:current_user, :can_admin_label).to_s,
-        allow_scoped_labels: issuable_sidebar[:scoped_labels_available].to_s,
-        can_edit: can_edit_issuable.to_s,
-        iid: issuable_sidebar[:iid],
-        issuable_type: issuable_type,
-        labels_fetch_path: issuable_sidebar[:project_labels_path],
-        labels_manage_path: project_labels_path(@project),
-        labels_update_path: issuable_sidebar[:issuable_json_path],
-        project_issues_path: issuable_sidebar[:project_issuables_path],
-        project_path: @project.full_path,
-        selected_labels: issuable_sidebar[:labels].to_json } }
+      .js-sidebar-labels{ data: sidebar_labels_data(issuable_sidebar, @project) }
 
       = render_if_exists 'shared/issuable/sidebar_weight', issuable_sidebar: issuable_sidebar
 

changelogs/unreleased/271459-fix-remove-label-inconsistency.yml

+---
+title: Fix remove label inconsistency
+merge_request: 46805
+author:
+type: fixed

spec/features/issues/user_edits_issue_spec.rb

@@ -138,6 +138,33 @@ RSpec.describe "Issues > User edits issue", :js do
             expect(page).not_to have_text('verisimilitude')
           end
         end
+
+        it 'can remove label without removing label added via quick action', :aggregate_failures do
+          # Add `syzygy` label with a quick action
+          note = find('#note-body')
+          page.within '.timeline-content-form' do
+            note.native.send_keys('/label ~syzygy')
+          end
+          click_button 'Comment'
+
+          wait_for_requests
+
+          page.within '.block.labels' do
+            # Remove `verisimilitude` label
+            within '.gl-label' do
+              click_button
+            end
+
+            wait_for_requests
+
+            expect(page).to have_text('syzygy')
+            expect(page).not_to have_text('verisimilitude')
+          end
+
+          expect(page).to have_text('removed verisimilitude label')
+          expect(page).not_to have_text('removed syzygy verisimilitude labels')
+          expect(issue.reload.labels.map(&:title)).to contain_exactly('syzygy')
+        end
       end
 
       describe 'update assignee' do

spec/frontend/sidebar/sidebar_labels_spec.js

 import { shallowMount } from '@vue/test-utils';
-import AxiosMockAdapter from 'axios-mock-adapter';
 import {
   mockLabels,
   mockRegularLabel,
 } from 'jest/vue_shared/components/sidebar/labels_select_vue/mock_data';
-import axios from '~/lib/utils/axios_utils';
+import updateIssueLabelsMutation from '~/boards/queries/issue_set_labels.mutation.graphql';
+import { MutationOperationMode } from '~/graphql_shared/utils';
+import { IssuableType } from '~/issue_show/constants';
 import SidebarLabels from '~/sidebar/components/labels/sidebar_labels.vue';
+import updateMergeRequestLabelsMutation from '~/sidebar/queries/update_merge_request_labels.mutation.graphql';
+import { toLabelGid } from '~/sidebar/utils';
 import { DropdownVariant } from '~/vue_shared/components/sidebar/labels_select_vue/constants';
 import LabelsSelect from '~/vue_shared/components/sidebar/labels_select_vue/labels_select_root.vue';
 
 describe('sidebar labels', () => {
-  let axiosMock;
   let wrapper;
 
   const defaultProps = {
@@ -23,29 +25,52 @@ describe('sidebar labels', () => {
     issuableType: 'issue',
     labelsFetchPath: '/gitlab-org/gitlab-test/-/labels.json?include_ancestor_groups=true',
     labelsManagePath: '/gitlab-org/gitlab-test/-/labels',
-    labelsUpdatePath: '/gitlab-org/gitlab-test/-/issues/1.json',
     projectIssuesPath: '/gitlab-org/gitlab-test/-/issues',
     projectPath: 'gitlab-org/gitlab-test',
   };
 
+  const $apollo = {
+    mutate: jest.fn().mockResolvedValue(),
+  };
+
+  const userUpdatedLabels = [
+    {
+      ...mockRegularLabel,
+      set: false,
+    },
+    {
+      id: 40,
+      title: 'Security',
+      color: '#ddd',
+      text_color: '#fff',
+      set: true,
+    },
+    {
+      id: 55,
+      title: 'Tooling',
+      color: '#ddd',
+      text_color: '#fff',
+      set: false,
+    },
+  ];
+
   const findLabelsSelect = () => wrapper.find(LabelsSelect);
 
-  const mountComponent = () => {
+  const mountComponent = (props = {}) => {
     wrapper = shallowMount(SidebarLabels, {
       provide: {
         ...defaultProps,
+        ...props,
+      },
+      mocks: {
+        $apollo,
       },
     });
   };
 
-  beforeEach(() => {
-    axiosMock = new AxiosMockAdapter(axios);
-  });
-
   afterEach(() => {
     wrapper.destroy();
     wrapper = null;
-    axiosMock.restore();
   });
 
   describe('LabelsSelect props', () => {
@@ -72,64 +97,94 @@ describe('sidebar labels', () => {
     });
   });
 
-  describe('when labels are updated', () => {
+  describe('when type is issue', () => {
     beforeEach(() => {
-      mountComponent();
+      mountComponent({ issuableType: IssuableType.Issue });
     });
 
-    it('makes an API call to update labels', async () => {
-      const labels = [
-        {
-          ...mockRegularLabel,
-          set: false,
-        },
-        {
-          id: 40,
-          title: 'Security',
-          color: '#ddd',
-          text_color: '#fff',
-          set: true,
+    describe('when labels are updated', () => {
+      it('invokes a mutation', () => {
+        findLabelsSelect().vm.$emit('updateSelectedLabels', userUpdatedLabels);
+
+        const expected = {
+          mutation: updateIssueLabelsMutation,
+          variables: {
+            input: {
+              addLabelIds: [40],
+              iid: defaultProps.iid,
+              projectPath: defaultProps.projectPath,
+              removeLabelIds: [26, 55],
             },
-        {
-          id: 55,
-          title: 'Tooling',
-          color: '#ddd',
-          text_color: '#fff',
-          set: false,
           },
-      ];
+        };
 
-      findLabelsSelect().vm.$emit('updateSelectedLabels', labels);
+        expect($apollo.mutate).toHaveBeenCalledWith(expected);
+      });
+    });
 
-      await axios.waitForAll();
+    describe('when label `x` is clicked', () => {
+      it('invokes a mutation', () => {
+        findLabelsSelect().vm.$emit('onLabelRemove', 27);
 
         const expected = {
-        [defaultProps.issuableType]: {
-          label_ids: [27, 28, 29, 40],
+          mutation: updateIssueLabelsMutation,
+          variables: {
+            input: {
+              iid: defaultProps.iid,
+              projectPath: defaultProps.projectPath,
+              removeLabelIds: [27],
+            },
           },
         };
 
-      expect(axiosMock.history.put[0].data).toEqual(JSON.stringify(expected));
+        expect($apollo.mutate).toHaveBeenCalledWith(expected);
+      });
     });
   });
 
-  describe('when label `x` is clicked', () => {
+  describe('when type is merge_request', () => {
     beforeEach(() => {
-      mountComponent();
+      mountComponent({ issuableType: IssuableType.MergeRequest });
     });
 
-    it('makes an API call to update labels', async () => {
-      findLabelsSelect().vm.$emit('onLabelRemove', 27);
+    describe('when labels are updated', () => {
+      it('invokes a mutation', () => {
+        findLabelsSelect().vm.$emit('updateSelectedLabels', userUpdatedLabels);
 
-      await axios.waitForAll();
+        const expected = {
+          mutation: updateMergeRequestLabelsMutation,
+          variables: {
+            input: {
+              iid: defaultProps.iid,
+              labelIds: [toLabelGid(27), toLabelGid(28), toLabelGid(29), toLabelGid(40)],
+              operationMode: MutationOperationMode.Replace,
+              projectPath: defaultProps.projectPath,
+            },
+          },
+        };
+
+        expect($apollo.mutate).toHaveBeenCalledWith(expected);
+      });
+    });
+
+    describe('when label `x` is clicked', () => {
+      it('invokes a mutation', () => {
+        findLabelsSelect().vm.$emit('onLabelRemove', 27);
 
         const expected = {
-        [defaultProps.issuableType]: {
-          label_ids: [26, 28, 29],
+          mutation: updateMergeRequestLabelsMutation,
+          variables: {
+            input: {
+              iid: defaultProps.iid,
+              labelIds: [toLabelGid(27)],
+              operationMode: MutationOperationMode.Remove,
+              projectPath: defaultProps.projectPath,
+            },
           },
         };
 
-      expect(axiosMock.history.put[0].data).toEqual(JSON.stringify(expected));
+        expect($apollo.mutate).toHaveBeenCalledWith(expected);
+      });
     });
   });
 });