Login in the sync page #135
Conversation
Fixes #57 If the current repository is a github/github enterprise repo but the user isn't logged in to its instance, then network git operations might fail. Show a warning on the Sync page if this is the case, and allow the user to login from there.
|
Very nice! I tried this out with a github.com repository and it worked great. However, I tried it with a GitHub Enterprise repository and it didn't work. Here's the steps I took.
Expected: An error that shows I'm not logged into the GitHub Enterprise instance corresponding to the remote.
Note that the tooltip says "api.github.com". It should probably have the domain of the GitHub Enterprise instance. When I click "Login" it launched the login dialog, but the dialog had "GitHub" selected instead of GitHub Enterprise. |
| return cm.Connections.ToObservable() | ||
| .Where(c => c.HostAddress.Equals(address)) | ||
| .SelectMany(c => c.Login()) | ||
| .Select(c => hosts.LookupHost(c.HostAddress)).Any(h => h.IsLoggedIn); |
There was a problem hiding this comment.
I think putting the .Any(...) call here on a new line aligned with .Where .SelectMany and Select would be more consistent with how we chain methods elsewhere.
|
🍨 |
|
Looks like our detection of enterprise instances when not authenticated was not quite correct. Now it should correctly detect them, the message should use the correct domain name and the home page should show the information about the repository if it's an enterprise instance repo (when not logged in). |
There's no way right now of telling the login dialog which tab to show. That's going to take a bigger change of how we instantiate views in general (to pass contextual information to them), and I need to do that for the PR views as well (part of the UIController refactor I'm doing), so I'll fix that once I have a system to do it properly in place. |
| // it'll throw if it's private or an enterprise instance requiring authentication | ||
| catch (ForbiddenException) | ||
| { | ||
| if (!HostAddress.IsGitHubDotComUri(OriginalUrl.ToRepositoryUrl())) |
There was a problem hiding this comment.
Flow analysis seems to think OriginalUrl could be null. However, it looks to me that's not legitimate. So maybe add an Assert here that it's not null?
There was a problem hiding this comment.
Yeah, flow analysis is completely wrong here, this exception can only be thrown inside the condition that guarantees that OriginalUrl is not null (and OriginalUrl is readonly it's impossible for it to be changed in a way that makes it non-null to go into the condition and null in the catch block). I'm doing some changes to the constructor so this it not an issue anymore.
shana
force-pushed
the
feature/login-on-sync-page
branch
from
9554dd5
to
a10b917
Compare
|
I've fixed things up and this branch is totally 💯 ready! ✨ |
|
I tested it with a GitHub Enterprise repository and it still doesn't work for me. |
|
Do you get information about the repo on the team explorer home page when you're not logged in? I've tried with garage and with a test enterprise instance, what instance are you trying with? Maybe there's something different about the instance you're using that's tripping the check wrongly. |
Nope. Just the basic Git stuff.
I'm trying with a repo in our internal GitHub Enterprise instance. You know the one. 😄 |
|
Ok, there's some creepy threading action going on which is making things not load properly. Fixicating... |
|
@haacked Ok, fixed it. Looks like different enterprise instances throw different types of exceptions when you're not logged in, and the exception I was catching was a little too specific. |
|
Ok, I'll test this right now. |
Ok, I'll log a separate issue for this. Also, I noticed that after I logged in, the message didn't automatically go away. And if I log out and then go back to the Sync button, I don't see the message. I'll log separate issues for that too. I think the current behavior is good enough for now. |
|
|
Fixes #57
If the current repository is a github/github enterprise repo but the user isn't logged in to its instance, then network git operations might fail. Show a warning on the Sync page if this is the case, and allow the
user to login from there.