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(#9065): add cht-datasource to support getting contact by uuid #9090

Open
wants to merge 45 commits into
base: master
Choose a base branch
from

Conversation

jkuester
Copy link
Contributor

@jkuester jkuester commented May 2, 2024

Description

#9065

cht-datasource

This PR converts shared-libs/cht-script-api into shared-libs/cht-datasource as a general purpose data-access layer intended for use within cht-core both in server and the client-side code as well as within custom config code for (tasks, targets, etc).

There are two modes of functionality exported from cht-datasource. Both require you to first acquire a DataContext (either remote or local depending on if you can interact directly with the CHT api server or if offline functionality is required).

import { getRemoteDataContext, getLocalDataContext } from '@medic/cht-datasource';

const dataContext = isOnlineOnly
  ? getRemoteDataContext(...)
  : getLocalDataContext(...);

Imperative

The imperative mode provides a hierarchical factory object that you can drill down into and interact with the datasource.

import { getDatasource } from '@medic/cht-datasource';

const datasource = getDatasource(dataContext);
const myUuid = 'my-uuid';
const myPerson = await datasource.v1.person.getByUuid(myUuid);

This mode is useful for exposing functionality to custom configuration code (tasks, targets, etc) by passing the datasource object directly into the custom function.

Declarative

The declarative mode provides more flexible access to API functions. The dataContext must be curried when using the functions.

import { Person, Qualifier } from '@medic/cht-datasource';

const getPerson = Person.V1.get(dataContext);
const myUuid = 'my-uuid';
const myPerson = await getPerson(Qualifier.byUuid(uuid));

Code review checklist

  • 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.

Compose URLs

If Build CI hasn't passed, these may 404:

License

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

Copy link
Member

@m5r m5r left a comment

Choose a reason for hiding this comment

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

+1 on what Gareth said about the API design it makes more sense to have the two functions implement the same interface. I wish the TS team had implemented microsoft/TypeScript#420, it would have been pretty nice in our scenario but we can surely make cht-datasource work without!

shared-libs/cht-datasource/src/index.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/index.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/libs/doc.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/libs/person.ts Outdated Show resolved Hide resolved
@jkuester jkuester requested review from m5r and sugat009 May 15, 2024 21:58
.mocharc.js Outdated Show resolved Hide resolved
@jkuester jkuester requested review from m5r and sugat009 May 24, 2024 20:36
@jkuester
Copy link
Contributor Author

jkuester commented May 24, 2024

@m5r @sugat009 This is ready for another review! Actually, IMHO this code is "complete" in that I would be comfortable merging it to master. It is the complete flow for adding an endpoint to "get person without lineage" along with the necessary unit/integration tests. That being said, before we actually merge this, we want to also include:

  • Get person without lineage
  • Get person with lineage
  • Get place without lineage
  • Get place with lineage

(These other three should be much quicker to add now that the patterns/utils are stabilizing).

@jkuester jkuester marked this pull request as ready for review May 24, 2024 20:47
Copy link
Contributor

@lorerod lorerod left a comment

Choose a reason for hiding this comment

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

From a testing perspective, this looks good. I just have a small suggestion. Thank you!

webapp/src/ts/services/cht-script-api.service.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
Copy link
Member

@m5r m5r left a comment

Choose a reason for hiding this comment

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

I did a thorough pass of the PR and it looks great! I know you're expecting to add a few more APIs to the Person and Place APIs but the groundwork you laid here is fantastic and ready to merge IMO 👏

I wouldn't mind having the additional APIs in another PR, it would make the changes easier to review

@jkuester jkuester requested a review from m5r June 11, 2024 19:41
@jkuester
Copy link
Contributor Author

@m5r this should be ready for a final review! 🙏

I wouldn't mind having the additional APIs in another PR, it would make the changes easier to review

Good call! Once this PR is approved, I will merge and then we can open the subsequent PRs against master. 👍

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.

None yet

5 participants