Skip to content

feat(budget): attach receipts and invoices to expenses - #2268

Merged
mauriceboe merged 3 commits into
liketrek:devfrom
dersimoezdag:feat/receipt-uploads-budget
Sep 8, 2026
Merged

feat(budget): attach receipts and invoices to expenses#2268
mauriceboe merged 3 commits into
liketrek:devfrom
dersimoezdag:feat/receipt-uploads-budget

Conversation

@dersimoezdag

Copy link
Copy Markdown

Description

Allows attaching receipts and invoices (images & PDFs) directly to expenses in both Desktop and Mobile views.

  • Users can upload multiple receipts or invoices per expense via drag-and-drop or file picker.
  • Displays an receipt chip ([ 🧾 Beleg ] / [ 🧾 Belege (3) ]) in the expense ledger rows.
  • Clicking the chip opens an in-app viewer supporting both images and PDFs, complete with tab switching and keyboard navigation (← / →) for multi-receipt expenses.
  • Automatically soft-deletes orphan receipts to the trip trash when an expense is deleted or an attachment is unlinked, preventing orphaned receipt files.

Related Issue or Discussion

none

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • I have read the Contributing Guidelines
  • My branch is up to date with dev
  • This PR targets the dev branch, not main (wiki-only PRs are exempt)
  • I have tested my changes locally
  • I have added/updated tests that prove my fix is effective or that my feature works
  • I have updated documentation if needed

@mauriceboe

Copy link
Copy Markdown
Collaborator

Thanks for this, and for building it the right way round. You hung it on the existing trip_files / file_links infrastructure instead of opening a second upload path, so the storage driver, the size and extension limits, the trash and the signed downloads all come along for free. Desktop and mobile both get it, the viewer with tabs and arrow keys is a nice touch, and cleaning up orphaned receipts when the expense goes is the part people usually forget. The direction is right and I want this in.

A few things need to happen before it can go in, and none of the CI jobs that would have told you have run yet (only check-target did, so lint, typecheck, tests and i18n parity never fired on this branch).

Blocking

  1. The client typecheck fails. BudgetItemReceipt is used in MCostsTab.tsx:82 and :557 but never imported, so cd client && npm run typecheck dies with TS2304 and the branch does not build.

  2. budget_item_id never reaches the upload handler. You extended FilesService.createFile / updateFile / findForeignLinkTarget, but files.controller.ts:151 and :164 still pass only reservation_id and place_id. The client's fd.append('budget_item_id', ...) is silently dropped: the file uploads but is neither written to trip_files.budget_item_id nor linked. It looks like it works in the edit flow only because saving re-links through receipt_file_ids afterwards. The new cross-trip check has the same gap: it runs on the link route but not on upload or update.

  3. i18n parity will fail. Nine new keys landed in en and de only; the other 21 locales need them, and i18n:parity:strict is a hard gate. Three of the nine (costs.receipt, costs.receiptCount, costs.uploadReceipt) look unused, worth dropping before translating them 23 times.

Worth fixing while you are in there

  1. Orphaned uploads. On create, files upload before the expense exists, so a failed addBudgetItem leaves them unlinked and no cleanup path reaches them (trashOrphanReceipt only runs from an existing item). In the edit modal it is the mirror image: the upload happens on selection, and Cancel leaves the file behind.

  2. Coverage. About 190 new server lines with one test. budget is pinned at 95 lines and files at 98 in server/vitest.config.ts, so this will drop below. trashOrphanReceipt's four bail-out branches, the update sync and the delete path are the ones that matter.

  3. Index drift. schema.ts creates trip_files.budget_item_id on fresh databases, but the migration creates idx_trip_files_budget_item_id inside the pragma_table_info guard. A fresh install gets the column and never the index; an upgrade gets both.

  4. The relationship is modelled twice, once as trip_files.budget_item_id and once as a file_links row, which is why the reads need a COALESCE plus DISTINCT plus a manual dedupe and why five write sites have to stay in step by hand. Not a blocker, but if you want to simplify, file_links alone would carry it.

Happy to look again once CI is green on it.

@mauriceboe mauriceboe added the waiting for response Blocked, needs input from author or user label Sep 6, 2026
@dersimoezdag
dersimoezdag force-pushed the feat/receipt-uploads-budget branch from 1767e1c to c6befa0 Compare September 6, 2026 16:19
@dersimoezdag

