Merge branch 'reset-jenkins-password-when-username-is-blank' into 'master'

Reset Jenkins password if username was left blank

So we not only reset password if URL was changed, but also for username. Also, still require username if a new password was set.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/22430

See merge request !775

app/models/project_services/jenkins_service.rb

@@ -5,7 +5,7 @@ class JenkinsService < CiService
   before_update :reset_password
   validates :username,
             presence: true,
-            if: ->(service) { service.activated? && service.password.present? }
+            if: ->(service) { service.activated? && service.password_touched? }
 
   default_value_for :push_events, true
   default_value_for :merge_requests_events, false
@@ -15,7 +15,7 @@ class JenkinsService < CiService
 
   def reset_password
     # don't reset the password if a new one is provided
-    if jenkins_url_changed? && !password_touched?
+    if (jenkins_url_changed? || username.blank?) && !password_touched?
       self.password = nil
     end
   end

app/views/shared/_field.html.haml

@@ -13,15 +13,16 @@
   - else
     = form.label name, title, class: "control-label"
   .col-sm-10
-    - if type == 'text'
+    - case type
+    - when 'text'
       = form.text_field name, class: "form-control", placeholder: placeholder
-    - elsif type == 'textarea'
+    - when 'textarea'
       = form.text_area name, rows: 5, class: "form-control", placeholder: placeholder
-    - elsif type == 'checkbox'
+    - when 'checkbox'
       = form.check_box name
-    - elsif type == 'select'
+    - when 'select'
       = form.select name, options_for_select(choices, value ? value : default_choice), {}, { class: "form-control" }
-    - elsif type == 'password'
+    - when 'password'
       = form.password_field name, autocomplete: "new-password", class: 'form-control'
     - if help
       %span.help-block= help

spec/models/project_services/jenkins_service_spec.rb

@@ -5,7 +5,9 @@ describe JenkinsService do
     it { is_expected.to belong_to :project }
     it { is_expected.to have_one :service_hook }
   end
+
   let(:project) { create(:project) }
+
   let(:jenkins_params) do
     {
       active: true,
@@ -25,19 +27,36 @@ describe JenkinsService do
         properties: {
           jenkins_url: 'http://jenkins.example.com/',
           password: 'password',
-          username: nil,
+          username: 'username',
         }
       )
     end
+
     subject { @jenkins_service }
 
     context 'when the service is active' do
       let(:active) { true }
+
+      context 'when password was not touched' do
+        before do
+          allow(subject).to receive(:password_touched?).and_return(false)
+        end
+
+        it { is_expected.not_to validate_presence_of :username }
+      end
+
+      context 'when password was touched' do
+        before do
+          allow(subject).to receive(:password_touched?).and_return(true)
+        end
+
         it { is_expected.to validate_presence_of :username }
       end
+    end
 
     context 'when the service is inactive' do
       let(:active) { false }
+
       it { is_expected.not_to validate_presence_of :username }
     end
   end
@@ -111,12 +130,18 @@ describe JenkinsService do
         )
       end
 
-      it 'reset password if url changed' do
+      it 'resets password if url changed' do
         @jenkins_service.jenkins_url = 'http://jenkins-edited.example.com/'
         @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
+        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