diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-09-19 15:09:58 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-09-19 15:09:58 +0000 |
commit | 6cf9209f12a250a371a6fdbd0812dda847ba1284 (patch) | |
tree | a1ba20014bc0cbbf31ec494ae369ea7eed73865a | |
parent | e3ae3532a1b225938535028bbdb7bfb8fc9b2f48 (diff) | |
download | gitlab-ce-6cf9209f12a250a371a6fdbd0812dda847ba1284.tar.gz |
Add latest changes from gitlab-org/gitlab@master
21 files changed, 1050 insertions, 9 deletions
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 228fb7bd6ea..dd3e07c9e80 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1144,9 +1144,6 @@ Rails/SaveBang: - 'spec/services/issuable/clone/attributes_rewriter_spec.rb' - 'spec/services/issuable/common_system_notes_service_spec.rb' - 'spec/services/labels/promote_service_spec.rb' - - 'spec/services/milestones/destroy_service_spec.rb' - - 'spec/services/milestones/promote_service_spec.rb' - - 'spec/services/milestones/transfer_service_spec.rb' - 'spec/services/notes/create_service_spec.rb' - 'spec/services/notification_recipients/build_service_spec.rb' - 'spec/services/notification_service_spec.rb' diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index 5d827675bff..0d2f89fb18d 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -37,6 +37,8 @@ class ProjectStatistics < ApplicationRecord end def refresh!(only: []) + return if Gitlab::Database.read_only? + COLUMNS_TO_REFRESH.each do |column, generator| if only.empty? || only.include?(column) public_send("update_#{column}") # rubocop:disable GitlabSecurity/PublicSend diff --git a/app/models/snippet_statistics.rb b/app/models/snippet_statistics.rb index 8545296d076..6fb6f0ef713 100644 --- a/app/models/snippet_statistics.rb +++ b/app/models/snippet_statistics.rb @@ -34,6 +34,8 @@ class SnippetStatistics < ApplicationRecord end def refresh! + return if Gitlab::Database.read_only? + update_commit_count update_repository_size update_file_count diff --git a/app/services/feature_flags/base_service.rb b/app/services/feature_flags/base_service.rb new file mode 100644 index 00000000000..9b27df90992 --- /dev/null +++ b/app/services/feature_flags/base_service.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module FeatureFlags + class BaseService < ::BaseService + include Gitlab::Utils::StrongMemoize + + AUDITABLE_ATTRIBUTES = %w(name description active).freeze + + protected + + def audit_event(feature_flag) + message = audit_message(feature_flag) + + return if message.blank? + + details = + { + custom_message: message, + target_id: feature_flag.id, + target_type: feature_flag.class.name, + target_details: feature_flag.name + } + + ::AuditEventService.new( + current_user, + feature_flag.project, + details + ) + end + + def save_audit_event(audit_event) + return unless audit_event + + audit_event.security_event + end + + def created_scope_message(scope) + "Created rule <strong>#{scope.environment_scope}</strong> "\ + "and set it as <strong>#{scope.active ? "active" : "inactive"}</strong> "\ + "with strategies <strong>#{scope.strategies}</strong>." + end + + def feature_flag_by_name + strong_memoize(:feature_flag_by_name) do + project.operations_feature_flags.find_by_name(params[:name]) + end + end + + def feature_flag_scope_by_environment_scope + strong_memoize(:feature_flag_scope_by_environment_scope) do + feature_flag_by_name.scopes.find_by_environment_scope(params[:environment_scope]) + end + end + end +end diff --git a/app/services/feature_flags/create_service.rb b/app/services/feature_flags/create_service.rb new file mode 100644 index 00000000000..b4ca90f7aae --- /dev/null +++ b/app/services/feature_flags/create_service.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module FeatureFlags + class CreateService < FeatureFlags::BaseService + def execute + return error('Access Denied', 403) unless can_create? + return error('Version is invalid', :bad_request) unless valid_version? + return error('New version feature flags are not enabled for this project', :bad_request) unless flag_version_enabled? + + ActiveRecord::Base.transaction do + feature_flag = project.operations_feature_flags.new(params) + + if feature_flag.save + save_audit_event(audit_event(feature_flag)) + + success(feature_flag: feature_flag) + else + error(feature_flag.errors.full_messages, 400) + end + end + end + + private + + def audit_message(feature_flag) + message_parts = ["Created feature flag <strong>#{feature_flag.name}</strong>", + "with description <strong>\"#{feature_flag.description}\"</strong>."] + + message_parts += feature_flag.scopes.map do |scope| + created_scope_message(scope) + end + + message_parts.join(" ") + end + + def can_create? + Ability.allowed?(current_user, :create_feature_flag, project) + end + + def valid_version? + !params.key?(:version) || Operations::FeatureFlag.versions.key?(params[:version]) + end + + def flag_version_enabled? + params[:version] != 'new_version_flag' || new_version_feature_flags_enabled? + end + + def new_version_feature_flags_enabled? + ::Feature.enabled?(:feature_flags_new_version, project, default_enabled: true) + end + end +end diff --git a/app/services/feature_flags/destroy_service.rb b/app/services/feature_flags/destroy_service.rb new file mode 100644 index 00000000000..c77e3e03ec3 --- /dev/null +++ b/app/services/feature_flags/destroy_service.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module FeatureFlags + class DestroyService < FeatureFlags::BaseService + def execute(feature_flag) + destroy_feature_flag(feature_flag) + end + + private + + def destroy_feature_flag(feature_flag) + return error('Access Denied', 403) unless can_destroy?(feature_flag) + + ActiveRecord::Base.transaction do + if feature_flag.destroy + save_audit_event(audit_event(feature_flag)) + + success(feature_flag: feature_flag) + else + error(feature_flag.errors.full_messages) + end + end + end + + def audit_message(feature_flag) + "Deleted feature flag <strong>#{feature_flag.name}</strong>." + end + + def can_destroy?(feature_flag) + Ability.allowed?(current_user, :destroy_feature_flag, feature_flag) + end + end +end diff --git a/app/services/feature_flags/disable_service.rb b/app/services/feature_flags/disable_service.rb new file mode 100644 index 00000000000..8a443ac1795 --- /dev/null +++ b/app/services/feature_flags/disable_service.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module FeatureFlags + class DisableService < BaseService + def execute + return error('Feature Flag not found', 404) unless feature_flag_by_name + return error('Feature Flag Scope not found', 404) unless feature_flag_scope_by_environment_scope + return error('Strategy not found', 404) unless strategy_exist_in_persisted_data? + + ::FeatureFlags::UpdateService + .new(project, current_user, update_params) + .execute(feature_flag_by_name) + end + + private + + def update_params + if remaining_strategies.empty? + params_to_destroy_scope + else + params_to_update_scope + end + end + + def remaining_strategies + strong_memoize(:remaining_strategies) do + feature_flag_scope_by_environment_scope.strategies.reject do |strategy| + strategy['name'] == params[:strategy]['name'] && + strategy['parameters'] == params[:strategy]['parameters'] + end + end + end + + def strategy_exist_in_persisted_data? + feature_flag_scope_by_environment_scope.strategies != remaining_strategies + end + + def params_to_destroy_scope + { scopes_attributes: [{ id: feature_flag_scope_by_environment_scope.id, _destroy: true }] } + end + + def params_to_update_scope + { scopes_attributes: [{ id: feature_flag_scope_by_environment_scope.id, strategies: remaining_strategies }] } + end + end +end diff --git a/app/services/feature_flags/enable_service.rb b/app/services/feature_flags/enable_service.rb new file mode 100644 index 00000000000..b4cbb32e003 --- /dev/null +++ b/app/services/feature_flags/enable_service.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +module FeatureFlags + class EnableService < BaseService + def execute + if feature_flag_by_name + update_feature_flag + else + create_feature_flag + end + end + + private + + def create_feature_flag + ::FeatureFlags::CreateService + .new(project, current_user, create_params) + .execute + end + + def update_feature_flag + ::FeatureFlags::UpdateService + .new(project, current_user, update_params) + .execute(feature_flag_by_name) + end + + def create_params + if params[:environment_scope] == '*' + params_to_create_flag_with_default_scope + else + params_to_create_flag_with_additional_scope + end + end + + def update_params + if feature_flag_scope_by_environment_scope + params_to_update_scope + else + params_to_create_scope + end + end + + def params_to_create_flag_with_default_scope + { + name: params[:name], + scopes_attributes: [ + { + active: true, + environment_scope: '*', + strategies: [params[:strategy]] + } + ] + } + end + + def params_to_create_flag_with_additional_scope + { + name: params[:name], + scopes_attributes: [ + { + active: false, + environment_scope: '*' + }, + { + active: true, + environment_scope: params[:environment_scope], + strategies: [params[:strategy]] + } + ] + } + end + + def params_to_create_scope + { + scopes_attributes: [{ + active: true, + environment_scope: params[:environment_scope], + strategies: [params[:strategy]] + }] + } + end + + def params_to_update_scope + { + scopes_attributes: [{ + id: feature_flag_scope_by_environment_scope.id, + active: true, + strategies: feature_flag_scope_by_environment_scope.strategies | [params[:strategy]] + }] + } + end + end +end diff --git a/app/services/feature_flags/update_service.rb b/app/services/feature_flags/update_service.rb new file mode 100644 index 00000000000..c837e50b104 --- /dev/null +++ b/app/services/feature_flags/update_service.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +module FeatureFlags + class UpdateService < FeatureFlags::BaseService + AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES = { + 'active' => 'active state', + 'environment_scope' => 'environment scope', + 'strategies' => 'strategies' + }.freeze + + def execute(feature_flag) + return error('Access Denied', 403) unless can_update?(feature_flag) + + ActiveRecord::Base.transaction do + feature_flag.assign_attributes(params) + + feature_flag.strategies.each do |strategy| + if strategy.name_changed? && strategy.name_was == ::Operations::FeatureFlags::Strategy::STRATEGY_GITLABUSERLIST + strategy.user_list = nil + end + end + + audit_event = audit_event(feature_flag) + + if feature_flag.save + save_audit_event(audit_event) + + success(feature_flag: feature_flag) + else + error(feature_flag.errors.full_messages, :bad_request) + end + end + end + + private + + def audit_message(feature_flag) + changes = changed_attributes_messages(feature_flag) + changes += changed_scopes_messages(feature_flag) + + return if changes.empty? + + "Updated feature flag <strong>#{feature_flag.name}</strong>. " + changes.join(" ") + end + + def changed_attributes_messages(feature_flag) + feature_flag.changes.slice(*AUDITABLE_ATTRIBUTES).map do |attribute_name, changes| + "Updated #{attribute_name} "\ + "from <strong>\"#{changes.first}\"</strong> to "\ + "<strong>\"#{changes.second}\"</strong>." + end + end + + def changed_scopes_messages(feature_flag) + feature_flag.scopes.map do |scope| + if scope.new_record? + created_scope_message(scope) + elsif scope.marked_for_destruction? + deleted_scope_message(scope) + else + updated_scope_message(scope) + end + end.compact # updated_scope_message can return nil if nothing has been changed + end + + def deleted_scope_message(scope) + "Deleted rule <strong>#{scope.environment_scope}</strong>." + end + + def updated_scope_message(scope) + changes = scope.changes.slice(*AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES.keys) + return if changes.empty? + + message = "Updated rule <strong>#{scope.environment_scope}</strong> " + message += changes.map do |attribute_name, change| + name = AUDITABLE_SCOPE_ATTRIBUTES_HUMAN_NAMES[attribute_name] + "#{name} from <strong>#{change.first}</strong> to <strong>#{change.second}</strong>" + end.join(' ') + + message + '.' + end + + def can_update?(feature_flag) + Ability.allowed?(current_user, :update_feature_flag, feature_flag) + end + end +end diff --git a/changelogs/unreleased/250041-does-not-refresh-project-snippet-statistics-on-a-read-only-instanc.yml b/changelogs/unreleased/250041-does-not-refresh-project-snippet-statistics-on-a-read-only-instanc.yml new file mode 100644 index 00000000000..f85da860bcd --- /dev/null +++ b/changelogs/unreleased/250041-does-not-refresh-project-snippet-statistics-on-a-read-only-instanc.yml @@ -0,0 +1,5 @@ +--- +title: Does not refresh project/snippet statistics on a read-only instance +merge_request: 42417 +author: +type: fixed diff --git a/changelogs/unreleased/rails-save-bang-33.yml b/changelogs/unreleased/rails-save-bang-33.yml new file mode 100644 index 00000000000..ed001e647ba --- /dev/null +++ b/changelogs/unreleased/rails-save-bang-33.yml @@ -0,0 +1,5 @@ +--- +title: Fix Rails/SaveBang offenses for spec files in spec/services/milestones/* +merge_request: 42775 +author: Rajendra Kadam +type: other diff --git a/spec/models/project_statistics_spec.rb b/spec/models/project_statistics_spec.rb index 47e57972cc1..9f40dbb3401 100644 --- a/spec/models/project_statistics_spec.rb +++ b/spec/models/project_statistics_spec.rb @@ -201,6 +201,23 @@ RSpec.describe ProjectStatistics do statistics.refresh!(only: [:commit_count]) end end + + context 'when the database is read-only' do + it 'does nothing' do + allow(Gitlab::Database).to receive(:read_only?) { true } + + expect(statistics).not_to receive(:update_commit_count) + expect(statistics).not_to receive(:update_repository_size) + expect(statistics).not_to receive(:update_wiki_size) + expect(statistics).not_to receive(:update_lfs_objects_size) + expect(statistics).not_to receive(:update_snippets_size) + expect(statistics).not_to receive(:save!) + expect(Namespaces::ScheduleAggregationWorker) + .not_to receive(:perform_async) + + statistics.refresh! + end + end end describe '#update_commit_count' do diff --git a/spec/models/snippet_statistics_spec.rb b/spec/models/snippet_statistics_spec.rb index 8def6a0bbd4..1fb4ed47169 100644 --- a/spec/models/snippet_statistics_spec.rb +++ b/spec/models/snippet_statistics_spec.rb @@ -75,15 +75,28 @@ RSpec.describe SnippetStatistics do end describe '#refresh!' do - subject { statistics.refresh! } - it 'retrieves and saves statistic data from repository' do expect(statistics).to receive(:update_commit_count) expect(statistics).to receive(:update_file_count) expect(statistics).to receive(:update_repository_size) expect(statistics).to receive(:save!) - subject + statistics.refresh! + end + + context 'when the database is read-only' do + it 'does nothing' do + allow(Gitlab::Database).to receive(:read_only?) { true } + + expect(statistics).not_to receive(:update_commit_count) + expect(statistics).not_to receive(:update_file_count) + expect(statistics).not_to receive(:update_repository_size) + expect(statistics).not_to receive(:save!) + expect(Namespaces::ScheduleAggregationWorker) + .not_to receive(:perform_async) + + statistics.refresh! + end end end diff --git a/spec/services/feature_flags/create_service_spec.rb b/spec/services/feature_flags/create_service_spec.rb new file mode 100644 index 00000000000..e80a24f9760 --- /dev/null +++ b/spec/services/feature_flags/create_service_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FeatureFlags::CreateService do + let(:project) { create(:project) } + let(:developer) { create(:user) } + let(:reporter) { create(:user) } + let(:user) { developer } + + before do + project.add_developer(developer) + project.add_reporter(reporter) + end + + describe '#execute' do + subject do + described_class.new(project, user, params).execute + end + + let(:feature_flag) { subject[:feature_flag] } + + context 'when feature flag can not be created' do + let(:params) { {} } + + it 'returns status error' do + expect(subject[:status]).to eq(:error) + end + + it 'returns validation errors' do + expect(subject[:message]).to include("Name can't be blank") + end + + it 'does not create audit log' do + expect { subject }.not_to change { AuditEvent.count } + end + end + + context 'when feature flag is saved correctly' do + let(:params) do + { + name: 'feature_flag', + description: 'description', + scopes_attributes: [{ environment_scope: '*', active: true }, + { environment_scope: 'production', active: false }] + } + end + + it 'returns status success' do + expect(subject[:status]).to eq(:success) + end + + it 'creates feature flag' do + expect { subject }.to change { Operations::FeatureFlag.count }.by(1) + end + + it 'creates audit event' do + expected_message = 'Created feature flag <strong>feature_flag</strong> '\ + 'with description <strong>"description"</strong>. '\ + 'Created rule <strong>*</strong> and set it as <strong>active</strong> '\ + 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>. '\ + 'Created rule <strong>production</strong> and set it as <strong>inactive</strong> '\ + 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>.' + + expect { subject }.to change { AuditEvent.count }.by(1) + expect(AuditEvent.last.details[:custom_message]).to eq(expected_message) + end + + context 'when user is reporter' do + let(:user) { reporter } + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Access Denied') + end + end + end + end +end diff --git a/spec/services/feature_flags/destroy_service_spec.rb b/spec/services/feature_flags/destroy_service_spec.rb new file mode 100644 index 00000000000..df83969e167 --- /dev/null +++ b/spec/services/feature_flags/destroy_service_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FeatureFlags::DestroyService do + include FeatureFlagHelpers + + let(:project) { create(:project) } + let(:developer) { create(:user) } + let(:reporter) { create(:user) } + let(:user) { developer } + let!(:feature_flag) { create(:operations_feature_flag, project: project) } + + before do + project.add_developer(developer) + project.add_reporter(reporter) + end + + describe '#execute' do + subject { described_class.new(project, user, params).execute(feature_flag) } + + let(:audit_event_message) { AuditEvent.last.details[:custom_message] } + let(:params) { {} } + + it 'returns status success' do + expect(subject[:status]).to eq(:success) + end + + it 'destroys feature flag' do + expect { subject }.to change { Operations::FeatureFlag.count }.by(-1) + end + + it 'creates audit log' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name}</strong>.") + end + + context 'when user is reporter' do + let(:user) { reporter } + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Access Denied') + end + end + + context 'when feature flag can not be destroyed' do + before do + allow(feature_flag).to receive(:destroy).and_return(false) + end + + it 'returns status error' do + expect(subject[:status]).to eq(:error) + end + + it 'does not create audit log' do + expect { subject }.not_to change { AuditEvent.count } + end + end + end +end diff --git a/spec/services/feature_flags/disable_service_spec.rb b/spec/services/feature_flags/disable_service_spec.rb new file mode 100644 index 00000000000..00369d63ccf --- /dev/null +++ b/spec/services/feature_flags/disable_service_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FeatureFlags::DisableService do + include FeatureFlagHelpers + + let_it_be(:user) { create(:user) } + let(:project) { create(:project) } + let(:service) { described_class.new(project, user, params) } + let(:params) { {} } + + before do + project.add_developer(user) + end + + describe '#execute' do + subject { service.execute } + + context 'with params to disable default strategy on prd scope' do + let(:params) do + { + name: 'awesome', + environment_scope: 'prd', + strategy: { name: 'userWithId', parameters: { 'userIds': 'User:1' } }.deep_stringify_keys + } + end + + context 'when there is a persisted feature flag' do + let!(:feature_flag) { create_flag(project, params[:name]) } + + context 'when there is a persisted scope' do + let!(:scope) do + create_scope(feature_flag, params[:environment_scope], true, strategies) + end + + context 'when there is a persisted strategy' do + let(:strategies) do + [ + { name: 'userWithId', parameters: { 'userIds': 'User:1' } }.deep_stringify_keys, + { name: 'userWithId', parameters: { 'userIds': 'User:2' } }.deep_stringify_keys + ] + end + + it 'deletes the specified strategy' do + subject + + scope.reload + expect(scope.strategies.count).to eq(1) + expect(scope.strategies).not_to include(params[:strategy]) + end + + context 'when strategies will be empty' do + let(:strategies) { [params[:strategy]] } + + it 'deletes the persisted scope' do + subject + + expect(feature_flag.scopes.exists?(environment_scope: params[:environment_scope])) + .to eq(false) + end + end + end + + context 'when there is no persisted strategy' do + let(:strategies) { [{ name: 'default', parameters: {} }] } + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Strategy not found') + end + end + end + + context 'when there is no persisted scope' do + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Feature Flag Scope not found') + end + end + end + + context 'when there is no persisted feature flag' do + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Feature Flag not found') + end + end + end + end +end diff --git a/spec/services/feature_flags/enable_service_spec.rb b/spec/services/feature_flags/enable_service_spec.rb new file mode 100644 index 00000000000..26dffcae0c7 --- /dev/null +++ b/spec/services/feature_flags/enable_service_spec.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FeatureFlags::EnableService do + include FeatureFlagHelpers + + let_it_be(:user) { create(:user) } + let(:project) { create(:project) } + let(:service) { described_class.new(project, user, params) } + let(:params) { {} } + + before do + project.add_developer(user) + end + + describe '#execute' do + subject { service.execute } + + context 'with params to enable default strategy on prd scope' do + let(:params) do + { + name: 'awesome', + environment_scope: 'prd', + strategy: { name: 'default', parameters: {} }.stringify_keys + } + end + + context 'when there is no persisted feature flag' do + it 'creates a new feature flag with scope' do + feature_flag = subject[:feature_flag] + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(subject[:status]).to eq(:success) + expect(feature_flag.name).to eq(params[:name]) + expect(feature_flag.default_scope).not_to be_active + expect(scope).to be_active + expect(scope.strategies).to include(params[:strategy]) + end + + context 'when params include default scope' do + let(:params) do + { + name: 'awesome', + environment_scope: '*', + strategy: { name: 'userWithId', parameters: { 'userIds': 'abc' } }.deep_stringify_keys + } + end + + it 'create a new feature flag with an active default scope with the specified strategy' do + feature_flag = subject[:feature_flag] + expect(subject[:status]).to eq(:success) + expect(feature_flag.default_scope).to be_active + expect(feature_flag.default_scope.strategies).to include(params[:strategy]) + end + end + end + + context 'when there is a persisted feature flag' do + let!(:feature_flag) { create_flag(project, params[:name]) } + + context 'when there is no persisted scope' do + it 'creates a new scope for the persisted feature flag' do + feature_flag = subject[:feature_flag] + scope = feature_flag.scopes.find_by_environment_scope(params[:environment_scope]) + expect(subject[:status]).to eq(:success) + expect(feature_flag.name).to eq(params[:name]) + expect(scope).to be_active + expect(scope.strategies).to include(params[:strategy]) + end + end + + context 'when there is a persisted scope' do + let!(:feature_flag_scope) do + create_scope(feature_flag, params[:environment_scope], active, strategies) + end + + let(:active) { true } + + context 'when the persisted scope does not have the specified strategy yet' do + let(:strategies) { [{ name: 'userWithId', parameters: { 'userIds': 'abc' } }] } + + it 'adds the specified strategy to the scope' do + subject + + feature_flag_scope.reload + expect(feature_flag_scope.strategies).to include(params[:strategy]) + end + + context 'when the persisted scope is inactive' do + let(:active) { false } + + it 'reactivates the scope' do + expect { subject } + .to change { feature_flag_scope.reload.active }.from(false).to(true) + end + end + end + + context 'when the persisted scope has the specified strategy already' do + let(:strategies) { [params[:strategy]] } + + it 'does not add a duplicated strategy to the scope' do + expect { subject } + .not_to change { feature_flag_scope.reload.strategies.count } + end + end + end + end + end + + context 'when strategy is not specified in params' do + let(:params) do + { + name: 'awesome', + environment_scope: 'prd' + } + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include('Scopes strategies must be an array of strategy hashes') + end + end + + context 'when environment scope is not specified in params' do + let(:params) do + { + name: 'awesome', + strategy: { name: 'default', parameters: {} }.stringify_keys + } + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include("Scopes environment scope can't be blank") + end + end + + context 'when name is not specified in params' do + let(:params) do + { + environment_scope: 'prd', + strategy: { name: 'default', parameters: {} }.stringify_keys + } + end + + it 'returns error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to include("Name can't be blank") + end + end + end +end diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb new file mode 100644 index 00000000000..16c3ff23443 --- /dev/null +++ b/spec/services/feature_flags/update_service_spec.rb @@ -0,0 +1,250 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FeatureFlags::UpdateService do + let(:project) { create(:project) } + let(:developer) { create(:user) } + let(:reporter) { create(:user) } + let(:user) { developer } + let(:feature_flag) { create(:operations_feature_flag, project: project, active: true) } + + before do + project.add_developer(developer) + project.add_reporter(reporter) + end + + describe '#execute' do + subject { described_class.new(project, user, params).execute(feature_flag) } + + let(:params) { { name: 'new_name' } } + let(:audit_event_message) do + AuditEvent.last.details[:custom_message] + end + + it 'returns success status' do + expect(subject[:status]).to eq(:success) + end + + it 'creates audit event with correct message' do + name_was = feature_flag.name + + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to( + eq("Updated feature flag <strong>new_name</strong>. "\ + "Updated name from <strong>\"#{name_was}\"</strong> "\ + "to <strong>\"new_name\"</strong>.") + ) + end + + context 'with invalid params' do + let(:params) { { name: nil } } + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:http_status]).to eq(:bad_request) + end + + it 'returns error messages' do + expect(subject[:message]).to include("Name can't be blank") + end + + it 'does not create audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + + context 'when user is reporter' do + let(:user) { reporter } + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq('Access Denied') + end + end + + context 'when nothing is changed' do + let(:params) { {} } + + it 'returns success status' do + expect(subject[:status]).to eq(:success) + end + + it 'does not create audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + + context 'description is being changed' do + let(:params) { { description: 'new description' } } + + it 'creates audit event with changed description' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to( + include("Updated description from <strong>\"\"</strong>"\ + " to <strong>\"new description\"</strong>.") + ) + end + end + + context 'when flag active state is changed' do + let(:params) do + { + active: false + } + end + + it 'creates audit event about changing active state' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to( + include('Updated active from <strong>"true"</strong> to <strong>"false"</strong>.') + ) + end + end + + context 'when scope active state is changed' do + let(:params) do + { + scopes_attributes: [{ id: feature_flag.scopes.first.id, active: false }] + } + end + + it 'creates audit event about changing active state' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to( + include("Updated rule <strong>*</strong> active state "\ + "from <strong>true</strong> to <strong>false</strong>.") + ) + end + end + + context 'when scope is renamed' do + let(:changed_scope) { feature_flag.scopes.create!(environment_scope: 'review', active: true) } + let(:params) do + { + scopes_attributes: [{ id: changed_scope.id, environment_scope: 'staging' }] + } + end + + it 'creates audit event with changed name' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to( + include("Updated rule <strong>staging</strong> environment scope "\ + "from <strong>review</strong> to <strong>staging</strong>.") + ) + end + + context 'when scope can not be updated' do + let(:params) do + { + scopes_attributes: [{ id: changed_scope.id, environment_scope: '' }] + } + end + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + end + + it 'returns error messages' do + expect(subject[:message]).to include("Scopes environment scope can't be blank") + end + + it 'does not create audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + end + + context 'when scope is deleted' do + let(:deleted_scope) { feature_flag.scopes.create!(environment_scope: 'review', active: true) } + let(:params) do + { + scopes_attributes: [{ id: deleted_scope.id, '_destroy': true }] + } + end + + it 'creates audit event with deleted scope' do + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to include("Deleted rule <strong>review</strong>.") + end + + context 'when scope can not be deleted' do + before do + allow(deleted_scope).to receive(:destroy).and_return(false) + end + + it 'does not create audit event' do + expect do + subject + end.to not_change { AuditEvent.count }.and raise_error(ActiveRecord::RecordNotDestroyed) + end + end + end + + context 'when new scope is being added' do + let(:new_environment_scope) { 'review' } + let(:params) do + { + scopes_attributes: [{ environment_scope: new_environment_scope, active: true }] + } + end + + it 'creates audit event with new scope' do + expected = 'Created rule <strong>review</strong> and set it as <strong>active</strong> '\ + 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>.' + + subject + + expect(audit_event_message).to include(expected) + end + + context 'when scope can not be created' do + let(:new_environment_scope) { '' } + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + end + + it 'returns error messages' do + expect(subject[:message]).to include("Scopes environment scope can't be blank") + end + + it 'does not create audit event' do + expect { subject }.not_to change { AuditEvent.count } + end + end + end + + context 'when the strategy is changed' do + let(:scope) do + create(:operations_feature_flag_scope, + feature_flag: feature_flag, + environment_scope: 'sandbox', + strategies: [{ name: "default", parameters: {} }]) + end + + let(:params) do + { + scopes_attributes: [{ + id: scope.id, + environment_scope: 'sandbox', + strategies: [{ + name: 'gradualRolloutUserId', + parameters: { + groupId: 'mygroup', + percentage: "40" + } + }] + }] + } + end + + it 'creates an audit event' do + expected = %r{Updated rule <strong>sandbox</strong> strategies from <strong>.*</strong> to <strong>.*</strong>.} + + expect { subject }.to change { AuditEvent.count }.by(1) + expect(audit_event_message).to match(expected) + end + end + end +end diff --git a/spec/services/milestones/destroy_service_spec.rb b/spec/services/milestones/destroy_service_spec.rb index 66c5c504c64..dd68471d927 100644 --- a/spec/services/milestones/destroy_service_spec.rb +++ b/spec/services/milestones/destroy_service_spec.rb @@ -45,7 +45,7 @@ RSpec.describe Milestones::DestroyService do let(:group_milestone) { create(:milestone, group: group) } before do - project.update(namespace: group) + project.update!(namespace: group) group.add_developer(user) end diff --git a/spec/services/milestones/promote_service_spec.rb b/spec/services/milestones/promote_service_spec.rb index f0a34241c74..8f4201d8d94 100644 --- a/spec/services/milestones/promote_service_spec.rb +++ b/spec/services/milestones/promote_service_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Milestones::PromoteService do end it 'raises error if project does not belong to a group' do - project.update(namespace: user.namespace) + project.update!(namespace: user.namespace) expect { service.execute(milestone) }.to raise_error(described_class::PromoteMilestoneError) end diff --git a/spec/services/milestones/transfer_service_spec.rb b/spec/services/milestones/transfer_service_spec.rb index 4a626fe688a..6f4f55b2bd0 100644 --- a/spec/services/milestones/transfer_service_spec.rb +++ b/spec/services/milestones/transfer_service_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Milestones::TransferService do new_group.add_maintainer(user) project.add_maintainer(user) # simulate project transfer - project.update(group: new_group) + project.update!(group: new_group) end context 'without existing milestone at the new group level' do |