account settings new approach backbone by akostadinov · Pull Request #4257 · 3scale/porta

akostadinov · 2026-03-23T21:22:44Z

This is first step in switching account settings form a huge rigid table with 64 columns to individual settings where we can extend with new settings at will (or deprecate settings). Without DDL changes.

Decided to split into multiple rounds because:

easier to review
this can be backpoerted along with THREESCALE-11928: Support Permissions-Policy header #4207 and THREESCALE- 6512: Allow custom CSP #4185 to 2.16 potentially without risking regressions while the actual conversion from the old settings table to happen in the next major release

This PR contains 2 example settings that will actually be implemented for #4207, I needed some example so I used these. But presently they only exist defined, they do nothing but demonstrate the idea.

qltysh · 2026-03-23T21:25:19Z

❌ 5 blocking issues (5 total)

Tool	Category	Rule	Count
rubocop	Lint	Move locale texts to the locale files in the `config/locales` directory.	2	❌
rubocop	Lint	Class has too many lines. [394/200]	1	❌
rubocop	Lint	Prefer `Rails\.root\.join$'path/to'$\.to\_s`.	1	❌
rubocop	Lint	`Rails\.root` is a `Pathname`, so you can use `Rails\.root\.join$'app', 'models', 'account\_setting'$`.	1	❌

jlledom · 2026-03-24T13:42:22Z

+# frozen_string_literal: true
+
+class AccountSetting < ApplicationRecord
+  self.store_full_sti_class = false


The problem with this is two classes with the same name under different scopes will evaluate as the same. We must ensure we don't repeat setting names, and ideally we declare all settings directly under AccountSetting scope

If the class with the same name is not a STI model with an AccountSetting base, that should not be an issue. And I can't imagine why one would create a STI model with the same name but at the base.

Or do you know about potential conflicts by mere existence of another class with the same name? If you know something, I assume I can test what the behavior would be.

tl;dr; despite copilot trying to tell me conflicts are easily possible, Rails has logic to correctly guess the namespace.

Ok, I did an experiment. I defined classes with same name (once before and once after AccountSetting).

class PermissionPolicyAdminPortal; end module Foo class PermissionPolicyAdminPortal; end end

Then I tried to retrieve the existing objects from the table:

