Skip to content
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

process android broker error codes which should result in UI required… #2200

Merged
merged 3 commits into from
Nov 18, 2020

Conversation

jennyf19
Copy link
Collaborator

… exceptions

not sure if this is what you guys want to do here. #2140 also, did not test on a device as I only have my personal pixel and don't want to mess up the authenticator app. :/

@@ -44,5 +44,7 @@ internal static class BrokerResponseConst

//Error codes returned from broker
public const string NoTokenFound = "no_tokens_found";
public const string NoAccountFound = "no_account_found";
public const string InvalidRefreshToken = "Broker refresh token is invalid";
Copy link
Collaborator Author

@jennyf19 jennyf19 Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is legit, i used the error codes from the customer's screen shot. #Resolved

Copy link
Member

@bgavrilMS bgavrilMS Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's legit :( Both Android broker and B2C miss the point of "error code" and send full blown messages :( #Resolved

@@ -79,7 +79,9 @@ private async Task<MsalTokenResponse> SendTokenRequestToBrokerAsync()
_logger.Info(
LogMessages.ErrorReturnedInBrokerResponse(msalTokenResponse.Error));

if (msalTokenResponse.Error == BrokerResponseConst.NoTokenFound)
if (msalTokenResponse.Error == BrokerResponseConst.NoTokenFound ||
Copy link
Member

@bgavrilMS bgavrilMS Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you prefix these constants with "Android"? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.


In reply to: 526108373 [](ancestors = 526108373)

}

[TestMethod]
public void NoAccountFoundThrowsUIRequiredTest()
Copy link
Member

@bgavrilMS bgavrilMS Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that the best place to add this the logic you've added is actually in AndroidBroker.cs; However we can't test that. Having tests beats a marginally better architecture. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the unit testing, especially since not sure how to manually test w/out messing up my authenticator app.


In reply to: 526108961 [](ancestors = 526108961)

Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment about naming

@jennyf19 jennyf19 merged commit 9b50785 into master Nov 18, 2020
@jennyf19 jennyf19 deleted the jennyf/UiExcept branch November 18, 2020 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants