Fix Rails/SaveBang offenses

Changelog: other
EE: true

.rubocop_todo/rails/save_bang.yml

@@ -18,21 +18,6 @@ Rails/SaveBang:
   - ee/spec/models/visible_approvable_spec.rb
   - ee/spec/models/vulnerabilities/feedback_spec.rb
   - ee/spec/models/vulnerabilities/issue_link_spec.rb
-  - ee/spec/services/geo/blob_verification_secondary_service_spec.rb
-  - ee/spec/services/geo/files_expire_service_spec.rb
-  - ee/spec/services/geo/metrics_update_service_spec.rb
-  - ee/spec/services/geo/registry_consistency_service_spec.rb
-  - ee/spec/services/geo/repository_verification_secondary_service_spec.rb
-  - ee/spec/services/groups/autocomplete_service_spec.rb
-  - ee/spec/services/ldap_group_reset_service_spec.rb
-  - ee/spec/services/lfs/unlock_file_service_spec.rb
-  - ee/spec/services/merge_trains/refresh_merge_request_service_spec.rb
-  - ee/spec/services/quick_actions/interpret_service_spec.rb
-  - ee/spec/services/slash_commands/global_slack_handler_spec.rb
-  - ee/spec/services/start_pull_mirroring_service_spec.rb
-  - ee/spec/services/status_page/trigger_publish_service_spec.rb
-  - ee/spec/services/todo_service_spec.rb
-  - ee/spec/services/vulnerability_feedback/create_service_spec.rb
   - spec/lib/backup/manager_spec.rb
   - spec/lib/gitlab/alerting/alert_spec.rb
   - spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb

ee/spec/services/geo/files_expire_service_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe Geo::FilesExpireService, :geo do
     let!(:upload_registry) { create(:geo_upload_registry, :synced, file_id: upload.id) }
 
     before do
-      project.update(path: "#{project.path}_renamed")
+      project.update!(path: "#{project.path}_renamed")
     end
 
     context 'when in Geo secondary node' do

ee/spec/services/geo/metrics_update_service_spec.rb

@@ -109,8 +109,8 @@ RSpec.describe Geo::MetricsUpdateService, :geo, :prometheus do
       it 'updates metrics for all nodes' do
         allow(GeoNodeStatus).to receive(:current_node_status).and_return(GeoNodeStatus.from_json(primary_data.as_json))
 
-        secondary.update(status: GeoNodeStatus.from_json(data.as_json))
-        another_secondary.update(status: GeoNodeStatus.from_json(data.as_json))
+        secondary.update!(status: GeoNodeStatus.from_json(data.as_json))
+        another_secondary.update!(status: GeoNodeStatus.from_json(data.as_json))
 
         subject.execute
 

ee/spec/services/geo/registry_consistency_service_spec.rb

@@ -75,7 +75,7 @@ RSpec.describe Geo::RegistryConsistencyService, :geo, :use_clean_rails_memory_st
         end
 
         it 'does not exceed batch size' do
-          not_expected = create(model_class_factory)
+          not_expected = create(model_class_factory) # rubocop:disable Rails/SaveBang
 
           subject.execute
 

ee/spec/services/geo/repository_verification_secondary_service_spec.rb

@@ -101,7 +101,7 @@ RSpec.describe Geo::RepositoryVerificationSecondaryService, :geo do
       end
 
       it 'ensures the next retry time is capped properly' do
-        registry.update("#{type}_retry_count" => 30)
+        registry.update!("#{type}_retry_count" => 30)
 
         service.execute
 
@@ -135,7 +135,7 @@ RSpec.describe Geo::RepositoryVerificationSecondaryService, :geo do
       end
 
       it 'ensures the next retry time is capped properly' do
-        registry.update("#{type}_retry_count" => 30)
+        registry.update!("#{type}_retry_count" => 30)
 
         service.execute
 

ee/spec/services/ldap_group_reset_service_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe LdapGroupResetService do
     group.add_owner(user)
     group.add_owner(ldap_user)
     group.add_user(ldap_user_2, Gitlab::Access::REPORTER)
-    group.ldap_group_links.create cn: 'developers', group_access: Gitlab::Access::DEVELOPER
+    group.ldap_group_links.create! cn: 'developers', group_access: Gitlab::Access::DEVELOPER
   end
 
   describe '#execute' do

ee/spec/services/lfs/unlock_file_service_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe Lfs::UnlockFileService do
           stub_licensed_features(file_locks: file_locks_license)
 
           project.add_developer(lock_author)
-          project.path_locks.create(path: lock.path, user: lock_author)
+          project.path_locks.create!(path: lock.path, user: lock_author)
         end
 
         context 'when File Locking is available' do

ee/spec/services/merge_trains/refresh_merge_request_service_spec.rb

@@ -201,7 +201,7 @@ RSpec.describe MergeTrains::RefreshMergeRequestService do
       before do
         merge_request.merge_train.refresh_pipeline!(pipeline.id)
         merge_request.merge_params[:sha] = merge_request.diff_head_sha
-        merge_request.save
+        merge_request.save!
       end
 
       context 'when the merge request is the first queue' do
@@ -220,7 +220,7 @@ RSpec.describe MergeTrains::RefreshMergeRequestService do
         context 'when it failed to merge the merge request' do
           before do
             allow(merge_request).to receive(:mergeable_state?) { true }
-            merge_request.update(merge_error: 'Branch has been updated since the merge was requested.')
+            merge_request.update!(merge_error: 'Branch has been updated since the merge was requested.')
             allow_next_instance_of(MergeRequests::MergeService) do |instance|
               allow(instance).to receive(:execute) { { result: :error } }
             end

ee/spec/services/quick_actions/interpret_service_spec.rb

@@ -125,7 +125,7 @@ RSpec.describe QuickActions::InterpretService do
         let(:merge_request) { create(:merge_request, source_project: project) }
 
         it 'fetches assignees and populates them if content contains /assign' do
-          merge_request.update(assignee_ids: [user.id])
+          merge_request.update!(assignee_ids: [user.id])
 
           _, updates = service.execute("/assign @#{user2.username}", merge_request)
 
@@ -134,7 +134,7 @@ RSpec.describe QuickActions::InterpretService do
 
         context 'assign command with multiple assignees' do
           it 'fetches assignee and populates assignee_ids if content contains /assign' do
-            merge_request.update(assignee_ids: [user.id])
+            merge_request.update!(assignee_ids: [user.id])
 
             _, updates = service.execute("/assign @#{user.username}\n/assign @#{user2.username} @#{user3.username}", issue)
 
@@ -147,7 +147,7 @@ RSpec.describe QuickActions::InterpretService do
             end
 
             it 'does not recognize /assign with multiple user references' do
-              merge_request.update(assignee_ids: [user.id])
+              merge_request.update!(assignee_ids: [user.id])
 
               _, updates = service.execute("/assign @#{user2.username} @#{user3.username}", merge_request)
 
@@ -163,7 +163,7 @@ RSpec.describe QuickActions::InterpretService do
         let(:merge_request) { create(:merge_request, source_project: project) }
 
         it 'fetches reviewers and populates them if content contains /assign_reviewer' do
-          merge_request.update(reviewer_ids: [user.id])
+          merge_request.update!(reviewer_ids: [user.id])
 
           _, updates = service.execute("/assign_reviewer @#{user2.username}\n/assign_reviewer @#{user3.username}", merge_request)
 
@@ -172,7 +172,7 @@ RSpec.describe QuickActions::InterpretService do
 
         context 'assign command with multiple reviewers' do
           it 'assigns multiple reviewers while respecting previous assignments' do
-            merge_request.update(reviewer_ids: [user.id])
+            merge_request.update!(reviewer_ids: [user.id])
 
             _, updates = service.execute("/assign_reviewer @#{user.username}\n/assign_reviewer @#{user2.username} @#{user3.username}", merge_request)
 
@@ -188,7 +188,7 @@ RSpec.describe QuickActions::InterpretService do
 
       context 'unassign_reviewer command with multiple assignees' do
         it 'unassigns both reviewers if content contains /unassign_reviewer @user @user1' do
