(fix) O3-2768: Display error in case there are no locations #906

trevor-james-nangosha · 2024-01-28T13:49:35Z

Requirements

This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as feat, fix, or chore, among others). See existing PR titles for inspiration.

For changes to apps

My work conforms to the O3 Styleguide and design documentation.

If applicable

My work includes tests or is validated by existing tests.
I have updated the esm-framework mock to reflect any API changes I have made.

Summary

This issue makes sure that an error is shown when no locations are returned from the backend on the location-selection screen

Screenshots

This is a small video that shows what happens in the aforementioned case. I would like to know if I am on the right track, or if I could possibly make some changes. @denniskigen

OpenMRS.-.Google.Chrome.2024-02-02.15-27-28.mp4

Related Issue

Issue is here

Other

ibacher · 2024-02-01T19:36:37Z

This looks ok. Instead of manually rendering a toast (which ends up with a slightly weird UX, use the framework's showToast() method. Also, ideally, the loading indicator would disappear. Instead of "No locations were found for this instance", it should probably say "No locations were found for this system" or something along those lines.

trevor-james-nangosha · 2024-02-02T12:35:03Z

@ibacher I have applied the changes as requested and also updated the video above to show the current state of things.

packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx

trevor-james-nangosha · 2024-02-10T13:21:49Z

Done with the changes as requested @ibacher

denniskigen · 2024-02-15T20:41:50Z

Sorry I'm getting to this late @trevor-james-nangosha @ibacher, but this is a mockup that Ciaran's thrown together for what this error notification could look like:

You could imagine this appearing in place of the search bar in case no locations are configured. Should we just show a subtitle instead of the reload button with something along the lines of Please contact your administrator @ibacher?

ibacher · 2024-02-15T20:43:39Z

Should we just show a subtitle instead of the reload button with something along the lines of Please contact your administrator @ibacher?

Maybe have a subtitle that says "If the issue persists please contact your administrator" and leave the refresh button?

denniskigen · 2024-02-15T20:46:10Z

Would clicking the Reload button just invoke window.location.reload() or would it do something else?

ibacher · 2024-02-19T17:10:37Z

Would clicking the Reload button just invoke window.location.reload() or would it do something else?

Presumably we could just call the mutator for SWR rather than doing a full page reload? This isn't a "component is broken" error, just a "data is missing error".

@trevor-james-nangosha To answer you're question from Slack, the image is just a sketch to show the inline notification. Leave the rest of the elements on the page.

packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx

jayasanka-sack · 2024-02-19T17:17:17Z

Isn't the fallback for no locations is to Login without a location? As I can remember that's how the location picker was written.

jayasanka-sack · 2024-02-19T17:18:54Z

packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx

+          title: t('error', 'Error'),
+          kind: 'error',
+          description: 'No locations were found for this system. Please contact your administrator',
+        });
        changeLocation();


We don't need to call this function, right?

Yea, did not see that. Will remove it!! Thanks.

@jayasanka-sack @vasharma05 What's up with this, should we remove this function call?

ibacher · 2024-02-20T01:09:18Z

Isn't the fallback for no locations is to Login without a location?

I think that was what was originally implemented just to get something in. Most of the code in various places assumes that there's a session location and, while the backend technically supports logins without locations, the RefApp hasn't since O2.

Looking at this from a slightly different angle: we can either stop the user up-front with no locations or we can write tests for every place that accesses the session location to ensure they continue to function if there is no location available.

mseaton · 2024-02-23T03:30:21Z

Looking at this from a slightly different angle: we can either stop the user up-front with no locations or we can write tests for every place that accesses the session location to ensure they continue to function if there is no location available

I do think this may be worth a follow-up discussion to confirm that we definitely want to make session location required in esm-core and the location picker, rather than just configured to be required within the refapp implementation of O3.

trevor-james-nangosha · 2024-02-25T06:40:28Z

Screenshot showing behavior.

use showToast instead of showNotification for error

suubi-joshua · 2024-03-19T13:33:41Z

Screenshot showing behavior.

Great job @trevor-james-nangosha but I think I think if the locations don't load the confirm button should not show neither should the checker for remember my location.

vasharma05 · 2024-03-22T05:45:07Z

packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx

@@ -16,6 +16,7 @@ import { useLoginLocations } from '../login.resource';
 import styles from './location-picker.scss';
 import { useDefaultLocation } from './location-picker.resource';
 import type { ConfigSchema } from '../config-schema';
