feat(#10706): add move contact hierarchy - #11311
Open
vikrantwiz02 wants to merge 9 commits into
Open
Conversation
Ports lineage-constraints: no cycles, only configured parent types, and no move that strands a place's primary contact. Two differences from cht-conf. It reads contact types through @medic/contact-types-utils rather than parsing the settings doc, since the server already has the parsed configuration. And it drops the check rejecting two sources from the same lineage, which exists because the CLI moves a list of contacts in one command; a move here has a single source. Violations are raised as BadRequestError so the caller gets the reason, and anything else, such as a database failure part way through validation, propagates instead of being reported as an invalid move.
Points a contact at a new parent lineage. Each operation carries the document id, the parent it is expected to still have, and the new minified lineage to write, so the handler computes nothing. An operation whose document has changed since the API gathered it is failed rather than clobbering the newer edit, matching set-contact. bulkDocs resolves even when individual writes fail, so every result is checked, and a failure does not stop the rest of the batch.
Gathers everything a move touches and queues it as a bulk operation. The subtree comes from contacts_by_depth, the reports the moved contacts authored come from the nouveau reports_by_freetext index queried on exact_match:"contact:<lowercased id>", and the surviving places whose primary contact moved come from contacts_by_primary_contact. Two actions are queued: set-parent for the contacts, and the existing set-contact for the reports and places, since in both of those the value being refreshed lives in a property called contact. Ids are gathered first and documents are then read a page at a time and turned straight into operations, so only one page is held at once. A district move can touch tens of thousands of documents and the request is the half of this that has to stay cheap. The nouveau results are paged with the bookmark rather than capped, so a prolific author is not silently truncated, and paging stops if the bookmark stops advancing. Ids are escaped before they are embedded in the quoted query phrase.
POST /api/v1/person/{id}/move and POST /api/v1/place/{id}/move, following the
per-type shape delete shipped with rather than a single /contact endpoint, so
a place cannot be moved through the person endpoint or the reverse.
Both share one handler and take a parent_id body property, where "root" moves
the contact to the top level, and an optional dry_run that returns the same
summary without queuing anything.
Gated on a new can_move_contact_hierarchy permission, which ships assigned to
no role. can_edit is held by almost every user and is a webapp permission, not
appropriate for guarding a server operation that rewrites a subtree.
Covers the permission gate, the wrong contact type, a missing parent_id, a circular move, a disallowed parent type, a move to the parent the contact already has, and a dry run that writes nothing. The end to end cases assert that the moved contact sits under the destination, that a descendant keeps its own parent with only the chain above it rewritten, and that the lineage cached on the reports the moved contacts authored follows while still naming the same author.
Splits the root-move check out of assertParentTypeIsAllowed and the paging loop out of getReportIdsByCreator, so neither exceeds the cognitive complexity threshold. Collapses the phrase escaping into a single replaceAll, and uses the dedicated chai matchers for undefined and null assertions.
…anagement-operations' into 10706-move
personFactory defaults every person to the patient_id 'test_woman_1', and a report records its subject's shortcode. contacts_by_depth emits that shortcode for the deleted subtree, so the delete tests matched the move fixtures' reports by subject and counted one report too many. Set an explicit shortcode on each move fixture person, as the person delete fixture already does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds the move half of the Advanced Contact Management API:
POST /api/v1/person/{id}/moveandPOST /api/v1/place/{id}/move, moving a contact and its whole subtree under a new parent.This is the second consumer of the bulk-operations framework that landed with delete in #11236, and it needs no changes to that framework. The design was reviewed on the Move tab of the technical design doc and follows what we settled there.
How it works. The request validates the move, gathers the documents it touches, and queues a bulk operation, returning a job id immediately. Sentinel applies the changes in batches, and the caller polls
GET /api/v1/bulk-operations/{id}.Two actions are queued, named for the field each writes:
set-parent(new)doc.parentset-contact(existing)doc.contactset-contactis reused unchanged because in both cases the value being refreshed lives in a property calledcontact: on a report it is the author reference, on a place it is the primary contact.Every operation is self-contained, carrying the document id, the value it expects to find, and the new value to write, so the handlers compute nothing. An operation whose document changed since it was gathered fails rather than clobbering the newer edit, and every
bulkDocsresult is checked.Validation. Ported from cht-conf's
lineage-constraints: no cycles, only configured parent types, no move that strands a place's primary contact, the source's own primary contact must be a person, and a move to the parent the contact already has is rejected rather than writing identical values back. Violations return a400with the reason; anything else, such as a database failure during validation, propagates as a500rather than being reported as an invalid move.Finding the reports. A move changes where the author sits, so the affected reports are the ones the moved contacts wrote, not the ones about them. These come from the nouveau
reports_by_freetextindex queried onexact_match:"contact:<lowercased id>". Results are paged with the bookmark rather than capped, so a prolific author is not silently truncated.Scale. Ids are gathered first and documents are then read a page at a time and turned straight into operations, so only one page is held in memory at once. A district move can touch tens of thousands of documents and the request is the half of this that has to stay cheap.
Permission. A new
can_move_contact_hierarchy, shipping assigned to no role.can_editis held by almost every user and is a webapp permission, not appropriate for guarding a server operation that rewrites a subtree.Endpoint shape. Per-type rather than a single
/contact/{uuid}endpoint, following what delete shipped with, so a place cannot be moved through the person endpoint or the reverse.One thing worth flagging for review: a move changes who can replicate the subtree. Moving a clinic between districts means the receiving district's offline users begin syncing those contacts and their reports, and the previous district's stop. That is what a move means, but it is an access change as much as a data change, which is part of why the permission is separate and admin-assigned.
The first commit lands the lineage manipulation helpers, which were written earlier but never merged while we focused on delete.
#10706
Code review checklist
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.