Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

@grokys
Copy link
Contributor

@grokys grokys commented Aug 9, 2018

When a login fails at VS startup, currently we just remove the connection from the TE connect page and pretend it never existed. With this PR, we show a message and allow the user to retry (in the case of non-auth-related) errors or sign out:

General connection error:

image

Auth error:

image

In addition, an error is shown in the GitHub pane with a link to open the Team Explorer Connect section to fix the problem:

image

The UI needs some love from @donokuda , but this is ready to play with!

Fixes #1305

grokys added 2 commits August 9, 2018 16:02
When a login error occurs, display a message in our Team Explorer Connect pane with a Rety and Sign Out button.
@grokys grokys added the WIP label Aug 9, 2018
@grokys grokys changed the title WIP: Better failure modes Better failure modes Aug 10, 2018
@grokys grokys self-assigned this Aug 10, 2018
@meaghanlewis
Copy link
Contributor

Tested this by changing my GitHub for Visual Studio credentials in the credential manager to be incorrect and by using the proxy server Burp.

@donokuda if you need any help testing your changes locally, let me know.

This LGTM, thanks @grokys! ✨

Copy link
Contributor

@StanleyGoldman StanleyGoldman left a comment

Choose a reason for hiding this comment

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

nameof maybe?


void InvitationSectionPropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == "IsVisible")
Copy link
Contributor

Choose a reason for hiding this comment

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

nameof maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was trying to stick with the style of the previous method which didn't use nameof, but you're right. I've converted this method and the previous to use nameof.

Copy link
Contributor

@StanleyGoldman StanleyGoldman left a comment

Choose a reason for hiding this comment

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

else if?

}
else

if (notGitHubRepo)
Copy link
Contributor

Choose a reason for hiding this comment

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

else if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this flag is set in the preceding block.

Copy link
Contributor

@StanleyGoldman StanleyGoldman left a comment

Choose a reason for hiding this comment

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

Minor nits, otherwise cool..

@meaghanlewis meaghanlewis merged commit 208c34e into master Aug 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide better failure modes when a connection error occurs

4 participants