Merge branch 'rails-save-bang-21' into 'master'

Fix Rails/SaveBang offenses for */spec/models/project_services/*

See merge request gitlab-org/gitlab!41320

.rubocop_todo.yml

@@ -729,8 +729,6 @@ Rails/SaveBang:
     - 'ee/spec/models/merge_train_spec.rb'
     - 'spec/models/packages/package_spec.rb'
     - 'ee/spec/models/project_ci_cd_setting_spec.rb'
-    - 'ee/spec/models/project_services/github_service_spec.rb'
-    - 'ee/spec/models/project_services/jenkins_service_spec.rb'
     - 'ee/spec/models/project_spec.rb'
     - 'ee/spec/models/protected_environment_spec.rb'
     - 'ee/spec/models/repository_spec.rb'
@@ -1113,12 +1111,6 @@ Rails/SaveBang:
     - 'spec/models/pages_domain_spec.rb'
     - 'spec/models/project_auto_devops_spec.rb'
     - 'spec/models/project_feature_spec.rb'
-    - 'spec/models/project_services/bamboo_service_spec.rb'
-    - 'spec/models/project_services/buildkite_service_spec.rb'
-    - 'spec/models/project_services/jira_service_spec.rb'
-    - 'spec/models/project_services/packagist_service_spec.rb'
-    - 'spec/models/project_services/pipelines_email_service_spec.rb'
-    - 'spec/models/project_services/teamcity_service_spec.rb'
     - 'spec/models/project_spec.rb'
     - 'spec/models/project_team_spec.rb'
     - 'spec/models/protectable_dropdown_spec.rb'

changelogs/unreleased/rails-save-bang-21.yml

+---
+title: Fix Rails/SaveBang offenses for *spec/models/project_services*
+merge_request: 41320
+author: Rajendra Kadam
+type: fixed

ee/spec/models/project_services/github_service_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe GithubService do
     }
   end
 
-  subject { described_class.create(service_params) }
+  subject { described_class.create!(service_params) }
 
   before do
     stub_licensed_features(github_project_service_integration: true)
@@ -38,7 +38,7 @@ RSpec.describe GithubService do
 
       describe '#valid?' do
         it 'is not valid' do
-          expect(subject).not_to be_valid
+          expect(described_class.new(service_params)).not_to be_valid
         end
       end
     end
@@ -80,7 +80,7 @@ RSpec.describe GithubService do
     let(:properties) { subject.reload.properties.symbolize_keys }
 
     it 'does not overwrite existing integrations' do
-      subject.update(service_params.slice(:properties))
+      subject.update!(service_params.slice(:properties))
 
       expect(properties).to match(service_params[:properties])
       expect(subject.static_context).to be_nil

ee/spec/models/project_services/jenkins_service_spec.rb

@@ -31,13 +31,14 @@ RSpec.describe JenkinsService do
 
   describe 'username validation' do
     before do
-      @jenkins_service = described_class.create(
+      @jenkins_service = described_class.create!(
         active: active,
         project: project,
         properties: {
           jenkins_url: 'http://jenkins.example.com/',
           password: 'password',
-          username: 'username'
+          username: 'username',
+          project_name: 'my_project'
         }
       )
     end
@@ -68,7 +69,7 @@ RSpec.describe JenkinsService do
           expect(subject).not_to validate_presence_of :username
 
           subject.password = ''
-          subject.save
+          subject.save!
         end
       end
     end
@@ -137,7 +138,7 @@ RSpec.describe JenkinsService do
       user = create(:user, username: 'username')
       project = create(:project, name: 'project')
       push_sample_data = Gitlab::DataBuilder::Push.build_sample(project, user)
-      jenkins_service = described_class.create(jenkins_params)
+      jenkins_service = described_class.create!(jenkins_params)
       stub_request(:post, jenkins_hook_url).with(headers: { 'Authorization' => jenkins_authorization })
 
       result = jenkins_service.test(push_sample_data)
@@ -167,7 +168,7 @@ RSpec.describe JenkinsService do
     let(:namespace) { create(:group, :private) }
     let(:project) { create(:project, :private, name: 'project', namespace: namespace) }
     let(:push_sample_data) { Gitlab::DataBuilder::Push.build_sample(project, user) }
-    let(:jenkins_service) { described_class.create(jenkins_params) }
+    let(:jenkins_service) { described_class.create!(jenkins_params) }
 
     before do
       stub_request(:post, jenkins_hook_url)
@@ -232,7 +233,7 @@ RSpec.describe JenkinsService do
 
     context 'when a password was previously set' do
       before do
-        @jenkins_service = described_class.create(
+        @jenkins_service = described_class.create!(
           project: project,
           properties: {
             jenkins_url: 'http://jenkins.example.com/',
@@ -244,26 +245,26 @@ RSpec.describe JenkinsService do
 
       it 'resets password if url changed' do
         @jenkins_service.jenkins_url = 'http://jenkins-edited.example.com/'
-        @jenkins_service.save
+        @jenkins_service.save!
         expect(@jenkins_service.password).to be_nil
       end
 
       it 'resets password if username is blank' do
         @jenkins_service.username = ''
-        @jenkins_service.save
+        @jenkins_service.save!
         expect(@jenkins_service.password).to be_nil
       end
 
       it 'does not reset password if username changed' do
         @jenkins_service.username = 'some_name'
-        @jenkins_service.save
+        @jenkins_service.save!
         expect(@jenkins_service.password).to eq('password')
       end
 
       it 'does not reset password if new url is set together with password, even if it\'s the same password' do
         @jenkins_service.jenkins_url = 'http://jenkins_edited.example.com/'
         @jenkins_service.password = 'password'
-        @jenkins_service.save
+        @jenkins_service.save!
         expect(@jenkins_service.password).to eq('password')
         expect(@jenkins_service.jenkins_url).to eq('http://jenkins_edited.example.com/')
       end
@@ -271,14 +272,14 @@ RSpec.describe JenkinsService do
       it 'resets password if url changed, even if setter called multiple times' do
         @jenkins_service.jenkins_url = 'http://jenkins1.example.com/'
         @jenkins_service.jenkins_url = 'http://jenkins1.example.com/'
-        @jenkins_service.save
+        @jenkins_service.save!
         expect(@jenkins_service.password).to be_nil
       end
     end
 
     context 'when no password was previously set' do
       before do
-        @jenkins_service = described_class.create(
+        @jenkins_service = described_class.create!(
           project: create(:project),
           properties: {
             jenkins_url: 'http://jenkins.example.com/',
@@ -290,7 +291,7 @@ RSpec.describe JenkinsService do
       it 'saves password if new url is set together with password' do
         @jenkins_service.jenkins_url = 'http://jenkins_edited.example.com/'
         @jenkins_service.password = 'password'
-        @jenkins_service.save
+        @jenkins_service.save!
         expect(@jenkins_service.password).to eq('password')
         expect(@jenkins_service.jenkins_url).to eq('http://jenkins_edited.example.com/')
       end

spec/models/project_services/bamboo_service_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe BambooService, :use_clean_rails_memory_store_caching do
   let_it_be(:project) { create(:project) }
 
   subject(:service) do
-    described_class.create(
+    described_class.create!(
       project: project,
       properties: {
         bamboo_url: bamboo_url,
@@ -85,7 +85,7 @@ RSpec.describe BambooService, :use_clean_rails_memory_store_caching do
           bamboo_service = service
 
           bamboo_service.bamboo_url = 'http://gitlab1.com'
-          bamboo_service.save
+          bamboo_service.save!
 
           expect(bamboo_service.password).to be_nil
         end
@@ -94,7 +94,7 @@ RSpec.describe BambooService, :use_clean_rails_memory_store_caching do
           bamboo_service = service
 
           bamboo_service.username = 'some_name'
-          bamboo_service.save
+          bamboo_service.save!
 
           expect(bamboo_service.password).to eq('password')
         end
@@ -104,7 +104,7 @@ RSpec.describe BambooService, :use_clean_rails_memory_store_caching do
 
           bamboo_service.bamboo_url = 'http://gitlab_edited.com'
           bamboo_service.password = 'password'
-          bamboo_service.save
+          bamboo_service.save!
 
           expect(bamboo_service.password).to eq('password')
           expect(bamboo_service.bamboo_url).to eq('http://gitlab_edited.com')
@@ -117,7 +117,7 @@ RSpec.describe BambooService, :use_clean_rails_memory_store_caching do
 
         bamboo_service.bamboo_url = 'http://gitlab_edited.com'
         bamboo_service.password = 'password'
-        bamboo_service.save
+        bamboo_service.save!
 
         expect(bamboo_service.password).to eq('password')
         expect(bamboo_service.bamboo_url).to eq('http://gitlab_edited.com')

spec/models/project_services/buildkite_service_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe BuildkiteService, :use_clean_rails_memory_store_caching do
   let(:project) { create(:project) }
 
   subject(:service) do
-    described_class.create(
+    described_class.create!(
       project: project,
       properties: {
         service_hook: true,

spec/models/project_services/jira_service_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe JiraService do
       }
     end
 
-    let(:service) { described_class.create(options) }
+    let(:service) { described_class.create!(options) }
 
     it 'sets the URL properly' do
       # jira-ruby gem parses the URI and handles trailing slashes fine:
@@ -102,7 +102,7 @@ RSpec.describe JiraService do
       }
     end
 
-    subject { described_class.create(params) }
+    subject { described_class.create!(params) }
 
     it 'does not store data into properties' do
       expect(subject.properties).to be_nil
@@ -189,7 +189,7 @@ RSpec.describe JiraService do
           let_it_be(:new_url) { 'http://jira-new.example.com' }
 
           before do
-            service.update(username: new_username, url: new_url)
+            service.update!(username: new_username, url: new_url)
           end
 
           it 'leaves properties field emtpy' do
@@ -209,12 +209,12 @@ RSpec.describe JiraService do
 
         context 'when updating the url, api_url, username, or password' do
           it 'updates deployment type' do
-            service.update(url: 'http://first.url')
-            service.jira_tracker_data.update(deployment_type: 'server')
+            service.update!(url: 'http://first.url')
+            service.jira_tracker_data.update!(deployment_type: 'server')
 
             expect(service.jira_tracker_data.deployment_server?).to be_truthy
 
-            service.update(api_url: 'http://another.url')
+            service.update!(api_url: 'http://another.url')
             service.jira_tracker_data.reload
 
             expect(service.jira_tracker_data.deployment_cloud?).to be_truthy
@@ -222,25 +222,25 @@ RSpec.describe JiraService do
           end
 
           it 'calls serverInfo for url' do
-            service.update(url: 'http://first.url')
+            service.update!(url: 'http://first.url')
 
             expect(WebMock).to have_requested(:get, /serverInfo/)
           end
 
           it 'calls serverInfo for api_url' do
-            service.update(api_url: 'http://another.url')
+            service.update!(api_url: 'http://another.url')
 
             expect(WebMock).to have_requested(:get, /serverInfo/)
           end
 
           it 'calls serverInfo for username' do
-            service.update(username: 'test-user')
+            service.update!(username: 'test-user')
 
             expect(WebMock).to have_requested(:get, /serverInfo/)
           end
 
           it 'calls serverInfo for password' do
-            service.update(password: 'test-password')
+            service.update!(password: 'test-password')
 
             expect(WebMock).to have_requested(:get, /serverInfo/)
           end
@@ -248,7 +248,7 @@ RSpec.describe JiraService do
 
         context 'when not updating the url, api_url, username, or password' do
           it 'does not update deployment type' do
-            service.update(jira_issue_transition_id: 'jira_issue_transition_id')
+            expect {service.update!(jira_issue_transition_id: 'jira_issue_transition_id')}.to raise_error(ActiveRecord::RecordInvalid)
 
             expect(WebMock).not_to have_requested(:get, /serverInfo/)
           end
@@ -268,7 +268,7 @@ RSpec.describe JiraService do
               it 'resets password if url changed' do
                 service
                 service.url = 'http://jira_edited.example.com'
-                service.save
+                service.save!
 
                 expect(service.reload.url).to eq('http://jira_edited.example.com')
                 expect(service.password).to be_nil
@@ -276,7 +276,7 @@ RSpec.describe JiraService do
 
               it 'does not reset password if url "changed" to the same url as before' do
                 service.url = 'http://jira.example.com'
-                service.save
+                service.save!
 
                 expect(service.reload.url).to eq('http://jira.example.com')
                 expect(service.password).not_to be_nil
@@ -284,7 +284,7 @@ RSpec.describe JiraService do
 
               it 'resets password if url not changed but api url added' do
                 service.api_url = 'http://jira_edited.example.com/rest/api/2'
-                service.save
+                service.save!
 
                 expect(service.reload.api_url).to eq('http://jira_edited.example.com/rest/api/2')
                 expect(service.password).to be_nil
@@ -293,7 +293,7 @@ RSpec.describe JiraService do
               it 'does not reset password if new url is set together with password, even if it\'s the same password' do
                 service.url = 'http://jira_edited.example.com'
                 service.password = password
-                service.save
+                service.save!
 
                 expect(service.password).to eq(password)
                 expect(service.url).to eq('http://jira_edited.example.com')
@@ -302,14 +302,14 @@ RSpec.describe JiraService do
               it 'resets password if url changed, even if setter called multiple times' do
                 service.url = 'http://jira1.example.com/rest/api/2'
                 service.url = 'http://jira1.example.com/rest/api/2'
-                service.save
+                service.save!
 
                 expect(service.password).to be_nil
               end
 
               it 'does not reset password if username changed' do
                 service.username = 'some_name'
-                service.save
+                service.save!
 
                 expect(service.reload.password).to eq(password)
               end
@@ -317,7 +317,7 @@ RSpec.describe JiraService do
               it 'does not reset password if password changed' do
                 service.url = 'http://jira_edited.example.com'
                 service.password = 'new_password'
-                service.save
+                service.save!
 
                 expect(service.reload.password).to eq('new_password')
               end
@@ -325,7 +325,7 @@ RSpec.describe JiraService do
               it 'does not reset password if the password is touched and same as before' do
                 service.url = 'http://jira_edited.example.com'
                 service.password = password
-                service.save
+                service.save!
 
                 expect(service.reload.password).to eq(password)
               end
@@ -342,20 +342,20 @@ RSpec.describe JiraService do
 
               it 'resets password if api url changed' do
                 service.api_url = 'http://jira_edited.example.com/rest/api/2'
-                service.save
+                service.save!
 
                 expect(service.password).to be_nil
               end
 
               it 'does not reset password if url changed' do
                 service.url = 'http://jira_edited.example.com'
-                service.save
+                service.save!
 
                 expect(service.password).to eq(password)
               end
 
               it 'resets password if api url set to empty' do
-                service.update(api_url: '')
+                service.update!(api_url: '')
 
                 expect(service.reload.password).to be_nil
               end
@@ -372,7 +372,7 @@ RSpec.describe JiraService do
             it 'saves password if new url is set together with password' do
               service.url = 'http://jira_edited.example.com/rest/api/2'
               service.password = 'password'
-              service.save
+              service.save!
               expect(service.reload.password).to eq('password')
               expect(service.reload.url).to eq('http://jira_edited.example.com/rest/api/2')
             end
@@ -441,7 +441,7 @@ RSpec.describe JiraService do
         allow_any_instance_of(JIRA::Resource::Issue).to receive(:key).and_return('JIRA-123')
         allow(JIRA::Resource::Remotelink).to receive(:all).and_return([])
 
-        @jira_service.save
+        @jira_service.save!
 
         project_issues_url = 'http://jira.example.com/rest/api/2/issue/JIRA-123'
         @transitions_url   = 'http://jira.example.com/rest/api/2/issue/JIRA-123/transitions'
@@ -709,9 +709,11 @@ RSpec.describe JiraService do
 
   describe '#test' do
     let(:server_info_results) { { 'url' => 'http://url', 'deploymentType' => 'Cloud' } }
+    let_it_be(:project) { create(:project, :repository) }
     let(:jira_service) do
       described_class.new(
         url: url,
+        project: project,
         username: username,
         password: password
       )
@@ -728,7 +730,7 @@ RSpec.describe JiraService do
       end
 
       it 'gets Jira project with API URL if set' do
-        jira_service.update(api_url: 'http://jira.api.com')
+        jira_service.update!(api_url: 'http://jira.api.com')
 
         expect(server_info).to eq(success: true, result: server_info_results)
         expect(WebMock).to have_requested(:get, /jira.api.com/)

spec/models/project_services/packagist_service_spec.rb

@@ -33,7 +33,7 @@ RSpec.describe PackagistService do
     let(:user)    { create(:user) }
     let(:project) { create(:project, :repository) }
     let(:push_sample_data) { Gitlab::DataBuilder::Push.build_sample(project, user) }
-    let(:packagist_service) { described_class.create(packagist_params) }
+    let(:packagist_service) { described_class.create!(packagist_params) }
 
     before do
       stub_request(:post, packagist_hook_url)

spec/models/project_services/pipelines_email_service_spec.rb

@@ -81,7 +81,7 @@ RSpec.describe PipelinesEmailService, :mailer do
     context 'when pipeline is succeeded' do
       before do
         data[:object_attributes][:status] = 'success'
-        pipeline.update(status: 'success')
+        pipeline.update!(status: 'success')
       end
 
       it_behaves_like 'sending email'
@@ -91,7 +91,7 @@ RSpec.describe PipelinesEmailService, :mailer do
       context 'on default branch' do
         before do
           data[:object_attributes][:ref] = project.default_branch
-          pipeline.update(ref: project.default_branch)
+          pipeline.update!(ref: project.default_branch)
         end
 
         context 'notifications are enabled only for default branch' do
@@ -115,7 +115,7 @@ RSpec.describe PipelinesEmailService, :mailer do
         before do
           create(:protected_branch, project: project, name: 'a-protected-branch')
           data[:object_attributes][:ref] = 'a-protected-branch'
-          pipeline.update(ref: 'a-protected-branch')
+          pipeline.update!(ref: 'a-protected-branch')
         end
 
         context 'notifications are enabled only for default branch' do
@@ -138,7 +138,7 @@ RSpec.describe PipelinesEmailService, :mailer do
       context 'on a neither protected nor default branch' do
         before do
           data[:object_attributes][:ref] = 'a-random-branch'
-          pipeline.update(ref: 'a-random-branch')
+          pipeline.update!(ref: 'a-random-branch')
         end
 
         context 'notifications are enabled only for default branch' do
@@ -177,7 +177,7 @@ RSpec.describe PipelinesEmailService, :mailer do
       context 'with succeeded pipeline' do
         before do
           data[:object_attributes][:status] = 'success'
-          pipeline.update(status: 'success')
+          pipeline.update!(status: 'success')
         end
 
         it_behaves_like 'not sending email'
@@ -195,7 +195,7 @@ RSpec.describe PipelinesEmailService, :mailer do
         context 'with succeeded pipeline' do
           before do
             data[:object_attributes][:status] = 'success'
-            pipeline.update(status: 'success')
+            pipeline.update!(status: 'success')
           end
 
           it_behaves_like 'not sending email'
@@ -206,7 +206,7 @@ RSpec.describe PipelinesEmailService, :mailer do
         context 'on default branch' do
           before do
             data[:object_attributes][:ref] = project.default_branch
-            pipeline.update(ref: project.default_branch)
+            pipeline.update!(ref: project.default_branch)
           end
 
           context 'notifications are enabled only for default branch' do
@@ -230,7 +230,7 @@ RSpec.describe PipelinesEmailService, :mailer do
           before do
             create(:protected_branch, project: project, name: 'a-protected-branch')
             data[:object_attributes][:ref] = 'a-protected-branch'
-            pipeline.update(ref: 'a-protected-branch')
+            pipeline.update!(ref: 'a-protected-branch')
           end
 
           context 'notifications are enabled only for default branch' do
@@ -253,7 +253,7 @@ RSpec.describe PipelinesEmailService, :mailer do
         context 'on a neither protected nor default branch' do
           before do
             data[:object_attributes][:ref] = 'a-random-branch'
-            pipeline.update(ref: 'a-random-branch')
+            pipeline.update!(ref: 'a-random-branch')
           end
 
           context 'notifications are enabled only for default branch' do
@@ -281,7 +281,7 @@ RSpec.describe PipelinesEmailService, :mailer do
       context 'with failed pipeline' do
         before do
           data[:object_attributes][:status] = 'failed'
-          pipeline.update(status: 'failed')
+          pipeline.update!(status: 'failed')
         end
 
         it_behaves_like 'not sending email'
@@ -295,7 +295,7 @@ RSpec.describe PipelinesEmailService, :mailer do
       context 'with failed pipeline' do
         before do
           data[:object_attributes][:status] = 'failed'
-          pipeline.update(status: 'failed')
+          pipeline.update!(status: 'failed')
         end
 
         it_behaves_like 'sending email'

spec/models/project_services/teamcity_service_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe TeamcityService, :use_clean_rails_memory_store_caching do
   let(:project) { create(:project) }
 
   subject(:service) do
-    described_class.create(
+    described_class.create!(
       project: project,
       properties: {
         teamcity_url: teamcity_url,
@@ -85,7 +85,7 @@ RSpec.describe TeamcityService, :use_clean_rails_memory_store_caching do
           teamcity_service = service
 
           teamcity_service.teamcity_url = 'http://gitlab1.com'
-          teamcity_service.save
+          teamcity_service.save!
 
           expect(teamcity_service.password).to be_nil
         end
@@ -94,7 +94,7 @@ RSpec.describe TeamcityService, :use_clean_rails_memory_store_caching do
           teamcity_service = service
 
           teamcity_service.username = 'some_name'
-          teamcity_service.save
+          teamcity_service.save!
 
           expect(teamcity_service.password).to eq('password')
         end
@@ -104,7 +104,7 @@ RSpec.describe TeamcityService, :use_clean_rails_memory_store_caching do
 
           teamcity_service.teamcity_url = 'http://gitlab_edited.com'
           teamcity_service.password = 'password'
-          teamcity_service.save
+          teamcity_service.save!
 
           expect(teamcity_service.password).to eq('password')
           expect(teamcity_service.teamcity_url).to eq('http://gitlab_edited.com')
@@ -117,7 +117,7 @@ RSpec.describe TeamcityService, :use_clean_rails_memory_store_caching do
 
         teamcity_service.teamcity_url = 'http://gitlab_edited.com'
         teamcity_service.password = 'password'
-        teamcity_service.save
+        teamcity_service.save!
 
         expect(teamcity_service.password).to eq('password')
         expect(teamcity_service.teamcity_url).to eq('http://gitlab_edited.com')