-          merge_request.update(reviewer_ids: [user.id, user2.id, user3.id])
+          merge_request.update!(reviewer_ids: [user.id, user2.id, user3.id])
 
           _, updates = service.execute("/unassign_reviewer @#{user.username} @#{user2.username}", merge_request)
 
@@ -230,7 +230,7 @@ RSpec.describe QuickActions::InterpretService do
         let(:merge_request) { create(:merge_request, source_project: project) }
 
         it 'unassigns user if content contains /unassign @user' do
-          merge_request.update(assignee_ids: [user.id, user2.id])
+          merge_request.update!(assignee_ids: [user.id, user2.id])
 
           _, updates = service.execute("/unassign @#{user2.username}", merge_request)
 
@@ -239,7 +239,7 @@ RSpec.describe QuickActions::InterpretService do
 
         context 'unassign command with multiple assignees' do
           it 'unassigns both users if content contains /unassign @user @user1' do
-            merge_request.update(assignee_ids: [user.id, user2.id, user3.id])
+            merge_request.update!(assignee_ids: [user.id, user2.id, user3.id])
 
             _, updates = service.execute("/unassign @#{user.username} @#{user2.username}", merge_request)
 
@@ -252,7 +252,7 @@ RSpec.describe QuickActions::InterpretService do
             end
 
             it 'does not recognize /unassign @user' do
-              merge_request.update(assignee_ids: [user.id, user2.id, user3.id])
+              merge_request.update!(assignee_ids: [user.id, user2.id, user3.id])
 
               _, updates = service.execute("/unassign @#{user.username}", merge_request)
 
@@ -1193,7 +1193,7 @@ RSpec.describe QuickActions::InterpretService do
     context 'approved merge request can be merged' do
       before do
         merge_request.update!(approvals_before_merge: 1)
-        merge_request.approvals.create(user: current_user)
+        merge_request.approvals.create!(user: current_user)
       end
 
       it_behaves_like 'empty command' do

ee/spec/services/slash_commands/global_slack_handler_spec.rb

@@ -40,7 +40,7 @@ RSpec.describe SlashCommands::GlobalSlackHandler do
       enable_slack_application(project)
 
       slack_integration = create(:slack_integration, integration: project.gitlab_slack_application_integration)
-      slack_integration.update(alias: project.full_path)
+      slack_integration.update!(alias: project.full_path)
 
       handler_with_valid_token(
         text: "#{project.full_path} issue new title",
@@ -69,7 +69,7 @@ RSpec.describe SlashCommands::GlobalSlackHandler do
       enable_slack_application(project)
 
       slack_integration = create(:slack_integration, integration: project.gitlab_slack_application_integration)
-      slack_integration.update(alias: project.full_path)
+      slack_integration.update!(alias: project.full_path)
 
       handler_with_valid_token(
         text: "#{project.full_path} issue new title",

ee/spec/services/start_pull_mirroring_service_spec.rb

@@ -33,7 +33,7 @@ RSpec.describe StartPullMirroringService do
       context 'when project mirror has been updated in the interval' do
         it 'schedules next execution' do
           freeze_time do
-            import_state.update(last_update_at: (interval_minutes - 1).minutes.ago, last_successful_update_at: 10.minutes.ago)
+            import_state.update!(last_update_at: (interval_minutes - 1).minutes.ago, last_successful_update_at: 10.minutes.ago)
 
             expect { execute }
               .to change { import_state.next_execution_timestamp }
@@ -45,7 +45,7 @@ RSpec.describe StartPullMirroringService do
 
       context 'when project mirror has been updated outside of the interval' do
         before do
-          import_state.update(last_update_at: (interval_minutes + 1).minutes.ago, last_successful_update_at: 10.minutes.ago)
+          import_state.update!(last_update_at: (interval_minutes + 1).minutes.ago, last_successful_update_at: 10.minutes.ago)
         end
 
         it_behaves_like 'force mirror update'
@@ -53,7 +53,7 @@ RSpec.describe StartPullMirroringService do
 
       context 'when project mirror has been updated in interval but has never been successfully updated' do
         before do
-          import_state.update(last_update_at: (interval_minutes - 1).minutes.ago, last_successful_update_at: nil)
+          import_state.update!(last_update_at: (interval_minutes - 1).minutes.ago, last_successful_update_at: nil)
         end
 
         it_behaves_like 'force mirror update'
@@ -81,7 +81,7 @@ RSpec.describe StartPullMirroringService do
   end
 
   before do
-    import_state.update(next_execution_timestamp: 1.minute.from_now)
+    import_state.update!(next_execution_timestamp: 1.minute.from_now)
   end
 
   context 'when pause_on_hard_failure is false' do
@@ -89,7 +89,7 @@ RSpec.describe StartPullMirroringService do
 
     context "when retried more than #{Gitlab::Mirror::MAX_RETRY} times" do
       before do
-        import_state.update(retry_count: Gitlab::Mirror::MAX_RETRY + 1)
+        import_state.update!(retry_count: Gitlab::Mirror::MAX_RETRY + 1)
       end
 
       it_behaves_like 'pull mirroring has started'
@@ -100,7 +100,7 @@ RSpec.describe StartPullMirroringService do
 
       context 'when mirror is due to be updated' do
         before do
-          import_state.update(next_execution_timestamp: 1.minute.ago)
+          import_state.update!(next_execution_timestamp: 1.minute.ago)
         end
 
         it_behaves_like 'pull mirroring has started'
@@ -109,7 +109,7 @@ RSpec.describe StartPullMirroringService do
 
     context 'when does not reach the max retry limit yet' do
       before do
-        import_state.update(retry_count: Gitlab::Mirror::MAX_RETRY - 1)
+        import_state.update!(retry_count: Gitlab::Mirror::MAX_RETRY - 1)
       end
 
       it_behaves_like 'pull mirroring has started'
@@ -117,7 +117,7 @@ RSpec.describe StartPullMirroringService do
 
       context 'when mirror is due to be updated' do
         before do
-          import_state.update(next_execution_timestamp: 1.minute.ago)
+          import_state.update!(next_execution_timestamp: 1.minute.ago)
         end
 
         it_behaves_like 'pull mirroring has not started', :success
@@ -130,7 +130,7 @@ RSpec.describe StartPullMirroringService do
 
     context "when retried more than #{Gitlab::Mirror::MAX_RETRY} times" do
       before do
-        import_state.update(retry_count: Gitlab::Mirror::MAX_RETRY + 1)
+        import_state.update!(retry_count: Gitlab::Mirror::MAX_RETRY + 1)
       end
 
       it_behaves_like 'retry count did not reset'
@@ -139,7 +139,7 @@ RSpec.describe StartPullMirroringService do
 
     context 'when does not reach the max retry limit yet' do
       before do
-        import_state.update(retry_count: Gitlab::Mirror::MAX_RETRY - 1)
+        import_state.update!(retry_count: Gitlab::Mirror::MAX_RETRY - 1)
       end
 
       it_behaves_like 'pull mirroring has started'
@@ -147,7 +147,7 @@ RSpec.describe StartPullMirroringService do
 
       context 'when mirror is due to be updated' do
         before do
-          import_state.update(next_execution_timestamp: 1.minute.ago)
+          import_state.update!(next_execution_timestamp: 1.minute.ago)
         end
 
         it_behaves_like 'pull mirroring has not started', :success

ee/spec/services/status_page/trigger_publish_service_spec.rb

@@ -130,7 +130,7 @@ RSpec.describe StatusPage::TriggerPublishService do
         context 'when destroyed' do
           include_examples 'trigger status page publish' do
             before do
-              triggered_by.destroy
+              triggered_by.destroy!
             end
           end
         end
@@ -161,7 +161,7 @@ RSpec.describe StatusPage::TriggerPublishService do
           context 'when destroyed' do
             include_examples 'trigger status page publish' do
               before do
-                triggered_by.destroy
+                triggered_by.destroy!
               end
             end
           end
@@ -241,7 +241,7 @@ RSpec.describe StatusPage::TriggerPublishService do
       context 'when status page is missing' do
         include_examples 'no trigger status page publish' do
           before do
-            project.status_page_setting.destroy
+            project.status_page_setting.destroy!
             project.reload
           end
         end

ee/spec/services/todo_service_spec.rb

@@ -79,7 +79,7 @@ RSpec.describe TodoService do
 
           context 'for directly addressed users' do
             before do
-              epic.update(description: description_directly_addressed)
+              epic.update!(description: description_directly_addressed)
             end
 
             let(:todo_params) { { action: Todo::DIRECTLY_ADDRESSED } }
@@ -90,7 +90,7 @@ RSpec.describe TodoService do
 
           context 'combined' do
             before do
-              epic.update(description: combined_mentions)
+              epic.update!(description: combined_mentions)
             end
 
             context 'mentioned users' do
@@ -111,7 +111,7 @@ RSpec.describe TodoService do
 
         context 'when an epic belongs to a private group' do
           before do
-            group.update(visibility: Gitlab::VisibilityLevel::PRIVATE)
+            group.update!(visibility: Gitlab::VisibilityLevel::PRIVATE)
           end
 
           context 'for mentioned users' do
@@ -137,7 +137,7 @@ RSpec.describe TodoService do
 
         context 'creates todos for group members when a group is mentioned' do
           before do
-            epic.update(description: group.to_reference)
+            epic.update!(description: group.to_reference)
           end
 
           let(:todo_params) { { action: Todo::DIRECTLY_ADDRESSED } }
@@ -161,7 +161,7 @@ RSpec.describe TodoService do
 
         context 'for directly addressed users' do
           before do
-            epic.update(description: description_directly_addressed)
+            epic.update!(description: description_directly_addressed)
           end
 
           let(:todo_params) { { action: Todo::DIRECTLY_ADDRESSED } }
@@ -173,7 +173,7 @@ RSpec.describe TodoService do
 
         context 'when toggling task list items' do
           before do
-            epic.update(description: "- [x] Task 1\n- [x] Task 2 FYI: #{mentions}")
+            epic.update!(description: "- [x] Task 1\n- [x] Task 2 FYI: #{mentions}")
           end
 
           it 'does not create todos' do
@@ -218,7 +218,7 @@ RSpec.describe TodoService do
 
           context 'for mentioned users' do
             before do
-              note.update(note: description_mentions)
+              note.update!(note: description_mentions)
             end
 
             let(:todo_params) { { action: Todo::MENTIONED } }
@@ -230,7 +230,7 @@ RSpec.describe TodoService do
 
           context 'for directly addressed users' do
             before do
-              note.update(note: description_directly_addressed)
+              note.update!(note: description_directly_addressed)
             end
 
             let(:todo_params) { { action: Todo::DIRECTLY_ADDRESSED } }
@@ -242,7 +242,7 @@ RSpec.describe TodoService do
 
           context 'combined' do
             before do
-              note.update(note: combined_mentions)
+              note.update!(note: combined_mentions)
             end
 
             context 'mentioned users' do
@@ -327,7 +327,7 @@ RSpec.describe TodoService do
     context 'an approver has lost access to the project', :sidekiq_inline do
       before do
         create(:approver, user: non_member, target: project)
-        project.members.find_by(user_id: non_member.id).destroy
+        project.members.find_by(user_id: non_member.id).destroy!
       end
 
       describe '#new_merge_request' do

ee/spec/services/vulnerability_feedback/create_service_spec.rb

@@ -197,7 +197,7 @@ RSpec.describe VulnerabilityFeedback::CreateService, '#execute' do
       it 'creates a new issue when feedback already exists and issue has been deleted' do
         result
 
-        expect { result[:vulnerability_feedback].issue.destroy}.to change(Issue, :count).by(-1)
+        expect { result[:vulnerability_feedback].issue.destroy! }.to change(Issue, :count).by(-1)
         expect { described_class.new(project, user, feedback_params.merge(feedback_type: 'issue')).execute }.to change(Issue, :count).by(1)
       end