Merge branch 'issue#220040-fix-rails-savebang-users-model' into 'master'

Fix Rails/SaveBang Rubocop offenses for user related models

See merge request gitlab-org/gitlab!57901

.rubocop_manual_todo.yml

@@ -219,9 +219,6 @@ Rails/SaveBang:
     - 'spec/models/service_spec.rb'
     - 'spec/models/snippet_spec.rb'
     - 'spec/models/upload_spec.rb'
-    - 'spec/models/user_preference_spec.rb'
-    - 'spec/models/user_spec.rb'
-    - 'spec/models/user_status_spec.rb'
 
 Rails/TimeZone:
   Enabled: true

changelogs/unreleased/issue-220040-fix-rails-savebang-users-model.yml

+---
+title: Fix Rails/SaveBang Rubocop offenses for user related models
+merge_request: 57901
+author: Huzaifa Iftikhar @huzaifaiftikhar
+type: other

spec/models/user_preference_spec.rb

@@ -61,7 +61,7 @@ RSpec.describe UserPreference do
   describe 'sort_by preferences' do
     shared_examples_for 'a sort_by preference' do
       it 'allows nil sort fields' do
-        user_preference.update(attribute => nil)
+        user_preference.update!(attribute => nil)
 
         expect(user_preference).to be_valid
       end

spec/models/user_spec.rb

@@ -136,7 +136,7 @@ RSpec.describe User do
       it 'creates `user_detail` when `bio` is first updated' do
         user = create(:user)
 
-        expect { user.update(bio: 'my bio') }.to change { user.user_detail.persisted? }.from(false).to(true)
+        expect { user.update!(bio: 'my bio') }.to change { user.user_detail.persisted? }.from(false).to(true)
       end
     end
 
@@ -659,9 +659,10 @@ RSpec.describe User do
         it 'does not accept not verified emails' do
           email = create(:email)
           user = email.user
-          user.update(notification_email: email.email)
+          user.notification_email = email.email
 
           expect(user).to be_invalid
+          expect(user.errors[:notification_email]).to include('is not an email you own')
         end
       end
 
@@ -669,7 +670,7 @@ RSpec.describe User do
         it 'accepts verified emails' do
           email = create(:email, :confirmed, email: 'test@test.com')
           user = email.user
-          user.update(public_email: email.email)
+          user.notification_email = email.email
 
           expect(user).to be_valid
         end
@@ -677,9 +678,10 @@ RSpec.describe User do
         it 'does not accept not verified emails' do
           email = create(:email)
           user = email.user
-          user.update(public_email: email.email)
+          user.public_email = email.email
 
           expect(user).to be_invalid
+          expect(user.errors[:public_email]).to include('is not an email you own')
         end
       end
 
@@ -1293,7 +1295,7 @@ RSpec.describe User do
           let(:secondary) { create(:email, :confirmed, email: 'secondary@example.com', user: user) }
 
           before do
-            user.emails.create(email_attrs)
+            user.emails.create!(email_attrs)
             user.tap { |u| u.update!(notification_email: email_attrs[:email]) }.reload
           end
 
@@ -1447,7 +1449,7 @@ RSpec.describe User do
     let!(:accessible_deploy_keys_project) { create(:deploy_keys_project, project: project) }
 
     before do
-      public_deploy_keys_project.deploy_key.update(public: true)
+      public_deploy_keys_project.deploy_key.update!(public: true)
       project.add_developer(user)
     end
 
@@ -1537,13 +1539,13 @@ RSpec.describe User do
       it 'receives callback when external changes' do
         expect(user).to receive(:ensure_user_rights_and_limits)
 
-        user.update(external: false)
+        user.update!(external: false)
       end
 
       it 'ensures correct rights and limits for user' do
         stub_config_setting(default_can_create_group: true)
 
-        expect { user.update(external: false) }.to change { user.can_create_group }.to(true)
+        expect { user.update!(external: false) }.to change { user.can_create_group }.to(true)
           .and change { user.projects_limit }.to(Gitlab::CurrentSettings.default_projects_limit)
       end
     end
@@ -1554,11 +1556,11 @@ RSpec.describe User do
       it 'receives callback when external changes' do
         expect(user).to receive(:ensure_user_rights_and_limits)
 
-        user.update(external: true)
+        user.update!(external: true)
       end
 
       it 'ensures correct rights and limits for user' do
-        expect { user.update(external: true) }.to change { user.can_create_group }.to(false)
+        expect { user.update!(external: true) }.to change { user.can_create_group }.to(false)
           .and change { user.projects_limit }.to(0)
       end
     end
@@ -2454,7 +2456,7 @@ RSpec.describe User do
     end
 
     context 'with a redirect route matching the given path' do
-      let!(:redirect_route) { user.namespace.redirect_routes.create(path: 'foo') }
+      let!(:redirect_route) { user.namespace.redirect_routes.create!(path: 'foo') }
 
       context 'without the follow_redirects option' do
         it 'returns nil' do
@@ -2555,7 +2557,7 @@ RSpec.describe User do
 
       expect(Gitlab::AvatarCache).to receive(:delete_by_email).with(*user.verified_emails)
 
-      user.update(avatar: fixture_file_upload('spec/fixtures/dk.png'))
+      user.update!(avatar: fixture_file_upload('spec/fixtures/dk.png'))
     end
   end
 
@@ -2869,12 +2871,12 @@ RSpec.describe User do
       expect(user.starred?(project1)).to be_truthy
       expect(user.starred?(project2)).to be_truthy
 
-      star1.destroy
+      star1.destroy!
 
       expect(user.starred?(project1)).to be_falsey
       expect(user.starred?(project2)).to be_truthy
 
-      star2.destroy
+      star2.destroy!
 
       expect(user.starred?(project1)).to be_falsey
       expect(user.starred?(project2)).to be_falsey
@@ -3424,7 +3426,7 @@ RSpec.describe User do
 
       expect(user.authorized_projects).to include(project)
 
-      member.destroy
+      member.destroy!
 
       expect(user.authorized_projects).not_to include(project)
     end
@@ -3449,7 +3451,7 @@ RSpec.describe User do
 
       expect(user2.authorized_projects).to include(project)
 
-      project.destroy
+      project.destroy!
 
       expect(user2.authorized_projects).not_to include(project)
     end
@@ -3463,7 +3465,7 @@ RSpec.describe User do
 
       expect(user.authorized_projects).to include(project)
 
-      group.destroy
+      group.destroy!
 
       expect(user.authorized_projects).not_to include(project)
     end
@@ -4463,9 +4465,10 @@ RSpec.describe User do
             end
 
             it 'adds the namespace errors to the user' do
-              user.update(username: new_username)
+              user.username = new_username
 
-              expect(user.errors.full_messages.first).to eq('A user, alias, or group already exists with that username.')
+              expect(user).to be_invalid
+              expect(user.errors[:base]).to include('A user, alias, or group already exists with that username.')
             end
           end
         end
@@ -5356,21 +5359,21 @@ RSpec.describe User do
 
         with_them do
           context 'when state was changed' do
-            subject { user.update(attributes) }
+            subject { user.update!(attributes) }
 
             include_examples 'update highest role with exclusive lease'
           end
         end
 
         context 'when state was not changed' do
-          subject { user.update(email: 'newmail@example.com') }
+          subject { user.update!(email: 'newmail@example.com') }
 
           include_examples 'does not update the highest role'
         end
       end
 
       describe 'destroy user' do
-        subject { user.destroy }
+        subject { user.destroy! }
 
         include_examples 'does not update the highest role'
       end
@@ -5392,7 +5395,7 @@ RSpec.describe User do
 
     context 'when user is a ghost user' do
       before do
-        user.update(user_type: :ghost)
+        user.update!(user_type: :ghost)
       end
 
       it { is_expected.to be false }
@@ -5410,7 +5413,7 @@ RSpec.describe User do
 
       with_them do
         before do
-          user.update(user_type: user_type)
+          user.update!(user_type: user_type)
         end
 
         it { is_expected.to be expected_result }
@@ -5433,7 +5436,7 @@ RSpec.describe User do
 
     context 'when user is an internal user' do
       before do
-        user.update(user_type: :ghost)
+        user.update!(user_type: :ghost)
       end
 
       it { is_expected.to be :forbidden }
@@ -5467,7 +5470,7 @@ RSpec.describe User do
 
     context 'when user is an internal user' do
       before do
-        user.update(user_type: 'alert_bot')
+        user.update!(user_type: 'alert_bot')
       end
 
       it_behaves_like 'does not require password to be present'
@@ -5475,7 +5478,7 @@ RSpec.describe User do
 
     context 'when user is a project bot user' do
       before do
-        user.update(user_type: 'project_bot')
+        user.update!(user_type: 'project_bot')
       end
 
       it_behaves_like 'does not require password to be present'

spec/models/user_status_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe UserStatus do
   it 'is expected to be deleted when the user is deleted' do
     status = create(:user_status)
 
-    expect { status.user.destroy }.to change { described_class.count }.from(1).to(0)
+    expect { status.user.destroy! }.to change { described_class.count }.from(1).to(0)
   end
 
   describe '#clear_status_after=' do