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

Allow selection of fork/parent pull requests. #1021

Merged
merged 27 commits into from Aug 1, 2017

Conversation

Projects
None yet
3 participants
@grokys
Contributor

grokys commented Jun 28, 2017

When working on a fork, previously the PR list would show PRs for that fork, which is often not what is wanted. This PR adds a selector dropdown to select the repository from which to display the pull requests, defaulting to the parent repository if the repository is a fork. If the repository is not a fork, the selector will be hidden.

image

This exposes a similar problem to #734 in that there quickly ends up being no room along the top row of filters for the repository name, meaning it wraps to a new line, causing ugly. We need to think of a new way to display filters for this and #734. cc: @donokuda

Depends on #1004, #1023
Fixes #863

grokys added some commits Jun 26, 2017

Optionally show PRs for forked repo.
Previously if the user was working on a fork, the PRs for the fork would be shown rather than the PRs for the parent repository. Change the default to show PRs for the parent repository with a (very ugly!) checkbox to switch to showing the PRs for the fork.
Recreate collection when switching to/from fork.
Recreate the `PullRequests` `TrackingCollection` when switching to/from showing PRs for a fork. Also use the repository owner name in `ModelService.GetPullRequests` instead of the current username.
Display correct PR in PR details view.
Take into account whether the PR is from a fork or not in `PullRequestDetailViewModel`.
Change how we determine if PR is from fork.
Previously we simply did a check to see if `Head.Owner` != `Base.Owner` but that didn't work when we were _in_ a fork and looking at a PR in the parent. In that case both head and base are in a different repository.
Make fork selection a dropdown link.
This is better than the ugly checkbox there was before, but it's easy for it to get too long and wrap, which looks bad.

grokys added a commit that referenced this pull request Jun 29, 2017

Added a version to host cache.
In #1021 we require the repository parent (in the case of forked repositories) - this data is returned from the API but wasn't getting stored in our cache, meaning that we have invalid data when data is returned from the cache as opposed to from the API.

This adds a `cacheVersion` to host caches. This version is checked when the host cache is first loaded, and if it doesn't match the expected version, the cache is invalidated. In this way each time we make a change as required by #1021 we can bump `HostCacheFactory.CacheVersion` to ensure the cache won't be returning invalid data.

grokys added some commits Jun 30, 2017

Fix failing tests.
`Repository.Owner` is now being used by `ModelService.GetPullRequest`.
@tierninho

This comment has been minimized.

Show comment
Hide comment
@tierninho

tierninho Jul 11, 2017

Collaborator

@grokys Tried testing this PR, but the PR page in the github window just hangs, with "Unable to complete read operation after process has exited" in the Output window. Not sure how to get around this.

screen shot 2017-07-11 at 10 08 58 am

Collaborator

tierninho commented Jul 11, 2017

@grokys Tried testing this PR, but the PR page in the github window just hangs, with "Unable to complete read operation after process has exited" in the Output window. Not sure how to get around this.

screen shot 2017-07-11 at 10 08 58 am

grokys added some commits Jul 11, 2017

Use new config key for PR owner/number.
Using the same one with different contents means that one cannot downgrade the assembly as the older version throws on the new format.
Merge remote-tracking branch 'origin/master' into fixes/863-fork-pull…
…-requests

 Conflicts:
	src/GitHub.InlineReviews/Services/PullRequestSession.cs
@grokys

This comment has been minimized.

Show comment
Hide comment
@grokys

grokys Jul 11, 2017

Contributor

@tierninho could you try now?

Contributor

grokys commented Jul 11, 2017

@tierninho could you try now?

@tierninho

This comment has been minimized.

Show comment
Hide comment
@tierninho

tierninho Jul 11, 2017

Collaborator

@grokys same result, still hangs.

Collaborator

tierninho commented Jul 11, 2017

@grokys same result, still hangs.

@jcansdale

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Jul 12, 2017

Contributor

Strange. This PR has been working well for me. 😕

Contributor

jcansdale commented Jul 12, 2017

Strange. This PR has been working well for me. 😕

@tierninho

This comment has been minimized.

Show comment
Hide comment
@tierninho

tierninho Jul 12, 2017

Collaborator

@grokys and I got this working on my machine after tweaking my setup 👍

Collaborator

tierninho commented Jul 12, 2017

@grokys and I got this working on my machine after tweaking my setup 👍

@jcansdale

Works on my machine! 😉

grokys and others added some commits Jul 14, 2017

Merge branch 'master' into fixes/863-fork-pull-requests
 Conflicts:
	src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs
GetPullRequestMergeBase before GetFileContent
This will ensure that required commits/blobs are available.
Make GetPullRequestMergeBase fetch from correct repo URLs
When merge-base can't be found, fetch the PR base and head from the correct repo URLs.
Show outdated Hide outdated src/GitHub.App/Services/GitClient.cs
{
var mergeBase = GetMergeBase(repo, baseSha, headSha);
if (mergeBase == null)
{
var pullHeadRef = $"refs/pull/{pullNumber}/head";
await Fetch(repo, remoteName, baseRef, pullHeadRef);
// TODO: Optimize and error check.

This comment has been minimized.

@jcansdale

jcansdale Jul 14, 2017

Contributor

I need to fix this.

@jcansdale

jcansdale Jul 14, 2017

Contributor

I need to fix this.

jcansdale added some commits Jul 14, 2017

Include DesignTimeStyleHelper in solution build
Build was failing because this was excluded.
Remove dependency on `refs/pull/PR/head` remote refs
We no longer know whether the local repo will be on base or head end of the PR.
Fetch using repo URL and PR base/head ref.
Show outdated Hide outdated src/GitHub.App/Services/GitClient.cs
@@ -394,6 +401,20 @@ public Task<bool> IsHeadPushed(IRepository repo)
});
}
Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs)
{
var tempRemoteName = $"{cloneUrl.Owner}-{Guid.NewGuid()}";

This comment has been minimized.

@jcansdale

jcansdale Jul 17, 2017

Contributor

This should only create temporary remotes when necessary (when cloneUrl != origin).

@jcansdale

jcansdale Jul 17, 2017

Contributor

This should only create temporary remotes when necessary (when cloneUrl != origin).

jcansdale and others added some commits Jul 18, 2017

Use `origin` remote when possible
There is a good change the PR base or head comes from `origin`. Use this remote when possible.
Merge remote-tracking branch 'origin/master' into fixes/863-fork-pull…
…-requests

 Conflicts:
	src/GitHub.App/Models/RemoteRepositoryModel.cs
	src/GitHub.App/Services/GitClient.cs
	src/GitHub.App/ViewModels/PullRequestListViewModel.cs
Set RemoteRepository in Load.
This was removed by a bad merge in 287ca67
@jcansdale

Didn't actually mean to block this.

@github github deleted a comment from jcansdale Jul 31, 2017

@grokys grokys merged commit ad6b434 into master Aug 1, 2017

5 checks passed

GitHub CLA @grokys has accepted the GitHub Contributor License Agreement.
Details
VisualStudio Build #7485433 succeeded in 93s
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins/build_log Jenkins Build Log
Details

@grokys grokys deleted the fixes/863-fork-pull-requests branch Aug 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment