Conversation
yatharthranjan
changed the title
yatharthranjan
changed the title
|
@yatharthranjan The solution looks good. Did you test it? Should I test it as well? |
|
@pvannierop I have not tested this yet. would be great if you can test it. Thanks. Also, wonder if we should only do this check if the other existing user is authorized=true and hasValidToken() |
this-Aditya
left a comment
this-Aditya
left a comment
There was a problem hiding this comment.
Hi @yatharthranjan, kindly review the comments inline, the pr seems fine otherwise.
| * Should be called before updating a user's token with an external user ID. | ||
| */ | ||
| private suspend fun validateExternalUserId(token: RestOauth2AccessToken?, user: RestSourceUser) { | ||
| if (token?.externalUserId != null) { |
There was a problem hiding this comment.
Should we add an else block to handle when externalUserId is null?
| ) = asyncService.runAsCoroutine(asyncResponse) { | ||
| val registration = registrationService.ensureRegistration(token) | ||
| val accessToken = authorizationService.requestAccessToken(payload, registration.user.sourceType) | ||
| val user = userRepository.updateToken(accessToken, registration.user) |
There was a problem hiding this comment.
Since we are replacing userRepository::updateToken, the variable userRepository is now unused and can be removed
There was a problem hiding this comment.
Thanks for reviewing this. Removed now.
|
Should I merge this, or is there any work left? |
Yes I think we can merge. |
4 participants
Attempts to fix #305