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.

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
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 3 commits into from

Conversation

Twiineenock
Copy link
Contributor

@Twiineenock 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
denniskigen reviewed Nov 6, 2024
View reviewed changes
Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

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 denniskigen requested a review from ibacher November 7, 2024 19:02
brandones
brandones reviewed Nov 25, 2024
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@brandones
Copy link
Contributor

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 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 Nov 25, 2024
@ibacher 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 Nov 25, 2024
@ibacher
Copy link
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
ibacher requested changes Nov 25, 2024
View reviewed changes
packages/esm-patient-common-lib/src/lab-resource/index.ts Outdated Show resolved Hide resolved
packages/esm-patient-common-lib/src/lab-resource/types/order.ts Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@brandones brandones Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@Twiineenock It looks like you've moved lab resource stuff from esm-patient-orders-app to esm-patient-common-lib. I think the idea of this ticket is that all the lab stuff should be in esm-patient-tests-app. Why is lab-resource in esm-patient-common-lib and not esm-patient-tests-app?

packages/esm-patient-common-lib/src/lab-resource/types/order.ts Outdated Show resolved Hide resolved
packages/esm-patient-common-lib/src/lab-resource/types/order.ts Outdated Show resolved Hide resolved
packages/esm-patient-common-lib/src/lab-resource/types/order.ts Outdated Show resolved Hide resolved
packages/esm-patient-common-lib/src/lab-resource/types/order.ts Outdated
previousOrder: string;
type: string;
action: string;
careSetting: string;
Copy link
Member

Choose a reason for hiding this comment

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

This, also, could be more tightly-bound.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Twiineenock please address this feedback

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';
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 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 Show resolved Hide resolved
@brandones
Copy link
Contributor

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

@Twiineenock
Copy link
Contributor Author

@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.

@Twiineenock Twiineenock force-pushed the chore/move-lab-results branch from a2dc87e to 938572d Compare December 19, 2024 14:26
Twiineenock
Twiineenock commented Dec 20, 2024
View reviewed changes
Copy link
Contributor Author

@Twiineenock Twiineenock left a comment

Choose a reason for hiding this comment

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

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.

@brandones
Copy link
Contributor

Hi @Twiineenock , I don't think it would make sense to put translations in esm-patient-common-lib, and we don't really have a way of doing that anyway.

Thank you for moving the translations over!

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

@ibacher ibacher ibacher requested changes

@denniskigen denniskigen denniskigen left review comments

@brandones brandones brandones left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Twiineenock @brandones @ibacher @denniskigen