+import { Snackbar } from '@openmrs/esm-styleguide/src/snackbars/snackbar.component';


import { showSnackbar } from "@openmrs/esm-framework"

vasharma05 · 2024-03-22T05:45:57Z

packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx


      const referrer = state?.referrer;
      const returnToUrl = new URLSearchParams(location?.search).get('returnToUrl');

-      const sessionDefined = locationUuid ? setSessionLocation(locationUuid, new AbortController()) : Promise.resolve();
+      const sessionDefined = locationUuid ? setSessionLocation(locationUuid, new AbortController()) : Promise.reject();


Since we have the check above for undefined/null locationUuid, why add this unnecessary promise here?

vasharma05 · 2024-03-22T05:46:58Z

packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx

+            <Snackbar
+              snackbar={{
+                id: 1,
+                actionButtonLabel: t('reload', 'Reload'),
+                onActionButtonClick: () => mutate(),
+                kind: 'error',
+                title: t('locationsFailedToLoad', 'Locations failed to load'),
+                subtitle: t(
+                  'locationIssueContactAdministrator',
+                  'If the issue persists please contact your administrator',
+                ),
+              }}
+              closeSnackbar={() => {}}
+            />


Snackbar is a notification, it's not a component to be shown in place of search.
Please read here for more info

We can use an ActionableSnackbar, using the props actionButtonLabel and onActionButtonClick in the showSnackbar. Learn more about showSnackbar here:

openmrs-esm-core/packages/framework/esm-styleguide/src/snackbars/index.tsx

Line 32 in 1306721

export function showSnackbar(snackbar: SnackbarDescriptor) {

vasharma05 · 2024-03-22T05:51:43Z

packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx

-      if (!locations?.length) {
-        changeLocation();
-      }


if (!error && !locations?.length) { changeLocation(); }

@ibacher, do we have any implementation at all where there are no locations?
Thanks!

It's not impossible, but I would say that that's a configuration error. Every functioning OMRS system should have at least one location.

…into issueO3_2768

vasharma05 · 2024-03-22T06:27:19Z

Updated error screen:

vasharma05 · 2024-03-22T06:35:09Z

Re-requesting review from @ibacher @samuelmale @denniskigen.
Thanks!

brandones · 2024-04-19T16:05:44Z

packages/framework/esm-translations/src/translations.ts

@@ -20,6 +20,7 @@ export const coreTranslations = {
  change: 'Change',
  close: 'Close',
  confirm: 'Confirm',
+  contactAdministratorIfIssuePersists: 'If the problem persists contact your system administrator.',


Suggested change

contactAdministratorIfIssuePersists: 'If the problem persists contact your system administrator.',

contactAdministratorIfIssuePersists: 'Contact your system administrator if the problem persists.',

brandones · 2024-04-19T16:07:55Z

Based on the designs it looks like we can do away with the top "Welcome" block.

ibacher · 2024-04-19T18:00:21Z

Based on the designs it looks like we can do away with the top "Welcome" block.

I thought the design there was just a sketch of the notification, not a full screen preview. I kind of prefer it with the "Welcome" block because it makes it look more like what it is (an unexpected error in one part of the system) rather than just a basically blank page.

brandones · 2024-04-19T20:52:18Z

That makes sense, thanks @ibacher . I think you're right.

brandones

I have only minor feedback; feel free to ship this when you feel happy with it @vasharma05 .

trevor-james-nangosha marked this pull request as ready for review February 2, 2024 12:35

ibacher requested changes Feb 2, 2024

View reviewed changes

trevor-james-nangosha force-pushed the issueO3_2768 branch from 0c4b494 to 20b5222 Compare February 3, 2024 08:24

denniskigen requested a review from ibacher February 9, 2024 06:39

ibacher reviewed Feb 9, 2024

View reviewed changes

denniskigen requested a review from ibacher February 15, 2024 10:37

denniskigen changed the title ~~(fix)o3-2768 Display error in case there are no locations~~ (fix) O3-2768: Display error in case there are no locations Feb 15, 2024

ibacher reviewed Feb 19, 2024

View reviewed changes

packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx Outdated Show resolved Hide resolved

jayasanka-sack reviewed Feb 19, 2024

View reviewed changes

trevor-james-nangosha added 5 commits February 25, 2024 10:09

Display error in case of no locations

b3c02d2

Some small changes as per review request

e062fab

Do not display search box in case of no locations

b82d714

use showToast instead of showNotification for error

Do nothing in changeLocation if locationUuid is not set

d6a758a

Run SWR mutate function when reload is clicked

23727dd

trevor-james-nangosha force-pushed the issueO3_2768 branch from 5488a67 to 23727dd Compare February 25, 2024 07:23

denniskigen requested review from ibacher and jayasanka-sack February 26, 2024 07:06

vasharma05 requested changes Mar 22, 2024

View reviewed changes

vasharma05 reviewed Mar 22, 2024

View reviewed changes

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

1324afe

…into issueO3_2768

vasharma05 added 3 commits March 22, 2024 12:01

Added inline notification displaying action to reload data

b55424f

Updated translations

7fdb791

Restored changeLocation

1ca7472

vasharma05 requested a review from samuelmale March 22, 2024 06:34

Fixed the url

4baa250

brandones reviewed Apr 19, 2024

View reviewed changes

brandones approved these changes Apr 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(fix) O3-2768: Display error in case there are no locations #906

(fix) O3-2768: Display error in case there are no locations #906

trevor-james-nangosha commented Jan 28, 2024 •

edited by denniskigen

ibacher commented Feb 1, 2024

trevor-james-nangosha commented Feb 2, 2024

trevor-james-nangosha commented Feb 10, 2024

denniskigen commented Feb 15, 2024

ibacher commented Feb 15, 2024

denniskigen commented Feb 15, 2024 •

edited

ibacher commented Feb 19, 2024

jayasanka-sack commented Feb 19, 2024

jayasanka-sack Feb 19, 2024

trevor-james-nangosha Feb 24, 2024

brandones Apr 19, 2024

ibacher commented Feb 20, 2024 •

edited

mseaton commented Feb 23, 2024

trevor-james-nangosha commented Feb 25, 2024

suubi-joshua commented Mar 19, 2024

vasharma05 Mar 22, 2024

vasharma05 Mar 22, 2024

vasharma05 Mar 22, 2024

vasharma05 Mar 22, 2024

vasharma05 Mar 22, 2024

vasharma05 Mar 22, 2024

ibacher Apr 19, 2024

vasharma05 commented Mar 22, 2024 •

edited by denniskigen

vasharma05 commented Mar 22, 2024

brandones Apr 19, 2024 •

edited by denniskigen

brandones commented Apr 19, 2024

ibacher commented Apr 19, 2024

brandones commented Apr 19, 2024

brandones left a comment

	contactAdministratorIfIssuePersists: 'If the problem persists contact your system administrator.',
	contactAdministratorIfIssuePersists: 'Contact your system administrator if the problem persists.',

(fix) O3-2768: Display error in case there are no locations #906

Are you sure you want to change the base?

(fix) O3-2768: Display error in case there are no locations #906

Conversation

trevor-james-nangosha commented Jan 28, 2024 • edited by denniskigen

Requirements

For changes to apps

If applicable

Summary

Screenshots

Related Issue

Other

ibacher commented Feb 1, 2024

trevor-james-nangosha commented Feb 2, 2024

trevor-james-nangosha commented Feb 10, 2024

denniskigen commented Feb 15, 2024

ibacher commented Feb 15, 2024

denniskigen commented Feb 15, 2024 • edited

ibacher commented Feb 19, 2024

jayasanka-sack commented Feb 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher commented Feb 20, 2024 • edited

mseaton commented Feb 23, 2024

trevor-james-nangosha commented Feb 25, 2024

suubi-joshua commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasharma05 commented Mar 22, 2024 • edited by denniskigen

vasharma05 commented Mar 22, 2024

brandones Apr 19, 2024 • edited by denniskigen

Choose a reason for hiding this comment

brandones commented Apr 19, 2024

ibacher commented Apr 19, 2024

brandones commented Apr 19, 2024

brandones left a comment

Choose a reason for hiding this comment

trevor-james-nangosha commented Jan 28, 2024 •

edited by denniskigen

denniskigen commented Feb 15, 2024 •

edited

ibacher commented Feb 20, 2024 •

edited

vasharma05 commented Mar 22, 2024 •

edited by denniskigen

brandones Apr 19, 2024 •

edited by denniskigen