diff --git a/app/controllers/admin/services_controller.rb b/app/controllers/admin/services_controller.rb
index a62170662e12c34070374c69f6396a616b82aeeb..461335883325e2c7b33b29c83114ce6ff880e5db 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 3047ee8a1ff02db6b068541daa467334b074e436..129068ef0192ed4c0e95d01c91237d212df75cd2 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 5f5255ab48727398a285a8d2247b222189de587a..4a18c772e5087af0375eb0d9ef0a96c3e0319041 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 fb11cad352ed014e3ac818a6bb2cbbf3c24d04ac..a548a557dfe8af6309139ddae8cb8fc9c47ca447 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 7e845d565b1c4c7cdcc712d89621b49d2f37885a..946ea1a096b2715d6ea568651ab3cb042be6d894 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)
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index da87ea5b84f175ce7835d6708021cf03558bf10d..d42b96294bab526ef6f056551bb5df456c5a806b 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -116,14 +116,47 @@ describe Service do
)
end
- it "returns false" do
+ it "returns false when the property has not been assigned a new value" do
service.username = "key_changed"
expect(service.prop_updated?(:bamboo_url)).to be_falsy
end
- it "returns true" do
- service.bamboo_url = "http://other.com"
+ it "returns true when the property has been assigned a different value" do
+ service.bamboo_url = "http://example.com"
expect(service.prop_updated?(:bamboo_url)).to be_truthy
end
+
+ it "returns true when the property has been re-assigned the same value" do
+ service.bamboo_url = 'http://gitlab.com'
+ expect(service.prop_updated?(:bamboo_url)).to be_truthy
+ end
+ end
+
+ describe "#prop_modified?" do
+ let(:service) do
+ BambooService.create(
+ project: create(:project),
+ properties: {
+ bamboo_url: 'http://gitlab.com',
+ username: 'mic',
+ password: "password"
+ }
+ )
+ end
+
+ it "returns false when the property has not been assigned a new value" do
+ service.username = "key_changed"
+ expect(service.prop_modified?(:bamboo_url)).to be_falsy
+ end
+
+ it "returns true when the property has been assigned a different value" do
+ service.bamboo_url = "http://example.com"
+ expect(service.prop_modified?(:bamboo_url)).to be_truthy
+ end
+
+ it "returns false when the property has been re-assigned the same value" do
+ service.bamboo_url = 'http://gitlab.com'
+ expect(service.prop_modified?(:bamboo_url)).to be_falsy
+ end
end
end