Merge branch 'fix/improve_reset_service_password_v2' into 'master'

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

New version of !1583, using the same failproof logic but this time mirroring the name and behaviour of the change-tracking methods of ActiveModel::Dirty in order to make it clearer and more natural.

Added more tests to clarify the expected behaviour.

This is an alternative to !1594

/cc @vsizov @rspeicher

See merge request !1600

5 files changed, 44 insertions, 11 deletions

diff --git a/app/controllers/admin/services_controller.rb b/app/controllers/admin/services_controller.rb
index a62170662e1..46133588332 100644
--- a/app/controllers/admin/services_controller.rb
+++ b/app/controllers/admin/services_controller.rb
@@ -39,7 +39,13 @@ class Admin::ServicesController < Admin::ApplicationController
   end
 
   def application_services_params
-    params.permit(:id,
+    application_services_params = params.permit(:id,
       service: Projects::ServicesController::ALLOWED_PARAMS)
+    if application_services_params[:service].is_a?(Hash)
+      Projects::ServicesController::FILTER_BLANK_PARAMS.each do |param|
+        application_services_params[:service].delete(param) if application_services_params[:service][param].blank? 
+      end
+    end
+    application_services_params
   end
 end
diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index 3047ee8a1ff..129068ef019 100644
--- a/app/controllers/projects/services_controller.rb
+++ b/app/controllers/projects/services_controller.rb
@@ -9,6 +9,10 @@ class Projects::ServicesController < Projects::ApplicationController
                     :note_events, :send_from_committer_email, :disable_diffs, :external_wiki_url,
                     :notify, :color,
                     :server_host, :server_port, :default_irc_uri, :enable_ssl_verification]
+
+  # Parameters to ignore if no value is specified
+  FILTER_BLANK_PARAMS = [:password]
+  
   # Authorize
   before_action :authorize_admin_project!
   before_action :service, only: [:edit, :update, :test]
@@ -59,7 +63,9 @@ class Projects::ServicesController < Projects::ApplicationController
 
   def service_params
     service_params = params.require(:service).permit(ALLOWED_PARAMS)
-    service_params.delete("password") if service_params["password"].blank?
+    FILTER_BLANK_PARAMS.each do |param|
+      service_params.delete(param) if service_params[param].blank?
+    end
     service_params
   end
 end
diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb
index 5f5255ab487..d31b12f539e 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 bamboo_url_changed? && !password_touched?
       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..0b022461250 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 teamcity_url_changed? && !password_touched?
       self.password = nil
     end
   end
diff --git a/app/models/service.rb b/app/models/service.rb
index 7e845d565b1..d610abd1683 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 in a similar way as ActiveModel::Dirty
   def self.prop_accessor(*args)
     args.each do |arg|
       class_eval %{
@@ -111,21 +114,39 @@ class Service < ActiveRecord::Base
         end
 
         def #{arg}=(value)
+          updated_properties['#{arg}'] = #{arg} unless #{arg}_changed?
           self.properties['#{arg}'] = value
         end
+
+        def #{arg}_changed?
+          #{arg}_touched? && #{arg} != #{arg}_was
+        end
+
+        def #{arg}_touched?
+          updated_properties.include?('#{arg}')
+        end
+
+        def #{arg}_was
+          updated_properties['#{arg}']
+        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.
-  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)
+  # 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 reset_updated_properties
+    @updated_properties = nil
+  end
+  
   def async_execute(data)
     return unless supported_events.include?(data[:object_kind])