feat: implement transaction reimbursement linking#858
feat: implement transaction reimbursement linking#858CylonN8 wants to merge 4 commits intowe-promise:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds reimbursement linking: new controller actions (match, link, unlink), transaction associations/validation for reimbursements, budget-period scope adjustments, views and routes for linking/unlinking, DB migrations to add Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser as Client (Browser)
participant Controller as TransactionsController
participant Model as Transaction
participant DB as Database
User->>Browser: Click "Link Expense" on inflow
Browser->>Controller: GET /transactions/:id/reimbursement_match?filter_date
Controller->>Model: Build candidate scope (visible, for_budget_period / in_period, category filters)
Model->>DB: Query candidate expenses (joins on entries / parent_entries)
DB-->>Model: Candidate rows
Controller-->>Browser: Render reimbursement_match (Turbo frame)
User->>Browser: Select candidate & submit
Browser->>Controller: POST /transactions/:id/link_reimbursement
Controller->>Model: Set original_expense_id on inflow -> validate_reimbursement_logic
alt validation passes
Model->>DB: Save transaction
DB-->>Model: OK
Controller-->>Browser: Success (redirect/flash)
else validation fails
Controller-->>Browser: Error response
end
User->>Browser: Click "Unlink"
Browser->>Controller: DELETE /transactions/:id/unlink_reimbursement
Controller->>Model: Clear original_expense_id (or nullify)
Model->>DB: Persist change
Controller-->>Browser: Success response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f81a12d27
ℹ️ 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".
…nsaction filtering
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/models/income_statement.rb`:
- Around line 68-71: Remove the unused date_range plumbing: update
build_period_total to stop passing date_range to totals_query and remove the
date_range parameter from the Totals class constructor and any Totals
instantiation (e.g., in totals_query), then delete the date_range-related param
preparation and usage inside Totals (including any start_date/end_date SQL
parameter building) and ensure transactions_subquery_sql and
transactions_only_query_sql rely on the existing for_budget_period scope for
date filtering; update totals_query, Totals.initialize, and any other callers to
remove the date_range argument so the code no longer prepares unused SQL date
parameters.
In `@app/views/transactions/show.html.erb`:
- Around line 332-376: The view iterates `@entry.transaction.reimbursements` and
calls reimbursement.merchant which can cause N+1 queries; update the controller
action that loads `@entry` (where `@entry` is assigned, e.g., EntriesController#show
or the finder method that sets `@entry`) to eager-load reimbursements and their
merchant/entry associations using includes (e.g., include entryable: {
reimbursements: [:merchant, :entry] }) so transaction.reimbursements and
reimbursement.merchant do not issue extra queries, and replace the hard-coded
strings "Reimbursements" and "Inflows linked to this expense." in the
show.html.erb with i18n keys (use t("...") and add the corresponding translation
keys).
- Around line 270-329: Replace all hardcoded user-facing strings in the
reimbursement block with i18n lookups using t(), specifically update the
headings and copy used in the conditional branch around
`@entry.transaction.original_expense` and the else branch that renders DS::Link:
replace "Reimburses Expense", "This transaction is marked as a reimbursement
for:", both "Link Expense" instances, and "Is this a reimbursement? Link it to
the original expense." with t() calls (e.g.,
t('transactions.show.reimbursement.title') etc.) in
app/views/transactions/show.html.erb, and add corresponding keys under
config/locales/views/transactions/en.yml (e.g.,
transactions.show.reimbursement.title,
transactions.show.reimbursement.description,
transactions.show.link_expense.text, transactions.show.link_expense.hint) to
provide English translations.
🧹 Nitpick comments (10)
db/migrate/20260131164519_add_original_expense_id_to_transactions.rb (1)
1-6: Consider adding a foreign key constraint for referential integrity.The migration adds the column and index but lacks a foreign key constraint to the
transactionstable itself. This could lead to orphaned references if the original expense is deleted outside of Rails callbacks.♻️ Suggested improvement
class AddOriginalExpenseIdToTransactions < ActiveRecord::Migration[7.2] def change add_column :transactions, :original_expense_id, :uuid unless column_exists?(:transactions, :original_expense_id) add_index :transactions, :original_expense_id unless index_exists?(:transactions, :original_expense_id) + unless foreign_key_exists?(:transactions, column: :original_expense_id) + add_foreign_key :transactions, :transactions, column: :original_expense_id, on_delete: :nullify + end end endThis aligns with the
dependent: :nullifybehavior defined in the model'shas_many :reimbursementsassociation and ensures database-level integrity.app/models/transaction.rb (2)
58-66: Potential N+1 query risk and readability concern infor_budget_periodscope.The raw SQL joins are functional but could cause issues:
- The scope only filters by date but doesn't eager-load the joined data, so subsequent access to
original_expensewill trigger additional queries.- The join aliases (
parent_trans,parent_entries) won't be accessible in Ruby after the query.Consider adding
.includes(:original_expense)when this scope is used in contexts that iterate over reimbursements.Also, the static analysis warning about "hardcoded passphrase" is a false positive - the tool incorrectly flagged the SQL JOIN syntax.
171-183: Clarify validation comments to match the sign convention.The validation logic appears correct based on the codebase convention (positive = expense/outflow, negative = income/inflow), but the comments are confusing. Line 173 says "inflow (negative amount)" but checks
positive?, and line 178 says "expense (positive amount)" but checksnegative?.📝 Suggested comment clarification
def validate_reimbursement_logic if original_expense_id.present? - # Child (reimbursement) must be an inflow (negative amount) + # Reimbursement must be an inflow - stored as negative amount in DB if entry&.amount&.positive? errors.add(:base, "Reimbursement must be an inflow (income/refund)") end - # Parent (original expense) must be an expense (positive amount) + # Original expense must be an outflow - stored as positive amount in DB if original_expense && original_expense.entry&.amount&.negative? errors.add(:base, "Original transaction must be an expense") end end endapp/controllers/transactions_controller.rb (3)
309-345: Use i18n for flash messages and consider authorization.A few observations on this new action:
- The flash messages should use i18n (e.g.,
t("transactions.reimbursement.linked")) to support localization.- Consider adding a guard to ensure
@transactionis actually an inflow (negative amount) before allowing the match operation.The query construction with
.includes(entry: :account)properly prevents N+1 queries.💡 Suggested improvement
def reimbursement_match `@transaction` = Current.family.transactions.find(params[:id]) + # Ensure this is an inflow being matched to an expense + unless `@transaction.entry.amount` < 0 + flash[:alert] = t("transactions.reimbursement_match.not_an_inflow") + redirect_back_or_to transactions_path + return + end + # Candidate expenses: posted (not pending), outflow (positive amount)
347-358: Use i18n for user-facing strings.Flash messages should use the
t()helper for localization support instead of hardcoded strings.🌐 Suggested i18n usage
if `@transaction.update`(original_expense_id: `@expense.id`) - flash[:notice] = "Reimbursement linked" + flash[:notice] = t("transactions.link_reimbursement.success") else - flash[:alert] = "Failed to link reimbursement: #{`@transaction.errors.full_messages.join`(', ')}" + flash[:alert] = t("transactions.link_reimbursement.failure", errors: `@transaction.errors.full_messages.join`(', ')) endAs per coding guidelines: "All user-facing strings must use localization (i18n) via
t()helper".
360-376: Use i18n for user-facing strings.Same localization concern applies here - flash messages should use the
t()helper.🌐 Suggested i18n usage
if `@inflow.update`(original_expense_id: nil) - flash[:notice] = "Reimbursement unlinked" + flash[:notice] = t("transactions.unlink_reimbursement.success") else - flash[:alert] = "Failed to unlink reimbursement" + flash[:alert] = t("transactions.unlink_reimbursement.failure") endAs per coding guidelines: "All user-facing strings must use localization (i18n) via
t()helper".app/views/transactions/reimbursement_match.html.erb (4)
1-6: Use i18n for user-facing strings.The dialog title and instructional text should use the
t()helper for localization.🌐 Suggested i18n usage
<%= render DS::Dialog.new do |dialog| %> - <% dialog.with_header(title: "Link Expense") %> + <% dialog.with_header(title: t("transactions.reimbursement_match.title")) %> <% dialog.with_body do %> <p class="text-sm text-secondary mb-4"> - Select the original expense transaction that this inflow reimburses. + <%= t("transactions.reimbursement_match.instructions") %> </p>As per coding guidelines: "All user-facing strings must use localization (i18n) via
t()helper".
8-18: Use functional design tokens instead of raw Tailwind colors.The form uses raw Tailwind color values (
text-gray-700,border-gray-300,border-indigo-500,ring-indigo-500) instead of design system tokens. Also, the label text should use i18n.🎨 Suggested design token usage
<div class="mb-4"> <%= form_with url: reimbursement_match_transaction_path(`@transaction`), method: :get, data: { turbo_frame: "reimbursement_candidates", controller: "auto-submit-form" } do |f| %> - <%= f.label :filter_date, "Filter by Date", class: "block text-sm font-medium text-gray-700 mb-1" %> + <%= f.label :filter_date, t("transactions.reimbursement_match.filter_by_date"), class: "block text-sm font-medium text-primary mb-1" %> <%= f.date_field :filter_date, value: `@filter_date`, max: Date.current, - class: "w-full rounded-md border-gray-300 shadow-sm focus:border-indigo-500 focus:ring-indigo-500 sm:text-sm", + class: "w-full rounded-md border-secondary shadow-sm focus:border-primary focus:ring-primary sm:text-sm", data: { auto_submit_form_target: "auto" } %> <% end %> </div>As per coding guidelines: "Always use functional tokens from design system (e.g.,
text-primarynottext-white) defined inapp/assets/tailwind/maybe-design-system.css".
23-67: Use functional design tokens for styling.Multiple raw Tailwind color classes are used instead of design system tokens:
border-gray-200,hover:bg-gray-50,text-indigo-600,text-gray-900,text-gray-500,text-green-600.🎨 Key replacements
-<div class="max-h-96 overflow-y-auto border border-gray-200 rounded-md divide-y divide-gray-200"> +<div class="max-h-96 overflow-y-auto border border-secondary rounded-md divide-y divide-secondary"> ... - <label class="block cursor-pointer hover:bg-gray-50 transition-colors p-3 group relative"> + <label class="block cursor-pointer hover:bg-container-hover transition-colors p-3 group relative"> ... - <%= f.radio_button :reimbursement_id, candidate.id, class: "h-4 w-4 text-indigo-600 border-gray-300 focus:ring-indigo-500" %> + <%= f.radio_button :reimbursement_id, candidate.id, class: "h-4 w-4 text-primary border-secondary focus:ring-primary" %> ... - <span class="text-sm font-medium text-gray-900 truncate block"> + <span class="text-sm font-medium text-primary truncate block"> ... - <span class="text-xs text-gray-500 truncate block"> + <span class="text-xs text-secondary truncate block"> ... - <span class="text-sm font-medium <%= candidate.entry.amount < 0 ? 'text-green-600' : 'text-gray-900' %>"> + <span class="text-sm font-medium <%= candidate.entry.amount < 0 ? 'text-success' : 'text-primary' %>">As per coding guidelines: "Always use functional tokens from design system".
69-79: Use i18n for user-facing strings.The button text and empty state messages should use the
t()helper.🌐 Suggested i18n usage
- <%= f.submit "Link Expense", class: "btn btn--primary w-full" %> + <%= f.submit t("transactions.reimbursement_match.link_button"), class: "btn btn--primary w-full" %> <% else %> <div class="text-center py-4 text-secondary"> - <p>No suitable expense transactions found.</p> + <p><%= t("transactions.reimbursement_match.no_candidates") %></p> <% if `@filter_date.present`? %> - <p class="text-xs mt-1">Showing expenses on <strong><%= `@filter_date` %></strong>.</p> + <p class="text-xs mt-1"><%= t("transactions.reimbursement_match.filtered_by_date", date: `@filter_date`) %></p> <% else %> - <p class="text-xs mt-1">Showing latest 50 expenses.</p> + <p class="text-xs mt-1"><%= t("transactions.reimbursement_match.showing_latest") %></p> <% end %> </div> <% end %>As per coding guidelines: "All user-facing strings must use localization (i18n) via
t()helper".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/models/transaction.rb`:
- Around line 175-187: The reimbursement sign checks in
validate_reimbursement_logic are inverted: when original_expense_id is present,
ensure the child reimbursement (self.entry.amount via entry) is positive (reject
if amount.negative?) and ensure the parent original_expense is negative (access
original_expense.entry.amount and reject if amount.positive?); update the two
conditional tests accordingly and keep the existing errors.add messages and
attribute references (original_expense_id, entry, original_expense,
validate_reimbursement_logic) so legitimate links are allowed per the UI
convention.
🧹 Nitpick comments (1)
app/models/transaction.rb (1)
58-70: Verify indexes backing the new budget-period joins.
for_budget_periodjoins ontransactions.original_expense_idandentries.entryable_id/entryable_type. If the migration didn’t already add supporting indexes (especially ontransactions.original_expense_id), consider adding them to keep budget-mode totals fast. As per coding guidelines: “Optimize database queries with proper indexes to prevent N+1 queries using includes/joins.”
|
(Hey, @CylonN8 ... ran this by Opus 4.5 today, hope you don't mind!) PR #858 Review: Transaction Reimbursement LinkingOverviewThis PR implements a feature to link reimbursement transactions to their original expenses, enabling reimbursements to be credited to the original expense's budget period rather than the month they're received. This addresses a legitimate budget tracking problem. 👍 What Works Well
|
|
Obviously some of this can be deferred to future PRs - and some might be straight up wrong/hallucinations. You know the drill! :-) |
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/views/transactions/show.html.erb`:
- Around line 303-309: The button currently calls
unlink_reimbursement_transaction_path(`@entry.transaction`, reimbursement_id:
`@entry.transaction.id`) which passes the same transaction as both route member
and reimbursement_id; change the route member to the parent expense and keep
reimbursement_id as the child transaction ID so the controller's
reimbursement_id logic is used consistently (e.g. call
unlink_reimbursement_transaction_path(`@entry.transaction.expense`,
reimbursement_id: `@entry.transaction.id`) instead of using `@entry.transaction` as
the first argument).
In `@db/schema.rb`:
- Around line 1349-1357: The transactions table has an original_expense_id
column and index but no foreign key constraint; add a migration that calls
add_foreign_key :transactions, :transactions, column: :original_expense_id,
on_delete: :nullify to enforce referential integrity for the self-reference
(original_expense_id → transactions) and ensure deletes nullify rather than
orphan or break linked records; also consider adding any necessary
nullability/validation changes on the transactions.original_expense_id column if
your model expects non-null or uniqueness constraints.
🧹 Nitpick comments (2)
app/models/budget.rb (1)
286-288: Minor inconsistency: bypasses memoizedincome_statementhelper.Line 287 calls
family.income_statementdirectly, while line 283 uses the memoized privateincome_statementmethod (defined on line 278). This creates an extraIncomeStatementallocation on repeated calls.Suggested fix
def income_totals - `@income_totals` ||= family.income_statement.income_totals(period: period, budget_mode: true) + `@income_totals` ||= income_statement.income_totals(period: period, budget_mode: true) endapp/views/transactions/show.html.erb (1)
271-271: Use design system tokens instead of raw Tailwind color values.Lines 271 and 331 use
border-gray-100, which is a raw Tailwind color. Per the project's design system, use functional tokens likeborder-secondaryorborder-primaryinstead.Similarly, line 358 uses
text-green-600—replace with the appropriate functional token (e.g.,text-successortext-amount-positive, depending on what's defined inmaybe-design-system.css).Suggested fix (partial)
- <div class="p-3 border-t border-gray-100"> + <div class="p-3 border-t border-secondary">- <%= reimbursement.date.to_formatted_s(:short) %> • <span class="text-green-600 font-medium"><%= reimbursement.amount_money.format %></span> + <%= reimbursement.date.to_formatted_s(:short) %> • <span class="text-success font-medium"><%= reimbursement.amount_money.format %></span>As per coding guidelines: "Always use functional tokens from design system (e.g.,
text-primarynottext-white) defined inapp/assets/tailwind/maybe-design-system.css"Also applies to: 331-331
| <%= button_to unlink_reimbursement_transaction_path(@entry.transaction, reimbursement_id: @entry.transaction.id), | ||
| method: :delete, | ||
| class: "btn btn--sm btn--ghost text-secondary hover:text-destructive shrink-0 ml-2", | ||
| title: t("transactions.show.unlink_reimbursement"), | ||
| data: { turbo_frame: "_top" } do %> | ||
| <%= icon "link-2-off", size: "sm" %> | ||
| <% end %> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the unlink_reimbursement controller action
rg -n -A 20 'def unlink_reimbursement' --type=rb app/Repository: we-promise/sure
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Find the routes file and check the unlink_reimbursement route
fd routes.rb app/configRepository: we-promise/sure
Length of output: 156
🏁 Script executed:
#!/bin/bash
# View the show.html.erb file around line 363 to compare usage
cat -n app/views/transactions/show.html.erb | sed -n '355,370p'Repository: we-promise/sure
Length of output: 1210
🏁 Script executed:
#!/bin/bash
# View the show.html.erb file around line 303 to confirm context
cat -n app/views/transactions/show.html.erb | sed -n '298,315p'Repository: we-promise/sure
Length of output: 1074
🏁 Script executed:
#!/bin/bash
# Search for the unlink_reimbursement controller action
rg -n -A 20 'def unlink_reimbursement' --type rbRepository: we-promise/sure
Length of output: 85
🏁 Script executed:
#!/bin/bash
# Find and search in routes files
fd -e rb routes | xargs rg -l 'unlink_reimbursement'Repository: we-promise/sure
Length of output: 75
🏁 Script executed:
#!/bin/bash
# Get more context around line 303 to understand what `@entry` represents
cat -n app/views/transactions/show.html.erb | sed -n '280,315p'Repository: we-promise/sure
Length of output: 2035
🏁 Script executed:
#!/bin/bash
# Get more context around line 363 to understand the section
cat -n app/views/transactions/show.html.erb | sed -n '340,370p'Repository: we-promise/sure
Length of output: 2077
🏁 Script executed:
#!/bin/bash
# Check routes.rb for the unlink_reimbursement route definition
rg -n 'unlink_reimbursement' config/routes.rb -A 2 -B 2Repository: we-promise/sure
Length of output: 202
🏁 Script executed:
#!/bin/bash
# Search for the unlink_reimbursement controller action in transactions controller
fd -e rb app/controllers | xargs rg -l 'unlink_reimbursement'Repository: we-promise/sure
Length of output: 579
🏁 Script executed:
#!/bin/bash
# Get more context above line 280 to understand the `@entry` variable and its structure
cat -n app/views/transactions/show.html.erb | sed -n '260,285p'Repository: we-promise/sure
Length of output: 1513
🏁 Script executed:
#!/bin/bash
# View the controller action unlink_reimbursement
cat -n app/controllers/transactions_controller.rb | grep -A 20 'def unlink_reimbursement'Repository: we-promise/sure
Length of output: 860
🏁 Script executed:
#!/bin/bash
# Get full context around line 363 - what section/conditional is it in?
cat -n app/views/transactions/show.html.erb | sed -n '330,375p'Repository: we-promise/sure
Length of output: 2998
🏁 Script executed:
#!/bin/bash
# Check what comes after line 312 else condition to see the other section context
cat -n app/views/transactions/show.html.erb | sed -n '312,340p'Repository: we-promise/sure
Length of output: 1889
Simplify line 303 to remove redundant parameter passing.
On line 303, unlink_reimbursement_transaction_path(@entry.transaction, reimbursement_id: @entry.transaction.id) passes the same transaction ID twice—as both the route member and the reimbursement_id parameter. The controller action uses reimbursement_id when present (line 361-363), so the route member (params[:id]) is always ignored.
For consistency with line 363 (which passes the expense as the route member and the child as reimbursement_id), line 303 should follow the same pattern. Either pass a dummy/consistent member ID, or unify both call sites to always route from the expense with reimbursement_id identifying the target to unlink.
🤖 Prompt for AI Agents
In `@app/views/transactions/show.html.erb` around lines 303 - 309, The button
currently calls unlink_reimbursement_transaction_path(`@entry.transaction`,
reimbursement_id: `@entry.transaction.id`) which passes the same transaction as
both route member and reimbursement_id; change the route member to the parent
expense and keep reimbursement_id as the child transaction ID so the
controller's reimbursement_id logic is used consistently (e.g. call
unlink_reimbursement_transaction_path(`@entry.transaction.expense`,
reimbursement_id: `@entry.transaction.id`) instead of using `@entry.transaction` as
the first argument).
| t.uuid "original_expense_id" | ||
| t.index ["category_id"], name: "index_transactions_on_category_id" | ||
| t.index ["external_id"], name: "index_transactions_on_external_id" | ||
| t.index ["extra"], name: "index_transactions_on_extra", using: :gin | ||
| t.index ["investment_activity_label"], name: "index_transactions_on_investment_activity_label" | ||
| t.index ["kind"], name: "index_transactions_on_kind" | ||
| t.index ["merchant_id"], name: "index_transactions_on_merchant_id" | ||
| t.index ["original_expense_id"], name: "index_transactions_on_original_expense_id" | ||
| end |
There was a problem hiding this comment.
Missing foreign key constraint for original_expense_id.
The column and index are present, but there's no corresponding add_foreign_key in the foreign keys section (lines 1428–1509). Without it, original_expense_id can reference a deleted or non-existent transaction, causing data integrity issues. The codebase already uses self-referencing FKs (e.g., syncs.parent_id → syncs).
Add a migration with:
add_foreign_key :transactions, :transactions, column: :original_expense_id, on_delete: :nullifyUsing on_delete: :nullify ensures that if the original expense is deleted, linked reimbursements don't break—they simply become unlinked.
Based on learnings: "Enforce null checks, unique indexes, and simple validations in the database schema for PostgreSQL"
🤖 Prompt for AI Agents
In `@db/schema.rb` around lines 1349 - 1357, The transactions table has an
original_expense_id column and index but no foreign key constraint; add a
migration that calls add_foreign_key :transactions, :transactions, column:
:original_expense_id, on_delete: :nullify to enforce referential integrity for
the self-reference (original_expense_id → transactions) and ensure deletes
nullify rather than orphan or break linked records; also consider adding any
necessary nullability/validation changes on the transactions.original_expense_id
column if your model expects non-null or uniqueness constraints.
As discussed in PR #824, there is a case where reimbursements cross budget periods.
For example, if you pay $300 at a restaurant in December for a group of friends, and you get reimbursed $250 in early January, you would expect your December budget to reflect that you only spent $50, not the full $300.
However, with the current behavior, the reimbursement is credited to January’s budget instead. This leaves December appearing over budget and January artificially inflated, even though the reimbursement is directly tied to a December expense.
This PR implements the ability to link a reimbursement transaction to its original expense.
When linked:
Summary by CodeRabbit
New Features
Chores
Bug Fixes
Documentation
Tests