# AccountReports: Developer Playbook

This is the process that produced every report in [AccountReports_Catalog.md](AccountReports_Catalog.md),
written down so the next report doesn't have to rediscover it. It's organized as: the process for
**migrating a legacy report**, then what changes for **building a genuinely new report** (one with
no legacy equivalent), then a running list of **lessons learned** — real bugs and gotchas found
during this work that will bite the next report too if you don't know about them.

Read this alongside the main repo `CLAUDE.md` (architecture rules, coding standards, anti-patterns)
— this document is specific to the AccountReports family; `CLAUDE.md` is the general GB5 backend
standard.

## What's automated today vs. developer-driven

Be honest with yourself and with stakeholders about this: **there is no automated migration tool
today.** Every report in this catalog was produced by a developer (working with an AI pair)
following the manual process below, step by step, with live verification at every stage. "Automated
migration" as a goal means something narrower right now: an AI assistant can do the mechanical parts
fast (reading legacy source, drafting SQL, generating the SL/BLL/DAL boilerplate, writing the seed
migration) — but a human (or an AI held to the same standard) still has to make every judgment call
below, and still has to verify every result against a live database and a live HTTP call. Don't
promise stakeholders "we'll just auto-migrate the rest" — promise "we have a proven, repeatable
process that goes faster each time because the shared framework grows."

## The process, step by step

### 1. Read the actual legacy source — don't guess

Find the legacy DAL method (`AccountsReportsDAL.cs` in the GB4.7 codebase) and its SQL query
builder constant (`AccountsReportQueryBuilder.cs`). Read the **entire** method and the **entire**
SQL text, not a summary. Legacy comments often carry load-bearing business rules
("`-- Added by Sarany as Per VV sir Sugesstion`", "`-- only with account post yes`") that look like
throwaway comments but encode a real, deliberate business decision — don't drop them just because
they're informally written.

If the report is a multi-step `#temp`-table pipeline (Collection/Payment Projection was), read every
step in order and understand what each one computes before deciding how to reimplement it — GB5's
`IQueryExecutor` doesn't give you an easy multi-batch temp-table path, so you'll usually be
translating the pipeline into a single query using `OUTER APPLY`/window functions (see step 3).

### 2. Verify the GB5 schema live — before writing a single line of new SQL

GB5 is not always schema-identical to legacy. Query the **live** database
(`INFORMATION_SCHEMA.COLUMNS`, or just read an existing GB5 QB file that already touches the same
table) for every table/column you're about to use. Things that have bitten this migration wave:

- A column that exists in both legacy and GB5 can have a completely different sign convention or
  sentinel-for-"not set" value (see "ID sign conventions" in Lessons Learned below).
- A DB function or view that looks like it should exist sometimes doesn't, or exists under a
  different name, or behaves differently (undated vs. as-of-date — see `FNPENDINGBILL` vs.
  `VPENDINGBILL` in the Catalog's Outstanding entry).
- A table used one way in one module (e.g. `TMMHEAD.PAYMENTTERMSID` looking MM/purchase-specific in
  legacy) can turn out to be genuinely shared across modules in GB5's actual schema — confirmed
  live for Collection/Payment Projection, which is *why* the cash-discount feature could be made
  bidirectional at all. Don't assume a table's apparent purpose from its legacy usage; check what it
  actually contains.

### 3. Design the SQL, adapted to GB5's conventions, not copy-pasted

- **Parameterize everything.** No string concatenation of caller-controlled values, ever — every
  report in this catalog uses Dapper's `DynamicParameters`, never raw string interpolation of a
  filter value. A genuinely variable *number* of SQL fragments (e.g. Ageing's configurable bucket
  count) is fine, as long as every *value* inside each fragment is still a bound parameter.
- **No leading CTE.** `IQueryExecutor.QueryPagedAsync` has exactly one overload and derives the row
  count by wrapping your entire query as `SELECT COUNT(*) FROM (<your SQL>) AS Total` — a query that
  opens with `;WITH ... AS (...)` can't be nested inside a derived table that way, and SQL Server
  rejects it. Every report that needs "value computed from earlier rows" logic (Ledger's opening
  balance, Collection Projection's account-level rollups) uses a correlated `OUTER APPLY`/subquery
  or a window function (`OVER (PARTITION BY ...)`) instead. This is the single most common structural
  mistake to avoid — build the habit of reaching for `OUTER APPLY` or `OVER()` before a CTE.
- **One consistent OU-access-ceiling.** Every report applies the same
  `V.OUID IN (SELECT ... FROM MUSERACCESSRIGHTS ... UNION SELECT ... FROM MORGANIZATIONGROUPDETAIL
  ...)` pattern (copy it verbatim from any existing QB file — e.g.
  `AccountLedgerReportsQB.OU_ACCESS_CEILING`-equivalent). If a DB function like `FNPENDINGBILL` also
  takes its own `@OUIds` parameter, pass it an empty string (its own internal length check bypasses
  its filter) and rely on the outer ceiling instead — one security mechanism, not two.
- **Match the shared alias convention if you can.** `V`/`VD`/`ACC`/`BTT` for
  `TVOUCHER`/`TVOUCHERDETAIL`/`MACCOUNT`/`MBIZTRANSACTIONTYPE` is the convention every report except
  Settlement Register uses — matching it means you can reuse `AccountReportFilterBuilder.Build()`
  and `BuildDetailOnly()` unchanged. If your query's natural shape doesn't fit that alias set (an
  audit-trail join like Settlement's, self-joining `TBILLALLOCATION`), don't force it — write a
  dedicated `Build<YourReport>Only()` method with its own aliases (see `BuildSettlementOnly` as the
  template) rather than contorting the query to match aliases it doesn't naturally have.

### 4. Validate the SQL live — before it touches any C# wiring

Never trust that a query compiles just because `dotnet build` passes — SQL text inside a C# string
constant isn't checked by the compiler. Before wiring a new query into a DAL class:

1. If the query is built from multiple `const string` fragments concatenated together (every QB file
   in this catalog does this), you need the **actual resolved SQL text**, not the raw literal
   between one fragment's quotes — write a tiny throwaway console program that references the QB
   class and prints the constant (a few lines, `dotnet run`), rather than trying to reconstruct the
   concatenation by hand or with a text-processing script. Getting this wrong (regex-extracting raw
   literal text instead of the resolved string) produces SQL with leftover C# syntax in it and wastes
   a whole round-trip.
2. Substitute `{DynamicFilter}`/`{OrderBy}`-style placeholders with real values, declare the `@`
   parameters with real sample values, and run it via `sqlcmd` against the actual target database
   (GB5DEMO for this wave) — not a mental read-through.
3. Sanity-check the actual returned numbers against what you know about the real data (e.g. "this
   receivable total should roughly match what we already validated for a sibling report").

### 5. Build the SL → BLL → DAL layers

Follow the existing template exactly — copy the newest sibling report's file structure
(`AccountSettlementRegisterReport.cs`/`AccountSettlementReportBLL.cs`/`AccountSettlementReportDAL.cs`
are a clean, recent example) rather than starting from a blank file. Specifically:
- DTO: reuse an existing scaffolded-but-orphaned DTO if one exists (check
  `AccountsDAL/DTO/AccountsReports/` and `AccountsDAL/DTO/Reports/` first) before creating a new one
  — several of these reports found a DTO that had already been scaffolded for the legacy row shape
  but never wired to anything.
- Criteria: extend the single shared `AccountReportCriteria` class (see
  [AccountReports_Reference.md](AccountReports_Reference.md) for the full current field list) and
  `AccountReportCriteriaParser.Parse()`'s switch statement, rather than inventing a new
  criteria-parsing path. Add a new field only when nothing existing covers the concept.
- Access control: pick `EnforceAsync` (real `PeriodFromDate`/`PeriodToDate` range) or
  `EnforceForAsOnDateAsync` (single `AsOnDate`, no range) based on what the report's criteria
  actually are — don't reuse `Build()`'s `PeriodFromDate`/`PeriodToDate` parameters for a report that
  has no real period range (see the SqlDateTime-overflow gotcha in Lessons Learned).

### 6. Check the existing MetaReport catalog live — before assuming you need new rows

Before writing a seed migration, query `MREPORT`/`MREPORTVIEW`/`MREPORTVSFIELDS` live for the
legacy report's name. Every report in this catalog found an **existing** `MREPORT` row with a
substantial pre-existing field catalog (from 21 fields up to 105) — reusing those field IDs directly
in new `MREPORTVIEWFIELDS` rows meant zero new `MREPORTVSFIELDS` rows were needed for 5 of the 6
report families. Don't assume either "the catalog is empty" or "I'll definitely need new field
rows" — check first. When you genuinely do need new fields (Collection/Payment Projection's 6 new
bidirectional/cash-discount columns), `MREPORTVSFIELDS` has more `NOT NULL` columns than the obvious
four (`REPORTVSFIELDSID`/`REPORTID`/`FIELDNAME`/`FIELDTITLE`) — `SLNO`, `FIELDTYPE`, and
`DISPLAYSLNO` are also required with no defaults; check an existing row's values for the right
`FIELDTYPE` code (1=string, 3=date, 6=decimal, 11=integer/weeks-style, confirmed by sampling real
rows) before guessing.

Write both a SQL Server and a Postgres version of the seed migration (the Postgres one is
mechanically identical except `GETUTCDATE()` → `NOW()`). Pick new IDs above every prior migration's
range to avoid collisions — check live before picking a range, don't just increment blindly.

**6a. Fill in `MREPORTVIEW.Remarks`/`MMENU.MenuDescription`/`MREPORT.Purpose` before shipping — this
is no longer just documentation.** As of the discoverability plan's MVP, `MREPORTVIEW.Remarks` is
selected through to the FE and shown as a tooltip on the view-picker dropdown
(`gbreportaction.component.ts`'s `ReportViewRemarks` binding) — a user choosing between multiple
views for the same report reads this text directly. The same text is also what a future report/menu-
level duplicate-check (modeled on AMP's `ApiRequestDuplicateCheck` — see the discoverability plan)
will score against to catch near-duplicate reports before they're built. Write a genuine, specific
plain-English sentence (what this view is for, when to use it over a sibling view) — not a
placeholder. Track this in the Catalog doc's "Remarks/Description populated" column.

### 7. Build, apply the migration, deploy, and test live

- `dotnet build` the affected `.csproj` — zero errors required.
- Apply the seed migration to GB5DEMO via `sqlcmd` — if a multi-statement migration partially fails
  (e.g. a later statement's foreign-key reference to an earlier failed insert), don't just re-run
  it — check what actually landed and clean up any partial insert first, or the retry will conflict.
- Build the deployable host project (`HRFinanceHost.csproj -c Release`), tar the changed
  DLL+PDB pairs, deploy per the process in the main repo `CLAUDE.md` ("Deployment Logging" section)
  — including the mandatory `DEPLOY_LOG.md` row. This is not optional.
- Test the actual HTTP endpoint with a real authenticated session — not just "the build succeeded."
  Get a real `LoginDTO` (see the digest-auth recipe referenced from the main deployment docs), call
  the endpoint with real criteria, and check the JSON response against what you expect from the live
  data. A `200` response wrapping a `400`-style error body is a real failure — check the response
  body's `GeneralErrors`, don't just check the outer HTTP status code.
- Know the difference between the FastEndpoints request body shape for a plain endpoint vs. a
  `BaseReportEndpoint`-derived one before you spend time debugging a `404`: a `[FromBody]` property
  on the request record binds to the **entire raw request body**, not a nested key matching the C#
  property name — send the `ReportCallingDTO`/`CriteriaDTO` shape at the JSON root, not wrapped under
  a `"ReportCallingDTO": {...}` key.

### 8. Commit precisely, update this documentation, and hold the push if the branch is contested

Stage only the exact files this piece of work touched — never `git add -A` on a shared branch with
concurrent work in flight. If the branch has diverged from `origin`, merge (don't blindly force
push), resolve conflicts by understanding *why* both sides changed the same file rather than
picking a side mechanically, and rebuild before pushing — a "clean" line-based auto-merge can still
silently produce code that doesn't compile (e.g. a merge that keeps your new method but drops the
import it needs, because the import line and the method body live far enough apart that git
resolves them independently). Always rebuild after a real merge, not just after resolving marked
conflicts.

## Building a genuinely new report (no legacy equivalent)

Steps 3 through 8 above are unchanged. What's different:

- **Step 1 (read legacy) doesn't apply** — instead, gather the actual business requirement directly
  (what question is this report answering, for which stakeholder, on what cadence). Write down the
  business rule in the same spirit as legacy's own inline comments used to (a short note explaining
  *why*, next to the code that implements it) — the next developer won't have a legacy source to
  cross-check against, so the reasoning has to live in this codebase's own comments.
- **Step 2 (verify schema) still applies fully** — you still need to confirm live what tables/columns
  actually contain, you just won't have a legacy query to start from.
- **Step 6 (MetaReport catalog) likely needs a brand-new `MREPORT` row**, not just new
  `MREPORTVIEW`/`MREPORTVSFIELDS` rows under an existing one. There's no CRUD for creating `MREPORT`
  rows in this repo yet (see the Admin Config Guide's "open item") — this will need either a new
  one-off migration pattern or, if this becomes common, a proper admin screen. Flag this explicitly
  to the team the first time it comes up rather than working around it silently.
- Add the new report to [AccountReports_Catalog.md](AccountReports_Catalog.md) using the same
  four-section template as every legacy migration — a genuinely new report deserves the same
  End-User/Admin/Developer/Status documentation as a migrated one.

## Lessons learned (keep this section updated)

- **ID sign/sentinel conventions are not `> 0`/`!= 0` by default in this schema.** Real IDs are large
  negative surrogate keys. "Is this filter set" checks must use `!= 0` (0 is the one value
  guaranteed never to be a real ID), except for a small number of fields whose own domain sentinel
  is `-1` (`OUGroupId`, `CostCenterId`'s "NONE" row, `FinanceBookId`'s "no book assigned" case) —
  those must check `!= -1` instead. Getting this backwards silently drops real filter values or
  matches an empty result set. Found and fixed live more than once this wave.
- **`AccountReportFilterBuilder.Build()` unconditionally binds `PeriodFromDate`/`PeriodToDate`** as
  real `DATETIME` Dapper parameters, even if the caller's SQL never references them. A report using
  a single `AsOnDate` instead of a period range (Outstanding, Ageing, Collection Projection) that
  leaves these at `DateTime.MinValue` gets an immediate ADO.NET "SqlDateTime overflow" the moment the
  parameter *value* is assigned — before the SQL text is even considered, and independent of whether
  the text uses the parameter. Fix: after calling `Build()`, immediately overwrite both with a real
  date (`criteria.AsOnDate`) — Dapper's `DynamicParameters.Add` on an existing name replaces the
  value rather than throwing.
- **A `>0`-style amount/ID check needs a live data check, not just a syntax check**, before you trust
  it. `VoucherAmount` is reliably non-negative in this system (confirmed against real data), so
  `FromAmount > 0`-style filters are fine as written — but don't assume every numeric column follows
  the same convention without checking; the ID-sign issue above is exactly this trap in a different
  column family.
- **Check `INFORMATION_SCHEMA.ROUTINES` before assuming a DB function is missing.** `FNPENDINGBILL`
  and `FNAGESLAB` both turned out to already exist, live, identical to legacy — confirmed via
  `OBJECT_DEFINITION`, not assumed from "I don't see it in this repo's own migration files."
- **A DB function's own parameter polarity can be inverted from what its name suggests.** Confirm
  with a live A/B call, not just by reading the function body — `FNPENDINGBILL`'s `@InfoRequired`
  reads as backwards from the name until you actually call it both ways with real data.
- **A "clean" git auto-merge can produce code that doesn't compile.** Git resolves non-overlapping
  line changes independently, even when they're semantically coupled (an import line vs. the method
  body that needs it, hundreds of lines apart). Always rebuild after a real merge — don't assume "no
  conflict markers" means "correct."