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

10261 Story: Practitioner Case List #5397

Merged
merged 42 commits into from
Oct 24, 2024
Merged

Conversation

Mwindo
Copy link
Contributor

@Mwindo Mwindo commented Sep 24, 2024

Description

This PR adds practitioner open and closed case lists for internal DAWSON users. (Ticket here)

Screenshot 2024-09-26 at 7 25 57 AM Screenshot 2024-09-26 at 7 26 05 AM

Overview of changes:

  • Adds a PractitionerCaseList component and PractitionerCaseIcons component. (The latter can probably be extended beyond practitioners and used elsewhere? Icons seem to be rendered differently in different areas of the app.)
  • Pulls out some logic from generatePractitionerCaseListPdfInteractor into a new interactor, getPractitionerCasesInteractor, which is also used in the actions that set up state for PractitionerCaseList.
  • Adds typing for state.practitionerDetails.

Notes:

  • A small number of practitioners have thousands of cases. After tests on the test environment, I determined that back-end pagination approaches were not worth the cost to implement: The worst-case page load time was a few seconds for me (even with poor network speed), and the practitioner with the most cases (4000+) is still be far from our response size limit (~.8 MB out of 6 MB). See conversation here. Since that comment, I have verified the responseSize number for this user in our logs:
Screenshot 2024-10-18 at 10 09 16 AM

Comment on lines +40 to +72
const openPagesPaginator = () => {
return (
practitionerInformationHelper.showOpenCasesPagination && (
<Paginator
currentPageIndex={practitionerInformationHelper.openCasesPageNumber}
totalPages={practitionerInformationHelper.totalOpenCasesPages}
onPageChange={selectedPage => {
setPractitionerOpenCasesPageSequence({
pageNumber: selectedPage,
});
}}
/>
)
);
};

const closedPagesPaginator = () => {
return (
practitionerInformationHelper.showClosedCasesPagination && (
<Paginator
currentPageIndex={
practitionerInformationHelper.closedCasesPageNumber
}
totalPages={practitionerInformationHelper.totalClosedCasesPages}
onPageChange={selectedPage => {
setPractitionerClosedCasesPageSequence({
pageNumber: selectedPage,
});
}}
/>
)
);
};
Copy link
Contributor Author

@Mwindo Mwindo Sep 25, 2024

Choose a reason for hiding this comment

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

Ideally, I would want pagination to live on PractitionerCaseList directly and be handled by using useState and URL params; however, this goes against our current use of Cerebral.

I chose what I thought was the next best option: Rather than pass in specific global state into PractitionerCaseList, which makes the component "messier," I chose to pass in the list of cases into PractitionerCaseList and handle pagination here in this component since it is already connected to global state. This keeps us in line with using Cerebral but also keeps PractitionerCaseList pretty simple and reusable.

Comment on lines +19 to +23
applicationContext
.getUseCases()
.getPractitionerCasesInteractor.mockImplementation(() => {
throw new Error('Unauthorized');
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unauthorized error is now thrown on getPractitionerCasesInteractor. That said, I think it is still important to test that generatePractitionerCaseListPdfInteractor independently "bubbles up" that error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tiniest of nits: could this also throw new UnauthorizedError('Unauthorized to view practitioners cases');? Wouldn't even be worthy of another commit, just if you happen to change something else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it. I'll keep this comment in mind.

@@ -82,75 +91,19 @@ describe('generatePractitionerCaseListPdfInteractor', () => {
).rejects.toThrow('Practitioner not found');
});

it('sorts open and closed cases before sending them to document generator', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A version of this test has been moved to getPractitionerCasesInteractor.

@Mwindo Mwindo marked this pull request as ready for review October 23, 2024 21:59
}
return (
(Case.getSortableDocketNumber(docketNumberA) || 0) -
(Case.getSortableDocketNumber(docketNumberB) || 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉🎉🎉

@Mwindo Mwindo marked this pull request as draft October 24, 2024 18:49
@Mwindo
Copy link
Contributor Author

Mwindo commented Oct 24, 2024

@zachrog voiced a concern about the cypress tests using seed data, which is reasonable and a good catch. Thank you! I will put into draft mode and address.

Comment on lines 141 to 147
it.each([
[0, 0],
[1, 1],
[pageSize, 1],
[pageSize + 1, 2],
[pageSize * 2 + 1, 3],
])(
Copy link
Contributor

Choose a reason for hiding this comment

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

Niiice!

Copy link
Contributor

@TomElliottFlexion TomElliottFlexion left a comment

Choose a reason for hiding this comment

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

Looks good (apart from seed data perhaps)! None of my comments are of much import

@Mwindo Mwindo marked this pull request as ready for review October 24, 2024 20:09
@jimlerza jimlerza merged commit bb9ca50 into ustaxcourt:staging Oct 24, 2024
44 checks passed
@jimlerza jimlerza deleted the 10261-story branch October 24, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants