OpenMRS Add pagination
#741
Conversation
mtuchi
changed the title
openMRS Add cursor, dateFns and pagination
mtuchi
changed the title
openMRS Add cursor, dateFns and paginationOpenMRS Add cursor, dateFns and pagination
There was a problem hiding this comment.
There's one code quality issue which I'm going to insist on changing.
But I'm generally not happy that this adaptor has zero tests, and that this PR introduces a lot of code duplication. It's not great.
If you think you can find the time, I would like to see the following changes:
- pagination logic pushed down to the request helper. It'll paginate any request by default, based on
body.links, unless offset is passed in. - A set of unit tests on request and paginated requests. Maybe 4-6 tests in total?
If I was doing this, I'd probably ask for half a day. It is not worth spending more than a day on.
mtuchi
changed the title
OpenMRS Add cursor, dateFns and paginationOpenMRS Add pagination
|
I have added couple of unit test and move the pagination implementation into util @josephjclark |
packages/openmrs/src/Utils.js
Outdated
| }); | ||
| logResponse(response); | ||
|
|
||
| allResponses |
There was a problem hiding this comment.
This still needs to be made into an if-else statement.
A ternary expression should always be on the right-hand-side of some expression. It shouldn't be a statement by itself.
There was a problem hiding this comment.
Made some improvements for the requestOptions
There was a problem hiding this comment.
Ah maybe I wasn't clear - I'm talking about this:
if (allResponses) {
allResponses.body.results.push(...response.body.results)
} else {
allResponses = response
}
There was a problem hiding this comment.
Thanks i have update the code accordingly
| 'GET', | ||
| '/ws/rest/v1/patient', | ||
| {}, | ||
| { q: 'Sarah', limit: 1, startIndex: 1 } |
There was a problem hiding this comment.
If you remove startIndex from this test,, does it still pass?
I'll bet it does - but it shouldn't!
You need to set up the interceptor to return a next page. That way you are properly testing that your adaptor code will ignore the next page IF the startIndex is set
There was a problem hiding this comment.
I have added the interceptor for the next page
There was a problem hiding this comment.
Ah, ok, but now we still need to assert that only one page was actually fetched. You need to check that body.results.length === 1
There was a problem hiding this comment.
@josephjclark Done, i have added the assertion for results to be equal to 1
Description
Add pagination support in
openmrsadaptor.Implementation
autoFetchRequest. For pagination supportRef #744