[8] pry(main)> AccountSetting.all.to_a AccountSetting Load (0.7ms) SELECT `account_settings`.* FROM `account_settings` => [#<AccountSetting::PermissionPolicyAdminPortal:0x00007fc17cae52a0 id: 1, account_id: 1, type: "PermissionPolicyAdminPortal", value: {"gah"=>["pop"]}, tenant_id: nil, created_at: Tue, 24 Mar 2026 17:47:55.373108000 UTC +00:00, updated_at: Tue, 24 Mar 2026 17:47:55.373108000 UTC +00:00>]

It correctly worked. Also looking at the Rails code, it seems to be choosing the properly namespaced classes. In this case we will have:

type_name = "PermissionPolicyAdminPortal" candidates = [] AccountSetting.name.scan(/::|$/) { candidates.unshift "#{$`}::#{type_name}" } candidates << type_name

This will result in candidate constants being ["AccountSetting::PermissionPolicyAdminPortal", "PermissionPolicyAdminPortal"] so then it will always first try the correct AccountSetting::PermissionPolicyAdminPortal.

But this is not all. In case that class does not exist anymore and we resolve to the wrong PermissionPolicyAdminPortal, then it will still fail with ActiveRecord::SubclassNotFound, see test_new_with_unrelated_namespaced_type

Which approach also gives me the idea that we can get rid of

Rails.autoloaders.main.eager_load_dir(File.join(__dir__, 'account_setting')) STI_CLASSES = descendants.select...

But later we will not be able to generate methods inside the Settings wrapper class. Instead we can override #method_missing. I don't like it but will make us avoid the hack above and use the #method_missing hack instead. Not sure which is better. Let me know whether you have an opinion about this.

What if...

class AccountSetting::PermissionPolicy::AdminPortal << AccountSetting; end class AccountSetting::PermissionPolicy::DeveloperPortal << AccountSetting; end class AccountSetting::ContentSecurityPolicy::AdminPortal << AccountSetting; end class AccountSetting::ContentSecurityPolicy::DeveloperPortal << AccountSetting; end

I think it's unlikely, but I wonder how would rails STI manage that without fully qualified names.

Also, what if you removed self.store_full_sti_class = false? I guess you would just need to edit the lookup methods, it's probably better, don't you think?

Can you explain the #method_missing hack? I don't get it

Instead of iterating over the STI classes and defining methods like

define_method :permission_policy_admin_portal find_or_create(:permission_policy_admin_portal).value end ...

we can have something like:

def method_missing(method_name, *args, &block) if method_name == :permission_policy_admin_portal find_or_create(:permission_policy_admin_portal).value elif ... else super # basically raise normally method missing end

Of course it will be more elaborate adding methods like permission_policy_admin_portal=, permission_policy_admin_portal?, state machine related methods, etc. but that illustrates the basic idea I think.

About STI resolution, it's OK, if you already checked it, I'm fine.

About methods definition, it looks to me that implementing method_missing is a more standard approach than interfering with the autoloader. I think I would go for the method_missing approach, though not a strong opinion.

The class_for_setting method in the AccountSetting can be implemented by camelizing it seems.

Also I think it's good to remove all logic possible from the AccountSetting model and move all the compatibility layer to Settings class, en ensure all such logic is only there.

method_missing is a more standard approach, I hate it but it is a more "standard" approach 😿

Agreed that compatibility logic is good to be separate. The point is to agree what is compatibility logic and what is just usage helpers. I'd like to keep the mapping between class and settings name at the same place for example.

I'd like to keep the mapping between class and settings name at the same place for example.

Fine for me.

This method_missing will definitely by of the compatibility layer for example.

jlledom · 2026-03-24T13:44:56Z

+class AccountSettingYamlSettingTest < ActiveSupport::TestCase
+  def setup
+    @account = FactoryBot.create(:simple_account)
+    @example_policy = { 'camera' => ['none'], 'microphone' => ['none'] }
+  end
+
+  test 'automatically serializes and deserializes hash values' do
+    setting = AccountSetting::PermissionPolicyAdminPortal.create!(
+      account: @account,
+      value: @example_policy
+    )
+
+    # Reload to ensure it was persisted and deserialized correctly
+    setting.reload
+    assert_equal @example_policy, setting.value
+  end


This should be placed on it's own test suite, to test YamlSetting class.

Also, It' would be good to add a few tests to ensure the presence validation over value is correct. e.g. What if value is {}, (blank space), or ''?

This should be placed on it's own test suite, to test YamlSetting class.

I don't want to split into many fails at this time. It is isolated in its own class so can easily be extracted if the file grows too much.

Also, It' would be good to add a few tests to ensure the presence validation over value is correct. e.g. What if value is {}, (blank space), or ''?

This is a good point but I'm not sure what we presently want. These classes will likely have a more thorough validation logic later and it will be tested in their own classes. Lets keep it simple at this point.

I don't want to split into many fails at this time. It is isolated in its own class so can easily be extracted if the file grows too much.

What's the problem with having a test file corresponding to a class you're testing? That's the expected approach I think. What is not expected in my opinion is to place the tests for a particular class under a test suite belonging to another class.

This is a good point but I'm not sure what we presently want. These classes will likely have a more thorough validation logic later and it will be tested in their own classes. Lets keep it simple at this point.

This PR is a base, but I think the code here aims to be merged and deployed. If we want to have yaml-formatted settings, why not testing yaml related scenarios for it? like emptyness interpretation. For leaf classes like PermissionPolicyAdminPortal, the validation tests should not be about yaml syntax, it should be about proper directives, keys presence, etc.

Also... why storing CSP and permission policy values in yaml format? I used yaml in the other PRs because that's the format we use for config files, but if we want it to be a user setting, why not just store it in text format, like literally the header value in a text field, or whatever other format both Rails and the user can understand easily.

If we ask the user to use yaml, we'll need a textarea with yaml validation in the UI. Even JSON would be better since we already have a UI JSON validator.

Sorry, I'm late to the party... I'm just catching up on this PR (trying to understand 😬 ).

I actually think that YAML is a good choice for the "value" of the setting. The UI can be represented in whichever format (maybe just a simple form with labels and drop-downs, not necessarily a text field), but I think it's much more comfortable to work with hashes (key-value), that indeed can be serialized as YAML in the DB.
I don't like plain text with delimeters, because it means that we need to be extra careful with at least two characters (the one that link the key and the value, and the delimeter between key-value pairs), we need to deal with escaping them if they can be permitted within the value (which, I think, can be common if we're talking about settings for HTTP headers).

But well, in the DB it's still "text" though, right?

YAML makes no sense because the header is plain text line anyway not in YAML/JSON format.

And we do no parsing of the header also.

Ah, so the idea that each setting defines its own format of value and validation rules?
That's fine too. I was thinking about the base class. If we were to adhere to the same format throughout all subclasses, YAML would give more flexibility than plain text, IMO (because maybe some sublcasses could take advantage of a more complex value format).

It's fine then.

yes, some settings will use serialize Boolean, some can be Hash although nothing needs hash for the time being. Thank you for the review!

jlledom · 2026-03-24T13:48:11Z

+  class_attribute :default_value, instance_writer: false, default: nil
+  class_attribute :non_null, instance_writer: false, default: false


In which scenario do you foresee these two being used?

if you look at #4247, we have settings that are non-null and settings with default values. Presently we don't. But I think anyway all settings would be non-null so I will remove the non_null attribute. It doesn't make any sense to me at least not anymore.

akostadinov · 2026-03-25T18:51:35Z

@jlledom I think I applied all discussed changes. Let me know if you see something missing!

codecov · 2026-03-25T19:06:06Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.34%. Comparing base (88187e5) to head (e6c30e8).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4257      +/-   ##
==========================================
- Coverage   88.35%   87.34%   -1.01%     
==========================================
  Files        1765     1769       +4     
  Lines       44340    44359      +19     
  Branches      686      686              
==========================================
- Hits        39177    38747     -430     
- Misses       5147     5596     +449     
  Partials       16       16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jlledom · 2026-03-26T09:18:24Z

+
+  audited associated_with: :account
+
+  validates :value, presence: true, allow_blank: true


Is this legal? can we validate presence and allow blank at the same time?

Tests pass so I assume so why not? It is just not nil.

They look exactly opposite:

https://guides.rubyonrails.org/active_record_validations.html#presence

https://guides.rubyonrails.org/active_record_validations.html#allow-blank

One of them forces value to not be blank?, and the other allows the value to be blank?.

true, fixed

jlledom · 2026-03-26T17:02:48Z

  self.background_deletion = %i[
    configuration_values
+    account_settings
    settings


Do we remove settings, now that it's not a model anymore?

it is still not migrated to account_settings so is a full-fledged model!

jlledom · 2026-03-26T17:03:44Z

+
+  audited associated_with: :account
+
+  validates :value, exclusion: { in: [nil], message: "cannot be nil" }


You will laugh... but according to claude what you want is:

Suggested change

validates :value, exclusion: { in: [nil], message: "cannot be nil" }

validates :value, presence: { allow_blank: true }

It is just trolling us. With that modification:

[1] pry(main)> as = AccountSetting.new(account_id: 5, value: nil) [2] pry(main)> as.valid? => true

I like this guy's humor

mayorova · 2026-03-27T14:40:22Z

@@ -0,0 +1,3 @@
+# frozen_string_literal: true
+
+class AccountSetting::PermissionPolicyAdminPortal < AccountSetting::HttpHeaders; end


OK, maybe a nitpick, but I find these things to be very annoying.

The header is called Permissions-Policy, so I'd suggest to add that s 🙃 Also maybe even use PermissionsPolicyHeaderAdmin so it's very clear what it is about (including Portal would be nice too, but too long 😅 )

Co-authored-by: IBM Bob

akostadinov requested a review from jlledom March 23, 2026 21:22

akostadinov force-pushed the account_settings_base branch from 05c5c69 to 8b23cb9 Compare March 23, 2026 21:23

jlledom reviewed Mar 24, 2026

View reviewed changes

jlledom previously approved these changes Mar 24, 2026

View reviewed changes

akostadinov dismissed jlledom’s stale review via 915617e March 24, 2026 16:53

jlledom reviewed Mar 25, 2026

View reviewed changes

Comment thread db/migrate/20260323130000_create_account_settings.rb

akostadinov requested a review from jlledom March 25, 2026 18:51

jlledom reviewed Mar 26, 2026

View reviewed changes

jlledom previously approved these changes Mar 27, 2026

View reviewed changes

mayorova reviewed Mar 27, 2026

View reviewed changes

akostadinov dismissed jlledom’s stale review via c3fb063 March 27, 2026 20:51

akostadinov force-pushed the account_settings_base branch 2 times, most recently from c3fb063 to 500ae57 Compare March 27, 2026 20:53

AccountSetting backbone

e52de2b

Co-authored-by: IBM Bob

akostadinov force-pushed the account_settings_base branch from 500ae57 to e52de2b Compare March 27, 2026 20:54

akostadinov requested a review from jlledom March 27, 2026 20:54

jlledom approved these changes Mar 30, 2026

View reviewed changes

akostadinov merged commit 5d225bf into master Mar 30, 2026
12 of 18 checks passed

akostadinov deleted the account_settings_base branch March 30, 2026 09:31

		class_attribute :default_value, instance_writer: false, default: nil
		class_attribute :non_null, instance_writer: false, default: false


		audited associated_with: :account

		validates :value, presence: true, allow_blank: true


		audited associated_with: :account

		validates :value, exclusion: { in: [nil], message: "cannot be nil" }

	validates :value, exclusion: { in: [nil], message: "cannot be nil" }
	validates :value, presence: { allow_blank: true }

		@@ -0,0 +1,3 @@
		# frozen_string_literal: true

		class AccountSetting::PermissionPolicyAdminPortal < AccountSetting::HttpHeaders; end

Conversation

akostadinov commented Mar 23, 2026

Uh oh!

qltysh Bot commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlledom Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akostadinov commented Mar 25, 2026

Uh oh!

codecov Bot commented Mar 25, 2026

Codecov Report

Uh oh!

Uh oh!

jlledom Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qltysh Bot commented Mar 23, 2026 •

edited

Loading

jlledom Mar 25, 2026 •

edited

Loading

jlledom Mar 26, 2026 •

edited

Loading