From c3b70ef5bb35ad4f2e1ae3d556c68a024f208501 Mon Sep 17 00:00:00 2001
From: Miguel Rincon <mrincon@gitlab.com>
Date: Mon, 14 Mar 2022 11:48:48 +0100
Subject: [PATCH] Explain delete permission on multi-project runners
This change adds a tooltip explaining to group owners why they can't
delete a runner that has been already added to multiple projects.
---
.../components/cells/runner_actions_cell.vue | 7 ++-
.../components/runner_delete_button.vue | 53 ++++++++++++++-----
app/assets/javascripts/runner/constants.js | 6 +++
.../graphql/list/group_runners.query.graphql | 1 +
locale/gitlab.pot | 6 +++
.../cells/runner_actions_cell_spec.js | 12 ++++-
.../components/runner_delete_button_spec.js | 40 ++++++++++++--
7 files changed, 106 insertions(+), 19 deletions(-)
diff --git a/app/assets/javascripts/runner/components/cells/runner_actions_cell.vue b/app/assets/javascripts/runner/components/cells/runner_actions_cell.vue
index c69321de0018f..d3535f8942791 100644
--- a/app/assets/javascripts/runner/components/cells/runner_actions_cell.vue
+++ b/app/assets/javascripts/runner/components/cells/runner_actions_cell.vue
@@ -44,6 +44,11 @@ export default {
<gl-button-group>
<runner-edit-button v-if="canUpdate && editUrl" :href="editUrl" />
<runner-pause-button v-if="canUpdate" :runner="runner" :compact="true" />
- <runner-delete-button v-if="canDelete" :runner="runner" :compact="true" @deleted="onDeleted" />
+ <runner-delete-button
+ :disabled="!canDelete"
+ :runner="runner"
+ :compact="true"
+ @deleted="onDeleted"
+ />
</gl-button-group>
</template>
diff --git a/app/assets/javascripts/runner/components/runner_delete_button.vue b/app/assets/javascripts/runner/components/runner_delete_button.vue
index 854c983f4daac..a546a2788de87 100644
--- a/app/assets/javascripts/runner/components/runner_delete_button.vue
+++ b/app/assets/javascripts/runner/components/runner_delete_button.vue
@@ -5,7 +5,12 @@ import { createAlert } from '~/flash';
import { sprintf } from '~/locale';
import { captureException } from '~/runner/sentry_utils';
import { getIdFromGraphQLId } from '~/graphql_shared/utils';
-import { I18N_DELETE_RUNNER, I18N_DELETED_TOAST } from '../constants';
+import {
+ I18N_DELETE_DISABLED_MANY_PROJECTS,
+ I18N_DELETE_DISABLED_UNKNOWN_REASON,
+ I18N_DELETE_RUNNER,
+ I18N_DELETED_TOAST,
+} from '../constants';
import RunnerDeleteModal from './runner_delete_modal.vue';
export default {
@@ -26,6 +31,11 @@ export default {
return runner?.id && runner?.shortSha;
},
},
+ disabled: {
+ type: Boolean,
+ required: false,
+ default: false,
+ },
compact: {
type: Boolean,
required: false,
@@ -75,7 +85,14 @@ export default {
return null;
},
tooltip() {
- // Only show tooltip when compact.
+ if (this.disabled && this.runner.projectCount > 1) {
+ return I18N_DELETE_DISABLED_MANY_PROJECTS;
+ }
+ if (this.disabled) {
+ return I18N_DELETE_DISABLED_UNKNOWN_REASON;
+ }
+
+ // Only show basic "delete" tooltip when compact.
// Also prevent a "sticky" tooltip: If this button is
// disabled, mouseout listeners don't run leaving the tooltip stuck
if (this.compact && !this.deleting) {
@@ -83,6 +100,14 @@ export default {
}
return '';
},
+ wrapperTabindex() {
+ if (this.disabled) {
+ // Trigger tooltip on keyboard-focusable wrapper
+ // See https://bootstrap-vue.org/docs/directives/tooltip
+ return '0';
+ }
+ return null;
+ },
},
methods: {
async onDelete() {
@@ -125,20 +150,22 @@ export default {
</script>
<template>
- <gl-button
- v-gl-tooltip.hover.viewport="tooltip"
- v-gl-modal="runnerDeleteModalId"
- :aria-label="ariaLabel"
- :icon="icon"
- :class="buttonClass"
- :loading="deleting"
- variant="danger"
- >
- {{ buttonContent }}
+ <div v-gl-tooltip="tooltip" class="btn-group" :tabindex="wrapperTabindex">
+ <gl-button
+ v-gl-modal="runnerDeleteModalId"
+ :aria-label="ariaLabel"
+ :icon="icon"
+ :class="buttonClass"
+ :loading="deleting"
+ :disabled="disabled"
+ variant="danger"
+ >
+ {{ buttonContent }}
+ </gl-button>
<runner-delete-modal
:modal-id="runnerDeleteModalId"
:runner-name="runnerName"
@primary="onDelete"
/>
- </gl-button>
+ </div>
</template>
diff --git a/app/assets/javascripts/runner/constants.js b/app/assets/javascripts/runner/constants.js
index bd5be2175adb1..1e6f9d7c66916 100644
--- a/app/assets/javascripts/runner/constants.js
+++ b/app/assets/javascripts/runner/constants.js
@@ -46,6 +46,12 @@ export const I18N_RESUME = __('Resume');
export const I18N_RESUME_TOOLTIP = s__('Runners|Resume accepting jobs');
export const I18N_DELETE_RUNNER = s__('Runners|Delete runner');
+export const I18N_DELETE_DISABLED_MANY_PROJECTS = s__(
+ 'Runners|Multi-project runners cannot be deleted',
+);
+export const I18N_DELETE_DISABLED_UNKNOWN_REASON = s__(
+ 'Runners|Runner cannot be deleted, please contact your administrator',
+);
export const I18N_DELETED_TOAST = s__('Runners|Runner %{name} was deleted');
export const I18N_LOCKED_RUNNER_DESCRIPTION = s__('Runners|You cannot assign to other projects');
diff --git a/app/assets/javascripts/runner/graphql/list/group_runners.query.graphql b/app/assets/javascripts/runner/graphql/list/group_runners.query.graphql
index b517f5e89a882..7aac2c1d4f2d4 100644
--- a/app/assets/javascripts/runner/graphql/list/group_runners.query.graphql
+++ b/app/assets/javascripts/runner/graphql/list/group_runners.query.graphql
@@ -30,6 +30,7 @@ query getGroupRunners(
editUrl
node {
...ListItem
+ projectCount # Used to determine why some project runners can't be deleted
}
}
pageInfo {
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 36f573bb5a3fb..fc0be9f54f4ab 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -32054,6 +32054,9 @@ msgstr ""
msgid "Runners|Members of the %{type} can register runners"
msgstr ""
+msgid "Runners|Multi-project runners cannot be deleted"
+msgstr ""
+
msgid "Runners|Name"
msgstr ""
@@ -32156,6 +32159,9 @@ msgstr ""
msgid "Runners|Runner assigned to project."
msgstr ""
+msgid "Runners|Runner cannot be deleted, please contact your administrator"
+msgstr ""
+
msgid "Runners|Runner is offline, last contact was %{runner_contact} ago"
msgstr ""
diff --git a/spec/frontend/runner/components/cells/runner_actions_cell_spec.js b/spec/frontend/runner/components/cells/runner_actions_cell_spec.js
index 0d57910686039..9ca99d1109b9d 100644
--- a/spec/frontend/runner/components/cells/runner_actions_cell_spec.js
+++ b/spec/frontend/runner/components/cells/runner_actions_cell_spec.js
@@ -92,6 +92,14 @@ describe('RunnerActionsCell', () => {
expect(findDeleteBtn().props('compact')).toBe(true);
});
+ it('Passes runner data to delete button', () => {
+ createComponent({
+ runner: mockRunner,
+ });
+
+ expect(findDeleteBtn().props('runner')).toEqual(mockRunner);
+ });
+
it('Emits delete events', () => {
const value = { name: 'Runner' };
@@ -104,7 +112,7 @@ describe('RunnerActionsCell', () => {
expect(wrapper.emitted('deleted')).toEqual([[value]]);
});
- it('Does not render the runner delete button when user cannot delete', () => {
+ it('Renders the runner delete disabled button when user cannot delete', () => {
createComponent({
runner: {
userPermissions: {
@@ -114,7 +122,7 @@ describe('RunnerActionsCell', () => {
},
});
- expect(findDeleteBtn().exists()).toBe(false);
+ expect(findDeleteBtn().props('disabled')).toBe(true);
});
});
});
diff --git a/spec/frontend/runner/components/runner_delete_button_spec.js b/spec/frontend/runner/components/runner_delete_button_spec.js
index 81c870f23cfbc..262658800bdaf 100644
--- a/spec/frontend/runner/components/runner_delete_button_spec.js
+++ b/spec/frontend/runner/components/runner_delete_button_spec.js
@@ -9,7 +9,11 @@ import waitForPromises from 'helpers/wait_for_promises';
import { captureException } from '~/runner/sentry_utils';
import { getIdFromGraphQLId } from '~/graphql_shared/utils';
import { createAlert } from '~/flash';
-import { I18N_DELETE_RUNNER } from '~/runner/constants';
+import {
+ I18N_DELETE_RUNNER,
+ I18N_DELETE_DISABLED_MANY_PROJECTS,
+ I18N_DELETE_DISABLED_UNKNOWN_REASON,
+} from '~/runner/constants';
import RunnerDeleteButton from '~/runner/components/runner_delete_button.vue';
import RunnerDeleteModal from '~/runner/components/runner_delete_modal.vue';
@@ -27,11 +31,12 @@ describe('RunnerDeleteButton', () => {
let wrapper;
let runnerDeleteHandler;
- const getTooltip = () => getBinding(wrapper.element, 'gl-tooltip').value;
- const getModal = () => getBinding(wrapper.element, 'gl-modal').value;
const findBtn = () => wrapper.findComponent(GlButton);
const findModal = () => wrapper.findComponent(RunnerDeleteModal);
+ const getTooltip = () => getBinding(wrapper.element, 'gl-tooltip').value;
+ const getModal = () => getBinding(findBtn().element, 'gl-modal').value;
+
const createComponent = ({ props = {}, mountFn = shallowMountExtended } = {}) => {
const { runner, ...propsData } = props;
@@ -88,6 +93,10 @@ describe('RunnerDeleteButton', () => {
expect(findModal().props('runnerName')).toBe(`#${mockRunnerId} (${mockRunner.shortSha})`);
});
+ it('Does not have tabindex when button is enabled', () => {
+ expect(wrapper.attributes('tabindex')).toBeUndefined();
+ });
+
it('Displays a modal when clicked', () => {
const modalId = `delete-runner-modal-${mockRunnerId}`;
@@ -230,4 +239,29 @@ describe('RunnerDeleteButton', () => {
});
});
});
+
+ describe.each`
+ reason | runner | tooltip
+ ${'runner belongs to more than 1 project'} | ${{ projectCount: 2 }} | ${I18N_DELETE_DISABLED_MANY_PROJECTS}
+ ${'unknown reason'} | ${{}} | ${I18N_DELETE_DISABLED_UNKNOWN_REASON}
+ `('When button is disabled because $reason', ({ runner, tooltip }) => {
+ beforeEach(() => {
+ createComponent({
+ props: {
+ disabled: true,
+ runner,
+ },
+ });
+ });
+
+ it('Displays a disabled delete button', () => {
+ expect(findBtn().props('disabled')).toBe(true);
+ });
+
+ it(`Tooltip "${tooltip}" is shown`, () => {
+ // tabindex is required for a11y
+ expect(wrapper.attributes('tabindex')).toBe('0');
+ expect(getTooltip()).toBe(tooltip);
+ });
+ });
});
--
GitLab