Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(#373): merge-contacts action #647

Merged
merged 44 commits into from
Dec 11, 2024
Merged

Conversation

kennsippell
Copy link
Member

@kennsippell kennsippell commented Nov 23, 2024

Description

#373

This adds new action cht merge-contacts. The change proposes to:

  • Move the code for move-contacts into a library lib/hierarchy-operations with interfaces move and merge
  • Parameterize the move-contacts code changing logic for: input validation, deletes the top-level contact, changes how lineages are updated, and adds report reassignment. All else remains unaltered.

Handles reports and contacts only. Unclear what other doc types I should be worried about.

Things I still need to do. Thinking to do them in their own PRs.

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.

src/fn/merge-contacts.js Outdated Show resolved Hide resolved
@kennsippell kennsippell changed the title New Merge-Contacts Action Merge-Contacts Action Nov 23, 2024
@kennsippell kennsippell changed the title Merge-Contacts Action feat(#373): merge-contacts action Nov 23, 2024
@kennsippell kennsippell requested review from jkuester and removed request for garethbowen November 26, 2024 02:24
@jkuester
Copy link
Contributor

I have started looking at this and reading through the related issues. Unfortunately, I will be OOO the rest of this week, but I plan to give this a proper review when I am back early next week. 👍

@kennsippell
Copy link
Member Author

@jkuester I'm very impressed with the depth of code review you provided. I bet that took a lot of time. Feedback is a gift, so thank you for your time.

@kennsippell kennsippell requested a review from jkuester December 6, 2024 10:21
Copy link
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

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

@kennsippell Thank you for hanging in there with me on this one! IMHO your refactors here have dramatically improved the maintainability of the code base (and as I said before, the new functionality is critical).

(FYI, trying out the conventional commits comment-style here. If you have any feedback good/bad/indifferent, drop it out on that Slack thread!)

I found one issue with the error message if you pass root as the destination, but otherwise just a couple minor suggestions and additional tests cases. Otherwise, this is good to go!

src/fn/merge-contacts.js Outdated Show resolved Hide resolved
src/lib/hierarchy-operations/lineage-constraints.js Outdated Show resolved Hide resolved
src/lib/hierarchy-operations/lineage-constraints.js Outdated Show resolved Hide resolved
src/lib/hierarchy-operations/lineage-constraints.js Outdated Show resolved Hide resolved
test/fn/move-contacts.spec.js Outdated Show resolved Hide resolved
test/lib/hierarchy-operations/jsdocs.spec.js Show resolved Hide resolved
@kennsippell kennsippell merged commit 006554c into main Dec 11, 2024
9 checks passed
@kennsippell kennsippell deleted the 373-merge-contacts-options branch December 11, 2024 21:28
@medic-ci
Copy link
Collaborator

🎉 This PR is included in version 4.2.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.

3 participants