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

Moved SignInFailed event to MicrosoftGraphService #2205

Merged
merged 14 commits into from
Jul 23, 2018

Conversation

OGcanviz
Copy link
Contributor

@OGcanviz OGcanviz commented Jun 6, 2018

Issue: #
#2171

PR Type

What kind of change does this PR introduce?

Bugfix

Sample app changes

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added/updated which fit documentation template. (for bug fixes / features)
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Contains NO breaking changes

Other information

@OGcanviz
Copy link
Contributor Author

OGcanviz commented Jun 6, 2018

The changes are to suppress the MsalServiceException exceptions in TryLoginAsync and ConnectForAnotherUserAsync, but do we need to handle that in LoginAsync too, which might cause changes in several components.

@@ -45,6 +45,11 @@ MicrosoftGraphService.Instance.Initialize(
MicrosoftGraphEnums.ServicesToInitialize.UserProfile,
PeoplePicker.RequiredDelegatedPermissions
);

MicrosoftGraphService.Instance.SignInFailed += (sender, e) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding this sample in every graph control docs, can you add it once to the Microsoft graph sample page

Copy link
Contributor

Choose a reason for hiding this comment

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

@nmetulev Done

@nmetulev nmetulev added this to the 3.1 milestone Jun 7, 2018
Copy link
Contributor

@azchohfi azchohfi left a comment

Choose a reason for hiding this comment

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

This is not only making the property obsolete, it is breaking the functionality. The SignInFailed event from the AadLogin class is never called anymore, so this is not only now obsolete, it is broken. Obsolete means "don't use anymore, will stop working", but this is broken now.

@alexchx
Copy link
Contributor

alexchx commented Jun 28, 2018

@azchohfi Yes, I missed that, the event AadLogin.SignInFailed can be called now if it's registered.

@alexchx
Copy link
Contributor

alexchx commented Jul 9, 2018

Buddies, any further feedback for this PR? Just let us know if there's any changes to be processed, thank you.

@nmetulev nmetulev modified the milestones: 3.1, 4.0 Jul 11, 2018
@azchohfi azchohfi merged commit a69c60a into CommunityToolkit:master Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants