[BACK-2780] Add new user profiles endpoint. #698
Conversation
lostlevels
force-pushed
the
jimmy/BACK-2780-new-profiles-endpoint
branch
from
5abaa66
to
7d32881
Compare
sending as `{"patient":{"about": "..."}}`.
search. Fix profile to legacy seagull mapping.
"isOtherPerson" to legacy response.
There was a problem hiding this comment.
Overall it looks good. I found few issues:
- There is a lot of cruft that was copied from shoreline. I believe the majority of those functions are not used. There's no reason to keep unused code in platform, because it only leads to confusion.
- There is probably a gap in the requirements that I discovered when I read the seagull code to try and figure out why the profile endpoints require custodian permissions. In order to fully deprecate and migrate existing seagull profiles from Mongo to Keycloak, we have to provide an alternative implementation of this functionality. For each user sharing their data with a user with a given id seagull fetches the user object from shoreline and merges it with the user profile attributes stored in the profile object in mongo. The response is sanitized depending on the actual permissions of the requesting user. This deserves its own ticket, but please do some research what the linked code does and then write the requirements. The alternative implementation should be much simpler, because the user account and profiles will be stored in a single service (Keycloak).
auth/service/api/v1/profile.go
Outdated
| results := make([]*user.User, 0, len(mergedUserPerms)) | ||
| // just doing sequentially fetching of users for now | ||
| for userID, trustPerms := range filteredUserPerms { | ||
| // Does this mean all users should already be migrated |
There was a problem hiding this comment.
Yes, we should migrate all users to mongo
| return slices.Contains(TRUES, value) | ||
| } | ||
|
|
||
| func parseUsersQuery(query url.Values) *usersProfileFilter { |
There was a problem hiding this comment.
My impression is that this query is not used at all. Can we check blip and uploader?
There was a problem hiding this comment.
On checking blip and uploader it seems they are indeed no query string parameters used.
blip
platform-client
uploader
uploader
Can you confirm that no query string parameters are passed for getAssociatedUsersDetails @krystophv @gniezen @jh-bate ?
There was a problem hiding this comment.
The blip and uploader calls are all piped through the platform-client function, so I believe you're correct - no query string params are being sent.
| profile = profile.ClearPatientInfo() | ||
| } else { | ||
| if trustorPerms.HasAny(permission.Custodian, permission.Read, permission.Write) { | ||
| // TODO: need to read seagull.value.settings - confirm this is actually used |
There was a problem hiding this comment.
Settings are not part of the profile object so probably there's no need to do anything?
There was a problem hiding this comment.
I see the settings and preferences returned in the old seagull code, but don't know if they are actually used by clients. @krystophv @clintonium-119 @gniezen Do you know if the returned result of paltform-client getAssociatedUsersDetails ever uses the returned users' settings or preferences in blip or uploader?
| // TODO: need to read seagull.value.settings - confirm this is actually used | ||
| } | ||
| if trustorPerms.Has(permission.Custodian) { | ||
| // TODO: need to read seagull.value.preferences - confirm this is actually used |
There was a problem hiding this comment.
Preferences are not part of the profile object so probably there's no need to do anything?
null fields work properly w/o changes.
Since a lot of this is copy paste from shoreline, for reviewers:
shoreline/user/user.go=>platform/user/full_user.go- (because there's already a type calledUser).platform/user.gouser.Userextended with shoreline user fields.shoreline/user/hasher.go=>platform/user/hasher.goshoreline/user/storage.go=>platform/user/user_accessor.go(somewhat like the interface of the Storage repository, but simplified).shoreline/user/migrationStore.go=>platform/user/keycloak/user_accessor.go(Takes the logic around User creation / manipulation and removes the fallback / mongodb stuff), implementsplatform/user.UserAccessorinterfaceshoreline/keycloak/client.go=>platform/user/keycloak/client.go(Wrapper around gocloak).