From a967aeb767fadf749436a6b428ad1544fabbd661 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Thu, 20 Jul 2023 10:58:21 +0200 Subject: [PATCH 1/4] feat: Add graphql mutation to delete package protection rules This MR adds a new graphql endpoint for deleting package protection rules. Changelog: added --- .../packages/protection/rule/delete.rb | 40 ++++++++ app/graphql/types/mutation_type.rb | 1 + .../protection/delete_rule_service.rb | 48 ++++++++++ doc/api/graphql/reference/index.md | 31 ++++++ locale/gitlab.pot | 3 + .../packages/protection/rule/delete_spec.rb | 96 +++++++++++++++++++ .../protection/delete_rule_service_spec.rb | 44 +++++++++ 7 files changed, 263 insertions(+) create mode 100644 app/graphql/mutations/packages/protection/rule/delete.rb create mode 100644 app/services/packages/protection/delete_rule_service.rb create mode 100644 spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb create mode 100644 spec/services/packages/protection/delete_rule_service_spec.rb diff --git a/app/graphql/mutations/packages/protection/rule/delete.rb b/app/graphql/mutations/packages/protection/rule/delete.rb new file mode 100644 index 00000000000000..bd0159d3c23c8f --- /dev/null +++ b/app/graphql/mutations/packages/protection/rule/delete.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module Mutations + module Packages + module Protection + module Rule + class Delete < ::Mutations::BaseMutation + graphql_name 'DeletePackagesProtectionRule' + description 'Deletes a protection rule for packages. ' \ + 'Available only when feature flag `packages_protected_packages` is enabled.' + + authorize :admin_package + + argument :id, + ::Types::GlobalIDType[::Packages::Protection::Rule], + required: true, + description: 'Global ID of the package protection rule to delete.' + + field :package_protection_rule, + Types::Packages::Protection::RuleType, + null: true, + description: 'Packages protection rule that was deleted successfully.' + + def resolve(id:, **_kwargs) + if Feature.disabled?(:packages_protected_packages) + raise_resource_not_available_error!("'packages_protected_packages' feature flag is disabled") + end + + package_protection_rule = authorized_find!(id: id) + + response = ::Packages::Protection::DeleteRuleService.new(package_protection_rule, + current_user: current_user).execute + + { package_protection_rule: response.payload[:package_protection_rule], errors: response.errors } + end + end + end + end + end +end diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 9c292404aed81c..410fa90e514a80 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -173,6 +173,7 @@ class MutationType < BaseObject extensions: [::Gitlab::Graphql::Limit::FieldCallCount => { limit: 1 }] mount_mutation Mutations::Packages::DestroyFile mount_mutation Mutations::Packages::Protection::Rule::Create, alpha: { milestone: '16.5' } + mount_mutation Mutations::Packages::Protection::Rule::Delete, alpha: { milestone: '16.5' } mount_mutation Mutations::Packages::DestroyFiles mount_mutation Mutations::Packages::Cleanup::Policy::Update mount_mutation Mutations::Echo diff --git a/app/services/packages/protection/delete_rule_service.rb b/app/services/packages/protection/delete_rule_service.rb new file mode 100644 index 00000000000000..f6231a16324c8c --- /dev/null +++ b/app/services/packages/protection/delete_rule_service.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Packages + module Protection + class DeleteRuleService + include Gitlab::Allowable + + # Todo: Make private + attr_reader :package_protection_rule, :current_user + + def initialize(package_protection_rule, current_user:) + if package_protection_rule.blank? || current_user.blank? + raise ArgumentError, + "package_protection_rule and current_user must be set" + end + + @package_protection_rule = package_protection_rule + @current_user = current_user + end + + def execute + unless can?(current_user, :admin_package, package_protection_rule.project) + error_message = _('Unauthorized to delete a package protection rule') + return service_response_error(message: error_message) + end + + unless package_protection_rule.persisted? + return service_response_error(message: package_protection_rule.errors.full_messages) + end + + deleted_package_protection_rule = package_protection_rule.destroy + + ServiceResponse.success(payload: { package_protection_rule: deleted_package_protection_rule }) + rescue ArgumentError => e + service_response_error(message: e.message) + end + + private + + def service_response_error(message:) + ServiceResponse.error( + message: message, + payload: { package_protection_rule: nil } + ) + end + end + end +end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 24ce86eb5c9135..b60744e8a0294e 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -3033,6 +3033,31 @@ Input type: `DeleteAnnotationInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +### `Mutation.deletePackagesProtectionRule` + +Deletes a protection rule for packages. Available only when feature flag `packages_protected_packages` is enabled. + +WARNING: +**Introduced** in 16.5. +This feature is an Experiment. It can be changed or removed at any time. + +Input type: `DeletePackagesProtectionRuleInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `id` | [`PackagesProtectionRuleID!`](#packagesprotectionruleid) | Global ID of the package protection rule to delete. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +| `packageProtectionRule` | [`PackagesProtectionRule`](#packagesprotectionrule) | Packages protection rule that was deleted successfully. | + ### `Mutation.designManagementDelete` Input type: `DesignManagementDeleteInput` @@ -30751,6 +30776,12 @@ A `PackagesPackageID` is a global ID. It is encoded as a string. An example `PackagesPackageID` is: `"gid://gitlab/Packages::Package/1"`. +### `PackagesProtectionRuleID` + +A `PackagesProtectionRuleID` is a global ID. It is encoded as a string. + +An example `PackagesProtectionRuleID` is: `"gid://gitlab/Packages::Protection::Rule/1"`. + ### `PackagesPypiMetadatumID` A `PackagesPypiMetadatumID` is a global ID. It is encoded as a string. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index a928d73e9a2b45..e123c9063e8eda 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -50701,6 +50701,9 @@ msgstr "" msgid "Unauthorized to create an environment" msgstr "" +msgid "Unauthorized to delete a package protection rule" +msgstr "" + msgid "Unauthorized to update the environment" msgstr "" diff --git a/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb b/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb new file mode 100644 index 00000000000000..12c6463ba10b3b --- /dev/null +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Deleting a package protection rule', :aggregate_failures, feature_category: :package_registry do + include GraphqlHelpers + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } } + + let(:mutation) { graphql_mutation(:delete_packages_protection_rule, input) } + let(:input) { { id: package_protection_rule.to_global_id } } + + let(:current_user) { project.owner } + + subject { post_graphql_mutation(mutation, current_user: current_user) } + + context 'with existing package protection rule' do + let!(:package_protection_rule) { create(:package_protection_rule, project: project) } + + it_behaves_like 'a working GraphQL mutation' + + it 'deletes specified package protection rule' do + expect { subject } + .to change { project.package_protection_rules.include?(package_protection_rule) }.from(true).to(false) + .and change { ::Packages::Protection::Rule.count }.from(1).to(0) + end + + it { is_expected.tap { expect_graphql_errors_to_be_empty } } + end + + context 'with existing package protection rule belonging to other project' do + # package protection rule of other project + let_it_be(:package_protection_rule) { create(:package_protection_rule) } + + it 'does not delete the specified package protection rule' do + expect { subject }.to change { ::Packages::Protection::Rule.count }.by(0) + + expect(::Packages::Protection::Rule.find(package_protection_rule.id)).to be_present + end + + it 'responds with an error message' do + subject.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } + end + end + + context 'without existing package protection rule' do + let!(:package_protection_rule) { create(:package_protection_rule, project: project).destroy! } + + it 'does not delete the package protection rule (as it does not exist)' do + expect { subject }.to change { Packages::Protection::Rule.count }.by(0) + end + end + + context 'when current_user does not have permission' do + let_it_be(:package_protection_rule) { create(:package_protection_rule, project: project) } + + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } + let_it_be(:guest) { create(:user).tap { |u| project.add_guest(u) } } + let_it_be(:anonymous) { create(:user) } + + where(:current_user) do + [ref(:developer), ref(:reporter), ref(:guest), ref(:anonymous)] + end + + with_them do + it 'returns an error' do + subject.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } + end + + it do + expect { subject }.not_to change { ::Packages::Protection::Rule.count } + expect(::Packages::Protection::Rule.find(package_protection_rule.id)).to be_present + end + end + end + + context "when feature flag ':packages_protected_packages' disabled" do + let_it_be(:package_protection_rule) { create(:package_protection_rule, project: project) } + + before do + stub_feature_flags(packages_protected_packages: false) + end + + it 'responds with an error message' do + subject.tap { expect_graphql_errors_to_include(/'packages_protected_packages' feature flag is disabled/) } + end + + it 'does not delete the specified package protection rule' do + expect { subject }.to change { ::Packages::Protection::Rule.count }.by(0) + + expect(::Packages::Protection::Rule.find(package_protection_rule.id)).to be_present + end + end +end diff --git a/spec/services/packages/protection/delete_rule_service_spec.rb b/spec/services/packages/protection/delete_rule_service_spec.rb new file mode 100644 index 00000000000000..34695b7916f432 --- /dev/null +++ b/spec/services/packages/protection/delete_rule_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Protection::DeleteRuleService, '#execute', feature_category: :package_registry do + let_it_be(:project) { create(:project) } + + let(:current_user) { project.owner } + + subject { described_class.new(package_protection_rule, current_user: current_user).execute } + + context 'with existing package protection rule' do + let!(:package_protection_rule) { create(:package_protection_rule, project: project) } + + it { is_expected.to be_success } + it { is_expected.to have_attributes payload: { package_protection_rule: package_protection_rule } } + + it do + expect { subject } + .to change { project.reload.package_protection_rules }.from([package_protection_rule]).to([]) + .and change { ::Packages::Protection::Rule.count }.from(1).to(0) + end + end + + context 'with deleted package protection rule' do + let!(:package_protection_rule) { create(:package_protection_rule, project: project).destroy! } + + it { is_expected.to be_error } + it { is_expected.to have_attributes payload: { package_protection_rule: nil } } + end + + context 'without package protection rule' do + let(:package_protection_rule) { nil } + + it { expect { subject }.to raise_error(ArgumentError) } + end + + context 'without current_user' do + let(:current_user) { nil } + let(:package_protection_rule) { build_stubbed(:package_protection_rule, project: project) } + + it { expect { subject }.to raise_error(ArgumentError) } + end +end -- GitLab From de26ebfea09af51dd65ea7f5c0468ae84aad2afd Mon Sep 17 00:00:00 2001 From: Gerardo Date: Sun, 22 Oct 2023 08:46:09 +0200 Subject: [PATCH 2/4] refactor: Apply suggestion from @jpcyiza, @l.rosa This commit includes: - Refactoring the request test for graphql mutation based on review comments - Remove unnecessary code in delete_rule_service --- .../protection/delete_rule_service.rb | 8 +- .../packages/protection/rule/delete_spec.rb | 82 ++++++++----------- .../protection/delete_rule_service_spec.rb | 62 +++++++++++--- 3 files changed, 88 insertions(+), 64 deletions(-) diff --git a/app/services/packages/protection/delete_rule_service.rb b/app/services/packages/protection/delete_rule_service.rb index f6231a16324c8c..28db4446b0be6c 100644 --- a/app/services/packages/protection/delete_rule_service.rb +++ b/app/services/packages/protection/delete_rule_service.rb @@ -11,7 +11,7 @@ class DeleteRuleService def initialize(package_protection_rule, current_user:) if package_protection_rule.blank? || current_user.blank? raise ArgumentError, - "package_protection_rule and current_user must be set" + 'package_protection_rule and current_user must be set' end @package_protection_rule = package_protection_rule @@ -24,11 +24,7 @@ def execute return service_response_error(message: error_message) end - unless package_protection_rule.persisted? - return service_response_error(message: package_protection_rule.errors.full_messages) - end - - deleted_package_protection_rule = package_protection_rule.destroy + deleted_package_protection_rule = package_protection_rule.destroy! ServiceResponse.success(payload: { package_protection_rule: deleted_package_protection_rule }) rescue ArgumentError => e diff --git a/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb b/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb index 12c6463ba10b3b..d5b602d466430c 100644 --- a/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/delete_spec.rb @@ -6,55 +6,58 @@ include GraphqlHelpers let_it_be(:project) { create(:project, :repository) } - let_it_be(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } } + let_it_be_with_refind(:package_protection_rule) { create(:package_protection_rule, project: project) } + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } let(:mutation) { graphql_mutation(:delete_packages_protection_rule, input) } + let(:mutation_response) { graphql_mutation_response(:delete_packages_protection_rule) } let(:input) { { id: package_protection_rule.to_global_id } } - let(:current_user) { project.owner } - subject { post_graphql_mutation(mutation, current_user: current_user) } - context 'with existing package protection rule' do - let!(:package_protection_rule) { create(:package_protection_rule, project: project) } + shared_examples 'an erroneous reponse' do + it { subject.tap { expect(mutation_response).to be_blank } } + it { expect { subject }.not_to change { ::Packages::Protection::Rule.count } } + end - it_behaves_like 'a working GraphQL mutation' + it_behaves_like 'a working GraphQL mutation' - it 'deletes specified package protection rule' do - expect { subject } - .to change { project.package_protection_rules.include?(package_protection_rule) }.from(true).to(false) - .and change { ::Packages::Protection::Rule.count }.from(1).to(0) - end + it 'responds with deleted package protection rule' do + subject - it { is_expected.tap { expect_graphql_errors_to_be_empty } } + expect(mutation_response).to include( + 'packageProtectionRule' => { + 'packageNamePattern' => package_protection_rule.package_name_pattern, + 'packageType' => package_protection_rule.package_type.upcase, + 'pushProtectedUpToAccessLevel' => package_protection_rule.push_protected_up_to_access_level.upcase + }, 'errors' => be_blank + ) end + it { is_expected.tap { expect_graphql_errors_to_be_empty } } + it { expect { subject }.to change { ::Packages::Protection::Rule.count }.from(1).to(0) } + context 'with existing package protection rule belonging to other project' do - # package protection rule of other project - let_it_be(:package_protection_rule) { create(:package_protection_rule) } + let_it_be(:package_protection_rule) do + create(:package_protection_rule, package_name_pattern: 'protection_rule_other_project') + end - it 'does not delete the specified package protection rule' do - expect { subject }.to change { ::Packages::Protection::Rule.count }.by(0) + it_behaves_like 'an erroneous reponse' - expect(::Packages::Protection::Rule.find(package_protection_rule.id)).to be_present - end + it { subject.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } } + end - it 'responds with an error message' do - subject.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } + context 'with deleted package protection rule' do + let!(:package_protection_rule) do + create(:package_protection_rule, project: project, package_name_pattern: 'protection_rule_deleted').destroy! end - end - context 'without existing package protection rule' do - let!(:package_protection_rule) { create(:package_protection_rule, project: project).destroy! } + it_behaves_like 'an erroneous reponse' - it 'does not delete the package protection rule (as it does not exist)' do - expect { subject }.to change { Packages::Protection::Rule.count }.by(0) - end + it { subject.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } } end context 'when current_user does not have permission' do - let_it_be(:package_protection_rule) { create(:package_protection_rule, project: project) } - let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } let_it_be(:guest) { create(:user).tap { |u| project.add_guest(u) } } @@ -65,32 +68,19 @@ end with_them do - it 'returns an error' do - subject.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } - end - - it do - expect { subject }.not_to change { ::Packages::Protection::Rule.count } - expect(::Packages::Protection::Rule.find(package_protection_rule.id)).to be_present - end + it_behaves_like 'an erroneous reponse' + + it { subject.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } } end end context "when feature flag ':packages_protected_packages' disabled" do - let_it_be(:package_protection_rule) { create(:package_protection_rule, project: project) } - before do stub_feature_flags(packages_protected_packages: false) end - it 'responds with an error message' do - subject.tap { expect_graphql_errors_to_include(/'packages_protected_packages' feature flag is disabled/) } - end - - it 'does not delete the specified package protection rule' do - expect { subject }.to change { ::Packages::Protection::Rule.count }.by(0) + it_behaves_like 'an erroneous reponse' - expect(::Packages::Protection::Rule.find(package_protection_rule.id)).to be_present - end + it { subject.tap { expect_graphql_errors_to_include(/'packages_protected_packages' feature flag is disabled/) } } end end diff --git a/spec/services/packages/protection/delete_rule_service_spec.rb b/spec/services/packages/protection/delete_rule_service_spec.rb index 34695b7916f432..07380fa05bba8e 100644 --- a/spec/services/packages/protection/delete_rule_service_spec.rb +++ b/spec/services/packages/protection/delete_rule_service_spec.rb @@ -4,29 +4,67 @@ RSpec.describe Packages::Protection::DeleteRuleService, '#execute', feature_category: :package_registry do let_it_be(:project) { create(:project) } - - let(:current_user) { project.owner } + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + let_it_be_with_refind(:package_protection_rule) { create(:package_protection_rule, project: project) } subject { described_class.new(package_protection_rule, current_user: current_user).execute } - context 'with existing package protection rule' do - let!(:package_protection_rule) { create(:package_protection_rule, project: project) } - + shared_examples 'a successful service response' do it { is_expected.to be_success } - it { is_expected.to have_attributes payload: { package_protection_rule: package_protection_rule } } + + it { + is_expected.to have_attributes( + errors: be_blank, + message: be_blank, + payload: { package_protection_rule: package_protection_rule } + ) + } + + it { subject.tap { expect { package_protection_rule.reload }.to raise_error ActiveRecord::RecordNotFound } } + end + + shared_examples 'an erroneous service response' do + it { is_expected.to be_error } + it { is_expected.to have_attributes(message: be_present, payload: { package_protection_rule: be_blank }) } it do - expect { subject } - .to change { project.reload.package_protection_rules }.from([package_protection_rule]).to([]) - .and change { ::Packages::Protection::Rule.count }.from(1).to(0) + expect { subject }.not_to change { Packages::Protection::Rule.count } + + expect { package_protection_rule.reload }.not_to raise_error end end + it_behaves_like 'a successful service response' + + it 'deletes the package protection rule in the database' do + expect { subject } + .to change { project.reload.package_protection_rules }.from([package_protection_rule]).to([]) + .and change { ::Packages::Protection::Rule.count }.from(1).to(0) + end + context 'with deleted package protection rule' do - let!(:package_protection_rule) { create(:package_protection_rule, project: project).destroy! } + let!(:package_protection_rule) do + create(:package_protection_rule, project: project, package_name_pattern: 'protection_rule_deleted').destroy! + end - it { is_expected.to be_error } - it { is_expected.to have_attributes payload: { package_protection_rule: nil } } + it_behaves_like 'a successful service response' + end + + context 'when current_user does not have permission' do + let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } + let_it_be(:guest) { create(:user).tap { |u| project.add_guest(u) } } + let_it_be(:anonymous) { create(:user) } + + where(:current_user) do + [ref(:developer), ref(:reporter), ref(:guest), ref(:anonymous)] + end + + with_them do + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes message: /Unauthorized to delete a package protection rule/ } + end end context 'without package protection rule' do -- GitLab From d2a805992a1771d0e914f1004cf98edbb4978ade Mon Sep 17 00:00:00 2001 From: Gerardo Date: Tue, 24 Oct 2023 21:15:01 +0200 Subject: [PATCH 3/4] refactor: Apply suggestion from @terrichu This commit includes: - Updating the milestone for this new graphql mutation to v16.6 . --- app/graphql/types/mutation_type.rb | 2 +- doc/api/graphql/reference/index.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 410fa90e514a80..475066e7279138 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -173,7 +173,7 @@ class MutationType < BaseObject extensions: [::Gitlab::Graphql::Limit::FieldCallCount => { limit: 1 }] mount_mutation Mutations::Packages::DestroyFile mount_mutation Mutations::Packages::Protection::Rule::Create, alpha: { milestone: '16.5' } - mount_mutation Mutations::Packages::Protection::Rule::Delete, alpha: { milestone: '16.5' } + mount_mutation Mutations::Packages::Protection::Rule::Delete, alpha: { milestone: '16.6' } mount_mutation Mutations::Packages::DestroyFiles mount_mutation Mutations::Packages::Cleanup::Policy::Update mount_mutation Mutations::Echo diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index b60744e8a0294e..a013d4181067c8 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -3038,7 +3038,7 @@ Input type: `DeleteAnnotationInput` Deletes a protection rule for packages. Available only when feature flag `packages_protected_packages` is enabled. WARNING: -**Introduced** in 16.5. +**Introduced** in 16.6. This feature is an Experiment. It can be changed or removed at any time. Input type: `DeletePackagesProtectionRuleInput` -- GitLab From 348f65f8ae414d2f8b4293314afb2f9049c35375 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Wed, 25 Oct 2023 18:31:39 +0200 Subject: [PATCH 4/4] test: Add test case to increase coverage and fix ci pipeline This commit includes: - Adding an additional test to increase coverage - Fixing the ci pipeline as it failed because one code path was not coverred by the test --- .../packages/protection/delete_rule_service.rb | 7 +++---- .../packages/protection/delete_rule_service_spec.rb | 10 ++++++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/app/services/packages/protection/delete_rule_service.rb b/app/services/packages/protection/delete_rule_service.rb index 28db4446b0be6c..a1fa111b57bee1 100644 --- a/app/services/packages/protection/delete_rule_service.rb +++ b/app/services/packages/protection/delete_rule_service.rb @@ -5,9 +5,6 @@ module Protection class DeleteRuleService include Gitlab::Allowable - # Todo: Make private - attr_reader :package_protection_rule, :current_user - def initialize(package_protection_rule, current_user:) if package_protection_rule.blank? || current_user.blank? raise ArgumentError, @@ -27,12 +24,14 @@ def execute deleted_package_protection_rule = package_protection_rule.destroy! ServiceResponse.success(payload: { package_protection_rule: deleted_package_protection_rule }) - rescue ArgumentError => e + rescue StandardError => e service_response_error(message: e.message) end private + attr_reader :package_protection_rule, :current_user + def service_response_error(message:) ServiceResponse.error( message: message, diff --git a/spec/services/packages/protection/delete_rule_service_spec.rb b/spec/services/packages/protection/delete_rule_service_spec.rb index 07380fa05bba8e..d64609d4df1acd 100644 --- a/spec/services/packages/protection/delete_rule_service_spec.rb +++ b/spec/services/packages/protection/delete_rule_service_spec.rb @@ -50,6 +50,16 @@ it_behaves_like 'a successful service response' end + context 'when error occurs during delete operation' do + before do + allow(package_protection_rule).to receive(:destroy!).and_raise(StandardError.new('Some error')) + end + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes message: /Some error/ } + end + context 'when current_user does not have permission' do let_it_be(:developer) { create(:user).tap { |u| project.add_developer(u) } } let_it_be(:reporter) { create(:user).tap { |u| project.add_reporter(u) } } -- GitLab