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

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

Merged
merged 12 commits into from
Jun 4, 2024
Merged

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

merged 12 commits into from
Jun 4, 2024

Conversation

trevor-james-nangosha
Copy link
Contributor

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

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

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
Copy link
Member

ibacher commented Feb 1, 2024

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
Copy link
Contributor Author

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

@trevor-james-nangosha trevor-james-nangosha marked this pull request as ready for review February 2, 2024 12:35
@trevor-james-nangosha
Copy link
Contributor Author

Done with the changes as requested @ibacher

@denniskigen
Copy link
Member

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:

Screenshot 2024-02-15 at 11 50 18

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
Copy link
Member

ibacher commented Feb 15, 2024

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
Copy link
Member

denniskigen commented Feb 15, 2024 •
edited

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

@denniskigen 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
Copy link
Member

ibacher commented Feb 19, 2024

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.

@jayasanka-sack
Copy link
Member

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
jayasanka-sack reviewed Feb 19, 2024
View reviewed changes
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();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@ibacher
Copy link
Member

ibacher commented Feb 20, 2024 •
edited

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
Copy link
Member

mseaton commented Feb 23, 2024

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
Copy link
Contributor Author

Screenshot showing behavior.
Screenshot (22)

@suubi-joshua
Copy link
Contributor

Screenshot showing behavior. Screenshot (22)

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
Copy link
Member

vasharma05 commented Mar 22, 2024 •
edited by denniskigen

Updated error screen:

image

@vasharma05
Copy link
Member

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

@brandones
Copy link
Collaborator

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

image

@ibacher
Copy link
Member

ibacher commented Apr 19, 2024

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
Copy link
Collaborator

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

brandones
brandones approved these changes Apr 19, 2024
View reviewed changes
Copy link
Collaborator

@brandones brandones left a comment

Choose a reason for hiding this comment

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

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

@brandones brandones dismissed stale reviews from vasharma05 and ibacher June 4, 2024 19:22

Requests were addressed

@brandones brandones merged commit 066cfd0 into openmrs:main Jun 4, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vasharma05 vasharma05 vasharma05 left review comments

@brandones brandones brandones approved these changes

@ibacher ibacher Awaiting requested review from ibacher

@jayasanka-sack jayasanka-sack Awaiting requested review from jayasanka-sack

@samuelmale samuelmale Awaiting requested review from samuelmale

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
9 participants
@trevor-james-nangosha @ibacher @denniskigen @jayasanka-sack @mseaton @suubi-joshua @vasharma05 @brandones @samuelmale