From 6cdd55bd7bf48439427a97599f9c6f316bdd5017 Mon Sep 17 00:00:00 2001 From: Norair Arutshyan Date: Sat, 19 Sep 2026 16:56:10 +0100 Subject: [PATCH] feat(shared): complete audit-mixin-auto-population groups 3 & 4 Budget (group 3): test proves update_budget's previously-stale updated_by is now kept fresh by the automatic listener, not manual CRUD code. Users (group 4): regression tests confirm BugReportModel and DonorGranteeModel now get created_by/updated_by populated for free through their real routes/JWT chain, with no CRUD changes needed. Also carries along the updated OpenSpec workflow conventions doc (sub-issues, branch naming) and config.yaml, plus two freshly staged proposals (budget-fix-295, frontend-fix-294) queued for later work. Co-Authored-By: Claude Sonnet 5 --- docs/development/WORKFLOW.md | 87 +++++++++++++ .../audit-mixin-auto-population/tasks.md | 10 +- .../.openspec.yaml | 5 + .../design.md | 33 +++++ .../proposal.md | 26 ++++ .../tasks.md | 10 ++ .../.openspec.yaml | 5 + .../design.md | 23 ++++ .../proposal.md | 25 ++++ .../tasks.md | 9 ++ openspec/config.yaml | 8 +- services/budget/tests/test_budget_crud.py | 33 ++++- .../users/tests/test_audit_trail_routes.py | 120 ++++++++++++++++++ 13 files changed, 385 insertions(+), 9 deletions(-) create mode 100644 docs/development/WORKFLOW.md create mode 100644 openspec/changes/budget-fix-295-category-cascade-delete-fix/.openspec.yaml create mode 100644 openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md create mode 100644 openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md create mode 100644 openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md create mode 100644 openspec/changes/frontend-fix-294-budget-delete-confirm-click/.openspec.yaml create mode 100644 openspec/changes/frontend-fix-294-budget-delete-confirm-click/design.md create mode 100644 openspec/changes/frontend-fix-294-budget-delete-confirm-click/proposal.md create mode 100644 openspec/changes/frontend-fix-294-budget-delete-confirm-click/tasks.md create mode 100644 services/users/tests/test_audit_trail_routes.py diff --git a/docs/development/WORKFLOW.md b/docs/development/WORKFLOW.md new file mode 100644 index 00000000..2384d567 --- /dev/null +++ b/docs/development/WORKFLOW.md @@ -0,0 +1,87 @@ +# OpenSpec → Issue → Branch Workflow + +How a piece of work moves from an OpenSpec change to merged code, and how the +naming stays consistent end to end. + +## 1. OpenSpec change naming + +Change directories (`openspec/changes//`) are a flat identifier to the +`openspec` CLI, so the name stays a single kebab-case string — no slashes: + +``` +--- +``` + +Example: `shared-feat-292-audit-mixin-auto-population` + +- **service** — lowercase, one of: `ai`, `backend`, `budget`, `chat`, + `feature`, `frontend`, `platform`, `shared`, `users` (same set used in + branch names, see below). +- **type** — `feat` | `fix` | `chore` | `refactor`, matching the commit + prefixes already used in this repo (not GitHub's `bug` label). + - `feat` — new capability + - `fix` — bug fix + - `chore` — infra/tooling/deps, no behavior change users would notice + - `refactor` — structural change, no behavior change +- **issue** — the GitHub issue number of the **parent** issue for this + whole change. Its only job is to hold the per-group sub-issues (§3) — it + doesn't carry its own task list. +- **description** — short kebab-case summary. + +**Ordering with the issue:** a change is often proposed before a ticket +exists. Flow is: + +1. `/opsx:propose` with a temporary kebab-case name (no issue number yet). +2. Once `tasks.md` exists and the scope is real, create the parent ticket: + `scripts/new-issue.sh "" "<body>"`. +3. Rename the change directory to fold in the issue number: + `mv openspec/changes/{<temp-name>,<service>-<type>-<issue>-<description>}`. + +## 2. Branch naming + +``` +<Service>/<type>/Issue-<sub-issue>/<description>[-group<N>] +``` + +`Service` is title-cased (`Shared`, `Budget`, `Frontend`, `AI`, ...); `type` +stays lowercase, taken straight from the OpenSpec change name. `<sub-issue>` +is that **group's own** sub-issue number (§3), not the parent's — e.g. +`Shared/feat/Issue-301/audit-mixin-auto-population-group2`. + +## 3. Task groups get their own sub-issue + +Each `tasks.md` group is tracked as a real GitHub sub-issue of the parent +(GitHub's native sub-issue relationship, not just a text mention) — created +automatically the first time a group is started via `scripts/start-group.sh`: + +- Title: `<description>: <group title> (group N)`. +- Body links back to the parent issue and the group's `tasks.md` section. +- Linked to the parent through GitHub's sub-issues API, so it shows up as a + checklist/progress bar on the parent issue. +- Added to project board 8. +- The sub-issue number is written back onto the group's header line in + `tasks.md` (`## N. Title — Issue #<sub-issue>`), so re-running the script + for the same group reuses it instead of creating a duplicate. + +## 4. Triggering a task group's branch + +Don't rely on remembering to create the sub-issue and branch correctly at +each group boundary — run: + +``` +scripts/start-group.sh <change-name> <group-number> +``` + +First run for a group: creates its sub-issue (as in §3), links it under the +parent, and derives the branch from it. Later runs for the same group: reads +the sub-issue back out of `tasks.md` instead of recreating it. Either way it +creates and checks out the branch and flips the sub-issue's project item to +**In Progress**. Refuses to run if the change name doesn't parse or the +group doesn't exist in `tasks.md`. + +## 5. PR / issue-closing convention + +Each group's PR closes its own sub-issue: `Closes #<sub-issue>` (fires the +board's Done automation for that item). Once every sub-issue under the +parent is closed, close the parent too — it's just a tracking issue at that +point. diff --git a/openspec/changes/audit-mixin-auto-population/tasks.md b/openspec/changes/audit-mixin-auto-population/tasks.md index e8e713c6..6ebdf65d 100644 --- a/openspec/changes/audit-mixin-auto-population/tasks.md +++ b/openspec/changes/audit-mixin-auto-population/tasks.md @@ -21,11 +21,11 @@ One task group = one GitHub ticket = one PR, merged before the next group starts - [x] 3.1 Resolved at design stage (design.md Decision 5): manual assignments confirmed redundant/in-sync with the automatic listener across all 4 services, no on-behalf-of divergence found. No per-call-site removal needed. - [ ] 3.2 (dropped — manual assignments stay in place per Decision 5; not removed) -- [ ] 3.3 Add/update a test proving the previously-stale-`updated_by` bug is fixed: create a row as user A, update it as user B, assert `updated_by` now equals B (not still A). -- [ ] 3.4 Run `services/budget`'s test suite clean; PR merged. +- [x] 3.3 Add/update a test proving the previously-stale-`updated_by` bug is fixed: create a row as user A, update it as user B, assert `updated_by` now equals B (not still A). (`services/budget/tests/test_budget_crud.py::TestUpdateBudgetAuditTrail` — exercises `update_budget`, which never sets `updated_by` itself, proving the automatic listener is what fixes it.) +- [x] 3.4 Run `services/budget`'s test suite clean; PR merged. (370 passed [+1 net new]. PR not yet opened.) ## 4. Enable in users service — depends on 1, 2 -- [ ] 4.1 Verify `DonorGranteeModel` and `BugReportModel` (both already use `AuditMixin` but currently leave the columns `NULL`) now get `created_by`/`updated_by` populated automatically with no CRUD changes needed. -- [ ] 4.2 Add regression tests for both models asserting `created_by` is populated on creation via their existing routes. -- [ ] 4.3 Run `services/users`'s test suite clean; PR merged. +- [x] 4.1 Verify `DonorGranteeModel` and `BugReportModel` (both already use `AuditMixin` but currently leave the columns `NULL`) now get `created_by`/`updated_by` populated automatically with no CRUD changes needed. (Confirmed: `create_bug_report`/`create_donor_grantee` never set `created_by`/`updated_by`; the automatic listener now fills both with no crud.py changes.) +- [x] 4.2 Add regression tests for both models asserting `created_by` is populated on creation via their existing routes. (`services/users/tests/test_audit_trail_routes.py` — real JWT through the actual routes, not `make_client`'s `get_validated_user` override, which would bypass the contextvar-setting code being tested.) +- [x] 4.3 Run `services/users`'s test suite clean; PR merged. (196 passed [+2 net new]; `shared` re-run clean too — 117 passed, 1 pre-existing unrelated worktree-scanning failure. PR not yet opened.) diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/.openspec.yaml b/openspec/changes/budget-fix-295-category-cascade-delete-fix/.openspec.yaml new file mode 100644 index 00000000..b99e4532 --- /dev/null +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/.openspec.yaml @@ -0,0 +1,5 @@ +schema: spec-driven +created: 2026-09-19 +author: Norair Arutshyan +priority: high +skip_specs: true diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md new file mode 100644 index 00000000..aaebfd85 --- /dev/null +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md @@ -0,0 +1,33 @@ +# Design + +## Context + +See proposal.md - Why for the failure mode. Relevant existing state: + +- `budget_categories.budget_id` has a DB-level FK with `ondelete="CASCADE"` (`services/budget/migrations/versions/000014_scope_budget_categories_to_budget.py`), added deliberately when categories were scoped one-per-budget — the DB already expects categories to disappear when their budget does. +- `BudgetModel.categories` (`services/budget/app/models/budget.py`) is a plain `relationship(..., back_populates="budget")` with no `cascade` and no `passive_deletes` setting. +- `delete_budget_service` (`services/budget/app/services/budget_services.py`) calls `get_budget_service`, which does **not** eager-load `categories` in this path, then calls `session.delete(budget)` + `commit()`. +- Without `passive_deletes=True`, SQLAlchemy's unit-of-work treats "this budget is being deleted" as "I must disassociate its `categories` collection" — since there's no delete cascade configured, it lazy-loads the (unloaded) collection during flush and issues an `UPDATE budget_categories SET budget_id = NULL ...` for each row, which the DB rejects (`budget_id` is `nullable=False`), surfacing as `IntegrityError` → the generic 400. +- Because the collection load happens inside `flush()`, which the SQLAlchemy asyncio extension runs inside a greenlet, this doesn't crash as a `MissingGreenlet` — it produces a normal `IntegrityError`, so it slipped past the `async-persistence` capability's existing "no implicit lazy-load" guard without tripping any greenlet-specific detection. + +## Goals / Non-Goals + +**Goals:** +- Make `DELETE /api/v1/budgets/{id}` succeed for a budget whose only remaining "problem" is a category row (no lines, reports, receipts, or currency conversions). +- Keep the DB as the single source of truth for the cascade, rather than teaching the ORM to duplicate it. + +**Non-Goals:** +- Cleaning up orphaned `budget_categories` rows when a line is deleted (separate, pre-existing bookkeeping gap; not required for this fix — see proposal.md). +- Changing cascade behavior for `budgets.lines` or `budgets.reports` — those intentionally block budget deletion via `IntegrityError` today (a budget with real lines/reports/receipts should not be silently hard-deletable), and that behavior is unaffected by this change. + +## Decisions + +**Use `passive_deletes=True` on `BudgetModel.categories`, not ORM-level `cascade="all, delete-orphan"`.** +- `passive_deletes=True` tells SQLAlchemy "don't manage this relationship's deletes yourself, the DB's `ON DELETE CASCADE` already does it" — it skips the lazy-load-and-null-out step entirely and just issues `DELETE FROM budgets WHERE id = ...`, letting Postgres cascade. +- Alternative considered: `cascade="all, delete-orphan"`. This would make SQLAlchemy load the collection and emit an explicit `DELETE FROM budget_categories WHERE id IN (...)` before deleting the budget — functionally similar end state, but it still requires the lazy-load (same eager-load discipline the `async-persistence` capability calls for) and does redundant work the DB already does via its own `ON DELETE CASCADE`. `passive_deletes=True` is the smaller, more precise fix that matches the existing DB-level intent from migration `000014`. +- `passive_deletes=True` requires the DB constraint to actually be `ON DELETE CASCADE` (true here, confirmed in the migration) — it would be the wrong choice if the constraint were `RESTRICT`/`NO ACTION`, since then rows would just silently fail to delete. + +## Risks / Trade-offs + +- [`passive_deletes=True` silently relies on the DB constraint matching model expectations; if a future migration changes `budget_categories_budget_id_fkey` back to `RESTRICT` without updating the model, budget deletion would start failing again with a real (unmasked) `IntegrityError`] → Acceptable: that failure mode is the same generic `IntegrityError` → 400 path already in place for lines/reports, so it fails safe (blocks deletion, doesn't corrupt data) rather than silently. +- [Orphaned categories with zero lines still accumulate until their budget is deleted] → Out of scope per Non-Goals; tracked separately as a known bookkeeping gap, not a correctness or security issue. diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md new file mode 100644 index 00000000..6e038454 --- /dev/null +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md @@ -0,0 +1,26 @@ +# Proposal + +## Why + +`DELETE /api/v1/budgets/{id}` returns a 400 ("Budget cannot be deleted while it has existing reports, funding receipts, or currency conversions") for budgets that have none of those, whenever a budget line was ever created and later deleted on that budget. This blocks legitimate budget deletion and was caught by the `auth-budget-chain` e2e spec's DELETE-budget step in CI, but it is a real product bug, not a test bug — any grantee who adds then removes a line before deleting a draft budget hits it. + +## What Changes + +- `BudgetModel.categories` gets `passive_deletes=True` so SQLAlchemy's ORM defers to the database's existing `ON DELETE CASCADE` on `budget_categories.budget_id` instead of trying to lazy-load the collection and null out each child's non-nullable `budget_id` during flush (which raises the spurious `IntegrityError`). +- Out of scope: cleaning up an orphaned category when its last line is deleted is a separate, pre-existing bookkeeping gap (also related to the already-known global-category-namespace issue) and is not needed to fix this bug — `passive_deletes=True` makes budget deletion correct regardless of whether an orphaned category row exists. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +None — this restores compliance with the already-declared `async-persistence` requirement ("No lazy-load MissingGreenlet risk": every relationship a route/service reads SHALL be explicitly eager-loaded or never accessed outside an awaited context, and no request path SHALL trigger an implicit lazy-load) and the `budget-categories` requirement that every category belongs to exactly one budget without ever becoming a dangling reference; it does not change either requirement's text. + +## Impact + +- `services/budget/app/models/budget.py` (`BudgetModel.categories` relationship) +- Fixes `DELETE /api/v1/budgets/{id}` for any budget that ever had a line added and removed +- Unblocks `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` in the `E2E Tests` CI workflow diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md new file mode 100644 index 00000000..f8215ad8 --- /dev/null +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md @@ -0,0 +1,10 @@ +# Tasks + +Workflow rule: one task group = one GitHub sub-issue (of this change's parent issue) = one PR, merged before the next group starts. + +## 1. Fix budget deletion cascade for orphaned categories + +- [ ] 1.1 Add `passive_deletes=True` to `BudgetModel.categories` in `services/budget/app/models/budget.py` +- [ ] 1.2 Add/extend a `services/budget` test that creates a budget, adds a line (auto-creating a category), deletes the line, then deletes the budget, and verify `DELETE /budgets/{id}` returns 200 with `success: true` instead of a 400 +- [ ] 1.3 Run `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` against the local docker-compose e2e stack (or the `E2E Tests` CI workflow) and verify the DELETE-budget assertion passes +- [ ] 1.4 Run `services/budget`'s test suite and lint clean; PR merged diff --git a/openspec/changes/frontend-fix-294-budget-delete-confirm-click/.openspec.yaml b/openspec/changes/frontend-fix-294-budget-delete-confirm-click/.openspec.yaml new file mode 100644 index 00000000..1d1d9444 --- /dev/null +++ b/openspec/changes/frontend-fix-294-budget-delete-confirm-click/.openspec.yaml @@ -0,0 +1,5 @@ +schema: spec-driven +created: 2026-09-19 +author: Norair Arutshyan +priority: low +skip_specs: true diff --git a/openspec/changes/frontend-fix-294-budget-delete-confirm-click/design.md b/openspec/changes/frontend-fix-294-budget-delete-confirm-click/design.md new file mode 100644 index 00000000..5f5915fb --- /dev/null +++ b/openspec/changes/frontend-fix-294-budget-delete-confirm-click/design.md @@ -0,0 +1,23 @@ +# Design + +## Context + +See proposal.md - Why. This is a single-file, single-method fix in a Playwright page object; none of the triggers for a fuller design doc (cross-cutting change, new dependency, data model change, security/perf/migration complexity) apply here. + +## Goals / Non-Goals + +**Goals:** +- Make `BudgetListPage.deleteBudget()` actually complete the delete flow the UI requires (click delete, then confirm). + +**Non-Goals:** +- Changing the underlying `ConfirmDeleteButton` UX or the archive-vs-hard-delete product behavior. +- Adding a dedicated `confirmDelete()` page-object method — one extra click inline is proportionate to the size of this fix. + +## Decisions + +- Add the "Yes" click directly inside `deleteBudget()` rather than exposing a separate confirmation step, since every current caller of `deleteBudget()` wants the delete to fully complete and there's no case where a caller wants to stop at "pending confirmation". +- Locate the "Yes" button scoped to the same row (`this.row(name).getByRole("button", { name: "Yes" })`) rather than a bare page-level `getByRole`, so the click can't accidentally hit a Yes/No pair open on a different row. + +## Risks / Trade-offs + +- [Confirmation button text changes later] → Low risk; it's a static UI string in `ConfirmDeleteButton`'s own render, easy to keep in sync since a text change would need a page-object update anyway for the "Delete budget" title too. diff --git a/openspec/changes/frontend-fix-294-budget-delete-confirm-click/proposal.md b/openspec/changes/frontend-fix-294-budget-delete-confirm-click/proposal.md new file mode 100644 index 00000000..83d54999 --- /dev/null +++ b/openspec/changes/frontend-fix-294-budget-delete-confirm-click/proposal.md @@ -0,0 +1,25 @@ +# Proposal + +## Why + +The browser e2e spec (`auth-budget-crud.spec.ts`) fails in CI on its final assertion: after calling `BudgetListPage.deleteBudget()`, the budget row is expected to disappear but never does. The helper only clicks the delete icon once, which merely reveals a Yes/No confirmation — it never confirms, so no delete/archive ever happens and the test fails on a false negative unrelated to any product bug. + +## What Changes + +- `BudgetListPage.deleteBudget()` clicks the "Delete budget" icon button, then clicks the resulting "Yes" confirmation button, so the archive mutation actually fires before the test asserts the row is gone. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +None — this fixes the `e2e-testing` capability's existing browser CRUD journey requirement (delete step) to actually execute as already specified; it does not change what that requirement says. + +## Impact + +- `frontend-typescript/e2e/pages/BudgetListPage.ts` (test helper only) +- No production/application code changes +- Unblocks the `browser` project's CRUD journey test in the `E2E Tests` CI workflow diff --git a/openspec/changes/frontend-fix-294-budget-delete-confirm-click/tasks.md b/openspec/changes/frontend-fix-294-budget-delete-confirm-click/tasks.md new file mode 100644 index 00000000..d02a9b85 --- /dev/null +++ b/openspec/changes/frontend-fix-294-budget-delete-confirm-click/tasks.md @@ -0,0 +1,9 @@ +# Tasks + +Workflow rule: one task group = one GitHub ticket = one PR, merged before the next group starts. + +## 1. Fix delete confirmation click in BudgetListPage + +- [ ] 1.1 Update `BudgetListPage.deleteBudget()` in `frontend-typescript/e2e/pages/BudgetListPage.ts` to click the "Yes" confirmation button (scoped to the same row) after clicking "Delete budget", and verify by reading the updated method against `ConfirmDeleteButton`'s rendered Yes/No markup +- [ ] 1.2 Run `frontend-typescript/e2e/specs/browser/auth-budget-crud.spec.ts` against the local docker-compose e2e stack and verify the delete-budget assertion (`toHaveCount(0)`) passes +- [ ] 1.3 Run the full e2e suite (`api` + `browser` projects) locally or via the `E2E Tests` CI workflow, confirm no regressions; PR merged diff --git a/openspec/config.yaml b/openspec/config.yaml index 72be15dd..f569a5b8 100644 --- a/openspec/config.yaml +++ b/openspec/config.yaml @@ -5,10 +5,11 @@ schema: spec-driven context: | Repo: GrantFlow (nonprofit grant budgeting/reporting platform). Services: services/budget, services/users, services/ai, services/chat (FastAPI); frontend-typescript (React + TypeScript, React Query); nginx gateway proxying /api/v1/<prefix>/ to each service. - GitHub tickets are created with scripts/new-issue.sh; branch naming is <ServiceName>/Issue-<number>/<short-desc> (ServiceName in Platform, Frontend, Shared, Budget, Users, AI, Chat). + OpenSpec change dir naming (see docs/development/WORKFLOW.md for the full convention): <service>-<type>-<issue>-<description>, flat kebab-case, e.g. shared-feat-292-audit-mixin-auto-population. service in ai, backend, budget, chat, feature, frontend, platform, shared, users; type in feat, fix, chore, refactor. Propose under a temp kebab-case name if the parent GitHub issue doesn't exist yet, then rename the dir once scripts/new-issue.sh creates it. Do NOT prefix the dir with a priority label (HIGH/MED/LOW) — priority lives only in .openspec.yaml's `priority` field. + Each tasks.md task group becomes its own GitHub sub-issue of that parent, created and linked via scripts/start-group.sh (not by hand) when the group's implementation starts. + Branch naming: <Service>/<type>/Issue-<sub-issue>/<description>[-group<N>] (Service title-cased: Platform, Frontend, Shared, Budget, Users, AI, Chat, Backend, Feature; type lowercase, same as the change's type; sub-issue is that group's own sub-issue number, not the parent's) — created by scripts/start-group.sh, not by hand. Code comments: keep to short one- or two-liners explaining WHY, not WHAT; no multi-line rationale blocks in source files. Never `git commit`, `git push`, or open a PR without the user's explicit go-ahead for that specific action — a prior approval (e.g. "commit and push this one") authorizes only that instance, not standing permission for later commits/pushes in the same session or change. - Priority convention: `.openspec.yaml` gets `priority: HIGH|MED|LOW`, and the change dir is prefixed to match, e.g. `HIGH-fix-registration-privilege-escalation`. Default MED. # Per-artifact rules (optional) # Add custom rules for specific artifacts. @@ -16,7 +17,8 @@ rules: proposal: - Keep proposals under 500 words tasks: - - "Workflow rule: one task group = one GitHub ticket = one PR, merged before the next group starts. State this rule verbatim as the first line of tasks.md, above the first '## 1.' heading." + - "Workflow rule: one task group = one GitHub sub-issue (of this change's parent issue) = one PR, merged before the next group starts. State this rule verbatim as the first line of tasks.md, above the first '## 1.' heading." + - "Every group's FIRST task (e.g. '1.0', '2.0') must be: 'Run `scripts/start-group.sh <change-name> <N>` to create/link this group's sub-issue and branch before starting any other work in this group.' Do not start on the rest of a group's tasks until this one is checked off." - Structure task groups as independently mergeable vertical slices (not horizontal implementation layers), ordered by dependency, so each group's PR is real, reviewable, shippable work on its own. - Every group's final task must be "Run <the project's tests/lint for the affected area> clean; PR merged" — do not bundle a separate cross-cutting "wrap-up" group at the end; fold final integration/manual-verification tasks into the last functional group instead. - Note in each group's heading which earlier group(s) it depends on (e.g. "## 3. <name> — ticket depends on 1, 2"), so the merge order is unambiguous. diff --git a/services/budget/tests/test_budget_crud.py b/services/budget/tests/test_budget_crud.py index 962984c1..ae0359c0 100644 --- a/services/budget/tests/test_budget_crud.py +++ b/services/budget/tests/test_budget_crud.py @@ -1,6 +1,9 @@ +import uuid + import pytest -from app.crud.budget_crud import create_budget +from app.crud.budget_crud import create_budget, update_budget +from shared.security.current_user_context import reset_current_user_id, set_current_user_id from tests.factories.user import ValidUserFactory @@ -36,3 +39,31 @@ async def test_provided_fields_are_persisted(self, db): assert budget.duration_months == 12 assert budget.donor_total_amount == 5000.0 assert budget.estimated_exchange_rate == 1.1 + + +@pytest.mark.anyio +class TestUpdateBudgetAuditTrail: + async def test_updated_by_reflects_the_editing_user_not_the_creator(self, db): + """update_budget never touches updated_by itself — only the listener should.""" + creator = ValidUserFactory() + editor = ValidUserFactory() + creator_id = uuid.UUID(creator["user_id"]) + editor_id = uuid.UUID(editor["user_id"]) + + token = set_current_user_id(creator_id) + try: + budget = await create_budget( + session=db, user_id=creator_id, name="Original", owner_id=creator["customer_id"] + ) + finally: + reset_current_user_id(token) + + assert budget.updated_by == creator_id + + token = set_current_user_id(editor_id) + try: + updated = await update_budget(session=db, budget_id=budget.id, name="Edited") + finally: + reset_current_user_id(token) + + assert updated.updated_by == editor_id diff --git a/services/users/tests/test_audit_trail_routes.py b/services/users/tests/test_audit_trail_routes.py new file mode 100644 index 00000000..84b7f918 --- /dev/null +++ b/services/users/tests/test_audit_trail_routes.py @@ -0,0 +1,120 @@ +"""Real-JWT route tests: make_client's get_validated_user override would skip the +contextvar-setting code these tests need to exercise.""" + +from datetime import datetime, timezone +from uuid import uuid4 + +import fakeredis +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import select +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine +from sqlalchemy.pool import StaticPool + +from app.api.bug_report_routes import get_db as bug_report_get_db +from app.api.donor_grantee_routes import get_db as donor_grantee_get_db +from app.models.base import Base +from app.models.bug_report import BugReportModel +from app.models.customer import DonorGranteeModel +from main import app +from shared.security import session_revocation +from shared.security.jwt_utils import create_access_token +from tests.factories.user import CustomerFactory + +pytestmark = pytest.mark.anyio + + +@pytest.fixture +async def bug_reports_db(): + """conftest's `db` fixture doesn't include bug_reports — separate table set.""" + engine = create_async_engine( + "sqlite+aiosqlite://", + connect_args={"check_same_thread": False}, + poolclass=StaticPool, + ) + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all, tables=[BugReportModel.__table__]) + maker = async_sessionmaker(engine, expire_on_commit=False) + async with maker() as session: + yield session + await engine.dispose() + + +@pytest.fixture(autouse=True) +def fake_redis(monkeypatch): + monkeypatch.setattr(session_revocation, "_redis_client", fakeredis.FakeStrictRedis()) + + +def _token_for(user_id: str, **extra_claims) -> str: + return create_access_token( + { + "user_id": user_id, + "session_id": str(uuid4()), + "role": "user", + "email_verified": True, + **extra_claims, + } + ) + + +class TestBugReportAuditTrail: + async def test_created_by_populated_via_real_auth_chain(self, bug_reports_db): + app.dependency_overrides[bug_report_get_db] = lambda: bug_reports_db + try: + user_id = str(uuid4()) + client = TestClient(app) + + response = client.post( + "/api/bug-reports/", + headers={"Authorization": f"Bearer {_token_for(user_id)}"}, + data={ + "description": "Something broke", + "page_path": "/budgets/123", + "user_agent": "Mozilla/5.0", + "client_timestamp": datetime( + 2026, 9, 19, tzinfo=timezone.utc + ).isoformat(), + }, + ) + finally: + del app.dependency_overrides[bug_report_get_db] + + assert response.status_code == 200 + bug_report = ( + await bug_reports_db.execute( + select(BugReportModel).where(BugReportModel.id == response.json()["id"]) + ) + ).scalar_one() + assert str(bug_report.created_by) == user_id + assert str(bug_report.updated_by) == user_id + + +class TestDonorGranteeAuditTrail: + async def test_created_by_populated_via_real_auth_chain(self, db): + donor = CustomerFactory.build(name="Donor Org", is_donor=True) + grantee = CustomerFactory.build(name="Grantee Org", is_ngo=True) + db.add_all([donor, grantee]) + await db.commit() + + app.dependency_overrides[donor_grantee_get_db] = lambda: db + try: + user_id = str(uuid4()) + token = _token_for(user_id, customer_id=str(donor.id), is_donor=True) + client = TestClient(app) + + response = client.post( + "/api/donor-grantees/", + headers={"Authorization": f"Bearer {token}"}, + json={"grantee_id": str(grantee.id)}, + ) + finally: + del app.dependency_overrides[donor_grantee_get_db] + + assert response.status_code == 200 + donor_grantee = ( + await db.execute( + select(DonorGranteeModel).where(DonorGranteeModel.id == response.json()["id"]) + ) + ).scalar_one() + assert str(donor_grantee.created_by) == user_id + assert str(donor_grantee.updated_by) == user_id