From 8b9d4598703475c625c0b65333791acced2f6288 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Wed, 26 Jul 2023 19:57:37 +0200 Subject: [PATCH 1/4] feat: Add graphql mutation to update package protection rules This MR adds a new graphql endpoint for updating package protection rules. Changelog: added --- .../packages/protection/rule/update.rb | 61 ++++++++ app/graphql/types/mutation_type.rb | 1 + .../protection/update_rule_service.rb | 54 +++++++ doc/api/graphql/reference/index.md | 28 ++++ locale/gitlab.pot | 3 + .../packages/protection/rule/update_spec.rb | 141 ++++++++++++++++++ .../protection/update_rule_service_spec.rb | 138 +++++++++++++++++ 7 files changed, 426 insertions(+) create mode 100644 app/graphql/mutations/packages/protection/rule/update.rb create mode 100644 app/services/packages/protection/update_rule_service.rb create mode 100644 spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb create mode 100644 spec/services/packages/protection/update_rule_service_spec.rb diff --git a/app/graphql/mutations/packages/protection/rule/update.rb b/app/graphql/mutations/packages/protection/rule/update.rb new file mode 100644 index 00000000000000..3d2916e60fa159 --- /dev/null +++ b/app/graphql/mutations/packages/protection/rule/update.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module Mutations + module Packages + module Protection + module Rule + class Update < ::Mutations::BaseMutation + graphql_name 'UpdatePackagesProtectionRule' + description 'Updates a protection rule to restrict access to project 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 be updated.' + + argument :package_name_pattern, + GraphQL::Types::String, + required: false, + validates: { allow_blank: false }, + description: + 'Package name protected by the protection rule. For example `@my-scope/my-package-*`. ' \ + 'Wildcard character `*` allowed.' + + argument :package_type, + Types::Packages::Protection::RulePackageTypeEnum, + required: false, + validates: { allow_blank: false }, + description: 'Package type protected by the protection rule. For example `NPM`.' + + argument :push_protected_up_to_access_level, + Types::Packages::Protection::RuleAccessLevelEnum, + required: false, + validates: { allow_blank: false }, + description: + 'Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' + + field :package_protection_rule, + Types::Packages::Protection::RuleType, + null: true, + description: 'Packages protection rule after mutation.' + + def resolve(id:, **kwargs) + package_protection_rule = authorized_find!(id: id) + + if Feature.disabled?(:packages_protected_packages, package_protection_rule.project) + raise_resource_not_available_error!("'packages_protected_packages' feature flag is disabled") + end + + response = ::Packages::Protection::UpdateRuleService.new(package_protection_rule, + current_user: current_user, params: kwargs).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 8055247fb8abfd..d0a9ea11a272d8 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -177,6 +177,7 @@ class MutationType < BaseObject 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.6' } + mount_mutation Mutations::Packages::Protection::Rule::Update, alpha: { milestone: '16.6' } mount_mutation Mutations::Packages::DestroyFiles mount_mutation Mutations::Packages::Cleanup::Policy::Update mount_mutation Mutations::Echo diff --git a/app/services/packages/protection/update_rule_service.rb b/app/services/packages/protection/update_rule_service.rb new file mode 100644 index 00000000000000..0dc7eb6a7b9836 --- /dev/null +++ b/app/services/packages/protection/update_rule_service.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Packages + module Protection + class UpdateRuleService + include Gitlab::Allowable + + ALLOWED_ATTRIBUTES = %i[ + package_name_pattern + package_type + push_protected_up_to_access_level + ].freeze + + def initialize(package_protection_rule, current_user:, params:) + 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 + @params = params || {} + end + + def execute + unless can?(current_user, :admin_package, package_protection_rule.project) + error_message = _('Unauthorized to update a package protection rule') + return service_response_error(message: error_message) + end + + package_protection_rule.update(params.slice(*ALLOWED_ATTRIBUTES)) + + if package_protection_rule.errors.present? + return service_response_error(message: package_protection_rule.errors.full_messages) + end + + ServiceResponse.success(payload: { package_protection_rule: package_protection_rule }) + rescue StandardError => e + service_response_error(message: e.message) + end + + private + + attr_reader :package_protection_rule, :current_user, :params + + 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 c8a5bf4a5cf73f..093c6d1849c58d 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -7494,6 +7494,34 @@ Input type: `UpdatePackagesCleanupPolicyInput` | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `packagesCleanupPolicy` | [`PackagesCleanupPolicy`](#packagescleanuppolicy) | Packages cleanup policy after mutation. | +### `Mutation.updatePackagesProtectionRule` + +Updates a protection rule to restrict access to project packages. Available only when feature flag `packages_protected_packages` is enabled. + +WARNING: +**Introduced** in 16.6. +This feature is an Experiment. It can be changed or removed at any time. + +Input type: `UpdatePackagesProtectionRuleInput` + +#### 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 be updated. | +| `packageNamePattern` | [`String`](#string) | Package name protected by the protection rule. For example `@my-scope/my-package-*`. Wildcard character `*` allowed. | +| `packageType` | [`PackagesProtectionRulePackageType`](#packagesprotectionrulepackagetype) | Package type protected by the protection rule. For example `NPM`. | +| `pushProtectedUpToAccessLevel` | [`PackagesProtectionRuleAccessLevel`](#packagesprotectionruleaccesslevel) | Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | + +#### 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 after mutation. | + ### `Mutation.updateRequirement` Input type: `UpdateRequirementInput` diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 09caceda198212..e7da008a60f7a1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -51281,6 +51281,9 @@ msgstr "" msgid "Unauthorized to delete a package protection rule" msgstr "" +msgid "Unauthorized to update a package protection rule" +msgstr "" + msgid "Unauthorized to update the environment" msgstr "" diff --git a/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb b/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb new file mode 100644 index 00000000000000..f553b3c1aa8611 --- /dev/null +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb @@ -0,0 +1,141 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Updating the packages protection rule', :aggregate_failures, feature_category: :package_registry do + include GraphqlHelpers + + let_it_be(:project) { create(:project) } + let_it_be_with_reload(:package_protection_rule) do + create(:package_protection_rule, project: project, push_protected_up_to_access_level: :developer) + end + + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + + let(:package_protection_rule_attributes) { build_stubbed(:package_protection_rule, project: project) } + + let(:mutation) do + graphql_mutation(:update_packages_protection_rule, input, + <<~QUERY + packageProtectionRule { + packageNamePattern + pushProtectedUpToAccessLevel + } + clientMutationId + errors + QUERY + ) + end + + let(:input) do + { + id: package_protection_rule.to_global_id, + package_name_pattern: "#{package_protection_rule.package_name_pattern}-updated", + push_protected_up_to_access_level: 'MAINTAINER' + } + end + + let(:mutation_response) { graphql_mutation_response(:update_packages_protection_rule) } + + subject { post_graphql_mutation(mutation, current_user: current_user) } + + shared_examples 'a successful response' do + it { subject.tap { expect_graphql_errors_to_be_empty } } + + it 'returns the updated package protection rule' do + subject + + expect(mutation_response).to include( + 'packageProtectionRule' => { + 'packageNamePattern' => input[:package_name_pattern], + 'pushProtectedUpToAccessLevel' => input[:push_protected_up_to_access_level] + } + ) + end + + it { + subject.tap do + expect(package_protection_rule.reload).to have_attributes( + package_name_pattern: input[:package_name_pattern], + push_protected_up_to_access_level: input[:push_protected_up_to_access_level].downcase + ) + end + } + end + + shared_examples 'an erroneous reponse' do + it { subject.tap { expect(mutation_response).to be_blank } } + it { expect { subject }.not_to change { package_protection_rule.reload.updated_at } } + end + + it_behaves_like 'a successful response' + + context 'with other existing package protection rule with same package_name_pattern' do + let_it_be_with_reload(:other_existing_package_protection_rule) do + create(:package_protection_rule, project: project, + package_name_pattern: "#{package_protection_rule.package_name_pattern}-other") + end + + let(:input) { super().merge(package_name_pattern: other_existing_package_protection_rule.package_name_pattern) } + + it { subject.tap { expect_graphql_errors_to_be_empty } } + + it 'returns the updated package protection rule' do + subject + + expect(mutation_response).to include( + 'errors' => be_present, + 'packageProtectionRule' => be_blank + ) + end + + it 'includes error message in response' do + subject + + expect(mutation_response['errors']).to eq ['Package name pattern has already been taken'] + end + end + + context 'with invalid input param `pushProtectedUpToAccessLevel`' do + let(:input) { super().merge(push_protected_up_to_access_level: nil) } + + it_behaves_like 'an erroneous reponse' + + it { subject.tap { expect_graphql_errors_to_include(/pushProtectedUpToAccessLevel can't be blank/) } } + end + + context 'with invalid input param `packageNamePattern`' do + let(:input) { super().merge(package_name_pattern: '') } + + it_behaves_like 'an erroneous reponse' + + it { subject.tap { expect_graphql_errors_to_include(/packageNamePattern can't be blank/) } } + 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 { 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 + before do + stub_feature_flags(packages_protected_packages: false) + end + + it_behaves_like 'an erroneous reponse' + + it 'returns error of disabled feature flag' do + subject.tap { expect_graphql_errors_to_include(/'packages_protected_packages' feature flag is disabled/) } + end + end +end diff --git a/spec/services/packages/protection/update_rule_service_spec.rb b/spec/services/packages/protection/update_rule_service_spec.rb new file mode 100644 index 00000000000000..dbb64d2ce30151 --- /dev/null +++ b/spec/services/packages/protection/update_rule_service_spec.rb @@ -0,0 +1,138 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Protection::UpdateRuleService, '#execute', feature_category: :environment_management do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:current_user) { create(:user, maintainer_projects: [project]) } + let_it_be_with_reload(:package_protection_rule) { create(:package_protection_rule, project: project) } + + let(:service) { described_class.new(package_protection_rule, current_user: current_user, params: params) } + + let(:params) do + attributes_for( + :package_protection_rule, + package_name_pattern: "#{package_protection_rule.package_name_pattern}-updated", + package_type: 'npm', + push_protected_up_to_access_level: 'owner' + ) + end + + subject { service.execute } + + shared_examples 'a successful service response' do + let(:expected_attributes) { params } + it { is_expected.to be_success } + + it { + is_expected.to have_attributes( + errors: be_blank, + message: be_blank, + payload: { package_protection_rule: be_a(Packages::Protection::Rule).and(have_attributes(expected_attributes)) } + ) + } + + it { expect { subject }.not_to change { Packages::Protection::Rule.count } } + + it { subject.tap { expect(package_protection_rule.reload).to have_attributes expected_attributes } } + end + + shared_examples 'an erroneous service response' do + it { is_expected.to be_error } + + it { + is_expected.to have_attributes( + errors: be_present, + message: be_present, + payload: { package_protection_rule: nil } + ) + } + + it { expect { subject }.not_to change { Packages::Protection::Rule.count } } + it { expect { subject }.not_to change { package_protection_rule.reload.updated_at } } + end + + it_behaves_like 'a successful service response' + + context 'with disallowed params' do + let(:params) { super().merge!(project_id: 1, unsupported_param: 'unsupported_param_value') } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { params.except(:project_id, :unsupported_param) } + end + end + + context 'with empty params' do + let(:params) { {} } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { package_protection_rule.attributes } + end + + it { expect { subject }.not_to change { package_protection_rule.reload.updated_at } } + end + + context 'with nil params' do + let(:params) { nil } + + it_behaves_like 'a successful service response' do + let(:expected_attributes) { package_protection_rule.attributes } + end + + it { expect { subject }.not_to change { package_protection_rule.reload.updated_at } } + end + + context 'when updated field `package_name_pattern` is already taken' do + let_it_be_with_reload(:other_existing_package_protection_rule) do + create(:package_protection_rule, project: project, + package_name_pattern: "#{package_protection_rule.package_name_pattern}-other") + end + + let(:params) { { package_name_pattern: other_existing_package_protection_rule.package_name_pattern } } + + it_behaves_like 'an erroneous service response' + + it { + expect { subject }.not_to( + change { other_existing_package_protection_rule.reload.package_name_pattern } + ) + } + + it { + is_expected.to have_attributes( + errors: match_array([/Package name pattern has already been taken/]), + message: match_array([/Package name pattern has already been taken/]) + ) + } + 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 errors: match_array(/Unauthorized/), message: /Unauthorized/ } + end + end + + context 'without package protection rule' do + let(:package_protection_rule) { nil } + let(:params) { {} } + + it { expect { subject }.to raise_error(ArgumentError) } + end + + context 'without current_user' do + let(:current_user) { nil } + + it { expect { subject }.to raise_error(ArgumentError) } + end +end -- GitLab From fbb3adbf635f6beddc3a63f785c35b25df145368 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Tue, 24 Oct 2023 21:07:23 +0200 Subject: [PATCH 2/4] refactor: Apply suggestion from @phillipwells This commit includes: - Changing documentation for the new graphql mutation --- app/graphql/mutations/packages/protection/rule/update.rb | 9 +++++---- doc/api/graphql/reference/index.md | 8 ++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/app/graphql/mutations/packages/protection/rule/update.rb b/app/graphql/mutations/packages/protection/rule/update.rb index 3d2916e60fa159..dc1f78e6822f9a 100644 --- a/app/graphql/mutations/packages/protection/rule/update.rb +++ b/app/graphql/mutations/packages/protection/rule/update.rb @@ -6,7 +6,8 @@ module Protection module Rule class Update < ::Mutations::BaseMutation graphql_name 'UpdatePackagesProtectionRule' - description 'Updates a protection rule to restrict access to project packages. ' \ + description 'Updates a package protection rule to restrict access to project packages. ' \ + 'You can prevent users without certain permissions from altering packages. ' \ 'Available only when feature flag `packages_protected_packages` is enabled.' authorize :admin_package @@ -21,21 +22,21 @@ class Update < ::Mutations::BaseMutation required: false, validates: { allow_blank: false }, description: - 'Package name protected by the protection rule. For example `@my-scope/my-package-*`. ' \ + 'Package name protected by the protection rule. For example, `@my-scope/my-package-*`. ' \ 'Wildcard character `*` allowed.' argument :package_type, Types::Packages::Protection::RulePackageTypeEnum, required: false, validates: { allow_blank: false }, - description: 'Package type protected by the protection rule. For example `NPM`.' + description: 'Package type protected by the protection rule. For example, `NPM`.' argument :push_protected_up_to_access_level, Types::Packages::Protection::RuleAccessLevelEnum, required: false, validates: { allow_blank: false }, description: - 'Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`.' + 'Maximum GitLab access level unable to push a package. For example, `DEVELOPER`, `MAINTAINER`, `OWNER`.' field :package_protection_rule, Types::Packages::Protection::RuleType, diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 093c6d1849c58d..fca6b472f30d4f 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -7496,7 +7496,7 @@ Input type: `UpdatePackagesCleanupPolicyInput` ### `Mutation.updatePackagesProtectionRule` -Updates a protection rule to restrict access to project packages. Available only when feature flag `packages_protected_packages` is enabled. +Updates a package protection rule to restrict access to project packages. You can prevent users without certain permissions from altering packages. Available only when feature flag `packages_protected_packages` is enabled. WARNING: **Introduced** in 16.6. @@ -7510,9 +7510,9 @@ Input type: `UpdatePackagesProtectionRuleInput` | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `id` | [`PackagesProtectionRuleID!`](#packagesprotectionruleid) | Global ID of the package protection rule to be updated. | -| `packageNamePattern` | [`String`](#string) | Package name protected by the protection rule. For example `@my-scope/my-package-*`. Wildcard character `*` allowed. | -| `packageType` | [`PackagesProtectionRulePackageType`](#packagesprotectionrulepackagetype) | Package type protected by the protection rule. For example `NPM`. | -| `pushProtectedUpToAccessLevel` | [`PackagesProtectionRuleAccessLevel`](#packagesprotectionruleaccesslevel) | Max GitLab access level unable to push a package. For example `DEVELOPER`, `MAINTAINER`, `OWNER`. | +| `packageNamePattern` | [`String`](#string) | Package name protected by the protection rule. For example, `@my-scope/my-package-*`. Wildcard character `*` allowed. | +| `packageType` | [`PackagesProtectionRulePackageType`](#packagesprotectionrulepackagetype) | Package type protected by the protection rule. For example, `NPM`. | +| `pushProtectedUpToAccessLevel` | [`PackagesProtectionRuleAccessLevel`](#packagesprotectionruleaccesslevel) | Maximum GitLab access level unable to push a package. For example, `DEVELOPER`, `MAINTAINER`, `OWNER`. | #### Fields -- GitLab From 24cdccc496ceda098549db31f57ef6ea08662e3b Mon Sep 17 00:00:00 2001 From: Gerardo Date: Thu, 26 Oct 2023 09:07:08 +0200 Subject: [PATCH 3/4] test: Add test case to increase coverage and fix ci pipeline This commit includes: - Rebase with branch `master` - 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/update_rule_service_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/services/packages/protection/update_rule_service_spec.rb b/spec/services/packages/protection/update_rule_service_spec.rb index dbb64d2ce30151..88e17f38a6b45b 100644 --- a/spec/services/packages/protection/update_rule_service_spec.rb +++ b/spec/services/packages/protection/update_rule_service_spec.rb @@ -62,6 +62,17 @@ end end + context 'when fields are invalid' do + let(:params) do + { package_name_pattern: '', package_type: 'unknown_package_type', + push_protected_up_to_access_level: 1000 } + end + + it_behaves_like 'an erroneous service response' + + it { is_expected.to have_attributes message: /'unknown_package_type' is not a valid package_type/ } + end + context 'with empty params' do let(:params) { {} } -- GitLab From 08d945e3584d17a9d17d54f047ba871aa9e815b8 Mon Sep 17 00:00:00 2001 From: Gerardo Date: Wed, 15 Nov 2023 11:22:53 +0000 Subject: [PATCH 4/4] refactor: Fix rubocop failures in ci pipeline --- .../packages/protection/rule/update_spec.rb | 27 +++++++---------- .../protection/update_rule_service_spec.rb | 29 ++++++++++--------- 2 files changed, 25 insertions(+), 31 deletions(-) diff --git a/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb b/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb index f553b3c1aa8611..cb4f9240311680 100644 --- a/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb +++ b/spec/requests/api/graphql/mutations/packages/protection/rule/update_spec.rb @@ -53,14 +53,14 @@ ) end - it { + it do subject.tap do expect(package_protection_rule.reload).to have_attributes( package_name_pattern: input[:package_name_pattern], push_protected_up_to_access_level: input[:push_protected_up_to_access_level].downcase ) end - } + end end shared_examples 'an erroneous reponse' do @@ -78,21 +78,14 @@ let(:input) { super().merge(package_name_pattern: other_existing_package_protection_rule.package_name_pattern) } - it { subject.tap { expect_graphql_errors_to_be_empty } } - - it 'returns the updated package protection rule' do - subject + it { is_expected.tap { expect_graphql_errors_to_be_empty } } - expect(mutation_response).to include( - 'errors' => be_present, - 'packageProtectionRule' => be_blank - ) + it 'returns a blank package protection rule' do + is_expected.tap { expect(mutation_response['packageProtectionRule']).to be_blank } end it 'includes error message in response' do - subject - - expect(mutation_response['errors']).to eq ['Package name pattern has already been taken'] + is_expected.tap { expect(mutation_response['errors']).to eq ['Package name pattern has already been taken'] } end end @@ -101,7 +94,7 @@ it_behaves_like 'an erroneous reponse' - it { subject.tap { expect_graphql_errors_to_include(/pushProtectedUpToAccessLevel can't be blank/) } } + it { is_expected.tap { expect_graphql_errors_to_include(/pushProtectedUpToAccessLevel can't be blank/) } } end context 'with invalid input param `packageNamePattern`' do @@ -109,7 +102,7 @@ it_behaves_like 'an erroneous reponse' - it { subject.tap { expect_graphql_errors_to_include(/packageNamePattern can't be blank/) } } + it { is_expected.tap { expect_graphql_errors_to_include(/packageNamePattern can't be blank/) } } end context 'when current_user does not have permission' do @@ -123,7 +116,7 @@ end with_them do - it { subject.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } } + it { is_expected.tap { expect_graphql_errors_to_include(/you don't have permission to perform this action/) } } end end @@ -135,7 +128,7 @@ it_behaves_like 'an erroneous reponse' it 'returns error of disabled feature flag' do - subject.tap { expect_graphql_errors_to_include(/'packages_protected_packages' feature flag is disabled/) } + is_expected.tap { expect_graphql_errors_to_include(/'packages_protected_packages' feature flag is disabled/) } end end end diff --git a/spec/services/packages/protection/update_rule_service_spec.rb b/spec/services/packages/protection/update_rule_service_spec.rb index 88e17f38a6b45b..70619a1caa3ddc 100644 --- a/spec/services/packages/protection/update_rule_service_spec.rb +++ b/spec/services/packages/protection/update_rule_service_spec.rb @@ -18,19 +18,20 @@ ) end - subject { service.execute } + subject(:service_execute) { service.execute } shared_examples 'a successful service response' do let(:expected_attributes) { params } + it { is_expected.to be_success } - it { + it do is_expected.to have_attributes( errors: be_blank, message: be_blank, payload: { package_protection_rule: be_a(Packages::Protection::Rule).and(have_attributes(expected_attributes)) } ) - } + end it { expect { subject }.not_to change { Packages::Protection::Rule.count } } @@ -40,13 +41,13 @@ shared_examples 'an erroneous service response' do it { is_expected.to be_error } - it { + it do is_expected.to have_attributes( errors: be_present, message: be_present, payload: { package_protection_rule: nil } ) - } + end it { expect { subject }.not_to change { Packages::Protection::Rule.count } } it { expect { subject }.not_to change { package_protection_rule.reload.updated_at } } @@ -80,7 +81,7 @@ let(:expected_attributes) { package_protection_rule.attributes } end - it { expect { subject }.not_to change { package_protection_rule.reload.updated_at } } + it { expect { service_execute }.not_to change { package_protection_rule.reload.updated_at } } end context 'with nil params' do @@ -90,7 +91,7 @@ let(:expected_attributes) { package_protection_rule.attributes } end - it { expect { subject }.not_to change { package_protection_rule.reload.updated_at } } + it { expect { service_execute }.not_to change { package_protection_rule.reload.updated_at } } end context 'when updated field `package_name_pattern` is already taken' do @@ -103,18 +104,18 @@ it_behaves_like 'an erroneous service response' - it { - expect { subject }.not_to( + it do + expect { service_execute }.not_to( change { other_existing_package_protection_rule.reload.package_name_pattern } ) - } + end - it { + it do is_expected.to have_attributes( errors: match_array([/Package name pattern has already been taken/]), message: match_array([/Package name pattern has already been taken/]) ) - } + end end context 'when current_user does not have permission' do @@ -138,12 +139,12 @@ let(:package_protection_rule) { nil } let(:params) { {} } - it { expect { subject }.to raise_error(ArgumentError) } + it { expect { service_execute }.to raise_error(ArgumentError) } end context 'without current_user' do let(:current_user) { nil } - it { expect { subject }.to raise_error(ArgumentError) } + it { expect { service_execute }.to raise_error(ArgumentError) } end end -- GitLab