Merge branch '271251-fe-devops-report-add-edit-and-delete-segment-functionality-2' into 'master'

DevOps Report: Edit segment functionality

See merge request gitlab-org/gitlab!49338

ee/app/assets/javascripts/admin/dev_ops_report/components/devops_adoption_app.vue

@@ -37,7 +37,7 @@ export default {
     return {
       isLoadingGroups: false,
       requestCount: 0,
-      selectedSegmentId: null,
+      selectedSegment: null,
       errors: {
         [DEVOPS_ADOPTION_ERROR_KEYS.groups]: false,
         [DEVOPS_ADOPTION_ERROR_KEYS.segments]: false,
@@ -75,6 +75,9 @@ export default {
     isLoading() {
       return this.isLoadingGroups || this.$apollo.queries.devopsAdoptionSegments.loading;
     },
+    modalKey() {
+      return this.selectedSegment?.id;
+    },
   },
   created() {
     this.fetchGroups();
@@ -111,6 +114,12 @@ export default {
         })
         .catch(error => this.handleError(DEVOPS_ADOPTION_ERROR_KEYS.groups, error));
     },
+    setSelectedSegment(segment) {
+      this.selectedSegment = segment;
+    },
+    clearSelectedSegment() {
+      this.selectedSegment = null;
+    },
   },
 };
 </script>
@@ -126,8 +135,9 @@ export default {
   <div v-else>
     <devops-adoption-segment-modal
       v-if="hasGroupData"
+      :key="modalKey"
       :groups="groups.nodes"
-      :segment-id="selectedSegmentId"
+      :segment="selectedSegment"
     />
     <div v-if="hasSegmentsData" class="gl-mt-3">
       <div
@@ -139,12 +149,20 @@ export default {
             <template #timestamp>{{ timestamp }}</template>
           </gl-sprintf>
         </span>
-        <gl-button v-gl-modal="$options.devopsSegmentModalId">{{
+        <gl-button v-gl-modal="$options.devopsSegmentModalId" @click="clearSelectedSegment">{{
           $options.i18n.tableHeader.button
         }}</gl-button>
       </div>
-      <devops-adoption-table :segments="devopsAdoptionSegments.nodes" />
+      <devops-adoption-table
+        :segments="devopsAdoptionSegments.nodes"
+        :selected-segment="selectedSegment"
+        @set-selected-segment="setSelectedSegment"
+      />
     </div>
-    <devops-adoption-empty-state v-else :has-groups-data="hasGroupData" />
+    <devops-adoption-empty-state
+      v-else
+      :has-groups-data="hasGroupData"
+      @clear-selected-segment="clearSelectedSegment"
+    />
   </div>
 </template>

ee/app/assets/javascripts/admin/dev_ops_report/components/devops_adoption_empty_state.vue

@@ -33,6 +33,7 @@ export default {
         v-gl-modal="$options.devopsSegmentModalId"
         :disabled="!hasGroupsData"
         variant="info"
+        @click="$emit('clear-selected-segment')"
         >{{ $options.i18n.button }}</gl-button
       >
     </template>

ee/app/assets/javascripts/admin/dev_ops_report/components/devops_adoption_segment_modal.vue

@@ -7,9 +7,10 @@ import {
   GlSprintf,
   GlAlert,
 } from '@gitlab/ui';
-import { convertToGraphQLIds, TYPE_GROUP } from '~/graphql_shared/utils';
+import { getIdFromGraphQLId, convertToGraphQLIds, TYPE_GROUP } from '~/graphql_shared/utils';
 import * as Sentry from '~/sentry/wrapper';
 import createDevopsAdoptionSegmentMutation from '../graphql/mutations/create_devops_adoption_segment.mutation.graphql';
+import updateDevopsAdoptionSegmentMutation from '../graphql/mutations/update_devops_adoption_segment.mutation.graphql';
 import { DEVOPS_ADOPTION_STRINGS, DEVOPS_ADOPTION_SEGMENT_MODAL_ID } from '../constants';
 import { addSegmentToCache } from '../utils/cache_updates';
 
@@ -24,8 +25,8 @@ export default {
     GlAlert,
   },
   props: {
-    segmentId: {
-      type: String,
+    segment: {
+      type: Object,
       required: false,
       default: null,
     },
@@ -37,8 +38,8 @@ export default {
   i18n: DEVOPS_ADOPTION_STRINGS.modal,
   data() {
     return {
-      name: '',
-      checkboxValues: [],
+      name: this.segment?.name || '',
+      checkboxValues: this.segment ? this.checkboxValuesFromSegment() : [],
       loading: false,
       errors: [],
     };
@@ -55,14 +56,17 @@ export default {
     },
     primaryOptions() {
       return {
-        text: this.$options.i18n.button,
-        attributes: [
-          {
-            variant: 'info',
-            loading: this.loading,
-            disabled: !this.canSubmit,
-          },
-        ],
+        button: {
+          text: this.segment ? this.$options.i18n.editingButton : this.$options.i18n.addingButton,
+          attributes: [
+            {
+              variant: 'info',
+              loading: this.loading,
+              disabled: !this.canSubmit,
+            },
+          ],
+        },
+        callback: this.segment ? this.updateSegment : this.createSegment,
       };
     },
     canSubmit() {
@@ -71,6 +75,9 @@ export default {
     displayError() {
       return this.errors[0];
     },
+    modalTitle() {
+      return this.segment ? this.$options.i18n.editingTitle : this.$options.i18n.addingTitle;
+    },
   },
   methods: {
     async createSegment() {
@@ -98,10 +105,35 @@ export default {
         if (errors.length) {
           this.errors = errors;
         } else {
-          this.name = '';
-          this.checkboxValues = [];
+          this.closeModal();
+        }
+      } catch (error) {
+        this.errors.push(this.$options.i18n.error);
+        Sentry.captureException(error);
+      } finally {
+        this.loading = false;
+      }
+    },
+    async updateSegment() {
+      try {
+        this.loading = true;
+        const {
+          data: {
+            updateDevopsAdoptionSegment: { errors },
+          },
+        } = await this.$apollo.mutate({
+          mutation: updateDevopsAdoptionSegmentMutation,
+          variables: {
+            id: this.segment.id,
+            name: this.name,
+            groupIds: convertToGraphQLIds(TYPE_GROUP, this.checkboxValues),
+          },
+        });
 
-          this.$refs.modal.hide();
+        if (errors.length) {
+          this.errors = errors;
+        } else {
+          this.closeModal();
         }
       } catch (error) {
         this.errors.push(this.$options.i18n.error);
@@ -113,6 +145,12 @@ export default {
     clearErrors() {
       this.errors = [];
     },
+    closeModal() {
+      this.$refs.modal.hide();
+    },
+    checkboxValuesFromSegment() {
+      return this.segment.groups.map(({ id }) => getIdFromGraphQLId(id));
+    },
   },
   devopsSegmentModalId: DEVOPS_ADOPTION_SEGMENT_MODAL_ID,
 };
@@ -121,12 +159,12 @@ export default {
   <gl-modal
     ref="modal"
     :modal-id="$options.devopsSegmentModalId"
-    :title="$options.i18n.title"
+    :title="modalTitle"
     size="sm"
     scrollable
-    :action-primary="primaryOptions"
+    :action-primary="primaryOptions.button"
     :action-cancel="cancelOptions"
-    @primary.prevent="createSegment"
+    @primary.prevent="primaryOptions.callback"
   >
     <gl-alert v-if="errors.length" variant="danger" class="gl-mb-3" @dismiss="clearErrors">
       {{ displayError }}

ee/app/assets/javascripts/admin/dev_ops_report/components/devops_adoption_table.vue

@@ -6,6 +6,7 @@ import DevopsAdoptionDeleteModal from './devops_adoption_delete_modal.vue';
 import {
   DEVOPS_ADOPTION_TABLE_TEST_IDS,
   DEVOPS_ADOPTION_STRINGS,
+  DEVOPS_ADOPTION_SEGMENT_MODAL_ID,
   DEVOPS_ADOPTION_SEGMENT_DELETE_MODAL_ID,
 } from '../constants';
 
@@ -24,6 +25,7 @@ export default {
     DevopsAdoptionDeleteModal,
   },
   i18n: DEVOPS_ADOPTION_STRINGS.table,
+  devopsSegmentModalId: DEVOPS_ADOPTION_SEGMENT_MODAL_ID,
   devopsSegmentDeleteModalId: DEVOPS_ADOPTION_SEGMENT_DELETE_MODAL_ID,
   directives: {
     GlModal: GlModalDirective,
@@ -82,11 +84,11 @@ export default {
       type: Array,
       required: true,
     },
-  },
-  data() {
-    return {
-      selectedSegment: null,
-    };
+    selectedSegment: {
+      type: Object,
+      required: false,
+      default: null,
+    },
   },
   methods: {
     popoverContainerId(name) {
@@ -96,7 +98,7 @@ export default {
       return `popover_id_for_${name}`;
     },
     setSelectedSegment(segment) {
-      this.selectedSegment = segment;
+      this.$emit('set-selected-segment', segment);
     },
   },
 };
@@ -181,13 +183,22 @@ export default {
               triggers="hover focus"
               placement="left"
             >
-              <gl-button
-                v-gl-modal="$options.devopsSegmentDeleteModalId"
-                category="tertiary"
-                variant="danger"
-                @click="setSelectedSegment(item)"
-                >{{ $options.i18n.deleteButton }}</gl-button
-              >
+              <div class="gl-display-inline-flex gl-flex-direction-column">
+                <gl-button
+                  v-gl-modal="$options.devopsSegmentModalId"
+                  category="tertiary"
+                  class="gl-w-max-content"
+                  @click="setSelectedSegment(item)"
+                  >{{ $options.i18n.editButton }}</gl-button
+                >
+                <gl-button
+                  v-gl-modal="$options.devopsSegmentDeleteModalId"
+                  category="tertiary"
+                  variant="danger"
+                  @click="setSelectedSegment(item)"
+                  >{{ $options.i18n.deleteButton }}</gl-button
+                >
+              </div>
             </gl-popover>
           </div>
         </div>

ee/app/assets/javascripts/admin/dev_ops_report/constants.js

@@ -36,8 +36,10 @@ export const DEVOPS_ADOPTION_STRINGS = {
     button: s__('DevopsAdoption|Add new segment'),
   },
   modal: {
-    title: s__('DevopsAdoption|New segment'),
-    button: s__('DevopsAdoption|Create new segment'),
+    addingTitle: s__('DevopsAdoption|New segment'),
+    editingTitle: s__('DevopsAdoption|Edit segment'),
+    addingButton: s__('DevopsAdoption|Create new segment'),
+    editingButton: s__('DevopsAdoption|Save changes'),
     cancel: __('Cancel'),
     namePlaceholder: s__('DevopsAdoption|My segment'),
     nameLabel: s__('DevopsAdoption|Name'),
@@ -46,6 +48,7 @@ export const DEVOPS_ADOPTION_STRINGS = {
     error: s__('DevopsAdoption|An error occured while saving the segment. Please try again.'),
   },
   table: {
+    editButton: s__('DevopsAdoption|Edit segment'),
     deleteButton: s__('DevopsAdoption|Delete segment'),
   },
   deleteModal: {

ee/app/assets/javascripts/admin/dev_ops_report/graphql/mutations/update_devops_adoption_segment.mutation.graphql

+mutation($id: AnalyticsDevopsAdoptionSegmentID!, $name: String!, $groupIds: [GroupID!]!) {
+  updateDevopsAdoptionSegment(input: { id: $id, name: $name, groupIds: $groupIds }) {
+    segment {
+      id
+      name
+      groups {
+        id
+      }
+      latestSnapshot {
+        issueOpened
+        mergeRequestOpened
+        mergeRequestApproved
+        runnerConfigured
+        pipelineSucceeded
+        deploySucceeded
+        securityScanSucceeded
+        recordedAt
+      }
+    }
+    errors
+  }
+}

ee/app/assets/javascripts/admin/dev_ops_report/graphql/queries/devops_adoption_segments.query.graphql

@@ -3,6 +3,9 @@ query devopsAdoptionSegments {
     nodes {
       id
       name
+      groups {
+        id
+      }
       latestSnapshot {
         issueOpened
         mergeRequestOpened

ee/spec/frontend/admin/dev_ops_report/components/devops_adoption_segment_modal_spec.js

@@ -14,6 +14,7 @@ import {
   segmentName,
   genericErrorMessage,
   dataErrorMessage,
+  devopsAdoptionSegmentsData,
 } from '../mock_data';
 
 const mockEvent = { preventDefault: jest.fn() };
@@ -22,28 +23,33 @@ const mutate = jest.fn().mockResolvedValue({
     createDevopsAdoptionSegment: {
       errors: [],
     },
-  },
-});
-const mutateWithDataErrors = jest.fn().mockResolvedValue({
-  data: {
-    createDevopsAdoptionSegment: {
-      errors: [dataErrorMessage],
+    updateDevopsAdoptionSegment: {
+      errors: [],
     },
   },
 });
+const mutateWithDataErrors = segment =>
+  jest.fn().mockResolvedValue({
+    data: {
+      [segment ? 'updateDevopsAdoptionSegment' : 'createDevopsAdoptionSegment']: {
+        errors: [dataErrorMessage],
+      },
+    },
+  });
 const mutateLoading = jest.fn().mockResolvedValue(new Promise(() => {}));
 const mutateWithErrors = jest.fn().mockRejectedValue(genericErrorMessage);
 
 describe('DevopsAdoptionSegmentModal', () => {
   let wrapper;
 
-  const createComponent = ({ mutationMock = mutate } = {}) => {
+  const createComponent = ({ mutationMock = mutate, segment = null } = {}) => {
     const $apollo = {
       mutate: mutationMock,
     };
 
     wrapper = shallowMount(DevopsAdoptionSegmentModal, {
       propsData: {
+        segment,
         groups: groupNodes,
       },
       stubs: {
@@ -151,110 +157,123 @@ describe('DevopsAdoptionSegmentModal', () => {
     },
   );
 
-  describe('submitting the form', () => {
-    describe('while waiting for the mutation', () => {
-      beforeEach(() => {
-        createComponent({ mutationMock: mutateLoading });
+  describe.each`
+    action                            | segment                                | additionalData
+    ${'creating a new segment'}       | ${null}                                | ${{ checkboxValues: groupIds, name: segmentName }}
+    ${'updating an existing segment'} | ${devopsAdoptionSegmentsData.nodes[0]} | ${{}}
+  `('handles the form submission correctly when $action', ({ segment, additionalData }) => {
+    describe('submitting the form', () => {
+      describe('while waiting for the mutation', () => {
+        beforeEach(() => {
+          createComponent({ mutationMock: mutateLoading, segment });
 
-        wrapper.setData({ checkboxValues: [1], name: segmentName });
-      });
+          wrapper.setData(additionalData);
+        });
 
-      it('disables the form inputs', async () => {
-        const checkboxes = findByTestId('groups');
-        const name = findByTestId('name');
+        it('disables the form inputs', async () => {
+          const checkboxes = findByTestId('groups');
+          const name = findByTestId('name');
 
-        expect(checkboxes.attributes('disabled')).not.toBeDefined();
-        expect(name.attributes('disabled')).not.toBeDefined();
+          expect(checkboxes.attributes('disabled')).not.toBeDefined();
+          expect(name.attributes('disabled')).not.toBeDefined();
 
-        findModal().vm.$emit('primary', mockEvent);
+          findModal().vm.$emit('primary', mockEvent);
 
-        await waitForPromises();
+          await nextTick();
 
-        expect(checkboxes.attributes('disabled')).toBeDefined();
-        expect(name.attributes('disabled')).toBeDefined();
-      });
+          expect(checkboxes.attributes('disabled')).toBeDefined();
+          expect(name.attributes('disabled')).toBeDefined();
+        });
 
-      it('disables the cancel button', async () => {
-        expect(cancelButtonDisabledState()).toBe(false);
+        it('disables the cancel button', async () => {
+          expect(cancelButtonDisabledState()).toBe(false);
 
-        findModal().vm.$emit('primary', mockEvent);
+          findModal().vm.$emit('primary', mockEvent);
 
-        await waitForPromises();
+          await nextTick();
 
-        expect(cancelButtonDisabledState()).toBe(true);
-      });
+          expect(cancelButtonDisabledState()).toBe(true);
+        });
 
-      it('sets the action button state to loading', async () => {
-        expect(actionButtonLoadingState()).toBe(false);
+        it('sets the action button state to loading', async () => {
+          expect(actionButtonLoadingState()).toBe(false);
 
-        findModal().vm.$emit('primary', mockEvent);
+          findModal().vm.$emit('primary', mockEvent);
 
-        await waitForPromises();
+          await nextTick();
 
-        expect(actionButtonLoadingState()).toBe(true);
+          expect(actionButtonLoadingState()).toBe(true);
+        });
       });
-    });
 
-    describe('successful submission', () => {
-      beforeEach(async () => {
-        createComponent();
+      describe('successful submission', () => {
+        beforeEach(() => {
+          createComponent({ segment });
 
-        wrapper.setData({ checkboxValues: groupIds, name: segmentName });
-        wrapper.vm.$refs.modal.hide = jest.fn();
+          wrapper.setData(additionalData);
 
-        findModal().vm.$emit('primary', mockEvent);
+          wrapper.vm.$refs.modal.hide = jest.fn();
 
-        await waitForPromises();
+          findModal().vm.$emit('primary', mockEvent);
+        });
+
+        it('submits the correct request variables', async () => {
+          const variables = segment
+            ? {
+                id: segment.id,
+                groupIds: [groupGids[0]],
+                name: segment.name,
+              }
+            : {
+                groupIds: groupGids,
+                name: segmentName,
+              };
+
+          expect(mutate).toHaveBeenCalledWith(
+            expect.objectContaining({
+              variables,
+            }),
+          );
+        });
+
+        it('closes the modal after a successful mutation', async () => {
+          expect(wrapper.vm.$refs.modal.hide).toHaveBeenCalled();
+        });
       });
 
-      it('submits the correct request variables', async () => {
-        expect(mutate).toHaveBeenCalledWith(
-          expect.objectContaining({
-            variables: {
-              groupIds: groupGids,
-              name: segmentName,
-            },
-          }),
-        );
-      });
+      describe('error handling', () => {
+        it.each`
+          errorType     | errorLocation  | mutationSpy                      | message
+          ${'generic'}  | ${'top level'} | ${mutateWithErrors}              | ${genericErrorMessage}
+          ${'specific'} | ${'data'}      | ${mutateWithDataErrors(segment)} | ${dataErrorMessage}
+        `(
+          'displays a $errorType error if the mutation has a $errorLocation error',
+          async ({ mutationSpy, message }) => {
+            createComponent({ mutationMock: mutationSpy, segment });
 
-      it('closes the modal after a successful mutation', async () => {
-        expect(wrapper.vm.$refs.modal.hide).toHaveBeenCalled();
-      });
-    });
+            findModal().vm.$emit('primary', mockEvent);
 
-    describe('error handling', () => {
-      it.each`
-        errorType     | errorLocation  | mutationSpy             | message
-        ${'generic'}  | ${'top level'} | ${mutateWithErrors}     | ${genericErrorMessage}
-        ${'specific'} | ${'data'}      | ${mutateWithDataErrors} | ${dataErrorMessage}
-      `(
-        'displays a $errorType error if the mutation has a $errorLocation error',
-        async ({ mutationSpy, message }) => {
-          createComponent({ mutationMock: mutationSpy });
+            await waitForPromises();
 
-          findModal().vm.$emit('primary', mockEvent);
+            const alert = findAlert();
 
-          await waitForPromises();
-
-          const alert = findAlert();
-
-          expect(alert.exists()).toBe(true);
-          expect(alert.props('variant')).toBe('danger');
-          expect(alert.text()).toBe(message);
-        },
-      );
+            expect(alert.exists()).toBe(true);
+            expect(alert.props('variant')).toBe('danger');
+            expect(alert.text()).toBe(message);
+          },
+        );
 
-      it('calls sentry on top level error', async () => {
-        jest.spyOn(Sentry, 'captureException');
+        it('calls sentry on top level error', async () => {
+          jest.spyOn(Sentry, 'captureException');
 
-        createComponent({ mutationMock: mutateWithErrors });
+          createComponent({ mutationMock: mutateWithErrors, segment });
 
-        findModal().vm.$emit('primary', mockEvent);
+          findModal().vm.$emit('primary', mockEvent);
 
-        await waitForPromises();
+          await waitForPromises();
 
-        expect(Sentry.captureException.mock.calls[0][0]).toBe(genericErrorMessage);
+          expect(Sentry.captureException.mock.calls[0][0]).toBe(genericErrorMessage);
+        });
       });
     });
   });

ee/spec/frontend/admin/dev_ops_report/mock_data.js

-export const groupData = [{ id: 'foo', full_name: 'Foo' }, { id: 'bar', full_name: 'Bar' }];
+export const groupData = [{ id: '1', full_name: 'Foo' }, { id: '2', full_name: 'Bar' }];
 
 export const pageData = {
   'x-next-page': 2,
@@ -8,23 +8,23 @@ export const groupNodes = [
   {
     __typename: 'Group',
     full_name: 'Foo',
-    id: 'foo',
+    id: '1',
   },
   {
     __typename: 'Group',
     full_name: 'Bar',
-    id: 'bar',
+    id: '2',
   },
 ];
 
-export const groupIds = ['foo', 'bar'];
+export const groupIds = ['1', '2'];
 
-export const groupGids = ['gid://gitlab/Group/foo', 'gid://gitlab/Group/bar'];
+export const groupGids = ['gid://gitlab/Group/1', 'gid://gitlab/Group/2'];
 
 export const nextGroupNode = {
   __typename: 'Group',
   full_name: 'Baz',
-  id: 'baz',
+  id: '3',
 };
 
 export const groupPageInfo = {
@@ -36,6 +36,11 @@ export const devopsAdoptionSegmentsData = {
     {
       name: 'Segment 1',
       id: 1,
+      groups: [
+        {
+          id: 'gid://gitlab/Group/1',
+        },
+      ],
       latestSnapshot: {
         issueOpened: true,
         mergeRequestOpened: true,

locale/gitlab.pot

@@ -9634,6 +9634,9 @@ msgstr ""
 msgid "DevopsAdoption|DevOps adoption uses segments to track adoption across key features. Segments are a way to track multiple related projects and groups at once. For example, you could create a segment for the engineering department or a particular product team."
 msgstr ""
 
+msgid "DevopsAdoption|Edit segment"
+msgstr ""
+
 msgid "DevopsAdoption|Feature adoption is based on usage in the last calendar month. Last updated: %{timestamp}."
 msgstr ""
 
@@ -9658,6 +9661,9 @@ msgstr ""
 msgid "DevopsAdoption|Runners"
 msgstr ""
 
+msgid "DevopsAdoption|Save changes"
+msgstr ""
+
 msgid "DevopsAdoption|Scanning"
 msgstr ""