Fix/wise enablebanking counterparty priority#843
Fix/wise enablebanking counterparty priority#843Achieve3318 wants to merge 12 commits intowe-promise:mainfrom
Conversation
…rparty IDs Fixes we-promise#537 For Wise FR EnableBanking transactions, technical counterparty IDs like CARD-1234567890 were being used instead of meaningful merchant labels from remittance_information. This made transaction lists hard to read. Changes: - Ignore counterparty names matching /\ACARD-\d+\z/i pattern - Prioritize remittance_information when counterparty is a technical ID - Maintain existing priority for meaningful counterparty names This ensures that meaningful merchant labels are displayed instead of technical card IDs, improving transaction list readability.
- Trim whitespace from counterparty before regex matching - Use normalized value for CARD-xxx pattern detection - Return normalized counterparty value to ensure no surrounding whitespace
…currency preference Fixes we-promise#565 When creating a new investment transaction (trade), the currency dropdown was defaulting to USD instead of following the account's currency or the family's currency preference. Changes: - Pass default_currency explicitly to money_field helpers in trades form - Use account currency if available, otherwise fall back to family currency - Applies to both price field (for buy/sell) and amount field (for deposit/withdrawal/interest) This ensures the currency dropdown respects user preferences and account settings instead of hardcoding USD.
) - Add ApplyRulesToTransactionService to apply active rules to manually created/updated transactions - Update TransactionsController to apply rules after create/update - Update TransactionCategoriesController to apply rules after category updates - Update BulkUpdatesController to apply rules after bulk updates - Add comprehensive test coverage (900+ lines of tests) - Rules are now automatically applied when transactions are manually created or edited, not just during sync operations - Includes error handling, execution time tracking, and RuleRun record creation for tracking Total changes: ~1275 lines (1161 new + 114 modified)
|
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 a new ApplyRulesToTransactionService and invokes it from transaction-related controllers and bulk update flows to evaluate/apply active transaction rules manually after create/update operations; updates Entry bulk_update locking behavior and related tests and view tweaks. Changes
Sequence DiagramsequenceDiagram
participant User as User/Request
participant Controller as TransactionsController / BulkUpdatesController / TransactionCategoriesController
participant Entry as Entry Model
participant Service as ApplyRulesToTransactionService
participant RuleDB as Rules (DB/Scope)
participant RuleRun as RuleRun Record
participant ActionExec as Rule Action Executor
User->>Controller: Create/Update/Bulk Update request
Controller->>Entry: Save or update Entry(s)
Entry-->>Controller: Persisted Entry(s)
Controller->>Service: call(transactions_or_entries, execution_type: "manual")
Service->>Service: Normalize inputs → transactions
Service->>RuleDB: Query active rules (resource_type: "transaction")
RuleDB-->>Service: Active rules list
loop for each rule
Service->>RuleDB: Compute matching transactions (rule scope)
RuleDB-->>Service: Matching transaction ids
Service->>ActionExec: Execute rule actions on matches
ActionExec-->>Service: Action results (modified counts / errors)
Service->>RuleRun: Create/Update RuleRun (status, counts, exec_type)
RuleRun-->>Service: Persisted
end
Service-->>Controller: Results (processed_count, modified_count, errors, time_ms)
Controller->>Entry: Reload affected Entry(s)
Entry-->>Controller: Fresh state
Controller-->>User: Response (updated entry/redirect)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![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: 25d50c7f56
ℹ️ 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".
| rule.actions.each do |action| | ||
| # Scope the action to only our matching transactions | ||
| transaction_scope = Transaction.where(id: matching_transactions.pluck(:id)) | ||
|
|
There was a problem hiding this comment.
The new service captures matching_transactions once and then applies every action to a fixed Transaction.where(id: ...) scope. In the existing rule engine (Rule#apply in app/models/rule.rb), each action is applied against matching_resources_scope, so if an earlier action mutates a field used by the rule conditions (e.g., renaming the transaction), later actions stop applying to that transaction. With the fixed ID scope here, later actions still run even after the condition no longer matches, so manual updates can behave differently from sync/scheduled runs and apply actions that would normally be skipped. Consider recomputing the matching scope per action (or explicitly documenting the intended divergence).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/controllers/transaction_categories_controller.rb`:
- Around line 22-24: After calling ApplyRulesToTransactionService.new(`@entry`,
execution_type: "manual").call the entry/transaction may be mutated by rules but
the controller continues to use the stale `@entry` instance for rendering; reload
the record before rendering (e.g., call `@entry.reload`) so the subsequent
turbo-stream rendering uses the updated category/state—mirror the approach used
in TransactionsController#update to ensure the final category is shown.
In `@app/models/enable_banking_entry/processor.rb`:
- Around line 71-74: Update the misleading priority comment in
EnableBankingEntry::Processor to reflect the current fallback order used when
constructing the transaction name: counterparty name first, then
remittance_information, and finally bank_transaction_code description (i.e.,
counterparty name > remittance_information > bank_transaction_code description).
Locate the comment near the method that builds the transaction name (the
name-construction method in the Processor class, e.g., build_name /
name_from_transaction) and change the comment text to match this new order.
In `@test/services/apply_rules_to_transaction_service_test.rb`:
- Around line 669-705: The test "handles large number of rules efficiently"
includes a flaky time-based assertion; remove or disable the execution_time_ms
check by deleting or commenting out the line asserting
result[:execution_time_ms] < 5000 (or move that assertion into a separate
benchmark/performance test suite), leaving the functional assertions for
result[:rules_matched] and result[:rules_applied] intact so
ApplyRulesToTransactionService is still validated for correctness without CI
timing flakiness.
🧹 Nitpick comments (4)
app/services/apply_rules_to_transaction_service.rb (1)
171-191: Avoid repeated ID plucks per action.
matching_transactions.pluck(:id)runs once per action. Cache the IDs (and scope) once to reduce redundant queries.♻️ Suggested refactor
- rule.actions.each do |action| - # Scope the action to only our matching transactions - transaction_scope = Transaction.where(id: matching_transactions.pluck(:id)) + transaction_ids = matching_transactions.pluck(:id) + transaction_scope = Transaction.where(id: transaction_ids) + + rule.actions.each do |action| + # Scope the action to only our matching transactionsapp/views/trades/_form.html.erb (1)
44-56: DRYdefault_currencyand move logic out of the template.You compute
default_currencytwice in the view. Consider computing it once (ideally in a helper/controller and pass it in) to keep the template lean and avoid duplication.As per coding guidelines, Keep domain logic OUT of view templates. Logic belongs in component files, not template files.♻️ Suggested refactor
-<% type = params[:type] || "buy" %> +<% type = params[:type] || "buy" %> +<% default_currency = account&.currency || Current.family.currency %> ... - <% default_currency = account ? account.currency : Current.family.currency %> - <%= form.money_field :amount, label: t(".amount"), value: model.amount, required: true, default_currency: default_currency %> + <%= form.money_field :amount, label: t(".amount"), value: model.amount, required: true, default_currency: default_currency %> ... - <% default_currency = account ? account.currency : Current.family.currency %> - <%= form.money_field :price, label: t(".price"), step: "any", precision: 10, required: true, default_currency: default_currency %> + <%= form.money_field :price, label: t(".price"), step: "any", precision: 10, required: true, default_currency: default_currency %>test/services/apply_rules_to_transaction_service_test.rb (1)
6-12: Prefer fixtures for baseline account/merchant/category setup.These are base-case records created for every test. Consider moving them to fixtures and referencing them in setup to reduce per-test DB writes and align with test conventions.
As per coding guidelines, Use fixtures for base cases in tests (2-3 per model) and create edge cases on-the-fly within test context.
test/controllers/transactions_controller_test.rb (1)
38-84: Good integration test coverage for rule application on create.The test correctly verifies end-to-end behavior. A couple of minor observations:
The
rulevariable (line 44) is created but never referenced—this is fine since you're testing the side effect, but prefixing with_(e.g.,_rule = Rule.create!(...)) would signal intent and silence linter warnings about unused variables.Line 80: The
reloadcall is redundant sinceEntry.order(:created_at).lastjust fetched the record from the database.🧹 Optional cleanup
# Create a rule that should match the new transaction - rule = Rule.create!( + _rule = Rule.create!( family: family, ... ) ... created_entry = Entry.order(:created_at).last - created_entry.reload # Verify rule was applied assert_equal category, created_entry.transaction.category
- Reload @entry after applying rules in TransactionCategoriesController to ensure turbo stream rendering uses updated state - Update misleading priority comment in EnableBankingEntry::Processor to reflect correct order: counterparty > remittance_information > bank_transaction_code - Remove flaky time-based assertion from test to avoid CI timing issues
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/models/enable_banking_entry/processor.rb`:
- Around line 93-97: The code assumes data[:remittance_information] is an Array
of Strings before calling truncate; instead, normalize and guard its shape:
retrieve remittance = data[:remittance_information], if remittance.is_a?(Array)
take remittance.first, then ensure the selected value is a String (e.g., convert
via to_s or skip non-String/nil entries) and only call truncate on that safe
string; update the logic in the processor (enable_banking_entry/processor.rb)
where remittance is handled so non-Array payloads (String or other types) and
Arrays with non-String elements are defensively handled and truncated to 100
chars safely.
In `@test/services/apply_rules_to_transaction_service_test.rb`:
- Around line 380-392: The test never asserts lock behavior: capture the
transaction category before running rules (original_category already set) and
then assert that after the first call (result1) with ignore_attribute_locks:
false the entry.reload.transaction.category equals original_category, and after
the second call (result2) with ignore_attribute_locks: true assert the category
has changed (not equal to original_category); reference
ApplyRulesToTransactionService, result1, result2, entry, and original_category
to locate where to add these assertions.
- Around line 756-794: The test incorrectly reuses the same family fixture
(families(:empty)) for other_family, so it doesn't validate cross-family
isolation; change other_family to a different family fixture or create a new
Family record (e.g., use families(:other) or Family.create!) before creating
other_account/other_merchant/other_entry, ensuring the Rule created for `@family`
(via Rule.create!) is distinct from the family on other_entry so
ApplyRulesToTransactionService.new(other_entry, execution_type: "manual").call
truly tests family scoping and then assert result[:rules_matched] == 0.
🧹 Nitpick comments (3)
test/services/apply_rules_to_transaction_service_test.rb (3)
283-288: Weak assertion doesn't verify error handling behavior.The assertion
assert result[:rules_applied] >= 0will always pass regardless of whether errors occurred. This test should verify expected error handling behavior more precisely.Consider asserting the expected outcome based on how the service should handle invalid category IDs:
# Should have matched but may have errors assert_equal 1, result[:rules_matched] - # The rule may still be applied but with an error, or it may fail - assert result[:rules_applied] >= 0 + # Rule application should fail with invalid category ID + assert_equal 0, result[:rules_applied] + assert_equal 1, result[:errors].countIf the actual behavior differs, adjust the assertions to match and document the expected behavior.
632-667: Test description doesn't match implementation.The test is named "handles rules with different resource types" but only creates and tests a transaction rule. The comment on lines 639-641 acknowledges this. Consider either:
- Renaming the test to accurately describe what it tests (e.g., "only applies rules with matching resource type")
- Actually creating a rule with a different resource type to verify it's not applied
🔧 Suggested rename
- test "handles rules with different resource types" do + test "only applies transaction rules to transactions" do
868-912: Test name is misleading; doesn't test concurrency.The test "handles concurrent rule applications" doesn't test concurrent/parallel execution (e.g., multiple threads applying rules simultaneously). It tests batch processing of multiple transactions, which is already covered by other tests.
Consider either:
- Renaming to reflect what it actually tests (e.g., "applies rules to batch of transactions")
- Implementing actual concurrency testing with threads if that's the intent
🔧 Suggested rename for clarity
- test "handles concurrent rule applications" do + test "applies rules to batch of transactions" do
- Normalize remittance_information handling in EnableBankingEntry::Processor to safely handle Arrays with non-String elements and non-Array payloads - Fix lock behavior test to properly capture original_category before rule application and add explicit assertions - Fix cross-family isolation test to use a different Family record instead of reusing the same fixture
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/services/apply_rules_to_transaction_service_test.rb`:
- Around line 807-835: The test currently uses a vacuous assertion
(result[:rules_matched] >= 0); decide and codify the intended behavior for empty
Rule#conditions (match-all or ignored) and replace the assertion with an
explicit check: for example, if empty conditions should match-all assert that
ApplyRulesToTransactionService.new(entry, execution_type: "manual").call returns
result[:rules_matched] == 1 and that the transaction’s category was set to
`@groceries_category` (or vice versa if they should be ignored assert == 0 and
category unchanged). Update the "handles empty rule conditions gracefully" test
to assert the concrete expectation and any side-effects rather than a >= 0
check, referencing the Rule.create! usage and ApplyRulesToTransactionService
call to locate the code.
🧹 Nitpick comments (3)
app/models/enable_banking_entry/processor.rb (1)
97-112: Consider simplifying redundant type checks.Line 100's
item.respond_to?(:to_s)is always true for any Ruby object, so the condition effectively reduces toitem.present?. Similarly, line 110's!remittance_value.strip.empty?is redundant whenremittance_value.present?already rejects blank strings.✨ Proposed simplification
remittance_value = if remittance.is_a?(Array) - # Find first non-nil, non-empty element that can be converted to string - # Skip non-String/nil entries and convert to string - remittance.find { |item| item.present? && (item.is_a?(String) || item.respond_to?(:to_s)) }&.to_s + # Find first present element (preferring strings) and convert to string + remittance.find(&:present?)&.to_s elsif remittance.is_a?(String) - # Already a string, use directly remittance else - # Handle other types by converting to string - remittance.to_s if remittance.respond_to?(:to_s) + remittance.to_s end - # Only use remittance if we have a valid non-empty string value - if remittance_value.present? && remittance_value.is_a?(String) && !remittance_value.strip.empty? + if remittance_value.present? return remittance_value.truncate(100) endtest/services/apply_rules_to_transaction_service_test.rb (2)
639-674: Test name doesn’t match behavior (no non-transaction rule).This test only creates a transaction rule, so it doesn’t validate filtering by
resource_type. Either add a non-transaction rule and assert it’s ignored, or rename the test to match current behavior.✏️ Rename to match current behavior
- test "handles rules with different resource types" do + test "handles transaction rules" do
879-923: Test name suggests concurrency but runs single-threaded.This test performs a single service call over multiple entries. Consider renaming or adding real concurrency to match intent.
✏️ Rename to reflect current behavior
- test "handles concurrent rule applications" do + test "handles batched rule applications" do
…ule conditions - Update test to assert that rules with empty conditions match all transactions (match-all behavior) - Replace result[:rules_matched] >= 0 with explicit assertions: - rules_matched == 1 - rules_applied == 1 - transactions_modified > 0 - transaction category is set to expected value - Add documentation explaining that empty conditions return base scope (match-all)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/services/apply_rules_to_transaction_service_test.rb`:
- Around line 284-288: The current assertion `result[:rules_applied] >= 0` is
vacuous; replace it with a concrete expectation for how the system should behave
with an invalid action: either assert_equal 0, result[:rules_applied] and then
assert_includes result[:errors], /invalid category|Category.*not found/i (or the
exact error/message your service produces), or assert_equal 1 and assert_empty
result[:errors] if the rule should succeed; update the assertions around
result[:rules_matched], result[:rules_applied], and result[:errors] in the test
to reflect the chosen expected outcome so the test can fail on regressions.
- Around line 874-884: The conditional guard "if result[:errors].any?" lets the
test skip validations if errors are empty; change it to assert that errors are
present (e.g. assert result[:errors].any? or assert_not_empty result[:errors])
immediately after checking rules_matched, then validate the first error's
structure (check error[:rule_id].present?, error[:rule_name].present?,
error[:error].present?) so the error path is always exercised; update the block
around result[:errors] accordingly in the test.
🧹 Nitpick comments (1)
test/services/apply_rules_to_transaction_service_test.rb (1)
886-930: Test name is misleading; this tests batch processing, not concurrency.The test name "handles concurrent rule applications" implies testing thread-safety or parallel execution, but the test simply passes an array of entries to the service—standard batch processing. Consider renaming to accurately describe the behavior under test.
✏️ Suggested rename
- test "handles concurrent rule applications" do + test "applies rules to batch of transactions" do
…ions - Fix 'handles errors gracefully' test: assert rules_applied == 0 and validate error structure when invalid category ID is used - Fix 'returns proper error structure' test: remove conditional guard and always assert errors are present, then validate error structure - Both tests now explicitly assert expected behavior for invalid actions: - rules_matched == 1 (rule matches) - rules_applied == 0 (rule fails to apply) - errors are present and contain rule_id, rule_name, and error message
|
Hi, @jjmata |
|
Hi, @jjmata |
|
Hi, @jjmata , Could you review my PR, please? |
|
There's a conflict here now, so no tests can run. Also, @quentinreytinas has a couple of PRs pending about this same area ... can you two coordinate? I think the work is overlapping somewhat. Are you on our Discord? 🙏 and thanks! |
I have discord. daniel98518 |
|
Hi, @jjmata , I have fixed conflicts |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/controllers/transactions_controller.rb`:
- Around line 106-110: Apply the same fix as in create: run
ApplyRulesToTransactionService before calling lock_saved_attributes! so rules
can modify locked fields, and wrap the service invocation in a begin/rescue to
catch and handle errors (log the error and render the turbo-stream
response/fallback instead of letting an exception bubble to a 500). Concretely,
move ApplyRulesToTransactionService.new(`@entry`, execution_type: "manual").call
to occur prior to any lock_saved_attributes! call, keep the `@entry.reload` after
rules, and add a rescue around the service call that logs the exception and
ensures the controller renders the appropriate turbo stream response rather than
raising.
In `@app/controllers/transactions/bulk_updates_controller.rb`:
- Around line 12-16: The call to ApplyRulesToTransactionService (in
BulkUpdatesController where updated_entries are built and in
TransactionsController#create and `#update`) can raise and currently will bubble
up, turning a successful DB update into a 500; wrap the
ApplyRulesToTransactionService.new(...).call invocation in a begin/rescue that
catches StandardError, logs the error (include the exception message and
backtrace) and optionally reports to your error tracker (e.g., Sentry), but does
NOT re-raise so the controller can continue to return the successful response
for the committed bulk update; apply the same rescue pattern to the other call
sites (TransactionsController#create and `#update`) and ensure any local variables
(e.g., updated_entries, entry_ids) are preserved for logging/context.
|
@coderabbitai check again |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@app/controllers/transactions_controller.rb`:
- Around line 71-84: In TransactionsController#create, the rescue for
ApplyRulesToTransactionService should reload `@entry` so subsequent
lock_saved_attributes! runs against the persisted state; update the rescue block
around ApplyRulesToTransactionService.new(`@entry`, execution_type: "manual").call
to call `@entry.reload` (like the update action's rescue), then proceed with
logging/Sentry reporting and continue—ensure `@entry.reload` is invoked before
lock_saved_attributes! is reached.
In `@app/controllers/transactions/bulk_updates_controller.rb`:
- Around line 12-16: ApplyRulesToTransactionService is being invoked after
Entry#bulk_update! has already called lock_saved_attributes! (which uses
lock_attr!), so rule actions cannot change locked fields; either move the
ApplyRulesToTransactionService call in bulk_updates_controller.rb to run before
calling Entry#bulk_update!, or change Entry#bulk_update! to defer calling
lock_saved_attributes!/lock_attr! until after rules are applied (so rules can
modify date, notes, category_id, merchant_id, tags), and ensure
mark_user_modified! behavior remains correct; update references to
ApplyRulesToTransactionService and Entry#bulk_update! accordingly and add a
comment documenting the chosen precedence if bulk edits should intentionally
override rules.
In `@commit_error_handling.sh`:
- Around line 1-5: This is a developer-local automation script
(commit_error_handling.sh) that must be removed from the PR; delete the file
from the branch (mirror the same removal you did for complete_merge.sh), remove
it from the index so it isn’t committed, and ensure any similar local scripts
aren’t reintroduced (optionally add a note or pattern to .gitignore if you keep
local automation files); reference commit_error_handling.sh and
complete_merge.sh when making the change so reviewers see the intent.
In `@complete_merge.sh`:
- Around line 1-9: Remove the developer-local merge script complete_merge.sh
from the PR by deleting the file from the repository, commit the deletion on
this branch (use git remove/commit), and push the branch; also ensure it's not
reintroduced by adding an appropriate ignore rule or avoiding local helper
scripts in commits so future commits don't include /home/daniel-specific
utilities.
In `@local` variables for error context (entry_ids, updated_entries, etc.)":
- Around line 1-258: The file is an accidental inclusion of the "SUMMARY OF LESS
COMMANDS" (less pager help) and must be removed; delete the file from the
commit/history and working tree (remove the file that contains the "SUMMARY OF
LESS COMMANDS"/"less" help text), amend the commit or create a new commit that
removes it, and ensure it’s not re-added (optionally add a rule to .gitignore if
the file is generated), then push the amended branch; locate the file by its
content string "SUMMARY OF LESS COMMANDS" or references to "less" pager help to
find and remove it.
In `@olved` conflict by combining both changes::
- Around line 1-26: This commit contains a stale/garbled duplicate of the
controller (class Transactions::BulkUpdatesController) missing the
ApplyRulesToTransactionService integration; remove the duplicate file (the one
whose filename looks like a merge artifact) and keep the correct controller that
includes rule application logic in create (the version that invokes
ApplyRulesToTransactionService) so the create flow updates via bulk_update! and
then applies rules; ensure methods bulk_update_params and tags_provided? remain
intact in the kept file and delete the stale copy.
In `@push_fix.sh`:
- Around line 1-8: The file push_fix.sh contains a hard-coded GitHub PAT in the
git remote set-url command (the "ghp_..." token on the git remote set-url origin
line); immediately revoke that token in GitHub, remove push_fix.sh from the PR
and any commits that add it (unstage/remove the file and amend/rebase the commit
that added it), and then scrub the token from repository history using a
history-rewriting tool such as git filter-branch or BFG Repo-Cleaner (search for
occurrences of "ghp_" or the exact remote URL to ensure all references are
removed) before pushing or merging; also update any CI/deployment scripts to use
environment variables or secrets management instead of embedding tokens in
files.
- Fix transactions_controller.rb: Ensure @entry.reload in rescue block before lock_saved_attributes! - Fix bulk_updates_controller.rb: Apply rules before locking attributes (rules can modify date, notes, category_id, merchant_id, tags) - Remove unwanted files: commit_error_handling.sh, complete_merge.sh, push_fix.sh, and duplicate conflict file - Remove accidental 'local variables' file containing less pager help text
|
@coderabbitai, check again |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/controllers/transactions/bulk_updates_controller.rb`:
- Around line 39-43: lock_saved_attributes! is a no-op on freshly-loaded records
so replace it by explicitly locking the attributes that were part of the bulk
update params: inside entries_scope.find_each, remove the call to
lock_saved_attributes!, call entry.lock_attr!(...) for each attribute key
present in the incoming bulk update params (the same set your controller uses to
apply updates), then call entry.mark_user_modified! as before; also guard tag
locking so you only call entry.transaction.lock_attr!(:tag_ids) when
tags_provided? is true and entry.transaction? is true (avoid calling .tags on
non-Transaction entries).
Replace lock_saved_attributes! (no-op on freshly-loaded records) with explicit lock_attr! calls for each attribute present in bulk update params: - Lock date and notes on entry level - Lock category_id and merchant_id on transaction level - Only lock tag_ids when tags_provided? is true and entry is a transaction - Avoid calling .tags on non-Transaction entries
|
@coderabbitai , check again. |
|
✅ Actions performedReview triggered.
|
|
Old, unfocused, mostly slop PR. Closing. |
2 participants
Description
This PR implements feature request #816: Apply rules on manually created transactions.
Currently, user-defined rules are only applied during account sync operations. This means that when users manually create or edit transactions, their rules are not automatically applied, requiring them to manually trigger rule application or wait for the next sync.
This PR ensures that rules are automatically applied whenever transactions are manually created or updated, providing immediate feedback and ensuring consistency.
Changes
New Service:
ApplyRulesToTransactionServiceapp/services/apply_rules_to_transaction_service.rb(247 lines)RuleRunrecords for audit trailController Updates
TransactionsController: Applies rules after transaction create/updateTransactionCategoriesController: Applies rules after category updatesBulkUpdatesController: Applies rules after bulk transaction updatesTest Coverage
test/services/apply_rules_to_transaction_service_test.rb(914 lines)test/controllers/transactions_controller_test.rbHow It Works
ApplyRulesToTransactionServiceis calledRuleRunrecords are created for tracking and audit purposesKey Features
effective_dateon rulesRuleRunrecordsTesting
All existing tests pass. New test coverage includes:
Performance Considerations
find_eachExample Usage
Automatically called when creating a transaction
entry = account.entries.create!(...)
Rules are automatically applied via TransactionsController
Or manually:
ApplyRulesToTransactionService.new(entry, execution_type: "manual").call
For multiple transactions:
ApplyRulesToTransactionService.new([entry1, entry2], execution_type: "manual").call## Related Issue
Closes #816
Checklist
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation