feat(imports): verify Sure NDJSON import readback#1869
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughPersist expected NDJSON counts on imports, snapshot pre/post readback counts during SureImport, compute deltas and mismatches, record verification results (matched/mismatch/failed/reverted), expose verification in the API/OpenAPI and UI, and add tests covering flows. ChangesSureImport Readback Verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/helpers/imports_helper.rb`:
- Line 35: Replace the hard-coded English labels in the DryRunResource.new calls
(e.g., the label: "Balances" instance in imports_helper.rb and the similar
entries at the other referenced lines) with i18n lookups using t() keys; update
each DryRunResource.new(label: ...) to call t with a descriptive key (for
example imports.dry_run.balances, imports.dry_run.<other_resource>) and add
those keys to the locale files so the labels render in other languages. Ensure
you update all occurrences mentioned (lines around the Balances line and the
other listed entries) and keep the icon, text_class, and bg_class unchanged.
In `@app/views/imports/_success.html.erb`:
- Around line 15-18: Summary: Guard against nil verification payload before
calling fetch on it. Fix: change how verification is obtained from
import.verification_payload[:readback] so it never yields nil (e.g., fallback to
an empty Hash or use safe navigation), then keep the existing fetch calls for
verification_status, checked_counts, and mismatches; update the reference to
import.verification_payload, verification, and the variables
verification_status, checked_counts, mismatches accordingly so NoMethodError
cannot occur when readback is nil.
🪄 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: 64b02e13-818e-4b36-a279-33518e844c30
📒 Files selected for processing (12)
app/helpers/imports_helper.rbapp/models/sure_import.rbapp/views/api/v1/imports/show.json.jbuilderapp/views/imports/_success.html.erbconfig/locales/views/imports/en.ymldb/migrate/20260519100000_add_sure_import_verification_to_imports.rbdb/schema.rbdocs/api/openapi.yamlspec/requests/api/v1/imports_spec.rbspec/swagger_helper.rbtest/controllers/api/v1/imports_controller_test.rbtest/models/sure_import_test.rb
![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: 4a34c2e2f9
ℹ️ 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".
|
Solid feature with comprehensive test coverage. A few observations:
Generated by Claude Code |
superagent-security
Bot
added
the
contributor:verified
There was a problem hiding this comment.
🧹 Nitpick comments (4)
app/helpers/imports_helper.rb (1)
34-48: ⚡ Quick winUse design-system functional tokens for the updated resource classes.
Lines 34–48 still use raw palette classes (
text-*-500,bg-*-500/5). Please switch these entries to functional tokens fromapp/assets/tailwind/sure-design-system.cssfor consistency and theme safety.As per coding guidelines: “Always prefer using functional tokens defined in sure-design-system.css … rather than raw Tailwind utilities.”
🤖 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/helpers/imports_helper.rb` around lines 34 - 48, The DryRunResource entries (e.g., transactions, balances, accounts, categories, tags, rules, merchants, recurring_transactions, transfers, rejected_transfers, trades, holdings, valuations, budgets, budget_categories) use raw palette classes like "text-*-500" and "bg-*-500/5"; replace those text_class and bg_class values with the corresponding functional token classes defined in sure-design-system.css (e.g., use semantic text tokens and background container/selected tokens rather than literal color utilities) so all DryRunResource.new(...) calls reference design-system tokens consistently; update each DryRunResource instantiation to map the existing color intent to the proper token names from app/assets/tailwind/sure-design-system.css.app/views/imports/_success.html.erb (1)
26-30: ⚡ Quick winFormat displayed counts server-side for readability.
At Line 26, Line 30, and Line 39, numeric output is unformatted. Use server-side number formatting (e.g.,
number_with_delimiter) for large values.Proposed patch
- <p class="font-medium text-primary"><%= verification.checked_total %></p> + <p class="font-medium text-primary"><%= number_with_delimiter(verification.checked_total) %></p> @@ - <p class="font-medium text-primary"><%= verification.mismatches_count %></p> + <p class="font-medium text-primary"><%= number_with_delimiter(verification.mismatches_count) %></p> @@ - <span class="font-medium text-primary"><%= counts["actual"] %> / <%= counts["expected"] %></span> + <span class="font-medium text-primary"> + <%= number_with_delimiter(counts["actual"].to_i) %> / + <%= number_with_delimiter(counts["expected"].to_i) %> + </span>As per coding guidelines: “Format currencies, numbers, dates, and other values server-side.”
Also applies to: 39-39
🤖 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/views/imports/_success.html.erb` around lines 26 - 30, The numeric counts rendered in the template (verification.checked_total and verification.mismatches_count and the third numeric output at the other occurrence) must be formatted server-side for readability: wrap each raw numeric interpolation with the Rails helper number_with_delimiter (e.g., replace <%= verification.checked_total %> with number_with_delimiter(verification.checked_total) and do the same for verification.mismatches_count and the other count), ensuring the view calls number_with_delimiter(...) for all three places so large values show delimiters.test/models/sure_import_test.rb (1)
357-361: ⚡ Quick winPrefer mocha-style stubbing/expectations for logger-path test consistency.
Use mocha (
stubs/expects) here instead of Miniteststubblocks to align with repo test conventions.Proposed refactor
- logged_messages = [] - - Rails.logger.stub(:warn, ->(message) { logged_messages << message }) do - `@import.stub`(:update_columns, ->(*) { raise StandardError, "verification write failed" }) do - `@import.send`(:record_failed_readback_verification!, before_counts:, error: original_error) - end - end - - assert_match(/Failed to record Sure import readback verification/, logged_messages.first) - assert_match(/verification write failed/, logged_messages.first) + `@import.stubs`(:update_columns).raises(StandardError.new("verification write failed")) + Rails.logger.expects(:warn).with(regexp_matches(/Failed to record Sure import readback verification.*verification write failed/)) + + `@import.send`(:record_failed_readback_verification!, before_counts:, error: original_error)As per coding guidelines, "Use the
mochagem for stubs and mocks".🤖 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/models/sure_import_test.rb` around lines 357 - 361, Replace Minitest block stubs with mocha-style stubs: stub the Rails.logger.warn and the `@import.update_columns` using mocha's stubs/raises so the test follows repo conventions. Specifically, change the Rails.logger.stub(:warn, ...) block to use Rails.logger.stubs(:warn) and capture the warned message into logged_messages via the stub's block, and change `@import.stub`(:update_columns, ->(*) { raise StandardError, "verification write failed" }) to `@import.stubs`(:update_columns).raises(StandardError, "verification write failed"); keep the call to `@import.send`(:record_failed_readback_verification!, before_counts:, error: original_error) unchanged. Ensure mocha is available in the test environment.test/helpers/imports_helper_test.rb (1)
20-22: ⚡ Quick winUse repo-standard test doubles (
mocha/OpenStruct) instead ofMinitest::Mock.These tests can use
OpenStruct(or minimal mocha stubs) to match repo conventions and avoid explicitverifycalls.Proposed refactor
- import = Minitest::Mock.new - import.expect(:verification_payload, {}) + import = OpenStruct.new(verification_payload: {}) @@ - import.verify @@ - import = Minitest::Mock.new - import.expect(:verification_payload, { readback: nil }) + import = OpenStruct.new(verification_payload: { readback: nil }) @@ - import.verify @@ - import = Minitest::Mock.new - import.expect( - :verification_payload, - { - readback: { - status: "mismatch", - checked_counts: { accounts: 1, transactions: "2" }, - mismatches: { - accounts: { expected: 1, actual: 0 }, - transactions: { expected: 2, actual: 1 }, - categories: { expected: 3, actual: 2 }, - tags: { expected: 4, actual: 3 } - } - } - } - ) + import = OpenStruct.new( + verification_payload: { + readback: { + status: "mismatch", + checked_counts: { accounts: 1, transactions: "2" }, + mismatches: { + accounts: { expected: 1, actual: 0 }, + transactions: { expected: 2, actual: 1 }, + categories: { expected: 3, actual: 2 }, + tags: { expected: 4, actual: 3 } + } + } + } + ) @@ - import.verifyAs per coding guidelines, "Use the
mochagem for stubs and mocks" and "Always preferOpenStructwhen creating mock instances".Also applies to: 34-36, 48-51
🤖 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/imports_helper_test.rb` around lines 20 - 22, Replace the Minitest::Mock usage with the repo-standard test doubles: create an OpenStruct (or a mocha stub) that responds to verification_payload instead of calling Minitest::Mock and expect; specifically, change the `import = Minitest::Mock.new` / `import.expect(:verification_payload, {})` pattern to something like an OpenStruct initialized with verification_payload (or a mocha stub that stubs `verification_payload`) and apply the same replacement for the other mock spots noted (the other `import` mock instances).
🤖 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.
Nitpick comments:
In `@app/helpers/imports_helper.rb`:
- Around line 34-48: The DryRunResource entries (e.g., transactions, balances,
accounts, categories, tags, rules, merchants, recurring_transactions, transfers,
rejected_transfers, trades, holdings, valuations, budgets, budget_categories)
use raw palette classes like "text-*-500" and "bg-*-500/5"; replace those
text_class and bg_class values with the corresponding functional token classes
defined in sure-design-system.css (e.g., use semantic text tokens and background
container/selected tokens rather than literal color utilities) so all
DryRunResource.new(...) calls reference design-system tokens consistently;
update each DryRunResource instantiation to map the existing color intent to the
proper token names from app/assets/tailwind/sure-design-system.css.
In `@app/views/imports/_success.html.erb`:
- Around line 26-30: The numeric counts rendered in the template
(verification.checked_total and verification.mismatches_count and the third
numeric output at the other occurrence) must be formatted server-side for
readability: wrap each raw numeric interpolation with the Rails helper
number_with_delimiter (e.g., replace <%= verification.checked_total %> with
number_with_delimiter(verification.checked_total) and do the same for
verification.mismatches_count and the other count), ensuring the view calls
number_with_delimiter(...) for all three places so large values show delimiters.
In `@test/helpers/imports_helper_test.rb`:
- Around line 20-22: Replace the Minitest::Mock usage with the repo-standard
test doubles: create an OpenStruct (or a mocha stub) that responds to
verification_payload instead of calling Minitest::Mock and expect; specifically,
change the `import = Minitest::Mock.new` / `import.expect(:verification_payload,
{})` pattern to something like an OpenStruct initialized with
verification_payload (or a mocha stub that stubs `verification_payload`) and
apply the same replacement for the other mock spots noted (the other `import`
mock instances).
In `@test/models/sure_import_test.rb`:
- Around line 357-361: Replace Minitest block stubs with mocha-style stubs: stub
the Rails.logger.warn and the `@import.update_columns` using mocha's stubs/raises
so the test follows repo conventions. Specifically, change the
Rails.logger.stub(:warn, ...) block to use Rails.logger.stubs(:warn) and capture
the warned message into logged_messages via the stub's block, and change
`@import.stub`(:update_columns, ->(*) { raise StandardError, "verification write
failed" }) to `@import.stubs`(:update_columns).raises(StandardError, "verification
write failed"); keep the call to
`@import.send`(:record_failed_readback_verification!, before_counts:, error:
original_error) unchanged. Ensure mocha is available in the test environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d05bb50a-a08e-49fb-8155-5f9f0e0b5707
📒 Files selected for processing (7)
app/helpers/imports_helper.rbapp/models/sure_import.rbapp/views/imports/_success.html.erbconfig/locales/views/imports/en.ymltest/controllers/api/v1/imports_controller_test.rbtest/helpers/imports_helper_test.rbtest/models/sure_import_test.rb
Done, done, done! Let me know if anything else is needed - will get it done ASAP! |
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
github-project-automation
Bot
moved this from Todo
to Done
in Latest release train (v0.7.1) - May 2026
2 participants
Summary
Adds durable Sure NDJSON import readback verification so completed imports carry expected upload counts plus family-scoped post-import database deltas.
What changed
GET /api/v1/imports/:idand the completed Sure import UI.docs/api/openapi.yaml.Why
A completed import status is not enough proof for large histories. This gives operators and API consumers a durable count-level check: what the uploaded package claimed, what the database looked like before and after import, and whether the readback delta matches expected records.
Related context
No existing open issue is fully closed by this slice.
Validation
bin/rails test test/models/sure_import_test.rb— 24 tests, 78 assertions.bin/rails test test/controllers/api/v1/imports_controller_test.rb— 55 tests, 325 assertions.RAILS_ENV=test bundle exec rake rswag:specs:swaggerize— 283 examples, 0 failures.bin/rubocop app/models/sure_import.rb app/helpers/imports_helper.rb test/models/sure_import_test.rb test/controllers/api/v1/imports_controller_test.rbbundle exec erb_lint app/views/imports/_success.html.erbnpm run lintbin/rails zeitwerk:checkbin/brakeman --no-pagerbin/importmap auditgit diff --checkRails commands were run with local test database credentials configured.
Notes
This closes the count-level proof gap for single-file Sure NDJSON imports. It does not add row-level fingerprints, split package resume behavior, or historical balance parity checks.
Future work, not included here:
Summary by CodeRabbit