(feat) O3-2790: Create a reusable location picker component

[jayasanka-sack]

Requirements

• [x] 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

• [x] My work conforms to the O3 Styleguide and design documentation.

If applicable

• [x] 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 PR introduces a significant enhancement by establishing a reusable location picker through the relocation of the current picker to our styleguide. The key modifications include:

• Component Refactoring:
  • Extracted the existing location picker from the login process.
  • Developed a new, standalone component that functions as a versatile location picker.
• Bug fixes:
  • Fix https://openmrs.atlassian.net/browse/O3-2759
• Other:
  • Designed the location picker to accommodate a default location UUID. Ensured that the default location appears at the top of the list, even if it falls outside the initial set of loaded locations.
  • Updated the component to not to show the default location if the search results doesn't contain the default location
  • Write and update existing test cases

Screenshots

No visual changes

Related Issue

https://openmrs.atlassian.net/browse/O3-2790

Other

[jayasanka-sack]

@ibacher The e2e test is failing for this PR as we don't package esm-framework. What can we do?

https://github.com/openmrs/openmrs-esm-core/blob/main/e2e/support/github/run-e2e-docker-env.sh

[ibacher]

Several things:

• What connections does this PR (move the location picker to the styleguide) have with the ticket (additional locations not loading in Chrome-based browsers)?
• I don't think data-fetching components, in general, belong in the styleguide without a strong justification. I'm having trouble seeing where else this component is useful.
• In general, we do not want to end up with a large component library because that becomes a maintenance headache.

[jayasanka-sack]

Thanks for your thoughtful review, @ibacher. Let me try address your queries here:

• The main ticket for this PR is O3-2790. It fixes O3-2759 as well.
• The idea for moving it to the style guide came from this this bug O3-2610. We came to this conclusion on the O3 lead call 2 weeks ago.
• The location picker is used on
  • Login
  • Programs
  • Start / edit visit
  • Form Engine
  • Appoinments
  • Cohort builder
• Each of these places has it's own location picker implementation. Apart from the login, all other places built with the false assumption that location API load all the locations at once. Some of them (ex: programs, appointment) has bad UX and produce some unexpected results like O3-2610.

I appreciate your feedback and would love to hear your thoughts. :)

[ibacher]

@jayasanka-sack So, I'm on-board with a component for viewing locations. I'm less keen on the data-fetching part living in the styleguide. That is, I think the fetching should be handled by a component that mounts this component and passed in as props rather than having the fetch logic itself in the styleguide. There might be a case for putting those hooks in esm-react-utils...

There are issues with components in the styleguide, like the translation system won't work nor will the configuration system. These need to come via props to the component. Components in the styleguide should really just be, effectively the HTML markup and component behaviour (like drop-downs or something).

[ibacher]

And, yeah, for the e2e tests in core, we also need to run the build step.

[brandones]

~Yeah let's please file a ticket specifically about this, under https://openmrs.atlassian.net/browse/O3-1422. Do O3-2790 separately.~ Whoops just saw that you changed the ticket, sorry I got them mixed up! I've added O3-2790 to the styleguide components epic.

Please also document the places in the code that this location picker would be used. What existing location pickers exist throughout O3? It's important to make sure that styleguide components are very well designed in terms of being adequate for current use cases (while being minimalist in their API and simple in their implementation).

I don't have strong feelings about whether the data fetching is included or not. I'm inclined to say that whether it should be included should depend on whether users of the component will want control over the data that gets used. For example, the Patient Banner Contact Details component fetches the patient object; this is fine because it would be very strange for someone to want to customize the patient data being used. But here, there is already the location tag prop, which indicates some need for customization. On the other hand, Infinite Scroll requires a pretty high degree of coupling between the UI and data fetcher. So it might be hard to design a good component API where the data is provided by the component user.

[brandones]

We do have esm-styleguide configuration—importantly, if the location picker uses that, it will be the same across the app.

And we now support core translations, as you all know :)

[brandones]

@jayasanka-sack I have merged this with the latest. Please give it a look over to see if you think I messed anything up. It was an extremely messy merge.

[brandones]

Also, the build is failing. Unfortunately it is a pretty difficult error to debug. @jayasanka-sack will you have time to work on this and get this in?

Once the build is passing I would support merging this in, if @ibacher agrees. I like the approach you've taken and think you did a great job with it. Unless there are reasons we might want the function-user to be able to customize the list of locations going in to the picker, I think having the picker handle the fetching/paging/etc itself is the right call.

[jayasanka-sack]

Thanks @brandones ! Let me try to fix this.

[brandones]

@jayasanka-sack Ping, any news or blockers to report?

[jayasanka-sack]

@brandones I tried to fix this earlier, but this seemed to take time to fix, so I had to deprioritize it in favor of the load testing work. I'll try working on it again.

[jayasanka-sack]

@brandones I fixed the issue: it was with this line: https://github.com/openmrs/openmrs-esm-core/blob/57f86e981f2907d5a1c31acaa6449998a6cb656d/packages/apps/esm-login-app/src/location-picker/location-picker.test.tsx#L47

[brandones]

@jayasanka-sack Nice! Is this the same issue?

@openmrs/esm-primary-navigation-app:test
cache miss, executing 5f88d48b99dbf1e6
 PASS  src/components/logo/logo.test.tsx (10.834 s)
 FAIL  src/root.component.test.tsx
  ● Test suite failed to run

    ReferenceError: Cannot access 'importDynamic' before initialization

      14 | export * from '@openmrs/esm-state/mock';
      15 | export * from '@openmrs/esm-styleguide/mock';
    > 16 | export * from '@openmrs/esm-translations/mock';
         |         ^
      17 |
      18 | /* esm-globals */
      19 |

Thanks for your continued work on this!

[jayasanka-sack]

@jayasanka-sack Nice! Is this the same issue?

Yes. fixed with: https://github.com/openmrs/openmrs-esm-core/pull/913/commits/17bb8c4f31be07864a229d9a90442d8a30901994

[brandones]

Whoops, same problem, other tests...

[jayasanka-sack]

@brandones We had a call yesterday on the O3 leads call, and we thought of first finding out why they are failing only on this PR before fixing the anti-pattern usage for mocks. @ibacher were you able to find any clue?

[jayasanka-sack]

@ibacher, were you able to find the root cause by any chance?

[brandones]

:tada: :tada: :tada: Thank you @jayasanka-sack and @ibacher on getting this through!!!

[jayasanka-sack]

Thanks a ton @brandones @ibacher! This is such a big relief!!

[jayasanka-sack]

Oops! The login E2E fails. I thought it was because we don't package esm-framework for PRs. Let me get it fixed

[jayasanka-sack]

It was failing for the same reason since it takes a bit of time to get the esm framework published to the registry, and the e2e job uses whatever the latest version. It's passing now!

Update: I was wrong. I was looking at the wrong job. I'll try to figure out what's wrong.

[ibacher]

@jayasanka-sack The ultimate issue, AFAICT, with E2E tests and framework changes is that on the Docker image, we rely on the latest published version of the app-shell (basically this stuff). The problem is that version of the app-shell will be linked to the old version of the framework and so stuff fails.

[jayasanka-sack]

Thanks @ibacher! Isn't it because we build the container on top of the openmrs-reference-application-3-frontend:nightly image that already has the framework on it, and it takes some time to build the image in Bamboo and publish it to docker hub?

https://github.com/openmrs/openmrs-esm-core/blob/08fe1eab4e7a67fedca9947e1d15f50df48cde2d/e2e/support/github/Dockerfile#L15

[ibacher]

Yeah, that's also a factor there... I mean, for a PR, though, the image will never be updated until the PR is merged.

[jayasanka-sack]

for a PR, though, the image will never be updated until the PR is merged.

I agree.

I was referring to this : https://github.com/openmrs/openmrs-esm-core/actions/runs/10181254724/attempts/1