[WIP] sane account settings by akostadinov · Pull Request #4247 · 3scale/porta

akostadinov · 2026-03-05T16:32:23Z

No description provided.

qltysh · 2026-03-05T16:34:57Z

❌ 97 blocking issues (100 total)

Tool	Category	Rule	Count
reek	Lint	Settings tests 'record' at least 6 times	6	❌
rubocop	Lint	Avoid using `update\_attribute` because it skips validations.	6	❌
rubocop	Style	Add empty line after guard clause.	5	❌
reek	Lint	Settings#save calls 'results.all?' 2 times	22	❌
reek	Lint	Settings#save has approx 9 statements	2	❌
reek	Lint	Settings has missing safe method 'toggle!'	2	❌
reek	Lint	Settings#assign_attributes manually dispatches method call	2	❌
rubocop	Lint	Rename `has\_privacy\_policy?` to `privacy\_policy?`.	2	❌
rubocop	Lint	Use `privacy\_policy\.present?` instead of `\!privacy\_policy\.blank?`.	2	❌
rubocop	Performance	Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant.	2	❌
rubocop	Lint	Class has too many lines. [400/200]	2	❌
rubocop	Lint	Block has too many lines. [33/25]	2	❌
reek	Lint	Settings#spam_protection_level refers to 'val' more than self (maybe move it to another class?)	14	❌
reek	Lint	Admin::Api::SettingsController#settings_params has the variable name 'v'	10	❌
rubocop	Style	Incorrect formatting, autoformat by running `qlty fmt`.	1	❌
reek	Lint	Settings has at least 32 methods	1	❌
reek	Lint	Settings#account is a writable attribute	1	❌
rubocop	Lint	Prefer `index\_by` over `each\_with\_object`.	1	❌
rubocop	Style	Use `to\_h \{ \.\.\. \}` instead of `each\_with\_object`.	1	❌
rubocop	Lint	Cyclomatic complexity for `save` is too high. [8/7]	1	❌
rubocop	Lint	Perceived complexity for `save` is too high. [9/8]	1	❌
rubocop	Lint	Predicate method names should end with `?`.	1	❌
rubocop	Style	Provide an exception class and message as arguments to `raise`.	1	❌
rubocop	Style	Indent the first argument one step more than `ActiveRecord::RecordInvalid\.new\(`.	1	❌
reek	Lint	Settings#errors contains iterators nested 2 deep	1	❌
rubocop	Style	Use `attr\_writer` to define trivial writer methods.	1	❌
rubocop	Style	Prefer `to\_s` over string interpolation.	1	❌
rubocop	Lint	Memoized variable `@settings\_facade` does not match method name `settings`. Use `@settings` instead.	1	❌
brakeman	Vulnerability	Potentially dangerous attribute available for mass assignment.	1	❌
reek	Lint	AccountSetting::BooleanSetting#self.serialize is controlled by argument 'value'	1	❌

@qltysh one-click actions:

Auto-fix formatting (qlty fmt && git push)

jlledom

After you reacted with a vomiting emoji to my CSP headers PR, I wanted my revenge. This was my moment, my intention was to be specially cruel here and destroy this PR and your self esteem. Regrettably, I actually like the PR. The only thing I dislike is having to create tens of empty classes. I left a few comments.

Also, the usage rules screen fails with the error:

undefined method `model_name' for class Settings

jlledom · 2026-03-12T15:35:48Z

    return unless settings_params.key?(:enforce_sso)

-    return unless Settings.type_for_attribute('enforce_sso').cast(settings_params[:enforce_sso])
+    settings.enforce_sso = settings_params[:enforce_sso]


We don't need to alter the in-memory the account settings here, we only need to cast the incoming value to evaluate it. We can use a new Setings instance for that.

We need a new account object to get a working new settings instance. We can create a new EnforceSSO object and do something similar to the original version. But I think this approach reads much nicer.

Do you see any risks? I think claude originally used the casting approach and I found it too ugly.

Mmm, I don't know, I don't like changing a value in the settings model, just for doing a check, just before updating that very model. I guess nothing can happen, but still. Uglyness is relative, on the other hand. IMO is better to use an approach that doesn't write on the settings model.

jlledom · 2026-03-12T15:58:14Z

+
+  validates :value, inclusion: { in: VALID_VALUES }
+
+  state_machine :value, initial: 'denied' do


I can't access the developer portal settings because of an undefined method:

NoMethodError (undefined method `multiple_applications_visible?' for an instance of Settings): config/abilities/buyer_any.rb:38:in `block in <main>'

This state machine is not defining the method.

wait, idea was to look at settings.rb and account_settings/* sorry, if I was not clear. I think that something broke by last iterations. Not ready for thoorough review. Just need to know if you see something bad on the hight level 😬

jlledom · 2026-03-12T16:04:40Z

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


wtf is this 🤔

jlledom · 2026-03-12T16:40:31Z

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class AccountSetting::StringSetting < AccountSetting


This one could be a subclass of TextSetting adding just the validation, right?

jlledom · 2026-03-12T16:53:20Z

+    switch_settings.each do |col|
+      type_name = col.to_s.camelize
+      execute <<~SQL.squish
+        INSERT INTO account_settings (account_id, type, value, tenant_id, created_at, updated_at)
+        SELECT account_id,
+               '#{type_name}',
+               #{col},
+               tenant_id,
+               '#{now}',
+               '#{now}'
+        FROM settings
+        WHERE #{col} IS NOT NULL AND #{col} != ''
+      SQL


The code is the same for text, string and switch settings, no need to repeat the code three times.

jlledom · 2026-03-12T16:54:00Z

+      end
+
+      add_index :account_settings, [:account_id, :type], unique: true, name: 'index_account_settings_on_account_id_and_type'
+      add_index :account_settings, :account_id, name: 'index_account_settings_on_account_id'


This index is not needed because it's already included in the index above.

jlledom · 2026-03-12T16:54:35Z

 # It's strongly recommended that you check this file into your version control system.

-ActiveRecord::Schema[7.1].define(version: 2025_05_22_195407) do
+ActiveRecord::Schema[7.1].define(version: 2026_02_28_120000) do


Fine, but we need schemas also for postgres and oracle.

jlledom · 2026-03-12T16:58:46Z

@@ -267,20 +267,22 @@ def setup
  end

  test 'settings be created lazily for existing account' do


The behavior changes, the test name isn't valid anymore.

jlledom · 2026-03-12T17:00:09Z

+# Ref: Rails Guides “Contributing to Ruby on Rails → Create an Executable Test Case”
+#      and typical AR issue examples that use this exact style. 
+
+begin


This doesn't seem should be part of the PR

jlledom

These are a few points that claude raised and I consider legit.

jlledom · 2026-03-13T08:32:24Z

  has_one :profile, dependent: :delete
-  has_one :settings, dependent: :destroy, inverse_of: :account, autosave: true
-  lazy_initialization_for :profile, :settings, if: :should_not_be_deleted?
+  has_many :account_settings, dependent: :delete_all, inverse_of: :account, autosave: true


:delete_all skips callbacks, so audit logs won't be recorded. If we want to record logs for individual destruction of each setting when deleting the whole account, we should use :destroy, otherwise, it's fine.

jlledom · 2026-03-13T08:49:01Z

+  def transition_to(target)
+    target = target.to_s
+    return if value == target
+
+    case target
+    when 'denied'
+      deny!
+    when 'hidden'
+      value == 'denied' ? allow! : hide!
+    when 'visible'
+      allow! if value == 'denied'
+      show!
+    end
+  end
+
+  def typed_assign(raw_value)
+    transition_to(raw_value)
+  end


This implementation persists immediately instead of storing the changes in-memory and persist them when calling save. Is it intended? Could this cause any problem?

This is a very good point. I need to investigate it.

jlledom · 2026-04-23T11:58:37Z

This can be closed, right?

akostadinov · 2026-04-23T13:26:09Z

No, I'm waiting for the pending header PRs to be merged as well the protected_attribute ones. I will then need to rebase.

I don't want to compete with them as well better try them out before introducing bigger changes.

initial implementation

865376b

akostadinov added 8 commits March 6, 2026 21:36

a little simplification

5b042d8

less AR

41c2ea9

lazy loading

3c82793

remove more AR methods from settings

1799b0b

remove more logic from Settings

425e189

remove more AR emulation

6669bbe

further move logic to AR and unify settings access

895b812

store_full_sti_class = false and small improvements

12945a3

jlledom reviewed Mar 12, 2026

View reviewed changes

jlledom reviewed Mar 13, 2026

View reviewed changes

akostadinov added 10 commits March 13, 2026 23:44

address comments

25f3318

some lost state machine methods

488b46d

reduce Switches module

abb87dc

remove setting names/values hardcoding in hashes

01ab5b2

filter empty values at the correct abstraction layer

02bb030

rename and sanitize THREESCALE_VISIBLE_SWITCHES

8d8d0f5

remove switch raw value accessors

9e2bfe0

sanitize some method calls

3ada1ed

sanitize another method call

6fa671f

a number of fixes

6061dff

akostadinov mentioned this pull request Mar 24, 2026

account settings new approach backbone #4257

Merged


		validates :value, inclusion: { in: VALID_VALUES }

		state_machine :value, initial: 'denied' do

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

		class AccountSetting::JanrainRelyingParty < AccountSetting::StringSetting; end

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

		class AccountSetting::StringSetting < AccountSetting

		@@ -267,20 +267,22 @@ def setup
		end

		test 'settings be created lazily for existing account' do

Conversation

akostadinov commented Mar 5, 2026

Uh oh!

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

Uh oh!

Uh oh!

jlledom left a comment • 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!

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 left a comment

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!

jlledom commented Apr 23, 2026

Uh oh!

akostadinov commented Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qltysh Bot commented Mar 5, 2026 •

edited

Loading

jlledom left a comment •

edited

Loading