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

(refactor) Generalizing Application Logic - Decoupling from UgandaEMR in Lab Workflows #65

Merged
merged 8 commits into from
Apr 15, 2024

Conversation

samuelmale
Copy link
Member

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This PR primarily removes logic that is specific to UgandaEMR's custom Lab workflows. I'm preparing a follow-up PR to migrate this logic to a dedicated UgandaEMR-specific repository. Additionally, the PR includes bug fixes, general code enhancements. However, there is room for improvement in code quality, and follow-up tasks will be created to address this.

Screenshots

https://www.loom.com/share/2aa94526398f4114bd96e06d64d56c44

Related Issue

https://openmrs.atlassian.net/browse/O3-2640

@samuelmale samuelmale changed the title Chore/generalize app (refactor) Generalizing Application Logic: Decoupling from UgandaEMR in Lab Workflows Mar 27, 2024
@samuelmale samuelmale changed the title (refactor) Generalizing Application Logic: Decoupling from UgandaEMR in Lab Workflows (Refactor) Generalizing Application Logic - Decoupling from UgandaEMR in Lab Workflows Mar 27, 2024
@samuelmale samuelmale changed the title (Refactor) Generalizing Application Logic - Decoupling from UgandaEMR in Lab Workflows (refactor) Generalizing Application Logic - Decoupling from UgandaEMR in Lab Workflows Mar 27, 2024
@gitcliff
Copy link
Contributor

LGTM ...

@gitcliff
Copy link
Contributor

gitcliff commented Apr 2, 2024

@pirupius @jabahum @ojwanganto WDYT?

Copy link

@makombe makombe left a comment

Choose a reason for hiding this comment

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

Thanks @samuelmale LGTM!

src/components/orders-table/orders-data-table.scss Outdated Show resolved Hide resolved
src/lab-tabs/modals/pickup-lab-request-modal.component.tsx Outdated Show resolved Hide resolved
import { InlineLoading } from "@carbon/react";
import styles from "./loader.scss";

const Loader: React.FC = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point, I hope we'll have such in a common/core lib where we just reuse.

Copy link
Member Author

Choose a reason for hiding this comment

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

Plus 100

Copy link
Member

@pirupius pirupius left a comment

Choose a reason for hiding this comment

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

Thanks @samuelmale for the amazing work. I've left some comments around some quick fixes we can add to this PR. And i'll start a conversation on the slack channel around the workflows

.vscode/settings.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
_default: false,
_description: "This enables sending results to patient via email",
},
enableSpecimenIdAutoGeneration: {
Copy link
Member

Choose a reason for hiding this comment

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

This is something i believe should be part of the default because it was a requirement for multiple implementations. With Ampath and KenyaEMR interested

Copy link
Member Author

Choose a reason for hiding this comment

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

@pirupius Do we have a clear ID generation strategy? I checked the KenyaEMR instance and this feature doesn't exist and I believed it's one of the reasons they forked

Copy link
Member

@pirupius pirupius Apr 3, 2024

Choose a reason for hiding this comment

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

@jabahum @slubwama please shade more light on this. @samuelmale you can see the demo on the UgandaEMR staging instance

Copy link
Member Author

Choose a reason for hiding this comment

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

@pirupius I'm familiar with how it works for the UgandaEMR and it seems to works perfectly. The plan is to port this feature in their custom repo so they won't be affected.

If we are to support the auto ID generation feature we have to either adopt UgandEMR's backend solution or figure out an ID gen strategy which is beyond the scope of this PR.

src/lab-tabs/modals/pickup-lab-request-modal.component.tsx Outdated Show resolved Hide resolved
src/laboratory-resource.ts Outdated Show resolved Hide resolved
@samuelmale
Copy link
Member Author

The "Tests ordered" tab is like a bucket of all tests ordered with different statuses. It's equipped with a filter that lets you filter the list by status. However there was no way for say "view new orders; orders without a fulfiller status and with a new action". To solve this problem, I've introduced a filter option called "NEW". This is important for UgandEMR's workflow.

2024-04-05 01-50-16 2024-04-05 01_52_34

@ojwanganto
Copy link
Contributor

@samuelmale I don't understand the rationale for the new filter especially for statuses already covered in other tabs. We need to discuss the transition process and the corresponding tabs that various fulfiller actions should map to. Am certain lab processes are common across different countries/implementations

@samuelmale
Copy link
Member Author

samuelmale commented Apr 5, 2024

@ojwanganto The "Tests ordered" is like a bucket of orders with different fulfiller statuses. However this varies across implementations, eg. For UgandaEMR's use case, this tab list only the freshly published orders (without a fulfiller status). This ensures that the lab attendants focus on the incoming fresh orders to take action.

I don't understand the rationale for the new filter especially for statuses already covered in other tabs.

Imagine having a very long mixed (orders of disparate statuses) list of orders but without a way of applying a filter that will let you focus on the recently published orders? The main rationale for this filter option (a derived status) is to bring in the ability of viewing those new orders. However, if this seems unnecessary I can revert; just let me know

@pirupius
Copy link
Member

pirupius commented Apr 5, 2024

@samuelmale @ojwanganto this takes us back to the conversation i started on slack about having that bucket of statuses (aka NEW) being the only ones shown in the first tab. It's the landing point and the primary users for this are the lab attendants who want to know what lab orders need to be worked on.

@ojwanganto
Copy link
Contributor

Thanks @samuelmale, @pirupius - my guess is that the implementation of the first tab should be common across :). It is basically what is fresh from a clinician to the lab.

@samuelmale
Copy link
Member Author

samuelmale commented Apr 8, 2024

@pirupius @ojwanganto I'm on board with dropping the filter in the "Tests Ordered" table and default to loading "NEW" orders. However this idea of having the filter was something the DIGI team was interested in and I believe we had arrived at a consensus of us having it that way? Should we drop the filter? cc: @gitcliff @caseyi @slubwama @jabahum

@jabahum
Copy link
Collaborator

jabahum commented Apr 15, 2024

LGTM unless there's any concerns from others

Copy link
Collaborator

@jabahum jabahum left a comment

Choose a reason for hiding this comment

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

LGTM

@pirupius
Copy link
Member

Merging this in to unblock other tasks. Any other enhancements will come in follow up PR's

@pirupius pirupius merged commit d44193c into main Apr 15, 2024
3 checks passed
@pirupius pirupius deleted the chore/generalize-app branch April 15, 2024 15:49
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.

6 participants