Skip to content

feat(#653): add --merge-primary-contacts flag for merge-contacts action#654

Merged
sugat009 merged 79 commits intomainfrom
653-merge-primaries
Apr 2, 2026
Merged

feat(#653): add --merge-primary-contacts flag for merge-contacts action#654
sugat009 merged 79 commits intomainfrom
653-merge-primaries

Conversation

@kennsippell
Copy link
Copy Markdown
Member

@kennsippell kennsippell commented Dec 9, 2024

Description

When --merge-primary-contacts is present during merge-contacts:

  • If both source and destination have a primary contact, the source primary contacts are deleted and all of their reports reassigned to the destination primary contact
  • If either source or destination don't have a primary contact, then the source primary contact is moved normally with no reports reassigned
  • If the destination has no primary contact, I considered updating the destination doc to use the source primary contact. Currently, nothing ever results in the destination contact being updated so I decided this might be overstepping.

Closes #653

Base branch is #652

Code review items

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or integration tests where appropriate
  • Backwards compatible: Works with existing data and configuration. Any breaking changes documented in the release notes.

License

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

@kennsippell
Copy link
Copy Markdown
Member Author

kennsippell commented Dec 16, 2024

One last review for these related PRs. Thanks @sugat009!

Comment thread src/lib/hierarchy-operations/index.js Outdated
Comment thread src/lib/hierarchy-operations/index.js
@sugat009 sugat009 self-requested a review December 18, 2024 06:20
@freddieptf
Copy link
Copy Markdown
Contributor

@sugat009 @kennsippell what's pending here? Would be great to get this merged as UMT is relying on this branch which is problematic when targeting cht >= v5

@sugat009
Copy link
Copy Markdown
Member

@freddieptf I'm not sure. It was already approved.

@andrablaj
Copy link
Copy Markdown
Member

@sugat009 can we revive this PR and merge if ready I see the CI is not passing - any idea why?

@sugat009
Copy link
Copy Markdown
Member

sugat009 commented Apr 1, 2026

@sugat009 can we revive this PR and merge if ready I see the CI is not passing - any idea why?

sure, let me check.

@sugat009
Copy link
Copy Markdown
Member

sugat009 commented Apr 1, 2026

The issues are all in the test layer. The feature code itself is correct and unchanged:

  1. Missing apiStub configuration: The merge with main brought in the Nouveau/version check code path. The --merge-primary-contacts tests weren't updated to set up apiStub.giveCommonResponse(...), causing API version check retries that exceed the 2000ms Mocha timeout.
  2. Incorrect test fixture expectations: The beforeEach report has patientId: 'district_2' (the place), but the assertions expected primary contact reassignment (person to person). A separate report with patientId: ''district_2_contact' is needed to exercise that logic.
  3. Missing 5.0.0 test counterparts: Every other merge test has both a 4.15.0 (CouchDB view) and 5.0.0 (Nouveau) variant. The --merge-primary-contacts tests need these as well.

@andrablaj
Copy link
Copy Markdown
Member

Thank you for the analysis @sugat009! Would it be a big effort to ship this and make any necessary updates?

@sugat009
Copy link
Copy Markdown
Member

sugat009 commented Apr 2, 2026

Thank you for the analysis @sugat009! Would it be a big effort to ship this and make any necessary updates?

Not really. It's mostly fixing the tests and data structures there. I have the fixed tests ready when debugging this. Should I push to this branch?

@andrablaj
Copy link
Copy Markdown
Member

Should I push to this branch?

Yes, please and let's have this shipped to support @freddieptf and team! Thank you so much!

@sugat009
Copy link
Copy Markdown
Member

sugat009 commented Apr 2, 2026

@freddieptf The CI is green now. Would you mind doing a quick round of testing before I merge this to main?

@freddieptf
Copy link
Copy Markdown
Contributor

@sugat009 sure, let me test it right now and see if everything works

@freddieptf
Copy link
Copy Markdown
Contributor

@sugat009 LGTM

@sugat009
Copy link
Copy Markdown
Member

sugat009 commented Apr 2, 2026

Thanks @freddieptf for testing! Merging to main.

@sugat009 sugat009 merged commit def533d into main Apr 2, 2026
13 of 14 checks passed
@sugat009 sugat009 deleted the 653-merge-primaries branch April 2, 2026 12:20
@medic-ci
Copy link
Copy Markdown
Collaborator

medic-ci commented Apr 2, 2026

🎉 This PR is included in version 6.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merge-contacts action should support mode where primary contacts are also merged

5 participants