From 5521cbbe5bf0531fa0912913f9d3233791a7d903 Mon Sep 17 00:00:00 2001 From: Norair Arutshyan Date: Fri, 25 Sep 2026 14:17:02 +0100 Subject: [PATCH 1/2] feat(export): export templates model, CRUD, and candidate resolution (#323) Group 5 of budget-feat-313-excel-export: ExportTemplateModel with a seeded system default, scoped CRUD, list_candidate_templates (system + own + funder's shared, live-gated on the donor-grantee relationship), and GET /budgets/{id}/export-templates. The system default is marked by an explicit is_system_default flag (000018), with a partial unique index allowing at most one and a CHECK tying it to a NULL owner; a NULL-owner UNIQUE constraint alone let duplicates through and 500'd the listing, update and delete paths. Tooling: the comment-brevity hook now blocks only newly introduced violations and uses ast/tokenize for Python so string literals aren't flagged; flow.py gains `cleanup` and a stale user-guide warning on `pr`. Co-Authored-By: Claude Opus 5.5 --- .claude/hooks/check_comment_brevity.py | 170 ++++++++++++------ .claude/settings.json | 13 +- CLAUDE.md | 13 +- docs/development/WORKFLOW.md | 21 ++- .../specs/budget-excel-export/spec.md | 11 -- .../specs/budget-export-templates/spec.md | 25 +++ .../budget-feat-313-excel-export/tasks.md | 18 +- .../changes/donor-crm-sync/.openspec.yaml | 1 + scripts/flow.py | 64 +++++++ services/budget/app/api/budget_routes.py | 13 ++ .../budget/app/crud/export_template_crud.py | 120 +++++++++++++ services/budget/app/models/__init__.py | 2 + services/budget/app/models/export_template.py | 44 +++++ .../app/schemas/export_template_schema.py | 66 +++++++ .../budget/app/services/budget_services.py | 4 +- .../app/services/donor_grantee_client.py | 30 +++- .../app/services/export_template_service.py | 51 ++++++ services/budget/main.py | 7 + .../versions/000017_add_export_templates.py | 80 +++++++++ ...add_export_template_system_default_flag.py | 49 +++++ services/budget/tests/conftest.py | 2 + .../budget/tests/factories/export_template.py | 18 ++ .../budget/tests/test_donor_grantee_gate.py | 52 +++--- .../budget/tests/test_excel_export_service.py | 44 +++++ .../budget/tests/test_export_template_crud.py | 58 ++++++ .../tests/test_export_template_model.py | 48 +++++ .../tests/test_export_template_schema.py | 29 +++ .../tests/test_export_template_service.py | 113 ++++++++++++ 28 files changed, 1044 insertions(+), 122 deletions(-) create mode 100644 openspec/changes/budget-feat-313-excel-export/specs/budget-export-templates/spec.md create mode 100644 services/budget/app/crud/export_template_crud.py create mode 100644 services/budget/app/models/export_template.py create mode 100644 services/budget/app/schemas/export_template_schema.py create mode 100644 services/budget/app/services/export_template_service.py create mode 100644 services/budget/migrations/versions/000017_add_export_templates.py create mode 100644 services/budget/migrations/versions/000018_add_export_template_system_default_flag.py create mode 100644 services/budget/tests/factories/export_template.py create mode 100644 services/budget/tests/test_export_template_crud.py create mode 100644 services/budget/tests/test_export_template_model.py create mode 100644 services/budget/tests/test_export_template_schema.py create mode 100644 services/budget/tests/test_export_template_service.py diff --git a/.claude/hooks/check_comment_brevity.py b/.claude/hooks/check_comment_brevity.py index fdda4353..2f1d0e6b 100755 --- a/.claude/hooks/check_comment_brevity.py +++ b/.claude/hooks/check_comment_brevity.py @@ -1,8 +1,12 @@ #!/usr/bin/env python3 -"""PostToolUse hook: flags multi-line comment blocks in a just-written/edited file.""" +"""PreToolUse hook (Write/Edit): blocks a new multi-line comment block before it's written.""" +import ast +import io import json import re import sys +import tokenize +from collections import Counter LINE_COMMENT_STYLES = { ".ts": "//", ".tsx": "//", ".js": "//", ".jsx": "//", @@ -15,88 +19,138 @@ } MAX_CONSECUTIVE = 2 # 3+ consecutive same-style comment lines triggers a flag MAX_BLOCK_LINES = 2 # a /* */ or docstring spanning more than this triggers a flag -MAX_FILE_BYTES = 2 * 1024 * 1024 +DOCSTRING_OWNERS = (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef) -def _line_at(text, offset): - return text.count("\n", 0, offset) + 1 +def comment_runs(numbered_lines, marker): + """Yield (first_lineno, text) for each run of more than MAX_CONSECUTIVE comment lines.""" + run = [] + for lineno, line in numbered_lines + [(None, "")]: + stripped = line.strip() + is_comment = stripped.startswith(marker) and not stripped.startswith(marker * 3) + if is_comment and (not run or run[-1][0] + 1 == lineno): + run.append((lineno, stripped)) + continue + if len(run) > MAX_CONSECUTIVE: + yield run[0][0], "\n".join(t for _, t in run) + run = [(lineno, stripped)] if is_comment else [] -def find_violations(ext, text): - lines = text.splitlines() - violations = [] - +def regex_violations(ext, text): + found = [] marker = LINE_COMMENT_STYLES.get(ext) if marker: - run_start = None - for i, line in enumerate(lines): - stripped = line.strip() - if stripped.startswith(marker) and not stripped.startswith(marker * 3): - if run_start is None: - run_start = i - else: - if run_start is not None and i - run_start > MAX_CONSECUTIVE: - violations.append((run_start + 1, i)) - run_start = None - if run_start is not None and len(lines) - run_start > MAX_CONSECUTIVE: - violations.append((run_start + 1, len(lines))) + found += comment_runs(list(enumerate(text.splitlines(), 1)), marker) + patterns = [] if ext in BLOCK_COMMENT_EXTS: - for m in re.finditer(r"/\*.*?\*/", text, re.DOTALL): - if m.group(0).count("\n") + 1 > MAX_BLOCK_LINES: - violations.append((_line_at(text, m.start()), _line_at(text, m.end()))) - + patterns.append(r"/\*.*?\*/") if ext == ".py": - for m in re.finditer(r'("""|\'\'\')(.*?)\1', text, re.DOTALL): + patterns.append(r'("{3}|\'{3})(.*?)\1') + for pattern in patterns: + for m in re.finditer(pattern, text, re.DOTALL): if m.group(0).count("\n") + 1 > MAX_BLOCK_LINES: - violations.append((_line_at(text, m.start()), _line_at(text, m.end()))) + found.append((text.count("\n", 0, m.start()) + 1, m.group(0))) + return found + + +def python_violations(text, file_path): + """Use tokenize/ast so string literals aren't mistaken for comments or docstrings.""" + tree = ast.parse(text) + comments = [ + (tok.start[0], tok.string) + for tok in tokenize.generate_tokens(io.StringIO(text).readline) + if tok.type == tokenize.COMMENT and tok.line.strip().startswith("#") + ] + found = list(comment_runs(comments, "#")) + + is_migration = "/migrations/versions/" in file_path + for node in ast.walk(tree): + if not isinstance(node, DOCSTRING_OWNERS) or not node.body: + continue + if isinstance(node, ast.Module) and is_migration: + continue # Alembic's generated Revision ID/Revises header + first = node.body[0] + if not (isinstance(first, ast.Expr) and isinstance(first.value, ast.Constant) + and isinstance(first.value.value, str)): + continue + if first.end_lineno - first.lineno + 1 > MAX_BLOCK_LINES: + found.append((first.lineno, first.value.value.strip())) + return found + + +def violations(ext, text, file_path): + if ext == ".py": + try: + return python_violations(text, file_path) + except (SyntaxError, tokenize.TokenError, ValueError): + pass + return regex_violations(ext, text) - return sorted(set(violations)) + +def before_and_after(tool_name, tool_input, file_path): + try: + with open(file_path, encoding="utf-8", errors="replace") as f: + before = f.read() + except OSError: + before = "" + + if tool_name == "Write": + return before, tool_input.get("content", "") + if tool_name == "Edit": + old, new = tool_input.get("old_string", ""), tool_input.get("new_string", "") + if not before or old not in before: + return "", new + count = -1 if tool_input.get("replace_all") else 1 + return before, before.replace(old, new, count) + return None, None def main(): try: payload = json.load(sys.stdin) except Exception: - return 0 + return + tool_name = payload.get("tool_name") tool_input = payload.get("tool_input") or {} - file_path = tool_input.get("file_path") or (payload.get("tool_response") or {}).get("filePath") - if not file_path or "." not in file_path.rsplit("/", 1)[-1]: - return 0 + file_path = tool_input.get("file_path", "") + if "." not in file_path.rsplit("/", 1)[-1]: + return ext = "." + file_path.rsplit(".", 1)[-1].lower() if ext not in LINE_COMMENT_STYLES and ext not in BLOCK_COMMENT_EXTS: - return 0 - - try: - import os - if os.path.getsize(file_path) > MAX_FILE_BYTES: - return 0 - with open(file_path, "r", encoding="utf-8", errors="ignore") as f: - text = f.read() - except OSError: - return 0 - - violations = find_violations(ext, text) - if not violations: - return 0 - - ranges = ", ".join(f"L{a}-{b}" if a != b else f"L{a}" for a, b in violations) - warning = ( - f"Comment brevity check: {file_path} has a multi-line comment block at {ranges}. " - "Trim it to one short line; move any longer rationale to a memory file instead." - ) - print(warning, file=sys.stderr) + return + + before, after = before_and_after(tool_name, tool_input, file_path) + if after is None: + return + + existing = Counter(text for _, text in violations(ext, before, file_path)) + introduced = [] + for lineno, text in violations(ext, after, file_path): + if existing[text] > 0: + existing[text] -= 1 + else: + introduced.append((lineno, text)) + if not introduced: + return + + lineno, text = introduced[0] + preview = text.splitlines()[0][:80] print(json.dumps({ - "systemMessage": warning, "hookSpecificOutput": { - "hookEventName": "PostToolUse", - "additionalContext": warning, - }, + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": ( + f"Blocked: this {tool_name} to {file_path} introduces a multi-line comment " + f"at line {lineno} ({preview!r}): more than {MAX_BLOCK_LINES} lines, or more " + f"than {MAX_CONSECUTIVE} consecutive comment lines. Trim it to one or two " + "lines; longer rationale belongs in the commit message, PR description, or docs." + ), + } })) - return 0 if __name__ == "__main__": - sys.exit(main()) + main() diff --git a/.claude/settings.json b/.claude/settings.json index 054c0cb7..e9f18bda 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -56,9 +56,7 @@ "timeout": 15 } ] - } - ], - "PostToolUse": [ + }, { "matcher": "Write|Edit", "hooks": [ @@ -66,7 +64,14 @@ "type": "command", "command": "python3 /home/noro/repos/GrandFlow/.claude/hooks/check_comment_brevity.py", "timeout": 15 - }, + } + ] + } + ], + "PostToolUse": [ + { + "matcher": "Write|Edit", + "hooks": [ { "type": "command", "command": "python3 /home/noro/repos/GrandFlow/.claude/hooks/check_test_factory_usage.py", diff --git a/CLAUDE.md b/CLAUDE.md index 3f025cae..6c224ae3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,10 +49,21 @@ This is enforced, not just documented: blocks manually-created branches whose name doesn't match the convention. - `scripts/git-hooks/pre-push` refuses to push a branch with a non-conforming name (requires `git config core.hooksPath scripts/git-hooks` — see - WORKFLOW.md §7, which also covers this hook's lint-mirroring behavior). + WORKFLOW.md §8, which also covers this hook's lint-mirroring behavior). - `.github/workflows/branch-naming.yml` re-checks the branch name on every push and PR as a backstop. For a ticket that isn't part of an OpenSpec change, use `scripts/flow.py issue "" "<body>"`. If such a task still needs a branch, ask the user before improvising a name outside the convention. + +## Comments — keep them short + +Comments (line-comment runs, `/* */` blocks, docstrings) are at most 2 lines. +Longer rationale belongs in a commit message, PR description, or docs — not +in the code. + +This is enforced, not just documented: `.claude/hooks/check_comment_brevity.py` +(a `PreToolUse` hook on `Write`/`Edit`) denies any write that introduces a +comment longer than that, so trim it before retrying rather than fighting the +hook. diff --git a/docs/development/WORKFLOW.md b/docs/development/WORKFLOW.md index 43c8f4d6..38c4d5d0 100644 --- a/docs/development/WORKFLOW.md +++ b/docs/development/WORKFLOW.md @@ -12,6 +12,7 @@ Everything below is driven by one tool, `scripts/flow.py`: | `flow.py start <change> <group>` | starting a group | parent + group → In Progress, branch from `main` | | `flow.py pr` | opening the PR | prints the `Closes` trailer | | `flow.py issue "<t>" "<body>"` | one-off ticket | issue on the board, outside any change | +| `flow.py cleanup [--yes]` | after merges pile up | deletes local branches whose remote was deleted | Sub-issues are matched to task groups by the issue number recorded in `tasks.md`, falling back to the `(group N)` title suffix when no number is recorded. @@ -140,7 +141,21 @@ scripts/flow.py issue "<title>" "<body>" Creates the issue and puts it on the board as Todo. If it needs a branch, agree a name with a human first — the convention above assumes a sub-issue exists. -## 7. Local lint enforcement before push +## 7. Local branch cleanup + +Squash-merging means `git branch --merged` never recognizes a merged group +branch, so they pile up locally even though the remote is clean. Run: + +``` +scripts/flow.py cleanup +``` + +It fetches with `--prune`, lists local branches whose upstream was deleted +(skipping the current branch and any branch checked out in another worktree), +and deletes them after a y/N confirmation (`-D`, since they're squash-merged +rather than fast-forward-mergeable). Pass `--yes` to skip the prompt. + +## 8. Local lint enforcement before push `scripts/git-hooks/pre-push` mirrors each service's CI lint step (`black --check`, `mypy`, `flake8`) locally, scoped to whichever service(s) the @@ -155,7 +170,7 @@ git config core.hooksPath scripts/git-hooks Bypass with `git push --no-verify` when intentionally needed. -## 8. Testing the tool itself +## 9. Testing the tool itself `scripts/test_flow.py` covers the change-name/group parsing and the `tasks.md` rewriting — the load-bearing, network-free parts: @@ -166,7 +181,7 @@ python3 -m pytest scripts/test_flow.py -q `.github/workflows/tooling.yml` runs the same tests plus `black`/`flake8` over `scripts/` on any push or PR that touches `scripts/**`, and the pre-push hook -(§7) mirrors it locally. +(§8) mirrors it locally. `scripts/flow.py --dry-run <subcommand> ...` prints every mutating `gh`/`git` call instead of running it, which is the safe way to preview `init` or `sync`. diff --git a/openspec/changes/budget-feat-313-excel-export/specs/budget-excel-export/spec.md b/openspec/changes/budget-feat-313-excel-export/specs/budget-excel-export/spec.md index dbf0b782..d6f34e8e 100644 --- a/openspec/changes/budget-feat-313-excel-export/specs/budget-excel-export/spec.md +++ b/openspec/changes/budget-feat-313-excel-export/specs/budget-excel-export/spec.md @@ -119,17 +119,6 @@ The export endpoint SHALL accept an optional template identifier and SHALL NOT s - **WHEN** a client re-sends a template identifier that has since been deleted or un-shared - **THEN** the request is rejected with an error identifying that the template is no longer available, rather than falling back to another template -### Requirement: Template-scoped ownership -An export template SHALL belong to exactly one organisation and SHALL carry a visibility of either private or shared with grantees. Template names SHALL be unique within an owning organisation. Every query for templates SHALL be scoped to a requesting organisation; the system SHALL NOT expose a way to read templates without an organisational scope. - -#### Scenario: Duplicate name within an organisation -- **WHEN** an organisation creates a second template with the name of one it already owns -- **THEN** the request is rejected, so which template a name refers to is never ambiguous - -#### Scenario: Same name across organisations -- **WHEN** two different organisations each create a template named "Annual Report" -- **THEN** both are accepted, and each organisation only ever sees its own - ### Requirement: Renderer options applied to the generated workbook A template SHALL define its output as a bounded set of options over the built-in renderer — which sheets to include, whether the donor-currency estimate column is shown, column header label overrides, and whether the audit footer is shown — validated on write. The system SHALL reject an unrecognised option at write time rather than ignoring it at generation time. Options absent from a stored template SHALL fall back to the system default's value at generation time. diff --git a/openspec/changes/budget-feat-313-excel-export/specs/budget-export-templates/spec.md b/openspec/changes/budget-feat-313-excel-export/specs/budget-export-templates/spec.md new file mode 100644 index 00000000..33e725fb --- /dev/null +++ b/openspec/changes/budget-feat-313-excel-export/specs/budget-export-templates/spec.md @@ -0,0 +1,25 @@ +# Spec Delta + +## Purpose + +Lets an organisation create, own, and share the export templates that `budget-excel-export`'s template selection and rendering consume — the storage, uniqueness, versioning, and access-scoping rules for those template records. + +## ADDED Requirements + +### Requirement: Template-scoped ownership +An export template SHALL belong to exactly one organisation and SHALL carry a visibility of either private or shared with grantees. Template names SHALL be unique within an owning organisation. Every query for templates SHALL be scoped to a requesting organisation; the system SHALL NOT expose a way to read templates without an organisational scope. + +#### Scenario: Duplicate name within an organisation +- **WHEN** an organisation creates a second template with the name of one it already owns +- **THEN** the request is rejected, so which template a name refers to is never ambiguous + +#### Scenario: Same name across organisations +- **WHEN** two different organisations each create a template named "Annual Report" +- **THEN** both are accepted, and each organisation only ever sees its own + +### Requirement: Template versioning on edit +An export template SHALL carry a version number that advances whenever its name, visibility, or options are updated. + +#### Scenario: Edit bumps version +- **WHEN** an organisation updates one of its own templates +- **THEN** the template's stored version increases, distinguishing the edited template from the one that produced any workbook exported before the edit diff --git a/openspec/changes/budget-feat-313-excel-export/tasks.md b/openspec/changes/budget-feat-313-excel-export/tasks.md index 9eee1a3e..29c67db9 100644 --- a/openspec/changes/budget-feat-313-excel-export/tasks.md +++ b/openspec/changes/budget-feat-313-excel-export/tasks.md @@ -46,20 +46,18 @@ _Generated by `scripts/flow.py sync budget-feat-313-excel-export` — do not edi ## 4. Frontend export button — depends on 1, 2, 3 — Issue #322 - [x] 4.1 Add an "Export to Excel" button to the single-budget detail view in `frontend-typescript`, visible whenever the viewer has read access to the budget (owner or funder), following existing button/permission conventions on that view — a plain button at this stage; group 7 turns it into a template picker -- [ ] 4.2 Wire the button to `GET /budgets/{budget_id}/export.xlsx` and trigger a browser download of the response with a filename derived from the budget's name; verify manually that a downloaded file opens in Excel/LibreOffice with all 3 sheets populated +- [x] 4.2 Wire the button to `GET /budgets/{budget_id}/export.xlsx` and trigger a browser download of the response with a filename derived from the budget's name; verify manually that a downloaded file opens in Excel/LibreOffice with all 3 sheets populated - [x] 4.3 Add inline error handling that shows a message without navigating away when the request fails; verify with a frontend test that simulates a failed request and asserts the user stays on the budget detail view with an error shown -- [ ] 4.4 Run frontend lint/tests clean; manually verify end-to-end against a real budget with lines, receipts, multi-lot conversions, and report expenses (owner and funder logins); PR merged (`Closes` this group's sub-issue) +- [x] 4.4 Run frontend lint/tests clean; manually verify end-to-end against a real budget with lines, receipts, multi-lot conversions, and report expenses (owner and funder logins); PR merged (`Closes` this group's sub-issue) ## 5. Export templates: model, ownership, and candidate resolution — depends on 1 — Issue #323 -> The `budget-export-templates` capability's spec delta is not yet authored (see proposal.md — Capabilities). Write it before starting this group. - -- [ ] 5.1 Add `ExportTemplateModel` to `services/budget/app/models/` with `owner_customer_id` (nullable — NULL marks the system default), `name`, `visibility` (`private` | `shared_with_grantees`), `options` (JSON), and `version`, with a unique constraint on `(owner_customer_id, name)`; include `AuditMixin` per the audit-mixin coverage convention; verify with a unit test asserting the unique constraint rejects a duplicate name within one organisation and accepts the same name across two -- [ ] 5.2 Add the Alembic migration creating `export_templates` and seeding the single system-default row whose `options` reproduce group 1's current output exactly; verify by running the migration up and down against a scratch DB and asserting the seeded row exists after up and the table is gone after down -- [ ] 5.3 Define the options blob's Pydantic schema (sheets to include, show donor-currency estimate column, column header label overrides, show audit footer), with every field optional and defaulted so a template stored before an option existed still renders; verify with unit tests that an unknown key is rejected and that a blob missing every optional key validates to the system default's values -- [ ] 5.4 Add `services/budget/app/crud/export_template_crud.py` with create/list/get/update/delete, every function taking the actor's `customer_id` so an unscoped read is not expressible; updating a template bumps `version`; deleting or editing the system default is rejected; verify with unit tests covering the scoping (org A cannot read org B's private template), the version bump, and the system-default guard -- [ ] 5.5 Add the candidate-resolution service `list_candidate_templates(actor, budget)` returning the system default plus the actor's own templates plus the budget funder's `shared_with_grantees` templates, gating the funder's on `donor_grantee_client.check_donor_grantee_relationship`; verify with unit tests covering a donor's shared template being offered, a donor's private one not being offered, an unrelated org's shared one not being offered, and a revoked relationship dropping the template on the next call with no cache in between -- [ ] 5.6 Add `GET /budgets/{budget_id}/export-templates` to `budget_routes.py`, authorized identically to `GET /budgets/{budget_id}`, returning each candidate tagged `system` / `own` / `donor`; verify with an integration test that owner and funder each get the candidate set the resolution rules predict and a non-viewer is rejected +- [x] 5.1 Add `ExportTemplateModel` to `services/budget/app/models/` with `is_system_default` (at most one row, enforced by a partial unique index; a CHECK ties it to a NULL `owner_customer_id`), `owner_customer_id` (nullable — NULL only for the system default), `name`, `visibility` (`private` | `shared_with_grantees`), `options` (JSON), and `version`, with a unique constraint on `(owner_customer_id, name)`; include `AuditMixin` per the audit-mixin coverage convention; verify with a unit test asserting the unique constraint rejects a duplicate name within one organisation and accepts the same name across two +- [x] 5.2 Add the Alembic migration creating `export_templates` and seeding the single system-default row whose `options` reproduce group 1's current output exactly; verify by running the migration up and down against a scratch DB and asserting the seeded row exists after up and the table is gone after down +- [x] 5.3 Define the options blob's Pydantic schema (sheets to include, show donor-currency estimate column, column header label overrides, show audit footer), with every field optional and defaulted so a template stored before an option existed still renders; verify with unit tests that an unknown key is rejected and that a blob missing every optional key validates to the system default's values +- [x] 5.4 Add `services/budget/app/crud/export_template_crud.py` with create/list/get/update/delete, every function taking the actor's `customer_id` so an unscoped read is not expressible; updating a template bumps `version`; deleting or editing the system default is rejected; verify with unit tests covering the scoping (org A cannot read org B's private template), the version bump, and the system-default guard +- [x] 5.5 Add the candidate-resolution service `list_candidate_templates(actor, budget)` returning the system default plus the actor's own templates plus the budget funder's `shared_with_grantees` templates, gating the funder's on `donor_grantee_client.check_donor_grantee_relationship`; verify with unit tests covering a donor's shared template being offered, a donor's private one not being offered, an unrelated org's shared one not being offered, and a revoked relationship dropping the template on the next call with no cache in between +- [x] 5.6 Add `GET /budgets/{budget_id}/export-templates` to `budget_routes.py`, authorized identically to `GET /budgets/{budget_id}`, returning each candidate tagged `system` / `own` / `donor`; verify with an integration test that owner and funder each get the candidate set the resolution rules predict and a non-viewer is rejected - [ ] 5.7 Run backend lint/tests clean for `services/budget`; PR merged (`Closes` this group's sub-issue) ## 6. Apply template options to generation — depends on 2, 3, 5 — Issue #324 diff --git a/openspec/changes/donor-crm-sync/.openspec.yaml b/openspec/changes/donor-crm-sync/.openspec.yaml index d7bc0110..33e2ed68 100644 --- a/openspec/changes/donor-crm-sync/.openspec.yaml +++ b/openspec/changes/donor-crm-sync/.openspec.yaml @@ -1,2 +1,3 @@ schema: spec-driven created: 2026-08-10 +priority: low diff --git a/scripts/flow.py b/scripts/flow.py index 75c62d10..2380ca99 100755 --- a/scripts/flow.py +++ b/scripts/flow.py @@ -9,6 +9,7 @@ start <change> <group> parent + group to In Progress, branch from main pr [--branch B] print the Closes trailer for a group's PR issue "<title>" ["<body>"] create a standalone issue on the board + cleanup [--yes] delete local branches whose remote was deleted GitHub is the source of truth: a group is matched to the sub-issue number tasks.md records, falling back to the "(group N)" title suffix, and every @@ -767,12 +768,34 @@ def find_change_for_issue(sub: int) -> tuple[Change, Group, list[Group]] | None: return None +USER_FACING_ROOT = "frontend-typescript/src/" +USER_FACING_EXCLUDE_RE = re.compile(r"\.(test|spec|stories)\.[jt]sx?$|/__tests__/|/__mocks__/") + + +def warn_if_user_guide_stale(branch: str) -> None: + base = run(["git", "merge-base", "main", branch], check=False) + if not base: + return + changed = run(["git", "diff", "--name-only", base, branch], check=False).splitlines() + user_facing = [ + f + for f in changed + if f.startswith(USER_FACING_ROOT) and not USER_FACING_EXCLUDE_RE.search(f) + ] + if user_facing and not any(f.startswith("docs/user-guide/") for f in changed): + warn( + "touches user-facing frontend code but not docs/user-guide/ — update the guide " + "(and its 'Last reviewed' line) if this changes what users see" + ) + + def cmd_pr(args) -> None: branch = args.branch or run(["git", "branch", "--show-current"]) match = BRANCH_RE.match(branch) if not match: die(f"branch '{branch}' doesn't match <Service>/<type>/Issue-<n>/<description>") sub = int(match.group(1)) + warn_if_user_guide_stale(branch) found = find_change_for_issue(sub) closes = [sub] @@ -799,6 +822,43 @@ def cmd_pr(args) -> None: print("\n".join(f"Closes #{n}" for n in closes)) +def cmd_cleanup(args) -> None: + run(["git", "fetch", "--prune", "origin"], mutating=True) + + current = run(["git", "branch", "--show-current"]) + worktree_branches = { + line.removeprefix("branch refs/heads/") + for line in run(["git", "worktree", "list", "--porcelain"]).splitlines() + if line.startswith("branch ") + } + + gone = [] + for line in run( + ["git", "for-each-ref", "refs/heads", "--format=%(refname:short) %(upstream:track)"] + ).splitlines(): + branch, _, track = line.partition(" ") + if "[gone]" in track and branch != current and branch not in worktree_branches: + gone.append(branch) + + if not gone: + print("no local branches with a deleted remote") + return + + print(f"{len(gone)} local branch(es) with a deleted remote:") + for branch in gone: + print(f" {branch}") + + if not args.yes: + answer = input("\nDelete these branches? [y/N] ").strip().lower() + if answer != "y": + print("aborted") + return + + for branch in gone: + run(["git", "branch", "-D", branch], mutating=True) + print(f" deleted {branch}") + + def cmd_issue(args) -> None: number = create_issue(args.title, args.body or "") if number is None: @@ -844,6 +904,10 @@ def main() -> None: p.add_argument("body", nargs="?") p.set_defaults(func=cmd_issue) + p = sub.add_parser("cleanup", help="delete local branches whose remote was deleted") + p.add_argument("--yes", action="store_true", help="skip the confirmation prompt") + p.set_defaults(func=cmd_cleanup) + args = parser.parse_args() DRY_RUN = args.dry_run if not Path(".git").exists(): diff --git a/services/budget/app/api/budget_routes.py b/services/budget/app/api/budget_routes.py index 7d412aab..a8509264 100644 --- a/services/budget/app/api/budget_routes.py +++ b/services/budget/app/api/budget_routes.py @@ -18,6 +18,7 @@ ) from app.schemas.budget_line_schema import BudgetLine from app.schemas.excel_import_schema import ExcelPrepareImportResult +from app.schemas.export_template_schema import ExportTemplateCandidate from app.schemas.mapping_schema import DonorTemplate, DonorTemplateCreate from app.schemas.with_lines_schema import CreateBudgetWithLinesRequest from app.services.budget_line_services import get_viewable_budget_lines_service @@ -38,6 +39,7 @@ from app.services.customer_client import require_donor from app.services.excel_export_service import export_budget_workbook_service from app.services.excel_import_service import prepare_excel_import_service +from app.services.export_template_service import list_candidate_templates from app.crud.budget_crud import get_budgets_by_creator from shared.observability import set_span_attributes from shared.security.dependencies import get_validated_user @@ -124,6 +126,17 @@ async def export_budget_workbook_endpoint( ) +@router.get("/{budget_id}/export-templates", response_model=list[ExportTemplateCandidate]) +async def list_budget_export_templates_endpoint( + budget_id: UUID, + db: AsyncSession = Depends(get_db), + valid_user=Depends(get_validated_user), +): + set_span_attributes(budget_id=budget_id) + budget = await get_viewable_budget_service(budget_id, valid_user, db) + return await list_candidate_templates(db, valid_user, budget) + + @router.patch("/{budget_id}", response_model=BudgetUpdate) async def update_budget_endpoint( budget_id: UUID, diff --git a/services/budget/app/crud/export_template_crud.py b/services/budget/app/crud/export_template_crud.py new file mode 100644 index 00000000..0ef5e6da --- /dev/null +++ b/services/budget/app/crud/export_template_crud.py @@ -0,0 +1,120 @@ +from uuid import UUID + +from fastapi import status +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from app.core.exceptions import DomainError +from app.models.export_template import ExportTemplateModel +from app.schemas.export_template_schema import ExportTemplateOptions, TemplateVisibility + + +async def create_export_template( + session: AsyncSession, + customer_id: UUID, + name: str, + visibility: TemplateVisibility, + options: ExportTemplateOptions, +) -> ExportTemplateModel: + template = ExportTemplateModel( + owner_customer_id=customer_id, + name=name, + visibility=visibility, + options=options.model_dump(mode="json"), + ) + session.add(template) + await session.commit() + await session.refresh(template) + return template + + +async def list_export_templates( + session: AsyncSession, customer_id: UUID +) -> list[ExportTemplateModel]: + result = await session.execute( + select(ExportTemplateModel).where(ExportTemplateModel.owner_customer_id == customer_id) + ) + return list(result.scalars().all()) + + +async def get_export_template( + session: AsyncSession, customer_id: UUID, template_id: UUID +) -> ExportTemplateModel | None: + """Scoped to the requesting organisation — an unscoped read is not expressible.""" + result = await session.execute( + select(ExportTemplateModel).where( + ExportTemplateModel.id == template_id, + ExportTemplateModel.owner_customer_id == customer_id, + ) + ) + return result.scalar_one_or_none() + + +async def get_system_default_template(session: AsyncSession) -> ExportTemplateModel | None: + result = await session.execute( + select(ExportTemplateModel).where(ExportTemplateModel.is_system_default.is_(True)) + ) + return result.scalar_one_or_none() + + +async def list_shared_export_templates( + session: AsyncSession, owner_customer_id: UUID +) -> list[ExportTemplateModel]: + """A donor's templates marked shared with grantees — candidate-resolution reads only.""" + result = await session.execute( + select(ExportTemplateModel).where( + ExportTemplateModel.owner_customer_id == owner_customer_id, + ExportTemplateModel.visibility == TemplateVisibility.shared_with_grantees, + ) + ) + return list(result.scalars().all()) + + +async def _is_system_default(session: AsyncSession, template_id: UUID) -> bool: + result = await session.execute( + select(ExportTemplateModel.id).where( + ExportTemplateModel.id == template_id, + ExportTemplateModel.is_system_default.is_(True), + ) + ) + return result.scalar_one_or_none() is not None + + +async def update_export_template( + session: AsyncSession, + customer_id: UUID, + template_id: UUID, + name: str | None = None, + visibility: TemplateVisibility | None = None, + options: ExportTemplateOptions | None = None, +) -> ExportTemplateModel | None: + if await _is_system_default(session, template_id): + raise DomainError("The system default template cannot be edited", status.HTTP_403_FORBIDDEN) + template = await get_export_template(session, customer_id, template_id) + if not template: + return None + if name is not None: + template.name = name + if visibility is not None: + template.visibility = visibility + if options is not None: + template.options = options.model_dump(mode="json") + template.version += 1 + await session.commit() + await session.refresh(template) + return template + + +async def delete_export_template( + session: AsyncSession, customer_id: UUID, template_id: UUID +) -> bool: + if await _is_system_default(session, template_id): + raise DomainError( + "The system default template cannot be deleted", status.HTTP_403_FORBIDDEN + ) + template = await get_export_template(session, customer_id, template_id) + if not template: + return False + await session.delete(template) + await session.commit() + return True diff --git a/services/budget/app/models/__init__.py b/services/budget/app/models/__init__.py index 457c84fe..0574a6ef 100644 --- a/services/budget/app/models/__init__.py +++ b/services/budget/app/models/__init__.py @@ -8,6 +8,7 @@ ReportLineConversionAllocationModel, ) from app.models.privileged_access_log import PrivilegedAccessLog +from app.models.export_template import ExportTemplateModel __all__ = [ "BudgetModel", @@ -21,4 +22,5 @@ "CurrencyConversionModel", "ReportLineConversionAllocationModel", "PrivilegedAccessLog", + "ExportTemplateModel", ] diff --git a/services/budget/app/models/export_template.py b/services/budget/app/models/export_template.py new file mode 100644 index 00000000..e4496497 --- /dev/null +++ b/services/budget/app/models/export_template.py @@ -0,0 +1,44 @@ +from __future__ import annotations +import uuid + +from sqlalchemy import ( + Boolean, CheckConstraint, Enum as SQLEnum, Index, Integer, JSON, String, UniqueConstraint, + text, +) +from sqlalchemy.orm import Mapped, mapped_column + +from app.models.base import Base +from app.schemas.export_template_schema import TemplateVisibility +from app.utils.db import GUID +from shared.db.audit_mixin import AuditMixin + + +class ExportTemplateModel(Base, AuditMixin): + __tablename__ = "export_templates" + __table_args__ = ( + UniqueConstraint("owner_customer_id", "name"), + Index( + "uq_export_templates_single_system_default", + "is_system_default", + unique=True, + postgresql_where=text("is_system_default"), + sqlite_where=text("is_system_default"), + ), + CheckConstraint( + "is_system_default = (owner_customer_id IS NULL)", + name="ck_export_templates_system_default_has_no_owner", + ), + ) + + is_system_default: Mapped[bool] = mapped_column( + Boolean, nullable=False, default=False, server_default=text("false") + ) + owner_customer_id: Mapped[uuid.UUID | None] = mapped_column(GUID(), nullable=True) + name: Mapped[str] = mapped_column(String, nullable=False) + visibility: Mapped[TemplateVisibility] = mapped_column( + SQLEnum(TemplateVisibility, name="export_template_visibility"), + nullable=False, + default=TemplateVisibility.private, + ) + options: Mapped[dict] = mapped_column(JSON, nullable=False) + version: Mapped[int] = mapped_column(Integer, nullable=False, default=1) diff --git a/services/budget/app/schemas/export_template_schema.py b/services/budget/app/schemas/export_template_schema.py new file mode 100644 index 00000000..1781dc5c --- /dev/null +++ b/services/budget/app/schemas/export_template_schema.py @@ -0,0 +1,66 @@ +from enum import Enum +from uuid import UUID + +from pydantic import BaseModel, ConfigDict, Field + + +class TemplateVisibility(str, Enum): + private = "private" + shared_with_grantees = "shared_with_grantees" + + +class TemplateSource(str, Enum): + """How a candidate template relates to the requesting viewer (see + list_candidate_templates).""" + + system = "system" + own = "own" + donor = "donor" + + +class ExportSheet(str, Enum): + """The fixed, closed set of sheet renderers a template's `sheets` option + selects among (design.md Decision 12) — never a per-template layout.""" + + original_budget = "original_budget" + dashboard = "dashboard" + expense_list = "expense_list" + + +class ExportTemplateOptions(BaseModel): + """Bounded renderer options (design.md Decision 9); unknown keys rejected.""" + + model_config = ConfigDict(extra="forbid") + + sheets: list[ExportSheet] = Field( + default_factory=lambda: [ + ExportSheet.original_budget, + ExportSheet.dashboard, + ExportSheet.expense_list, + ] + ) + show_donor_currency_estimate: bool = True + column_labels: dict[str, str] = Field(default_factory=dict) + show_audit_footer: bool = True + + +class ExportTemplateCreate(BaseModel): + name: str + visibility: TemplateVisibility = TemplateVisibility.private + options: ExportTemplateOptions = Field(default_factory=ExportTemplateOptions) + + +class ExportTemplateUpdate(BaseModel): + name: str | None = None + visibility: TemplateVisibility | None = None + options: ExportTemplateOptions | None = None + + +class ExportTemplateCandidate(BaseModel): + """One entry in the `GET /budgets/{budget_id}/export-templates` response.""" + + id: UUID + name: str + visibility: TemplateVisibility + version: int + source: TemplateSource diff --git a/services/budget/app/services/budget_services.py b/services/budget/app/services/budget_services.py index d630872c..3a26199f 100644 --- a/services/budget/app/services/budget_services.py +++ b/services/budget/app/services/budget_services.py @@ -89,7 +89,7 @@ async def create_budget_service( # superuser, budget.owner_id (either client-supplied or the # FIXME fallback above) — never None in practice. assert owner_id is not None - validate_donor_grantee_relationship( + await validate_donor_grantee_relationship( budget.funding_customer_id, owner_id, raise_domain_error=True ) new_budget = await create_budget( @@ -287,7 +287,7 @@ async def update_budget_service(budget_id: UUID, budget: BudgetCreate, valid_use # funding_customer_id in the same request must be validated against # the new owner, otherwise the gate could be bypassed by reassigning # to an unapproved grantee after the check. - validate_donor_grantee_relationship( + await validate_donor_grantee_relationship( budget.funding_customer_id, owner_id or valid_budget.owner_id, raise_domain_error=True ) diff --git a/services/budget/app/services/donor_grantee_client.py b/services/budget/app/services/donor_grantee_client.py index 308117bc..f0912dfb 100644 --- a/services/budget/app/services/donor_grantee_client.py +++ b/services/budget/app/services/donor_grantee_client.py @@ -1,37 +1,55 @@ import uuid -import requests +import httpx from app.core.config import settings from app.core.exceptions import DomainError DONOR_GRANTEE_SERVICE_URL = settings.donor_grantee_service_url +REQUEST_TIMEOUT_SECONDS = 5 +_client: httpx.AsyncClient = httpx.AsyncClient(timeout=REQUEST_TIMEOUT_SECONDS) class DonorGranteeServiceError(Exception): pass -def check_donor_grantee_relationship( +async def init_urls(): + global DONOR_GRANTEE_SERVICE_URL, _client + + DONOR_GRANTEE_SERVICE_URL = settings.donor_grantee_service_url + _client = httpx.AsyncClient(timeout=REQUEST_TIMEOUT_SECONDS) + print(f"✅ Donor-grantee client initialized: {DONOR_GRANTEE_SERVICE_URL}") + + +async def close_urls(): + """Gracefully close HTTP client session.""" + global _client # noqa: F824 + if _client: + await _client.aclose() + print("🛑 Donor-grantee client closed") + + +async def check_donor_grantee_relationship( donor_id: str | uuid.UUID, grantee_id: str | uuid.UUID ) -> bool: """No caching, deliberately — unlike get_customer_cached, revocation must take effect on the very next call, not after some cache TTL/eviction.""" try: - resp = requests.get( + resp = await _client.get( f"{DONOR_GRANTEE_SERVICE_URL}exists", params={"donor_id": str(donor_id), "grantee_id": str(grantee_id)}, ) resp.raise_for_status() return bool(resp.json().get("exists", False)) - except requests.RequestException as e: + except httpx.HTTPError as e: raise DonorGranteeServiceError( f"Failed to check donor-grantee relationship for donor {donor_id}, " f"grantee {grantee_id}" ) from e -def validate_donor_grantee_relationship( +async def validate_donor_grantee_relationship( donor_id: str | uuid.UUID, grantee_id: str | uuid.UUID, raise_domain_error: bool = False, @@ -39,7 +57,7 @@ def validate_donor_grantee_relationship( """Assert a donor_grantees row exists linking donor_id (funder) to grantee_id (owner).""" Error = DomainError if raise_domain_error else ValueError try: - exists = check_donor_grantee_relationship(donor_id, grantee_id) + exists = await check_donor_grantee_relationship(donor_id, grantee_id) except DonorGranteeServiceError as e: raise Error(str(e)) diff --git a/services/budget/app/services/export_template_service.py b/services/budget/app/services/export_template_service.py new file mode 100644 index 00000000..0b7ba37e --- /dev/null +++ b/services/budget/app/services/export_template_service.py @@ -0,0 +1,51 @@ +from sqlalchemy.ext.asyncio import AsyncSession + +from app.crud.export_template_crud import ( + get_system_default_template, + list_export_templates, + list_shared_export_templates, +) +from app.models.budget import BudgetModel +from app.schemas.export_template_schema import ExportTemplateCandidate, TemplateSource +from app.services.donor_grantee_client import ( + DonorGranteeServiceError, + check_donor_grantee_relationship, +) + + +async def list_candidate_templates( + db: AsyncSession, valid_user: dict, budget: BudgetModel +) -> list[ExportTemplateCandidate]: + """System default + caller's own + funder's shared templates, live-gated (no caching).""" + customer_id = valid_user.get("customer_id") + assert customer_id is not None # guaranteed by get_viewable_budget_service upstream + candidates: list[ExportTemplateCandidate] = [] + + default = await get_system_default_template(db) + if default: + candidates.append(_to_candidate(default, TemplateSource.system)) + + for template in await list_export_templates(db, customer_id): + candidates.append(_to_candidate(template, TemplateSource.own)) + + donor_id = budget.funding_customer_id + if donor_id and str(donor_id) != str(customer_id): + try: + is_grantee = await check_donor_grantee_relationship(donor_id, budget.owner_id) + except DonorGranteeServiceError: + is_grantee = False + if is_grantee: + for template in await list_shared_export_templates(db, donor_id): + candidates.append(_to_candidate(template, TemplateSource.donor)) + + return candidates + + +def _to_candidate(template, source: TemplateSource) -> ExportTemplateCandidate: + return ExportTemplateCandidate( + id=template.id, + name=template.name, + visibility=template.visibility, + version=template.version, + source=source, + ) diff --git a/services/budget/main.py b/services/budget/main.py index e858570a..2bc11911 100644 --- a/services/budget/main.py +++ b/services/budget/main.py @@ -42,6 +42,10 @@ init_urls as customer_client_init_urls, close_urls as close_customer_client_urls, ) +from app.services.donor_grantee_client import ( # noqa: E402 + init_urls as donor_grantee_client_init_urls, + close_urls as close_donor_grantee_client_urls, +) from app.services.event_consumer import init_consumer, close_consumer, start_consumer # noqa: E402 from app.services.privileged_access_audit import write_privileged_access_log # noqa: E402 @@ -74,6 +78,8 @@ async def lifespan(app: FastAPI): logger.info("user_client_initialized") await customer_client_init_urls() logger.info("customer_client_initialized") + await donor_grantee_client_init_urls() + logger.info("donor_grantee_client_initialized") await init_consumer() logger.info("event_consumer_initialized") await start_consumer() @@ -87,6 +93,7 @@ async def lifespan(app: FastAPI): logger.info("app_shutdown", service="budget") await close_user_client_urls() await close_customer_client_urls() + await close_donor_grantee_client_urls() await close_consumer() logger.info("event_consumer_stopped") diff --git a/services/budget/migrations/versions/000017_add_export_templates.py b/services/budget/migrations/versions/000017_add_export_templates.py new file mode 100644 index 00000000..2d8038d3 --- /dev/null +++ b/services/budget/migrations/versions/000017_add_export_templates.py @@ -0,0 +1,80 @@ +"""Add export_templates table + seed system-default template (budget-feat-313-excel-export group 5) + +Revision ID: 000017 +Revises: 000016 +Create Date: 2026-09-25 00:00:00.000000 + +""" + +import json +import uuid +from datetime import datetime, timezone +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa +import shared.db.type_decorators + +revision: str = "000017" +down_revision: Union[str, Sequence[str], None] = "000016" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +# Reproduces group 1's fixed 3-sheet output exactly (design.md Decision 10). +_SYSTEM_DEFAULT_OPTIONS = { + "sheets": ["original_budget", "dashboard", "expense_list"], + "show_donor_currency_estimate": True, + "column_labels": {}, + "show_audit_footer": True, +} +_SYSTEM_DEFAULT_NAME = "GrandFlow Default" + + +def upgrade() -> None: + op.create_table( + "export_templates", + sa.Column("id", shared.db.type_decorators.GUID(), nullable=False), + sa.Column("owner_customer_id", shared.db.type_decorators.GUID(), nullable=True), + sa.Column("name", sa.String(), nullable=False), + sa.Column( + "visibility", + sa.Enum("private", "shared_with_grantees", name="export_template_visibility"), + nullable=False, + ), + sa.Column("options", sa.JSON(), nullable=False), + sa.Column("version", sa.Integer(), nullable=False, server_default=sa.text("1")), + sa.Column( + "created_at", sa.DateTime(timezone=True), server_default=sa.func.now(), nullable=False + ), + sa.Column("updated_at", sa.DateTime(timezone=True), nullable=True), + sa.Column("created_by", shared.db.type_decorators.GUID(), nullable=True), + sa.Column("updated_by", shared.db.type_decorators.GUID(), nullable=True), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint( + "owner_customer_id", "name", name="uq_export_templates_owner_customer_id_name" + ), + ) + op.create_index(op.f("ix_export_templates_id"), "export_templates", ["id"], unique=False) + + op.get_bind().execute( + sa.text( + """ + INSERT INTO export_templates + (id, owner_customer_id, name, visibility, options, version, created_at) + VALUES + (:id, NULL, :name, 'private', :options, 1, :created_at) + """ + ), + { + "id": str(uuid.uuid4()), + "name": _SYSTEM_DEFAULT_NAME, + "options": json.dumps(_SYSTEM_DEFAULT_OPTIONS), + "created_at": datetime.now(timezone.utc), + }, + ) + + +def downgrade() -> None: + op.drop_index(op.f("ix_export_templates_id"), table_name="export_templates") + op.drop_table("export_templates") + sa.Enum(name="export_template_visibility").drop(op.get_bind(), checkfirst=True) diff --git a/services/budget/migrations/versions/000018_add_export_template_system_default_flag.py b/services/budget/migrations/versions/000018_add_export_template_system_default_flag.py new file mode 100644 index 00000000..4e04e7a4 --- /dev/null +++ b/services/budget/migrations/versions/000018_add_export_template_system_default_flag.py @@ -0,0 +1,49 @@ +"""Add export_templates.is_system_default with single-default and owner constraints + +Revision ID: 000018 +Revises: 000017 +Create Date: 2026-09-25 00:00:00.000000 + +""" + +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + +revision: str = "000018" +down_revision: Union[str, Sequence[str], None] = "000017" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.add_column( + "export_templates", + sa.Column( + "is_system_default", sa.Boolean(), nullable=False, server_default=sa.text("false") + ), + ) + op.execute( + "UPDATE export_templates SET is_system_default = true WHERE owner_customer_id IS NULL" + ) + op.create_check_constraint( + "ck_export_templates_system_default_has_no_owner", + "export_templates", + "is_system_default = (owner_customer_id IS NULL)", + ) + op.create_index( + "uq_export_templates_single_system_default", + "export_templates", + ["is_system_default"], + unique=True, + postgresql_where=sa.text("is_system_default"), + ) + + +def downgrade() -> None: + op.drop_index("uq_export_templates_single_system_default", table_name="export_templates") + op.drop_constraint( + "ck_export_templates_system_default_has_no_owner", "export_templates", type_="check" + ) + op.drop_column("export_templates", "is_system_default") diff --git a/services/budget/tests/conftest.py b/services/budget/tests/conftest.py index 33615580..9be12400 100644 --- a/services/budget/tests/conftest.py +++ b/services/budget/tests/conftest.py @@ -27,6 +27,7 @@ ReportLineConversionAllocationModel, ) from app.models.privileged_access_log import PrivilegedAccessLog # noqa: E402 +from app.models.export_template import ExportTemplateModel # noqa: E402 from shared.security.dependencies import get_validated_user # noqa: E402 from shared.security.jwt_utils import create_access_token # noqa: E402 from tests.factories.user import ValidUserFactory # noqa: E402 @@ -71,6 +72,7 @@ async def db(): CurrencyConversionModel.__table__, ReportLineConversionAllocationModel.__table__, PrivilegedAccessLog.__table__, + ExportTemplateModel.__table__, ], ) maker = async_sessionmaker(engine, expire_on_commit=False) diff --git a/services/budget/tests/factories/export_template.py b/services/budget/tests/factories/export_template.py new file mode 100644 index 00000000..3d7f1066 --- /dev/null +++ b/services/budget/tests/factories/export_template.py @@ -0,0 +1,18 @@ +import factory +from uuid import uuid4 + +from app.models.export_template import ExportTemplateModel +from app.schemas.export_template_schema import ExportTemplateOptions, TemplateVisibility + + +class ExportTemplateFactory(factory.Factory): + class Meta: + model = ExportTemplateModel + + id = factory.LazyFunction(uuid4) + owner_customer_id = factory.LazyFunction(uuid4) + is_system_default = factory.LazyAttribute(lambda o: o.owner_customer_id is None) + name = factory.Faker("word") + visibility = TemplateVisibility.private + options = factory.LazyFunction(lambda: ExportTemplateOptions().model_dump(mode="json")) + version = 1 diff --git a/services/budget/tests/test_donor_grantee_gate.py b/services/budget/tests/test_donor_grantee_gate.py index 7e2d2e32..c5a5a748 100644 --- a/services/budget/tests/test_donor_grantee_gate.py +++ b/services/budget/tests/test_donor_grantee_gate.py @@ -14,8 +14,8 @@ from unittest.mock import AsyncMock, patch from uuid import uuid4 +import httpx import pytest -import requests from app.core.exceptions import DomainError from app.schemas.budget_schema import BudgetCreate, BudgetStatus @@ -45,39 +45,29 @@ def _payload(**kwargs): return BudgetCreate(**kwargs) -class _FakeResponse: - def __init__(self, payload): - self._payload = payload +def _fake_response(payload): + return httpx.Response(200, json=payload, request=httpx.Request("GET", "http://test")) - def raise_for_status(self): - pass - def json(self): - return self._payload +def _patch_client_get(**kwargs): + return patch( + "app.services.donor_grantee_client._client.get", new_callable=AsyncMock, **kwargs + ) class TestCheckDonorGranteeRelationship: def test_relationship_exists_returns_true(self): - with patch( - "app.services.donor_grantee_client.requests.get", - return_value=_FakeResponse({"exists": True}), - ): - assert check_donor_grantee_relationship(DONOR_ID, GRANTEE_ID) is True + with _patch_client_get(return_value=_fake_response({"exists": True})): + assert asyncio.run(check_donor_grantee_relationship(DONOR_ID, GRANTEE_ID)) is True def test_relationship_missing_returns_false(self): - with patch( - "app.services.donor_grantee_client.requests.get", - return_value=_FakeResponse({"exists": False}), - ): - assert check_donor_grantee_relationship(DONOR_ID, GRANTEE_ID) is False + with _patch_client_get(return_value=_fake_response({"exists": False})): + assert asyncio.run(check_donor_grantee_relationship(DONOR_ID, GRANTEE_ID)) is False def test_request_failure_raises_service_error(self): - with patch( - "app.services.donor_grantee_client.requests.get", - side_effect=requests.RequestException("boom"), - ): + with _patch_client_get(side_effect=httpx.ConnectError("boom")): with pytest.raises(DonorGranteeServiceError): - check_donor_grantee_relationship(DONOR_ID, GRANTEE_ID) + asyncio.run(check_donor_grantee_relationship(DONOR_ID, GRANTEE_ID)) class TestValidateDonorGranteeRelationship: @@ -86,7 +76,7 @@ def test_existing_relationship_passes(self): "app.services.donor_grantee_client.check_donor_grantee_relationship", return_value=True, ): - validate_donor_grantee_relationship(DONOR_ID, GRANTEE_ID) # does not raise + asyncio.run(validate_donor_grantee_relationship(DONOR_ID, GRANTEE_ID)) # does not raise def test_missing_relationship_raises_value_error(self): with patch( @@ -94,7 +84,7 @@ def test_missing_relationship_raises_value_error(self): return_value=False, ): with pytest.raises(ValueError): - validate_donor_grantee_relationship(DONOR_ID, GRANTEE_ID) + asyncio.run(validate_donor_grantee_relationship(DONOR_ID, GRANTEE_ID)) def test_missing_relationship_raises_domain_error_when_flagged(self): with patch( @@ -102,7 +92,11 @@ def test_missing_relationship_raises_domain_error_when_flagged(self): return_value=False, ): with pytest.raises(DomainError): - validate_donor_grantee_relationship(DONOR_ID, GRANTEE_ID, raise_domain_error=True) + asyncio.run( + validate_donor_grantee_relationship( + DONOR_ID, GRANTEE_ID, raise_domain_error=True + ) + ) def test_service_error_raises_domain_error_when_flagged(self): with patch( @@ -110,7 +104,11 @@ def test_service_error_raises_domain_error_when_flagged(self): side_effect=DonorGranteeServiceError("unreachable"), ): with pytest.raises(DomainError): - validate_donor_grantee_relationship(DONOR_ID, GRANTEE_ID, raise_domain_error=True) + asyncio.run( + validate_donor_grantee_relationship( + DONOR_ID, GRANTEE_ID, raise_domain_error=True + ) + ) class TestCreateBudgetServiceGate: diff --git a/services/budget/tests/test_excel_export_service.py b/services/budget/tests/test_excel_export_service.py index 81a0392f..d06b2868 100644 --- a/services/budget/tests/test_excel_export_service.py +++ b/services/budget/tests/test_excel_export_service.py @@ -9,6 +9,7 @@ from app.crud.excel_export_crud import ReportLineAllocationDetail, ReportLineExpense from app.models.budget import BudgetCategoryModel, BudgetLineModel, BudgetModel from app.schemas.budget_schema import BudgetStatus +from app.schemas.export_template_schema import TemplateVisibility from app.services.excel_export_service import ( SHEET3_TITLE, DashboardSheet, @@ -20,6 +21,7 @@ ) from tests.factories.budget import BudgetCategoryFactory, BudgetFactory, BudgetLineFactory from tests.factories.currency_ledger import CurrencyConversionFactory, FundingReceiptFactory +from tests.factories.export_template import ExportTemplateFactory OWNER_ID = str(uuid4()) FUNDER_ID = str(uuid4()) @@ -1342,3 +1344,45 @@ async def test_stranger_is_rejected(self, db, make_client): response = client.get(f"/api/v1/budgets/{budget.id}/export.xlsx") assert response.status_code == 400 + + +@pytest.mark.anyio +class TestExportTemplatesRoute: + async def test_owner_gets_predicted_candidate_set(self, db, make_client): + budget = await _make_budget(db, funding_customer_id=FUNDER_ID) + await _add_export_template(db, owner_customer_id=None, name="GrandFlow Default") + await _add_export_template(db, owner_customer_id=OWNER_ID, name="My Template") + client = make_client(db=db, customer_id=OWNER_ID) + + response = client.get(f"/api/v1/budgets/{budget.id}/export-templates") + + assert response.status_code == 200 + sources = {c["source"] for c in response.json()} + assert sources == {"system", "own"} + + async def test_funder_gets_predicted_candidate_set(self, db, make_client): + budget = await _make_budget(db, funding_customer_id=FUNDER_ID) + await _add_export_template(db, owner_customer_id=None, name="GrandFlow Default") + client = make_client(db=db, customer_id=FUNDER_ID) + + response = client.get(f"/api/v1/budgets/{budget.id}/export-templates") + + assert response.status_code == 200 + assert {c["source"] for c in response.json()} == {"system"} + + async def test_non_viewer_is_rejected(self, db, make_client): + budget = await _make_budget(db) + client = make_client(db=db, customer_id=STRANGER_ID) + + response = client.get(f"/api/v1/budgets/{budget.id}/export-templates") + + assert response.status_code == 400 + + +async def _add_export_template(db, owner_customer_id, name, visibility=TemplateVisibility.private): + template = ExportTemplateFactory.build( + owner_customer_id=owner_customer_id, name=name, visibility=visibility + ) + db.add(template) + await db.commit() + return template diff --git a/services/budget/tests/test_export_template_crud.py b/services/budget/tests/test_export_template_crud.py new file mode 100644 index 00000000..3982b0a5 --- /dev/null +++ b/services/budget/tests/test_export_template_crud.py @@ -0,0 +1,58 @@ +from uuid import uuid4 + +import pytest + +from app.core.exceptions import DomainError +from app.crud.export_template_crud import ( + create_export_template, + delete_export_template, + get_export_template, + update_export_template, +) +from app.schemas.export_template_schema import ExportTemplateOptions, TemplateVisibility +from tests.factories.export_template import ExportTemplateFactory + +pytestmark = pytest.mark.anyio + + +class TestScoping: + async def test_org_a_cannot_read_org_bs_private_template(self, db): + org_a, org_b = uuid4(), uuid4() + template = ExportTemplateFactory.build(owner_customer_id=org_b, name="Board Report") + db.add(template) + await db.commit() + + assert await get_export_template(db, org_a, template.id) is None + assert await get_export_template(db, org_b, template.id) is not None + + +class TestVersionBump: + async def test_update_increments_version(self, db): + owner_id = uuid4() + template = await create_export_template( + db, owner_id, "Draft", TemplateVisibility.private, ExportTemplateOptions() + ) + assert template.version == 1 + + updated = await update_export_template(db, owner_id, template.id, name="Final") + + assert updated.version == 2 + assert updated.name == "Final" + + +class TestSystemDefaultGuard: + async def test_editing_the_system_default_is_rejected(self, db): + default = ExportTemplateFactory.build(owner_customer_id=None, name="GrandFlow Default") + db.add(default) + await db.commit() + + with pytest.raises(DomainError): + await update_export_template(db, uuid4(), default.id, name="Hacked") + + async def test_deleting_the_system_default_is_rejected(self, db): + default = ExportTemplateFactory.build(owner_customer_id=None, name="GrandFlow Default") + db.add(default) + await db.commit() + + with pytest.raises(DomainError): + await delete_export_template(db, uuid4(), default.id) diff --git a/services/budget/tests/test_export_template_model.py b/services/budget/tests/test_export_template_model.py new file mode 100644 index 00000000..724392b1 --- /dev/null +++ b/services/budget/tests/test_export_template_model.py @@ -0,0 +1,48 @@ +from uuid import uuid4 + +import pytest +from sqlalchemy.exc import IntegrityError + +from tests.factories.export_template import ExportTemplateFactory + +pytestmark = pytest.mark.anyio + + +async def _add_template(db, owner_customer_id, name="Annual Report"): + template = ExportTemplateFactory.build(owner_customer_id=owner_customer_id, name=name) + db.add(template) + await db.commit() + return template + + +class TestExportTemplateUniqueness: + async def test_duplicate_name_within_one_organisation_is_rejected(self, db): + owner_id = uuid4() + await _add_template(db, owner_id) + + with pytest.raises(IntegrityError): + await _add_template(db, owner_id) + + async def test_same_name_accepted_across_two_organisations(self, db): + await _add_template(db, uuid4()) + await _add_template(db, uuid4()) + + +class TestSystemDefaultConstraints: + async def test_a_second_system_default_is_rejected(self, db): + await _add_template(db, None, "GrandFlow Default") + + with pytest.raises(IntegrityError): + await _add_template(db, None, "Another Default") + + async def test_system_default_with_an_owner_is_rejected(self, db): + db.add(ExportTemplateFactory.build(owner_customer_id=uuid4(), is_system_default=True)) + + with pytest.raises(IntegrityError): + await db.commit() + + async def test_ownerless_non_default_is_rejected(self, db): + db.add(ExportTemplateFactory.build(owner_customer_id=None, is_system_default=False)) + + with pytest.raises(IntegrityError): + await db.commit() diff --git a/services/budget/tests/test_export_template_schema.py b/services/budget/tests/test_export_template_schema.py new file mode 100644 index 00000000..6009bc0b --- /dev/null +++ b/services/budget/tests/test_export_template_schema.py @@ -0,0 +1,29 @@ +import pytest +from pydantic import ValidationError + +from app.schemas.export_template_schema import ExportSheet, ExportTemplateOptions + + +class TestExportTemplateOptions: + def test_unknown_key_rejected(self): + with pytest.raises(ValidationError): + ExportTemplateOptions(made_up_option=True) + + def test_missing_optional_keys_validate_to_system_default_values(self): + options = ExportTemplateOptions() + + assert options.sheets == [ + ExportSheet.original_budget, + ExportSheet.dashboard, + ExportSheet.expense_list, + ] + assert options.show_donor_currency_estimate is True + assert options.column_labels == {} + assert options.show_audit_footer is True + + def test_partial_blob_fills_remaining_fields_with_defaults(self): + options = ExportTemplateOptions(sheets=[ExportSheet.original_budget]) + + assert options.sheets == [ExportSheet.original_budget] + assert options.show_donor_currency_estimate is True + assert options.show_audit_footer is True diff --git a/services/budget/tests/test_export_template_service.py b/services/budget/tests/test_export_template_service.py new file mode 100644 index 00000000..3925109d --- /dev/null +++ b/services/budget/tests/test_export_template_service.py @@ -0,0 +1,113 @@ +from unittest.mock import patch +from uuid import uuid4 + +import pytest + +from app.schemas.export_template_schema import TemplateSource, TemplateVisibility +from app.services.export_template_service import list_candidate_templates +from tests.factories.budget import BudgetFactory +from tests.factories.export_template import ExportTemplateFactory + +pytestmark = pytest.mark.anyio + + +async def _make_budget(db, owner_id, funding_customer_id=None): + budget = BudgetFactory.build(owner_id=owner_id, funding_customer_id=funding_customer_id) + db.add(budget) + await db.commit() + await db.refresh(budget) + return budget + + +async def _add_template(db, owner_customer_id, name, visibility=TemplateVisibility.private): + template = ExportTemplateFactory.build( + owner_customer_id=owner_customer_id, name=name, visibility=visibility + ) + db.add(template) + await db.commit() + return template + + +class TestListCandidateTemplates: + async def test_grantee_sees_funders_shared_template(self, db): + grantee_id, donor_id = uuid4(), uuid4() + budget = await _make_budget(db, owner_id=grantee_id, funding_customer_id=donor_id) + shared = await _add_template( + db, donor_id, "Donor Report", TemplateVisibility.shared_with_grantees + ) + + with patch( + "app.services.export_template_service.check_donor_grantee_relationship", + return_value=True, + ): + candidates = await list_candidate_templates( + db, {"customer_id": grantee_id}, budget + ) + + donor_candidates = [c for c in candidates if c.source == TemplateSource.donor] + assert [c.id for c in donor_candidates] == [shared.id] + + async def test_donors_private_template_is_not_offered(self, db): + grantee_id, donor_id = uuid4(), uuid4() + budget = await _make_budget(db, owner_id=grantee_id, funding_customer_id=donor_id) + await _add_template(db, donor_id, "Internal Only", TemplateVisibility.private) + + with patch( + "app.services.export_template_service.check_donor_grantee_relationship", + return_value=True, + ): + candidates = await list_candidate_templates( + db, {"customer_id": grantee_id}, budget + ) + + assert [c for c in candidates if c.source == TemplateSource.donor] == [] + + async def test_unrelated_organisations_shared_template_is_not_offered(self, db): + grantee_id, donor_id, unrelated_id = uuid4(), uuid4(), uuid4() + budget = await _make_budget(db, owner_id=grantee_id, funding_customer_id=donor_id) + await _add_template( + db, unrelated_id, "Unrelated Org Template", TemplateVisibility.shared_with_grantees + ) + + with patch( + "app.services.export_template_service.check_donor_grantee_relationship", + return_value=True, + ): + candidates = await list_candidate_templates( + db, {"customer_id": grantee_id}, budget + ) + + assert [c for c in candidates if c.source == TemplateSource.donor] == [] + + async def test_revoked_relationship_drops_template_on_next_call_with_no_cache(self, db): + grantee_id, donor_id = uuid4(), uuid4() + budget = await _make_budget(db, owner_id=grantee_id, funding_customer_id=donor_id) + await _add_template( + db, donor_id, "Donor Report", TemplateVisibility.shared_with_grantees + ) + + with patch( + "app.services.export_template_service.check_donor_grantee_relationship", + return_value=True, + ): + first_call = await list_candidate_templates(db, {"customer_id": grantee_id}, budget) + with patch( + "app.services.export_template_service.check_donor_grantee_relationship", + return_value=False, + ): + second_call = await list_candidate_templates(db, {"customer_id": grantee_id}, budget) + + assert any(c.source == TemplateSource.donor for c in first_call) + assert not any(c.source == TemplateSource.donor for c in second_call) + + async def test_system_default_and_own_templates_always_included(self, db): + grantee_id = uuid4() + budget = await _make_budget(db, owner_id=grantee_id) + await _add_template(db, None, "GrandFlow Default") + await _add_template(db, grantee_id, "My Template") + + candidates = await list_candidate_templates(db, {"customer_id": grantee_id}, budget) + + sources = {c.source for c in candidates} + assert TemplateSource.system in sources + assert TemplateSource.own in sources From 02c39313017acc9a3c738fe3c5ce7ba44f86e162 Mon Sep 17 00:00:00 2001 From: Norair Arutshyan <n.arutshyan@gmail.com> Date: Fri, 25 Sep 2026 14:17:41 +0100 Subject: [PATCH 2/2] style(export): black-format export_template model imports Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- services/budget/app/models/export_template.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/services/budget/app/models/export_template.py b/services/budget/app/models/export_template.py index e4496497..6a5b8e47 100644 --- a/services/budget/app/models/export_template.py +++ b/services/budget/app/models/export_template.py @@ -2,7 +2,14 @@ import uuid from sqlalchemy import ( - Boolean, CheckConstraint, Enum as SQLEnum, Index, Integer, JSON, String, UniqueConstraint, + Boolean, + CheckConstraint, + Enum as SQLEnum, + Index, + Integer, + JSON, + String, + UniqueConstraint, text, ) from sqlalchemy.orm import Mapped, mapped_column