Skip to content

fix(accounts): subtype dropped on create when assigned before accountable_type#2432

Open
vlnd0 wants to merge 1 commit into
we-promise:mainfrom
vlnd0:fix/account-subtype-permit-order
Open

fix(accounts): subtype dropped on create when assigned before accountable_type#2432
vlnd0 wants to merge 1 commit into
we-promise:mainfrom
vlnd0:fix/account-subtype-permit-order

Conversation

@vlnd0

@vlnd0 vlnd0 commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Problem

Creating an account and choosing a subtype (e.g. a Cash/Depository account set to Savings) silently drops the subtype on create — the account renders with the type's fallback label (Depository.display_name → "Cash") instead of the chosen "Savings". Editing an existing account works; only create is affected.

Cause

#2356 made Account#subtype= build the accountable so a top-level subtype survives create:

def subtype=(value)
  self.accountable = accountable_class.new if accountable.nil? && accountable_type.present?
  accountable&.subtype = value
end

This assumes accountable_type is already set when subtype= runs. The real controller path violates that assumption:

  • account_params permits :name, :balance, :subtype, :currency, :accountable_type, …:subtype before :accountable_type.
  • ActionController::Parameters#permit builds its result by iterating the filter list in order, so the permitted hash carries subtype before accountable_type.
  • Mass-assignment therefore calls subtype= while accountable_type (and thus accountable_class) is still blank → the build is skipped → accountable&.subtype= is a no-op → subtype dropped.

The existing regression test set accountable_type first, so it exercised the working order and never caught this.

Fix

Make the writer order-independent. When subtype arrives before the type is known, stash it and apply it from an accountable_type= override once the type — and therefore accountable_class — is available.

def subtype=(value)
  self.accountable = accountable_class.new if accountable.nil? && accountable_type.present?
  if accountable
    accountable.subtype = value
  else
    @deferred_subtype = value
  end
end

def accountable_type=(value)
  super
  if defined?(@deferred_subtype)
    pending = @deferred_subtype
    remove_instance_variable(:@deferred_subtype)
    self.subtype = pending
  end
end

Tests

Added regression coverage for:

  • subtype assigned before accountable_type (the permit order).
  • create_and_sync with attributes in permit order.

Both fail on main and pass with this change. Existing subtype tests still pass.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed account subtype assignment to work correctly regardless of the order attributes are set, improving reliability of account configuration.

…_type

we-promise#2356 build the accountable inside Account#subtype= so a top-level subtype
survives create. That fix assumes accountable_type is already set when
subtype= runs, but the real controller path violates it: strong-params
permit preserves filter order, and account_params lists :subtype before
:accountable_type. So on create subtype= runs while accountable_type (and
accountable_class) is still blank, the build is skipped, and the chosen
subtype is silently dropped — the account renders with the type's fallback
label (e.g. a Depository shows 'Cash' instead of 'Savings').

The existing regression test only covered the accountable_type-first order,
so it never caught this.

Make the writer order-independent: when subtype arrives before the type,
stash it and apply it from an accountable_type= override once the type is
known. Add regression tests for the permit order and for create_and_sync.
@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cd704f98-c2ad-4269-a405-f6ec0a92afdb

📥 Commits

Reviewing files that changed from the base of the PR and between fdcd0c7 and f3dee90.

📒 Files selected for processing (2)
  • app/models/account.rb
  • test/models/account_test.rb

📝 Walkthrough

Walkthrough

Account#subtype= is updated to defer assignment into @deferred_subtype when accountable_type is not yet set. A new Account#accountable_type= override calls super and then flushes any deferred subtype by re-invoking subtype=. Two tests cover both the isolated assignment-order scenario and the create_and_sync path.

Changes

Deferred subtype assignment in Account model

Layer / File(s) Summary
Deferred subtype= and accountable_type= writers
app/models/account.rb
subtype= now builds and updates accountable when accountable_type is already set, or stores the value in @deferred_subtype. A new accountable_type= override calls super, then drains @deferred_subtype by re-invoking self.subtype=.
Tests for assignment-order subtype persistence
test/models/account_test.rb
Two new tests assert that subtype set before accountable_type is not lost on accountable construction, and that create_and_sync with subtype ahead of accountable_type in the attribute hash persists the value on both Account and accountable.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • we-promise/sure#2356: Directly modifies the same Account#subtype= method in app/models/account.rb to prevent subtype loss when accountable is not yet built, and adds overlapping account_test.rb coverage for subtype assignment during creation.

Poem

🐰 A subtype came early, before its type friend,
So I tucked it away 'round a cozy bend.
When accountable_type finally arrived,
The deferred little value had quietly survived.
No attribute lost, no silent nil returned—
A rabbit keeps every subtype that's earned! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main bug fix: that subtype attributes were being dropped during account creation when assigned before accountable_type. This accurately reflects the primary change in the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@vlnd0

vlnd0 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@jjmata if all ok, i suggest to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant