summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2020-09-19 15:09:58 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2020-09-19 15:09:58 +0000
commit6cf9209f12a250a371a6fdbd0812dda847ba1284 (patch)
treea1ba20014bc0cbbf31ec494ae369ea7eed73865a
parente3ae3532a1b225938535028bbdb7bfb8fc9b2f48 (diff)
downloadgitlab-ce-6cf9209f12a250a371a6fdbd0812dda847ba1284.tar.gz
Add latest changes from gitlab-org/gitlab@master
-rw-r--r--.rubocop_todo.yml3
-rw-r--r--app/models/project_statistics.rb2
-rw-r--r--app/models/snippet_statistics.rb2
-rw-r--r--app/services/feature_flags/base_service.rb55
-rw-r--r--app/services/feature_flags/create_service.rb52
-rw-r--r--app/services/feature_flags/destroy_service.rb33
-rw-r--r--app/services/feature_flags/disable_service.rb46
-rw-r--r--app/services/feature_flags/enable_service.rb93
-rw-r--r--app/services/feature_flags/update_service.rb87
-rw-r--r--changelogs/unreleased/250041-does-not-refresh-project-snippet-statistics-on-a-read-only-instanc.yml5
-rw-r--r--changelogs/unreleased/rails-save-bang-33.yml5
-rw-r--r--spec/models/project_statistics_spec.rb17
-rw-r--r--spec/models/snippet_statistics_spec.rb19
-rw-r--r--spec/services/feature_flags/create_service_spec.rb79
-rw-r--r--spec/services/feature_flags/destroy_service_spec.rb61
-rw-r--r--spec/services/feature_flags/disable_service_spec.rb91
-rw-r--r--spec/services/feature_flags/enable_service_spec.rb153
-rw-r--r--spec/services/feature_flags/update_service_spec.rb250
-rw-r--r--spec/services/milestones/destroy_service_spec.rb2
-rw-r--r--spec/services/milestones/promote_service_spec.rb2
-rw-r--r--spec/services/milestones/transfer_service_spec.rb2
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