-
Notifications
You must be signed in to change notification settings - Fork 248
Updated GIDSignIn + GIDSignInInternalOptions Implementations + Unit Tests #553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updated GIDSignIn + GIDSignInInternalOptions Implementations + Unit Tests #553
Conversation
GoogleSignIn/Sources/GIDSignIn.m
Outdated
| } | ||
| } | ||
|
|
||
| - (BOOL)processTokenClaimsForOptions:(GIDSignInInternalOptions *)options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be following an existing pattern in this file, but this method "works" by relying on side effect behavior, which is not great. The side effect, setting the token claims JSON on the passed options, is separated from the return, a BOOL. IMHO side effects like this make code harder to read (e.g., in the first snippet pasted below).
So, you're code above would go from:
// If tokenClaims are invalid or JSON serialization fails, return with an error.
if (![self processTokenClaimsForOptions:options error:&claimsError]) {
if (options.completion) {
self->_currentOptions = nil;
dispatch_async(dispatch_get_main_queue(), ^{
options.completion(nil, claimsError);
});
}
return;
}
To this:
// If tokenClaims are invalid or JSON serialization fails, return with an error.
options.tokenClaimsAsJSON = [self processTokenClaimsForOptions:options error:&claimsError])
if (options.completion && /* Check that token claims are good on options; otherwise, set completion with error */) {
self->_currentOptions = nil;
dispatch_async(dispatch_get_main_queue(), ^{
options.completion(nil, claimsError);
});
return;
}
Wouldn't it be simpler to just return the token claims json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. I have updated the function accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we even need processTokenClaimsForOptions.
|
|
||
| if (tokenClaimsError) { | ||
| XCTestExpectation *tokenClaimsErrorExpectation = | ||
| [self expectationWithDescription:@"Callback called"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not reuse "Callback called" since it's extremely vague. Please use something more descriptive like "Token claims error expectation."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this expectation altogether.
| } | ||
|
|
||
| if (tokenClaimsError) { | ||
| XCTestExpectation *tokenClaimsErrorExpectation = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused by this...you make an expectation and then fulfill it right away? Can you help me to understand what is going on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated this condition.
…kenResponse + Testing
…to fatIDTokenWithAuthTime
This pull request updates the existing implementation of
GIDSignInclass andGIDSignInInternalOptionsclass.Key changes:
signWith...initializers to provide public api's to request for tokenClaims.GIDSignInInternalOptionsto store tokenClaims.OIDAuthorizationRequest.