From de123aba96869395eb4878d00947fcb35f27306a Mon Sep 17 00:00:00 2001
From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Fri, 3 Dec 2021 22:35:30 +0530
Subject: [PATCH] Fix Rails/SaveBang offenses
Changelog: other
EE: true
---
.rubocop_todo/rails/save_bang.yml | 9 --------
ee/spec/lib/gitlab/auth/ldap/access_spec.rb | 8 +++----
ee/spec/lib/gitlab/auth/o_auth/user_spec.rb | 2 +-
ee/spec/lib/gitlab/auth/saml/user_spec.rb | 22 +++++++++----------
.../lib/gitlab/elastic/search_results_spec.rb | 4 ++--
ee/spec/lib/gitlab/geo_spec.rb | 2 +-
ee/spec/lib/gitlab/git_access_spec.rb | 6 ++---
.../group/relation_factory_spec.rb | 2 +-
ee/spec/lib/gitlab/mirror_spec.rb | 2 +-
9 files changed, 24 insertions(+), 33 deletions(-)
diff --git a/.rubocop_todo/rails/save_bang.yml b/.rubocop_todo/rails/save_bang.yml
index f83bb1fddef01..e3d97a45afdba 100644
--- a/.rubocop_todo/rails/save_bang.yml
+++ b/.rubocop_todo/rails/save_bang.yml
@@ -2,15 +2,6 @@
Rails/SaveBang:
Exclude:
- ee/spec/lib/analytics/merge_request_metrics_calculator_spec.rb
- - ee/spec/lib/gitlab/auth/ldap/access_spec.rb
- - ee/spec/lib/gitlab/auth/o_auth/user_spec.rb
- - ee/spec/lib/gitlab/auth/saml/user_spec.rb
- - ee/spec/lib/gitlab/elastic/search_results_spec.rb
- - ee/spec/lib/gitlab/email/handler/ee/service_desk_handler_spec.rb
- - ee/spec/lib/gitlab/geo_spec.rb
- - ee/spec/lib/gitlab/git_access_spec.rb
- - ee/spec/lib/gitlab/import_export/group/relation_factory_spec.rb
- - ee/spec/lib/gitlab/mirror_spec.rb
- ee/spec/models/application_setting_spec.rb
- ee/spec/models/approval_merge_request_rule_spec.rb
- ee/spec/models/approval_project_rule_spec.rb
diff --git a/ee/spec/lib/gitlab/auth/ldap/access_spec.rb b/ee/spec/lib/gitlab/auth/ldap/access_spec.rb
index 6520e00aef934..feabdcf5ef1fc 100644
--- a/ee/spec/lib/gitlab/auth/ldap/access_spec.rb
+++ b/ee/spec/lib/gitlab/auth/ldap/access_spec.rb
@@ -262,7 +262,7 @@
it 'does not performs the membership update for existing users' do
user.created_at = Time.now - 10.minutes
user.last_credential_check_at = Time.now
- user.save
+ user.save!
expect(LdapGroupLink).not_to receive(:where)
expect(LdapGroupSyncWorker).not_to receive(:perform_async)
@@ -321,7 +321,7 @@
context 'user has at least one LDAPKey' do
before do
- user.keys.ldap.create key: ssh_key, title: 'to be removed'
+ user.keys.ldap.create! key: ssh_key, title: 'to be removed'
end
it 'removes a SSH key if it is no longer in LDAP' do
@@ -356,7 +356,7 @@
it 'updates existing Kerberos identity in GitLab if Active Directory has a different one' do
allow_any_instance_of(EE::Gitlab::Auth::Ldap::Person).to receive_messages(kerberos_principal: 'otherlogin@BAR.COM')
- user.identities.build(provider: 'kerberos', extern_uid: 'mylogin@FOO.COM').save
+ user.identities.build(provider: 'kerberos', extern_uid: 'mylogin@FOO.COM').save!
expect { access.update_user }.not_to change(user.identities.where(provider: 'kerberos'), :count)
expect(user.identities.where(provider: 'kerberos').last.extern_uid).to eq('otherlogin@BAR.COM')
@@ -364,7 +364,7 @@
it 'does not remove Kerberos identities from GitLab if they are none in the LDAP provider' do
allow_any_instance_of(EE::Gitlab::Auth::Ldap::Person).to receive_messages(kerberos_principal: nil)
- user.identities.build(provider: 'kerberos', extern_uid: 'otherlogin@BAR.COM').save
+ user.identities.build(provider: 'kerberos', extern_uid: 'otherlogin@BAR.COM').save!
expect { access.update_user }.not_to change(user.identities.where(provider: 'kerberos'), :count)
expect(user.identities.where(provider: 'kerberos').last.extern_uid).to eq('otherlogin@BAR.COM')
diff --git a/ee/spec/lib/gitlab/auth/o_auth/user_spec.rb b/ee/spec/lib/gitlab/auth/o_auth/user_spec.rb
index 9934436f9162a..608eaeea9b4bb 100644
--- a/ee/spec/lib/gitlab/auth/o_auth/user_spec.rb
+++ b/ee/spec/lib/gitlab/auth/o_auth/user_spec.rb
@@ -27,7 +27,7 @@
stub_ldap_person_find_by_uid(uid, ldap_entry)
- oauth_user.save
+ oauth_user.save # rubocop:disable Rails/SaveBang
end
it 'links the LDAP person to the GitLab user' do
diff --git a/ee/spec/lib/gitlab/auth/saml/user_spec.rb b/ee/spec/lib/gitlab/auth/saml/user_spec.rb
index f46798ea4f3ed..4fac2fc01beab 100644
--- a/ee/spec/lib/gitlab/auth/saml/user_spec.rb
+++ b/ee/spec/lib/gitlab/auth/saml/user_spec.rb
@@ -50,7 +50,7 @@ def stub_saml_group_config(type, groups)
%w(admin auditor).each do |group_type|
it "marks the user as #{group_type} when the user is in the configured group" do
stub_saml_group_config(group_type, %w(Developers))
- saml_user.save
+ saml_user.save # rubocop:disable Rails/SaveBang
expect(gl_user).to be_valid
expect(gl_user.public_send(group_type)).to be_truthy
@@ -58,7 +58,7 @@ def stub_saml_group_config(type, groups)
it "does not mark the user as #{group_type} when the user is not in the configured group" do
stub_saml_group_config(group_type, %w(Admin))
- saml_user.save
+ saml_user.save # rubocop:disable Rails/SaveBang
expect(gl_user).to be_valid
expect(gl_user.public_send(group_type)).to be_falsey
@@ -67,7 +67,7 @@ def stub_saml_group_config(type, groups)
it "demotes from #{group_type} if not in the configured group" do
create(:user, email: 'john@mail.com', username: 'john').update_attribute(group_type, true)
stub_saml_group_config(group_type, %w(Admin))
- saml_user.save
+ saml_user.save # rubocop:disable Rails/SaveBang
expect(gl_user).to be_valid
expect(gl_user.public_send(group_type)).to be_falsey
@@ -76,14 +76,14 @@ def stub_saml_group_config(type, groups)
it "does not demote from #{group_type} if not configured" do
create(:user, email: 'john@mail.com', username: 'john').update_attribute(group_type, true)
stub_saml_group_config(group_type, [])
- saml_user.save
+ saml_user.save # rubocop:disable Rails/SaveBang
expect(gl_user).to be_valid
expect(gl_user.public_send(group_type)).to be_truthy
end
it "skips #{group_type} if not configured" do
- saml_user.save
+ saml_user.save # rubocop:disable Rails/SaveBang
expect(gl_user).to be_valid
expect(gl_user.public_send(group_type)).to be_falsey
@@ -96,7 +96,7 @@ def stub_saml_group_config(type, groups)
context 'required groups' do
context 'not defined' do
it 'lets anyone in' do
- saml_user.save
+ saml_user.save # rubocop:disable Rails/SaveBang
expect(gl_user).to be_valid
end
@@ -109,21 +109,21 @@ def stub_saml_group_config(type, groups)
it 'lets members in' do
stub_saml_required_group_config(%w(Developers))
- saml_user.save
+ saml_user.save # rubocop:disable Rails/SaveBang
expect(gl_user).to be_valid
end
it 'unblocks already blocked members' do
stub_saml_required_group_config(%w(Developers))
- saml_user.save.ldap_block
+ saml_user.save.ldap_block # rubocop:disable Rails/SaveBang
expect(saml_user.find_user).to be_active
end
it 'does not unblock manually blocked members' do
stub_saml_required_group_config(%w(Developers))
- saml_user.save.block!
+ saml_user.save.block! # rubocop:disable Rails/SaveBang
expect(saml_user.find_user).not_to be_active
end
@@ -131,14 +131,14 @@ def stub_saml_group_config(type, groups)
it 'does not allow non-members' do
stub_saml_required_group_config(%w(ArchitectureAstronauts))
- expect { saml_user.save }.to raise_error Gitlab::Auth::OAuth::User::SignupDisabledError
+ expect { saml_user.save }.to raise_error Gitlab::Auth::OAuth::User::SignupDisabledError # rubocop:disable Rails/SaveBang
end
it 'blocks non-members' do
orig_groups = auth_hash.extra.raw_info["groups"]
auth_hash.extra.raw_info.add("groups", "ArchitectureAstronauts")
stub_saml_required_group_config(%w(ArchitectureAstronauts))
- saml_user.save
+ saml_user.save # rubocop:disable Rails/SaveBang
auth_hash.extra.raw_info.set("groups", orig_groups)
expect(saml_user.find_user).to be_ldap_blocked
diff --git a/ee/spec/lib/gitlab/elastic/search_results_spec.rb b/ee/spec/lib/gitlab/elastic/search_results_spec.rb
index 24da153184854..f5813c8b78ec7 100644
--- a/ee/spec/lib/gitlab/elastic/search_results_spec.rb
+++ b/ee/spec/lib/gitlab/elastic/search_results_spec.rb
@@ -1126,7 +1126,7 @@ def self.ruby_method_123(ruby_another_method_arg)
let(:limit_project_ids) { [private_project2.id] }
before do
- private_project2.project_members.create(user: user, access_level: ProjectMember::DEVELOPER)
+ private_project2.project_members.create!(user: user, access_level: ProjectMember::DEVELOPER)
end
context 'issues' do
@@ -1190,7 +1190,7 @@ def self.ruby_method_123(ruby_another_method_arg)
context 'when user is admin' do
context 'when admin mode enabled', :enable_admin_mode do
it 'returns right set of milestones' do
- user.update(admin: true)
+ user.update!(admin: true)
public_project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE)
public_project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE)
internal_project.project_feature.update!(issues_access_level: ProjectFeature::DISABLED)
diff --git a/ee/spec/lib/gitlab/geo_spec.rb b/ee/spec/lib/gitlab/geo_spec.rb
index 82c0094e946a6..90a82aef93878 100644
--- a/ee/spec/lib/gitlab/geo_spec.rb
+++ b/ee/spec/lib/gitlab/geo_spec.rb
@@ -73,7 +73,7 @@
end
it 'returns false when primary does not exist' do
- primary_node.destroy
+ primary_node.destroy!
expect(described_class.primary_node_configured?).to be_falsey
end
diff --git a/ee/spec/lib/gitlab/git_access_spec.rb b/ee/spec/lib/gitlab/git_access_spec.rb
index 3b905c09a5742..00a0ff1fea88d 100644
--- a/ee/spec/lib/gitlab/git_access_spec.rb
+++ b/ee/spec/lib/gitlab/git_access_spec.rb
@@ -480,7 +480,7 @@ def self.run_permission_checks(permissions_matrix)
project.add_role(user, role)
end
- protected_branch.save
+ protected_branch.save!
aggregate_failures do
matrix.each do |action, allowed|
@@ -506,7 +506,7 @@ def self.run_group_permission_checks(permissions_matrix)
create(:project_group_link, role, group: group,
project: project)
- protected_branch.save
+ protected_branch.save!
aggregate_failures do
matrix.each do |action, allowed|
@@ -880,7 +880,7 @@ def stub_redis
let(:actor) { deploy_key }
before do
- deploy_key.deploy_keys_projects.create(project: project, can_push: true)
+ deploy_key.deploy_keys_projects.create!(project: project, can_push: true)
end
it 'allows push and pull access' do
diff --git a/ee/spec/lib/gitlab/import_export/group/relation_factory_spec.rb b/ee/spec/lib/gitlab/import_export/group/relation_factory_spec.rb
index 0f7a31f33ce47..5dc4a56957ccd 100644
--- a/ee/spec/lib/gitlab/import_export/group/relation_factory_spec.rb
+++ b/ee/spec/lib/gitlab/import_export/group/relation_factory_spec.rb
@@ -9,7 +9,7 @@
let(:importer_user) { admin }
let(:excluded_keys) { [] }
let(:created_object) do
- described_class.create(
+ described_class.create( # rubocop:disable Rails/SaveBang
relation_sym: relation_sym,
relation_hash: relation_hash,
relation_index: 1,
diff --git a/ee/spec/lib/gitlab/mirror_spec.rb b/ee/spec/lib/gitlab/mirror_spec.rb
index 810358dc4f0fd..a9f807add3996 100644
--- a/ee/spec/lib/gitlab/mirror_spec.rb
+++ b/ee/spec/lib/gitlab/mirror_spec.rb
@@ -28,7 +28,7 @@
describe 'without jobs already running' do
before do
- Sidekiq::Cron::Job.find("update_all_mirrors_worker")&.destroy
+ Sidekiq::Cron::Job.find("update_all_mirrors_worker")&.destroy # rubocop:disable Rails/SaveBang
end
it 'creates update_all_mirrors_worker' do
--
GitLab