SF-3292 Re-pull auth profile if user not in SF#3979
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3979 +/- ##
==========================================
- Coverage 81.04% 81.03% -0.01%
==========================================
Files 645 645
Lines 41525 41531 +6
Branches 6740 6762 +22
==========================================
+ Hits 33652 33656 +4
+ Misses 6774 6760 -14
- Partials 1099 1115 +16 ☔ View full report in Codecov by Harness. |
|
📸 Screenshot diff deployed! (1 change) View the visual diff at: https://pr-3979--sf-screenshot-diffs.netlify.app |
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami reviewed 10 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/app/app.component.ts line 317 at r1 (raw file):
const isLoggedIn = await this.authService.isLoggedIn; if (isLoggedIn && this.currentUserDoc.data == null) { await this.authService.pullAuthUserProfile();
Seems like if we hit this block, we should await the user doc loading before continuing. Otherwise the rest of the method will continue to operate with a doc that doesn't have its data loaded. Unless I'm misreading something?
I think it could be done with:
await this.currentUserDoc.onlineFetch();src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/checking_en.json line 383 at r1 (raw file):
"project_link_is_invalid": "The project link is invalid. Please contact the person who sent you the link and ask for a new one.", "role_not_found": "The role you were assigned on this project is no longer available. Please contact the person who sent you the link and ask for a new one.", "user_missing": "Your user account could not be found. Please log out join using the link again.",
There's a word missing from this sentence.
Also, is it necessary to show this to a user, or could we just re-call the endpoint?
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/auth.service.ts line 287 at r1 (raw file):
pullAuthUserProfile(): Promise<void> { return this.commandService.onlineInvoke(USERS_URL, 'pullAuthUserProfile');
Good improvement. I was going to say I think this should be added to the users service, but I think we can't do that without creating a circular dependency. (UserService currently depends on AuthService)
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman made 2 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Nateowami).
src/SIL.XForge.Scripture/ClientApp/src/app/app.component.ts line 317 at r1 (raw file):
Previously, Nateowami wrote…
Seems like if we hit this block, we should await the user doc loading before continuing. Otherwise the rest of the method will continue to operate with a doc that doesn't have its data loaded. Unless I'm misreading something?
I think it could be done with:
await this.currentUserDoc.onlineFetch();
Done. I thought the call to getCurrentUser() above this will handle that via the subscriber(), but that will cause the update to occur asynchronously, and so just skip the interface language update.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/checking_en.json line 383 at r1 (raw file):
There's a word missing from this sentence.
Done.
Also, is it necessary to show this to a user, or could we just re-call the endpoint?
I think it is necessary, as the up to two attempts by the frontend to talk to Auth0 and create the user will have failed for this message to be displayed.
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on pmachapman).
I have marked this as testing not required, as it must be tested by a developer to recreate the breaking of the auth profile pulling that caused this bug to occur.
This change is