address commentsupdate_passwd_service

3 files changed, 43 insertions, 5 deletions

diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb
index 3ff02ee75e2..1665c74cd7f 100644
--- a/app/models/project_services/teamcity_service.rb
+++ b/app/models/project_services/teamcity_service.rb
@@ -45,7 +45,7 @@ class TeamcityService < CiService
   end
 
   def reset_password
-    if prop_updated?(:teamcity_url) && !prop_updated?(:password)
+    if prop_updated?(:teamcity_url) && !password_touched?
       self.password = nil
     end
   end
diff --git a/app/models/service.rb b/app/models/service.rb
index af5ce1635a6..eaf1eaca146 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -125,8 +125,9 @@ class Service < ActiveRecord::Base
   # ActiveRecord does not provide a mechanism to track changes in serialized keys.
   # This is why we need to perform extra query to do it mannually.
   def prop_updated?(prop_name)
-    return false if send("#{prop_name}_was").nil?
-    send("#{prop_name}_was") != send(prop_name)
+    value_was = send("#{prop_name}_was")
+    return false if value_was.nil?
+    value_was != send(prop_name)
   end
 
   def async_execute(data)
diff --git a/spec/models/project_services/teamcity_service_spec.rb b/spec/models/project_services/teamcity_service_spec.rb
index fb5ebbda75e..d6c4e2df4ee 100644
--- a/spec/models/project_services/teamcity_service_spec.rb
+++ b/spec/models/project_services/teamcity_service_spec.rb
@@ -41,18 +41,19 @@ describe TeamcityService, models: true do
       )
     end
 
-    it "reset password if url changed" do
+    it "reset password if url is changed" do
       @teamcity_service.teamcity_url = 'http://gitlab1.com'
       @teamcity_service.save
       expect(@teamcity_service.password).to be_nil
     end
 
-    it "does not reset password if username changed" do
+    it "does not reset password if username is changed" do
       @teamcity_service.username = "some_name"
       @teamcity_service.save
       expect(@teamcity_service.password).to eq("password")
     end
 
+
     it "does not reset password if new url is set together with password" do
       @teamcity_service.teamcity_url = 'http://gitlab_edited.com'
       @teamcity_service.password = '123'
@@ -60,5 +61,41 @@ describe TeamcityService, models: true do
       expect(@teamcity_service.password).to eq("123")
       expect(@teamcity_service.teamcity_url).to eq("http://gitlab_edited.com")
     end
+
+    it "does not reset password if new url is set together with password, even if it's the same password" do
+      @teamcity_service.teamcity_url = 'http://gitlab_edited.com'
+      @teamcity_service.password = 'password'
+      @teamcity_service.save
+      expect(@teamcity_service.password).to eq("password")
+      expect(@teamcity_service.teamcity_url).to eq("http://gitlab_edited.com")
+    end
+
+
+    context "when no password was set before" do
+      before do
+        @teamcity_service = TeamcityService.create(
+          project: create(:project),
+          properties: {
+            teamcity_url: 'http://gitlab.com',
+            username: 'mic'
+          }
+        )
+      end
+
+      it "saves password if new url is set together with password" do
+        @teamcity_service.teamcity_url = 'http://gitlab_edited.com'
+        @teamcity_service.password = 'password'
+        @teamcity_service.save
+        expect(@teamcity_service.password).to eq("password")
+        expect(@teamcity_service.teamcity_url).to eq("http://gitlab_edited.com")
+      end
+    end
+
+    it "resets password if url is changed, even if setter called multiple times" do
+      @teamcity_service.teamcity_url = 'http://gitlab1.com'
+      @teamcity_service.teamcity_url = 'http://gitlab1.com'
+      @teamcity_service.save
+      expect(@teamcity_service.password).to be_nil
+    end
   end
 end