Improve runner deletion modal

This change improves the runner deletion flow for administrators by
displaying more details in a modal before deleting a runner.

Changelog: changed

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/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);