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

(feat) O3-4398 : add new mutate lab orders prop to enable workspace callers to correctly mutate data #2225

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

(feat) O3-4398 : add new mutate lab orders prop to enable workspace callers to correctly mutate data #2225

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7789a1
feat: add new mutate lab orders prop to enable workspace callers to c…
amosmachora Feb 3, 2025
569eec4
refactor: clean up code used for logging
amosmachora Feb 3, 2025
04e7abd
code review suggestion from @denniskigen
amosmachora Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/esm-patient-orders-app/src/lab-results/lab-results-form.component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@ import styles from './lab-results-form.scss';

export interface LabResultsFormProps extends DefaultPatientWorkspaceProps {
order: Order;
mutateLabOrders?: () => void;
Copy link
Member

@denniskigen denniskigen Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The current implementation of the mutateLabOrders callback appears to serve two purposes simultaneously:

  1. Launching the test-results-form-workspace workspace.
  2. Triggering revalidation of lab orders

Is this dual responsibility intentional? Given that we're already operating within the workspace context when invoking this form, should the workspace launch be considered a separate concern from the data revalidation?

If so, consider separating these concerns by:

  1. Keeping the workspace launch as a distinct action.
  2. Passing only the revalidation logic via props.

This would better align with the Single Responsibility Principle and prevent workspace re-launching from within an already active workspace context.

Copy link
Member

Choose a reason for hiding this comment

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

I've left this additional comment that should clarify what I meant above.

Copy link
Member

Choose a reason for hiding this comment

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

I've also tested this out locally and I don't see how the Add lab results button in the Laboratory app is related to the Add results button in the Orders app.

Your other PR adds a mutate function that gets passed down to the workspace when you launch the Lab results form by clicking the Add lab results button in the Laboratory app.

On the other hand, the Add results button in the Orders table in Patient Chart has a different handler altogether.

Could you help shed some light on that, @amosmachora?

Copy link
Author

Choose a reason for hiding this comment

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

The two buttons are not related at all. They all launch the same workspace on their click handlers but from different places in the app.

That's why I wouldn't want to couple the mutateLabOrders functionality to the workspace itself and would like it to remain agnostic and have what it needs be passed down as an optional prop. I am not sure I am answering your question.

Do you think they should use the same handler?

Copy link
Member

@denniskigen denniskigen Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So the thing is, AFAICT, even if you pass mutateLabOrders down as a prop to the workspace, it only gets passed to the workspace that gets launched from clicking the Add lab results button in the Lab app.

Adding it as a prop to the orders app doesn't work because the logic that triggers launching that workspace has no knowledge of the mutateLabOrders prop.

Copy link
Author

Choose a reason for hiding this comment

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

Isn't that what we want? I think that was essentially what I was trying to achieve to find a way for whatever caller to pass a mutate function and have it just work. This was essential in the lab since it allowed for the mutation of lab orders rendered as rows. if for example in the orders app, a need would arise to mutate something (highly unlikely) they would pass a prop and it would just work.

Now that I think of it maybe the prop could be named more generically like mutate.

Adding it as a prop to the orders app doesn't work because the logic that triggers launching that workspace has no knowledge of the mutateLabOrders prop.

that's why I made it optional since it is very specific to the lab workspace and no other workspace instance.

Does this feel like a hack? Any better ideas

}

const LabResultsForm: React.FC<LabResultsFormProps> = ({
closeWorkspace,
closeWorkspaceWithSavedChanges,
order,
promptBeforeClosing,
mutateLabOrders,
}) => {
const { t } = useTranslation();
const abortController = useAbortController();
Expand Down Expand Up @@ -179,8 +181,9 @@ const LabResultsForm: React.FC<LabResultsFormProps> = ({
abortController,
);
closeWorkspaceWithSavedChanges();
mutateResults();
mutateOrderData();
mutateResults();
mutateLabOrders();
amosmachora marked this conversation as resolved.
Show resolved Hide resolved
showNotification(
'success',
t('successfullySavedLabResults', 'Lab results for {{orderNumber}} have been successfully updated', {
Expand Down