From 67db38fc01aeefe8ebce455c223d0292bd5d2ce4 Mon Sep 17 00:00:00 2001
From: Siddharth Asthana <siddharthasthana31@gmail.com>
Date: Sat, 4 Dec 2021 00:17:25 +0530
Subject: [PATCH] Fix Rails/SaveBang offenses
Changelog: other
EE: true
---
.rubocop_todo/rails/save_bang.yml | 7 ----
ee/spec/models/application_setting_spec.rb | 4 +-
.../approval_merge_request_rule_spec.rb | 6 +--
ee/spec/models/approval_project_rule_spec.rb | 10 ++---
ee/spec/models/burndown_spec.rb | 6 +--
.../elasticsearch_indexed_namespace_spec.rb | 2 +-
ee/spec/models/gitlab_subscription_spec.rb | 2 +-
ee/spec/models/issue_spec.rb | 42 +++++++++----------
8 files changed, 36 insertions(+), 43 deletions(-)
diff --git a/.rubocop_todo/rails/save_bang.yml b/.rubocop_todo/rails/save_bang.yml
index 807f2d6485d3f..15361fd674359 100644
--- a/.rubocop_todo/rails/save_bang.yml
+++ b/.rubocop_todo/rails/save_bang.yml
@@ -2,13 +2,6 @@
Rails/SaveBang:
Exclude:
- ee/spec/lib/analytics/merge_request_metrics_calculator_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
- - ee/spec/models/burndown_spec.rb
- - ee/spec/models/elasticsearch_indexed_namespace_spec.rb
- - ee/spec/models/gitlab_subscription_spec.rb
- - ee/spec/models/issue_spec.rb
- ee/spec/models/protected_environment_spec.rb
- ee/spec/models/repository_spec.rb
- ee/spec/models/scim_identity_spec.rb
diff --git a/ee/spec/models/application_setting_spec.rb b/ee/spec/models/application_setting_spec.rb
index fdaa06bffef2b..eca17d7d95338 100644
--- a/ee/spec/models/application_setting_spec.rb
+++ b/ee/spec/models/application_setting_spec.rb
@@ -755,7 +755,7 @@ def expect_is_es_licensed
it "doesn't call the update lifetime service" do
expect(::PersonalAccessTokens::Instance::UpdateLifetimeService).not_to receive(:new)
- setting.save
+ setting.save!
end
end
@@ -770,7 +770,7 @@ def expect_is_es_licensed
expect(service).to receive(:execute)
end
- setting.save
+ setting.save!
end
end
end
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index 9aefffa4d200d..cd625b2c0968d 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -78,7 +78,7 @@
expect(rule).not_to be_valid
expect(rule.errors.messages).to eq(rule_type: ['any-approver for the merge request already exists'])
- expect { rule.save(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)
+ expect { rule.save!(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)
end
end
end
@@ -264,7 +264,7 @@
context 'when project merge_requests_author_approval is true' do
it 'contains author' do
- merge_request.project.update(merge_requests_author_approval: true)
+ merge_request.project.update!(merge_requests_author_approval: true)
expect(subject.approvers).to contain_exactly(merge_request.author)
end
@@ -272,7 +272,7 @@
context 'when project merge_requests_author_approval is false' do
before do
- merge_request.project.update(merge_requests_author_approval: false)
+ merge_request.project.update!(merge_requests_author_approval: false)
end
it 'does not contain author' do
diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb
index d759b54cbe10b..c034a232ca630 100644
--- a/ee/spec/models/approval_project_rule_spec.rb
+++ b/ee/spec/models/approval_project_rule_spec.rb
@@ -233,7 +233,7 @@
expect(rule).not_to be_valid
expect(rule.errors.messages).to eq(rule_type: ['any-approver for the project already exists'])
- expect { rule.save(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)
+ expect { rule.save!(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)
end
end
@@ -273,28 +273,28 @@
end
describe '#audit_add users after :add' do
- let(:action!) { rule.update(users: [user, new_user]) }
+ let(:action!) { rule.update!(users: [user, new_user]) }
let(:message) { 'Added User Spiderman to approval group on Vulnerability rule' }
it_behaves_like 'auditable'
end
describe '#audit_remove users after :remove' do
- let(:action!) { rule.update(users: []) }
+ let(:action!) { rule.update!(users: []) }
let(:message) { 'Removed User Batman from approval group on Vulnerability rule' }
it_behaves_like 'auditable'
end
describe '#audit_add groups after :add' do
- let(:action!) { rule.update(groups: [group, new_group]) }
+ let(:action!) { rule.update!(groups: [group, new_group]) }
let(:message) { 'Added Group Avengers to approval group on Vulnerability rule' }
it_behaves_like 'auditable'
end
describe '#audit_remove groups after :remove' do
- let(:action!) { rule.update(groups: []) }
+ let(:action!) { rule.update!(groups: []) }
let(:message) { 'Removed Group Justice League from approval group on Vulnerability rule' }
it_behaves_like 'auditable'
diff --git a/ee/spec/models/burndown_spec.rb b/ee/spec/models/burndown_spec.rb
index d9eeda83958de..de18c9ec7af6a 100644
--- a/ee/spec/models/burndown_spec.rb
+++ b/ee/spec/models/burndown_spec.rb
@@ -39,7 +39,7 @@
let_it_be(:non_member) { create(:user) }
subject do
- project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+ project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
described_class.new(milestone.issues_visible_to_user(non_member), milestone.start_date, milestone.due_date).as_json.each { |event| event[:created_at] = event[:created_at].to_date }
end
@@ -57,13 +57,13 @@
end
it "returns empty array if milestone start date is nil" do
- milestone.update(start_date: nil)
+ milestone.update!(start_date: nil)
expect(subject).to eq([])
end
it "returns empty array if milestone due date is nil" do
- milestone.update(due_date: nil)
+ milestone.update!(due_date: nil)
expect(subject).to eq([])
end
diff --git a/ee/spec/models/elasticsearch_indexed_namespace_spec.rb b/ee/spec/models/elasticsearch_indexed_namespace_spec.rb
index 08be17961c611..a127669d59ef3 100644
--- a/ee/spec/models/elasticsearch_indexed_namespace_spec.rb
+++ b/ee/spec/models/elasticsearch_indexed_namespace_spec.rb
@@ -37,7 +37,7 @@
context 'with plans' do
Plan::PAID_HOSTED_PLANS.each do |plan|
plan_factory = "#{plan}_plan"
- let_it_be(plan_factory) { create(plan_factory) }
+ let_it_be(plan_factory) { create(plan_factory) } # rubocop:disable Rails/SaveBang
end
let_it_be(:namespaces) { create_list(:namespace, 3) }
diff --git a/ee/spec/models/gitlab_subscription_spec.rb b/ee/spec/models/gitlab_subscription_spec.rb
index c425e4491446b..609f57fe41fa0 100644
--- a/ee/spec/models/gitlab_subscription_spec.rb
+++ b/ee/spec/models/gitlab_subscription_spec.rb
@@ -6,7 +6,7 @@
using RSpec::Parameterized::TableSyntax
%i[free_plan bronze_plan premium_plan ultimate_plan].each do |plan|
- let_it_be(plan) { create(plan) }
+ let_it_be(plan) { create(plan) } # rubocop:disable Rails/SaveBang
end
describe 'default values', :freeze_time do
diff --git a/ee/spec/models/issue_spec.rb b/ee/spec/models/issue_spec.rb
index 3a397984f871f..ff6e55f47538c 100644
--- a/ee/spec/models/issue_spec.rb
+++ b/ee/spec/models/issue_spec.rb
@@ -546,7 +546,7 @@
award_emoji = create(:award_emoji, :upvote, awardable: issue)
- award_emoji.destroy
+ award_emoji.destroy!
end
end
end
@@ -620,7 +620,7 @@
end
it 'positions issues even when after and before positions are the same' do
- issue1.update relative_position: issue.relative_position
+ issue1.update! relative_position: issue.relative_position
new_issue.move_between(issue, issue1)
[issue, issue1].each(&:reset)
@@ -630,7 +630,7 @@
end
it 'positions issues between other two if distance is 1' do
- issue1.update relative_position: issue.relative_position + 1
+ issue1.update! relative_position: issue.relative_position + 1
new_issue.move_between(issue, issue1)
[issue, issue1].each(&:reset)
@@ -640,8 +640,8 @@
end
it 'positions issue in the middle of other two if distance is big enough' do
- issue.update relative_position: 6000
- issue1.update relative_position: 10000
+ issue.update! relative_position: 6000
+ issue1.update! relative_position: 10000
new_issue.move_between(issue, issue1)
@@ -662,8 +662,8 @@
end
it 'positions issue in the middle of other two if distance is not big enough' do
- issue.update relative_position: 100
- issue1.update relative_position: 400
+ issue.update! relative_position: 100
+ issue1.update! relative_position: 400
new_issue.move_between(issue, issue1)
@@ -671,8 +671,8 @@
end
it 'positions issue in the middle of other two is there is no place' do
- issue.update relative_position: 100
- issue1.update relative_position: 101
+ issue.update! relative_position: 100
+ issue1.update! relative_position: 101
new_issue.move_between(issue, issue1)
[issue, issue1].each(&:reset)
@@ -682,10 +682,10 @@
end
it 'uses rebalancing if there is no place' do
- issue.update relative_position: 100
- issue1.update relative_position: 101
+ issue.update! relative_position: 100
+ issue1.update! relative_position: 101
issue2 = create(:issue, relative_position: 102, project: project)
- new_issue.update relative_position: 103
+ new_issue.update! relative_position: 103
new_issue.move_between(issue1, issue2)
new_issue.save!
@@ -698,10 +698,10 @@
end
it 'positions issue right if we pass non-sequential parameters' do
- issue.update relative_position: 99
- issue1.update relative_position: 101
+ issue.update! relative_position: 99
+ issue1.update! relative_position: 101
issue2 = create(:issue, relative_position: 102, project: project)
- new_issue.update relative_position: 103
+ new_issue.update! relative_position: 103
new_issue.move_between(issue, issue2)
new_issue.save!
@@ -849,7 +849,7 @@
create(:issue_link, source: issue, target: blocked_issue_2, link_type: IssueLink::TYPE_BLOCKS)
create(:issue_link, source: issue, target: blocked_issue_3, link_type: IssueLink::TYPE_BLOCKS)
# Set to 0 for proper testing, this is being set by IssueLink callbacks.
- issue.update(blocking_issues_count: 0)
+ issue.update!(blocking_issues_count: 0)
expect { issue.update_blocking_issues_count! }
.to change { issue.blocking_issues_count }.from(0).to(3)
@@ -871,7 +871,7 @@
end
before do
- blocked_issue.update(blocking_issues_count: 0)
+ blocked_issue.update!(blocking_issues_count: 0)
end
context 'when blocked issue is closed' do
@@ -887,9 +887,9 @@
context 'when blocked issue is reopened' do
before do
blocked_issue.close
- blocked_issue.update(blocking_issues_count: 0)
- blocking_issue1.update(blocking_issues_count: 0)
- blocking_issue2.update(blocking_issues_count: 0)
+ blocked_issue.update!(blocking_issues_count: 0)
+ blocking_issue1.update!(blocking_issues_count: 0)
+ blocking_issue2.update!(blocking_issues_count: 0)
end
it 'updates blocking and blocked issues cache' do
@@ -1021,7 +1021,7 @@
before do
stub_licensed_features(incident_sla: license_available)
issue_type = incident_type ? 'incident' : 'issue'
- issue.update(issue_type: issue_type)
+ issue.update!(issue_type: issue_type)
end
it 'returns the expected value' do
--
GitLab