Improve invalidation of stored service password if the endpoint URL is changed

It now allows to specify a password at the same time as the new URL, and works
on the service template admin pages.

3 files changed, 28 insertions, 8 deletions

diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb
index 5f5255ab487..4a18c772e50 100644
--- a/app/models/project_services/bamboo_service.rb
+++ b/app/models/project_services/bamboo_service.rb
@@ -48,7 +48,7 @@ class BambooService < CiService
   end
 
   def reset_password
-    if prop_updated?(:bamboo_url)
+    if prop_modified?(:bamboo_url) && !prop_updated?(:password)
       self.password = nil
     end
   end
diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb
index fb11cad352e..a548a557dfe 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)
+    if prop_modified?(:teamcity_url) && !prop_updated?(:password)
       self.password = nil
     end
   end
diff --git a/app/models/service.rb b/app/models/service.rb
index 7e845d565b1..946ea1a096b 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -33,6 +33,8 @@ class Service < ActiveRecord::Base
 
   after_initialize :initialize_properties
 
+  after_commit :reset_updated_properties
+
   belongs_to :project
   has_one :service_hook
 
@@ -103,6 +105,7 @@ class Service < ActiveRecord::Base
 
   # Provide convenient accessor methods
   # for each serialized property.
+  # Also keep track of updated properties.
   def self.prop_accessor(*args)
     args.each do |arg|
       class_eval %{
@@ -111,19 +114,36 @@ class Service < ActiveRecord::Base
         end
 
         def #{arg}=(value)
+          updated_properties['#{arg}'] = #{arg} unless updated_properties.include?('#{arg}') 
           self.properties['#{arg}'] = value
         end
       }
     end
   end
 
-  # 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.
+  # Returns a hash of the properties that have been assigned a new value since last save,
+  # indicating their original values (attr => original value).
+  # ActiveRecord does not provide a mechanism to track changes in serialized keys, 
+  # so we need a specific implementation for service properties.
+  # This allows to track changes to properties set with the accessor methods,
+  # but not direct manipulation of properties hash.
+  def updated_properties
+    @updated_properties ||= ActiveSupport::HashWithIndifferentAccess.new
+  end
+
   def prop_updated?(prop_name)
-    relation_name = self.type.underscore
-    previous_value = project.send(relation_name).send(prop_name)
-    return false if previous_value.nil?
-    previous_value != send(prop_name)
+    # Check if a new value was set.
+    # The new value may or may not be the same as the old value
+    updated_properties.include?(prop_name)
+  end
+
+  def prop_modified?(prop_name)
+    # Check if new value was set and it is different from the old value
+    prop_updated?(prop_name) && updated_properties[prop_name] != send(prop_name)
+  end
+
+  def reset_updated_properties
+    @updated_properties = nil
   end
 
   def async_execute(data)