(feat) O3-2262 & O3-2263: Previously logged in locations should show at the top of the locations' list #954
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…into fix/location-picker-preferred-location
…into fix/location-picker-preferred-location
Closed
4 tasks
|
Size Change: -413 kB (-10%) 👏 Total Size: 3.73 MB
ℹ️ View Unchanged
|
vasharma05
force-pushed
the
feat/O3-2263
branch
from
c70b305
to
33bec2a
Compare
vasharma05
changed the title
…into feat/O3-2263
vasharma05
force-pushed
the
feat/O3-2263
branch
from
3a54046
to
a35f34f
Compare
ibacher
previously requested changes
Mar 22, 2024
packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx
Outdated
Show resolved
Hide resolved
packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx
Outdated
Show resolved
Hide resolved
packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx
Outdated
Show resolved
Hide resolved
packages/apps/esm-login-app/src/location-picker/location-picker.component.tsx
Outdated
Show resolved
Hide resolved
| }, | ||
| [savePreference, defaultLocation, updateUserPropsWithDefaultLocation, t, isUpdateFlow], | ||
| ); | ||
| export function getUpdatedLoggedInLocations(previousLoggedInLocations: string, locationUuid: string): string { |
There was a problem hiding this comment.
This function needs a better name, e.g., getDefaultLoginLocations(). Similarly, I think locationUuid could use a better name, e.g., defaultLocationUuid.
There was a problem hiding this comment.
Hi @ibacher!
Actually this function stores all the logged in locations joined with ,
There was a problem hiding this comment.
I get what the function does. My point was that the name here is both quite long and not very descriptive. getDefaultLoginLocations() is based on my suggestion below to renamed getUserPropertiesWithDefaultAndLogInLocation() to getDefaultLocationUserProperties().
There was a problem hiding this comment.
Also, shouldn't this function ensure the invariant that no more than 10 locations are stored in the updatedLoggedInLocations() (less a concern in newer versions of the backend where user properties can be longer, but there's always a maximum length and truncation will almost certainly lead to invalid UUIDs in the list).
packages/apps/esm-login-app/src/location-picker/location-picker.resource.ts
Outdated
Show resolved
Hide resolved
| return results; | ||
| } | ||
|
|
||
| export function useLocations(useLoginLocationTag: boolean, count: number = 0, searchQuery: string = '') { |
There was a problem hiding this comment.
Since this only pertains to login locations. I'm also unclear on why we need this in the framework itself and not as part of the login app, which is where I think it belongs (there are other uses to locations; these should not be default restricted to login locations and they should not have the same logic that we're using here to surface locations; in other cases where we need locations, it makes sense to surface the location the user is logged into, but not the previous locations they have logged into).
Suggested change
| export function useLocations(useLoginLocationTag: boolean, count: number = 0, searchQuery: string = '') { | |
| export function useLoginLocations(useLoginLocationTag: boolean, count: number = 0, searchQuery: string = '') { |
There was a problem hiding this comment.
it makes sense to surface the location the user is logged into, but not the previous locations they have logged into
I actually thought that this case you mentioned above would have to handled in other places, say location dropdown somewhere else in UI, to use both default(logged in) and previous logged in locations to show at the top, hence using the same hook everywhere else rather than creating this again and again.
Should I move this from framework to login app again?
Also, we can pass the useLoginLocationTag as true/false, hence I believe useLocations is a better name for handling both the cases. Please confirm on this as well.
Thanks!
There was a problem hiding this comment.
So a couple of things here:
- Locations have an entire tagging system, which are intended for a sort of "folksonomy" of locations. That is, it might make sense (for other applications) to tag all pharmacy locations as "Pharmacy" and then, say, in the dispensing app, load those "Pharmacy" locations as the list of valid locations a user can dispense from. The upshot of this, I think, is that a framework-wide
useLocations()hook would take an arbitrarystringfor tags rather than a boolean to indicate whether to use a single tag or not. - I'm not convinced that previous logged in locations are the right thing to display for anything other than previous logged-in locations. For example, if I am logged into Clinic 1 which has the sub-locations Room A, Room B, and Room C, it might make more sense for a form (like say the start visit form) to present me first with Room A, Room B, and Room C, since those are the sub-locations of the place I am currently working.
Both of these kind of speak against this being a useful enough function to include in the framework, especially with the name useLocations().
There was a problem hiding this comment.
What I'm getting at isn't that we shouldn't have a useLocations() hook in the framework. It's that that hook should have different properties from the hook that is specific to login locations. It would be possible to implement a useLoginLocations() hook in terms of a generic useLocations() hook, but implementing a generic useLocations() hook cannot be easily done from a hook that implements logic that mostly makes sense in the context of login locations.
|
Where are we at with this @vasharma05 ? |
|
Hi @brandones ! |
|
@vasharma05 Rename |
|
@vasharma05 Thanks for making the requested changes. Can you resolve the conflicts and we'll get this in? |
ibacher
force-pushed
the
main
branch
2 times, most recently
from
8f994a6
to
9cee659
Compare
|
Ping @vasharma05 , it's so close, just have to resolve the conflicts... |
|
Ping @vasharma05 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
This PR now saves the last 10 logged in locations of the users in the userProperties under the key
previousLoggedInLocationsas a string joined by,. The previous logged in locations are then shown at the top of the list before the rest of the locations.The order of the locations in the list is:
Screenshots
Screen.Recording.2024-03-21.at.11.48.50.mov
Related Issue
https://openmrs.atlassian.net/browse/O3-2262
https://openmrs.atlassian.net/browse/O3-2263
Other
Built over #851