From 23f862f6035f7221d058e03d5cc1bb664572cc1f Mon Sep 17 00:00:00 2001 From: Norair Arutshyan Date: Wed, 23 Sep 2026 13:08:12 +0100 Subject: [PATCH 1/4] feat(export): add single-budget Excel export with async customer client - excel_export_service.py builds Sheet 1 (Original Budget) from a budget's categories/lines, with formula-linked summary/detail/footer sections - customer_client.py converted from requests+lru_cache to httpx.AsyncClient with a manual LRU cache, wired into main.py's lifespan (matches user_client.py's existing pattern); all callers/tests updated to await it - code-review fixes: guard external service calls in the export path, treat duration_months None/0 identically, drop dead rate_cell param, order budget categories/lines by created_at/id in SQL instead of re-sorting in Python, share the start+duration date-math formula between _compute_end_date and _period_label, simplify redundant limit=None branches, restore the auth rationale comment on /budgets/by-creator/{user_id} Co-Authored-By: Claude Sonnet 5 --- .../budget-feat-313-excel-export/design.md | 36 +- .../budget-feat-313-excel-export/proposal.md | 19 +- .../specs/budget-excel-export-ui/spec.md | 38 +- .../specs/budget-excel-export/spec.md | 104 ++++- .../budget-feat-313-excel-export/tasks.md | 38 +- .../.openspec.yaml | 4 + .../proposal.md | 25 ++ .../specs/budget-customer-cache/spec.md | 32 ++ .../tasks.md | 12 + services/budget/app/api/budget_routes.py | 26 +- .../budget/app/crud/budget_category_crud.py | 9 +- services/budget/app/crud/budget_line_crud.py | 11 +- .../budget/app/services/budget_services.py | 17 +- .../budget/app/services/customer_client.py | 71 ++- .../app/services/excel_export_service.py | 410 ++++++++++++++++++ services/budget/main.py | 7 + .../tests/test_budget_currency_fields.py | 6 +- .../tests/test_budget_donor_commitment.py | 25 +- services/budget/tests/test_budget_services.py | 3 + .../budget/tests/test_customer_validation.py | 44 +- .../budget/tests/test_donor_grantee_gate.py | 26 +- .../budget/tests/test_excel_export_service.py | 371 ++++++++++++++++ 22 files changed, 1250 insertions(+), 84 deletions(-) create mode 100644 openspec/changes/budget-feat-customer-profile-cache/.openspec.yaml create mode 100644 openspec/changes/budget-feat-customer-profile-cache/proposal.md create mode 100644 openspec/changes/budget-feat-customer-profile-cache/specs/budget-customer-cache/spec.md create mode 100644 openspec/changes/budget-feat-customer-profile-cache/tasks.md create mode 100644 services/budget/app/services/excel_export_service.py create mode 100644 services/budget/tests/test_excel_export_service.py diff --git a/openspec/changes/budget-feat-313-excel-export/design.md b/openspec/changes/budget-feat-313-excel-export/design.md index 75267a7c..99bffdd3 100644 --- a/openspec/changes/budget-feat-313-excel-export/design.md +++ b/openspec/changes/budget-feat-313-excel-export/design.md @@ -8,15 +8,20 @@ See proposal.md - Why. Relevant existing data model (all in `services/budget/app - No existing per-budget-line planned-vs-actual rollup query exists; `dashboard_crud.budget_breakdown` is the closest precedent but aggregates cross-budget, not per-line. - `openpyxl==3.1.5` is already a dependency (currently import-only, via `excel_import_service.py`); no new package needed for writing. - `attachment_routes.py` already establishes the `StreamingResponse` file-download pattern for this service. +- `DonorGranteeModel` (`services/users/app/models/customer.py`) records which NGOs a donor funds; the budget service already reaches it through `donor_grantee_client.check_donor_grantee_relationship`, deliberately uncached so a revoked relationship takes effect on the very next call. +- The existing `donor_templates` table (`budget_donor_template_crud.py`, migration `000012_add_donor_template_fingerprint.py`) is a different thing wearing a similar name: an *import*-side layout fingerprint cache, not an export template. Its known problems (no tenant scoping, no uniqueness, no review gate) are tracked separately in the `budget-template-storing` proposal and are not addressed here. ## Goals / Non-Goals **Goals:** -- Generate one workbook per request, entirely from live data, with no new tables or stored artifacts. +- Generate one workbook per request, entirely from live data. The only stored artifact is the template that selects the export's options — never the generated workbook itself. +- Make today's fixed format the seeded system-default template, so multi-template support adds a row rather than a branch. - Reuse the currency-ledger's real allocation data wherever it exists; only fall back to the planning-time estimate for the genuinely unresolved gap, and mark that gap visibly. **Non-Goals:** - Donor-template-shaped round-trip export (deferred; see proposal). +- A declarative layout engine. Templates vary bounded renderer options, not arbitrary cell layout (see Decision 9). +- Per-grantee template shares. Visibility is all-of-a-donor's-grantees or private (see Decision 7). - Caching, background generation, or emailing the file — synchronous request/response only, matching the size of a single budget's data. - Any change to how receipts, conversions, or allocations are recorded (`ledger-budget`'s in-flight edit/delete/reset work is unrelated and unaffected). @@ -35,8 +40,37 @@ A single budget's data (lines, ledger, report lines) is small; no need for the a **4. Sheet 2's estimated-portion flag is a cell style (e.g. italic + light fill + a legend row), not a separate column.** Keeps the sheet's column count matching the user's I/J/K scope instead of doubling columns for a rare partial-estimate case. +**5. Sheet 1 is a live formula chain, not values computed by openpyxl: every number above the raw per-line amount is an Excel formula, not a Python-computed literal.** +Per-line local amount is the only literal value. Each category's Detailed Budget subtotal is `=SUM()`; each line's donor-currency estimate is `=/`; each subtotal's estimate is `=SUM()`. Budget Summary doesn't recompute anything — each category row there is a direct cell reference to that category's own Detailed Budget subtotal (`=C19`/`=D19`), and its TOTAL row is `=SUM(...)` over those reference rows. The footer's "Total expenditures" row adds the Detailed Budget subtotal cells directly (`=C19+C25`), independent of Budget Summary's TOTAL, mirroring the hand-built reference file (`uploads/budget/Demo budget 3 (1).xlsx`) the user supplied as the exact target — one deviation: that file's Budget Summary rows both referenced the *same* subtotal cell (a copy-paste slip), corrected here so each category row references its own. Distinct from the deferred donor-template-replay change (`budget-export-from-excel`'s follow-on), where static values were floated specifically to dodge `openpyxl.insert_rows()` not adjusting existing formula ranges on row insertion — Sheet 1 is generated fresh on every request (no row insertion into a pre-existing file), so that risk doesn't apply, and a live chain lets a donor hand-edit any line amount or the rate cell and see every subtotal/total/estimate recalculate. Trade-off: tests assert formula strings for every non-leaf cell, since openpyxl never evaluates formulas itself (verified end-to-end by round-tripping a sample workbook through `soffice --headless --convert-to csv`, confirming LibreOffice's formula engine actually computes the expected totals, not just that the formula strings parse). + +**6. Header's organisation/donor name comes from `customer_client.get_customer_cached`, not a new field on `BudgetModel`.** +`owner_id`/`funding_customer_id` already resolve to a customer name via the existing service-to-service lookup (same pattern as `budget_services.py`'s `local_currency` fallback); no new cross-service plumbing needed. `external_funder_name` is the fallback when `funding_customer_id` is unset. "Project Ref.no." was considered and deliberately dropped — no backing field exists on `BudgetModel` and none was added for this change. + +**7. Templates are owner-scoped rows with a coarse `visibility` enum; per-grantee shares are deferred behind an unchanged resolver signature.** +`export_templates` carries `owner_customer_id`, `name`, `visibility` (`private` | `shared_with_grantees`), and the options blob, unique on `(owner_customer_id, name)`. A grantee may use a donor's template only when that template is `shared_with_grantees` **and** `check_donor_grantee_relationship(donor, grantee)` returns true — the relationship is the access boundary, so revoking it revokes template access immediately, with no cache to expire. Every CRUD function takes the actor's `customer_id` in its signature, so an unscoped query is not expressible; this is a direct response to the unscoped-lookup defect found in `donor_templates` (see `budget-template-storing` problem 1). +*Alternative considered*: an `export_template_shares` join table for per-grantee grants up front. Deferred — the stated need is "a donor publishes this to its grantees", which the enum covers. The resolver is specified as `list_candidate_templates(actor, budget) -> list[Template]` precisely so adding the join table later changes that function's body and nothing above it. + +**8. Template selection is always explicit; there is no silent fallback cascade.** +`GET /budgets/{budget_id}/export-templates` returns the candidates (each tagged `system` | `own` | `donor`). On export: `template_id` omitted with exactly one candidate uses it; omitted with more than one is rejected (400); an id outside the candidate list is rejected (403). No precedence rule between "my org's template" and "my funder's template" is defined, because nothing ever picks between them automatically. +*Alternative considered*: a resolution cascade (explicit → budget default → donor default → org default → system). Rejected — it forces an arbitrary and contestable precedence decision between a grantee's own default and its funder's, and it makes the template that produced a given file an inference rather than a recorded choice. +*Forward note*: a remembered per-budget choice is planned, but as a **client-side pre-fill of the picker**, not a server-side implicit pick. The server stays strict. This keeps the audit trail unambiguous and turns "my saved template was un-shared or deleted" into a clean 400 asking the user to choose again, instead of a silent format change on an export they assumed was routine. + +**9. A template's body is a bounded renderer-options blob, not a layout description.** +The blob names the renderer plus options over it: which sheets to include, whether the donor-currency estimate column is shown, per-column header label overrides, and whether the audit footer is shown. It is validated against a Pydantic schema on write, so an unknown key is rejected at the API rather than silently ignored at render time. +*Alternative considered*: the full declarative JSON layout engine (region map + semantic formula anchors + named styles). Deferred to its own change. The prerequisite is a golden characterization test over today's output — a cell-by-cell snapshot (value, number format, bold, fill, border) across the edge cases this code already handles: no categories, a category with zero lines, unset `estimated_exchange_rate`, `extra_fields` present and absent, null currency. Without that oracle there is no way to demonstrate the engine reproduces the current format exactly, so building the engine first would be building blind. + +**10. The system default is a seeded row, not a special case in code.** +Migration seeds one `export_templates` row (`owner_customer_id` NULL, `visibility` effectively global) whose options reproduce today's output exactly. `generate_budget_export_workbook` therefore always runs against a template — existing tests keep passing because the seeded options are the current behaviour. There is exactly one rendering path, and the default is continuously exercised by every export. + +**11. The template that produced a workbook is recorded in the workbook itself, not in a new table.** +`_audit_line()` already writes "Generated by OpenGrantFlow · · "; it gains the template name and version. Templates are versioned (an edit bumps `version`), so a file exported last quarter stays explicable after its template is edited. +*Alternative considered*: an `export_history` table recording every generated file. Rejected as scope creep — nothing currently needs to enumerate past exports, and the footer answers the question the audit trail actually asks. + ## Risks / Trade-offs - [A budget with many report lines/allocations could make generation slow] → Out of scope for a "simple export" of one budget; single-budget expense volume in practice is small (tens to low hundreds of lines), revisit only if real usage shows otherwise. - [`estimated_exchange_rate` unset on an older or draft budget leaves Sheet 1's donor-currency column and Sheet 2's deviation column blank for that budget] → Matches existing GrandFlow convention (donor dashboard already excludes rather than fabricates); the export's local-currency figures are still fully populated. - [Category subtotal rows in Sheet 2 need the same rollup as Sheet 1's category grouping] → Reuse the same category-grouping logic/order between Sheet 1 and Sheet 2 rather than deriving it twice, to avoid the two sheets silently disagreeing on category order or membership. +- [A grantee's remembered template is later un-shared, deleted, or its donor relationship revoked] → The strict server (Decision 8) turns this into a 400 that names the problem and asks for a new selection, rather than silently exporting a different format. The picker must surface that message inline, not as a generic failure. +- [The options blob is a schema that will grow, and old rows will lag it] → Validate on write against a versioned Pydantic schema and treat every option as optional-with-a-default at render time, so a row written before an option existed still renders. Adding an option must never require a data migration. +- [Renderer options interact with Sheets 2/3, which are not built yet] → The sheet-subset option is specified now but only becomes meaningful once groups 2 and 3 land; group 6 depends on them for that reason. Until then the option validates but has a single legal value. diff --git a/openspec/changes/budget-feat-313-excel-export/proposal.md b/openspec/changes/budget-feat-313-excel-export/proposal.md index 65ed7539..4791f21d 100644 --- a/openspec/changes/budget-feat-313-excel-export/proposal.md +++ b/openspec/changes/budget-feat-313-excel-export/proposal.md @@ -10,23 +10,28 @@ A grantee owner has no way to hand a single budget's numbers to a donor, auditor - **Sheet 1 — Original Budget**: budget lines grouped by category with subtotals, mirroring the example's layout. Each line shows its amount in the budget's `local_currency` plus a derived donor-currency estimate column (`amount ÷ estimated_exchange_rate`), consistent with how GrandFlow already treats `estimated_exchange_rate` elsewhere as an approximate, non-stored conversion (donor dashboard, budget-line toggle). - **Sheet 2 — Budget vs. Report Dashboard**: per budget-line (and category subtotal) rows with only 3 result columns (matching the example's `I`/`J`/`K`; its interim/final-report date-range split in `E–H` is out of scope): Total Expenses (`local_currency`, direct sum of `report_lines.amount`), Total Expenses Converted (donor currency — real ledger rate for the portion covered by `ReportLineConversionAllocation`, falling back to `estimated_exchange_rate` for any unsatisfied remainder, with that estimated portion visually flagged so it's never mistaken for a real bank rate), and Deviation (budgeted-in-donor-currency minus converted actual). An income section lists every recorded `CurrencyConversion` as its own row (date, donor amount, local amount, implied rate) rather than one row per receipt, since one receipt can convert across several dates/rates. - **Sheet 3 — List of Expenses**: one row per report-line expense; an expense whose payment was funded by more than one currency-conversion lot gets one row per allocation (subline), each carrying its own conversion date, rate, and converted amount, directly mirroring the real `ReportLineConversionAllocation` data with no aggregation. -- Frontend: an "Export to Excel" button on the single-budget view that downloads the generated file. +- **Multi-template export**: a new `export_templates` table lets an organisation save named export templates. A template selects the GrandFlow renderer plus a small options blob (which sheets to include, whether to show the donor-currency estimate column, custom column header labels, whether to show the audit footer) — not a free-form layout. A seeded system-default template reproduces today's fixed 3-sheet output, so the default becomes the first row in the table rather than a branch in the code. +- **Donor→grantee sharing**: a template's `visibility` (`private` | `shared_with_grantees`) controls whether a donor's template is offered to the NGOs that donor already funds, reusing the existing `donor_grantees` relationship as the access boundary. +- Add `GET /budgets/{budget_id}/export-templates`: the templates this viewer may use for this budget (the system default, their own organisation's, and any shared by the budget's funder). +- `GET /budgets/{budget_id}/export.xlsx` accepts an optional `template_id`. Selection is always explicit: when more than one candidate exists and none is given, the request is rejected rather than silently defaulting to one. +- Frontend: an "Export to Excel" action on the single-budget view that downloads the generated file, presenting a template picker when more than one template is available; plus a template management view for creating, editing, and sharing an organisation's own templates. - Wire the new route into all three gateway configs (`nginx-dev.conf`, `nginx.conf`, `Caddyfile`). -**Explicitly out of scope**: regenerating a budget back into a *donor's own* Excel layout (the round-trip feature already deferred by the archived `budget-export-from-excel` — actually an import feature — proposal). This is a new, fixed GrandFlow-authored export format, not a donor-template replay. +**Explicitly out of scope**: regenerating a budget back into a *donor's own* Excel layout (the round-trip feature already deferred by the archived `budget-export-from-excel` — actually an import feature — proposal), and the declarative JSON layout engine that would eventually let a donor describe arbitrary layouts. Templates here vary a GrandFlow-authored format through bounded options; they do not replay a donor's spreadsheet. Also out of scope: per-grantee template shares (a template is shared with all of a donor's grantees, or with none) and uploaded `.xlsx` skeletons. ## Capabilities ### New Capabilities -- `budget-excel-export`: backend generation of the 3-sheet workbook from a budget's lines, ledger, and report-line data. -- `budget-excel-export-ui`: the frontend entry point (button + download) that triggers the export on a single-budget view. +- `budget-excel-export`: backend generation of the 3-sheet workbook from a budget's lines, ledger, and report-line data, plus the template selection and renderer options that vary it. +- `budget-export-templates`: creating, editing, and sharing an organisation's export templates. *(Spec delta not yet authored — see tasks group 5.)* +- `budget-excel-export-ui`: the frontend entry point (template picker + download) on a single-budget view, and the template management view. ### Modified Capabilities (none — this only reads existing `budget-currency-ledger`, `budget-reports`, and `budget-categories` data; no requirement in those specs changes) ## Impact -- **Backend**: new `services/budget/app/services/excel_export_service.py` (workbook generation), new per-budget-line rollup query (join `budget_lines`→`report_lines`, no existing precedent — closest is `dashboard_crud.budget_breakdown`, which is cross-budget, not per-line), new route in `budget_routes.py` (`GET /{budget_id}/export.xlsx`), reusing the `StreamingResponse` pattern from `attachment_routes.py`. -- **Frontend**: one button + fetch/download handler on the budget detail view; no new page. +- **Backend**: new `services/budget/app/services/excel_export_service.py` (workbook generation), new per-budget-line rollup query (join `budget_lines`→`report_lines`, no existing precedent — closest is `dashboard_crud.budget_breakdown`, which is cross-budget, not per-line), new route in `budget_routes.py` (`GET /{budget_id}/export.xlsx`), reusing the `StreamingResponse` pattern from `attachment_routes.py`. Adds `ExportTemplateModel` + CRUD, a candidate-resolution service reusing `donor_grantee_client.check_donor_grantee_relationship`, and a `GET /{budget_id}/export-templates` route. +- **Frontend**: template picker + fetch/download handler on the budget detail view, plus a new template management view. - **Gateway**: route addition to `nginx-dev.conf`, `nginx.conf`, `Caddyfile`. -- **No schema/migration changes** — pure read of existing tables. +- **Schema**: one migration adding `export_templates` (owner-scoped, unique on `(owner_customer_id, name)`) and seeding the system-default row. Workbook generation itself stays a pure read of existing tables — no generated file is ever stored. diff --git a/openspec/changes/budget-feat-313-excel-export/specs/budget-excel-export-ui/spec.md b/openspec/changes/budget-feat-313-excel-export/specs/budget-excel-export-ui/spec.md index dccfa0c0..0781ad94 100644 --- a/openspec/changes/budget-feat-313-excel-export/specs/budget-excel-export-ui/spec.md +++ b/openspec/changes/budget-feat-313-excel-export/specs/budget-excel-export-ui/spec.md @@ -2,11 +2,11 @@ ## Purpose -Gives a budget owner or funder viewer a visible way to trigger and download the single-budget Excel export from the budget detail view. +Gives a budget owner or funder viewer a visible way to choose an export template, trigger the single-budget Excel export, and download it from the budget detail view — and gives an organisation a place to manage and share its own templates. ## ADDED Requirements -### Requirement: Export button on budget detail view +### Requirement: Export action on budget detail view The frontend SHALL show an "Export to Excel" action on the single-budget detail view, visible to anyone with read access to that budget (owner or funder), that downloads the generated workbook via `GET /budgets/{budget_id}/export.xlsx`. #### Scenario: Owner triggers export @@ -23,3 +23,37 @@ The frontend SHALL show an inline error, without navigating away from the budget #### Scenario: Backend error surfaced - **WHEN** the export endpoint returns an error - **THEN** the frontend shows an inline error message and the user remains on the budget detail view + +### Requirement: Template picker before export +When more than one export template is available for a budget, the frontend SHALL require the viewer to choose one before the export request is sent, showing each option's source (the built-in default, the viewer's own organisation, or the budget's funder). When only one template is available, the frontend SHALL export directly without prompting. + +#### Scenario: Multiple templates available +- **WHEN** a viewer with access to more than one template clicks "Export to Excel" +- **THEN** the frontend presents the available templates, labelled by source, and sends the export request only once one is chosen + +#### Scenario: Only the default available +- **WHEN** a viewer has access to no templates beyond the built-in default +- **THEN** clicking "Export to Excel" starts the download immediately, with no picker shown + +#### Scenario: Chosen template no longer available +- **WHEN** the export is rejected because the chosen template has been deleted or un-shared +- **THEN** the frontend shows that reason inline and reopens the picker with the current options, rather than showing a generic failure + +### Requirement: Template management view +The frontend SHALL provide a view where an organisation's authorised users can create, rename, edit, and delete that organisation's own export templates, and set each one's visibility to either private or shared with the organisation's grantees. The built-in default SHALL be shown as read-only. + +#### Scenario: Donor shares a template with its grantees +- **WHEN** a donor's authorised user sets one of their templates to shared with grantees +- **THEN** that template becomes selectable by the NGOs that donor funds, on budgets the donor funds + +#### Scenario: Another organisation's templates are not listed +- **WHEN** a user opens the template management view +- **THEN** only their own organisation's templates and the read-only built-in default are listed, with no template owned by any other organisation shown or editable + +#### Scenario: Built-in default is read-only +- **WHEN** a user views the built-in default template in the management view +- **THEN** it is displayed without edit or delete controls + +#### Scenario: Duplicate name rejected inline +- **WHEN** a user saves a template using a name their organisation already uses +- **THEN** the frontend shows the validation error inline on the form without losing the entered values 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 0a30064f..a8b13610 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 @@ -2,7 +2,7 @@ ## Purpose -Lets a budget owner (or funder viewer) generate a 3-sheet Excel workbook of a single budget's plan, real-currency ledger, and expense history, for sharing outside the app. +Lets a budget owner (or funder viewer) generate an Excel workbook of a single budget's plan, real-currency ledger, and expense history, for sharing outside the app — using either the built-in default format or an export template their organisation owns or their funder has shared with them. ## ADDED Requirements @@ -22,16 +22,24 @@ The system SHALL provide an endpoint that generates and streams a `.xlsx` workbo - **THEN** the system rejects the request without generating a workbook ### Requirement: Sheet 1 — Original Budget -The workbook's first sheet SHALL list every budget line grouped by category, with a subtotal row per category and a grand-total row, each line showing its amount in the budget's `local_currency` and a derived estimate in the budget's `actual_currency` (`amount ÷ estimated_exchange_rate`) when `estimated_exchange_rate` is set. +The workbook's first sheet SHALL open with a header block (organisation name, donor name, project name, project period, estimated currency and rate), followed by a Budget Summary section (one row per category totalling that category's lines, plus a grand-total row), followed by a Detailed Budget section listing every budget line grouped under its category with a subtotal row per category, and a footer (a repeated total-expenditures row, an authorised-signatory line, and a project-contact-person line). Every amount is shown in the budget's `local_currency`; the donor-currency estimate (`amount ÷ estimated_exchange_rate`) is a live Excel formula referencing the header's rate cell, computed when `estimated_exchange_rate` is set. #### Scenario: Categories and subtotals shown - **WHEN** the budget has lines across more than one category -- **THEN** Sheet 1 groups lines under their category, with a subtotal per category and a grand total for the whole budget +- **THEN** Sheet 1's Budget Summary and Detailed Budget sections both group lines under their category, each with a subtotal per category and a grand total for the whole budget + +#### Scenario: Category with no lines is still shown +- **WHEN** a budget category has zero budget lines +- **THEN** the category still appears in both the Budget Summary and Detailed Budget sections with a zero subtotal, rather than being silently omitted #### Scenario: No donor-currency estimate available - **WHEN** the budget has no `estimated_exchange_rate` set - **THEN** Sheet 1 shows the local-currency amount only, leaving the donor-currency estimate column blank for that budget rather than showing a fabricated figure +#### Scenario: Donor-currency estimate recalculates with the rate +- **WHEN** the budget has an `estimated_exchange_rate` set +- **THEN** every figure above a raw line amount is a live formula (a line's estimate divides its local amount by the header's rate cell; a category's subtotal and subtotal-estimate are `SUM` formulas over its lines; Budget Summary's category rows reference that category's own subtotal cell; every TOTAL/Total-expenditures row sums those reference cells), so editing the rate cell or any line amount in the exported file recalculates every subtotal, total, and estimate + ### Requirement: Sheet 2 — Budget vs. Report Dashboard, income section The workbook's second sheet SHALL open with an income section listing every recorded currency conversion for the budget as its own row (converted date, donor-currency amount, local-currency amount, implied rate), plus a total row, rather than one row per funding receipt. @@ -68,3 +76,93 @@ The workbook's third sheet SHALL list every report-line expense across all of th #### Scenario: Expense not yet allocated - **WHEN** a report-line expense has no currency-conversion allocation at all - **THEN** Sheet 3 shows it as a single row with its local-currency amount and no conversion date or rate + +### Requirement: Available export templates for a budget +The system SHALL provide an endpoint listing the export templates a given viewer may use for a given budget: the system default, every template owned by the viewer's organisation, and every template owned by the budget's funder that is marked shared with grantees. Each entry SHALL identify whether it is the system default, the viewer's own, or a funder's. + +#### Scenario: Grantee sees a funder's shared template +- **WHEN** a grantee owner requests the available templates for a budget funded by a donor that owns a template marked shared with grantees +- **THEN** the response includes the system default, the grantee's own templates, and that donor's shared template, each tagged with its source + +#### Scenario: A donor's private template is not offered +- **WHEN** the budget's funder owns a template marked private +- **THEN** that template does not appear in the grantee's available templates + +#### Scenario: An unrelated organisation's shared template is not offered +- **WHEN** an organisation that does not fund this budget owns a template marked shared with grantees +- **THEN** that template does not appear in the available templates for this budget + +#### Scenario: Funding relationship is revoked +- **WHEN** the donor-grantee relationship that made a donor's template available is revoked +- **THEN** the very next request for available templates omits that template, without waiting for any cache to expire + +### Requirement: Explicit template selection +The export endpoint SHALL accept an optional template identifier and SHALL NOT select a template implicitly when more than one is available. When no identifier is given and exactly one template is available, the system SHALL use it. When no identifier is given and more than one is available, the system SHALL reject the request and indicate that a template must be chosen. When an identifier is given that is not among the viewer's available templates for that budget, the system SHALL reject the request as forbidden. + +#### Scenario: Only the default is available +- **WHEN** a viewer with no organisation templates and no funder-shared templates exports a budget without naming a template +- **THEN** the system generates the workbook using the system default + +#### Scenario: Choice required +- **WHEN** a viewer with more than one available template exports a budget without naming one +- **THEN** the request is rejected with an error stating that a template must be chosen, and no workbook is generated + +#### Scenario: Template the viewer may not use +- **WHEN** a viewer names a template that is private to another organisation, or shared by an organisation that does not fund this budget +- **THEN** the request is rejected as forbidden + +#### Scenario: Previously chosen template is no longer available +- **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. + +#### Scenario: Sheet subset +- **WHEN** a budget is exported with a template that includes only Sheet 1 +- **THEN** the generated workbook contains only the Original Budget sheet + +#### Scenario: Column labels overridden +- **WHEN** a template overrides the description column's header label +- **THEN** the generated workbook shows the overridden label, and every figure and formula is otherwise unchanged from the default output + +#### Scenario: Unknown option rejected +- **WHEN** a template is saved with an option key the schema does not define +- **THEN** the save is rejected with a validation error + +#### Scenario: Template predating a newly added option +- **WHEN** a template stored before an option existed is used for an export +- **THEN** the export succeeds, applying the system default's value for the missing option + +### Requirement: System default template +The system SHALL provide a built-in default template, available to every organisation, that produces the same workbook this capability's other requirements describe. It SHALL be generated through the same rendering path as any other template, and SHALL NOT be editable or deletable by any organisation. + +#### Scenario: Default reproduces the built-in format +- **WHEN** a budget is exported with the system default template +- **THEN** the workbook is identical to what the export produces with no template concept at all + +#### Scenario: Default cannot be modified +- **WHEN** an organisation attempts to edit or delete the system default template +- **THEN** the request is rejected + +### Requirement: Generated workbook records its template +The generated workbook SHALL identify, in its audit footer alongside the exporting user and timestamp, the name and version of the template that produced it. Editing a template SHALL advance its version, so a previously exported file remains attributable to the template content that produced it. + +#### Scenario: Footer names the template +- **WHEN** a budget is exported with any template +- **THEN** the workbook's audit footer shows that template's name and version together with the exporting user and export timestamp + +#### Scenario: Template edited after an export +- **WHEN** a template is edited after a workbook was exported with it +- **THEN** the template's version advances, and the already-exported workbook still shows the earlier version diff --git a/openspec/changes/budget-feat-313-excel-export/tasks.md b/openspec/changes/budget-feat-313-excel-export/tasks.md index ac21ca72..7e1d666a 100644 --- a/openspec/changes/budget-feat-313-excel-export/tasks.md +++ b/openspec/changes/budget-feat-313-excel-export/tasks.md @@ -4,11 +4,11 @@ Workflow rule: one task group = one GitHub sub-issue (of this change's parent is ## 1. Export endpoint scaffolding + Sheet 1 (Original Budget) — Issue #316 -- [ ] 1.0 Run `scripts/start-group.sh budget-excel-export 1` to create/link this group's sub-issue and branch before starting any other work in this group. -- [ ] 1.1 Add `services/budget/app/services/excel_export_service.py` with a `generate_budget_export_workbook(budget, categories, lines, ...)` entry point that creates an `openpyxl.Workbook()` and returns bytes; verify with a unit test that it returns a valid `.xlsx` (round-trips through `openpyxl.load_workbook`) -- [ ] 1.2 Implement Sheet 1 generation: budget lines grouped by category (reuse `BudgetCategoryModel` ordering), subtotal row per category, grand-total row, local-currency amount column, and a donor-currency estimate column (`amount ÷ estimated_exchange_rate`) left blank when `estimated_exchange_rate` is unset; verify with a unit test asserting cell values for a budget with 2 categories and a budget with no `estimated_exchange_rate` -- [ ] 1.3 Add `GET /budgets/{budget_id}/export.xlsx` to `services/budget/app/api/budget_routes.py`, authorized the same way as `GET /budgets/{budget_id}` (owner or funder), returning a `StreamingResponse` (mirroring `attachment_routes.py`'s pattern) with the correct `Content-Type`/`Content-Disposition`; verify with an integration test that owner and funder both get 200 and a non-owner/non-funder gets rejected -- [ ] 1.4 Wire the new route into `nginx-dev.conf`, `nginx.conf`, and `Caddyfile`; verify by confirming the route pattern matches the existing `/budgets/{budget_id}/...` entries in all three files +- [x] 1.0 Run `scripts/start-group.sh budget-excel-export 1` to create/link this group's sub-issue and branch before starting any other work in this group. +- [x] 1.1 Add `services/budget/app/services/excel_export_service.py` with a `generate_budget_export_workbook(budget, categories, lines, ...)` entry point that creates an `openpyxl.Workbook()` and returns bytes; verify with a unit test that it returns a valid `.xlsx` (round-trips through `openpyxl.load_workbook`) +- [x] 1.2 Implement Sheet 1 generation: header block (org/donor name via `customer_client`, project name/period, estimated currency/rate), a Budget Summary section (its title row doubles as the column-header row; one row per category referencing its Detailed Budget subtotal cell, plus a `SUM`-formula TOTAL row), a Detailed Budget section (category header row, lines, then a `SUM`-formula "Subtotal" row — categories with zero lines still shown, at 0), and a footer (a "Total expenditures" row summing each category's subtotal cell, signature/contact-person lines); per-line local amount is the only literal value, every other figure is a formula, blank when `estimated_exchange_rate` is unset; column widths/number formats matched to a user-supplied reference file; verify with unit tests covering 2 categories, no `estimated_exchange_rate`, and an empty category, plus a LibreOffice headless round-trip confirming the formulas actually compute (not just parse) +- [x] 1.3 Add `GET /budgets/{budget_id}/export.xlsx` to `services/budget/app/api/budget_routes.py`, authorized the same way as `GET /budgets/{budget_id}` (owner or funder), returning a `StreamingResponse` (mirroring `attachment_routes.py`'s pattern) with the correct `Content-Type`/`Content-Disposition`; verify with an integration test that owner and funder both get 200 and a non-owner/non-funder gets rejected +- [x] 1.4 Wire the new route into `nginx-dev.conf`, `nginx.conf`, and `Caddyfile`; verify by confirming the route pattern matches the existing `/budgets/{budget_id}/...` entries in all three files (no changes needed — all three already proxy the whole `/api/v1/budgets/` prefix as a catch-all block, so `export.xlsx` is already covered) - [ ] 1.5 Run backend lint/tests clean for `services/budget`; PR merged (`Closes` this group's sub-issue) ## 2. Sheet 2 — Budget vs. Report Dashboard — depends on 1 @@ -27,7 +27,33 @@ Workflow rule: one task group = one GitHub sub-issue (of this change's parent is ## 4. Frontend export button — depends on 1, 2, 3 -- [ ] 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 +- [ ] 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 - [ ] 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) + +## 5. Export templates: model, ownership, and candidate resolution — depends on 1 + +> 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 +- [ ] 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 + +- [ ] 6.1 Thread an optional `template_id` through `GET /budgets/{budget_id}/export.xlsx`: resolve against `list_candidate_templates`, use the single candidate when none is given, reject with 400 when none is given and more than one exists, reject with 403 when the given id is not a candidate, and reject with a distinct "no longer available" error when a previously valid id has been deleted or un-shared; verify with integration tests covering each of those four outcomes +- [ ] 6.2 Make `generate_budget_export_workbook` take the resolved template's options and apply them — sheet subset, donor-currency estimate column visibility, column header label overrides, audit footer visibility — so the system default runs the same path as any other template; verify that group 1's and groups 2–3's existing tests pass unchanged when the system default is used, plus new tests asserting a Sheet-1-only template yields one sheet and a label override changes only the header text while every figure and formula is unchanged +- [ ] 6.3 Extend `_audit_line()` to include the template's name and version alongside the exporting user and timestamp; verify with a unit test asserting the footer text for a named template and for the system default +- [ ] 6.4 Run backend lint/tests clean for `services/budget`; PR merged (`Closes` this group's sub-issue) + +## 7. Template picker and management UI — depends on 4, 5, 6 + +- [ ] 7.1 Replace group 4's plain button with a template picker: fetch `GET /budgets/{budget_id}/export-templates` on the budget detail view, export directly when only one candidate exists, and present the options labelled by source (default / own / funder) when more than one does; verify with frontend tests covering the single-candidate path (no picker shown) and the multi-candidate path (picker shown, request sent only after a choice) +- [ ] 7.2 Handle the "chosen template is no longer available" rejection by showing the reason inline and reopening the picker with freshly fetched options, rather than a generic failure; verify with a frontend test simulating that rejection +- [ ] 7.3 Add the template management view: list the organisation's own templates plus the read-only built-in default, with create/rename/edit/delete and a visibility control (private / shared with grantees), following existing form and permission conventions; mobile viewports get a card layout rather than a table, per the go-forward convention; verify with frontend tests asserting the default renders without edit/delete controls and that a duplicate-name error is shown inline without clearing the form +- [ ] 7.4 Run frontend lint/tests clean; manually verify end-to-end that a donor can share a template, a funded grantee sees it in the picker on that donor's budget, an unrelated NGO does not, and revoking the relationship removes it; PR merged (`Closes` this group's sub-issue) diff --git a/openspec/changes/budget-feat-customer-profile-cache/.openspec.yaml b/openspec/changes/budget-feat-customer-profile-cache/.openspec.yaml new file mode 100644 index 00000000..fa97a031 --- /dev/null +++ b/openspec/changes/budget-feat-customer-profile-cache/.openspec.yaml @@ -0,0 +1,4 @@ +schema: spec-driven +created: 2026-09-22 +author: Norair Arutshyan +priority: low diff --git a/openspec/changes/budget-feat-customer-profile-cache/proposal.md b/openspec/changes/budget-feat-customer-profile-cache/proposal.md new file mode 100644 index 00000000..76c40d27 --- /dev/null +++ b/openspec/changes/budget-feat-customer-profile-cache/proposal.md @@ -0,0 +1,25 @@ +# Proposal + +## Why + +Budget service resolves customer/organisation details (name, `is_donor`, `is_ngo`) via `customer_client.get_customer_cached()`, a synchronous HTTP call to the users service wrapped only in a process-local, unbounded `lru_cache` — no persistence, no fallback, cold on every restart. This is weaker than the equivalent user-lookup path (`get_users_by_ids_cached`), which is backed by a real local table (`user_profiles`) populated via event consumption with HTTP fallback-and-backfill. Not currently a bottleneck; filed low-priority to fix the asymmetry before it becomes one (e.g. under load, or as more call sites depend on customer lookups, such as `budget-feat-313-excel-export`). + +## What Changes + +- Add a `customer_profiles` table in budget service (mirroring `user_profiles`' shape: `customer_id` PK, `name`, `is_donor`, `is_ngo`, `cached_at`). +- Change `get_customer_cached` (and callers in `customer_client.py`, `budget_services.py`, `excel_export_service.py`) to read this table first, falling back to HTTP on miss and writing the result back — same write-through-on-miss pattern already used by `get_users_by_ids_cached`, no new event type or publisher change required. +- No proactive invalidation for now (accepting staleness on rare customer-detail changes, consistent with current behavior); revisit only if this becomes a real problem. + +## Capabilities + +### New Capabilities +- `budget-customer-cache`: local read-through cache for customer/organisation details (name, `is_donor`, `is_ngo`) used by budget service, replacing the unbounded in-memory `lru_cache`. + +### Modified Capabilities +(none — no existing spec currently documents `get_customer_cached`'s caching behavior) + +## Impact + +- Affected code: `services/budget/app/services/customer_client.py`, new `services/budget/app/models/customer_cache.py`, new Alembic migration, callers in `budget_services.py` and `excel_export_service.py`. +- No changes to users service, no new events, no new consumers. +- Low priority / not scoped in detail — revisit if the current in-memory cache becomes a measurable bottleneck. diff --git a/openspec/changes/budget-feat-customer-profile-cache/specs/budget-customer-cache/spec.md b/openspec/changes/budget-feat-customer-profile-cache/specs/budget-customer-cache/spec.md new file mode 100644 index 00000000..b423233b --- /dev/null +++ b/openspec/changes/budget-feat-customer-profile-cache/specs/budget-customer-cache/spec.md @@ -0,0 +1,32 @@ +# Spec Delta + +## Purpose + +Provides budget service with a locally persisted, read-through cache of customer/organisation details (name, `is_donor`, `is_ngo`), so lookups by `customer_id` survive process restarts instead of depending on an in-memory-only cache. + +## ADDED Requirements + +### Requirement: Customer details are read-through cached locally +The system SHALL resolve customer details (name, `is_donor`, `is_ngo`) for a given `customer_id` by first checking a local store, and only calling the users service over HTTP on a cache miss. + +#### Scenario: Cache hit +- **WHEN** customer details for a given `customer_id` already exist in the local store +- **THEN** the system returns them without making an HTTP call to the users service + +#### Scenario: Cache miss +- **WHEN** customer details for a given `customer_id` do not exist in the local store +- **THEN** the system fetches them via HTTP from the users service, persists them locally, and returns them + +### Requirement: Cached customer details persist across restarts +The system SHALL persist cached customer details in a local table (not only in-process memory), so a service restart does not discard previously resolved entries. + +#### Scenario: Lookup after restart +- **WHEN** a customer's details were cached before a service restart +- **THEN** a subsequent lookup for that `customer_id` after restart is served from the local store without an HTTP call + +### Requirement: Staleness is accepted, not actively invalidated +The system SHALL NOT proactively invalidate or refresh cached customer details when the source customer record changes. Staleness is an accepted trade-off given how infrequently these fields change; active invalidation is out of scope unless this becomes a demonstrated problem. + +#### Scenario: Source customer record changes after caching +- **WHEN** a customer's name or `is_donor`/`is_ngo` flags change in the users service after being cached in budget service +- **THEN** budget service continues returning the previously cached values until that entry is naturally refreshed (e.g. via a future cache-miss path), with no dedicated invalidation mechanism required diff --git a/openspec/changes/budget-feat-customer-profile-cache/tasks.md b/openspec/changes/budget-feat-customer-profile-cache/tasks.md new file mode 100644 index 00000000..46c97385 --- /dev/null +++ b/openspec/changes/budget-feat-customer-profile-cache/tasks.md @@ -0,0 +1,12 @@ +# 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. Local read-through customer cache + +- [ ] 1.0 Run `scripts/start-group.sh budget-feat-customer-profile-cache 1` to create/link this group's sub-issue and branch before starting any other work in this group. +- [ ] 1.1 Add `CustomerProfileModel` (`customer_profiles` table: `customer_id` PK, `name`, `is_donor`, `is_ngo`, `cached_at`) in `services/budget/app/models/`, mirroring `UserProfileModel`'s shape, and generate the Alembic migration. +- [ ] 1.2 Change `get_customer_cached` in `services/budget/app/services/customer_client.py` to a DB-first, HTTP-fallback-and-backfill lookup mirroring `get_users_by_ids_cached` in `services/budget/app/services/user_cache.py`; drop the `lru_cache` decorator. +- [ ] 1.3 Verify existing callers (`budget_services.py`'s `local_currency` lookup, `excel_export_service.py`'s organisation/donor name lookups) work unchanged against the new function signature. +- [ ] 1.4 Add/update tests covering cache-hit, cache-miss-with-backfill, and persistence-across-a-fresh-DB-session (real test DB session, not a mock, per this repo's async-SQLAlchemy test convention) in `services/budget/tests/`. +- [ ] 1.5 Run budget service's tests and lint clean; PR merged. diff --git a/services/budget/app/api/budget_routes.py b/services/budget/app/api/budget_routes.py index e7613b30..7d412aab 100644 --- a/services/budget/app/api/budget_routes.py +++ b/services/budget/app/api/budget_routes.py @@ -1,5 +1,8 @@ # /services/budget/app/api/budget_routes.py +import io + from fastapi import APIRouter, Depends, File, HTTPException, UploadFile, status +from fastapi.responses import StreamingResponse from sqlalchemy.ext.asyncio import AsyncSession from uuid import uuid4, UUID # noqa: F401 @@ -33,10 +36,12 @@ get_grantee_dashboard_summary_service, ) 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.crud.budget_crud import get_budgets_by_creator from shared.observability import set_span_attributes from shared.security.dependencies import get_validated_user +from shared.storage.storage_service import safe_content_disposition router = APIRouter(prefix="/budgets", tags=["Public Budgets"]) private_router = APIRouter(prefix="/budgets", tags=["Private Budgets"]) @@ -104,6 +109,21 @@ async def get_budget_endpoint( return budget +@router.get("/{budget_id}/export.xlsx") +async def export_budget_workbook_endpoint( + budget_id: UUID, + db: AsyncSession = Depends(get_db), + valid_user=Depends(get_validated_user), +): + set_span_attributes(budget_id=budget_id) + budget, workbook_bytes = await export_budget_workbook_service(db, valid_user, budget_id) + return StreamingResponse( + io.BytesIO(workbook_bytes), + media_type="application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + headers={"Content-Disposition": safe_content_disposition(f"{budget.name}.xlsx")}, + ) + + @router.patch("/{budget_id}", response_model=BudgetUpdate) async def update_budget_endpoint( budget_id: UUID, @@ -188,11 +208,7 @@ async def get_budgets_by_creator_endpoint( db: AsyncSession = Depends(get_db), valid_user=Depends(get_validated_user), ): - # Called by the users service to build a data-subject data-export — the - # users service forwards the requesting user's own token, so this is - # self-service only, same as delete_my_account. Unlike /customers/by_ids/, - # this sits on the public router with no gateway-level path exclusion, so - # it must enforce this itself rather than trust the "internal" convention. + # Unlike /customers/by_ids/, no gateway exclusion protects this path — this is the only guard. if str(valid_user["user_id"]) != str(user_id): raise HTTPException(status_code=403, detail="Not authorized to view this user's budgets") budgets = await get_budgets_by_creator(db, user_id) diff --git a/services/budget/app/crud/budget_category_crud.py b/services/budget/app/crud/budget_category_crud.py index 35535b2f..51f610f5 100644 --- a/services/budget/app/crud/budget_category_crud.py +++ b/services/budget/app/crud/budget_category_crud.py @@ -87,12 +87,15 @@ async def get_budget_category( async def list_budget_categories( - session: AsyncSession, budget_id: UUID | None = None, limit: int = 100 + session: AsyncSession, budget_id: UUID | None = None, limit: int | None = 100 ): - query = select(BudgetCategoryModel) + query = select(BudgetCategoryModel).order_by( + BudgetCategoryModel.created_at, BudgetCategoryModel.id + ) if budget_id: query = query.where(BudgetCategoryModel.budget_id == budget_id) - result = await session.execute(query.limit(limit)) + query = query.limit(limit) + result = await session.execute(query) return list(result.scalars().all()) diff --git a/services/budget/app/crud/budget_line_crud.py b/services/budget/app/crud/budget_line_crud.py index be23c014..19dfbec8 100644 --- a/services/budget/app/crud/budget_line_crud.py +++ b/services/budget/app/crud/budget_line_crud.py @@ -91,14 +91,19 @@ async def list_budget_lines( session: AsyncSession, budget_id: UUID | None = None, customer_id: UUID | None = None, - limit: int = 100, + limit: int | None = 100, ): - query = select(BudgetLineModel).options(selectinload(BudgetLineModel.category)) + query = ( + select(BudgetLineModel) + .options(selectinload(BudgetLineModel.category)) + .order_by(BudgetLineModel.created_at, BudgetLineModel.id) + ) if budget_id: query = query.where(BudgetLineModel.budget_id == budget_id) if customer_id: query = query.join(BudgetLineModel.budget).where(BudgetModel.owner_id == customer_id) - result = await session.execute(query.limit(limit)) + query = query.limit(limit) + result = await session.execute(query) return list(result.scalars().all()) diff --git a/services/budget/app/services/budget_services.py b/services/budget/app/services/budget_services.py index ae036bcc..11a112ae 100644 --- a/services/budget/app/services/budget_services.py +++ b/services/budget/app/services/budget_services.py @@ -1,6 +1,6 @@ import asyncio import structlog -from datetime import datetime, timezone +from datetime import date, datetime, timezone from dateutil.relativedelta import relativedelta from fastapi import status, HTTPException from shared.observability import set_span_attributes @@ -66,7 +66,7 @@ async def create_budget_service( ): if budget.funding_customer_id: - validate_customer_can_fund(budget.funding_customer_id, raise_domain_error=True) + await validate_customer_can_fund(budget.funding_customer_id, raise_domain_error=True) owner_id = valid_user.get("customer_id") @@ -79,7 +79,7 @@ async def create_budget_service( # A bare superuser (no impersonation session) must not be able to # create a budget under an arbitrary customer_id — mirrors the same # check in update_budget_service. - validate_customer_can_own(budget.owner_id, raise_domain_error=True) + await validate_customer_can_own(budget.owner_id, raise_domain_error=True) owner_id = budget.owner_id @@ -230,7 +230,7 @@ async def _resolve_updatable_budget( async def update_budget_service(budget_id: UUID, budget: BudgetCreate, valid_user: dict, db): if budget.funding_customer_id: - validate_customer_can_fund(budget.funding_customer_id, raise_domain_error=True) + await validate_customer_can_fund(budget.funding_customer_id, raise_domain_error=True) # Broader than "a bare confirm with no other fields" — this also covers a # confirm bundled with a metadata edit, so both the archived/already- @@ -603,7 +603,7 @@ async def create_budget_with_lines_service( local_currency = request.local_currency if not local_currency: # Fall back to the org's default currency (Decision 8); errors propagate, no silent GBP. - local_currency = get_customer_cached(owner_id).get("currency") + local_currency = (await get_customer_cached(owner_id)).get("currency") # commit=False throughout: budget + categories + lines + total are flushed only, # one db.commit() below makes the whole operation atomic (design.md Decision 5). @@ -685,13 +685,18 @@ async def create_budget_with_lines_service( ) from e +def _add_duration_months(start_date: date, duration_months: int | None) -> date: + """Shared with excel_export_service._period_label — keep both in sync.""" + return start_date + relativedelta(months=duration_months or 0) + + def _compute_end_date(budget: BudgetModel): """Mirrors report_services.create_report_service's default period_end — the single source of truth for this formula, so the frontend displays end_date from here rather than reimplementing the math.""" if not budget.start_date: return None - return budget.start_date + relativedelta(months=budget.duration_months or 0) + return _add_duration_months(budget.start_date, budget.duration_months) def _compute_estimated_local_cap(budget: BudgetModel) -> float | None: diff --git a/services/budget/app/services/customer_client.py b/services/budget/app/services/customer_client.py index 7db64fe7..ff1200ea 100644 --- a/services/budget/app/services/customer_client.py +++ b/services/budget/app/services/customer_client.py @@ -1,26 +1,45 @@ -import requests +import uuid +from collections import OrderedDict + +import httpx from app.core.config import settings from fastapi import status -from functools import lru_cache -import uuid from app.core.exceptions import DomainError CUSTOMER_SERVICE_URL = settings.customer_service_url +_client: httpx.AsyncClient = httpx.AsyncClient(base_url=CUSTOMER_SERVICE_URL) + +_CACHE_MAXSIZE = 128 +_customer_cache: "OrderedDict[str, dict]" = OrderedDict() class CustomerServiceError(Exception): pass -def get_customer(customer_id: str | uuid.UUID) -> dict: - """Uses the no-auth by_ids/ internal endpoint (not GET /customers/{id}, - which now requires a JWT) since this is a service-to-service call with - no user token to forward.""" +async def init_urls(): + global CUSTOMER_SERVICE_URL, _client + + CUSTOMER_SERVICE_URL = settings.customer_service_url + _client = httpx.AsyncClient(base_url=CUSTOMER_SERVICE_URL) + print(f"✅ Customer client initialized: {CUSTOMER_SERVICE_URL}") + + +async def close_urls(): + """Gracefully close HTTP client session.""" + global _client # noqa: F824 + if _client: + await _client.aclose() + print("🛑 Customer client closed") + + +async def get_customer(customer_id: str | uuid.UUID) -> dict: + """No-auth by_ids/ endpoint: this is a service-to-service call, no user token to forward.""" try: - resp = requests.post(f"{CUSTOMER_SERVICE_URL}by_ids/", json=[str(customer_id)]) + resp = await _client.post(f"{CUSTOMER_SERVICE_URL}by_ids/", json=[str(customer_id)]) resp.raise_for_status() items = resp.json() - except requests.RequestException as e: + except httpx.HTTPError as e: raise CustomerServiceError(f"Failed to fetch customer {customer_id}") from e if not items: @@ -28,16 +47,27 @@ def get_customer(customer_id: str | uuid.UUID) -> dict: return items[0] -@lru_cache(maxsize=128) -def get_customer_cached(customer_id: str | uuid.UUID) -> dict: - return get_customer(customer_id) +async def get_customer_cached(customer_id: str | uuid.UUID) -> dict: + """Same maxsize=128, no-TTL semantics as the old @lru_cache, async-compatible.""" + key = str(customer_id) + if key in _customer_cache: + _customer_cache.move_to_end(key) + return _customer_cache[key] + + customer = await get_customer(customer_id) + _customer_cache[key] = customer + if len(_customer_cache) > _CACHE_MAXSIZE: + _customer_cache.popitem(last=False) + return customer -def validate_customer_can_fund(customer_id: str | uuid.UUID, raise_domain_error: bool = False): +async def validate_customer_can_fund( + customer_id: str | uuid.UUID, raise_domain_error: bool = False +): """Assert the customer has is_donor=True (can issue grants).""" Error = DomainError if raise_domain_error else ValueError try: - customer = get_customer_cached(customer_id) + customer = await get_customer_cached(customer_id) except CustomerServiceError as e: raise Error(str(e)) @@ -47,21 +77,18 @@ def validate_customer_can_fund(customer_id: str | uuid.UUID, raise_domain_error: def require_donor(valid_user: dict) -> None: - """Assert the authenticated user's customer has is_donor=True. - - Reads the flag directly off the decoded JWT payload (get_validated_user's - output) rather than calling get_customer_cached — that cache is unbounded - with no TTL, and is_donor now travels in the token claims (ticket #135). - """ + """Reads is_donor off the JWT claims rather than get_customer_cached (ticket #135).""" if not valid_user.get("is_donor"): raise DomainError("Customer is not a donor", status.HTTP_403_FORBIDDEN) -def validate_customer_can_own(customer_id: str | uuid.UUID, raise_domain_error: bool = False): +async def validate_customer_can_own( + customer_id: str | uuid.UUID, raise_domain_error: bool = False +): """Assert the customer has is_ngo=True (can receive grants / own budgets).""" Error = DomainError if raise_domain_error else ValueError try: - customer = get_customer_cached(customer_id) + customer = await get_customer_cached(customer_id) except CustomerServiceError as e: raise Error(str(e)) diff --git a/services/budget/app/services/excel_export_service.py b/services/budget/app/services/excel_export_service.py new file mode 100644 index 00000000..6b17cdd1 --- /dev/null +++ b/services/budget/app/services/excel_export_service.py @@ -0,0 +1,410 @@ +import io +from datetime import date, datetime, timezone +from uuid import UUID + +from openpyxl import Workbook +from openpyxl.styles import Border, Font, PatternFill, Side +from openpyxl.utils import get_column_letter + +from app.crud.budget_category_crud import list_budget_categories +from app.crud.budget_line_crud import list_budget_lines +from app.models.budget import BudgetCategoryModel, BudgetLineModel, BudgetModel +from app.services.budget_services import _add_duration_months, get_viewable_budget_service +from app.services.customer_client import CustomerServiceError, get_customer_cached +from app.services.user_cache import get_users_by_ids_cached + +SHEET1_TITLE = "Original Budget" +_BOLD = Font(bold=True) +_AUDIT_FONT = Font(italic=True, size=9, color="808080") +_TOTAL_FILL = PatternFill(start_color="D9D9D9", end_color="D9D9D9", fill_type="solid") +_TOP_BORDER = Border(top=Side(style="thin")) +_DESCRIPTION_COL_WIDTH = 26.63 +_EXTRA_COL_WIDTH = 20.0 +_AMOUNT_COL_WIDTH = 17.77 +_ESTIMATE_COL_WIDTH = 17.53 +_HEADER_ROW_COUNT = 6 +_RATE_ROW = _HEADER_ROW_COUNT +_DESCRIPTION_COL = 1 + + +async def export_budget_workbook_service( + db, valid_user: dict, budget_id: UUID +) -> tuple[BudgetModel, bytes]: + """Loads one budget's categories/lines (auth: owner or funder, same as + GET /budgets/{budget_id}) and builds its export workbook.""" + budget = await get_viewable_budget_service(budget_id, valid_user, db) + categories = await list_budget_categories(db, budget_id=budget_id, limit=None) + lines = await list_budget_lines(db, budget_id=budget_id, limit=None) + organisation_name = None + try: + organisation_name = (await get_customer_cached(budget.owner_id)).get("name") + except CustomerServiceError: + pass + + donor_name = budget.external_funder_name + if budget.funding_customer_id: + try: + donor_name = (await get_customer_cached(budget.funding_customer_id)).get("name") + except CustomerServiceError: + donor_name = budget.external_funder_name + + try: + users = await get_users_by_ids_cached( + [str(valid_user["user_id"])], valid_user.get("token", "") + ) + except Exception: + users = {} + exported_by = users.get(str(valid_user["user_id"]), {}).get("email") + return budget, generate_budget_export_workbook( + budget, categories, lines, + organisation_name=organisation_name, donor_name=donor_name, + exported_by=exported_by, exported_at=datetime.now(timezone.utc), + ) + + +def generate_budget_export_workbook( + budget: BudgetModel, + categories: list[BudgetCategoryModel], + lines: list[BudgetLineModel], + organisation_name: str | None = None, + donor_name: str | None = None, + exported_by: str | None = None, + exported_at: datetime | None = None, +) -> bytes: + """Builds the export workbook for one budget. Group 1 populates only + Sheet 1 (Original Budget); Sheets 2/3 land in later task groups.""" + wb = Workbook() + ws = wb.active + ws.title = SHEET1_TITLE + _write_sheet1( + ws, budget, categories, lines, organisation_name, donor_name, exported_by, exported_at + ) + + buf = io.BytesIO() + wb.save(buf) + return buf.getvalue() + + +def _period_label(start_date: date | None, duration_months: int | None) -> str | None: + if not start_date: + return None + if not duration_months: # None (unset) and 0 (BudgetModel's column default) both mean "unknown" + return start_date.strftime("%m/%Y") + end_date = _add_duration_months(start_date, duration_months - 1) + return f"{start_date.strftime('%m/%Y')}-{end_date.strftime('%m/%Y')}" + + +def _currency_format(currency: str | None) -> str: + return f'#,##0.00" {currency}"' if currency else "#,##0.00" + + +def _bold_row(ws, row: int) -> None: + for cell in ws[row]: + cell.font = _BOLD + + +def _apply_box_border(ws, start_row: int, end_row: int, end_col: int) -> None: + thin = Side(style="thin") + for row in range(start_row, end_row + 1): + for column in range(1, end_col + 1): + ws.cell(row=row, column=column).border = Border( + top=thin if row == start_row else None, + bottom=thin if row == end_row else None, + left=thin if column == 1 else None, + right=thin if column == end_col else None, + ) + + +def _extra_field_keys(lines: list[BudgetLineModel]) -> list[str]: + """Distinct extra_fields keys across the budget, in first-seen order.""" + keys: dict = {} + for line in lines: + for key in line.extra_fields or {}: + keys[key] = None + return list(keys) + + +def _amount_columns(extra_keys: list[str]) -> dict: + """Amount/Estimate shift right by one column per distinct extra field key.""" + amount_col = _DESCRIPTION_COL + 1 + len(extra_keys) + estimate_col = amount_col + 1 + return { + "amount_col": amount_col, + "estimate_col": estimate_col, + "amount_letter": get_column_letter(amount_col), + "estimate_letter": get_column_letter(estimate_col), + } + + +def _plan_rows(ordered_category_ids: list, lines_by_category_id: dict) -> dict: + """Computes every row number up front — Budget Summary formulas reference + Detailed Budget's subtotal rows before those rows are written.""" + summary_header_row = _RATE_ROW + 3 + summary_rows = {} + row = summary_header_row + for category_id in ordered_category_ids: + row += 1 + summary_rows[category_id] = row + summary_total_row = row + 1 if ordered_category_ids else summary_header_row + 1 + + detail_title_row = summary_total_row + 2 + detail_header_rows: dict = {} + detail_line_rows: dict = {} + detail_subtotal_rows: dict = {} + row = detail_title_row + for category_id in ordered_category_ids: + row += 1 + detail_header_rows[category_id] = row + line_rows = [] + for _ in lines_by_category_id.get(category_id, []): + row += 1 + line_rows.append(row) + detail_line_rows[category_id] = line_rows + row += 1 + detail_subtotal_rows[category_id] = row + row += 1 # blank separator after the category block + + footer_total_row = row + 2 if ordered_category_ids else detail_title_row + 2 + signature_line_row = footer_total_row + 3 + contact_line_row = signature_line_row + 4 + audit_row = contact_line_row + 3 + + return { + "summary_header_row": summary_header_row, + "summary_rows": summary_rows, + "summary_total_row": summary_total_row, + "detail_title_row": detail_title_row, + "detail_header_rows": detail_header_rows, + "detail_line_rows": detail_line_rows, + "detail_subtotal_rows": detail_subtotal_rows, + "footer_total_row": footer_total_row, + "signature_line_row": signature_line_row, + "contact_line_row": contact_line_row, + "audit_row": audit_row, + } + + +def _write_sheet1( + ws, + budget: BudgetModel, + categories: list[BudgetCategoryModel], + lines: list[BudgetLineModel], + organisation_name: str | None, + donor_name: str | None, + exported_by: str | None, + exported_at: datetime | None, +) -> None: + has_rate = bool(budget.estimated_exchange_rate) + rate_cell = _write_header(ws, budget, organisation_name, donor_name) + + local_fmt = _currency_format(budget.local_currency) + estimate_fmt = _currency_format(budget.actual_currency) + amount_header = f"Amount ({budget.local_currency})" if budget.local_currency else "Amount" + estimate_header = ( + f"Estimate ({budget.actual_currency})" if budget.actual_currency else "Donor Estimate" + ) + + categories_by_id = {category.id: category for category in categories} + lines_by_category_id: dict = {} + for line in lines: + category = categories_by_id.get(line.category_id) + lines_by_category_id.setdefault(category.id if category else None, []).append(line) + + # categories/lines already arrive ordered by created_at, id from the CRUD queries. + ordered_category_ids = list(categories_by_id) + if None in lines_by_category_id: + ordered_category_ids.append(None) + + category_names = { + cid: (categories_by_id[cid].name if cid else "Uncategorized") + for cid in ordered_category_ids + } + category_lines = { + cid: lines_by_category_id.get(cid, []) for cid in ordered_category_ids + } + extra_keys = _extra_field_keys(lines) + cols = _amount_columns(extra_keys) + _set_column_widths(ws, extra_keys, cols) + + plan = _plan_rows(ordered_category_ids, lines_by_category_id) + + _write_budget_summary( + ws, plan, ordered_category_ids, category_names, amount_header, estimate_header, + extra_keys, cols, local_fmt, estimate_fmt, has_rate, + ) + _write_detailed_budget( + ws, plan, ordered_category_ids, category_names, category_lines, extra_keys, cols, + local_fmt, estimate_fmt, has_rate, rate_cell, + ) + _write_footer( + ws, plan, ordered_category_ids, cols, local_fmt, estimate_fmt, has_rate, + exported_by, exported_at, + ) + + +def _set_column_widths(ws, extra_keys: list[str], cols: dict) -> None: + ws.column_dimensions[get_column_letter(_DESCRIPTION_COL)].width = _DESCRIPTION_COL_WIDTH + for i in range(len(extra_keys)): + ws.column_dimensions[get_column_letter(_DESCRIPTION_COL + 1 + i)].width = _EXTRA_COL_WIDTH + ws.column_dimensions[get_column_letter(cols["amount_col"])].width = _AMOUNT_COL_WIDTH + ws.column_dimensions[get_column_letter(cols["estimate_col"])].width = _ESTIMATE_COL_WIDTH + + +def _write_header( + ws, budget: BudgetModel, organisation_name: str | None, donor_name: str | None +) -> str: + """Writes the header block and returns the rate cell ref for formulas.""" + fields = ( + ("Organisation Name", organisation_name), + ("Donor Name", donor_name), + ("Project Name", budget.name), + ("Project Period", _period_label(budget.start_date, budget.duration_months)), + ("Estimated Currency", budget.actual_currency), + ("Estimated Exchange Rate", budget.estimated_exchange_rate), + ) + for row, (label, value) in enumerate(fields, start=1): + ws.cell(row=row, column=1, value=label) + ws.cell(row=row, column=2, value=value) + _bold_row(ws, row) + + return f"$B${_RATE_ROW}" + + +def _write_budget_summary( + ws, plan, ordered_category_ids, category_names, amount_header, estimate_header, + extra_keys, cols, local_fmt, estimate_fmt, has_rate, +) -> None: + amount_col, estimate_col = cols["amount_col"], cols["estimate_col"] + amount_letter, estimate_letter = cols["amount_letter"], cols["estimate_letter"] + + header_row = plan["summary_header_row"] + ws.cell(row=header_row, column=1, value="BUDGET SUMMARY") + for i, key in enumerate(extra_keys): + ws.cell(row=header_row, column=_DESCRIPTION_COL + 1 + i, value=key) + ws.cell(row=header_row, column=amount_col, value=amount_header) + ws.cell(row=header_row, column=estimate_col, value=estimate_header) + _bold_row(ws, header_row) + + for category_id in ordered_category_ids: + row = plan["summary_rows"][category_id] + detail_subtotal_row = plan["detail_subtotal_rows"][category_id] + ws.cell(row=row, column=1, value=category_names[category_id]) + _set_cell(ws, row, amount_col, f"={amount_letter}{detail_subtotal_row}", local_fmt) + if has_rate: + _set_cell( + ws, row, estimate_col, f"={estimate_letter}{detail_subtotal_row}", estimate_fmt + ) + if ordered_category_ids: + first_summary_row = plan["summary_rows"][ordered_category_ids[0]] + last_summary_row = plan["summary_rows"][ordered_category_ids[-1]] + _apply_box_border(ws, first_summary_row, last_summary_row, estimate_col) + + total_row = plan["summary_total_row"] + ws.cell(row=total_row, column=1, value="TOTAL") + if ordered_category_ids: + first_row = plan["summary_rows"][ordered_category_ids[0]] + last_row = plan["summary_rows"][ordered_category_ids[-1]] + amount_range = f"{amount_letter}{first_row}:{amount_letter}{last_row}" + _set_cell(ws, total_row, amount_col, f"=SUM({amount_range})", local_fmt) + if has_rate: + estimate_range = f"{estimate_letter}{first_row}:{estimate_letter}{last_row}" + _set_cell(ws, total_row, estimate_col, f"=SUM({estimate_range})", estimate_fmt) + else: + _set_cell(ws, total_row, amount_col, 0.0, local_fmt) + _bold_row(ws, total_row) + + +def _write_detailed_budget( + ws, plan, ordered_category_ids, category_names, category_lines, extra_keys, cols, + local_fmt, estimate_fmt, has_rate, rate_cell, +) -> None: + amount_col, estimate_col = cols["amount_col"], cols["estimate_col"] + amount_letter, estimate_letter = cols["amount_letter"], cols["estimate_letter"] + + ws.cell(row=plan["detail_title_row"], column=1, value="DETAILED BUDGET") + _bold_row(ws, plan["detail_title_row"]) + + for category_id in ordered_category_ids: + header_row = plan["detail_header_rows"][category_id] + ws.cell(row=header_row, column=1, value=category_names[category_id]) + _bold_row(ws, header_row) + + line_rows = plan["detail_line_rows"][category_id] + for line, row in zip(category_lines[category_id], line_rows): + ws.cell(row=row, column=1, value=line.description) + for i, key in enumerate(extra_keys): + value = (line.extra_fields or {}).get(key) + ws.cell(row=row, column=_DESCRIPTION_COL + 1 + i, value=value) + _set_cell(ws, row, amount_col, line.amount or 0.0, local_fmt) + if has_rate: + _set_cell(ws, row, estimate_col, f"={amount_letter}{row}/{rate_cell}", estimate_fmt) + if line_rows: + _apply_box_border(ws, line_rows[0], line_rows[-1], estimate_col) + + subtotal_row = plan["detail_subtotal_rows"][category_id] + ws.cell(row=subtotal_row, column=1, value="Subtotal") + if line_rows: + amount_range = f"{amount_letter}{line_rows[0]}:{amount_letter}{line_rows[-1]}" + _set_cell(ws, subtotal_row, amount_col, f"=SUM({amount_range})", local_fmt) + if has_rate: + estimate_range = ( + f"{estimate_letter}{line_rows[0]}:{estimate_letter}{line_rows[-1]}" + ) + _set_cell(ws, subtotal_row, estimate_col, f"=SUM({estimate_range})", estimate_fmt) + else: + _set_cell(ws, subtotal_row, amount_col, 0.0, local_fmt) + if has_rate: + formula = f"={amount_letter}{subtotal_row}/{rate_cell}" + _set_cell(ws, subtotal_row, estimate_col, formula, estimate_fmt) + _bold_row(ws, subtotal_row) + + +def _audit_line(exported_by: str | None, exported_at: datetime | None) -> str: + parts = ["Generated by OpenGrantFlow"] + if exported_by: + parts.append(exported_by) + if exported_at: + parts.append(exported_at.strftime("%Y-%m-%d %H:%M UTC")) + return " · ".join(parts) + + +def _write_footer( + ws, plan, ordered_category_ids, cols, local_fmt, estimate_fmt, has_rate, + exported_by, exported_at, +) -> None: + amount_col, estimate_col = cols["amount_col"], cols["estimate_col"] + amount_letter, estimate_letter = cols["amount_letter"], cols["estimate_letter"] + + total_row = plan["footer_total_row"] + ws.cell(row=total_row, column=1, value="Total expenditures") + if ordered_category_ids: + subtotal_rows = [plan["detail_subtotal_rows"][cid] for cid in ordered_category_ids] + c_formula = "=" + "+".join(f"{amount_letter}{r}" for r in subtotal_rows) + _set_cell(ws, total_row, amount_col, c_formula, local_fmt) + if has_rate: + d_formula = "=" + "+".join(f"{estimate_letter}{r}" for r in subtotal_rows) + _set_cell(ws, total_row, estimate_col, d_formula, estimate_fmt) + else: + _set_cell(ws, total_row, amount_col, 0.0, local_fmt) + for cell in ws[total_row]: + cell.font = _BOLD + cell.fill = _TOTAL_FILL + + signature_row = plan["signature_line_row"] + ws.cell(row=signature_row, column=1).border = _TOP_BORDER + ws.cell(row=signature_row + 1, column=1, value="Authorised Signatory") + + contact_row = plan["contact_line_row"] + ws.cell(row=contact_row, column=1).border = _TOP_BORDER + ws.cell(row=contact_row + 1, column=1, value="Project contact person") + + audit_cell = ws.cell( + row=plan["audit_row"], column=1, value=_audit_line(exported_by, exported_at) + ) + audit_cell.font = _AUDIT_FONT + + +def _set_cell(ws, row: int, column: int, value, number_format: str): + cell = ws.cell(row=row, column=column, value=value) + cell.number_format = number_format + return cell diff --git a/services/budget/main.py b/services/budget/main.py index b5512e39..e858570a 100644 --- a/services/budget/main.py +++ b/services/budget/main.py @@ -38,6 +38,10 @@ init_urls as user_client_init_urls, close_urls as close_user_client_urls, ) +from app.services.customer_client import ( # noqa: E402 + init_urls as customer_client_init_urls, + close_urls as close_customer_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 @@ -68,6 +72,8 @@ async def lifespan(app: FastAPI): async with asyncio.timeout(30): await user_client_init_urls() logger.info("user_client_initialized") + await customer_client_init_urls() + logger.info("customer_client_initialized") await init_consumer() logger.info("event_consumer_initialized") await start_consumer() @@ -80,6 +86,7 @@ async def lifespan(app: FastAPI): logger.info("app_shutdown", service="budget") await close_user_client_urls() + await close_customer_client_urls() await close_consumer() logger.info("event_consumer_stopped") diff --git a/services/budget/tests/test_budget_currency_fields.py b/services/budget/tests/test_budget_currency_fields.py index 00bf4da1..3a9f8c72 100644 --- a/services/budget/tests/test_budget_currency_fields.py +++ b/services/budget/tests/test_budget_currency_fields.py @@ -8,7 +8,7 @@ """ from datetime import date -from unittest.mock import patch +from unittest.mock import AsyncMock, patch from uuid import uuid4 import pytest @@ -89,6 +89,7 @@ def test_confirm_without_start_date_anywhere_is_rejected(self): with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( @@ -117,6 +118,7 @@ def test_confirm_with_start_date_already_on_record_is_allowed(self): with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( @@ -147,6 +149,7 @@ def test_confirm_with_start_date_in_payload_is_allowed(self): with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( @@ -201,6 +204,7 @@ def test_non_confirm_update_does_not_require_start_date(self): with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( diff --git a/services/budget/tests/test_budget_donor_commitment.py b/services/budget/tests/test_budget_donor_commitment.py index 8421e53b..2d15733d 100644 --- a/services/budget/tests/test_budget_donor_commitment.py +++ b/services/budget/tests/test_budget_donor_commitment.py @@ -102,6 +102,7 @@ def test_set_donor_commitment_on_unconfirmed_budget_is_accepted(self): with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( @@ -136,6 +137,7 @@ def test_donor_commitment_edit_rejected_on_confirmed_budget(self): with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( @@ -161,6 +163,7 @@ def test_estimated_exchange_rate_edit_rejected_on_confirmed_budget(self): with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( @@ -239,7 +242,11 @@ async def test_donor_total_amount_and_rate_can_be_cleared(self, db): payload = BudgetUpdate(donor_total_amount=None, estimated_exchange_rate=None) - with patch("app.services.budget_services.validate_customer_can_fund", return_value=None): + with patch( + "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, + return_value=None, + ): result = await update_budget_service(budget.id, payload, _valid_user(), db) assert result.donor_total_amount is None @@ -264,7 +271,11 @@ async def test_omitting_the_fields_leaves_them_unchanged(self, db): payload = BudgetUpdate(name="Renamed") - with patch("app.services.budget_services.validate_customer_can_fund", return_value=None): + with patch( + "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, + return_value=None, + ): result = await update_budget_service(budget.id, payload, _valid_user(), db) assert result.name == "Renamed" @@ -345,7 +356,11 @@ async def test_funding_customer_id_can_be_set_from_unset(self, db): payload = BudgetUpdate(funding_customer_id=donor_id, external_funder_name="") with ( - patch("app.services.budget_services.validate_customer_can_fund", return_value=None), + patch( + "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, + return_value=None, + ), patch( "app.services.budget_services.validate_donor_grantee_relationship", return_value=None, @@ -426,6 +441,7 @@ async def test_resaving_the_same_donor_alongside_other_edits_is_accepted(self, d with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( @@ -453,6 +469,7 @@ def test_confirmed_at_set_on_first_confirm(self): with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( @@ -486,6 +503,7 @@ def test_confirmed_at_updated_on_reconfirm_after_revert(self): with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( @@ -512,6 +530,7 @@ def test_confirmed_at_not_touched_on_non_confirm_update(self): with ( patch( "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, return_value=None, ), patch( diff --git a/services/budget/tests/test_budget_services.py b/services/budget/tests/test_budget_services.py index 332366f2..d1ebc444 100644 --- a/services/budget/tests/test_budget_services.py +++ b/services/budget/tests/test_budget_services.py @@ -379,6 +379,7 @@ def test_with_lines_creates_budget_with_ai_draft_status(self): ), patch( "app.services.budget_services.get_customer_cached", + new_callable=AsyncMock, return_value={"currency": "GBP"}, ), ): @@ -563,6 +564,7 @@ def test_with_lines_falls_back_to_org_currency_when_local_currency_missing(self) ), patch( "app.services.budget_services.get_customer_cached", + new_callable=AsyncMock, return_value={"currency": "AMD"}, ) as mock_get_customer, ): @@ -580,6 +582,7 @@ def test_with_lines_fails_when_currency_fallback_unreachable(self): with patch( "app.services.budget_services.get_customer_cached", + new_callable=AsyncMock, side_effect=CustomerServiceError("unreachable"), ): response = client.post("/api/v1/budgets/with-lines", json=VALID_PAYLOAD) diff --git a/services/budget/tests/test_customer_validation.py b/services/budget/tests/test_customer_validation.py index d299c5ce..964994b0 100644 --- a/services/budget/tests/test_customer_validation.py +++ b/services/budget/tests/test_customer_validation.py @@ -7,7 +7,7 @@ Subgranting orgs (is_ngo=True, is_donor=True) must pass both checks. """ import pytest -from unittest.mock import patch +from unittest.mock import AsyncMock, patch from uuid import uuid4 from app.services.customer_client import ( @@ -24,72 +24,82 @@ def _customer(is_ngo=False, is_donor=False): return {"id": CUSTOMER_ID, "name": "Test Org", "is_ngo": is_ngo, "is_donor": is_donor} +@pytest.mark.anyio class TestValidateCustomerCanFund: - def test_donor_passes(self): + async def test_donor_passes(self): with patch( "app.services.customer_client.get_customer_cached", + new_callable=AsyncMock, return_value=_customer(is_donor=True), ): - result = validate_customer_can_fund(CUSTOMER_ID) + result = await validate_customer_can_fund(CUSTOMER_ID) assert result["is_donor"] is True - def test_subgranting_ngo_passes(self): + async def test_subgranting_ngo_passes(self): with patch( "app.services.customer_client.get_customer_cached", + new_callable=AsyncMock, return_value=_customer(is_ngo=True, is_donor=True), ): - result = validate_customer_can_fund(CUSTOMER_ID) + result = await validate_customer_can_fund(CUSTOMER_ID) assert result["is_donor"] is True - def test_pure_ngo_raises_value_error(self): + async def test_pure_ngo_raises_value_error(self): with patch( "app.services.customer_client.get_customer_cached", + new_callable=AsyncMock, return_value=_customer(is_ngo=True, is_donor=False), ): with pytest.raises(ValueError): - validate_customer_can_fund(CUSTOMER_ID) + await validate_customer_can_fund(CUSTOMER_ID) - def test_pure_ngo_raises_domain_error_when_flagged(self): + async def test_pure_ngo_raises_domain_error_when_flagged(self): with patch( "app.services.customer_client.get_customer_cached", + new_callable=AsyncMock, return_value=_customer(is_ngo=True, is_donor=False), ): with pytest.raises(DomainError): - validate_customer_can_fund(CUSTOMER_ID, raise_domain_error=True) + await validate_customer_can_fund(CUSTOMER_ID, raise_domain_error=True) +@pytest.mark.anyio class TestValidateCustomerCanOwn: - def test_ngo_passes(self): + async def test_ngo_passes(self): with patch( "app.services.customer_client.get_customer_cached", + new_callable=AsyncMock, return_value=_customer(is_ngo=True), ): - result = validate_customer_can_own(CUSTOMER_ID) + result = await validate_customer_can_own(CUSTOMER_ID) assert result["is_ngo"] is True - def test_subgranting_ngo_passes(self): + async def test_subgranting_ngo_passes(self): with patch( "app.services.customer_client.get_customer_cached", + new_callable=AsyncMock, return_value=_customer(is_ngo=True, is_donor=True), ): - result = validate_customer_can_own(CUSTOMER_ID) + result = await validate_customer_can_own(CUSTOMER_ID) assert result["is_ngo"] is True - def test_pure_donor_raises_value_error(self): + async def test_pure_donor_raises_value_error(self): with patch( "app.services.customer_client.get_customer_cached", + new_callable=AsyncMock, return_value=_customer(is_ngo=False, is_donor=True), ): with pytest.raises(ValueError): - validate_customer_can_own(CUSTOMER_ID) + await validate_customer_can_own(CUSTOMER_ID) - def test_pure_donor_raises_domain_error_when_flagged(self): + async def test_pure_donor_raises_domain_error_when_flagged(self): with patch( "app.services.customer_client.get_customer_cached", + new_callable=AsyncMock, return_value=_customer(is_ngo=False, is_donor=True), ): with pytest.raises(DomainError): - validate_customer_can_own(CUSTOMER_ID, raise_domain_error=True) + await validate_customer_can_own(CUSTOMER_ID, raise_domain_error=True) class TestRequireDonor: diff --git a/services/budget/tests/test_donor_grantee_gate.py b/services/budget/tests/test_donor_grantee_gate.py index 8378864a..7e2d2e32 100644 --- a/services/budget/tests/test_donor_grantee_gate.py +++ b/services/budget/tests/test_donor_grantee_gate.py @@ -11,7 +11,7 @@ et al. mock validate_customer_can_fund at the customer_client boundary. """ import asyncio -from unittest.mock import patch +from unittest.mock import AsyncMock, patch from uuid import uuid4 import pytest @@ -116,7 +116,11 @@ def test_service_error_raises_domain_error_when_flagged(self): class TestCreateBudgetServiceGate: def test_create_rejected_without_approved_relationship(self): with ( - patch("app.services.budget_services.validate_customer_can_fund", return_value=None), + patch( + "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, + return_value=None, + ), patch( "app.services.donor_grantee_client.check_donor_grantee_relationship", return_value=False, @@ -130,7 +134,11 @@ def test_create_rejected_without_approved_relationship(self): def test_create_succeeds_with_approved_relationship(self): budget = BudgetFactory.build(owner_id=GRANTEE_ID, funding_customer_id=DONOR_ID) with ( - patch("app.services.budget_services.validate_customer_can_fund", return_value=None), + patch( + "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, + return_value=None, + ), patch( "app.services.donor_grantee_client.check_donor_grantee_relationship", return_value=True, @@ -173,7 +181,11 @@ def test_attaching_funder_rejected_without_approved_relationship(self): status=BudgetStatus.draft, ) with ( - patch("app.services.budget_services.validate_customer_can_fund", return_value=None), + patch( + "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, + return_value=None, + ), patch("app.services.budget_services.get_budget", return_value=existing), patch( "app.services.donor_grantee_client.check_donor_grantee_relationship", @@ -193,7 +205,11 @@ def test_attaching_funder_succeeds_with_approved_relationship(self): status=BudgetStatus.draft, ) with ( - patch("app.services.budget_services.validate_customer_can_fund", return_value=None), + patch( + "app.services.budget_services.validate_customer_can_fund", + new_callable=AsyncMock, + return_value=None, + ), patch("app.services.budget_services.get_budget", return_value=existing), patch( "app.services.donor_grantee_client.check_donor_grantee_relationship", diff --git a/services/budget/tests/test_excel_export_service.py b/services/budget/tests/test_excel_export_service.py new file mode 100644 index 00000000..a3615313 --- /dev/null +++ b/services/budget/tests/test_excel_export_service.py @@ -0,0 +1,371 @@ +import io +from datetime import date, datetime, timezone +from unittest.mock import AsyncMock, patch +from uuid import uuid4 + +import pytest +from openpyxl import load_workbook + +from app.models.budget import BudgetCategoryModel, BudgetLineModel, BudgetModel +from app.schemas.budget_schema import BudgetStatus +from app.services.excel_export_service import ( + _audit_line, + _period_label, + generate_budget_export_workbook, +) +from tests.factories.budget import BudgetCategoryFactory, BudgetFactory, BudgetLineFactory + +OWNER_ID = str(uuid4()) +FUNDER_ID = str(uuid4()) +STRANGER_ID = str(uuid4()) + + +class TestPeriodLabel: + def test_none_start_date_returns_none(self): + assert _period_label(None, 12) is None + + def test_formats_start_and_computed_end_month(self): + assert _period_label(date(2026, 1, 1), 12) == "01/2026-12/2026" + + def test_no_duration_shows_start_month_only(self): + assert _period_label(date(2026, 3, 15), None) == "03/2026" + + def test_zero_duration_shows_start_month_only(self): + """0 is BudgetModel.duration_months's column default — same "unknown" case as None.""" + assert _period_label(date(2026, 3, 15), 0) == "03/2026" + + +class TestAuditLine: + def test_includes_user_and_timestamp_when_both_known(self): + exported_at = datetime(2026, 9, 22, 14, 30, tzinfo=timezone.utc) + assert ( + _audit_line("a@b.com", exported_at) + == "Generated by OpenGrantFlow · a@b.com · 2026-09-22 14:30 UTC" + ) + + def test_omits_missing_parts(self): + assert _audit_line(None, None) == "Generated by OpenGrantFlow" + + +class TestGenerateBudgetExportWorkbook: + def test_returns_valid_xlsx_roundtrip(self): + budget = BudgetFactory.build() + + data = generate_budget_export_workbook(budget, [], []) + + wb = load_workbook(io.BytesIO(data)) + assert wb.sheetnames == ["Original Budget"] + + def test_full_layout_with_rate(self): + budget = BudgetFactory.build( + local_currency="GBP", actual_currency="USD", estimated_exchange_rate=2.0 + ) + cat1 = BudgetCategoryFactory.build( + budget=budget, budget_id=budget.id, name="Personnel", created_at=datetime(2026, 1, 1) + ) + cat2 = BudgetCategoryFactory.build( + budget=budget, budget_id=budget.id, name="Travel", created_at=datetime(2026, 1, 2) + ) + line1 = BudgetLineFactory.build( + budget=budget, + budget_id=budget.id, + category=cat1, + category_id=cat1.id, + description="Salaries", + amount=1000.0, + created_at=datetime(2026, 1, 1), + ) + line2 = BudgetLineFactory.build( + budget=budget, + budget_id=budget.id, + category=cat2, + category_id=cat2.id, + description="Flights", + amount=200.0, + created_at=datetime(2026, 1, 2), + ) + + data = generate_budget_export_workbook( + budget, + [cat1, cat2], + [line1, line2], + organisation_name="Test Org", + donor_name="Test Donor", + exported_by="exporter@example.com", + exported_at=datetime(2026, 9, 22, 14, 30, tzinfo=timezone.utc), + ) + + ws = load_workbook(io.BytesIO(data))["Original Budget"] + rows = list(ws.iter_rows(values_only=True)) + + # Header block; row 6 (Estimated Exchange Rate) is the formulas' rate cell ($B$6). + # No extra_fields anywhere in this budget, so Amount/Estimate sit in B/C (no gap column). + assert rows[0] == ("Organisation Name", "Test Org", None) + assert rows[1] == ("Donor Name", "Test Donor", None) + assert rows[2] == ("Project Name", budget.name, None) + assert rows[4] == ("Estimated Currency", "USD", None) + assert rows[5] == ("Estimated Exchange Rate", 2.0, None) + + # Budget Summary: title doubles as the column header row; each category + # references its own Detailed Budget subtotal cell, not a recomputed value. + assert rows[8] == ("BUDGET SUMMARY", "Amount (GBP)", "Estimate (USD)") + assert rows[9] == ("Personnel", "=B17", "=C17") + assert rows[10] == ("Travel", "=B21", "=C21") + assert rows[11] == ("TOTAL", "=SUM(B10:B11)", "=SUM(C10:C11)") + assert ws.cell(row=10, column=1).border.top.style == "thin" + assert ws.cell(row=10, column=1).border.left.style == "thin" + assert ws.cell(row=11, column=3).border.bottom.style == "thin" + assert ws.cell(row=11, column=3).border.right.style == "thin" + assert ws.cell(row=12, column=1).border.top.style is None + + # Detailed Budget: category header row (no repeated column headers), lines, + # then a SUM-formula subtotal row so hand-edits to a line amount recalculate. + assert rows[13] == ("DETAILED BUDGET", None, None) + assert rows[14] == ("Personnel", None, None) + assert rows[15] == ("Salaries", 1000.0, "=B16/$B$6") + assert rows[16] == ("Subtotal", "=SUM(B16:B16)", "=SUM(C16:C16)") + assert rows[18] == ("Travel", None, None) + assert rows[19] == ("Flights", 200.0, "=B20/$B$6") + assert rows[20] == ("Subtotal", "=SUM(B20:B20)", "=SUM(C20:C20)") + + assert ws.cell(row=16, column=1).border.top.style == "thin" + assert ws.cell(row=16, column=1).border.left.style == "thin" + assert ws.cell(row=16, column=3).border.top.style == "thin" + assert ws.cell(row=16, column=3).border.right.style == "thin" + assert ws.cell(row=16, column=1).border.bottom.style == "thin" + assert ws.cell(row=20, column=1).border.top.style == "thin" + assert ws.cell(row=20, column=1).border.bottom.style == "thin" + + # Footer: total expenditures sums each category's own subtotal cell. + assert rows[23] == ("Total expenditures", "=B17+B21", "=C17+C21") + assert rows[27] == ("Authorised Signatory", None, None) + assert rows[31] == ("Project contact person", None, None) + assert ws.cell(row=27, column=1).border.top.style == "thin" + assert ws.cell(row=31, column=1).border.top.style == "thin" + + assert rows[33] == ( + "Generated by OpenGrantFlow · exporter@example.com · 2026-09-22 14:30 UTC", + None, + None, + ) + assert ws.cell(row=34, column=1).font.italic is True + + total_expenditures_cell = ws.cell(row=24, column=1) + assert total_expenditures_cell.font.bold is True + assert total_expenditures_cell.fill.fgColor.rgb == "00D9D9D9" + + assert ws.column_dimensions["A"].width == 26.63 + assert ws.column_dimensions["B"].width == 17.77 + assert ws["B16"].number_format == '#,##0.00" GBP"' + assert ws["C16"].number_format == '#,##0.00" USD"' + + def test_blank_estimate_column_when_rate_unset(self): + budget = BudgetFactory.build( + local_currency="GBP", actual_currency="USD", estimated_exchange_rate=None + ) + category = BudgetCategoryFactory.build(budget=budget, budget_id=budget.id, name="Personnel") + line = BudgetLineFactory.build( + budget=budget, + budget_id=budget.id, + category=category, + category_id=category.id, + description="Salaries", + amount=1000.0, + ) + + data = generate_budget_export_workbook(budget, [category], [line]) + + rows = list(load_workbook(io.BytesIO(data))["Original Budget"].iter_rows(values_only=True)) + assert rows[5] == ("Estimated Exchange Rate", None, None) + assert rows[9] == ("Personnel", "=B16", None) + assert rows[13] == ("Personnel", None, None) + assert rows[14] == ("Salaries", 1000.0, None) + assert rows[15] == ("Subtotal", "=SUM(B15:B15)", None) + assert rows[18] == ("Total expenditures", "=B16", None) + + def test_single_extra_field_key_becomes_its_own_column(self): + budget = BudgetFactory.build(local_currency="GBP") + category = BudgetCategoryFactory.build(budget=budget, budget_id=budget.id, name="Staff") + line1 = BudgetLineFactory.build( + budget=budget, + budget_id=budget.id, + category=category, + category_id=category.id, + description="No extra", + amount=1000.0, + extra_fields=None, + created_at=datetime(2026, 1, 1), + ) + line2 = BudgetLineFactory.build( + budget=budget, + budget_id=budget.id, + category=category, + category_id=category.id, + description="AAAAAAA", + amount=6000.0, + extra_fields={"Description custom": "Hello world"}, + created_at=datetime(2026, 1, 2), + ) + + data = generate_budget_export_workbook(budget, [category], [line1, line2]) + + ws = load_workbook(io.BytesIO(data))["Original Budget"] + rows = list(ws.iter_rows(values_only=True)) + assert ws.max_column == 4 + assert rows[8] == ("BUDGET SUMMARY", "Description custom", "Amount (GBP)", "Donor Estimate") + assert rows[9] == ("Staff", None, "=C17", None) + assert rows[13] == ("Staff", None, None, None) + assert rows[14] == ("No extra", None, 1000, None) + assert rows[15] == ("AAAAAAA", "Hello world", 6000, None) + assert ws.column_dimensions["B"].width == 20.0 + + def test_multiple_extra_field_keys_become_separate_columns(self): + budget = BudgetFactory.build(local_currency="GBP") + category = BudgetCategoryFactory.build(budget=budget, budget_id=budget.id, name="Staff") + line = BudgetLineFactory.build( + budget=budget, + budget_id=budget.id, + category=category, + category_id=category.id, + description="Coordinator", + amount=500.0, + extra_fields={"Notes": "Approved", "Vendor": "Acme"}, + ) + + data = generate_budget_export_workbook(budget, [category], [line]) + + ws = load_workbook(io.BytesIO(data))["Original Budget"] + rows = list(ws.iter_rows(values_only=True)) + assert ws.max_column == 5 + assert rows[8] == ( + "BUDGET SUMMARY", "Notes", "Vendor", "Amount (GBP)", "Donor Estimate" + ) + assert rows[14] == ("Coordinator", "Approved", "Acme", 500, None) + + def test_no_extra_fields_omits_extra_columns_entirely(self): + budget = BudgetFactory.build(local_currency="GBP") + category = BudgetCategoryFactory.build(budget=budget, budget_id=budget.id, name="Staff") + line = BudgetLineFactory.build( + budget=budget, + budget_id=budget.id, + category=category, + category_id=category.id, + description="Salaries", + amount=1000.0, + extra_fields=None, + ) + + data = generate_budget_export_workbook(budget, [category], [line]) + + ws = load_workbook(io.BytesIO(data))["Original Budget"] + rows = list(ws.iter_rows(values_only=True)) + assert ws.max_column == 3 + assert rows[8] == ("BUDGET SUMMARY", "Amount (GBP)", "Donor Estimate") + assert rows[13] == ("Staff", None, None) + assert rows[14] == ("Salaries", 1000, None) + + def test_empty_category_still_shown_with_zero_subtotal(self): + budget = BudgetFactory.build(local_currency="GBP") + empty_category = BudgetCategoryFactory.build( + budget=budget, budget_id=budget.id, name="Contingency" + ) + + data = generate_budget_export_workbook(budget, [empty_category], []) + + rows = list(load_workbook(io.BytesIO(data))["Original Budget"].iter_rows(values_only=True)) + assert rows[9] == ("Contingency", "=B15", None) + assert rows[13] == ("Contingency", None, None) + assert rows[14] == ("Subtotal", 0.0, None) + + +async def _make_budget(db, owner_id=OWNER_ID, funding_customer_id=None): + # Raw model, not BudgetFactory: setting its .budget relationship nulls the FK at flush. + budget = BudgetModel( + name="Test Budget", + owner_id=owner_id, + funding_customer_id=funding_customer_id, + status=BudgetStatus.confirmed, + start_date=date(2026, 1, 1), + duration_months=12, + local_currency="GBP", + ) + db.add(budget) + await db.commit() + await db.refresh(budget) + return budget + + +async def _make_category(db, budget_id, name="Personnel"): + category = BudgetCategoryModel(budget_id=budget_id, name=name) + db.add(category) + await db.commit() + await db.refresh(category) + return category + + +async def _make_line(db, budget_id, category_id, amount=1000.0): + line = BudgetLineModel( + budget_id=budget_id, category_id=category_id, description="Salaries", amount=amount + ) + db.add(line) + await db.commit() + await db.refresh(line) + return line + + +@pytest.mark.anyio +class TestExportBudgetWorkbookRoute: + async def test_owner_can_export(self, db, make_client): + budget = await _make_budget(db) + category = await _make_category(db, budget.id) + await _make_line(db, budget.id, category.id) + client = make_client(db=db, customer_id=OWNER_ID) + + with ( + patch( + "app.services.excel_export_service.get_customer_cached", + new_callable=AsyncMock, + return_value={"name": "Test Org"}, + ), + patch( + "app.services.excel_export_service.get_users_by_ids_cached", + new_callable=AsyncMock, + return_value={}, + ), + ): + response = client.get(f"/api/v1/budgets/{budget.id}/export.xlsx") + + assert response.status_code == 200 + assert response.headers["content-type"] == ( + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" + ) + wb = load_workbook(io.BytesIO(response.content)) + assert wb.sheetnames == ["Original Budget"] + + async def test_funder_can_export(self, db, make_client): + budget = await _make_budget(db, funding_customer_id=FUNDER_ID) + client = make_client(db=db, customer_id=FUNDER_ID) + + with ( + patch( + "app.services.excel_export_service.get_customer_cached", + new_callable=AsyncMock, + return_value={"name": "Test Org"}, + ), + patch( + "app.services.excel_export_service.get_users_by_ids_cached", + new_callable=AsyncMock, + return_value={}, + ), + ): + response = client.get(f"/api/v1/budgets/{budget.id}/export.xlsx") + + assert response.status_code == 200 + + async def test_stranger_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.xlsx") + + assert response.status_code == 400 From a37862928d0d956b2940265cf76e54c373ad094d Mon Sep 17 00:00:00 2001 From: Norair Arutshyan Date: Wed, 23 Sep 2026 13:09:30 +0100 Subject: [PATCH 2/4] style: apply black formatting to excel export / customer client files Pre-push hook's black --check caught formatting drift in the two files touched by the previous commit. Co-Authored-By: Claude Sonnet 5 --- .../budget/app/services/customer_client.py | 4 +- .../app/services/excel_export_service.py | 92 ++++++++++++++----- 2 files changed, 72 insertions(+), 24 deletions(-) diff --git a/services/budget/app/services/customer_client.py b/services/budget/app/services/customer_client.py index ff1200ea..a2472300 100644 --- a/services/budget/app/services/customer_client.py +++ b/services/budget/app/services/customer_client.py @@ -82,9 +82,7 @@ def require_donor(valid_user: dict) -> None: raise DomainError("Customer is not a donor", status.HTTP_403_FORBIDDEN) -async def validate_customer_can_own( - customer_id: str | uuid.UUID, raise_domain_error: bool = False -): +async def validate_customer_can_own(customer_id: str | uuid.UUID, raise_domain_error: bool = False): """Assert the customer has is_ngo=True (can receive grants / own budgets).""" Error = DomainError if raise_domain_error else ValueError try: diff --git a/services/budget/app/services/excel_export_service.py b/services/budget/app/services/excel_export_service.py index 6b17cdd1..a48f4bc1 100644 --- a/services/budget/app/services/excel_export_service.py +++ b/services/budget/app/services/excel_export_service.py @@ -56,9 +56,13 @@ async def export_budget_workbook_service( users = {} exported_by = users.get(str(valid_user["user_id"]), {}).get("email") return budget, generate_budget_export_workbook( - budget, categories, lines, - organisation_name=organisation_name, donor_name=donor_name, - exported_by=exported_by, exported_at=datetime.now(timezone.utc), + budget, + categories, + lines, + organisation_name=organisation_name, + donor_name=donor_name, + exported_by=exported_by, + exported_at=datetime.now(timezone.utc), ) @@ -219,9 +223,7 @@ def _write_sheet1( cid: (categories_by_id[cid].name if cid else "Uncategorized") for cid in ordered_category_ids } - category_lines = { - cid: lines_by_category_id.get(cid, []) for cid in ordered_category_ids - } + category_lines = {cid: lines_by_category_id.get(cid, []) for cid in ordered_category_ids} extra_keys = _extra_field_keys(lines) cols = _amount_columns(extra_keys) _set_column_widths(ws, extra_keys, cols) @@ -229,16 +231,41 @@ def _write_sheet1( plan = _plan_rows(ordered_category_ids, lines_by_category_id) _write_budget_summary( - ws, plan, ordered_category_ids, category_names, amount_header, estimate_header, - extra_keys, cols, local_fmt, estimate_fmt, has_rate, + ws, + plan, + ordered_category_ids, + category_names, + amount_header, + estimate_header, + extra_keys, + cols, + local_fmt, + estimate_fmt, + has_rate, ) _write_detailed_budget( - ws, plan, ordered_category_ids, category_names, category_lines, extra_keys, cols, - local_fmt, estimate_fmt, has_rate, rate_cell, + ws, + plan, + ordered_category_ids, + category_names, + category_lines, + extra_keys, + cols, + local_fmt, + estimate_fmt, + has_rate, + rate_cell, ) _write_footer( - ws, plan, ordered_category_ids, cols, local_fmt, estimate_fmt, has_rate, - exported_by, exported_at, + ws, + plan, + ordered_category_ids, + cols, + local_fmt, + estimate_fmt, + has_rate, + exported_by, + exported_at, ) @@ -271,8 +298,17 @@ def _write_header( def _write_budget_summary( - ws, plan, ordered_category_ids, category_names, amount_header, estimate_header, - extra_keys, cols, local_fmt, estimate_fmt, has_rate, + ws, + plan, + ordered_category_ids, + category_names, + amount_header, + estimate_header, + extra_keys, + cols, + local_fmt, + estimate_fmt, + has_rate, ) -> None: amount_col, estimate_col = cols["amount_col"], cols["estimate_col"] amount_letter, estimate_letter = cols["amount_letter"], cols["estimate_letter"] @@ -315,8 +351,17 @@ def _write_budget_summary( def _write_detailed_budget( - ws, plan, ordered_category_ids, category_names, category_lines, extra_keys, cols, - local_fmt, estimate_fmt, has_rate, rate_cell, + ws, + plan, + ordered_category_ids, + category_names, + category_lines, + extra_keys, + cols, + local_fmt, + estimate_fmt, + has_rate, + rate_cell, ) -> None: amount_col, estimate_col = cols["amount_col"], cols["estimate_col"] amount_letter, estimate_letter = cols["amount_letter"], cols["estimate_letter"] @@ -347,9 +392,7 @@ def _write_detailed_budget( amount_range = f"{amount_letter}{line_rows[0]}:{amount_letter}{line_rows[-1]}" _set_cell(ws, subtotal_row, amount_col, f"=SUM({amount_range})", local_fmt) if has_rate: - estimate_range = ( - f"{estimate_letter}{line_rows[0]}:{estimate_letter}{line_rows[-1]}" - ) + estimate_range = f"{estimate_letter}{line_rows[0]}:{estimate_letter}{line_rows[-1]}" _set_cell(ws, subtotal_row, estimate_col, f"=SUM({estimate_range})", estimate_fmt) else: _set_cell(ws, subtotal_row, amount_col, 0.0, local_fmt) @@ -369,8 +412,15 @@ def _audit_line(exported_by: str | None, exported_at: datetime | None) -> str: def _write_footer( - ws, plan, ordered_category_ids, cols, local_fmt, estimate_fmt, has_rate, - exported_by, exported_at, + ws, + plan, + ordered_category_ids, + cols, + local_fmt, + estimate_fmt, + has_rate, + exported_by, + exported_at, ) -> None: amount_col, estimate_col = cols["amount_col"], cols["estimate_col"] amount_letter, estimate_letter = cols["amount_letter"], cols["estimate_letter"] From f550c3bd3e45b03ae71de8eb5d70458826d365cd Mon Sep 17 00:00:00 2001 From: Norair Arutshyan Date: Wed, 23 Sep 2026 14:03:02 +0100 Subject: [PATCH 3/4] fix(types): satisfy mypy on category/line id and owner_id fallback CI's mypy caught three arg-type errors not surfaced by the local pre-push cache: annotate categories_by_id/ordered_category_ids as UUID | None (category_id is a nullable FK), and assert owner_id is not None before the get_customer_cached call, matching the same idiom already used for owner_id elsewhere in this file. Co-Authored-By: Claude Sonnet 5 --- services/budget/app/services/budget_services.py | 1 + services/budget/app/services/excel_export_service.py | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/services/budget/app/services/budget_services.py b/services/budget/app/services/budget_services.py index 11a112ae..d630872c 100644 --- a/services/budget/app/services/budget_services.py +++ b/services/budget/app/services/budget_services.py @@ -599,6 +599,7 @@ async def create_budget_with_lines_service( ): try: owner_id = request.owner_id or valid_user.get("customer_id") + assert owner_id is not None local_currency = request.local_currency if not local_currency: diff --git a/services/budget/app/services/excel_export_service.py b/services/budget/app/services/excel_export_service.py index a48f4bc1..956e911f 100644 --- a/services/budget/app/services/excel_export_service.py +++ b/services/budget/app/services/excel_export_service.py @@ -208,14 +208,16 @@ def _write_sheet1( f"Estimate ({budget.actual_currency})" if budget.actual_currency else "Donor Estimate" ) - categories_by_id = {category.id: category for category in categories} + categories_by_id: dict[UUID | None, BudgetCategoryModel] = { + category.id: category for category in categories + } lines_by_category_id: dict = {} for line in lines: category = categories_by_id.get(line.category_id) lines_by_category_id.setdefault(category.id if category else None, []).append(line) # categories/lines already arrive ordered by created_at, id from the CRUD queries. - ordered_category_ids = list(categories_by_id) + ordered_category_ids: list[UUID | None] = list(categories_by_id) if None in lines_by_category_id: ordered_category_ids.append(None) From ffa9506378f8ef9b47f7b4dde37bba52fb59977f Mon Sep 17 00:00:00 2001 From: Norair Arutshyan Date: Wed, 23 Sep 2026 14:14:49 +0100 Subject: [PATCH 4/4] feat(export): refactor sheet writing into _SheetWriter base class and implement subclasses for budget, dashboard, and expense list sheets --- .../budget-feat-313-excel-export/design.md | 5 +++++ .../changes/budget-feat-313-excel-export/tasks.md | 15 ++++++++------- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/openspec/changes/budget-feat-313-excel-export/design.md b/openspec/changes/budget-feat-313-excel-export/design.md index 99bffdd3..6cf0d487 100644 --- a/openspec/changes/budget-feat-313-excel-export/design.md +++ b/openspec/changes/budget-feat-313-excel-export/design.md @@ -66,6 +66,11 @@ Migration seeds one `export_templates` row (`owner_customer_id` NULL, `visibilit `_audit_line()` already writes "Generated by OpenGrantFlow · · "; it gains the template name and version. Templates are versioned (an edit bumps `version`), so a file exported last quarter stays explicable after its template is edited. *Alternative considered*: an `export_history` table recording every generated file. Rejected as scope creep — nothing currently needs to enumerate past exports, and the footer answers the question the audit trail actually asks. +**12. Sheet-writing code is organized as one class per sheet, sharing a `_SheetWriter` base — a code-organization decision, orthogonal to Decision 9's template-options axis.** +Group 1's Sheet 1 implementation threads the same handful of values (`ws`, `plan`, `cols`, `local_fmt`, `estimate_fmt`, `has_rate`) through five-plus free functions; Sheets 2 and 3 land with the same shape of state. `_SheetWriter` holds the shared cell-writing helpers (`_bold_row`, `_set_cell`, `_apply_box_border`, currency formatting) as methods; `OriginalBudgetSheet`, `DashboardSheet`, `ExpenseListSheet` each subclass it with a `write()` entry point. `generate_budget_export_workbook` stays a thin dispatcher choosing which sheet classes to instantiate — group 6's template-driven sheet subset (Decision 9) selects among these same fixed classes, it does not introduce new ones per template. +*Alternative considered*: one class per donor-selectable template (each a bespoke layout), inheriting a shared parent. Rejected — same trap as Decision 9's declarative-engine alternative: unbounded, hard to review, and duplicates the single-rendering-path guarantee Decision 10 exists to give. Templates still only vary bounded options over a fixed, closed set of sheet renderers. +*Timing*: introduced in group 2 (converting Sheet 1's free functions into `OriginalBudgetSheet` alongside building `DashboardSheet`), not group 1. A base class guessed from one caller is unproven; group 2 gives two real sheets to validate what's actually shared before group 3 adds a third. + ## Risks / Trade-offs - [A budget with many report lines/allocations could make generation slow] → Out of scope for a "simple export" of one budget; single-budget expense volume in practice is small (tens to low hundreds of lines), revisit only if real usage shows otherwise. diff --git a/openspec/changes/budget-feat-313-excel-export/tasks.md b/openspec/changes/budget-feat-313-excel-export/tasks.md index 7e1d666a..2237d8dc 100644 --- a/openspec/changes/budget-feat-313-excel-export/tasks.md +++ b/openspec/changes/budget-feat-313-excel-export/tasks.md @@ -13,16 +13,17 @@ Workflow rule: one task group = one GitHub sub-issue (of this change's parent is ## 2. Sheet 2 — Budget vs. Report Dashboard — depends on 1 -- [ ] 2.1 Add `services/budget/app/crud/excel_export_crud.py` with a per-budget-line rollup query (join `budget_lines` → `report_lines`, grouped by `budget_line_id`) returning each line's total local-currency expenses and its allocations' `(amount_allocated, conversion.donor_amount, conversion.local_amount)` tuples; verify with a unit test against a seeded budget with lines spanning fully-allocated, partially-allocated, and zero-expense cases -- [ ] 2.2 Implement the converted-expense calculation in `excel_export_service.py`: real per-allocation rate for allocated amounts plus `estimated_exchange_rate` for any unsatisfied remainder, flagging a line as "includes estimate" when a remainder exists; verify with a unit test covering fully-allocated (no flag), partially-allocated (flagged, blended figure), and fully-unallocated (fully estimated, flagged) cases -- [ ] 2.3 Implement Sheet 2's income section: one row per `CurrencyConversion` for the budget (converted date, donor amount, local amount, implied rate) plus a total row; verify with a unit test using a budget with one funding receipt and multiple conversions, asserting one row per conversion -- [ ] 2.4 Implement Sheet 2's per-budget-line and category-subtotal rows (expenses local, expenses converted, deviation), reusing Sheet 1's category grouping/order and applying the estimated-portion cell style (italic + fill, per design.md Decision 4) where flagged; verify with a unit test asserting column values and that flagged cells carry the style -- [ ] 2.5 Run backend lint/tests clean for `services/budget`; PR merged (`Closes` this group's sub-issue) +- [ ] 2.1 Introduce a `_SheetWriter` base class in `excel_export_service.py` holding the shared cell-writing helpers (`_bold_row`, `_set_cell`, `_apply_box_border`, currency formatting) as methods, and refactor Sheet 1's existing free functions into an `OriginalBudgetSheet(_SheetWriter)` subclass with a `write()` entry point — pure refactor, no output change; verify group 1's existing Sheet 1 tests pass unchanged (see design.md Decision 12) +- [ ] 2.2 Add `services/budget/app/crud/excel_export_crud.py` with a per-budget-line rollup query (join `budget_lines` → `report_lines`, grouped by `budget_line_id`) returning each line's total local-currency expenses and its allocations' `(amount_allocated, conversion.donor_amount, conversion.local_amount)` tuples; verify with a unit test against a seeded budget with lines spanning fully-allocated, partially-allocated, and zero-expense cases +- [ ] 2.3 Implement the converted-expense calculation in `excel_export_service.py`: real per-allocation rate for allocated amounts plus `estimated_exchange_rate` for any unsatisfied remainder, flagging a line as "includes estimate" when a remainder exists; verify with a unit test covering fully-allocated (no flag), partially-allocated (flagged, blended figure), and fully-unallocated (fully estimated, flagged) cases +- [ ] 2.4 Implement Sheet 2 as a `DashboardSheet(_SheetWriter)` subclass: an income section (one row per `CurrencyConversion` for the budget — converted date, donor amount, local amount, implied rate — plus a total row); verify with a unit test using a budget with one funding receipt and multiple conversions, asserting one row per conversion +- [ ] 2.5 Add `DashboardSheet`'s per-budget-line and category-subtotal rows (expenses local, expenses converted, deviation), reusing Sheet 1's category grouping/order and applying the estimated-portion cell style (italic + fill, per design.md Decision 4) where flagged; verify with a unit test asserting column values and that flagged cells carry the style +- [ ] 2.6 Run backend lint/tests clean for `services/budget`; PR merged (`Closes` this group's sub-issue) ## 3. Sheet 3 — List of Expenses with per-allocation sublines — depends on 1 - [ ] 3.1 Extend `excel_export_crud.py` with a query returning every report line across all of the budget's reports, each with its ordered list of allocations (or none) -- [ ] 3.2 Implement Sheet 3 generation: one row per report line when zero or one allocation exists, one row per allocation (subline) when a report line has more than one, each subline carrying its own conversion date/rate/converted amount; verify with a unit test covering an unallocated expense, a single-lot expense, and a multi-lot expense (asserting the multi-lot rows sum to the expense's full amount) +- [ ] 3.2 Implement Sheet 3 as an `ExpenseListSheet(_SheetWriter)` subclass (see design.md Decision 12): one row per report line when zero or one allocation exists, one row per allocation (subline) when a report line has more than one, each subline carrying its own conversion date/rate/converted amount; verify with a unit test covering an unallocated expense, a single-lot expense, and a multi-lot expense (asserting the multi-lot rows sum to the expense's full amount) - [ ] 3.3 Run backend lint/tests clean for `services/budget`; PR merged (`Closes` this group's sub-issue) ## 4. Frontend export button — depends on 1, 2, 3 @@ -47,7 +48,7 @@ Workflow rule: one task group = one GitHub sub-issue (of this change's parent is ## 6. Apply template options to generation — depends on 2, 3, 5 - [ ] 6.1 Thread an optional `template_id` through `GET /budgets/{budget_id}/export.xlsx`: resolve against `list_candidate_templates`, use the single candidate when none is given, reject with 400 when none is given and more than one exists, reject with 403 when the given id is not a candidate, and reject with a distinct "no longer available" error when a previously valid id has been deleted or un-shared; verify with integration tests covering each of those four outcomes -- [ ] 6.2 Make `generate_budget_export_workbook` take the resolved template's options and apply them — sheet subset, donor-currency estimate column visibility, column header label overrides, audit footer visibility — so the system default runs the same path as any other template; verify that group 1's and groups 2–3's existing tests pass unchanged when the system default is used, plus new tests asserting a Sheet-1-only template yields one sheet and a label override changes only the header text while every figure and formula is unchanged +- [ ] 6.2 Make `generate_budget_export_workbook` take the resolved template's options and apply them — sheet subset, donor-currency estimate column visibility, column header label overrides, audit footer visibility — so the system default runs the same path as any other template; the sheet-subset option selects which of the fixed `OriginalBudgetSheet`/`DashboardSheet`/`ExpenseListSheet` classes (design.md Decision 12) the dispatcher instantiates, never a new layout; verify that group 1's and groups 2–3's existing tests pass unchanged when the system default is used, plus new tests asserting a Sheet-1-only template yields one sheet and a label override changes only the header text while every figure and formula is unchanged - [ ] 6.3 Extend `_audit_line()` to include the template's name and version alongside the exporting user and timestamp; verify with a unit test asserting the footer text for a named template and for the system default - [ ] 6.4 Run backend lint/tests clean for `services/budget`; PR merged (`Closes` this group's sub-issue)