-
Notifications
You must be signed in to change notification settings - Fork 8
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
OpenMRS
Add pagination
#741
Conversation
openMRS
Add cursor, dateFns and pagination
openMRS
Add cursor, dateFns and paginationOpenMRS
Add cursor, dateFns and pagination
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
OpenMRS
Add cursor, dateFns and paginationOpenMRS
Add pagination
I have added couple of unit test and move the pagination implementation into util @josephjclark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I much prefer this approach 👍 And thanks for the extra test coverage. Just a few little comments to address.
packages/openmrs/src/Utils.js
Outdated
}); | ||
logResponse(response); | ||
|
||
allResponses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some improvements for the requestOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the interceptor for the next page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josephjclark Done, i have added the assertion for results to be equal to 1
Description
Add pagination support in
openmrs
adaptor.Implementation
autoFetchRequest
. For pagination supportRef #744