diff --git a/src/GitHub.App/Models/PullRequestDetailArgument.cs b/src/GitHub.App/Models/PullRequestDetailArgument.cs index 4fa31ad910..3710e75a68 100644 --- a/src/GitHub.App/Models/PullRequestDetailArgument.cs +++ b/src/GitHub.App/Models/PullRequestDetailArgument.cs @@ -1,5 +1,6 @@ using System; using GitHub.ViewModels; +using GitHub.Primitives; namespace GitHub.Models { @@ -9,9 +10,9 @@ namespace GitHub.Models public class PullRequestDetailArgument { /// - /// Gets or sets the repository containing the pull request. + /// Gets or sets the owner of the repository containing the pull request. /// - public IRemoteRepositoryModel Repository { get; set; } + public string RepositoryOwner { get; set; } /// /// Gets or sets the number of the pull request. diff --git a/src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs b/src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs index 0888ca72a1..c94c5f02c8 100644 --- a/src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs +++ b/src/GitHub.App/SampleData/PullRequestDetailViewModelDesigner.cs @@ -36,7 +36,7 @@ public PullRequestDetailViewModelDesigner() { var repoPath = @"C:\Repo"; - Model = new PullRequestModel(419, + Model = new PullRequestModel(419, "Error handling/bubbling from viewmodels to views to viewhosts", new AccountDesigner { Login = "shana", IsUser = true }, DateTime.Now.Subtract(TimeSpan.FromDays(3))) @@ -75,7 +75,7 @@ public PullRequestDetailViewModelDesigner() public IPullRequestModel Model { get; } public IPullRequestSession Session { get; } public ILocalRepositoryModel LocalRepository { get; } - public IRemoteRepositoryModel RemoteRepository { get; } + public string RemoteRepositoryOwner { get; } public string SourceBranchDisplayName { get; set; } public string TargetBranchDisplayName { get; set; } public int CommentCount { get; set; } diff --git a/src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs b/src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs index 5317ccb3f6..9302ddb8d8 100644 --- a/src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs +++ b/src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs @@ -148,13 +148,13 @@ private set public ILocalRepositoryModel LocalRepository { get; } /// - /// Gets the remote repository that contains the pull request. + /// Gets the owner of the remote repository that contains the pull request. /// /// /// The remote repository may be different from the local repository if the local /// repository is a fork and the user is viewing pull requests from the parent repository. /// - public IRemoteRepositoryModel RemoteRepository { get; private set; } + public string RemoteRepositoryOwner { get; private set; } /// /// Gets the session for the pull request. @@ -326,13 +326,13 @@ public IReadOnlyList ChangedFilesTree public override void Initialize(ViewWithData data) { int number; - var repo = RemoteRepository; + var repoOwner = RemoteRepositoryOwner; if (data != null) { var arg = (PullRequestDetailArgument)data.Data; number = arg.Number; - repo = arg.Repository; + repoOwner = arg.RepositoryOwner; } else { @@ -349,7 +349,7 @@ public override void Initialize(ViewWithData data) } ErrorMessage = OperationError = null; - modelService.GetPullRequest(repo, number) + modelService.GetPullRequest(repoOwner, LocalRepository.Name, number) .TakeLast(1) .ObserveOn(RxApp.MainThreadScheduler) .Catch(ex => @@ -359,23 +359,23 @@ public override void Initialize(ViewWithData data) IsLoading = IsBusy = false; return Observable.Empty(); }) - .Subscribe(x => Load(repo, x).Forget()); + .Subscribe(x => Load(repoOwner, x).Forget()); } /// /// Loads the view model from octokit models. /// - /// The remote repository. + /// The owner of the remote repository. /// The pull request model. - public async Task Load(IRemoteRepositoryModel remoteRepository, IPullRequestModel pullRequest) + public async Task Load(string remoteRepositoryOwner, IPullRequestModel pullRequest) { - Guard.ArgumentNotNull(remoteRepository, nameof(remoteRepository)); + Guard.ArgumentNotNull(remoteRepositoryOwner, nameof(remoteRepositoryOwner)); try { var firstLoad = (Model == null); Model = pullRequest; - RemoteRepository = remoteRepository; + RemoteRepositoryOwner = remoteRepositoryOwner; Session = await sessionManager.GetSession(pullRequest); Title = Resources.PullRequestNavigationItemText + " #" + pullRequest.Number; diff --git a/src/GitHub.App/ViewModels/PullRequestListViewModel.cs b/src/GitHub.App/ViewModels/PullRequestListViewModel.cs index 180f4a4734..05c8653abc 100644 --- a/src/GitHub.App/ViewModels/PullRequestListViewModel.cs +++ b/src/GitHub.App/ViewModels/PullRequestListViewModel.cs @@ -331,7 +331,7 @@ void DoOpenPullRequest(object pullRequest) { Data = new PullRequestDetailArgument { - Repository = SelectedRepository, + RepositoryOwner = SelectedRepository.Owner, Number = (int)pullRequest, } }; diff --git a/src/GitHub.Exports.Reactive/ViewModels/IPullRequestDetailViewModel.cs b/src/GitHub.Exports.Reactive/ViewModels/IPullRequestDetailViewModel.cs index ff6926810f..5397b256a3 100644 --- a/src/GitHub.Exports.Reactive/ViewModels/IPullRequestDetailViewModel.cs +++ b/src/GitHub.Exports.Reactive/ViewModels/IPullRequestDetailViewModel.cs @@ -83,13 +83,13 @@ public interface IPullRequestDetailViewModel : IViewModel, IHasLoading, IHasBusy ILocalRepositoryModel LocalRepository { get; } /// - /// Gets the remote repository that contains the pull request. + /// Gets the owner of the remote repository that contains the pull request. /// /// /// The remote repository may be different from the local repository if the local /// repository is a fork and the user is viewing pull requests from the parent repository. /// - IRemoteRepositoryModel RemoteRepository { get; } + string RemoteRepositoryOwner { get; } /// /// Gets a string describing how to display the pull request's source branch. diff --git a/src/GitHub.Exports/Settings/PkgCmdID.cs b/src/GitHub.Exports/Settings/PkgCmdID.cs index b41e29fdfe..1c8ba5d56e 100644 --- a/src/GitHub.Exports/Settings/PkgCmdID.cs +++ b/src/GitHub.Exports/Settings/PkgCmdID.cs @@ -9,6 +9,7 @@ public static class PkgCmdIDList public const int idGitHubToolbar = 0x1120; public const int showGitHubPaneCommand = 0x200; public const int openPullRequestsCommand = 0x201; + public const int showCurrentPullRequestCommand = 0x202; public const int backCommand = 0x300; public const int forwardCommand = 0x301; public const int refreshCommand = 0x302; diff --git a/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj b/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj index 18049444c1..db0729ecf3 100644 --- a/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj +++ b/src/GitHub.VisualStudio/GitHub.VisualStudio.csproj @@ -311,6 +311,7 @@ + diff --git a/src/GitHub.VisualStudio/GitHub.VisualStudio.vsct b/src/GitHub.VisualStudio/GitHub.VisualStudio.vsct index 24fd2ab45c..8dc212ef03 100644 --- a/src/GitHub.VisualStudio/GitHub.VisualStudio.vsct +++ b/src/GitHub.VisualStudio/GitHub.VisualStudio.vsct @@ -88,6 +88,16 @@ + + - + - + @@ -260,7 +270,7 @@ - + @@ -271,13 +281,14 @@ + - + @@ -313,7 +324,7 @@ - + diff --git a/src/GitHub.VisualStudio/Menus/MenuProvider.cs b/src/GitHub.VisualStudio/Menus/MenuProvider.cs index e1d2db07b9..0edf16514e 100644 --- a/src/GitHub.VisualStudio/Menus/MenuProvider.cs +++ b/src/GitHub.VisualStudio/Menus/MenuProvider.cs @@ -44,7 +44,8 @@ public MenuProvider(IGitHubServiceProvider serviceProvider) { new AddConnection(serviceProvider), new OpenPullRequests(serviceProvider), - new ShowGitHubPane(serviceProvider) + new ShowGitHubPane(serviceProvider), + new ShowCurrentPullRequest(serviceProvider) }; DynamicMenus = new List diff --git a/src/GitHub.VisualStudio/Menus/ShowCurrentPullRequest.cs b/src/GitHub.VisualStudio/Menus/ShowCurrentPullRequest.cs new file mode 100644 index 0000000000..058fc75c47 --- /dev/null +++ b/src/GitHub.VisualStudio/Menus/ShowCurrentPullRequest.cs @@ -0,0 +1,40 @@ +using System; +using GitHub.Exports; +using GitHub.UI; +using GitHub.Services; +using GitHub.Extensions; +using GitHub.Models; + +namespace GitHub.VisualStudio.Menus +{ + [ExportMenu(MenuType = MenuType.OpenPullRequests)] + public class ShowCurrentPullRequest : MenuBase, IMenuHandler + { + public ShowCurrentPullRequest(IGitHubServiceProvider serviceProvider) + : base(serviceProvider) + { + Guard.ArgumentNotNull(serviceProvider, nameof(serviceProvider)); + } + + public Guid Guid => Guids.guidGitHubCmdSet; + public int CmdId => PkgCmdIDList.showCurrentPullRequestCommand; + + public void Activate(object data = null) + { + var pullRequestSessionManager = ServiceProvider.ExportProvider.GetExportedValueOrDefault(); + var session = pullRequestSessionManager?.CurrentSession; + if (session == null) + { + return; // No active PR session. + } + + var pullRequest = session.PullRequest; + var arg = new PullRequestDetailArgument { RepositoryOwner = session.RepositoryOwner, Number = pullRequest.Number }; + var viewWithData = new ViewWithData(UIControllerFlow.PullRequestDetail) { Data = arg }; + + var manager = ServiceProvider.TryGetService(); + var host = manager.ShowHomePane(); + host?.ShowView(viewWithData); + } + } +} diff --git a/src/GitHub.VisualStudio/UI/Views/PullRequestDetailView.xaml.cs b/src/GitHub.VisualStudio/UI/Views/PullRequestDetailView.xaml.cs index e5263147be..2436540919 100644 --- a/src/GitHub.VisualStudio/UI/Views/PullRequestDetailView.xaml.cs +++ b/src/GitHub.VisualStudio/UI/Views/PullRequestDetailView.xaml.cs @@ -26,6 +26,7 @@ using Task = System.Threading.Tasks.Task; using Microsoft.VisualStudio.TextManager.Interop; using System.Text; +using System.Globalization; using Microsoft.VisualStudio.Text.Projection; namespace GitHub.VisualStudio.UI.Views @@ -75,12 +76,18 @@ protected override void OnVisualParentChanged(DependencyObject oldParent) void DoOpenOnGitHub() { - var repo = ViewModel.RemoteRepository; var browser = VisualStudioBrowser; - var url = repo.CloneUrl.ToRepositoryUrl().Append("pull/" + ViewModel.Model.Number); + var cloneUrl = ViewModel.LocalRepository.CloneUrl; + var url = ToPullRequestUrl(cloneUrl.Host, ViewModel.RemoteRepositoryOwner, ViewModel.LocalRepository.Name, ViewModel.Model.Number); browser.OpenUrl(url); } + static Uri ToPullRequestUrl(string host, string owner, string repositoryName, int number) + { + var url = string.Format(CultureInfo.InvariantCulture, "https://{0}/{1}/{2}/pull/{3}", host, owner, repositoryName, number); + return new Uri(url); + } + async Task DoOpenFile(IPullRequestFileNode file, bool workingDirectory) { try diff --git a/test/UnitTests/GitHub.App/ViewModels/PullRequestDetailViewModelTests.cs b/test/UnitTests/GitHub.App/ViewModels/PullRequestDetailViewModelTests.cs index 87f5518bd9..8ed721922d 100644 --- a/test/UnitTests/GitHub.App/ViewModels/PullRequestDetailViewModelTests.cs +++ b/test/UnitTests/GitHub.App/ViewModels/PullRequestDetailViewModelTests.cs @@ -26,9 +26,8 @@ public class TheBodyProperty public async Task ShouldUsePlaceholderBodyIfNoneExists() { var target = CreateTarget(); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest(body: string.Empty)); + await target.Load("remoteRepositoryOwner", CreatePullRequest(body: string.Empty)); Assert.Equal("*No description provided.*", target.Body); } @@ -41,12 +40,11 @@ public async Task ShouldAcceptNullHead() { var target = CreateTarget(); var model = CreatePullRequest(); - var repo = Substitute.For(); // PullRequest.Head can be null for example if a user deletes the repository after creating the PR. model.Head = null; - await target.Load(repo, model); + await target.Load("remoteRepositoryOwner", model); Assert.Equal("[invalid]", target.SourceBranchDisplayName); } @@ -59,7 +57,6 @@ public async Task ShouldCreateChangesTree() { var target = CreateTarget(); var pr = CreatePullRequest(); - var repo = Substitute.For(); pr.ChangedFiles = new[] { @@ -70,7 +67,7 @@ public async Task ShouldCreateChangesTree() new PullRequestFileModel("dir2/f4.cs", "abc", PullRequestFileStatus.Modified), }; - await target.Load(repo, pr); + await target.Load("remoteRepositoryOwner", pr); Assert.Equal(3, target.ChangedFilesTree.Count); @@ -107,7 +104,6 @@ public async Task FileCommentCountShouldTrackSessionInlineComments() var outdatedThread = CreateThread(-1); var session = Substitute.For(); var sessionManager = Substitute.For(); - var repo = Substitute.For(); file.InlineCommentThreads.Returns(new[] { thread1 }); session.GetFile("readme.md").Returns(Task.FromResult(file)); @@ -120,7 +116,7 @@ public async Task FileCommentCountShouldTrackSessionInlineComments() new PullRequestFileModel("readme.md", "abc", PullRequestFileStatus.Modified), }; - await target.Load(repo, pr); + await target.Load("remoteRepositoryOwner", pr); Assert.Equal(1, ((IPullRequestFileNode)target.ChangedFilesTree[0]).CommentCount); file.InlineCommentThreads.Returns(new[] { thread1, thread2 }); @@ -158,9 +154,8 @@ public async Task CheckedOutAndUpToDate() var target = CreateTarget( currentBranch: "pr/123", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.False(target.Checkout.CanExecute(null)); Assert.Null(target.CheckoutState); @@ -172,9 +167,8 @@ public async Task NotCheckedOut() var target = CreateTarget( currentBranch: "master", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.True(target.Checkout.CanExecute(null)); Assert.True(target.CheckoutState.IsEnabled); @@ -188,9 +182,8 @@ public async Task NotCheckedOutWithWorkingDirectoryDirty() currentBranch: "master", existingPrBranch: "pr/123", dirty: true); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.False(target.Checkout.CanExecute(null)); Assert.Equal("Cannot checkout as your working directory has uncommitted changes.", target.CheckoutState.ToolTip); @@ -202,9 +195,8 @@ public async Task CheckoutExistingLocalBranch() var target = CreateTarget( currentBranch: "master", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest(number: 123)); + await target.Load("remoteRepositoryOwner", CreatePullRequest(number: 123)); Assert.True(target.Checkout.CanExecute(null)); Assert.Equal("Checkout pr/123", target.CheckoutState.Caption); @@ -215,9 +207,8 @@ public async Task CheckoutNonExistingLocalBranch() { var target = CreateTarget( currentBranch: "master"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest(number: 123)); + await target.Load("remoteRepositoryOwner", CreatePullRequest(number: 123)); Assert.True(target.Checkout.CanExecute(null)); Assert.Equal("Checkout to pr/123", target.CheckoutState.Caption); @@ -230,11 +221,10 @@ public async Task UpdatesOperationErrorWithExceptionMessage() currentBranch: "master", existingPrBranch: "pr/123"); var pr = CreatePullRequest(); - var repo = Substitute.For(); pr.Head = new GitReferenceModel("source", null, "sha", (string)null); - await target.Load(repo, pr); + await target.Load("remoteRepositoryOwner", pr); Assert.False(target.Checkout.CanExecute(null)); Assert.Equal("The source repository is no longer available.", target.CheckoutState.ToolTip); @@ -246,9 +236,8 @@ public async Task SetsOperationErrorOnCheckoutFailure() var target = CreateTarget( currentBranch: "master", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.True(target.Checkout.CanExecute(null)); @@ -263,9 +252,8 @@ public async Task ClearsOperationErrorOnCheckoutSuccess() var target = CreateTarget( currentBranch: "master", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.True(target.Checkout.CanExecute(null)); await Assert.ThrowsAsync(async () => await target.Checkout.ExecuteAsyncTask()); @@ -281,9 +269,8 @@ public async Task ClearsOperationErrorOnCheckoutRefresh() var target = CreateTarget( currentBranch: "master", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.True(target.Checkout.CanExecute(null)); await Assert.ThrowsAsync(async () => await target.Checkout.ExecuteAsyncTask()); @@ -302,9 +289,8 @@ public async Task NotCheckedOut() var target = CreateTarget( currentBranch: "master", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.False(target.Pull.CanExecute(null)); Assert.Null(target.UpdateState); @@ -316,9 +302,8 @@ public async Task CheckedOutAndUpToDate() var target = CreateTarget( currentBranch: "pr/123", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.False(target.Pull.CanExecute(null)); Assert.Equal(0, target.UpdateState.CommitsAhead); @@ -333,9 +318,8 @@ public async Task CheckedOutAndBehind() currentBranch: "pr/123", existingPrBranch: "pr/123", behindBy: 2); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.True(target.Pull.CanExecute(null)); Assert.Equal(0, target.UpdateState.CommitsAhead); @@ -351,9 +335,8 @@ public async Task CheckedOutAndAheadAndBehind() existingPrBranch: "pr/123", aheadBy: 3, behindBy: 2); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.True(target.Pull.CanExecute(null)); Assert.Equal(3, target.UpdateState.CommitsAhead); @@ -369,9 +352,8 @@ public async Task CheckedOutAndBehindFork() existingPrBranch: "pr/123", prFromFork: true, behindBy: 2); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.True(target.Pull.CanExecute(null)); Assert.Equal(0, target.UpdateState.CommitsAhead); @@ -385,9 +367,8 @@ public async Task UpdatesOperationErrorWithExceptionMessage() var target = CreateTarget( currentBranch: "master", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); await Assert.ThrowsAsync(() => target.Pull.ExecuteAsyncTask(null)); Assert.Equal("Pull threw", target.OperationError); @@ -402,9 +383,8 @@ public async Task NotCheckedOut() var target = CreateTarget( currentBranch: "master", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.False(target.Push.CanExecute(null)); Assert.Null(target.UpdateState); @@ -416,9 +396,8 @@ public async Task CheckedOutAndUpToDate() var target = CreateTarget( currentBranch: "pr/123", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.False(target.Push.CanExecute(null)); Assert.Equal(0, target.UpdateState.CommitsAhead); @@ -433,9 +412,8 @@ public async Task CheckedOutAndAhead() currentBranch: "pr/123", existingPrBranch: "pr/123", aheadBy: 2); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.True(target.Push.CanExecute(null)); Assert.Equal(2, target.UpdateState.CommitsAhead); @@ -450,9 +428,8 @@ public async Task CheckedOutAndBehind() currentBranch: "pr/123", existingPrBranch: "pr/123", behindBy: 2); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.False(target.Push.CanExecute(null)); Assert.Equal(0, target.UpdateState.CommitsAhead); @@ -468,9 +445,8 @@ public async Task CheckedOutAndAheadAndBehind() existingPrBranch: "pr/123", aheadBy: 3, behindBy: 2); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.False(target.Push.CanExecute(null)); Assert.Equal(3, target.UpdateState.CommitsAhead); @@ -486,9 +462,8 @@ public async Task CheckedOutAndAheadOfFork() existingPrBranch: "pr/123", prFromFork: true, aheadBy: 2); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); Assert.True(target.Push.CanExecute(null)); Assert.Equal(2, target.UpdateState.CommitsAhead); @@ -502,9 +477,8 @@ public async Task UpdatesOperationErrorWithExceptionMessage() var target = CreateTarget( currentBranch: "master", existingPrBranch: "pr/123"); - var repo = Substitute.For(); - await target.Load(repo, CreatePullRequest()); + await target.Load("remoteRepositoryOwner", CreatePullRequest()); await Assert.ThrowsAsync(() => target.Push.ExecuteAsyncTask(null)); Assert.Equal("Push threw", target.OperationError);