feat(#10036): add `PersonQualifier` and related functions #10043

apoorvapendse · 2025-06-11T11:51:22Z

Description

WIP to integrate PersonQualifier to eventually add support for creating person documents.

Related to #10036.

Code review checklist

UI/UX backwards compatible: Test it works for the new design (enabled by default). And test it works in the old design, enable can_view_old_navigation permission to see the old design. Test it has appropriate design for RTL languages.
Readable: Concise, well named, follows the style guide, documented if necessary.
Documented: Configuration and user documentation on cht-docs
Tested: Unit and/or e2e where appropriate
Internationalised: All user facing text
Backwards compatible: Works with existing data and configuration or includes a migration. 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.

Signed-off-by: apoorvapendse <[email protected]>

sugat009 · 2025-06-11T14:02:53Z

shared-libs/cht-datasource/src/qualifier.ts

+
+  if (data.type === 'contact' && !hasField(data, { name: 'contact_type', type: 'string' })) {
+    return false;
+  } else if (!(data.type === 'person')) {


Suggested change

} else if (!(data.type === 'person')) {

} else if (data.type !== 'person') {

sugat009 · 2025-06-11T14:08:16Z

shared-libs/cht-datasource/src/qualifier.ts

+  if (!isRecord(data)) {
+    throw new InvalidArgumentError('Invalid "data": expected an object.');
+  }
+  const qualifier = { ...data };
+  if ('reported_date' in qualifier && !isValidReportedDate(qualifier.reported_date)) {
+    throw new InvalidArgumentError(
+      // eslint-disable-next-line max-len
+      `Invalid reported_date. Expected format to be 'YYYY-MM-DDTHH:mm:ssZ', 'YYYY-MM-DDTHH:mm:ss.SSSZ', or a Unix epoch.`
+    );
+  }
+  if (!isContactQualifier(qualifier) || !hasField(data, { name: 'parent', type: 'object' })) {
+    throw new InvalidArgumentError(`Missing or empty required fields [${JSON.stringify(data)}].`);
+  }


question: can the byContactQualifier function be reused here?

Same problem mentioned here.

sugat009 · 2025-06-11T14:12:11Z

shared-libs/cht-datasource/src/qualifier.ts

+  if (!isContactQualifier(data)) {
+    return false;
+  }


issue: this should be at the top of the file because the first two checks are included in that function

That causes TypeScript to think that data is a ContactQualifer, which results in a lot of assertions for further checks.
Is there some way to deal with that?

yes, there a few ways to deal with this.

reset type

export const isPersonQualifier = (data: unknown): data is PersonQualifier => { if (!isContactQualifier(data)) { return false; } // Reset data back to unknown for further type checking const unknownData = data as unknown; // Continue with your additional checks on unknownData // ... };

Double refactor the code. Put the common code from isContactQualifier and isPersonQualifier into another function.

const checkContactQualifierFields = (data: unknown): data is Record<string, unknown> => { return isRecord(data) && hasFields(data, [ {name: 'type', type: 'string', ensureTruthyValue: true}, {name: 'name', type: 'string', ensureTruthyValue: true} ]) && (!('reported_date' in data) || isValidReportedDate(data.reported_date)); }; export const isContactQualifier = (qualifier: unknown): qualifier is ContactQualifier => { return checkContactQualifierFields(qualifier); }; export const isPersonQualifier = (data: unknown): data is PersonQualifier => { if (!checkContactQualifierFields(data)) { return false; } // data is still treated as Record<string, unknown> here, not ContactQualifier // Add your PersonQualifier-specific checks // ... return true; };

Try out the 2nd option as I think it will get the ball rolling for the other comments.

sugat009 · 2025-06-11T14:20:31Z

shared-libs/cht-datasource/src/qualifier.ts

+  if (data.type === 'contact' && !hasField(data, { name: 'contact_type', type: 'string' })) {
+    throw new InvalidArgumentError(`Missing or empty required fields [${JSON.stringify(data)}].`);
+  } else if (!(data.type === 'person')) {
+    throw new InvalidArgumentError('Expected `type` to be `person`.');
+  }


issue: duplicate code with lines 358-362, refactor into a function

Same argument as the one I made here, we throw different errors for different stuff and booleans in case of the is functions.

I think we did something similar for the ContactQualifier, where we redundantly copied the checks to throw errors/false returns based on whether it was a is or by function.

sugat009 · 2025-06-11T14:22:41Z

shared-libs/cht-datasource/src/qualifier.ts

+  // Ensure parent lineage doesn't have any additional properties other than `_id` and `parent`.
+  let parent = data.parent;
+  while (parent) {
+    if (Object.keys(parent).length > 2) {
+      // This means that the parent certainly has extra fields and is not minfied/de-hydrated as per
+      // our liking as `isNormalized` check ensures that it does have two keys `_id` and `parent`.
+      throw new InvalidArgumentError(`Additional fields found in the parent lineage [${JSON.stringify(data)}].`);
+    }
+    parent = data.parent;
+  }


issue: duplicate code with lines 344-352, refactor into a function

The reason I didn't refactor into a function is because this check depends on first checking isNormalizedParent which is done in the previous steps of both the functions, so it cannot be used independently.
also including isNormalizedParent call in the new extracted function wouldn't be convenient because of the fact that it throws a different error and the same throwing error or returning false dilemma we have been facing for the qualifiers.

sugat009 · 2025-06-11T14:34:51Z

shared-libs/cht-datasource/src/qualifier.ts

+  let parent = data.parent;
+  while (parent) {
+    if (Object.keys(parent).length > 2) {
+      // This means that the parent certainly has extra fields and is not minfied/de-hydrated as per
+      // our liking as `isNormalized` check ensures that it does have two keys `_id` and `parent`.
+      return false;
+    }
+    parent = data.parent;
+  }


question: this is more of a design decision. Personally, I think it's cleaner to have just a immediate parent as an field and not allow more nests. We could add the grandparents and so on from the server side. There's no benefit of the current way other than not having to query the database for the grandparents.

@jkuester What are you thoughts on this?

(Cross posting from the call with hopefully better clarity)
I think going from the current version to the version @sugat009 mentioned would be easier for the user.
Transitioning from a stricter to more lenient data requirement would introduce complexity and we can make it broader after doing the "MVP" version.
The ideal version would probably be the one where someone can just pass the immediate parent_id or the entire lineage(normalized or hydrated) and we just ignore everything apart from the immediate parent to do our own lineage checks up the hierarchy.

The current assumption with createPerson and PersonQualifier methods is that the user passed in the correct lineage. We do throw errors if it is bloated or misses the required _id, parent fields.
I'd like to keep it this way and maybe come back to building the more forgiving and robust version later down the line.
Would love to hear your thoughts @jkuester, @sugat009.

Signed-off-by: apoorvapendse <[email protected]>

sugat009 · 2025-06-12T06:25:52Z

shared-libs/cht-datasource/src/qualifier.ts

+  byContactQualifierNonAssertive(data);
+  return data as ContactQualifier;


question: shouldn't this be returning the function instead like this?

Suggested change

byContactQualifierNonAssertive(data);

return data as ContactQualifier;

return byContactQualifierNonAssertive(data) as ContactQualifier;

Yeah makes sense.

sugat009 · 2025-06-12T06:34:59Z

shared-libs/cht-datasource/src/qualifier.ts

+  if (!hasValidContactType(data) && !hasValidLegacyContactType(data, 'person')) {
+    return false;
+  }
+
+  return true;


Suggested change

if (!hasValidContactType(data) && !hasValidLegacyContactType(data, 'person')) {

return false;

}

return true;

return hasValidContactType(data) || hasValidLegacyContactType(data, 'person');

equivalent?

sugat009 · 2025-06-12T06:36:06Z

shared-libs/cht-datasource/src/qualifier.ts

+  if (data.type === 'contact' && hasField(data, { name: 'contact_type', type: 'string' })) {
+    return true;
+  }
+  return false;


Suggested change

if (data.type === 'contact' && hasField(data, { name: 'contact_type', type: 'string' })) {

return true;

}

return false;

return data.type === 'contact' && hasField(data, { name: 'contact_type', type: 'string' })

Signed-off-by: apoorvapendse <[email protected]>

shared-libs/cht-datasource/src/qualifier.ts

sugat009 · 2025-06-12T15:23:15Z

shared-libs/cht-datasource/src/qualifier.ts

+      {name: 'type', type: 'string', ensureTruthyValue: true}, 
+      {name: 'name', type: 'string', ensureTruthyValue: true}
+    ]) &&
+    (!('reported_date' in data) || isValidReportedDate(data.reported_date));


question: if the reported_date does not exist, shouldn't it be added to it? Or are you planning on adding it elsewhere?

Makes sense, added in the latest commit.

Signed-off-by: apoorvapendse <[email protected]>

sugat009

Almost there. Some final touches.

sugat009 · 2025-06-13T04:36:34Z

shared-libs/cht-datasource/src/qualifier.ts

+  insertReportedDateIfMissing(data);
+
  const qualifier = {...data};
-  if ('reported_date' in qualifier && !isValidReportedDate(qualifier.reported_date)) {
+  if (!isValidReportedDate(qualifier.reported_date)) {


issue: the addition of date should be done after line 242, because the whole point of deep copying the data object into qualifier is to not mutate the passed in data object

sugat009 · 2025-06-13T04:41:19Z

shared-libs/cht-datasource/test/qualifier.spec.ts

+        .throw(`Additional fields found in the parent lineage [${JSON.stringify(data)}].`);
+    });
+
+    it('builds qualifier for valid objects', () => {


issue: can you add a test for when a reported_date already exists within the passed body

Yes, added in the last commit.

Resolves: medic#10043 (comment)

feat(medic#10036): add PersonQualifier and related functions

215d1b2

Signed-off-by: apoorvapendse <[email protected]>

sugat009 self-requested a review June 11, 2025 13:39

sugat009 requested changes Jun 11, 2025

View reviewed changes

apoorvapendse force-pushed the create-person-local branch from 945afa9 to da56aa3 Compare June 11, 2025 15:27

apoorvapendse requested a review from sugat009 June 11, 2025 15:56

refactor isContactQualifier to use common checker

cbc4aaa

Signed-off-by: apoorvapendse <[email protected]>

apoorvapendse force-pushed the create-person-local branch from da56aa3 to 7c8a9b8 Compare June 12, 2025 05:20

refactor to reuse byContactQualifierNonAssertive

07a3e92

Signed-off-by: apoorvapendse <[email protected]>

apoorvapendse force-pushed the create-person-local branch from 7c8a9b8 to 07a3e92 Compare June 12, 2025 05:21

apoorvapendse added 2 commits June 12, 2025 11:17

extract lineage bloat check in a function.

8e37179

Signed-off-by: apoorvapendse <[email protected]>

refactor contact type checker to use boolean pattern

740295c

Signed-off-by: apoorvapendse <[email protected]>

sugat009 requested changes Jun 12, 2025

View reviewed changes

apoorvapendse added 3 commits June 12, 2025 12:44

fix logical error in hasBloatedLineage

9081f1f

Signed-off-by: apoorvapendse <[email protected]>

add unit tests for byPersonQualifier

577ed6e

Signed-off-by: apoorvapendse <[email protected]>

refactor boolean returns

7958908

apoorvapendse requested a review from sugat009 June 12, 2025 11:49

add tests for isPersonQualifier

b5b3551

sugat009 requested changes Jun 12, 2025

View reviewed changes

hydrate reported_date if absent in by.* functions

3427c99

Signed-off-by: apoorvapendse <[email protected]>

apoorvapendse force-pushed the create-person-local branch from e9e775f to 3427c99 Compare June 12, 2025 17:08

sugat009 requested changes Jun 13, 2025

View reviewed changes

mutate qualifier instead of referenced obj.

5fe209a

apoorvapendse added a commit to apoorvapendse/cht-core that referenced this pull request Jun 13, 2025

add test containing reported_date in data

7549817

Resolves: medic#10043 (comment)

add test containing reported_date in data

566a085

Resolves: medic#10043 (comment)

apoorvapendse force-pushed the create-person-local branch from 7549817 to 566a085 Compare June 13, 2025 05:56

apoorvapendse requested a review from sugat009 June 13, 2025 08:33

sugat009 approved these changes Jun 13, 2025

View reviewed changes

sugat009 merged commit c734c65 into medic:9835-add-cht-datasource-apis-for-creation-and-update-of-contacts-and-reports Jun 13, 2025
22 checks passed

apoorvapendse mentioned this pull request Jun 15, 2025

feat(#10038): add PlaceQualifier and related methods #10057

Merged

6 tasks

	} else if (!(data.type === 'person')) {
	} else if (data.type !== 'person') {

		byContactQualifierNonAssertive(data);
		return data as ContactQualifier;

	byContactQualifierNonAssertive(data);
	return data as ContactQualifier;
	return byContactQualifierNonAssertive(data) as ContactQualifier;

feat(#10036): add PersonQualifier and related functions #10043

feat(#10036): add PersonQualifier and related functions #10043

Uh oh!

Conversation

apoorvapendse commented Jun 11, 2025

Description

Code review checklist

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoorvapendse Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoorvapendse Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoorvapendse Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sugat009 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

feat(#10036): add `PersonQualifier` and related functions #10043

feat(#10036): add `PersonQualifier` and related functions #10043

apoorvapendse Jun 11, 2025 •

edited

Loading

apoorvapendse Jun 17, 2025 •

edited

Loading

apoorvapendse Jun 17, 2025 •

edited

Loading