Skip to content

fix(#11370): find authored reports by submitter, not freetext contact - #11378

Merged
jkuester merged 2 commits into
medic:10706-dmp-2026-add-api-endpoints-for-advanced-contact-management-operationsfrom
vikrantwiz02:11370-move-submitter-lookup
Sep 4, 2026
Merged

jkuester merged 2 commits into
medic:10706-dmp-2026-add-api-endpoints-for-advanced-contact-management-operationsfrom
vikrantwiz02:11370-move-submitter-lookup

Conversation

@vikrantwiz02

Copy link
Copy Markdown
Contributor

Description

Move stopped finding the reports its contacts authored after the feature branch was synced with master. Four integration tests catch it, all the same way:

set-contact: { places: 1, reports: 0 }   actual
set-contact: { places: 1, reports: 1 }   expected

The cause is #11371. It removed the contact:<id> emission from reports_by_freetext, on the grounds that a contact mapping had nothing to do with freetext search, and made submitter searchable on docs_by_replication_key instead. Move queries exactly that removed emission with exact_match:"contact:<lowercased id>". #11371 landed on master on 26 August and move (#11311) merged into the feature branch on 26 August, so the two crossed and neither side saw the other. cht-conf's equivalent consumer was already migrated in medic/cht-conf#838.

Worth being explicit about the failure mode: nothing errors. A move silently skips every report its contacts authored and leaves them caching a lineage that no longer holds.

The fix, which is also a simplification

Finding those reports previously took two index reads: a freetext query for the report ids, then an _id query on docs_by_replication_key to read the author out of stored fields, because submitter was stored but not searchable. Now that it is searchable, one query by submitter returns both the report and the author its guard has to check, so getReportIdsByCreator and getReportAuthorIds collapse into a single getReportAuthorPairs.

Two details the single query changes:

Blast radius

Move was the only cht-core consumer of the removed emission. The other reports_by_freetext references (shared-libs/cht-datasource/src/local/report.ts, local/libs/nouveau.ts, api/src/services/monitoring.js) are genuine freetext search and unaffected.

Verification

  • 25 unit tests passing in api/tests/mocha/services/move-contact.spec.js, rewritten for the single-query design
  • unit-api 1729 passing, coverage above every threshold
  • ESLint clean

The four integration tests that currently fail on #11275 are the regression test for this; they should pass with this change.

Why this matters now

#11275 cannot go to master while move is broken on the feature branch.

Closes #10706

Code review checklist

  • Readable: Concise, well named, follows the style guide
  • Tested: Unit and/or e2e where appropriate
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.
  • AI disclosure: Please disclose use of AI per the guidelines.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

…ntact

Master's medic#11371 moved the report-author indexing out of
reports_by_freetext and into docs_by_replication_key as a searchable
submitter field, on the grounds that a contact mapping had nothing to do
with freetext search. That landed on master the same day move merged into
the feature branch, so neither side saw the other, and syncing the two
broke move.

Move looked reports up with exact_match:"contact:<lowercased id>" against
reports_by_freetext. That emission no longer exists, so the query returns
nothing and a move silently skips every report its contacts authored,
leaving them caching a lineage that no longer holds. Four integration
tests catch it, all reporting set-contact.reports as 0 where 1 is
expected.

The fix is also a simplification. Finding the reports needed two index
reads: a freetext query for the ids, then an _id query here to read the
author out of stored fields, because submitter was stored but not
searchable. Now that it is searchable, one query by submitter returns both
the report and the author its guard has to check, so getReportIdsByCreator
and getReportAuthorIds collapse into getReportAuthorPairs.

Two details the single query changes. Ids are matched verbatim rather than
lowercased, since this index uses the keyword analyzer, which is what
Josh's own test for medic#11370 does. And because chunked queries can each
return the same report, hits are de-duplicated as they arrive rather than
relying on a Set of ids built by the first pass.

25 unit tests passing, 1729 across unit-api, eslint clean.
@ken-talltree-io

Copy link
Copy Markdown
Contributor

Ran the branch and mutation-tested the changed logic rather than just reading it. Comment only.

Verified: the fix is correct

Checked the two things that would make the new single-query approach unsound:

  • Doc-type safety. submitter in docs_by_replication_key is only ever indexed under the data_record branch, and only if (doc.form && doc.contact) — i.e. only on report docs with an author. There's no way for the new submitter:(ids) query to pick up a contact or place doc, so collapsing the old two-read (freetext id lookup → _id lookup) into one doesn't lose the type filtering the old approach got implicitly from reports_by_freetext only indexing reports.
  • Case-sensitivity rationale. docs_by_replication_key's default_analyzer is keyword, confirming the PR's claim that matching ids verbatim (no .toLowerCase()) is correct for this index — unlike the old reports_by_freetext query, which used whitespace.

Then mutated the changed function directly and reran move-contact.spec.js:

  • Removed the seen.has(hit.id) dedup guard → caught immediately by "does not queue the same report twice when two chunks both match it" (added in this PR).
  • Swapped hit.fields?.submitter for a wrong field name → caught by the exact-value assertion in "refreshes the cached lineage on reports the moved contacts authored" (it checks current_contact_id and contact._id, not just the count).

Both died. Reran the pagination/stuck-bookmark/escaping tests by inspection against the same logic and they check out too — the "stops paging when the bookmark does not advance" test in particular would genuinely hang on a stuck-bookmark mutation, not just fail an assertion, so that guard is real.

One nit, not a defect

The comment on buildSetContactOperations's filter (move-contact.js:162-165) is now stale:

The freetext query matches a lowercased id and the author can change between the two index reads, so a report can come back whose author is not moving at all...

That specific race — staleness between two separate reads — is gone now that id and author come from the same query response. The test right next to the equivalent case (skips a report whose submitter is not in the moved subtree) already has the corrected framing: staleness between the index read and the eventual write, not between two reads. Worth carrying that same wording back into the code comment so it doesn't describe a mechanism that no longer exists.


This review used Claude Code (Anthropic) under my direction: I read the index definitions and diff, directed the mutation testing, and verified both mutations failed and the fix passes as described.

@jkuester
jkuester self-requested a review September 3, 2026 16:29

@jkuester jkuester left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice to see this query shift work well! 🚀

@jkuester
jkuester merged commit 4ab2d28 into medic:10706-dmp-2026-add-api-endpoints-for-advanced-contact-management-operations Sep 4, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants