From 34f88cebdff1ef06a98a4a8b01573869f597f27b Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Wed, 5 Feb 2025 18:06:27 +0100 Subject: [PATCH 1/4] Protected packages: Add minimumAccessLevelForDelete in proj. settings UI This MR integrates the attribute `minimumAccessLevelForDelete` to the project settings UI. This attribute is used to set the minimum access level required to delete a package. With this, project owners can use the frontend to set the minimum access level. The attribute is similarly integrated as the other attribute `minimumAccessLevelForPush`. Changelog: added --- .../packages_protection_rule_form.vue | 50 +++++++++++- .../components/packages_protection_rules.vue | 35 +++++++- .../settings/project/constants.js | 28 ++++++- ..._packages_protection_rule.mutation.graphql | 1 + ...et_packages_protection_rules.query.graphql | 1 + .../packages_and_registries_controller.rb | 1 + locale/gitlab.pot | 15 ++++ .../packages_protection_rule_form_spec.js | 74 ++++++++++++++++- .../packages_protection_rules_spec.js | 79 +++++++++++++++++++ .../settings/project/settings/mock_data.js | 3 + ...packages_and_registries_controller_spec.rb | 1 + 11 files changed, 279 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue index 79586bc42dfedc..c5fea9dd749330 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue +++ b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue @@ -22,6 +22,11 @@ const PACKAGES_PROTECTION_RULES_SAVED_ERROR_MESSAGE = s__( 'PackageRegistry|Something went wrong while saving the package protection rule.', ); +const GRAPHQL_ACCESS_LEVEL_VALUE_NULL = null; +// const GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER = 'MAINTAINER'; +const GRAPHQL_ACCESS_LEVEL_VALUE_OWNER = 'OWNER'; +const GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN = 'ADMIN'; + export default { components: { GlButton, @@ -54,13 +59,20 @@ export default { packageNamePattern: this.rule?.packageNamePattern ?? '', packageType: this.rule?.packageType ?? 'NPM', minimumAccessLevelForPush: - this.rule?.minimumAccessLevelForPush ?? GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, + // this.isNewRule() ? this.rule.minimumAccessLevelForPush : GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, + this.rule ? this.rule.minimumAccessLevelForPush : GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, + minimumAccessLevelForDelete: this.rule + ? this.rule.minimumAccessLevelForDelete + : GRAPHQL_ACCESS_LEVEL_VALUE_OWNER, }, updateInProgress: false, alertErrorMessage: '', }; }, computed: { + isNewRule() { + return Boolean(this.rule); + }, mutation() { return this.rule ? updatePackagesProtectionRuleMutation @@ -112,6 +124,24 @@ export default { return packageTypeOptions.sort((a, b) => a.text.localeCompare(b.text)); }, + minimumAccessLevelForPushOptions() { + return [ + { value: GRAPHQL_ACCESS_LEVEL_VALUE_NULL, text: __('Developer (default)') }, + { value: GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, text: __('Maintainer') }, + { value: GRAPHQL_ACCESS_LEVEL_VALUE_OWNER, text: __('Owner') }, + { value: GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN, text: s__('AdminUsers|Administrator') }, + ]; + }, + minimumAccessLevelForDeleteOptions() { + return [ + { + value: GRAPHQL_ACCESS_LEVEL_VALUE_NULL, + text: s__('PackageRegistry|Maintainer (default)'), + }, + { value: GRAPHQL_ACCESS_LEVEL_VALUE_OWNER, text: __('Owner') }, + { value: GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN, text: s__('AdminUsers|Administrator') }, + ]; + }, }, methods: { submit() { @@ -153,7 +183,6 @@ export default { this.$emit('cancel'); }, }, - minimumAccessLevelForPushOptions: MinimumAccessLevelOptions, }; @@ -214,9 +243,22 @@ export default { + + + + diff --git a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue index 488ec241d9c199..fcc60849e6576d 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue +++ b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue @@ -17,11 +17,17 @@ import { getPackageTypeLabel } from '~/packages_and_registries/package_registry/ import deletePackagesProtectionRuleMutation from '~/packages_and_registries/settings/project/graphql/mutations/delete_packages_protection_rule.mutation.graphql'; import PackagesProtectionRuleForm from '~/packages_and_registries/settings/project/components/packages_protection_rule_form.vue'; import { getAccessLevelLabel } from '~/packages_and_registries/settings/project/utils'; +import { + PackagesMinimumAccessForPushLevelText, + PackagesMinimumAccessForDeleteLevelText, +} from '~/packages_and_registries/settings/project/constants'; import { s__, __ } from '~/locale'; +import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; const PAGINATION_DEFAULT_PER_PAGE = 10; const I18N_MINIMUM_ACCESS_LEVEL_FOR_PUSH = s__('PackageRegistry|Minimum access level for push'); +const I18N_MINIMUM_ACCESS_LEVEL_FOR_DELETE = s__('PackageRegistry|Minimum access level for delete'); export default { components: { @@ -40,6 +46,7 @@ export default { GlModal: GlModalDirective, GlTooltip: GlTooltipDirective, }, + mixins: [glFeatureFlagsMixin()], inject: ['projectPath'], i18n: { delete: __('Delete'), @@ -59,6 +66,7 @@ export default { ), }, minimumAccessLevelForPush: I18N_MINIMUM_ACCESS_LEVEL_FOR_PUSH, + minimumAccessLevelForDelete: I18N_MINIMUM_ACCESS_LEVEL_FOR_DELETE, }, data() { return { @@ -80,10 +88,17 @@ export default { ? s__('PackageRegistry|Edit protection rule') : s__('PackageRegistry|Add protection rule'); }, + fields() { + if (this.glFeatures.packagesProtectedPackagesDelete) { + return this.$options.fields; + } + return this.$options.fields.filter((field) => field.key !== 'minimumAccessLevelForDelete'); + }, tableItems() { return this.packageProtectionRulesQueryResult.map((packagesProtectionRule) => { return { id: packagesProtectionRule.id, + minimumAccessLevelForDelete: packagesProtectionRule.minimumAccessLevelForDelete, minimumAccessLevelForPush: packagesProtectionRule.minimumAccessLevelForPush, packageNamePattern: packagesProtectionRule.packageNamePattern, packageType: packagesProtectionRule.packageType, @@ -219,6 +234,11 @@ export default { label: I18N_MINIMUM_ACCESS_LEVEL_FOR_PUSH, tdClass: '!gl-align-middle', }, + { + key: 'minimumAccessLevelForDelete', + label: I18N_MINIMUM_ACCESS_LEVEL_FOR_DELETE, + tdClass: '!gl-align-middle', + }, { key: 'rowActions', label: __('Actions'), @@ -228,6 +248,8 @@ export default { ], getAccessLevelLabel, getPackageTypeLabel, + minimumAccessForPushLevelText: PackagesMinimumAccessForPushLevelText, + minimumAccessForDeleteLevelText: PackagesMinimumAccessForDeleteLevelText, modal: { id: 'delete-package-protection-rule-confirmation-modal' }, modalActionPrimary: { text: s__('PackageRegistry|Delete package protection rule'), @@ -272,7 +294,7 @@ export default { v-else-if="containsTableItems" class="gl-border-t-1 gl-border-t-gray-100 gl-border-t-solid" :items="tableItems" - :fields="$options.fields" + :fields="fields" stacked="md" :aria-label="$options.i18n.settingBlockTitle" :busy="isLoadingPackageProtectionRules" @@ -289,7 +311,16 @@ export default { + + diff --git a/app/assets/javascripts/packages_and_registries/settings/project/constants.js b/app/assets/javascripts/packages_and_registries/settings/project/constants.js index d8764bba6f7373..c5d27c5108bf6c 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/constants.js +++ b/app/assets/javascripts/packages_and_registries/settings/project/constants.js @@ -122,9 +122,9 @@ export const GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER = 'MAINTAINER'; const GRAPHQL_ACCESS_LEVEL_VALUE_OWNER = 'OWNER'; export const MinimumAccessLevelText = { - [GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN]: s__('AdminUsers|Administrator'), [GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER]: __('Maintainer'), [GRAPHQL_ACCESS_LEVEL_VALUE_OWNER]: __('Owner'), + [GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN]: s__('AdminUsers|Administrator'), }; export const MinimumAccessLevelOptions = [ @@ -133,6 +133,32 @@ export const MinimumAccessLevelOptions = [ { value: GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN, text: s__('AdminUsers|Administrator') }, ]; +export const PackagesMinimumAccessForPushLevelText = { + null: s__('PackageRegistry|Developer (default)'), + [GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER]: __('Maintainer'), + [GRAPHQL_ACCESS_LEVEL_VALUE_OWNER]: __('Owner'), + [GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN]: s__('AdminUsers|Administrator'), +}; + +export const PackagesMinimumAccessForDeleteLevelText = { + null: s__('PackageRegistry|Maintainer (default)'), + [GRAPHQL_ACCESS_LEVEL_VALUE_OWNER]: __('Owner'), + [GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN]: s__('AdminUsers|Administrator'), +}; + +export const PackagesMinimumAccessLevelOptions = [ + { value: null, text: __('Developer (default)') }, + { value: GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, text: __('Maintainer') }, + { value: GRAPHQL_ACCESS_LEVEL_VALUE_OWNER, text: __('Owner') }, + { value: GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN, text: s__('AdminUsers|Administrator') }, +]; + +export const PackagesMinimumAccessLevelForDeleteOptions = [ + { value: null, text: __('Developer (default)') }, + { value: GRAPHQL_ACCESS_LEVEL_VALUE_OWNER, text: __('Owner') }, + { value: GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN, text: s__('AdminUsers|Administrator') }, +]; + export const FETCH_SETTINGS_ERROR_MESSAGE = s__( 'ContainerRegistry|Something went wrong while fetching the cleanup policy.', ); diff --git a/app/assets/javascripts/packages_and_registries/settings/project/graphql/mutations/create_packages_protection_rule.mutation.graphql b/app/assets/javascripts/packages_and_registries/settings/project/graphql/mutations/create_packages_protection_rule.mutation.graphql index 5286c70129d66c..372a5f2743e5e3 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/graphql/mutations/create_packages_protection_rule.mutation.graphql +++ b/app/assets/javascripts/packages_and_registries/settings/project/graphql/mutations/create_packages_protection_rule.mutation.graphql @@ -4,6 +4,7 @@ mutation createPackagesProtectionRule($input: CreatePackagesProtectionRuleInput! id packageNamePattern packageType + minimumAccessLevelForDelete minimumAccessLevelForPush } errors diff --git a/app/assets/javascripts/packages_and_registries/settings/project/graphql/queries/get_packages_protection_rules.query.graphql b/app/assets/javascripts/packages_and_registries/settings/project/graphql/queries/get_packages_protection_rules.query.graphql index dd9c8187f92559..3abdeaec5a71e2 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/graphql/queries/get_packages_protection_rules.query.graphql +++ b/app/assets/javascripts/packages_and_registries/settings/project/graphql/queries/get_packages_protection_rules.query.graphql @@ -15,6 +15,7 @@ query getProjectPackageProtectionRules( packageNamePattern packageType minimumAccessLevelForPush + minimumAccessLevelForDelete } pageInfo { ...PageInfo diff --git a/app/controllers/projects/settings/packages_and_registries_controller.rb b/app/controllers/projects/settings/packages_and_registries_controller.rb index 8bf28f43be8948..d2e04c538f52a6 100644 --- a/app/controllers/projects/settings/packages_and_registries_controller.rb +++ b/app/controllers/projects/settings/packages_and_registries_controller.rb @@ -35,6 +35,7 @@ def registry_settings_enabled! def set_feature_flag_packages_protected_packages push_frontend_feature_flag(:packages_protected_packages_conan, project) push_frontend_feature_flag(:packages_protected_packages_maven, project) + push_frontend_feature_flag(:packages_protected_packages_delete, project) end def set_feature_flag_container_registry_protected_tags diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 253b92d27526b9..279f86f6e7b3fd 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -21325,6 +21325,12 @@ msgstr "" msgid "Developer" msgstr "" +msgid "Developer (Default)" +msgstr "" + +msgid "Developer (default)" +msgstr "" + msgid "Development widget is not enabled for this work item type" msgstr "" @@ -42015,6 +42021,9 @@ msgstr "" msgid "PackageRegistry|Deleting this package while request forwarding is enabled for the project can pose a security risk. Do you want to delete %{name} version %{version} anyway? %{docLinkStart}What are the risks?%{docLinkEnd}" msgstr "" +msgid "PackageRegistry|Developer (Default)" +msgstr "" + msgid "PackageRegistry|Duplicate packages" msgstr "" @@ -42123,6 +42132,9 @@ msgstr "" msgid "PackageRegistry|Machine learning model" msgstr "" +msgid "PackageRegistry|Maintainer (Default)" +msgstr "" + msgid "PackageRegistry|Manage storage used by package assets" msgstr "" @@ -42138,6 +42150,9 @@ msgstr "" msgid "PackageRegistry|Maven XML" msgstr "" +msgid "PackageRegistry|Minimum access level for delete" +msgstr "" + msgid "PackageRegistry|Minimum access level for push" msgstr "" diff --git a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js index 7c7e1a3b69131c..a0a20deef09473 100644 --- a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js +++ b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js @@ -26,6 +26,7 @@ describe('Packages Protection Rule Form', () => { glFeatures: { packagesProtectedPackagesConan: true, packagesProtectedPackagesMaven: true, + packagesProtectedPackagesDelete: true, }, }; @@ -34,6 +35,8 @@ describe('Packages Protection Rule Form', () => { const findPackageTypeSelect = () => wrapper.findByRole('combobox', { name: /type/i }); const findMinimumAccessLevelForPushSelect = () => wrapper.findByRole('combobox', { name: /minimum access level for push/i }); + const findMinimumAccessLevelForDeleteSelect = () => + wrapper.findByRole('combobox', { name: /minimum access level for delete/i }); const findCancelButton = () => wrapper.findByRole('button', { name: /cancel/i }); const findSubmitButton = () => wrapper.findByTestId('submit-btn'); const findForm = () => wrapper.findComponent(GlForm); @@ -130,7 +133,39 @@ describe('Packages Protection Rule Form', () => { const minimumAccessLevelForPushSelectOptions = findMinimumAccessLevelForPushSelect() .findAll('option') .wrappers.map((option) => option.element.value); - expect(minimumAccessLevelForPushSelectOptions).toEqual(['MAINTAINER', 'OWNER', 'ADMIN']); + expect(minimumAccessLevelForPushSelectOptions).toEqual([ + '', + 'MAINTAINER', + 'OWNER', + 'ADMIN', + ]); + }); + }); + + describe('form field "minimumAccessLevelForDeleteSelect"', () => { + it('contains only the options for maintainer and owner', () => { + mountComponent(); + + expect(findMinimumAccessLevelForDeleteSelect().exists()).toBe(true); + const minimumAccessLevelForDeleteSelectOptions = findMinimumAccessLevelForDeleteSelect() + .findAll('option') + .wrappers.map((option) => option.element.value); + expect(minimumAccessLevelForDeleteSelectOptions).toEqual(['', 'OWNER', 'ADMIN']); + }); + + describe('when feature flag packagesProtectedPackagesDelete is disabled', () => { + it('does not show form field "minimumAccessLevelForDeleteSelect"', () => { + mountComponent({ + provide: { + ...defaultProvidedValues, + glFeatures: { + ...defaultProvidedValues.glFeatures, + packagesProtectedPackagesDelete: false, + }, + }, + }); + expect(findMinimumAccessLevelForDeleteSelect().exists()).toBe(false); + }); }); }); @@ -146,6 +181,7 @@ describe('Packages Protection Rule Form', () => { expect(findPackageNamePatternInput().attributes('disabled')).toBe('disabled'); expect(findPackageTypeSelect().attributes('disabled')).toBe('disabled'); expect(findMinimumAccessLevelForPushSelect().attributes('disabled')).toBe('disabled'); + expect(findMinimumAccessLevelForDeleteSelect().attributes('disabled')).toBe('disabled'); }); it('displays a loading spinner', () => { @@ -256,11 +292,40 @@ describe('Packages Protection Rule Form', () => { await submitForm(); expect(mutationResolver).toHaveBeenCalledWith({ - input: { projectPath: 'path', ...createPackagesProtectionRuleMutationInput }, + input: { + projectPath: 'path', + ...createPackagesProtectionRuleMutationInput, + minimumAccessLevelForDelete: 'OWNER', + }, }); expect(updatePackagesProtectionRuleMutationResolver).not.toHaveBeenCalled(); }); + it('dispatches correct apollo mutation when no minimumAccessLevelForPush is selected', async () => { + const mutationResolver = jest + .fn() + .mockResolvedValue(createPackagesProtectionRuleMutationPayload()); + + mountComponentWithApollo({ mutationResolver }); + + await findPackageNamePatternInput().setValue( + createPackagesProtectionRuleMutationInput.packageNamePattern, + ); + await findMinimumAccessLevelForPushSelect().setValue(''); + await findMinimumAccessLevelForDeleteSelect().setValue('ADMIN'); + + await submitForm(); + + expect(mutationResolver).toHaveBeenCalledWith({ + input: { + projectPath: 'path', + ...createPackagesProtectionRuleMutationInput, + minimumAccessLevelForPush: null, + minimumAccessLevelForDelete: 'ADMIN', + }, + }); + }); + it('emits event "submit" when apollo mutation successful', async () => { const mutationResolver = jest .fn() @@ -316,7 +381,10 @@ describe('Packages Protection Rule Form', () => { createPackagesProtectionRuleMutationInput.packageNamePattern, ); await findMinimumAccessLevelForPushSelect().findAll('option').at(0).setSelected(); + await findMinimumAccessLevelForDeleteSelect().findAll('option').at(2).setSelected(); + findForm().trigger('submit'); + await waitForPromises(); }; @@ -343,6 +411,8 @@ describe('Packages Protection Rule Form', () => { input: { id: packagesProtectionRulesData[0].id, ...createPackagesProtectionRuleMutationInput, + minimumAccessLevelForDelete: 'ADMIN', + minimumAccessLevelForPush: null, }, }); }); diff --git a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js index 72b044f0ee8b9a..ae5a55e16c4343 100644 --- a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js +++ b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js @@ -15,6 +15,7 @@ import { packagesProtectionRulesData, deletePackagesProtectionRuleMutationPayload, } from '../mock_data'; +import { PackagesMinimumAccessForPushLevelText } from '../../../../../../../app/assets/javascripts/packages_and_registries/settings/project/constants'; Vue.use(VueApollo); @@ -24,6 +25,9 @@ describe('Packages protection rules project settings', () => { const defaultProvidedValues = { projectPath: 'path', + glFeatures: { + packagesProtectedPackagesDelete: true, + }, }; const $toast = { show: jest.fn() }; @@ -36,6 +40,10 @@ describe('Packages protection rules project settings', () => { extendedWrapper(wrapper.findByRole('table', { name: /protected packages/i })); const findTableBody = () => extendedWrapper(findTable().findAllByRole('rowgroup').at(1)); const findTableRow = (i) => extendedWrapper(findTableBody().findAllByRole('row').at(i)); + const findMinimumAccessLevelForPushInTableRow = (i) => + findTableRow(i).findByTestId('minimum-access-level-push-value'); + const findMinimumAccessLevelForDeleteInTableRow = (i) => + findTableRow(i).findByTestId('minimum-access-level-delete-value'); const findTableRowButtonDelete = (i) => extendedWrapper(wrapper.findAllByTestId('delete-rule-btn').at(i)); const findTableLoadingIcon = () => wrapper.findComponent(GlLoadingIcon); @@ -134,6 +142,7 @@ describe('Packages protection rules project settings', () => { expect(findTableRow(i).text()).toContain(protectionRule.packageNamePattern); expect(findTableRow(i).text()).toContain('npm'); expect(findTableRow(i).text()).toContain('Maintainer'); + expect(findTableRow(i).text()).toContain('Maintainer'); }, ); }); @@ -291,6 +300,76 @@ describe('Packages protection rules project settings', () => { }); }); + describe('column "Minimum access level for push"', () => { + it('renders correct value for blank value', async () => { + const packagesProtectionRuleQueryResolver = jest.fn().mockResolvedValue( + packagesProtectionRuleQueryPayload({ + nodes: [ + { + ...packagesProtectionRulesData[0], + minimumAccessLevelForPush: null, + minimumAccessLevelForDelete: 'ADMIN', + }, + ], + }), + ); + + createComponent({ packagesProtectionRuleQueryResolver }); + + await waitForPromises(); + + expect(findMinimumAccessLevelForPushInTableRow(0).text()).toContain('Developer (default)'); + expect(findMinimumAccessLevelForDeleteInTableRow(0).text()).toContain('Administrator'); + }); + }); + + describe('column "Minimum access level for delete"', () => { + it('renders correct value for blank value', async () => { + const packagesProtectionRuleQueryResolver = jest.fn().mockResolvedValue( + packagesProtectionRuleQueryPayload({ + nodes: [ + { + ...packagesProtectionRulesData[0], + minimumAccessLevelForPush: 'OWNER', + minimumAccessLevelForDelete: null, + }, + ], + }), + ); + + createComponent({ packagesProtectionRuleQueryResolver }); + + await waitForPromises(); + + expect(findMinimumAccessLevelForPushInTableRow(0).text()).toContain('Owner'); + expect(findMinimumAccessLevelForDeleteInTableRow(0).text()).toContain( + 'Maintainer (default)', + ); + }); + + describe('when feature flag packagesProtectedPackagesDelete is disabled', () => { + const findTableColumnHeaderMinimumAccessLevelForDelete = () => + wrapper.findByRole('columnheader', { name: /minimum access level for delete/i }); + + it('does not show column "Minimum access level for delete"', async () => { + createComponent({ + provide: { + ...defaultProvidedValues, + glFeatures: { + ...defaultProvidedValues.glFeatures, + packagesProtectedPackagesDelete: false, + }, + }, + }); + + await waitForPromises(); + + expect(findTableColumnHeaderMinimumAccessLevelForDelete().exists()).toBe(false); + expect(findMinimumAccessLevelForDeleteInTableRow(0).exists()).toBe(false); + }); + }); + }); + describe('column "Actions"', () => { describe('button "Delete"', () => { it('exists in table', async () => { diff --git a/spec/frontend/packages_and_registries/settings/project/settings/mock_data.js b/spec/frontend/packages_and_registries/settings/project/settings/mock_data.js index a49a76103aa2cf..3fee2fc83faf1e 100644 --- a/spec/frontend/packages_and_registries/settings/project/settings/mock_data.js +++ b/spec/frontend/packages_and_registries/settings/project/settings/mock_data.js @@ -98,12 +98,14 @@ export const packagesProtectionRulesData = [ id: `gid://gitlab/Packages::Protection::Rule/${i}`, packageNamePattern: `@flight/flight-maintainer-${i}-*`, packageType: 'NPM', + minimumAccessLevelForDelete: 'OWNER', minimumAccessLevelForPush: 'MAINTAINER', })), { id: 'gid://gitlab/Packages::Protection::Rule/16', packageNamePattern: '@flight/flight-owner-16-*', packageType: 'NPM', + minimumAccessLevelForDelete: 'OWNER', minimumAccessLevelForPush: 'OWNER', }, ]; @@ -145,6 +147,7 @@ export const createPackagesProtectionRuleMutationPayload = ({ override, errors = export const createPackagesProtectionRuleMutationInput = { packageNamePattern: `@flight/flight-developer-14-*`, packageType: 'NPM', + minimumAccessLevelForDelete: 'MAINTAINER', minimumAccessLevelForPush: 'MAINTAINER', }; diff --git a/spec/requests/projects/settings/packages_and_registries_controller_spec.rb b/spec/requests/projects/settings/packages_and_registries_controller_spec.rb index b72e8668065c83..3a742d92f914b0 100644 --- a/spec/requests/projects/settings/packages_and_registries_controller_spec.rb +++ b/spec/requests/projects/settings/packages_and_registries_controller_spec.rb @@ -29,6 +29,7 @@ it_behaves_like 'pushed feature flag', :packages_protected_packages_conan it_behaves_like 'pushed feature flag', :packages_protected_packages_maven + it_behaves_like 'pushed feature flag', :packages_protected_packages_delete it_behaves_like 'pushed feature flag', :container_registry_protected_tags end end -- GitLab From 5d29018f7611fb321c2628257a9323a0367bd853 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Wed, 19 Mar 2025 12:16:12 +0100 Subject: [PATCH 2/4] refactor: Applying suggestions and patch from @markrian Fix tests for Vue 3 by appliying the patch suggested by @markrian, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180416#note_2407815646 --- .../packages_protection_rule_form.vue | 40 +++++++++---- .../packages_protection_rule_form_spec.js | 58 ++++++++++++++++++- 2 files changed, 84 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue index c5fea9dd749330..f852321d5e2152 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue +++ b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue @@ -13,17 +13,15 @@ import createPackagesProtectionRuleMutation from '~/packages_and_registries/sett import updatePackagesProtectionRuleMutation from '~/packages_and_registries/settings/project/graphql/mutations/update_packages_protection_rule.mutation.graphql'; import { s__, __ } from '~/locale'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; -import { - MinimumAccessLevelOptions, - GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, -} from '~/packages_and_registries/settings/project/constants'; +import { GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER } from '~/packages_and_registries/settings/project/constants'; const PACKAGES_PROTECTION_RULES_SAVED_ERROR_MESSAGE = s__( 'PackageRegistry|Something went wrong while saving the package protection rule.', ); -const GRAPHQL_ACCESS_LEVEL_VALUE_NULL = null; -// const GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER = 'MAINTAINER'; +// Needs to be an empty string instead of `null` for @vue/compat. The value +// should be transformed back to `null` as an input to the GraphQL query. +const GRAPHQL_ACCESS_LEVEL_VALUE_NULL = ''; const GRAPHQL_ACCESS_LEVEL_VALUE_OWNER = 'OWNER'; const GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN = 'ADMIN'; @@ -58,11 +56,14 @@ export default { packageProtectionRuleFormData: { packageNamePattern: this.rule?.packageNamePattern ?? '', packageType: this.rule?.packageType ?? 'NPM', - minimumAccessLevelForPush: - // this.isNewRule() ? this.rule.minimumAccessLevelForPush : GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, - this.rule ? this.rule.minimumAccessLevelForPush : GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, + // minimumAccessLevelForPush: this.isExistingRule + // ? this.rule.minimumAccessLevelForPush || GRAPHQL_ACCESS_LEVEL_VALUE_NULL + // : GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, + minimumAccessLevelForPush: this.rule + ? this.rule.minimumAccessLevelForPush || GRAPHQL_ACCESS_LEVEL_VALUE_NULL + : GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, minimumAccessLevelForDelete: this.rule - ? this.rule.minimumAccessLevelForDelete + ? this.rule.minimumAccessLevelForDelete || GRAPHQL_ACCESS_LEVEL_VALUE_NULL : GRAPHQL_ACCESS_LEVEL_VALUE_OWNER, }, updateInProgress: false, @@ -70,7 +71,7 @@ export default { }; }, computed: { - isNewRule() { + isExistingRule() { return Boolean(this.rule); }, mutation() { @@ -100,12 +101,20 @@ export default { return { projectPath: this.projectPath, ...this.packageProtectionRuleFormData, + minimumAccessLevelForPush: + this.packageProtectionRuleFormData.minimumAccessLevelForPush || null, + minimumAccessLevelForDelete: + this.packageProtectionRuleFormData.minimumAccessLevelForDelete || null, }; }, updatePackagesProtectionRuleMutationInput() { return { id: this.rule?.id, ...this.packageProtectionRuleFormData, + minimumAccessLevelForPush: + this.packageProtectionRuleFormData.minimumAccessLevelForPush || null, + minimumAccessLevelForDelete: + this.packageProtectionRuleFormData.minimumAccessLevelForDelete || null, }; }, packageTypeOptions() { @@ -125,12 +134,18 @@ export default { return packageTypeOptions.sort((a, b) => a.text.localeCompare(b.text)); }, minimumAccessLevelForPushOptions() { - return [ + const options = [ { value: GRAPHQL_ACCESS_LEVEL_VALUE_NULL, text: __('Developer (default)') }, { value: GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, text: __('Maintainer') }, { value: GRAPHQL_ACCESS_LEVEL_VALUE_OWNER, text: __('Owner') }, { value: GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN, text: s__('AdminUsers|Administrator') }, ]; + + if (!this.glFeatures.packagesProtectedPackagesDelete) { + options.shift(); + } + + return options; }, minimumAccessLevelForDeleteOptions() { return [ @@ -148,6 +163,7 @@ export default { this.clearAlertErrorMessage(); this.updateInProgress = true; + const input = this.rule ? this.updatePackagesProtectionRuleMutationInput : this.createPackagesProtectionRuleMutationInput; diff --git a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js index a0a20deef09473..8e7e2547c904b5 100644 --- a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js +++ b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js @@ -41,6 +41,14 @@ describe('Packages Protection Rule Form', () => { const findSubmitButton = () => wrapper.findByTestId('submit-btn'); const findForm = () => wrapper.findComponent(GlForm); + const setSelectValue = async (selectWrapper, value) => { + await selectWrapper.setValue(value); + // Work around compat flag which prevents change event from being triggered by setValue. + // TODO: Disable WRAPPER_SET_VALUE_DOES_NOT_TRIGGER_CHANGE globally: + // https://gitlab.com/gitlab-org/gitlab/-/issues/526008 + await selectWrapper.trigger('change'); + }; + const mountComponent = ({ data, config, props, provide = defaultProvidedValues } = {}) => { wrapper = mountExtended(PackagesProtectionRuleForm, { provide, @@ -140,6 +148,39 @@ describe('Packages Protection Rule Form', () => { 'ADMIN', ]); }); + + it('sets correct option for "null" value', async () => { + mountComponent({ + props: { + rule: { ...packagesProtectionRulesData[0], minimumAccessLevelForPush: null }, + }, + }); + + expect(findMinimumAccessLevelForPushSelect().element.value).toBe(''); + + }); + + describe('when feature flag packagesProtectedPackagesDelete is disabled', () => { + it('does not show option "Developer (default)"', () => { + mountComponent({ + provide: { + ...defaultProvidedValues, + glFeatures: { + ...defaultProvidedValues.glFeatures, + packagesProtectedPackagesDelete: false, + }, + }, + }); + + expect(findMinimumAccessLevelForPushSelect().exists()).toBe(true); + + const minimumAccessLevelForPushSelectOptions = findMinimumAccessLevelForPushSelect() + .findAll('option') + .wrappers.map((option) => option.element.value); + + expect(minimumAccessLevelForPushSelectOptions).toEqual(['MAINTAINER', 'OWNER', 'ADMIN']); + }); + }); }); describe('form field "minimumAccessLevelForDeleteSelect"', () => { @@ -153,6 +194,19 @@ describe('Packages Protection Rule Form', () => { expect(minimumAccessLevelForDeleteSelectOptions).toEqual(['', 'OWNER', 'ADMIN']); }); + describe('when form has prop "rule"', () => { + it('sets correct option for "null" value', async () => { + mountComponent({ + props: { + rule: { ...packagesProtectionRulesData[0], minimumAccessLevelForDelete: null }, + }, + }); + + expect(findMinimumAccessLevelForDeleteSelect().element.value).toBe(''); + + }); + }); + describe('when feature flag packagesProtectedPackagesDelete is disabled', () => { it('does not show form field "minimumAccessLevelForDeleteSelect"', () => { mountComponent({ @@ -311,8 +365,8 @@ describe('Packages Protection Rule Form', () => { await findPackageNamePatternInput().setValue( createPackagesProtectionRuleMutationInput.packageNamePattern, ); - await findMinimumAccessLevelForPushSelect().setValue(''); - await findMinimumAccessLevelForDeleteSelect().setValue('ADMIN'); + await setSelectValue(findMinimumAccessLevelForPushSelect(), ''); + await setSelectValue(findMinimumAccessLevelForDeleteSelect(), 'ADMIN'); await submitForm(); -- GitLab From 0c0b55f59e01a744c89dccc7a1be1a3a1c1c2f24 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Tue, 25 Mar 2025 20:24:27 +0100 Subject: [PATCH 3/4] refactor: Apply ternary operator for feature flag handling Apply suggestion from @psjakubowska from another related MR, see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/184810#note_2415469047 --- .../components/packages_protection_rule_form.vue | 12 ++++-------- .../project/components/packages_protection_rules.vue | 7 +++---- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue index f852321d5e2152..cace49df735666 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue +++ b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue @@ -134,18 +134,14 @@ export default { return packageTypeOptions.sort((a, b) => a.text.localeCompare(b.text)); }, minimumAccessLevelForPushOptions() { - const options = [ - { value: GRAPHQL_ACCESS_LEVEL_VALUE_NULL, text: __('Developer (default)') }, + return [ + ...(this.glFeatures.packagesProtectedPackagesDelete + ? [{ value: GRAPHQL_ACCESS_LEVEL_VALUE_NULL, text: __('Developer (default)') }] + : []), { value: GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, text: __('Maintainer') }, { value: GRAPHQL_ACCESS_LEVEL_VALUE_OWNER, text: __('Owner') }, { value: GRAPHQL_ACCESS_LEVEL_VALUE_ADMIN, text: s__('AdminUsers|Administrator') }, ]; - - if (!this.glFeatures.packagesProtectedPackagesDelete) { - options.shift(); - } - - return options; }, minimumAccessLevelForDeleteOptions() { return [ diff --git a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue index fcc60849e6576d..858eab0ee9641a 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue +++ b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rules.vue @@ -89,10 +89,9 @@ export default { : s__('PackageRegistry|Add protection rule'); }, fields() { - if (this.glFeatures.packagesProtectedPackagesDelete) { - return this.$options.fields; - } - return this.$options.fields.filter((field) => field.key !== 'minimumAccessLevelForDelete'); + return this.glFeatures.packagesProtectedPackagesDelete + ? this.$options.fields + : this.$options.fields.filter((field) => field.key !== 'minimumAccessLevelForDelete'); }, tableItems() { return this.packageProtectionRulesQueryResult.map((packagesProtectionRule) => { -- GitLab From 7d47738eab3e615bad7178b6883879ebf0d96c94 Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Tue, 25 Mar 2025 20:24:57 +0100 Subject: [PATCH 4/4] refactor: Finalizing MR - Fix final eslint warnings - Remove commented code - Add helper method `isExistingRule()` - Add test helper methods --- .../packages_protection_rule_form.vue | 27 +++++++------- locale/gitlab.pot | 7 +--- .../packages_protection_rule_form_spec.js | 37 ++++++++++--------- .../packages_protection_rules_spec.js | 1 - 4 files changed, 34 insertions(+), 38 deletions(-) diff --git a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue index cace49df735666..41681e373b0a78 100644 --- a/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue +++ b/app/assets/javascripts/packages_and_registries/settings/project/components/packages_protection_rule_form.vue @@ -56,14 +56,11 @@ export default { packageProtectionRuleFormData: { packageNamePattern: this.rule?.packageNamePattern ?? '', packageType: this.rule?.packageType ?? 'NPM', - // minimumAccessLevelForPush: this.isExistingRule - // ? this.rule.minimumAccessLevelForPush || GRAPHQL_ACCESS_LEVEL_VALUE_NULL - // : GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, - minimumAccessLevelForPush: this.rule - ? this.rule.minimumAccessLevelForPush || GRAPHQL_ACCESS_LEVEL_VALUE_NULL + minimumAccessLevelForPush: this.isExistingRule() + ? this.rule.minimumAccessLevelForPush ?? GRAPHQL_ACCESS_LEVEL_VALUE_NULL : GRAPHQL_ACCESS_LEVEL_VALUE_MAINTAINER, - minimumAccessLevelForDelete: this.rule - ? this.rule.minimumAccessLevelForDelete || GRAPHQL_ACCESS_LEVEL_VALUE_NULL + minimumAccessLevelForDelete: this.isExistingRule() + ? this.rule.minimumAccessLevelForDelete ?? GRAPHQL_ACCESS_LEVEL_VALUE_NULL : GRAPHQL_ACCESS_LEVEL_VALUE_OWNER, }, updateInProgress: false, @@ -71,22 +68,21 @@ export default { }; }, computed: { - isExistingRule() { - return Boolean(this.rule); - }, mutation() { - return this.rule + return this.isExistingRule() ? updatePackagesProtectionRuleMutation : createPackagesProtectionRuleMutation; }, mutationKey() { - return this.rule ? 'updatePackagesProtectionRule' : 'createPackagesProtectionRule'; + return this.isExistingRule() + ? 'updatePackagesProtectionRule' + : 'createPackagesProtectionRule'; }, showLoadingIcon() { return this.updateInProgress; }, submitButtonText() { - return this.rule ? __('Save changes') : s__('PackageRegistry|Add rule'); + return this.isExistingRule() ? __('Save changes') : s__('PackageRegistry|Add rule'); }, isEmptyPackageName() { return !this.packageProtectionRuleFormData.packageNamePattern; @@ -155,12 +151,15 @@ export default { }, }, methods: { + isExistingRule() { + return Boolean(this.rule); + }, submit() { this.clearAlertErrorMessage(); this.updateInProgress = true; - const input = this.rule + const input = this.isExistingRule() ? this.updatePackagesProtectionRuleMutationInput : this.createPackagesProtectionRuleMutationInput; diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 279f86f6e7b3fd..777319a8bc413a 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -21325,9 +21325,6 @@ msgstr "" msgid "Developer" msgstr "" -msgid "Developer (Default)" -msgstr "" - msgid "Developer (default)" msgstr "" @@ -42021,7 +42018,7 @@ msgstr "" msgid "PackageRegistry|Deleting this package while request forwarding is enabled for the project can pose a security risk. Do you want to delete %{name} version %{version} anyway? %{docLinkStart}What are the risks?%{docLinkEnd}" msgstr "" -msgid "PackageRegistry|Developer (Default)" +msgid "PackageRegistry|Developer (default)" msgstr "" msgid "PackageRegistry|Duplicate packages" @@ -42132,7 +42129,7 @@ msgstr "" msgid "PackageRegistry|Machine learning model" msgstr "" -msgid "PackageRegistry|Maintainer (Default)" +msgid "PackageRegistry|Maintainer (default)" msgstr "" msgid "PackageRegistry|Manage storage used by package assets" diff --git a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js index 8e7e2547c904b5..497aa407aca23d 100644 --- a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js +++ b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rule_form_spec.js @@ -134,14 +134,16 @@ describe('Packages Protection Rule Form', () => { }); describe('form field "minimumAccessLevelForPushSelect"', () => { + const findMinimumAccessLevelForPushSelectOptionValues = () => + findMinimumAccessLevelForPushSelect() + .findAll('option') + .wrappers.map((option) => option.element.value); + it('contains only the options for maintainer and owner', () => { mountComponent(); expect(findMinimumAccessLevelForPushSelect().exists()).toBe(true); - const minimumAccessLevelForPushSelectOptions = findMinimumAccessLevelForPushSelect() - .findAll('option') - .wrappers.map((option) => option.element.value); - expect(minimumAccessLevelForPushSelectOptions).toEqual([ + expect(findMinimumAccessLevelForPushSelectOptionValues()).toEqual([ '', 'MAINTAINER', 'OWNER', @@ -149,7 +151,7 @@ describe('Packages Protection Rule Form', () => { ]); }); - it('sets correct option for "null" value', async () => { + it('sets correct option for "null" value', () => { mountComponent({ props: { rule: { ...packagesProtectionRulesData[0], minimumAccessLevelForPush: null }, @@ -157,7 +159,6 @@ describe('Packages Protection Rule Form', () => { }); expect(findMinimumAccessLevelForPushSelect().element.value).toBe(''); - }); describe('when feature flag packagesProtectedPackagesDelete is disabled', () => { @@ -173,29 +174,30 @@ describe('Packages Protection Rule Form', () => { }); expect(findMinimumAccessLevelForPushSelect().exists()).toBe(true); - - const minimumAccessLevelForPushSelectOptions = findMinimumAccessLevelForPushSelect() - .findAll('option') - .wrappers.map((option) => option.element.value); - - expect(minimumAccessLevelForPushSelectOptions).toEqual(['MAINTAINER', 'OWNER', 'ADMIN']); + expect(findMinimumAccessLevelForPushSelectOptionValues()).toEqual([ + 'MAINTAINER', + 'OWNER', + 'ADMIN', + ]); }); }); }); describe('form field "minimumAccessLevelForDeleteSelect"', () => { + const findMinimumAccessLevelForDeleteSelectOptionValues = () => + findMinimumAccessLevelForDeleteSelect() + .findAll('option') + .wrappers.map((option) => option.element.value); + it('contains only the options for maintainer and owner', () => { mountComponent(); expect(findMinimumAccessLevelForDeleteSelect().exists()).toBe(true); - const minimumAccessLevelForDeleteSelectOptions = findMinimumAccessLevelForDeleteSelect() - .findAll('option') - .wrappers.map((option) => option.element.value); - expect(minimumAccessLevelForDeleteSelectOptions).toEqual(['', 'OWNER', 'ADMIN']); + expect(findMinimumAccessLevelForDeleteSelectOptionValues()).toEqual(['', 'OWNER', 'ADMIN']); }); describe('when form has prop "rule"', () => { - it('sets correct option for "null" value', async () => { + it('sets correct option for "null" value', () => { mountComponent({ props: { rule: { ...packagesProtectionRulesData[0], minimumAccessLevelForDelete: null }, @@ -203,7 +205,6 @@ describe('Packages Protection Rule Form', () => { }); expect(findMinimumAccessLevelForDeleteSelect().element.value).toBe(''); - }); }); diff --git a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js index ae5a55e16c4343..73671048b85c4c 100644 --- a/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js +++ b/spec/frontend/packages_and_registries/settings/project/settings/components/packages_protection_rules_spec.js @@ -15,7 +15,6 @@ import { packagesProtectionRulesData, deletePackagesProtectionRuleMutationPayload, } from '../mock_data'; -import { PackagesMinimumAccessForPushLevelText } from '../../../../../../../app/assets/javascripts/packages_and_registries/settings/project/constants'; Vue.use(VueApollo); -- GitLab