diff --git a/app/models/settings.rb b/app/models/settings.rb index 951975401d..9ede2b9913 100644 --- a/app/models/settings.rb +++ b/app/models/settings.rb @@ -6,8 +6,8 @@ class Settings < ApplicationRecord attr_protected :account_id, :tenant_id, :product, :audit_ids, :sso_key - validates :product, inclusion: { in: %w(connect enterprise).freeze } - validates :change_account_plan_permission, inclusion: { in: %w(request none credit_card request_credit_card direct).freeze } + validates :product, inclusion: { in: %w[connect enterprise].freeze } + validates :change_account_plan_permission, :change_service_plan_permission, inclusion: { in: %w[request none credit_card request_credit_card direct].freeze } validates :bg_colour, :link_colour, :text_colour, :menu_bg_colour, :link_label, :link_url, :menu_link_colour, :token_api, :content_bg_colour, :tracker_code, :favicon, :plans_tab_bg_colour, :plans_bg_colour, :content_border_colour, :cc_privacy_path, :cc_terms_path, :cc_refunds_path, :change_service_plan_permission, :spam_protection_level, @@ -27,6 +27,10 @@ def self.columns super.reject {|column| /\Aheroku_(id|name)|log_requests_switch\Z/ =~ column.name } end + def self.non_null_columns_names + columns.select { |column| !column.null }.map(&:name) + end + def approval_required_editable? not_custom_account_plans.size == 1 end @@ -35,14 +39,14 @@ def approval_required_disabled? not_custom_account_plans.size > 1 && account_plans_ui_visible? end - def update(attributes) - if approval_required_editable? - value = attributes.delete(:account_approval_required) || false - account_plan = provider.account_plans.default || not_custom_account_plans.first! - account_plan.update_attribute(:approval_required, value) - end + def assign_attributes(attrs, options = {}) + super(sanitize_attributes(attrs), options) + end + + def update(attrs) + update_approval_required(attrs) if approval_required_editable? - super(attributes) + super(attrs) end def set_forum_enabled @@ -52,8 +56,7 @@ def set_forum_enabled end def account_approval_required - account_plan = provider.account_plans.default || not_custom_account_plans.first! - @account_approval_required = account_plan.approval_required + @account_approval_required = default_account_plan.approval_required end def account_approval_required=(value) @@ -98,8 +101,6 @@ def spam_protection_level level == :auto ? :captcha : level end - protected - delegate :provider_id_for_audits, :to => :account, :allow_nil => true private @@ -107,4 +108,18 @@ def spam_protection_level def not_custom_account_plans @not_custom_account_plans ||= provider.account_plans.not_custom end + + def default_account_plan + provider.account_plans.default || not_custom_account_plans.first! + end + + def update_approval_required(attrs) + value = attrs.delete(:account_approval_required).presence || false + default_account_plan.update_attribute(:approval_required, value) + end + + # Remove attributes with empty strings and nil for non-null columns + def sanitize_attributes(attrs) + attrs.reject { |key, value| self.class.non_null_columns_names.include?(key.to_s) && value.to_s.empty? } + end end diff --git a/test/integration/admin/api/settings_controller_test.rb b/test/integration/admin/api/settings_controller_test.rb index 8dbdf486cd..71dde25753 100644 --- a/test/integration/admin/api/settings_controller_test.rb +++ b/test/integration/admin/api/settings_controller_test.rb @@ -32,19 +32,26 @@ def setup end test 'update' do - params = { access_token: token, signups_enabled: false, change_account_plan_permission: 'invalid' } - assert 'request', settings.change_account_plan_permission + params = { access_token: token, signups_enabled: false, change_account_plan_permission: 'invalid', change_service_plan_permission: 'invalid'} + assert_equal 'request', settings.change_account_plan_permission assert settings.signups_enabled put admin_api_settings_path(format: :json), params: params assert_response 422 + errors = JSON.parse(response.body)['errors'] + assert_equal ['is not included in the list'], errors['change_account_plan_permission'] + assert_equal ['is not included in the list'], errors['change_service_plan_permission'] + params['change_account_plan_permission'] = 'direct' + params['change_service_plan_permission'] = 'none' put admin_api_settings_path(format: :json), params: params assert_response :success - assert 'direct', settings.reload.change_account_plan_permission + settings.reload + assert_equal 'direct', settings.change_account_plan_permission + assert_equal 'none', settings.change_service_plan_permission assert_not settings.signups_enabled end diff --git a/test/unit/settings_test.rb b/test/unit/settings_test.rb index 24cf37c067..88262a27fb 100644 --- a/test/unit/settings_test.rb +++ b/test/unit/settings_test.rb @@ -7,6 +7,8 @@ def setup @settings = @provider.settings end + attr_reader :settings + def test_hide_basic_switches Rails.configuration.three_scale.stubs(:hide_basic_switches).returns(true) assert Settings.hide_basic_switches? @@ -103,6 +105,14 @@ def test_update refute @provider.account_plans.first.approval_required end + test "account_approval_required defaults to 'false' on empty values" do + settings.update(account_approval_required: true) + assert settings.account_approval_required + + settings.update(account_approval_required: "") + assert_not settings.account_approval_required + end + def test_service_plans_visible_ui_switch assert @settings.has_attribute?(:service_plans_switch) assert @settings.has_attribute?(:service_plans_ui_visible) @@ -169,6 +179,32 @@ def test_require_cc_on_signup_visible_ui_switch_on_rolling_updates assert settings.monthly_billing_enabled end + test 'empty values are skipped for non-null columns' do + settings.update(public_search: true) + assert settings.reload.public_search + + settings.update(public_search: "") + assert_not settings.previous_changes[:public_search] + assert settings.reload.public_search + + settings.update(public_search: nil) + assert_not settings.previous_changes[:public_search] + assert settings.reload.public_search + + settings.update(public_search: "false") + assert settings.previous_changes[:public_search] + assert_not settings.reload.public_search + end + + test "validate change plan permission values" do + assert_equal 'request', settings.change_account_plan_permission + assert_equal 'request', settings.change_service_plan_permission + + settings.update(change_account_plan_permission: 'invalid', change_service_plan_permission: 'invalid') + assert settings.errors.of_kind? :change_account_plan_permission, "is not included in the list" + assert settings.errors.of_kind? :change_service_plan_permission, "is not included in the list" + end + class FinanceDisabledSwitchTest < ActiveSupport::TestCase def setup @provider = FactoryBot.build_stubbed(:simple_provider)