Copy link
Copy Markdown
Author

Thanks for the nice review and helpful feedback! I've addressed all issues and your suggestions in the latest push:

1. Client Typecheck (BudgetItemReceipt)

  • Added the missing import BudgetItemReceipt in MCostsTab.tsx. The client code now builds and typechecks cleanly with zero errors in feature files.

2. Files Controller Wiring & Cross-Trip Checks

  • Passed budget_item_id through to createFile and updateFile in FilesController.
  • Wired assertLinkTargets across upload, update, and link endpoints to prevent cross-trip IDOR when referencing budget items.

3. i18n Strict Parity

  • I removed the 3 unused keys (costs.receipt, costs.receiptCount, costs.uploadReceipt).
  • Synchronized the 6 active receipt keys across all 23 locale files (shared/src/i18n/*/budget.ts).
  • npm run i18n:parity:strict passes cleanly.
  • Translated the keys with help of ai.

4. Staged Uploads / No More Orphaned Files

  • Implemented a staging pattern (pendingReceiptFiles) on both Desktop (CostsPanel.tsx) and Mobile (MCostSheet.tsx).
  • Selected files are kept local in state and are only uploaded when the user actually hits "Save".
  • Clicking "Cancel" cleanly discards the selection without touching the server/storage.
  • If an error occurs during the submit pipeline after upload, any newly uploaded files are permanently deleted as a rollback.

5. Dual-Modeling & Index Drift Cleaned Up

  • Dropped the duplicate budget_item_id column from trip_files in schema.ts and types.ts.
  • Receipt linking is now exclusively modeled via file_links.budget_item_id, eliminating duplicate write sites and queries needing COALESCE/DISTINCT.
  • Updated migration 206 to only add budget_item_id and the corresponding index to file_links.

6. Test Coverage

  • Added full unit and DB test suites in budget.service.db.test.ts and files.controller.test.ts:
  • Covered all 4 bail-out branches in trashOrphanReceipt (non-existent, legacy link, external link, and orphan cleanup).
  • Tested receipt linking on create, sync on update, and cleanup on expense deletion.
  • Added positive & cross-trip negative assertion tests for the files controller.

The branch has been updated with these fixes. Ready for another look whenever you have time!

dersimoezdag and others added 2 commits September 8, 2026 21:01
Attaching a receipt no longer lets the budget domain delete one. Trashing the
file was reachable from the REST route, the MCP tool and the plugin RPC, none
of which check file_delete, and a receipt id is any file on the trip, so anyone
who could edit an expense could trash a document they were not allowed to
touch. A concurrent edit could do it by accident: the client sends the full set
of receipt ids from a snapshot taken when the form opened, so saving an expense
someone else had just attached a file to moved that file to the trash. Removing
a receipt now removes the link and nothing else.

The link row is treated as shared, because it is. One row can tie a file to a
place and to an expense at once, so the sync clears only the receipt half and
leaves the rest, the foreign key is SET NULL rather than CASCADE, and a receipt
already in the trash keeps its link so restoring it comes back attached.

The upload and its rollback moved into one module both shells call. It was
copied into six places, and all six rolled back with the permanent-delete
route, which only accepts a file that is already in the trash and therefore
answered 404 every time: a failed save left every uploaded file on the trip,
attached to the expense, and the next attempt uploaded another copy. It now
trashes first, keeps the pending files for the retry, and says so when a file
could not be removed instead of swallowing it.

Escape in the receipt viewer no longer closes the expense form behind it, the
download fallback uses a real accent class rather than a utility that does not
exist, and the upload indicator is actually switched on.

Tests: the receipt link behaviour on both service paths, the four new branches
in the files service, the upload module, and the viewer.
@mauriceboe
mauriceboe force-pushed the feat/receipt-uploads-budget branch from c6befa0 to 3b192a2 Compare September 8, 2026 19:38
listFiles projects linked_budget_item_ids now, so the harness table it selects
from needs budget_item_id or every list call in that file answers 500. Same
gap the collab harness had.
@mauriceboe
mauriceboe merged commit 4611f0a into liketrek:dev Sep 8, 2026
13 checks passed
@mauriceboe mauriceboe mentioned this pull request Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting for response Blocked, needs input from author or user

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants