Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Conversation

@StanleyGoldman
Copy link
Contributor

@StanleyGoldman StanleyGoldman commented Sep 11, 2017

Currently our only exposure to the API is PublishView. In the event of an authentication failure we need to switch the PublishView to the AuthenticationView

  • If the user's authentication is not who we expect
    img

-If there is no authentication present
img

  • Sign In vs Publish
    img

Depends on:

@StanleyGoldman
Copy link
Contributor Author

Do we need message to explain to the user that we need your authentication?
@shana @donokuda

@donokuda
Copy link
Contributor

Do we need message to explain to the user that we need your authentication?

Yup, I'm 👍 in favor of explaining why they're taken to the authentication screen. Any strong feelings about checking first if they're signed in before displaying the publish form? If they're not logged in, then we could show them the sign-in form with a little error up on top:

I can push up an example branch if that helps!

@StanleyGoldman
Copy link
Contributor Author

Cool. Thanks for the draft.
I think I got it, I'll let you know if I need more.

@StanleyGoldman StanleyGoldman force-pushed the fixes/api-failure-switch-auth-view branch from dc579f2 to 90d1f0c Compare September 12, 2017 20:19
@donokuda
Copy link
Contributor

Added back the header styles:

var usernameMismatchException = exception as TokenUsernameMismatchException;
if (usernameMismatchException != null)
{
message = "Your credentials need to be refreshed";
Copy link
Member

Choose a reason for hiding this comment

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

Been thinking about this some more and talking to some people about what messages should say, and we should probably explain a bit more why we're doing this.

How about:

We've detected that your stored credentials are out of sync with your current user. This can happen if you have signed in to git outside of Unity. Sign in again to refresh your credentials.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds much better.


if (usernameMismatchException == null && keychainEmptyException == null)
{
message = "There was an error validating your account";
Copy link
Member

Choose a reason for hiding this comment

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

All of these messages should be in const fields somewhere so we can localize them later


if (usernameMismatchException == null && keychainEmptyException == null)
{
message = "There was an error validating your account";
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this message is a bit unhelpful, especially given that it's the first thing they see right after clicking the button. When does this happen and is this our fault or the users'? If it's our fault, then we should followup with "Please sign in again" to tell them how to proceed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually hasn't happened to me yet, so it would probably be best to leave the original exception message there instead.

@StanleyGoldman
Copy link
Contributor Author

@donokuda I feel like the current auth view does not match the publish view. I think it was different before because you would completely leave the auth view and then separately enter the publish view. After these changes that will no longer be the case, one will flow into another.

Current Auth View:
img

Current Publish View:
img

Maybe it's just the lack of an invertocat that makes me feel like it's missing.

@donokuda
Copy link
Contributor

Here's where this PR is at right now. It's ready for some feedback:

Sign in view with warning:

screen shot 2017-10-27 at 9 18 45 am

2FA view (is there a way to suppress that error if this is the first time viewing this screen?)

screen shot 2017-10-27 at 9 24 19 am

I also noticed when you hit "back" from the 2FA, it will still persist the error message even though it's not relevant to the screen before. Is there a way to prevent that from happening?

screen shot 2017-10-27 at 9 30 21 am

…t-objects

Preventing the exposure of Octokit objects to the codebase
@shana
Copy link
Member

shana commented Nov 2, 2017

I like this UI a lot, those message box styles rock!

I also noticed when you hit "back" from the 2FA, it will still persist the error message even though it's not relevant to the screen before. Is there a way to prevent that from happening?

We probably need to call Repaint/Redraw on the window to refesh the UI here

I wonder how we handle the Esc key. We should probably close the window and cancel the whole thing.


if (shouldProceed)
{
//Proceed as current user
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need to work out what happens here..

}
else
{
//Logout current user and try again
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here..

@StanleyGoldman
Copy link
Contributor Author

Actually... the more I look at it this code will never run, PopupWindow and AuthenticationView already has us covered on this one.

var tokenUsernameMismatchException = exception as TokenUsernameMismatchException;
if (tokenUsernameMismatchException != null)
{
Logger.Trace("Token Username Mismatch");
var shouldProceed = EditorUtility.DisplayDialog(AuthenticationChangedTitle,
string.Format(AuthenticationChangedMessageFormat,
tokenUsernameMismatchException.CachedUsername,
tokenUsernameMismatchException.CurrentUsername), AuthenticationChangedProceed, AuthenticationChangedLogout);
if (shouldProceed)
{
//Proceed as current user
}
else
{
//Logout current user and try again
}
return;
}

Copy link
Member

@shana shana left a comment

Choose a reason for hiding this comment

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

omgomgomgomgomg this looks so awesome! 🎉 🚀

@shana shana merged commit 4280ed4 into master Nov 3, 2017
@shana shana deleted the fixes/api-failure-switch-auth-view branch November 3, 2017 23:21
@StanleyGoldman
Copy link
Contributor Author

Fixes #414

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.

3 participants