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.

Sign up for GitHub

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

Jump to bottom

(feat) O3-4136: Move lab-results components From esm-patient-orders-app To esm-patient-tests-app #2091

Open

Twiineenock wants to merge 3 commits into openmrs:main from Twiineenock:chore/move-lab-results

Contributor

Twiineenock commented Nov 4, 2024 •

edited by denniskigen

Loading

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

The esm-patient-orders-app should be limited to handling generic order-management functionalities. All laboratory-specific components and features should be contained within the esm-patient-tests-app (see)

Screenshots

Related Issue

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

Other

After the move!

lab-results.webm

denniskigen reviewed

View reviewed changes

Member

denniskigen left a comment

Good start, @Twiineenock. I’ve left some feedback.

packages/esm-patient-tests-app/src/lab-results/lab-results-form.test.tsx Outdated Show resolved Hide resolved

denniskigen requested a review from ibacher

November 7, 2024 19:02

brandones reviewed

View reviewed changes

packages/esm-patient-common-lib/src/lab-resource/types/encounter.ts Outdated

+              import { type OpenmrsResource, type Visit } from '@openmrs/esm-framework';
+              import { type Order } from '@openmrs/esm-patient-common-lib';
+              export interface Encounter {

Contributor

brandones Nov 25, 2024

I'm inclined to say we should add these instead to src/types/index.ts

Contributor

brandones commented Nov 25, 2024

Code looks good to me with the one caveat I mentioned. @ibacher could you just confirm that this work covers the extent of what you'd imagined with O3-4136? It really just moves the lab results entry workspace.

ibacher changed the title ~~(chore) O3-4136: Move lab-results components From esm-patient-orders-app To esm-patient-tests-app~~ (fix) O3-4136: Move lab-results components From esm-patient-orders-app To esm-patient-tests-app

ibacher changed the title ~~(fix) O3-4136: Move lab-results components From esm-patient-orders-app To esm-patient-tests-app~~ (feat) O3-4136: Move lab-results components From esm-patient-orders-app To esm-patient-tests-app

Member

ibacher commented Nov 25, 2024

Couple of quick comments:

(chore) should only be used for things that do not touch any code used in production or tests, things like docs, eslint configuration, tsconfig, all the tooling that we use that isn't part of the app. Anything that touches any code is always bigger than a (chore). This is, in fact, technically breaking, but really more the size of (feat) or, equivalently, (refactor).
As @brandones alluded to, I had hoped to excise every specific reference to "tests" from the orders app (and, in another ticket, "medications"), so that the "orders" app is just the plumbing for orders. However, this is definitely a step in the right direction and I don't mind PRs that partially fulfill things as long as we clearly document that on Jira.

ibacher requested changes

View reviewed changes

packages/esm-patient-common-lib/src/lab-resource/index.ts Outdated

@@ @@ -0,0 +1,3 @@ @@
+              export * from './lab.resource';
+              export * from './types/encounter';
+              export * from './types/order';

Member

ibacher Nov 25, 2024

nit:

Suggested change

      
            export * from './types/order';
          
            export * from './types/order';

packages/esm-patient-common-lib/src/lab-resource/types/order.ts Outdated

Member

ibacher Nov 25, 2024

See this is, to me, the thing that should not exist, a shared reference to "lab orders"

packages/esm-patient-common-lib/src/lab-resource/types/order.ts Outdated

		@@ -0,0 +1,16 @@
		import { type Order } from '@openmrs/esm-patient-common-lib';

		export interface PatientMedicationFetchResponse {

Member

ibacher Nov 25, 2024

Why is this type in /lab-resource/types. It seems like it definitely belongs somewhere else.

packages/esm-patient-common-lib/src/lab-resource/types/order.ts Outdated

+              export interface OrderDiscontinuationPayload {
+                previousOrder: string;
+                type: string;
+                action: string;

Member

ibacher Nov 25, 2024

I feel like order action is a restricted (enum) type we already have defined. Please re-use that.

packages/esm-patient-common-lib/src/lab-resource/types/order.ts Outdated

+                encounter: string;
+                patient: string;
+                concept: string;
+                orderer: { display: string; person: { display: string }; uuid: string };

Member

ibacher Nov 25, 2024

Is this the result we get back or the type for the message we send? Display strings should never be sent to the backend under any circumstances.

Contributor Author

Twiineenock Dec 16, 2024 •

edited

Loading

@ibacher , its the message we send! Should we get rid of this orderer: { display: string; person: { display: string }; uuid: string }; prop and all its instances in this context?

Member

ibacher Dec 16, 2024

It should hold the uuid of the current practitioner for the user placing the order.

packages/esm-patient-common-lib/src/lab-resource/types/order.ts Outdated

+                previousOrder: string;
+                type: string;
+                action: string;
+                careSetting: string;

Member

ibacher Nov 25, 2024

This, also, could be more tightly-bound.

packages/esm-patient-tests-app/src/lab-results/useLabResultsFormSchema.tsx

@@ @@ -1,5 +1,5 @@ @@
               import { z } from 'zod';
-              import { type LabOrderConcept, useOrderConceptByUuid } from './lab-results.resource';
+              import { type LabOrderConcept, useOrderConceptByUuid } from '@openmrs/esm-patient-common-lib';

Member

ibacher Nov 25, 2024

This is probably minor, but do we really need a type for the concept for a lab order? That seems... decadent.

packages/esm-patient-orders-app/translations/en.json

                 "indication": "Indication",
                 "launchOrderBasket": "Launch order basket",
-                "loading": "Loading",
-                "loadingInitialValues": "Loading initial values",
-                "loadingTestDetails": "Loading test details",

Member

ibacher Nov 25, 2024

In this case, where we're moving strings between packages, it is important that we retain the existing translations in non-English languages. Unfortunately, the only way I know to do that is some tedious copy-and-paste work, but this would be the rare instance were manually updating the translation files is for the best.

Contributor

brandones commented Dec 19, 2024

@Twiineenock is it clear to you how to move forward with this?


          more fixes

79e51c1

Contributor Author

Twiineenock commented Dec 19, 2024

@Twiineenock is it clear to you how to move forward with this?

@brandones , a lot was merged in since my first commit, I am making changes locally and pushing soon.


          fix lab-results test

938572d

Twiineenock force-pushed the chore/move-lab-results branch from a2dc87e to 938572d Compare

December 19, 2024 14:26


          add en translations to tests-app

46001e2

Twiineenock commented

View reviewed changes

Contributor Author

Twiineenock left a comment

Hi @ibacher and @brandones,

What are your thoughts on setting up translations in the esm-patient-common-lib? I recently added a file with translations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet