feat(statements): add account statement vault#1753
Conversation
Add web-only statement uploads, account linking, duplicate detection, and per-account coverage/reconciliation checks without mutating transactions. Extend ActiveStorage authorization and targeted tests for family/account scoping.
Preserve linked statement records when an account is deleted by moving them back to the unmatched inbox, then expand coverage for upload validation, sanitized parser metadata, unavailable reconciliation, and missing-month coverage.
Address review and security findings in the statement vault by preserving sanitized parser metadata, failing closed on orphaned statement blobs, avoiding account_id mass assignment permits, and adding regression coverage for link/delete edge cases.
Prioritize SHA-256 duplicate detection while preserving MD5 fallback for legacy rows. Remove free-form account notes from statement matching, document direct account-destroy unlinking, and add year-selectable historical coverage with muted out-of-range months.
Clarify legacy MD5 checksum use, whitelist statement balance helper dispatch, and preserve sanitized parser metadata. Hide statement management controls from read-only viewers while keeping server-side authorization unchanged.
Allow the changelog provider lookup in the self-hosting settings system test, include Statement Vault in settings navigation coverage, and align the feature title casing. Update the devcontainer so ActiveStorage and parallel system tests can run in the documented environment.
Place Statement Vault with account settings instead of between Imports and Exports. Keep settings footer ordering and system navigation coverage aligned, including the non-admin visibility guard.
Resolve CodeRabbit review feedback for statement upload validation, duplicate race handling, account statement matching semantics, metadata detection, ActiveStorage authorization tests, and small UI/style cleanups.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an account statements feature: uploads with metadata detection, deduplication, account suggestion, reconciliation coverage, UI (index/show/account tab), authorization for blob serving, DB schema/migration, helpers/locales, devcontainer tweaks, and extensive tests. ChangesAccount Statements Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant App as RailsApp
participant DB as Database
participant AS as ActiveStorage
User->>Browser: upload statement files (multipart)
Browser->>App: POST /account_statements create
App->>App: prepare_upload!, compute checksums, detect content type
App->>AS: attach original file
App->>App: MetadataDetector -> AccountMatcher
App->>DB: persist AccountStatement row (sanitized_parser_output, checksums, enums)
App-->>Browser: redirect with flash (created/duplicates/errors)
Browser->>App: GET blob/download
App->>App: authorize_protected_attachment (dispatch by record_type)
App->>AS: serve authorized blob URL or deny
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
brin-security-scanner
Bot
added
contributor:verified
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f020a4d546
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.devcontainer/Dockerfile (1)
6-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify why libvips42 is being added in the statement vault PR.
The comment correctly notes that libvips42 supports ActiveStorage profile image variants (used in the User model), but AccountStatement's
original_fileattachment doesn't use image variants—it only stores PDF/CSV/XLSX documents. This makes the comment's relevance to this statement-focused PR unclear.Either update the comment to explain the broader infrastructure context (e.g., "libvips42 is required by image_processing for profile image variants and may be useful for future features"), or confirm this is an opportunistic dependency addition unrelated to the account statement vault feature.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.devcontainer/Dockerfile around lines 6 - 15, Update the Dockerfile comment about libvips42 to clarify intent: mention that libvips42 is required by image_processing/ruby-vips for ActiveStorage profile image variants (e.g., User avatar processing) and either state that this is added opportunistically for future image processing needs beyond the AccountStatement original_file (which stores PDF/CSV/XLSX) or explicitly note it was added solely to support other features; reference libvips42, image_processing/ruby-vips, ActiveStorage, and AccountStatement.original_file in the comment so reviewers understand the broader infrastructure rationale.
🧹 Nitpick comments (3)
app/models/account_statement.rb (3)
253-263: 💤 Low value
unlink!rolls back when the recomputed suggestion fails validation.If
AccountMatcher#best_matchreturns an account that failssuggested_account_belongs_to_family(e.g., cross-family due to a matcher bug), the entireunlink!operation fails and the statement stays linked. This is asserted as intentional in the test at lines 493-517, but the UX consequence is that a matcher regression silently blocks all unlinks. Consider defensively nullifying the suggestion when invalid instead of failing the unlink:♻️ Optional defensive variation
def unlink! transaction do update!( account: nil, review_status: :unmatched, match_confidence: nil ) assign_account_match - save! + unless save + self.suggested_account = nil + self.match_confidence = nil + save! + end end end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/account_statement.rb` around lines 253 - 263, The unlink! transaction currently updates the statement then calls assign_account_match which uses AccountMatcher#best_match and may set a suggested account that later fails suggested_account_belongs_to_family, causing the whole transaction to roll back; change unlink! (or adjust assign_account_match) so that after computing the suggestion you validate it with suggested_account_belongs_to_family and, if invalid, set suggested_account (and match_confidence) to nil instead of allowing an exception/validation failure to bubble up — ensure you reference unlink!, assign_account_match, AccountMatcher#best_match and suggested_account_belongs_to_family when making the check so the unlink completes even when the matcher returns an invalid cross-family suggestion.
111-123: ⚡ Quick win
purge_original_fileraising will mask the original upload exception.
purge_original_fileperforms a synchronouspurge(storage I/O) and is called from every rescue path. If purge itself raises (e.g., transient S3/disk error), the new exception will replace the originalActiveRecord::RecordNotUnique/RecordInvalid/DuplicateUploadError, and the caller (andcreate_from_prepared_upload!’s own rescue chain) loses the real cause. The “purges staged blob when database uniqueness race is re-raised” test relies on the original error propagating.Consider isolating cleanup failures so the original exception always wins:
♻️ Suggested refactor
def purge_original_file(statement) return unless statement&.original_file&.attached? statement.original_file.purge + rescue StandardError => e + Rails.logger.error("Failed to purge staged statement file: #{e.class}: #{e.message}") endYou can also consolidate the duplicated purge in the
RecordNotUniquebranch:rescue ActiveRecord::RecordNotUnique duplicate = duplicate_for(family, prepared_upload) - if duplicate - purge_original_file(statement) - raise DuplicateUploadError, duplicate - end - purge_original_file(statement) + raise DuplicateUploadError, duplicate if duplicate raiseAlso applies to: 174-178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/account_statement.rb` around lines 111 - 123, The rescue blocks call purge_original_file(statement) which can raise and mask the original exception (e.g., ActiveRecord::RecordNotUnique); change the error handling so purge errors are isolated and do not replace the original error: when rescuing ActiveRecord::RecordNotUnique, call duplicate_for(family, prepared_upload) as now, consolidate the single purge_original_file call into that branch, but wrap the purge call in its own begin/rescue that logs or ignores purge failures (so they don’t raise), then re-raise the original DuplicateUploadError or original exception; apply the same pattern for the other rescue (StandardError / RecordInvalid) paths and for the similar code around lines 174-178 so cleanup failures never overwrite the original exception from create_from_prepared_upload!.
64-66: 💤 Low valueConsider aligning
linkedscope semantics with thelinked?predicate.The
linkedscope filters byaccount_id IS NOT NULL, while the auto-generatedlinked?enum predicate checksreview_status == "linked". The test at lines 193-210 affirms this divergence, but it’s a subtle source of bugs (e.g.,family.account_statements.linkedexcludes records wherelinked?returns true if account is nil but review_status was set directly). Consider scoping by both columns (or renaming towith_account/account_linked) so the scope and predicate agree.- scope :linked, -> { where.not(account_id: nil) } + scope :linked, -> { where.not(account_id: nil).where(review_status: "linked") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/account_statement.rb` around lines 64 - 66, The current scope :linked only checks account_id while the generated linked? predicate checks review_status == "linked"; update the scope to align semantics by including both conditions (e.g. change scope :linked to -> { where.not(account_id: nil).or(where(review_status: "linked")) } or equivalently use where("account_id IS NOT NULL OR review_status = ?", "linked") ), or rename the scope to :with_account/:account_linked if you intend it to mean “has an account” only; reference scope :linked and the linked? enum predicate/review_status when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.devcontainer/Dockerfile:
- Around line 6-15: Update the Dockerfile comment about libvips42 to clarify
intent: mention that libvips42 is required by image_processing/ruby-vips for
ActiveStorage profile image variants (e.g., User avatar processing) and either
state that this is added opportunistically for future image processing needs
beyond the AccountStatement original_file (which stores PDF/CSV/XLSX) or
explicitly note it was added solely to support other features; reference
libvips42, image_processing/ruby-vips, ActiveStorage, and
AccountStatement.original_file in the comment so reviewers understand the
broader infrastructure rationale.
---
Nitpick comments:
In `@app/models/account_statement.rb`:
- Around line 253-263: The unlink! transaction currently updates the statement
then calls assign_account_match which uses AccountMatcher#best_match and may set
a suggested account that later fails suggested_account_belongs_to_family,
causing the whole transaction to roll back; change unlink! (or adjust
assign_account_match) so that after computing the suggestion you validate it
with suggested_account_belongs_to_family and, if invalid, set suggested_account
(and match_confidence) to nil instead of allowing an exception/validation
failure to bubble up — ensure you reference unlink!, assign_account_match,
AccountMatcher#best_match and suggested_account_belongs_to_family when making
the check so the unlink completes even when the matcher returns an invalid
cross-family suggestion.
- Around line 111-123: The rescue blocks call purge_original_file(statement)
which can raise and mask the original exception (e.g.,
ActiveRecord::RecordNotUnique); change the error handling so purge errors are
isolated and do not replace the original error: when rescuing
ActiveRecord::RecordNotUnique, call duplicate_for(family, prepared_upload) as
now, consolidate the single purge_original_file call into that branch, but wrap
the purge call in its own begin/rescue that logs or ignores purge failures (so
they don’t raise), then re-raise the original DuplicateUploadError or original
exception; apply the same pattern for the other rescue (StandardError /
RecordInvalid) paths and for the similar code around lines 174-178 so cleanup
failures never overwrite the original exception from
create_from_prepared_upload!.
- Around line 64-66: The current scope :linked only checks account_id while the
generated linked? predicate checks review_status == "linked"; update the scope
to align semantics by including both conditions (e.g. change scope :linked to ->
{ where.not(account_id: nil).or(where(review_status: "linked")) } or
equivalently use where("account_id IS NOT NULL OR review_status = ?", "linked")
), or rename the scope to :with_account/:account_linked if you intend it to mean
“has an account” only; reference scope :linked and the linked? enum
predicate/review_status when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97cf439f-e23a-4fa2-8fe4-f94bf1631691
📒 Files selected for processing (37)
.devcontainer/Dockerfile.devcontainer/docker-compose.ymlapp/components/UI/account_page.html.erbapp/components/UI/account_page.rbapp/controllers/account_statements_controller.rbapp/helpers/account_statements_helper.rbapp/helpers/settings_helper.rbapp/models/account.rbapp/models/account_statement.rbapp/models/account_statement/account_matcher.rbapp/models/account_statement/coverage.rbapp/models/account_statement/metadata_detector.rbapp/models/family.rbapp/views/account_statements/index.html.erbapp/views/account_statements/show.html.erbapp/views/accounts/show.html.erbapp/views/accounts/show/_menu.html.erbapp/views/accounts/show/_statements.html.erbapp/views/settings/_settings_nav.html.erbconfig/initializers/active_storage_authorization.rbconfig/locales/models/account_statement/en.ymlconfig/locales/views/account_statements/en.ymlconfig/locales/views/accounts/en.ymlconfig/locales/views/settings/en.ymlconfig/locales/views/settings/fr.ymlconfig/routes.rbdb/migrate/20260505120000_create_account_statements.rbdb/migrate/20260505130000_harden_account_statements.rbdb/migrate/20260505131000_validate_account_statement_constraints.rbdb/schema.rbtest/controllers/account_statements_controller_test.rbtest/controllers/accounts_controller_test.rbtest/fixtures/account_statements.ymltest/integration/active_storage_authorization_test.rbtest/models/account_statement_test.rbtest/system/settings_test.rbtest/test_helper.rb
|
Thoughtful V1 scope — keeping this as canonical storage + coverage without transaction mutation is the right call. A few things worth reviewing: Three separate migrations for one feature Custom ActiveStorage blob authorization is the highest-risk piece
Generated by Claude Code |
Squash the statement vault migration hardening into the feature migration, tighten Active Storage authorization edge cases, bound CSV metadata detection, and add real PDF fixture coverage for stored statements. Validation: targeted statement/auth/controller/provider tests, full Rails suite, system tests, RuboCop, Biome, Brakeman, Zeitwerk, importmap audit, npm audit, ERB lint, CodeRabbit, and Codex Security all passed locally.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/models/account.rb (1)
260-265: ⚡ Quick winRemove the duplicate
create_from_kraken_accountmethod definition at line 268 or deduplicate it to use the shared helper.The newer helper-based version at line 264 is never executed because Ruby keeps the later definition at line 268. This makes the refactor dead code. Future changes to
create_from_crypto_exchange_accountwill miss Kraken's implementation until the duplicate is removed.Evidence
Line 264: def create_from_kraken_account(kraken_account) # Calls create_from_crypto_exchange_account (dead code) Line 268: def create_from_kraken_account(kraken_account) # Full implementation (actually runs)Ruby's "last definition wins" behavior means line 268 shadows line 264.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/account.rb` around lines 260 - 265, There are two definitions of create_from_kraken_account: the helper-based wrapper that calls create_from_crypto_exchange_account and a later full implementation which overrides it; remove the duplicate full implementation (or consolidate its logic into create_from_crypto_exchange_account) so create_from_kraken_account consistently delegates to create_from_crypto_exchange_account (update any specific Kraken-only logic into create_from_crypto_exchange_account or a shared hook so the wrapper remains the single source of truth).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/account.rb`:
- Line 5: Change the lifecycle hook so statement unlinking happens only after
the account deletion has committed: keep a before_destroy hook (e.g. in Account)
that collects and stores the affected statement IDs (e.g.
`@statement_ids_to_move`) but remove the update_all there, and add an
after_destroy_commit hook that calls move_account_statements_to_inbox (or a new
helper like perform_move_account_statements_to_inbox) which performs the
update_all using the captured IDs; update the move_account_statements_to_inbox
implementation to read the stored IDs and return early if none are present to
avoid touching statements when the destroy didn’t commit.
In `@db/migrate/20260505120000_create_account_statements.rb`:
- Around line 10-30: The migration leaves several string columns unbounded;
update the create_account_statements migration to add length limits and simple
DB checks: add limit: options on t.string for filename (e.g. 255), content_type
(e.g. 100), institution_name_hint/account_name_hint (e.g. 200),
account_last4_hint (limit: 4), and currency (limit: 3), and add corresponding
PostgreSQL check constraints (via add_check_constraint or t.check_constraint) to
enforce length bounds (e.g. char_length(...) <= N) for those same columns; keep
existing null:false defaults where specified (e.g. filename, content_type,
upload_status, review_status) and ensure sanitized_parser_output remains jsonb
default {}.
---
Nitpick comments:
In `@app/models/account.rb`:
- Around line 260-265: There are two definitions of create_from_kraken_account:
the helper-based wrapper that calls create_from_crypto_exchange_account and a
later full implementation which overrides it; remove the duplicate full
implementation (or consolidate its logic into
create_from_crypto_exchange_account) so create_from_kraken_account consistently
delegates to create_from_crypto_exchange_account (update any specific
Kraken-only logic into create_from_crypto_exchange_account or a shared hook so
the wrapper remains the single source of truth).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69f0a008-a707-4bf6-9fd7-17d0f4f3ac29
📒 Files selected for processing (12)
app/helpers/settings_helper.rbapp/models/account.rbapp/models/account_statement/account_matcher.rbapp/models/account_statement/metadata_detector.rbapp/models/family.rbconfig/initializers/active_storage_authorization.rbconfig/locales/views/settings/en.ymlconfig/routes.rbdb/migrate/20260505120000_create_account_statements.rbdb/schema.rbtest/integration/active_storage_authorization_test.rbtest/models/account_statement_test.rb
✅ Files skipped from review due to trivial changes (1)
- config/locales/views/settings/en.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- config/routes.rb
- app/models/family.rb
- app/helpers/settings_helper.rb
- app/models/account_statement/metadata_detector.rb
- app/models/account_statement/account_matcher.rb
- test/models/account_statement_test.rb
Move statement unlinking to after account destroy commit, keep Kraken account creation on the shared crypto helper, and add statement metadata length limits with DB checks. Validation: fresh devcontainer with fresh DB via db:prepare, focused account/statement/Kraken/Binance tests, RuboCop, Brakeman, Zeitwerk, git diff --check, CodeRabbit, and Codex Security passed before commit.
Move statement tab data setup out of the ERB partial, harden reconciliation labels and coverage initialization, and tighten statement schema constraints. Validation: CodeRabbit and Codex Security reviewed the current PR diff; Rails focused tests, full Rails tests, system tests, RuboCop, Brakeman, Zeitwerk, ERB lint, npm lint, importmap audit, npm audit, and git diff --check passed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/helpers/account_statements_helper_test.rb (1)
5-10: ⚡ Quick winPrefer i18n-backed expectations over hard-coded labels.
Use
I18n.t(...)in assertions so this test verifies behavior without coupling to exact English copy.Suggested change
- assert_equal "Opening balance", account_statement_reconciliation_label({ key: "opening_balance" }) - assert_equal "Closing balance", account_statement_reconciliation_label({ "key" => "closing_balance" }) - assert_equal "Unknown check", account_statement_reconciliation_label({}) - assert_equal "Unknown check", account_statement_reconciliation_label(nil) - assert_equal "Unknown check", account_statement_reconciliation_label([]) + assert_equal I18n.t("account_statements.reconciliation.checks.opening_balance"), account_statement_reconciliation_label({ key: "opening_balance" }) + assert_equal I18n.t("account_statements.reconciliation.checks.closing_balance"), account_statement_reconciliation_label({ "key" => "closing_balance" }) + assert_equal I18n.t("account_statements.reconciliation.checks.unknown"), account_statement_reconciliation_label({}) + assert_equal I18n.t("account_statements.reconciliation.checks.unknown"), account_statement_reconciliation_label(nil) + assert_equal I18n.t("account_statements.reconciliation.checks.unknown"), account_statement_reconciliation_label([])As per coding guidelines, user-facing strings in Ruby/ERB should use
t()and hierarchical i18n keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/helpers/account_statements_helper_test.rb` around lines 5 - 10, Replace hard-coded expected strings in the test for account_statement_reconciliation_label with i18n lookups: use I18n.t(...) (or t(...)) with the appropriate translation keys for "opening_balance", "closing_balance", and the fallback "unknown_check" so the assertions call account_statement_reconciliation_label(...) and compare to I18n.t('your.scope.opening_balance') / I18n.t('your.scope.closing_balance') / I18n.t('your.scope.unknown_check') respectively; update the four assertions that currently use literal English to use these I18n.t calls so the test verifies behavior against translations rather than fixed copy.app/models/account.rb (1)
5-6: ⚡ Quick winAdd a one-line comment documenting the deliberate absence of
dependent:onaccount_statements.The two-phase destroy pattern is correct and the FK constraint is configured with
on_delete: :nullifyas expected. Thebefore_destroyhook captures statement IDs, and theafter_destroy_commithook moves them to the inbox by nullifyingaccount_idand resettingreview_statusandmatch_confidence. However, line 82'shas_many :account_statements(which intentionally omitsdependent:so statements survive account deletion) deviates from every otherhas_manyon this model and risks accidental breakage. A brief comment will clarify intent for future maintainers:Suggested inline comment
has_one_attached :logo, dependent: :purge_later - has_many :account_statements + # No `dependent:` on purpose: linked statements must survive account deletion and + # be moved into the unmatched inbox via `move_account_statements_to_inbox` (run + # in `after_destroy_commit`). FK is `on_delete: :nullify`. + has_many :account_statements🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/account.rb` around lines 5 - 6, Add a one-line inline comment on the has_many :account_statements association explaining that the absence of a dependent: option is deliberate because the model uses a two-phase deletion: before_destroy :capture_account_statement_ids_to_move records IDs and after_destroy_commit :move_account_statements_to_inbox nullifies account_id and updates statement fields; this prevents accidental future maintainers from adding dependent: :destroy or similar.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/accounts_controller.rb`:
- Around line 51-52: The call to build_statement_tab_data in AccountsController
is being invoked unconditionally and eagerly loads statement-vault datasets
(coverage, statement blobs, reconciliation) even when `@tab` != "statements"; wrap
or gate calls to build_statement_tab_data so they only run when the statements
tab is active (e.g., if `@tab` == "statements" or params[:tab] == "statements")
and apply the same conditional fix for the other occurrence around the code
referenced at lines 238-245; ensure any helper methods invoked by
build_statement_tab_data (coverage loading, blob/reconciliation queries) are
only reached under that same condition to avoid extra DB/attachment work.
---
Nitpick comments:
In `@app/models/account.rb`:
- Around line 5-6: Add a one-line inline comment on the has_many
:account_statements association explaining that the absence of a dependent:
option is deliberate because the model uses a two-phase deletion: before_destroy
:capture_account_statement_ids_to_move records IDs and after_destroy_commit
:move_account_statements_to_inbox nullifies account_id and updates statement
fields; this prevents accidental future maintainers from adding dependent:
:destroy or similar.
In `@test/helpers/account_statements_helper_test.rb`:
- Around line 5-10: Replace hard-coded expected strings in the test for
account_statement_reconciliation_label with i18n lookups: use I18n.t(...) (or
t(...)) with the appropriate translation keys for "opening_balance",
"closing_balance", and the fallback "unknown_check" so the assertions call
account_statement_reconciliation_label(...) and compare to
I18n.t('your.scope.opening_balance') / I18n.t('your.scope.closing_balance') /
I18n.t('your.scope.unknown_check') respectively; update the four assertions that
currently use literal English to use these I18n.t calls so the test verifies
behavior against translations rather than fixed copy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 184a7dca-3215-45dd-acc0-c5bfd4acc581
📒 Files selected for processing (13)
app/components/UI/account_page.rbapp/controllers/accounts_controller.rbapp/helpers/account_statements_helper.rbapp/models/account.rbapp/models/account_statement/coverage.rbapp/views/accounts/show.html.erbapp/views/accounts/show/_statements.html.erbconfig/locales/views/account_statements/en.ymldb/migrate/20260505120000_create_account_statements.rbdb/schema.rbtest/helpers/account_statements_helper_test.rbtest/models/account_statement_test.rbtest/models/account_test.rb
🚧 Files skipped from review as they are similar to previous changes (6)
- db/schema.rb
- app/helpers/account_statements_helper.rb
- app/views/accounts/show/_statements.html.erb
- db/migrate/20260505120000_create_account_statements.rb
- config/locales/views/account_statements/en.yml
- test/models/account_statement_test.rb
Added this to the devcontainer |
Would be nice to always have a one PR = one migration, where it makes sense like here. |
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Pretty close, but there's still a couple of open comments that I think need attention. Some small alignment issues in the UI but we can clean those up in the follow-up PdfImport of statements flow update.
Feel free to separate those issues out if you want to merge as-is, and we complete the work in the next PR, @JSONbored.
2 participants
Summary
PdfImport,FamilyDocument, and vector-store document-import/search flows out of scope for this V1.Feature overview
This V1 answers one core question: "Do I have every month backed by an actual statement, and does Sure's transaction history appear to match it?"
Users can upload statements directly from an account's Statements tab or upload generally into the Statement Vault inbox. Sure keeps the original file, stores bounded metadata and a SHA-256 content digest, suggests likely account matches from conservative hints, and shows year-selectable monthly coverage for each account. Reconciliation is review-only: it compares statement opening/closing balances and period movement against existing balance records when enough data exists, then flags mismatches without changing transactions.
What changed
AccountStatementwith family ownership, optional account linkage, ActiveStorage original-file attachment, SHA-256 duplicate identity, file metadata, statement hints, period/balance metadata, review status, match confidence, and sanitized parser output.Why
Statements are the source documents behind account history, taxes, bookkeeping, disputes, audits, migrations, and provider-history gaps. This creates the local vault and coverage model first so future import/reconciliation work can build on a canonical statement record instead of coupling parsing, transaction mutation, and UI in one step.
Related prior art
This is a clean recreation of the previous Statement Vault PR #1675 on the current upstream fork network after the old PR/branch became unusable during the fork-network transition.
PR #1382 explored the AI-assisted PDF import/reconciliation direction. This PR intentionally does not merge that
PdfImportjob/UI/tool-calling behavior into V1. It does not require AI, does not sync provider statements, and does not create/update/import transactions from uploaded statements. PDF transaction extraction and AI-assisted matching can build later on top ofAccountStatementas explicit review-before-import flows. ExistingPdfImport,FamilyDocument, and vector-store document search flows remain separate document-import/search paths and are intentionally not changed here.Planned future improvements
These are intentionally deferred so this PR stays focused on the canonical
AccountStatementfoundation: secure storage, account linking, conservative metadata/matching, historical coverage, and review-only balance mismatch flags.PdfImportjob/UI behavior into this V1.Screenshots
Captured from headless Chromium against the checked-in devcontainer with deterministic local Statement Vault fixtures.
Account Statements tab
Statement Vault inbox
Unmatched statement suggestion
Mismatched statement detail
Validation
Run in the packaged devcontainer for this worktree after recreating clean devcontainer volumes:
bin/rails db:prepare && bin/rails db:schema:dumpbin/rails test test/models/account_statement_test.rb test/controllers/account_statements_controller_test.rb test/controllers/accounts_controller_test.rb test/integration/active_storage_authorization_test.rb test/system/settings_test.rbbin/rubocop app/components/UI/account_page.rb app/controllers/account_statements_controller.rb app/helpers/account_statements_helper.rb app/helpers/settings_helper.rb app/models/account.rb app/models/account_statement.rb app/models/account_statement/account_matcher.rb app/models/account_statement/coverage.rb app/models/account_statement/metadata_detector.rb app/models/family.rb config/initializers/active_storage_authorization.rb db/migrate/20260505120000_create_account_statements.rb db/migrate/20260505130000_harden_account_statements.rb db/migrate/20260505131000_validate_account_statement_constraints.rb test/controllers/account_statements_controller_test.rb test/controllers/accounts_controller_test.rb test/integration/active_storage_authorization_test.rb test/models/account_statement_test.rb test/system/settings_test.rb test/test_helper.rbbundle exec erb_lint app/components/UI/account_page.html.erb app/views/account_statements/index.html.erb app/views/account_statements/show.html.erb app/views/accounts/show.html.erb app/views/accounts/show/_menu.html.erb app/views/accounts/show/_statements.html.erb app/views/settings/_settings_nav.html.erbbin/brakeman --no-pagerbin/rails zeitwerk:checkgit diff --check/Users/shadowbook/.codex/skills/sure-upstream-contributions/scripts/sure_quality_gate .Notes
PdfImport,FamilyDocument, or vector-store writes/indexing from Statement Vault uploads.Summary by CodeRabbit
New Features
Chores