Skip to content

chore(#10543): add PERSON contact type constant and replace magic strings#10735

Open
gmarav05 wants to merge 32 commits intomedic:masterfrom
gmarav05:fix/add-person-contact-type-constant
Open

chore(#10543): add PERSON contact type constant and replace magic strings#10735
gmarav05 wants to merge 32 commits intomedic:masterfrom
gmarav05:fix/add-person-contact-type-constant

Conversation

@gmarav05
Copy link
Copy Markdown
Contributor

Description

Adds PERSON constant to the CONTACT_TYPES object in the shared constants library, replacing the magic string 'person' with a named constant.

Closes #10543

Code review checklist

  • Readable: Concise, well named, follows the style guide
  • Tested: Unit and/or e2e where appropriate
  • AI disclosure: Used AI assistance for guidance on implementation

License

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

@gmarav05 gmarav05 changed the title fix: add constant for 'person' document type Add constant for 'person' document type Mar 23, 2026
@dianabarsan
Copy link
Copy Markdown
Member

@gmarav05 as with #10747, the occurences of the magic string also need to be updated to use the constant. adding the constant is not sufficient. #

@gmarav05
Copy link
Copy Markdown
Contributor Author

@gmarav05 as with #10747, the occurences of the magic string also need to be updated to use the constant. adding the constant is not sufficient. #

yeah sure i will soon update this. Sorry I was busy in college exams since 2 days. Now, I can finally start contributing again.

@dianabarsan
Copy link
Copy Markdown
Member

@gmarav05 no worries! Your contributions are most welcome!

I did a spree where I took all PRs that didn't have comments or reviews, to make sure nobody is left hanging without a response. It wasn't meant to communicate urgency.

Good luck with exams!!!

@gmarav05 gmarav05 changed the title Add constant for 'person' document type chore(#10540): add PARTNERS document type constant Apr 2, 2026
@gmarav05
Copy link
Copy Markdown
Contributor Author

gmarav05 commented Apr 2, 2026

Hello @sugat009, Should i make changes in all the files like tests/integration/ and tests/e2e/
Screenshot 2026-04-02 at 4 48 26 PM

@sugat009
Copy link
Copy Markdown
Member

sugat009 commented Apr 2, 2026

Hello @sugat009, Should i make changes in all the files like tests/integration/ and tests/e2e/ Screenshot 2026-04-02 at 4 48 26 PM

yes

@gmarav05
Copy link
Copy Markdown
Contributor Author

gmarav05 commented Apr 2, 2026

Hello @sugat009, I tried to replace most of them. However I left few intentionally like these

Screenshot 2026-04-02 at 7 42 47 PM

please let me know if i should change anything or update anything.

@sugat009
Copy link
Copy Markdown
Member

sugat009 commented Apr 6, 2026

@gmarav05 Good call leaving those out. The _id: 'person' instances are arbitrary document identifiers in test fixtures, not contact type references. They should stay as literal strings.

However, please also leave the config/ files (config/default/, config/demo/, config/covid-19/) unchanged. These are reference configuration templates that CHT implementers copy into their own projects, where @medic/constants may not be available. Adding that dependency makes the configs non-portable. The '!person' negation patterns in those same files also can't use the constant cleanly, which creates inconsistency within the same file.

Focus the constant replacement on production source code and their corresponding tests where 'person' is used as a type or contact_type value.

@sugat009
Copy link
Copy Markdown
Member

sugat009 commented Apr 6, 2026

@gmarav05 The PR title appears to be incorrect. It currently says chore(#10540): add PARTNERS document type constant but this PR adds the PERSON constant to CONTACT_TYPES and closes #10543.

Please update it to something like: chore(#10543): add PERSON contact type constant and replace magic strings

@gmarav05 gmarav05 changed the title chore(#10540): add PARTNERS document type constant chore(#10543): add PERSON contact type constant and replace magic strings Apr 6, 2026
@gmarav05
Copy link
Copy Markdown
Contributor Author

gmarav05 commented Apr 6, 2026

Thanks for the detailed feedback @sugat009 , I have reverted all changes in the config/ directories (config/default, config/demo, config/covid-19) to keep them as literal strings and also limited the use of the PERSON constant.

Please let me know if any changes are required or feedback.

@gmarav05 gmarav05 force-pushed the fix/add-person-contact-type-constant branch from 6c26881 to 7fb4bee Compare April 6, 2026 14:16
Signed-off-by: Aravind <gmarav005@gmail.com>
@gmarav05 gmarav05 changed the title chore(#10543): add PERSON contact type constant and replace magic strings chore(#10543): add PERSON contact type constant & replace magic strings Apr 6, 2026
Signed-off-by: Aravind <gmarav005@gmail.com>
@gmarav05
Copy link
Copy Markdown
Contributor Author

gmarav05 commented Apr 7, 2026

Hello @sugat009, Can you please re-run the failing CI checks.

The errors are not related to the PERSON constant changes and the hydration.spec.js failure may be a flaky integration test. When I ran CI tests locally all unit tests are passing.

Signed-off-by: Aravind <gmarav005@gmail.com>
@gmarav05
Copy link
Copy Markdown
Contributor Author

gmarav05 commented Apr 8, 2026

Sorry @sugat009, I was self reviewing few from files changed tab and found that i replaced few test files with type: to appliesToType: CONTACT_TYPES.PERSON. I have now fixed that. 😅

@gmarav05
Copy link
Copy Markdown
Contributor Author

gmarav05 commented Apr 8, 2026

hello @sugat009, Can you please re-trigger this check again, earlier it passed now after update it is failing again.

@gmarav05
Copy link
Copy Markdown
Contributor Author

gmarav05 commented Apr 9, 2026

Hello @sugat009, the 'Compile the app' CI check failed due to a ShellCheck binary download rate limit error. Can you please re-trigger this check again, earlier it passed now after update it is failing again.

@sugat009
Copy link
Copy Markdown
Member

sugat009 commented Apr 9, 2026

Hello @sugat009, the 'Compile the app' CI check failed due to a ShellCheck binary download rate limit error. Can you please re-trigger this check again, earlier it passed now after update it is failing again.

sure

@dianabarsan
Copy link
Copy Markdown
Member

Sorry for the delay @gmarav05 . I will prioritize this review tomorrow.

@gmarav05
Copy link
Copy Markdown
Contributor Author

Sorry for the delay @gmarav05 . I will prioritize this review tomorrow.

No problem @dianabarsan, please review it when you have time.

Copy link
Copy Markdown
Member

@dianabarsan dianabarsan left a comment

Choose a reason for hiding this comment

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

Very very nice! Almost perfect! Just a few comment inline.

Comment thread shared-libs/cht-datasource/test/libs/parameter-validators.spec.ts
Comment thread shared-libs/cht-datasource/test/person.spec.ts Outdated
Comment thread tests/e2e/visual/contacts/contact-user-hierarchy-creation.wdio-spec.js Outdated
Comment thread tests/integration/api/controllers/contact.spec.js Outdated
Comment thread tests/integration/sentinel/transitions/update-notifications.spec.js Outdated
Comment thread tests/integration/sentinel/transitions/update-notifications.spec.js Outdated
Comment thread tests/integration/shared-libs/cht-datasource/contact.spec.js Outdated
Comment thread webapp/tests/karma/ts/services/contact-save.service.spec.ts Outdated
Comment thread webapp/tests/karma/ts/services/rules-engine.service.spec.ts Outdated
…on-type usages

Signed-off-by: Aravind <gmarav005@gmail.com>
@gmarav05
Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review @dianabarsan, I have addressed all the feedback and removed redundant local constants, reverted the doc-id usages back to literal strings and fixed the _id case in rules-engine spec.

Please let me know if any changes are required.

@gmarav05 gmarav05 requested a review from dianabarsan April 18, 2026 04:30
@gmarav05
Copy link
Copy Markdown
Contributor Author

hello @dianabarsan, can you please review this PR when you are free.

Signed-off-by: Aravind <gmarav005@gmail.com>
@gmarav05 gmarav05 force-pushed the fix/add-person-contact-type-constant branch 2 times, most recently from 2f970a7 to ae31004 Compare April 28, 2026 12:20
@gmarav05
Copy link
Copy Markdown
Contributor Author

Hello @dianabarsan, conflicts resolved and PR is up to date. Ready for review.

@gmarav05
Copy link
Copy Markdown
Contributor Author

Hello @dianabarsan, Can you please review this PR when you are free.

Copy link
Copy Markdown
Member

@dianabarsan dianabarsan left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I did a quick go-through and found some misses and inconsistencies. The naming inconsistency that I flagged, please apply it in all files where present, not just the one I added a comment for.

Thanks!

Comment thread admin/tests/unit/services/contact-types.spec.js Outdated
Comment thread shared-libs/contact-types-utils/src/index.js
Comment thread shared-libs/contact-types-utils/test/index.js Outdated
Comment thread shared-libs/contact-types-utils/test/index.js Outdated
Comment thread shared-libs/contacts/test/unit/places.spec.js Outdated
Comment thread shared-libs/contacts/test/unit/places.spec.js Outdated
Comment thread tests/integration/api/controllers/person.spec.js Outdated
Comment thread webapp/tests/karma/ts/services/contact-types.service.spec.ts Outdated
Comment thread webapp/web-components/cht-form/src/app.component.ts Outdated
@gmarav05 gmarav05 force-pushed the fix/add-person-contact-type-constant branch from 132048d to 09f356e Compare April 30, 2026 10:58
@gmarav05 gmarav05 requested a review from dianabarsan April 30, 2026 11:00
@gmarav05
Copy link
Copy Markdown
Contributor Author

gmarav05 commented May 1, 2026

Hello @dianabarsan, I have addressed the feedback and updated everything. Can you please review this PR when you are free.

Also there are others PR as well please take a look when you are free.

Thank you.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add constant for 'person' document type

4 participants