diff options
| -rw-r--r-- | app/controllers/admin/services_controller.rb | 8 | ||||
| -rw-r--r-- | app/controllers/projects/services_controller.rb | 8 | ||||
| -rw-r--r-- | app/models/project_services/bamboo_service.rb | 2 | ||||
| -rw-r--r-- | app/models/project_services/teamcity_service.rb | 2 | ||||
| -rw-r--r-- | app/models/service.rb | 32 | ||||
| -rw-r--r-- | spec/models/service_spec.rb | 39 |
6 files changed, 13 insertions, 78 deletions
diff --git a/app/controllers/admin/services_controller.rb b/app/controllers/admin/services_controller.rb index 46133588332..a62170662e1 100644 --- a/app/controllers/admin/services_controller.rb +++ b/app/controllers/admin/services_controller.rb @@ -39,13 +39,7 @@ class Admin::ServicesController < Admin::ApplicationController end def application_services_params - application_services_params = params.permit(:id, + 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 129068ef019..3047ee8a1ff 100644 --- a/app/controllers/projects/services_controller.rb +++ b/app/controllers/projects/services_controller.rb @@ -9,10 +9,6 @@ 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] @@ -63,9 +59,7 @@ class Projects::ServicesController < Projects::ApplicationController def service_params service_params = params.require(:service).permit(ALLOWED_PARAMS) - FILTER_BLANK_PARAMS.each do |param| - service_params.delete(param) if service_params[param].blank? - end + service_params.delete("password") if service_params["password"].blank? service_params end end diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb index 4a18c772e50..5f5255ab487 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_modified?(:bamboo_url) && !prop_updated?(:password) + if prop_updated?(:bamboo_url) self.password = nil end end diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb index a548a557dfe..fb11cad352e 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_modified?(:teamcity_url) && !prop_updated?(:password) + if prop_updated?(:teamcity_url) self.password = nil end end diff --git a/app/models/service.rb b/app/models/service.rb index 946ea1a096b..7e845d565b1 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -33,8 +33,6 @@ class Service < ActiveRecord::Base after_initialize :initialize_properties - after_commit :reset_updated_properties - belongs_to :project has_one :service_hook @@ -105,7 +103,6 @@ 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 %{ @@ -114,36 +111,19 @@ class Service < ActiveRecord::Base end def #{arg}=(value) - updated_properties['#{arg}'] = #{arg} unless updated_properties.include?('#{arg}') self.properties['#{arg}'] = value end } end end - # 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 - + # 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) - # 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 + 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) end def async_execute(data) diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index d42b96294ba..da87ea5b84f 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -116,47 +116,14 @@ describe Service do ) end - it "returns false when the property has not been assigned a new value" do + it "returns false" do service.username = "key_changed" expect(service.prop_updated?(: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" + it "returns true" do + service.bamboo_url = "http://other.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 |