(feat) O3-2760: Present workspaces should leverage the `promptBeforeClosing` function #1613

vasharma05 · 2024-01-24T06:16:18Z

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 updates the existing workspaces implementation to use the promptBeforeClosing functions in the workspaces.

This PR makes the following changes in the codebase:

Shifts all the workspaces registered as extensions from routes.jsonto registering them viaregisterWorkspace`.
Added workspace title translations in the index.ts
All the workspaces now use the promptBeforeClosing function.
All the workspace forms (except the Lab order form) use React Hook Form, which returns a state isDirty, which tells whether the user made any change to the form or not. The promptBeforeClosing keeps an eye on the form state isDirty and decides whether to show the user a prompt when closing form without saving form changes.

CloseWorkspace

Workspace's closeWorkspace now takes an object of the following keys as an argument:

ignoreChanges : Whether to just close the workspace
onWorkspaceClose: Callback function to be called when the workspace is closed.

P.S. The onWorkspaceClose function can be used in the following manner:

❌ 
workspace.closeWorkspace();
clearGlobalStore()

✅ 
workspace.closeWorkspace({
	onWorkspaceClose: () => clearGlobalStore();
})

GetWhetherWorkspaceCanBeClosed

A new function getWhetherWorkspaceCanBeClosed is introduced which tells whether a workspace can be directly closed or the user will be shown a prompt before closing the function.

Screenshots

None

Related Issue

https://issues.openmrs.org/browse/O3-2760

Other

Also solves O3-2714.

…rkspaces

packages/esm-patient-common-lib/src/workspaces/workspaces.ts

packages/esm-patient-common-lib/src/types/workspace.ts

…nt lists

…ations app

…s app

…ams app

ibacher

Wouldn't it make sense if the closeWorkspace() function returned a Promise<void> rather than void? Then instead of needing all this logic to embed the onWorkspaceClose() callback, callers that need to do something after the workspace closes can just do:

closeWorkspace().then(/* do something */);

packages/esm-patient-common-lib/src/workspaces/workspaces.ts

…s app

…ient-chart into feat/O3-2760

usamaidrsk · 2024-02-05T07:55:02Z

@vasharma05 order is cleared upon prompting.

order-removed-even-before-user-discards.mp4

vasharma05 · 2024-02-05T08:16:35Z

Thank you @usamaidrsk , I'll fix it!

…rkspaces and keeping the closeWorkspace default behaviour intact

brandones · 2024-02-09T17:58:11Z

@vasharma05 Please re-request review when ready. I've got my eye on this.

packages/esm-patient-chart-app/src/root.component.tsx

denniskigen · 2024-02-09T22:10:41Z

packages/esm-patient-chart-app/src/root.component.tsx

- * t('unsavedChangesInForms', 'There are unsaved changes in the following workspaces. Do you want to discard changes in the following workspaces? {{workspaceNames}}')
- * t('closeWorkspaces', 'Discard changes in {{count}} workspaces', {count})
+
+ * t('unsavedChangesInOpenedWorkspace', 'You have unsaved changes in the opened workspace. Do you want to discard these changes?')


Suggested change

* t('unsavedChangesInOpenedWorkspace', 'You have unsaved changes in the opened workspace. Do you want to discard these changes?')

* t('unsavedChangesInOpenedWorkspace', 'You have unsaved changes in the currently opened workspace. Do you want to discard these changes?')

denniskigen · 2024-02-09T22:11:43Z

packages/esm-patient-chart-app/src/root.component.tsx

+ * t('closingAllWorkspacesPromptTitle','You have unsaved changes')
+ * t('closingAllWorkspacesPromptBody', 'There are unsaved changes in the following workspaces. Do you want to discard changes in the following workspaces? {{workspaceNames}}')
+ * t('closeAllOpenedWorkspaces', 'Discard changes in {{count}} workspaces', {count})
+ * t('unsavedChangesInWorkspace', 'There are unsaved changes in {{workspaceName}}. Please save them before opening another workspace.')


Nit: I'm not sure that relying on workspace names for this particular translation is the most ideal approach because they're so arbitrary. That said, I don't have a better alternative.

Since we had this in the previous versions as well, I say we move ahead with this

packages/esm-patient-chart-app/src/visit/visit-form/visit-form.component.tsx

denniskigen · 2024-02-09T22:16:14Z

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

-  "closeWorkspaces_other": "Discard changes in {{count}} workspaces",
+  "closeAllOpenedWorkspaces_one": "Discard changes in {{count}} workspaces",
+  "closeAllOpenedWorkspaces_other": "Discard changes in {{count}} workspaces",
+  "closingAllWorkspacesPromptBody": "There are unsaved changes in the following workspaces. Do you want to discard changes in the following workspaces? {{workspaceNames}}",


It doesn't seem right to repeat the phrase 'following workspaces' twice. Also, wouldn't it be better to list out the open workspaces as list items?

Suggested change

"closingAllWorkspacesPromptBody": "There are unsaved changes in the following workspaces. Do you want to discard changes in the following workspaces? {{workspaceNames}}",

"closingAllWorkspacesPromptBody": "There are unsaved changes in several workspaces. Do you want to discard changes in the following workspaces? {{workspaceNames}}",

packages/esm-patient-common-lib/src/workspaces/workspaces.ts

packages/esm-patient-conditions-app/src/conditions/conditions-form.component.tsx

Co-authored-by: Dennis Kigen <[email protected]>

vasharma05 · 2024-02-15T15:50:28Z

I had a discussion with Ciaran today in the O3 UI/UX Design hours, and he said that the Discard button in the workspace will not directly close the workspace, but will show the prompt if there are changes in the form and the user can click the Discard Changes button in the prompt to close the form.

Hence I will be reverting on the discardChangesAndCloseWorkspace and replace it back with closeWorkspace and ignoreChanges = false.

So sorry everyone for hanging this PR so long. Thank you for bearing with me and I'll finalize the PR for final review.
Thanks a lot!

brandones · 2024-02-15T15:59:07Z

Good call talking to Ciaran about it! Thanks for doing such a thorough job.

…ient-chart into feat/O3-2760

…-esm-patient-chart into feat/O3-2760

denniskigen · 2024-02-16T19:51:48Z

Do let us know when you're ready so we can do a fresh pass of reviews, @vasharma05! This is very consequential work, thanks for taking it on!

vasharma05 · 2024-02-19T09:07:08Z

Hi @denniskigen!
The final changes are pushed and the PR is ready for final review.
CC: @brandones @ibacher

brandones · 2024-02-19T17:16:54Z

packages/esm-patient-allergies-app/src/allergies/allergies-form/allergy-form.component.tsx

@@ -154,7 +166,7 @@ function AllergyForm({ closeWorkspace, patientUuid }: DefaultWorkspaceProps) {
          (response: FetchResponse) => {
            if (response.status === 201) {
              mutate();
-              closeWorkspace();
+              closeWorkspace({ ignoreChanges: true });


Huh, I see. Now that ignoreChanges is false by default, we have to pass this argument on saving & exiting, which is a little awkward. Nevertheless I still think this is the right approach.

Actually, I guess the thing that is weird is actually that isDirty is still true after the form is submitted. Logically, it seems like it shouldn't be.

brandones · 2024-02-19T18:07:07Z

packages/esm-patient-common-lib/src/workspaces/workspaces.ts

+ * @returns
+ */
+
+export function showWorkspacePrompts(


I don't really like this as a pattern; I think it's inappropriate unless the same prompts are being launched from many different places in the code. It is better to have the content of the prompt housed in the context that it is relevant to. But that's just sort of a note for posterity; let's get this PR in.

brandones

LGTM. I agree with @denniskigen 's suggestions about copy but this is a big PR that's been around for a while already. Requesting his forgiveness and that we fix the copy after merging this.

Thanks so much for all your work on this @vasharma05 !

…ient-chart into feat/O3-2760

jayasanka-sack · 2024-02-21T15:05:23Z

@vasharma05, the program enrollment E2E test has been failing since this PR. Can we sort it out pronto? New PRs are feeling the heat. Let's stick to the branch protection rules and avoid merging when checks are failing.

vasharma05 · 2024-02-21T15:42:30Z

Sure @jayasanka-sack !
I'll fix this first thing in the morning

jayasanka-sack · 2024-02-21T15:43:19Z

Thanks, Vineet! ❤️

denniskigen · 2024-03-26T09:02:19Z

LGTM. I agree with @denniskigen 's suggestions about copy but this is a big PR that's been around for a while already. Requesting his forgiveness and that we fix the copy after merging this.

Thanks so much for all your work on this @vasharma05 !

Coming back to this like the Proverbial grammar Sith lord. Can we fix the copy @brandones @vasharma05?

brandones · 2024-03-28T21:25:57Z

I'll do this after we move workspaces into core.

I have an idea: sometimes workspace titles are a little funny. But we can make workspace names whatever we want; there's no reason they need to be lowercased-and-hyphenated. Why don't we make workspace names exactly what should appear in notifications about that workspace? e.g. the allergy form, which has title "Record a new allergy" could have name "Allergy Form". So then when you try to close it you see "You have unsaved changes in the Allergy Form." What do you think of that, @denniskigen ?

ibacher · 2024-03-29T16:10:33Z

I think the only issue with that is it needs to be translatable, since, presumably, we don’t want to need a separate allergy workspace per language, but I like the idea.

Updated the workspaces.ts to accomodate proper checks when closing wo…

845b05f

…rkspaces

vasharma05 marked this pull request as draft January 24, 2024 06:16

vasharma05 requested review from brandones, denniskigen and ibacher January 24, 2024 06:16

vasharma05 added 6 commits January 24, 2024 11:54

Allergies workspace registered correctly with register workspace

af5f69d

Fixed the closing workspace prompt

c753397

Added the promptBeforeClosing in the Appointments form

a77418d

Added the isDirty check in allergies form

7b949cd

Removed extra translations

b7443a9

Added the isDirty check in conditions workspace

4b59882

brandones reviewed Jan 24, 2024

View reviewed changes

packages/esm-patient-common-lib/src/workspaces/workspaces.ts Outdated Show resolved Hide resolved

brandones reviewed Jan 24, 2024

View reviewed changes

packages/esm-patient-common-lib/src/types/workspace.ts Outdated Show resolved Hide resolved

vasharma05 added 8 commits January 29, 2024 16:58

Added the isDirty check in start-visit workspace

7694a33

Updated the workspaces implementation in the forms app

fc217a5

Updated the workspace title and implementation in the immunization app

ce0f30c

Updated the workspace title and workspace implementation in the patie…

f08149f

…nt lists

Updated the workspace title and workspace implementation in the medic…

ee58d58

…ations app

Updated the workspace title and workspace implementation in the note app

b32fa2e

Updated the workspace title and workspace implementation in the order…

7571d1f

…s app

Updated the workspace title and workspace implementation in the progr…

5dc2ad1

…ams app

ibacher reviewed Jan 29, 2024

View reviewed changes

vasharma05 added 6 commits February 1, 2024 19:15

Updated the workspace title and workspace implementation in the vital…

4c72519

…s app

Added JSDoc in the workspaces.ts

033fc97

Merge branch 'main' of https://www.github.com/openmrs/openmrs-esm-pat…

8e69b41

…ient-chart into feat/O3-2760

Merge branch 'main' of https://www.github.com/openmrs/openmrs-esm-pat…

4665234

…ient-chart into feat/O3-2760

Removed duplicate functions from workspaces.ts

cb67ac6

Exported getWhetherWorkspaceCanBeClosed from common-lib

d71829e

Introduced new function 'discardChangesAndCloseWorkspace' to close wo…

3780617

…rkspaces and keeping the closeWorkspace default behaviour intact

vasharma05 mentioned this pull request Feb 8, 2024

(feat) Add ability to cancel orders #1640

Merged

3 tasks

denniskigen reviewed Feb 9, 2024

View reviewed changes

Updated text

cc55902

Co-authored-by: Dennis Kigen <[email protected]>

vasharma05 added 7 commits February 16, 2024 18:17

Reverted the implementation of discardChangesAndCloseWorkspace

f5c71d0

Prompt function should be removed if the workspace is closed

ef9e258

Merge branch 'main' of https://www.github.com/openmrs/openmrs-esm-pat…

b25b1ee

…ient-chart into feat/O3-2760

Fixed failing test

ac8b76f

Fixed TS issues

17615cf

Merge branch 'feat/O3-2760' of https://www.github.com/openmrs/openmrs…

ac2f597

…-esm-patient-chart into feat/O3-2760

Review changes

dc8b294

brandones reviewed Feb 19, 2024

View reviewed changes

brandones approved these changes Feb 19, 2024

View reviewed changes

vasharma05 added 3 commits February 21, 2024 12:18

Merge branch 'main' of https://www.github.com/openmrs/openmrs-esm-pat…

aea04ec

…ient-chart into feat/O3-2760

Removed appointments workspace

a4c5e3f

Updated translation

a16a39b

vasharma05 merged commit f9d9db4 into main Feb 21, 2024
5 of 6 checks passed

vasharma05 deleted the feat/O3-2760 branch February 21, 2024 07:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat) O3-2760: Present workspaces should leverage the `promptBeforeClosing` function #1613

(feat) O3-2760: Present workspaces should leverage the `promptBeforeClosing` function #1613

vasharma05 commented Jan 24, 2024 •

edited

Loading

ibacher left a comment

usamaidrsk commented Feb 5, 2024

vasharma05 commented Feb 5, 2024

brandones commented Feb 9, 2024

denniskigen Feb 9, 2024

denniskigen Feb 9, 2024

vasharma05 Feb 12, 2024

denniskigen Feb 9, 2024

vasharma05 commented Feb 15, 2024

brandones commented Feb 15, 2024

denniskigen commented Feb 16, 2024

vasharma05 commented Feb 19, 2024

brandones Feb 19, 2024

brandones Feb 19, 2024

brandones Feb 19, 2024

brandones left a comment •

edited

Loading

jayasanka-sack commented Feb 21, 2024 •

edited

Loading

vasharma05 commented Feb 21, 2024

jayasanka-sack commented Feb 21, 2024

denniskigen commented Mar 26, 2024 •

edited

Loading

brandones commented Mar 28, 2024

ibacher commented Mar 29, 2024

	* t('unsavedChangesInOpenedWorkspace', 'You have unsaved changes in the opened workspace. Do you want to discard these changes?')
	* t('unsavedChangesInOpenedWorkspace', 'You have unsaved changes in the currently opened workspace. Do you want to discard these changes?')

	"closingAllWorkspacesPromptBody": "There are unsaved changes in the following workspaces. Do you want to discard changes in the following workspaces? {{workspaceNames}}",
	"closingAllWorkspacesPromptBody": "There are unsaved changes in several workspaces. Do you want to discard changes in the following workspaces? {{workspaceNames}}",

(feat) O3-2760: Present workspaces should leverage the promptBeforeClosing function #1613

(feat) O3-2760: Present workspaces should leverage the promptBeforeClosing function #1613

Conversation

vasharma05 commented Jan 24, 2024 • edited Loading

Requirements

Summary

CloseWorkspace

GetWhetherWorkspaceCanBeClosed

Screenshots

Related Issue

Other

ibacher left a comment

Choose a reason for hiding this comment

usamaidrsk commented Feb 5, 2024

vasharma05 commented Feb 5, 2024

brandones commented Feb 9, 2024

denniskigen Feb 9, 2024

Choose a reason for hiding this comment

denniskigen Feb 9, 2024

Choose a reason for hiding this comment

vasharma05 Feb 12, 2024

Choose a reason for hiding this comment

denniskigen Feb 9, 2024

Choose a reason for hiding this comment

vasharma05 commented Feb 15, 2024

brandones commented Feb 15, 2024

denniskigen commented Feb 16, 2024

vasharma05 commented Feb 19, 2024

brandones Feb 19, 2024

Choose a reason for hiding this comment

brandones Feb 19, 2024

Choose a reason for hiding this comment

brandones Feb 19, 2024

Choose a reason for hiding this comment

brandones left a comment • edited Loading

Choose a reason for hiding this comment

jayasanka-sack commented Feb 21, 2024 • edited Loading

vasharma05 commented Feb 21, 2024

jayasanka-sack commented Feb 21, 2024

denniskigen commented Mar 26, 2024 • edited Loading

brandones commented Mar 28, 2024

ibacher commented Mar 29, 2024

(feat) O3-2760: Present workspaces should leverage the `promptBeforeClosing` function #1613

(feat) O3-2760: Present workspaces should leverage the `promptBeforeClosing` function #1613

vasharma05 commented Jan 24, 2024 •

edited

Loading

brandones left a comment •

edited

Loading

jayasanka-sack commented Feb 21, 2024 •

edited

Loading

denniskigen commented Mar 26, 2024 •

edited

Loading