Merge pull request #3818 from 3scale/settings-api-validations

Settings API validations

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,13 +101,25 @@ def spam_protection_level
     level == :auto ? :captcha : level
   end
 
-  protected
-
   delegate :provider_id_for_audits, :to => :account, :allow_nil => true
 
   private
 
   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

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
 

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)