Repository navigation
feat(#11151): add byForm qualifier to Report.v1.getUuidsPage - #11327
Conversation
Adds a FormQualifier so report UUIDs can be paged by the form they were recorded with, replacing direct medic-client/reports_by_form page queries. - Qualifier.byForm / isFormQualifier, following the existing byFreetext and byContactType shape - Report.v1.getUuidsPage and getUuids accept FreetextQualifier | FormQualifier; freetext is matched first so existing callers are unchanged - Local adapter queries medic-client/reports_by_form keyed on the form code; remote adapter calls GET /api/v1/report/by-form/:formCode - getUuidsPageByForm / getUuidsByForm on the Report Datasource - The form code is never normalised; it is only URL-encoded as a path segment The caller sweep found no in-scope page queries: every existing caller of medic-client/reports_by_form is an aggregate or single-result sample, so each is preserved with a comment pointing at the aggregate API follow-up.
Review follow-ups on the byForm qualifier: - isFormQualifier used hasField, which only checks the field's type, so a blank form code passed the assert while byForm itself rejected it. A request for /api/v1/report/by-form/%20 therefore returned 200 with an empty page instead of 400. Both now go through hasStringFieldWithValue, so builder and guard agree. Whitespace inside a form code is still preserved -- this rejects blank values, it does not normalise. - Drop assertFormQualifier, which was never used. - Add the getDatasource per-key tests for getUuidsPageByForm and getUuidsByForm that every sibling Datasource method already has.
SonarCloud typescript:S5906 - .to.be.null is also the convention used throughout this package.
|
All checks passed — thank you @ken-talltree-io! This PR is ready for review. |
|
Assigned you on #11151, so AndraBot's check clears and this is out of I did reproduce your three ticket-text problems on
@Hareet, can you confirm these three? The same directory list and the same builder/guard snippet appear in #11147, #11150 and #11152, so if they hold they'd want the same fix across the series before anyone picks those up. On the endpoint shape: I agree with you, a @jkuester, one more for you: |
100% agre with the
Would it be a big lift to just update this PR to support querying by multiple form ids? (Or is there a separate issue for that? Since we have a confirmed use-case where we need to query by multiple ids, I would rather just structure our API to support that. (If someone needs to get reports for just one form, you can always just pass an array with one entry...) |
|
The original issue tickets were created for use with cht-agent. The tickets were generated before the data-access domain was added, which will cover |
Replaces GET /api/v1/report/by-form/:formCode with a ?form= parameter on the existing /api/v1/report/uuid route, and widens it from one form code to many, both per review. The separate path was taken from the ticket, which @jkuester identified as generated rather than specified. Keeping every getUuidsPage qualifier on one route matches how ?freetext= already works and stops the route list growing a segment per qualifier as the series lands. Multiple codes because the confirmed caller wants them: the search lib already queries this view with keys [[a],[b]], so a single-form API would have been the wrong shape from the start. FormQualifier now holds a non-empty array, deduplicated, order preserved. An empty array is rejected rather than returning an empty page, since it can only mean the caller made a mistake. Paging uses one keys query rather than a query per form. CouchDB groups rows by key in the order supplied, so skip stays meaningful across pages only while that order is stable — which is why the qualifier preserves it. A test pages across a form boundary at a page size that splits it. ?form= accepts a comma-separated list or the parameter repeated, the same as ?ids= on /api/v1/report. Freetext still wins when both are given, so existing callers are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both done in
|
|
Checking in — it's been a couple of days since the rework in
No rush if you're just busy — just didn't want either question to sit unanswered by accident. |
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Thank you for this contribution.
This is a good, careful change. It adds a byForms qualifier to the report UUID route. It keeps full backward compatibility. The freetext path stays first, and the old error messages do not change. The build passes and the unit tests pass.
Verified good
- The build passes.
tscreports no errors. - The unit tests pass. I ran the six changed cht-datasource specs (462 pass) and the api controller spec (19 pass).
- The live API is correct for one form, a comma list, a repeated parameter, the freetext-wins rule, and multi-form paging with no gap and no duplicate.
I request one change before merge: add the integration test coverage that the freetext sibling already has (see the blocking comment). Every other comment is non-blocking: one behaviour question, two notes, and several nitpicks.
| // serves form queries would leave a rejection unobserved. The freetext branch still awaits (and | ||
| // so still surfaces) the real error. | ||
| promisedUseNouveau.catch(() => { /* no-op */ }); | ||
| const queryViewByForms = queryDocIdsByKeys(medicDb, 'medic-client/reports_by_form'); |
There was a problem hiding this comment.
issue (blocking): test coverage for byForms is below the freetext sibling.
byForms has no integration test. The freetext sibling has integration tests for the same endpoint at two levels:
- Real CouchDB:
tests/integration/shared-libs/cht-datasource/report.spec.jsrunsgetUuidsPageandgetUuidswithbyFreetext, and it checks paging across pages (lines 198 to 294).byFormshas none of these. - HTTP route:
tests/integration/api/controllers/report.spec.js:231(GET /api/v1/report/uuid) checksfreetextmany times. It checksformzero times.
Only an integration test runs this real medic-client/reports_by_form view. The unit tests use fakes. So they cannot catch a wrong view name, a key in place of keys, or a change to the view.
Two unit helpers also lack the direct tests that their siblings have:
assertFreetextOrFormQualifierhas no test. The siblingassertFreetextQualifierhas one intest/libs/parameter-validators.spec.ts.queryDocIdsByKeyshas no direct test intest/local/libs/doc.spec.ts. Both sibling helpers have one. The report spec only stubs it.
The two form error tests (api/tests/mocha/controllers/report.spec.js:244, :272) check that no query runs, but they do not check the error status or the message. So a change from 400 to 500 would still pass.
Please add integration coverage for byForms to match the freetext sibling, add the two missing unit tests, and assert the status and message in the error tests.
| && hasField(qualifier, { name: 'forms', type: 'object' }) | ||
| && Array.isArray(qualifier.forms) | ||
| && qualifier.forms.length > 0 | ||
| && qualifier.forms.every(form => isString(form) && form.trim().length > 0); |
There was a problem hiding this comment.
issue (non-blocking): the validator trims form codes, but the query does not.
isFormQualifier accepts a code when form.trim().length > 0. byForms then stores the code as given, and the view uses the code as given. So ?form= pregnancy passes validation, but it matches zero reports. I confirmed this on the live API: ?form=pregnancy returns 2 reports, and the padded form returns an empty page with no error. Is the silent empty page the wanted result? If not, please trim the code before you store it, or reject a code that only passes because of trimming.
| // from one page to the next — the view returns rows grouped by key in the order supplied. | ||
| const skip = validateCursor(cursor); | ||
| const getPageFn = getOfflineFreetextQueryPageFn(freetextQualifier); | ||
| const keys = qualifier.forms.map(form => [form]); |
There was a problem hiding this comment.
note: the form branch does not remove duplicate form codes.
The freetext branch removes duplicates, because it calls normalizeFreetextQualifier first. byForms removes duplicates too, so the HTTP route and the public getUuidsByForms method are safe. But a caller that builds a FormQualifier by hand and calls the internal getUuidsPage keeps the duplicates. I confirmed the effect on the view: two identical pregnancy keys return the rows twice, so the page repeats reports. Removing duplicates here, or in assertFreetextOrFormQualifier, closes it.
| const skip = validateCursor(cursor); | ||
| const getPageFn = getOfflineFreetextQueryPageFn(freetextQualifier); | ||
| const keys = qualifier.forms.map(form => [form]); | ||
| const getPageFn = (limit: number, skip: number) => queryViewByForms(keys, limit, skip); |
There was a problem hiding this comment.
note (efficiency): the form page query uses skip on reports_by_form.
Deep paging is therefore O(skip). There is no forms Nouveau index, so the form path has no bookmark cursor like the freetext server path. This matches the offline freetext branch in the same file, so it is acceptable. It matters only for a form with very many reports paged deeply.
| const buildFormQualifier = (form) => { | ||
| const formsArray = (Array.isArray(form) ? form : form.split(',')).filter(Boolean); | ||
| if (!formsArray.length) { | ||
| throw new InvalidArgumentError(`Invalid forms [${JSON.stringify(form)}].`); |
There was a problem hiding this comment.
nitpick: the 400 body for an invalid form has two shapes.
?form= gives Invalid forms [""]. ?form=%20%20 gives Invalid forms [[" "]]. One is a string, the other is an array. This line throws the first, and byForms throws the second. Both are 400.
| * @returns a generator for fetching all matching report identifiers | ||
| * @throws InvalidArgumentError if the form codes are not a non-empty array of non-blank strings | ||
| */ | ||
| getUuidsByForms: (forms: string[]) => AsyncGenerator<string, null>; |
There was a problem hiding this comment.
nitpick: the forms type allows an empty array at compile time.
getUuidsByForms(forms: string[]) and FormQualifier accept an empty array at compile time, but byForms rejects it at run time. The sibling byContactIds([string, ...string[]]) uses a non-empty tuple, so it rejects an empty array at compile time. A non-empty tuple would move the check earlier.
| /** | ||
| * A qualifier that identifies entities based on the codes of the forms used to record them. | ||
| */ | ||
| export type FormQualifier = Readonly<{ forms: string[] }>; |
There was a problem hiding this comment.
nitpick: the new names mix singular and plural.
The type FormQualifier and the guard isFormQualifier are singular. The field forms, the builder byForms, and the methods getUuidsByForms are plural. The sibling qualifiers stay plural throughout. The PR and issue title also say "byForm", but the shipped symbol is byForms.
|
|
||
| // Aggregate (grouped) query used to enumerate the distinct forms in use. Deliberately left | ||
| // on db.query by the cht-datasource view->qualifier migration: Qualifier.byForms only covers | ||
| // the page query — see the aggregate API follow-up ticket. |
There was a problem hiding this comment.
nitpick: the comments point to an unlinked ticket.
This comment, and the ones at report-mapper.js:77 and impact.js:6, name "the aggregate API follow-up ticket" and Report.v1.countByForm, but they give no issue number or link. Please add the issue number.
| /** | ||
| * Builds a qualifier for finding entities recorded with any of the given forms. | ||
| * @param forms the form codes to search with (e.g. `['pregnancy']`). Each is matched verbatim against | ||
| * the document's `form` field — they are not normalised. Duplicates are removed. |
There was a problem hiding this comment.
nitpick: the new comments use em-dashes and British spelling.
This comment and other new comments use em-dashes and the word "normalise". The cht-datasource library uses "normalize" and has no em-dashes in its source. Please match the library style.
| * - in: query | ||
| * name: form | ||
| * required: false | ||
| * x-since: 5.3.0 |
There was a problem hiding this comment.
nitpick: x-since on the form parameter is at the parameter level.
Every other x-since in the controllers is at the operation level. The placement may be intentional, because the parameter arrived in 5.3.0 while the operation is from 4.18.0. If so, please keep it. If a tool reads only operation-level notes, it may miss this one.
… byForms Addresses @sugat009's review of the multi-form qualifier PR. Blocking: adds integration test coverage for byForms matching the freetext sibling at both the shared-libs level (real CouchDB keys query, paging across pages, single form) and the HTTP route level (GET /api/v1/report/uuid?form=), plus the two missing unit tests (assertFreetextOrFormsQualifier, queryDocIdsByKeys) and status/message assertions on the two error tests that only checked "no query ran". Non-blocking: a form code with leading/trailing whitespace now fails validation instead of silently matching nothing, since the value is compared verbatim rather than trimmed; and duplicate form codes are now dropped at the query point in local/report.ts itself, not just by byForms(), so a hand-built qualifier that bypasses the builder can't double-count. Nitpicks: renamed FormQualifier/isFormQualifier to FormsQualifier/isFormsQualifier to match the IdsQualifier/ ContactIdsQualifier plural convention; forms is now a non-empty tuple like contactIds; the two invalid-form 400 bodies use one consistent shape; unlinked ticket references now point at medic#11328; and new comments match the library's existing style (no em-dashes, American spelling). AI disclosure: written with Claude Code (Claude Sonnet 5), acting on Ken's request to address this review round. It read the review comments, made each fix, and verified the two substantive behavioral changes (whitespace rejection, dedupe-at-query-point) by reverting each and confirming the new tests fail without it. Ken reviewed the diff before it was pushed and is accountable for it.
|
Thanks for the careful review — all fixed in Blocking: integration coverageAdded at both levels the freetext sibling has it:
Also added the two missing unit tests: One honest caveat: I don't have a live CHT/CouchDB instance locally, so the two integration files are unverified — checked by hand against the fixture data and the Non-blocking
Nitpicks
AI disclosure: this round was written with Claude Code (Claude Sonnet 5), acting on my request to work through your review comment-by-comment. It read the review, made each fix, and — for the two behavioral changes — reverted each one locally and confirmed the relevant new test actually fails without it before restoring the fix, rather than trusting that a test passing once meant it was doing its job. I reviewed the diff before it went up and I'm accountable for it. |
The quality gate failed on new_violations=7 (threshold 0). All seven are in the two integration specs added for byForms; none implicate the qualifier itself. 3 x javascript:S8785 - drop `async` from the `describe` callbacks at api/controllers/report.spec.js:419 and cht-datasource/report.spec.js:304 and :359. Verified currently harmless before removing: none of the three had an `await` in the callback's own execution path, so every `it()` was still registering. It was a latent hazard rather than a live one - if a top-level `await` were added later, the `it()`s after it would vanish silently and the suite would pass having run fewer tests than it contains. 4 x javascript:S5906 - `expect(page.data).to.have.lengthOf(n)` in place of the generic `expect(page.data.length).to.be.equal(n)`, at api/controllers/report.spec.js:473-474 and cht-datasource/report.spec.js:333-334. Verified locally only: eslint clean on both files, and an AST comparison against the previous head shows the registration structure unchanged (40 and 36 describe/it registrations before and after). The integration suites themselves need k3d and were not run locally.
|
Two things, neither of them blocking — one to save you a detour, one that's your call. The two red integration jobs aren't this PRWorth saying before the X costs anyone time:
The same test failed identically on the previous head ( @witash has since filed #11359 for it and opened #11360, using this branch's run as the example — Offer: sweep the
|
| File | Pre-existing async describes on master |
|---|---|
tests/integration/api/controllers/report.spec.js |
:135, :231 |
tests/integration/shared-libs/cht-datasource/report.spec.js |
:146, :160, :198, :288 |
So in report.spec.js my new GET /api/v1/report/uuid with form param block sits directly under an
async one, and in the datasource spec getUuidsPage byForms sits directly under getUuids. Sonar
only gates new code, so the old six don't trip it.
Static check, not a CI claim: none of the six has an await in the describe callback's own
execution path — every await is inside a nested it() or hook body — so dropping async is a
no-op for registration, same as it was for the three I changed.
Happy to do any of: sweep all six here, open a separate PR for them, or leave them alone. I'd
lean toward a separate PR so this one stays scoped to #11151, but it's your repo's convention to
apply, not mine. Just say which.
@sugat009 — otherwise this is ready for another look whenever it suits.
sugat009
left a comment
sugat009
left a comment
There was a problem hiding this comment.
Re-review: approving. All points addressed.
Thank you for the thorough fixes. I re-checked each finding against the new head and re-ran the build, tests, and the live API.
- The blocking gap is closed:
byFormsnow has integration coverage at both levels, the two missing unit tests are added, and the error tests assert the status and message. - The behaviour fixes are correct: padded codes are rejected, duplicate forms no longer repeat rows, and both invalid-form bodies match.
- The nitpicks are done, and the follow-up is tracked as
#11328.
Build clean, 1200 and 19 tests pass, CI green. Nice work.
|
Thanks so much for your contribution @ken-talltree-io ! I'm happy to share that your code is shipping in CHT 5.3 and your name will be proudly listed as contributor in the release notes. For future contributions (which we hope there are more!), please take the time to set up and run the CHT locally before submitting a pull request. Being able to run your changes locally to test and verify it works is a critical part of being a developer for the CHT. If you get stuck with setup, please post to the forum - we'd love to get you unstuck! |
5 participants
Description
Adds a
FormQualifierso report UUIDs can be paged by the forms they were recorded with, replacing directmedic-client/reports_by_formpage queries.Qualifier.byForms/isFormQualifier, following the shape ofbyIdsReport.v1.getUuidsPageandgetUuidsacceptFreetextQualifier | FormQualifierkeysquery againstmedic-client/reports_by_form; remote adapter sends?form=to the existingGET /api/v1/report/uuidgetUuidsPageByForms/getUuidsByFormson theReportDatasourcebyFormsis exposed underQualifierautomatically —index.tsalready doesexport * as Qualifier from './qualifier'— so requirement 6 needed no change.Revised after review (#11327 review). Two changes from the first revision, both @jkuester's: the endpoint moved from a
by-form/:formCodepath to a?form=query param on the existing route, and the qualifier widened from one form code to many.Revised again after @sugat009's review: added the integration coverage this was missing (see the Tests section), fixed a validation gap where a form code with surrounding whitespace silently matched nothing instead of 400ing, closed a dedupe gap for hand-built qualifiers that bypass
byForms(), and applied the naming/type/comment nitpicks —FormQualifier/isFormQualifierare nowFormsQualifier/isFormsQualifierto matchIdsQualifier,formsis a non-empty tuple likecontactIds, and the two invalid-form 400 bodies now share one shape. Full breakdown in this comment.The caller sweep found nothing in scope
The ticket asks for page-query callers to be swapped. There are none — every existing caller of
medic-client/reports_by_formis an aggregate or a single-result sample, all explicitly out of scope:api/src/services/impact.jsreduce: true, group: trueapi/src/services/export/report-mapper.js:59group: trueapi/src/services/export/report-mapper.js:72key: [form], limit: 1, include_docs: trueEach is preserved with a comment pointing at the aggregate API follow-up (#11328), per the ticket.
Three notes on the audit itself — these look templated across the
Datasource View Migrationseries rather than specific to this ticket, so the details are in a comment on #11151:... webapp/src admin/src shared-libs) it returns zero matches — every caller is inapi/src, which isn't in the path. It reports "clean" rather than erroring, so following the ticket literally would mean concluding there are no callers at all.query(...)-shaped grep can find:shared-libs/search/src/generate-search-requests.js:66goes throughgetRequestForMultidropdown(...). Left untouched — see below, since it drove the multi-form design.Design decisions worth a look
One deliberate deviation from the ticket text. The
FormQualifiersnippet in the requirements pairs its builder (!isString(form) || form.length === 0) with ahasFieldtype guard. Those two disagree:hasFieldonly checks the field's type, so the guard accepts{ form: '' }while the builder throws on''— andgetUuidsPage's assert runs the guard, not the builder. As specified, a blank code returned200 {"data":[],"cursor":null}instead of400. The guard and the builder now share a single definition of validity, so a blank entry is rejected wherever it enters, including one blank among otherwise valid codes. Internal whitespace is still preserved verbatim, so the "do not normalise" constraint holds.Endpoint shape — resolved. The ticket specified
GET /api/v1/report/by-form/:formCode, which @jkuester identified as generated rather than intended.?form=now sits on the existing/api/v1/report/uuidbeside?freetext=, so everygetUuidsPagequalifier stays on one route and the route list doesn't grow a segment per qualifier as the series lands. It accepts?form=a,bor the parameter repeated, the same as?ids=on/api/v1/report.Multiple form codes, and why the single-form version was the wrong shape.
generate-search-requests.js:66already queries this view askeys: [['P'], ['R']], reduce: false— multi-key is what the codebase does withreports_by_formtoday. SoFormQualifierholds an array, deduplicated with order preserved, and the local adapter issues onekeysquery rather than one per form.Order preservation is load-bearing rather than cosmetic: CouchDB groups rows by key in the order supplied, so
skipis only meaningful across pages while that order is stable. A test pages across a form boundary at a page size that splits it, which is where a per-key rather than whole-result-set skip would surface.An empty array is rejected rather than returning an empty page — unlike
IdsQualifier, which accepts one — because it can only mean the caller made a mistake and an empty page would hide it.That caller still can't consume the API, and I don't think it can.
formRequestbuilds a view-request descriptor thatsearch.jsexecutes and then intersects with the date / validity / place filters (search.js:41,53). That term needs the complete unpaged id set to intersect against, whereasgetUuidsPagereturns a cursor-paged slice. Migrating it needs either an unpaged variant or the whole filter composition moving behind the datasource — its own ticket, and it touches the offline search path.Naming. The wire parameter is
?form=; the qualifier field isforms: string[], matchingids: string[]onIdsQualifier. Each layer is consistent with its neighbours and they differ from each other. Easy to change either side.Qualifier precedence. Freetext is matched first in both adapters, so a qualifier satisfying both shapes behaves exactly as before. Covered by a test in each adapter.
Error messages preserved. Widening the assert would have changed the
InvalidArgumentErrormessage for existing invalid-freetext calls, which the API surfaces in 400 bodies. Only form-shaped input gets the new message; everything else keeps the originalInvalid freetext [...]text verbatim.Unawaited promise. The form branch returns without awaiting
promisedUseNouveau, so a binding that only ever served form queries could leave a rejection unobserved. Added an explicit no-op.catch(); the freetext branch still awaits and surfaces the real error.Tests
test/qualifier.spec.ts—byFormsdedupes, preserves order, does not normalise, and rejects empty arrays, bare strings, blanks and non-stringstest/local/report.spec.ts— dispatch hits the right view with onekeyscall; freetext machinery untouchedtest/remote/report.spec.ts—?form=on the shared route, comma joining, cursor omission, freetext precedencetest/local/report.spec.ts— behavioural pagination against a key-aware fake view that does not stub page assembly, walked across cursor pages at several limits asserting real UUIDs and cursor values, including across a form boundary and with a form that has no reportstest/report.spec.ts—getDatasourceper-key tests for both new methods, matching the freetext trioapi/tests/mocha/controllers/report.spec.js— comma-separated and repeated?form=, single code, empty entries skipped, empty value rejected without querying (now asserting status/message, not just that no query ran), freetext precedence, and the unchanged missing-freetext pathtest/libs/parameter-validators.spec.ts—assertFreetextOrFormsQualifier, mirroringassertFreetextQualifier's coveragetest/local/libs/doc.spec.ts—queryDocIdsByKeysdirectly, mirroringqueryDocIdsByKey'stest/local/report.spec.ts— a hand-built qualifier that bypassesbyForms()still gets its duplicate form codes dropped before the view querytests/integration/shared-libs/cht-datasource/report.spec.jsandtests/integration/api/controllers/report.spec.js— real-CouchDB and HTTP-route coverage forbyForms/?form=, matching the freetext blocks including paging across pages; the HTTP-level invalid-cursor test asserts the 400 the form path throws (it validates synchronously, unlike freetext's 500-from-Nouveau)Every new behaviour was mutation-tested rather than eyeballed: querying only the first key fails 3 tests, dropping the dedupe fails 1, accepting an empty array fails 2, and sending only the first form remotely fails 1.
That run also caught one of my own tests verifying nothing.
queries a duplicated form only oncepassed with the dedupe removed and with only the first key queried, becausefetchAndFilterIdskeeps its ownidSet(local/libs/doc.ts:184-199) and drops repeat uuids regardless of what the qualifier does. It was green for a reason unrelated to the change, so I deleted it rather than reword it; the qualifier spec pins the actual dedupe.Verification
cht-datasource: 1130 passing, 0 failingunit-shared-lib(all shared libs): every lib passes exceptshared-libs/outbound, which misses its 95% nyc threshold (94.85 / 89.55 / 94.89). Byte-identical onmaster, andoutboundhas no dependency oncht-datasource.cht-datasourcemeets its own coverage threshold with the new code.unit-api: 1648 passing, 4 failing. Ran again at the merge-base (98162aac3), rebuildingcht-datasourcethere so the comparison is fair: 1641 passing, 4 failing, and the failing set is identical — a migration case,Monitoring service, and twoReplication Health servicecases. So this change adds 7 passing tests and no failures.shared-libs/search: 79 passing, untouched by this change but it shares the view.Both acceptance-criteria scripts are green modulo pre-existing breakage rather than exiting 0 outright — flagging the distinction rather than claiming a clean run.
Update: integration coverage for
?form=is now added at bothtests/integration/shared-libs/cht-datasource/report.spec.jsandtests/integration/api/controllers/report.spec.js, mirroring the freetext blocks. I still don't have a live CHT instance to run them against, so they're unverified — checked by hand against the fixture data and the realkeys-query semantics the production code already relies on, but not run. Flagging that plainly rather than claiming a green run.One environment note that cost me time, in case it helps anyone else:
unit-apionly loads ifapi/tests/mocha/services/sso-login.spec.jsis passed first, exactly as thepackage.jsonscript does. Globbingapi/tests/mocha/**/*.json its own dies inopenid-clientwithCannot use import statement outside a modulebefore a single test runs.Closes #11151
Code review checklist
@openapiJSDoc block, matching the sibling report routes. Happy to open a cht-docs PR if more is wanted.InvalidArgumentErrormessages are preserved verbatim.AI Disclosure: Written with Claude Code (Claude Opus) — exploring the codebase, drafting the implementation and tests, running the suites and the mutations above. I reviewed the result at each stage and I'm accountable for it.
That review is where most of the corrections came from. In the first revision it caught a validation gap where the builder and type guard disagreed on blank form codes, an unused
assertFormQualifierexport, and missinggetDatasourceper-key tests. In this revision it caught the vacuous test described in the Tests section. Worth stating plainly given the ticket: theby-form/:formCodeendpoint in the original revision came from following the ticket text literally, and the ticket text turned out to be generated — I flagged the shape as inconsistent at the time but implemented it as written rather than pushing on it, which is the thing I'd do differently.This latest round (addressing @sugat009's review, Claude Sonnet 5) worked through the review comment-by-comment: the integration tests, the two missing unit tests, the two behavioral fixes, and the naming/type/comment nitpicks. For the two behavioral fixes it reverted each one locally and confirmed the corresponding new test actually fails without it, rather than trusting a passing test on its own. I reviewed the diff before it went up and I'm accountable for it; the one thing I couldn't verify myself is the two new integration test files, since I don't have a live CHT instance — said so plainly above rather than claiming a run I don't have.
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.