Merge branch '339653-improve-delete-runner-flow' into 'master'

Improve runner deletion modal

See merge request gitlab-org/gitlab!75432

app/assets/javascripts/runner/components/cells/runner_actions_cell.vue

 <script>
-import { GlButton, GlButtonGroup, GlTooltipDirective } from '@gitlab/ui';
+import { GlButton, GlButtonGroup, GlModalDirective, GlTooltipDirective } from '@gitlab/ui';
 import createFlash from '~/flash';
-import { __, s__ } from '~/locale';
+import { __, s__, sprintf } from '~/locale';
 import runnerDeleteMutation from '~/runner/graphql/runner_delete.mutation.graphql';
 import runnerActionsUpdateMutation from '~/runner/graphql/runner_actions_update.mutation.graphql';
 import { captureException } from '~/runner/sentry_utils';
+import { getIdFromGraphQLId } from '~/graphql_shared/utils';
+import RunnerDeleteModal from '../runner_delete_modal.vue';
 
-const i18n = {
-  I18N_EDIT: __('Edit'),
-  I18N_PAUSE: __('Pause'),
-  I18N_RESUME: __('Resume'),
-  I18N_REMOVE: __('Remove'),
-  I18N_REMOVE_CONFIRMATION: s__('Runners|Are you sure you want to delete this runner?'),
-};
+const I18N_EDIT = __('Edit');
+const I18N_PAUSE = __('Pause');
+const I18N_RESUME = __('Resume');
+const I18N_DELETE = s__('Runners|Delete runner');
+const I18N_DELETED_TOAST = s__('Runners|Runner %{name} was deleted');
 
 export default {
   name: 'RunnerActionsCell',
   components: {
     GlButton,
     GlButtonGroup,
+    RunnerDeleteModal,
   },
   directives: {
     GlTooltip: GlTooltipDirective,
+    GlModal: GlModalDirective,
   },
   props: {
     runner: {
@@ -48,21 +50,29 @@ export default {
         // mouseout listeners don't run leaving the tooltip stuck
         return '';
       }
-      return this.isActive ? i18n.I18N_PAUSE : i18n.I18N_RESUME;
+      return this.isActive ? I18N_PAUSE : I18N_RESUME;
     },
     deleteTitle() {
-      // Prevent a "sticky" tooltip: If element gets removed,
-      // mouseout listeners don't run and leaving the tooltip stuck
-      return this.deleting ? '' : i18n.I18N_REMOVE;
+      if (this.deleting) {
+        // Prevent a "sticky" tooltip: If this button is disabled,
+        // mouseout listeners don't run leaving the tooltip stuck
+        return '';
+      }
+      return I18N_DELETE;
+    },
+    runnerId() {
+      return getIdFromGraphQLId(this.runner.id);
+    },
+    runnerName() {
+      return `#${this.runnerId} (${this.runner.shortSha})`;
+    },
+    runnerDeleteModalId() {
+      return `delete-runner-modal-${this.runnerId}`;
     },
   },
   methods: {
     async onToggleActive() {
       this.updating = true;
-      // TODO In HAML iteration we had a confirmation modal via:
-      //   data-confirm="_('Are you sure?')"
-      // this may not have to ported, this is an easily reversible operation
-
       try {
         const toggledActive = !this.runner.active;
 
@@ -91,12 +101,8 @@ export default {
     },
 
     async onDelete() {
-      // TODO Replace confirmation with gl-modal
-      // eslint-disable-next-line no-alert
-      if (!window.confirm(i18n.I18N_REMOVE_CONFIRMATION)) {
-        return;
-      }
-
+      // Deleting stays "true" until this row is removed,
+      // should only change back if the operation fails.
       this.deleting = true;
       try {
         const {
@@ -115,11 +121,13 @@ export default {
         });
         if (errors && errors.length) {
           throw new Error(errors.join(' '));
+        } else {
+          // Use $root to have the toast message stay after this element is removed
+          this.$root.$toast?.show(sprintf(I18N_DELETED_TOAST, { name: this.runnerName }));
         }
       } catch (e) {
-        this.onError(e);
-      } finally {
         this.deleting = false;
+        this.onError(e);
       }
     },
 
@@ -133,14 +141,15 @@ export default {
       captureException({ error, component: this.$options.name });
     },
   },
-  i18n,
+  I18N_EDIT,
+  I18N_DELETE,
 };
 </script>
 
 <template>
   <gl-button-group>
     <!--
-      This button appears for administratos: those with
+      This button appears for administrators: those with
       access to the adminUrl. More advanced permissions policies
       will allow more granular permissions.
 
@@ -148,16 +157,14 @@ export default {
     -->
     <gl-button
       v-if="runner.adminUrl"
-      v-gl-tooltip.hover.viewport
+      v-gl-tooltip.hover.viewport="$options.I18N_EDIT"
       :href="runner.adminUrl"
-      :title="$options.i18n.I18N_EDIT"
-      :aria-label="$options.i18n.I18N_EDIT"
+      :aria-label="$options.I18N_EDIT"
       icon="pencil"
       data-testid="edit-runner"
     />
     <gl-button
-      v-gl-tooltip.hover.viewport
-      :title="toggleActiveTitle"
+      v-gl-tooltip.hover.viewport="toggleActiveTitle"
       :aria-label="toggleActiveTitle"
       :icon="toggleActiveIcon"
       :loading="updating"
@@ -165,14 +172,20 @@ export default {
       @click="onToggleActive"
     />
     <gl-button
-      v-gl-tooltip.hover.viewport
-      :title="deleteTitle"
+      v-gl-tooltip.hover.viewport="deleteTitle"
+      v-gl-modal="runnerDeleteModalId"
       :aria-label="deleteTitle"
       icon="close"
       :loading="deleting"
       variant="danger"
       data-testid="delete-runner"
-      @click="onDelete"
+    />
+
+    <runner-delete-modal
+      :ref="runnerDeleteModalId"
+      :modal-id="runnerDeleteModalId"
+      :runner-name="runnerName"
+      @primary="onDelete"
     />
   </gl-button-group>
 </template>

app/assets/javascripts/runner/components/runner_delete_modal.vue

+<script>
+import { GlModal } from '@gitlab/ui';
+import { __, s__, sprintf } from '~/locale';
+
+const I18N_TITLE = s__('Runners|Delete runner %{name}?');
+const I18N_BODY = s__(
+  'Runners|The runner will be permanently deleted and no longer available for projects or groups in the instance. Are you sure you want to continue?',
+);
+const I18N_PRIMARY = s__('Runners|Delete runner');
+const I18N_CANCEL = __('Cancel');
+
+export default {
+  components: {
+    GlModal,
+  },
+  props: {
+    runnerName: {
+      type: String,
+      required: true,
+    },
+  },
+  computed: {
+    title() {
+      return sprintf(I18N_TITLE, { name: this.runnerName });
+    },
+  },
+  methods: {
+    onPrimary() {
+      this.$refs.modal.hide();
+    },
+  },
+  actionPrimary: { text: I18N_PRIMARY, attributes: { variant: 'danger' } },
+  actionCancel: { text: I18N_CANCEL },
+  I18N_BODY,
+};
+</script>
+
+<template>
+  <gl-modal
+    ref="modal"
+    size="sm"
+    :title="title"
+    :action-primary="$options.actionPrimary"
+    :action-cancel="$options.actionCancel"
+    v-bind="$attrs"
+    v-on="$listeners"
+    @primary="onPrimary"
+  >
+    {{ $options.I18N_BODY }}
+  </gl-modal>
+</template>

app/assets/javascripts/runner/components/runner_list.vue

@@ -81,6 +81,7 @@ export default {
       :tbody-tr-attr="runnerTrAttr"
       data-testid="runner-list"
       stacked="md"
+      primary-key="id"
       fixed
     >
       <template v-if="!runners.length" #table-busy>

locale/gitlab.pot

@@ -30097,9 +30097,6 @@ msgstr ""
 msgid "Runners|Architecture"
 msgstr ""
 
-msgid "Runners|Are you sure you want to delete this runner?"
-msgstr ""
-
 msgid "Runners|Associated with one or more projects"
 msgstr ""
 
@@ -30121,6 +30118,12 @@ msgstr ""
 msgid "Runners|Copy registration token"
 msgstr ""
 
+msgid "Runners|Delete runner"
+msgstr ""
+
+msgid "Runners|Delete runner %{name}?"
+msgstr ""
+
 msgid "Runners|Deploy GitLab Runner in AWS"
 msgstr ""
 
@@ -30241,6 +30244,9 @@ msgstr ""
 msgid "Runners|Runner #%{runner_id}"
 msgstr ""
 
+msgid "Runners|Runner %{name} was deleted"
+msgstr ""
+
 msgid "Runners|Runner ID"
 msgstr ""
 
@@ -30298,6 +30304,9 @@ msgstr ""
 msgid "Runners|Tags"
 msgstr ""
 
+msgid "Runners|The runner will be permanently deleted and no longer available for projects or groups in the instance. Are you sure you want to continue?"
+msgstr ""
+
 msgid "Runners|This runner has never connected to this instance"
 msgstr ""
 

spec/features/admin/admin_runners_spec.rb

@@ -59,6 +59,42 @@ RSpec.describe "Admin Runners" do
         end
       end
 
+      describe 'delete runner' do
+        let!(:runner) { create(:ci_runner, description: 'runner-foo') }
+
+        before do
+          visit admin_runners_path
+
+          within "[data-testid='runner-row-#{runner.id}']" do
+            click_on 'Delete runner'
+          end
+        end
+
+        it 'shows a confirmation modal' do
+          expect(page).to have_text "Delete runner ##{runner.id} (#{runner.short_sha})?"
+          expect(page).to have_text "Are you sure you want to continue?"
+        end
+
+        it 'deletes a runner' do
+          within '.modal' do
+            click_on 'Delete runner'
+          end
+
+          expect(page.find('.gl-toast')).to have_text(/Runner .+ deleted/)
+          expect(page).not_to have_content 'runner-foo'
+        end
+
+        it 'cancels runner deletion' do
+          within '.modal' do
+            click_on 'Cancel'
+          end
+
+          wait_for_requests
+
+          expect(page).to have_content 'runner-foo'
+        end
+      end
+
       describe 'search' do
         before do
           create(:ci_runner, :instance, description: 'runner-foo')

spec/frontend/runner/components/cells/runner_actions_cell_spec.js

@@ -3,13 +3,17 @@ import VueApollo from 'vue-apollo';
 import createMockApollo from 'helpers/mock_apollo_helper';
 import { extendedWrapper } from 'helpers/vue_test_utils_helper';
 import waitForPromises from 'helpers/wait_for_promises';
+import { createMockDirective, getBinding } from 'helpers/vue_mock_directive';
 import createFlash from '~/flash';
+import { getIdFromGraphQLId } from '~/graphql_shared/utils';
+
+import { captureException } from '~/runner/sentry_utils';
 import RunnerActionCell from '~/runner/components/cells/runner_actions_cell.vue';
+import RunnerDeleteModal from '~/runner/components/runner_delete_modal.vue';
 import getGroupRunnersQuery from '~/runner/graphql/get_group_runners.query.graphql';
 import getRunnersQuery from '~/runner/graphql/get_runners.query.graphql';
 import runnerDeleteMutation from '~/runner/graphql/runner_delete.mutation.graphql';
 import runnerActionsUpdateMutation from '~/runner/graphql/runner_actions_update.mutation.graphql';
-import { captureException } from '~/runner/sentry_utils';
 import { runnersData } from '../../mock_data';
 
 const mockRunner = runnersData.data.runners.nodes[0];
@@ -25,12 +29,16 @@ jest.mock('~/runner/sentry_utils');
 
 describe('RunnerTypeCell', () => {
   let wrapper;
+
+  const mockToastShow = jest.fn();
   const runnerDeleteMutationHandler = jest.fn();
   const runnerActionsUpdateMutationHandler = jest.fn();
 
   const findEditBtn = () => wrapper.findByTestId('edit-runner');
   const findToggleActiveBtn = () => wrapper.findByTestId('toggle-active-runner');
+  const findRunnerDeleteModal = () => wrapper.findComponent(RunnerDeleteModal);
   const findDeleteBtn = () => wrapper.findByTestId('delete-runner');
+  const getTooltip = (w) => getBinding(w.element, 'gl-tooltip')?.value;
 
   const createComponent = ({ active = true } = {}, options) => {
     wrapper = extendedWrapper(
@@ -38,6 +46,7 @@ describe('RunnerTypeCell', () => {
         propsData: {
           runner: {
             id: mockRunner.id,
+            shortSha: mockRunner.shortSha,
             adminUrl: mockRunner.adminUrl,
             active,
           },
@@ -47,6 +56,15 @@ describe('RunnerTypeCell', () => {
           [runnerDeleteMutation, runnerDeleteMutationHandler],
           [runnerActionsUpdateMutation, runnerActionsUpdateMutationHandler],
         ]),
+        directives: {
+          GlTooltip: createMockDirective(),
+          GlModal: createMockDirective(),
+        },
+        mocks: {
+          $toast: {
+            show: mockToastShow,
+          },
+        },
         ...options,
       }),
     );
@@ -72,18 +90,22 @@ describe('RunnerTypeCell', () => {
   });
 
   afterEach(() => {
+    mockToastShow.mockReset();
     runnerDeleteMutationHandler.mockReset();
     runnerActionsUpdateMutationHandler.mockReset();
 
     wrapper.destroy();
   });
 
+  describe('Edit Action', () => {
     it('Displays the runner edit link with the correct href', () => {
       createComponent();
 
       expect(findEditBtn().attributes('href')).toBe(mockRunner.adminUrl);
     });
+  });
 
+  describe('Toggle active action', () => {
     describe.each`
       state       | label       | icon       | isActive | newActiveValue
       ${'active'} | ${'Pause'}  | ${'pause'} | ${true}  | ${false}
@@ -96,7 +118,7 @@ describe('RunnerTypeCell', () => {
       it(`Displays a ${icon} button`, () => {
         expect(findToggleActiveBtn().props('loading')).toBe(false);
         expect(findToggleActiveBtn().props('icon')).toBe(icon);
-      expect(findToggleActiveBtn().attributes('title')).toBe(label);
+        expect(getTooltip(findToggleActiveBtn())).toBe(label);
         expect(findToggleActiveBtn().attributes('aria-label')).toBe(label);
       });
 
@@ -109,7 +131,7 @@ describe('RunnerTypeCell', () => {
       it(`After the ${icon} button is clicked, stale tooltip is removed`, async () => {
         await findToggleActiveBtn().vm.$emit('click');
 
-      expect(findToggleActiveBtn().attributes('title')).toBe('');
+        expect(getTooltip(findToggleActiveBtn())).toBe('');
         expect(findToggleActiveBtn().attributes('aria-label')).toBe('');
       });
 
@@ -191,40 +213,39 @@ describe('RunnerTypeCell', () => {
         });
       });
     });
-
-  describe('When the user clicks a runner', () => {
-    beforeEach(() => {
-      jest.spyOn(window, 'confirm');
-
-      createComponent();
   });
 
-    afterEach(() => {
-      window.confirm.mockRestore();
+  describe('Delete action', () => {
+    beforeEach(() => {
+      createComponent(
+        {},
+        {
+          stubs: { RunnerDeleteModal },
+        },
+      );
     });
 
-    describe('When the user confirms deletion', () => {
-      beforeEach(async () => {
-        window.confirm.mockReturnValue(true);
-        await findDeleteBtn().vm.$emit('click');
-      });
+    it('Delete button opens delete modal', () => {
+      const modalId = getBinding(findDeleteBtn().element, 'gl-modal').value;
 
-      it('The user sees a confirmation alert', () => {
-        expect(window.confirm).toHaveBeenCalledTimes(1);
-        expect(window.confirm).toHaveBeenCalledWith(expect.any(String));
+      expect(findRunnerDeleteModal().attributes('modal-id')).toBeDefined();
+      expect(findRunnerDeleteModal().attributes('modal-id')).toBe(modalId);
     });
 
-      it('The delete mutation is called correctly', () => {
-        expect(runnerDeleteMutationHandler).toHaveBeenCalledTimes(1);
-        expect(runnerDeleteMutationHandler).toHaveBeenCalledWith({
-          input: { id: mockRunner.id },
+    it('Delete modal shows the runner name', () => {
+      expect(findRunnerDeleteModal().props('runnerName')).toBe(
+        `#${getIdFromGraphQLId(mockRunner.id)} (${mockRunner.shortSha})`,
+      );
     });
+    it('The delete button does not have a loading icon', () => {
+      expect(findDeleteBtn().props('loading')).toBe(false);
+      expect(getTooltip(findDeleteBtn())).toBe('Delete runner');
     });
 
-      it('When delete mutation is called, current runners are refetched', async () => {
+    it('When delete mutation is called, current runners are refetched', () => {
       jest.spyOn(wrapper.vm.$apollo, 'mutate');
 
-        await findDeleteBtn().vm.$emit('click');
+      findRunnerDeleteModal().vm.$emit('primary');
 
       expect(wrapper.vm.$apollo.mutate).toHaveBeenCalledWith({
         mutation: runnerDeleteMutation,
@@ -238,31 +259,39 @@ describe('RunnerTypeCell', () => {
       });
     });
 
-      it('The delete button does not have a loading state', () => {
-        expect(findDeleteBtn().props('loading')).toBe(false);
-        expect(findDeleteBtn().attributes('title')).toBe('Remove');
+    describe('When delete is clicked', () => {
+      beforeEach(() => {
+        findRunnerDeleteModal().vm.$emit('primary');
       });
 
-      it('After the delete button is clicked, loading state is shown', async () => {
-        await findDeleteBtn().vm.$emit('click');
+      it('The delete mutation is called correctly', () => {
+        expect(runnerDeleteMutationHandler).toHaveBeenCalledTimes(1);
+        expect(runnerDeleteMutationHandler).toHaveBeenCalledWith({
+          input: { id: mockRunner.id },
+        });
+      });
 
+      it('The delete button has a loading icon', () => {
         expect(findDeleteBtn().props('loading')).toBe(true);
+        expect(getTooltip(findDeleteBtn())).toBe('');
       });
 
-      it('After the delete button is clicked, stale tooltip is removed', async () => {
-        await findDeleteBtn().vm.$emit('click');
-
-        expect(findDeleteBtn().attributes('title')).toBe('');
+      it('The toast notification is shown', () => {
+        expect(mockToastShow).toHaveBeenCalledTimes(1);
+        expect(mockToastShow).toHaveBeenCalledWith(
+          expect.stringContaining(`#${getIdFromGraphQLId(mockRunner.id)} (${mockRunner.shortSha})`),
+        );
+      });
     });
 
     describe('When delete fails', () => {
       describe('On a network error', () => {
         const mockErrorMsg = 'Delete error!';
 
-          beforeEach(async () => {
+        beforeEach(() => {
           runnerDeleteMutationHandler.mockRejectedValueOnce(new Error(mockErrorMsg));
 
-            await findDeleteBtn().vm.$emit('click');
+          findRunnerDeleteModal().vm.$emit('primary');
         });
 
         it('error is reported to sentry', () => {
@@ -275,13 +304,17 @@ describe('RunnerTypeCell', () => {
         it('error is shown to the user', () => {
           expect(createFlash).toHaveBeenCalledTimes(1);
         });
+
+        it('toast notification is not shown', () => {
+          expect(mockToastShow).not.toHaveBeenCalled();
+        });
       });
 
       describe('On a validation error', () => {
         const mockErrorMsg = 'Runner not found!';
         const mockErrorMsg2 = 'User not allowed!';
 
-          beforeEach(async () => {
+        beforeEach(() => {
           runnerDeleteMutationHandler.mockResolvedValue({
             data: {
               runnerDelete: {
@@ -290,7 +323,7 @@ describe('RunnerTypeCell', () => {
             },
           });
 
-            await findDeleteBtn().vm.$emit('click');
+          findRunnerDeleteModal().vm.$emit('primary');
         });
 
         it('error is reported to sentry', () => {
@@ -306,25 +339,4 @@ describe('RunnerTypeCell', () => {
       });
     });
   });
-
-    describe('When the user does not confirm deletion', () => {
-      beforeEach(async () => {
-        window.confirm.mockReturnValue(false);
-        await findDeleteBtn().vm.$emit('click');
-      });
-
-      it('The user sees a confirmation alert', () => {
-        expect(window.confirm).toHaveBeenCalledTimes(1);
-      });
-
-      it('The delete mutation is not called', () => {
-        expect(runnerDeleteMutationHandler).toHaveBeenCalledTimes(0);
-      });
-
-      it('The delete button does not have a loading state', () => {
-        expect(findDeleteBtn().props('loading')).toBe(false);
-        expect(findDeleteBtn().attributes('title')).toBe('Remove');
-      });
-    });
-  });
 });

spec/frontend/runner/components/runner_delete_modal_spec.js

+import { GlModal } from '@gitlab/ui';
+import { mount, shallowMount } from '@vue/test-utils';
+import RunnerDeleteModal from '~/runner/components/runner_delete_modal.vue';
+
+describe('RunnerDeleteModal', () => {
+  let wrapper;
+
+  const findGlModal = () => wrapper.findComponent(GlModal);
+
+  const createComponent = ({ props = {} } = {}, mountFn = shallowMount) => {
+    wrapper = mountFn(RunnerDeleteModal, {
+      attachTo: document.body,
+      propsData: {
+        runnerName: '#99 (AABBCCDD)',
+        ...props,
+      },
+      attrs: {
+        modalId: 'delete-runner-modal-99',
+      },
+    });
+  };
+
+  it('Displays title', () => {
+    createComponent();
+
+    expect(findGlModal().props('title')).toBe('Delete runner #99 (AABBCCDD)?');
+  });
+
+  it('Displays buttons', () => {
+    createComponent();
+
+    expect(findGlModal().props('actionPrimary')).toMatchObject({ text: 'Delete runner' });
+    expect(findGlModal().props('actionCancel')).toMatchObject({ text: 'Cancel' });
+  });
+
+  it('Displays contents', () => {
+    createComponent();
+
+    expect(findGlModal().html()).toContain(
+      'The runner will be permanently deleted and no longer available for projects or groups in the instance. Are you sure you want to continue?',
+    );
+  });
+
+  describe('When modal is confirmed by the user', () => {
+    let hideModalSpy;
+
+    beforeEach(() => {
+      createComponent({}, mount);
+      hideModalSpy = jest.spyOn(wrapper.vm.$refs.modal, 'hide').mockImplementation(() => {});
+    });
+
+    it('Modal gets hidden', () => {
+      expect(hideModalSpy).toHaveBeenCalledTimes(0);
+
+      findGlModal().vm.$emit('primary');
+
+      expect(hideModalSpy).toHaveBeenCalledTimes(1);
+    });
+  });
+});

spec/frontend/runner/components/runner_list_spec.js

@@ -52,6 +52,12 @@ describe('RunnerList', () => {
     ]);
   });
 
+  it('Sets runner id as a row key', () => {
+    createComponent({}, shallowMount);
+
+    expect(findTable().attributes('primary-key')).toBe('id');
+  });
+
   it('Displays a list of runners', () => {
     expect(findRows()).toHaveLength(4);