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(#9193): add functionality of getting people as pages or async iterables in cht-datasource #9311

Merged
merged 36 commits into from
Aug 23, 2024

Conversation

sugat009
Copy link
Member

@sugat009 sugat009 commented Aug 12, 2024

Description

Added functionality of getting people as using pages or as AsyncGenerator using cht-datasource along with addition of REST endpoint /api/v1/person which provides people as pages.

Issues: #9237 , #9238 , #9241

Closes #9241

Usage:

  1. Pagination API
Person.v1.getPageByType(Qualifier.byContactType(personType), cursor, limit); // limit and cursor are optional with defaults as 100 and "0" respectively
  1. Async Generator API
const getAllIterator = Person.v1.getByType(ctx)(Qualifier.byContactType('person'));
  1. REST API endpoint
HTTP GET api/v1/person

Query Params:

  • personType - the person_type to fetch
  • limit - default is 100
  • cursor - default is 0

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

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.

sugat009 and others added 7 commits August 9, 2024 19:18
Removes padStart from CouchDb views. This function was not available in the Javascript engine packaged with CouchDb v2, which ships in CHT v4.3.x and lower.
Unfortunately, when the view code crashes, it triggers a warning in CouchDb and the document is just skipped, while the view index advances. This means that that document needs to be edited in order to appear as a view result, or the whole index needs to be rebuilt, after upgrading to CouchDb 3.

#9298
@sugat009 sugat009 added the Type: Feature Add something new label Aug 12, 2024
@sugat009 sugat009 requested a review from jkuester August 12, 2024 13:15
@sugat009 sugat009 self-assigned this Aug 12, 2024
@sugat009 sugat009 linked an issue Aug 12, 2024 that may be closed by this pull request
@sugat009 sugat009 marked this pull request as ready for review August 13, 2024 05:03
@sugat009
Copy link
Member Author

The CI is failing for a param's JSDoc which exists but ESLint is marking it as does not exist.
https://github.com/medic/cht-core/actions/runs/10352638921/job/28653693985?pr=9311#step:11:88
Looking into it right now.
This might have been due to the upgrade of ESLint as this was passing before.

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.

This is looking great! Today I was able to make it through the cht-datasource implementation code (where I left some minor comments, mostly related to the null/undefined cursor discussion from Slack). Tomorrow I will continue with the rest!

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/index.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/index.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/qualifier.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/libs/data-context.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/local/person.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/local/person.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/libs/core.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/local/person.ts Outdated Show resolved Hide resolved
@sugat009 sugat009 requested a review from jkuester August 14, 2024 09:24
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.

This is great! I have made it though the rest of the implementation code and all the tests. Just a number of nitpicks thoughout and a few logic tweaks, but otherwise this is nearly complete.

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/person.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/person.ts Outdated Show resolved Hide resolved
shared-libs/cht-datasource/src/person.ts Outdated Show resolved Hide resolved
api/src/server-utils.js Outdated Show resolved Hide resolved
api/src/controllers/person.js Outdated Show resolved Hide resolved
api/tests/mocha/controllers/person.spec.js Outdated Show resolved Hide resolved
tests/integration/api/controllers/person.spec.js Outdated Show resolved Hide resolved
...offlineUserPlaceHierarchy
}
];
const responsePage = await getPage(Qualifier.byContactType(personType), cursor, limit);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const responsePage = await getPage(Qualifier.byContactType(personType), cursor, limit);
const responsePage = await getPage(Qualifier.byContactType(personType));

I don't want to get too deep into edge cases with these integration tests, but can we have this test cover the case where we use the default cursor/limit values. Then add another test where we call getPage twice. On the first call we give it a null cursor and a limit of 4. Then on the second call, we pass in the cursor value returned from the first call (and again a limit of 4). Then we can assert that between the two calls, we got the expected 7 persons back. One of the key things I want to be careful to assert in the new test is the value returned for the page cursor. Should be '4' on the first call and then null on the second.

Copy link
Contributor

Choose a reason for hiding this comment

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

One more case that I thought of that we should probably have is to call the Person.v1.getAll function and load up all the returned persons and make sure we get all 7 of them. Just a safety check that our generator code integrates fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to get too deep into edge cases with these integration tests, but can we have this test cover the case where we use the default cursor/limit values. Then add another test where we call getPage twice. On the first call we give it a null cursor and a limit of 4. Then on the second call, we pass in the cursor value returned from the first call (and again a limit of 4). Then we can assert that between the two calls, we got the expected 7 persons back. One of the key things I want to be careful to assert in the new test is the value returned for the page cursor. Should be '4' on the first call and then null on the second.

The reason I kept the integration test with the "non-paging" values for limit and cursor is that the objects being returned from the database are not in a consistent order for the order that we are passing into the utils.saveDocs() function. In our case, we pass const allDocItems = [contact0, contact1, contact2, place0, place1, place2, patient]; into utils.saveDocs but it is not guaranteed that contacts will be returned in the same order. The order is undeterministic and random; it could be the order in which they were indexed into the view, so the page cannot be compared. For example, if I request a page with 4 items, it could be any 4 items.

Copy link
Contributor

Choose a reason for hiding this comment

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

The order is undeterministic and random; it could be the order in which they were indexed into the view

Minor, but I just want to clarify that order the data is returned should not be random. My understanding is that data returned from couch views is deterministically ordered based on the key value and in cases like this where we have many results for the same key, the order is by _id value. Each call to getPage with the same parameters, should return the same set of data (otherwise our skip-based paging would not work 😓). However, this does not help us much for these integration tests since, as you have observed, the UUID's assigned as the _id value for our new contacts are random and so, the order of the returned data could change between test runs.

All that being said, I don't think we need to worry too much about trying to assert the order of the data returned. We don't even need to assert the exact docs returned for each page. For the test where we get multiple pages, it seems enough to just assert the size of the data array for each page. Then at the end combine the data arrays into one array that contains all the contacts and do your equalInAnyOrder assertion to make sure that ultimately we got back all the contacts we expected.

sugat009 and others added 2 commits August 15, 2024 13:28
@sugat009 sugat009 requested a review from jkuester August 20, 2024 15:10
@jkuester jkuester requested a review from tatilepizs August 20, 2024 15: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.

Left a couple other minor comments around the tests, but otherwise this is ready to go!

One thing we do not want to forget, @sugat009, is that we need to update the api reference docs to include documentation for the new REST endpoint. Here is one of the previous PRs for example.

…rollers/person.spec.js

and address PR comments
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.

@sugat009 sugat009 merged commit 34dd030 into master Aug 23, 2024
45 checks passed
@sugat009 sugat009 deleted the 9193-api-endpoints-for-getting-contacts-by-type branch August 23, 2024 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Add something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create API endpoint for getting people API Endpoints for getting contacts by type
5 participants