From c219ddab75a398137b6f10183e17704c22353088 Mon Sep 17 00:00:00 2001 From: Baptiste Bouchereau Date: Sat, 22 Aug 2026 14:58:50 +0200 Subject: [PATCH 01/10] Add plan for inline sub category assignment on the transaction list Covers the UI spec for the uncategorized-row cell, the new PATCH endpoint and assigner service, the progressive-enhancement frontend, functional and unit test cases, the dependency decision (none), and the performance and security implications. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q1NqsuwvBdHz19hkzKKmie --- docs/plans/inline-sub-category-assignment.md | 560 +++++++++++++++++++ 1 file changed, 560 insertions(+) create mode 100644 docs/plans/inline-sub-category-assignment.md diff --git a/docs/plans/inline-sub-category-assignment.md b/docs/plans/inline-sub-category-assignment.md new file mode 100644 index 0000000..f278ad3 --- /dev/null +++ b/docs/plans/inline-sub-category-assignment.md @@ -0,0 +1,560 @@ +Inline sub category assignment on the transaction list +===================================================== + +Goal +---- + +Let a user assign a sub category to an **uncategorized** transaction directly from +`/transaction/`, without opening the edit page. + +This is the fast path of the monthly workflow: after an import, the long tail of +transactions that no rule matched has to be categorized one by one. Today that costs +two page loads and a form submit per transaction. + +Scope +----- + +**In scope**: the Sub category cell of rows where `transaction.subCategory is null`. + +**Out of scope for this branch** (deliberately, see [Follow-ups](#follow-ups)): + +* editing the sub category of an already-categorized row (click-to-edit), +* bulk selection / "apply to selected", +* the "create a rule from this" prompt after a save, +* the uncategorized counter and filter shortcut next to the transaction total. + +--- + +1. UI specification +------------------- + +### 1.1 Cell states + +The Sub category cell of an uncategorized row moves through three states. + +``` + Label Amount Created Account Sub category Actions + ────────────────────────────────────────────────────────────────────────────────────────── + ▎Ovh sas - date de valeur… -34.30 02/04/2025 N26 [Select a category ▾] ✎ ⚌ (1) idle + ▎Campus.coach - date de v… -15.00 12/04/2025 N26 [Groceries ▾] ⋯ ✎ ⚌ (2) saving + Ccm loire divatte - date… -20.00 12/04/2025 N26 Groceries ✓ saved · undo ✎ ⚌ (3) saved +``` + +1. **Idle** — a ``, styled `custom-select custom-select-sm` (Bootstrap 4.1 is already + loaded). Native gives free keyboard type-ahead and a real picker on mobile. See + [§6](#6-third-party-libraries) for why not a combobox widget. + +### 1.3 Keyboard flow + +This is the point of the feature and constrains the markup: + +* Every uncategorized row carries a **live, focusable** select — not a click-to-edit + affordance — so `Tab` walks from one row needing work to the next. +* Saving happens on `change`, so the pass is *Tab → type the first letters → Tab*, with no + mouse and no page reload. +* The select must **not** be removed from the tab order while saving, and focus must not be + stolen or reset by the response handler. + +### 1.4 Row-level marker + +Rows with no sub category get a `.uncategorized-transaction` class and an amber left +border in `public/styles/main.css`. `templates/transaction/import/validate_transactions.html.twig` +already sets `.existing-transaction` / `.new-transaction` row classes, so this follows an +existing convention. + +The class is removed client-side on a successful save, and restored on undo. + +### 1.5 No reflow + +The usual entry point for a categorizing pass is the existing `Categorized: No` filter — at +which point every row the user categorizes stops matching the active filter. The row must +**not** be removed, hidden, or re-sorted. It stays where it is, dimmed, and disappears only +on the next page load. Reflowing the list under the cursor mid-pass is how people lose +their place and mis-assign the next row. + +--- + +2. Backend +---------- + +### 2.1 New route + +One route, on `TransactionController`: + +```php +#[Route('/{id}/sub-category', name: 'transaction_set_sub_category', methods: ['PATCH'])] +public function setSubCategory( + Request $request, + Transaction $transaction, + TransactionSubCategoryAssigner $assigner, +): Response +``` + +`PATCH` matches the existing convention (`transaction_categorize` and `elasticsearch_export` +are both `PATCH`), and `framework.http_method_override` is already `true`, so the plain-form +fallback can reach it via a `_method` hidden input. + +The path segment `sub-category` cannot collide with `transaction_delete` (`/{id}`, +`DELETE`) or `transaction_edit` (`/{id}/edit`). + +### 2.2 Request contract + +| Parameter | Required | Meaning | +|---|---|---| +| `_token` | yes | CSRF token, id `set-sub-category` | +| `subCategory` | yes | Sub category UUID, or empty string to clear (undo) | + +No Symfony Form type is used. A form would bind by field name and invites accidental +widening later; here exactly one property may change and the controller reads exactly one +parameter. See [§8.3](#83-mass-assignment). + +### 2.3 New service: `App\Services\TransactionSubCategoryAssigner` + +```php +public function assign(Transaction $transaction, ?SubCategory $subCategory): void +``` + +Responsibilities: + +1. set the sub category on the transaction, +2. if `TransactionDiffChecker::subCategoryChanged()` reports a change, set + `categorizedManually` to `$transaction->isCategorized()`, +3. **validate** the entity (see 2.4) and throw a typed exception on violation, +4. flush. + +**Why a service rather than inline controller code.** The `categorizedManually` rule is +already duplicated between `TransactionController::new()` and `TransactionController::edit()` +in slightly different forms; a third inline copy is the point at which the rule stops being +maintainable. It also gives the unit tests in [§5.2](#52-unit-tests) a real target. This +matches the existing service style (`TransactionCategorizer`, `TransactionDiffChecker`, +`RuleChecker`). + +This branch wires only the new action to the service. Migrating `new()` and `edit()` onto it +is a separate, mechanical change — noted in [Follow-ups](#follow-ups) to keep this diff +reviewable. + +### 2.4 Validation — and why it is mandatory + +`Transaction::checkSubCategory()` is a `#[ORM\PreUpdate]` callback that throws a **raw +`\Exception`** when the sub category's transaction type does not match the transaction's. +If an invalid pair reaches `flush()`, the result is an uncaught exception and a 500 with no +usable response body. + +So the assigner must validate **before** flushing, using the existing +`TransactionSubCategoryIsLogicalConstraint` via `ValidatorInterface`, and surface the +translated violation message. The constraint validator already calls `checkSubCategory()` +and catches the exception, so no new validation logic is needed — only that it is actually +run on this path. + +### 2.5 Response contract + +`Accept: application/json` (what the JS sends) → `JsonResponse`: + +| Status | Body | Case | +|---|---|---| +| `200` | `{"id": "...", "subCategory": {"id": "...", "name": "Groceries"}, "categorized": true}` | assigned | +| `200` | `{"id": "...", "subCategory": null, "categorized": false}` | cleared (undo) | +| `403` | `{"error": ""}` | invalid CSRF token | +| `404` | — | unknown transaction id (handled by the argument resolver) | +| `422` | `{"error": ""}` | unknown sub category id, or type mismatch | + +Otherwise (the no-JavaScript fallback, which posts a normal form) → redirect back to +`transaction_index`, preserving the current query string so filters and page number +survive. On error, put the message in the session flash bag. + +Content negotiation on `$request->getPreferredFormat()` / the `Accept` header keeps both +paths on one action. + +`JsonResponse` from HttpFoundation is enough — see [§6](#6-third-party-libraries). + +### 2.6 Elasticsearch + +Nothing to do, but worth recording so nobody adds a guard "just in case": + +`ElasticsearchSyncStatusUpdater::onFlush()` only flips the `toSyncInElasticsearch` boolean; +the only listener that actually talks to Elasticsearch is +`ElasticsearchTransactionRemover`, bound to `preRemove`. An update therefore has **no +external dependency** and cannot fail because Elasticsearch is down. The transaction is +correctly re-flagged for the next export. + +--- + +3. Frontend +----------- + +### 3.1 Progressive enhancement + +Each uncategorized cell renders a real, self-sufficient form: + +```twig +
+ + + + +
+``` + +The JS module **hides the button on successful init** rather than the template wrapping it in +`