Merge pull request #1442 from github/fixes/1440-clear-PR-session

Allow PR session to close when user changes to non-PR branch

Author: jcansdale

src/GitHub.InlineReviews/Services/PullRequestSessionManager.cs

@@ -67,12 +67,10 @@ public PullRequestSessionManager(
             this.modelServiceFactory = modelServiceFactory;
 
             Observable.FromEventPattern(teamExplorerContext, nameof(teamExplorerContext.StatusChanged))
-                .StartWith((EventPattern<object>)null)
                 .ObserveOn(RxApp.MainThreadScheduler)
-                .Subscribe(_ => RepoChanged(teamExplorerContext.ActiveRepository).Forget());
+                .Subscribe(_ => StatusChanged().Forget());
 
             teamExplorerContext.WhenAnyValue(x => x.ActiveRepository)
-                .Skip(1)
                 .ObserveOn(RxApp.MainThreadScheduler)
                 .Subscribe(x => RepoChanged(x).Forget());
         }
@@ -159,8 +157,8 @@ public async Task<IPullRequestSession> GetSession(IPullRequestModel pullRequest)
             {
                 // The branch for the PR was not previously marked with the PR number in the git
                 // config so we didn't pick up that the current branch is a PR branch. That has
-                // now been corrected, so call RepoChanged to make sure everything is up-to-date.
-                await RepoChanged(repository);
+                // now been corrected, so call StatusChanged to make sure everything is up-to-date.
+                await StatusChanged();
             }
 
             return await GetSessionInternal(pullRequest);
@@ -191,29 +189,33 @@ public PullRequestTextBufferInfo GetTextBufferInfo(ITextBuffer buffer)
 
         async Task RepoChanged(ILocalRepositoryModel localRepositoryModel)
         {
-            try
-            {
-                if (localRepositoryModel != repository)
-                {
-                    repository = localRepositoryModel;
-                    CurrentSession = null;
-                    sessions.Clear();
-                }
+            repository = localRepositoryModel;
+            CurrentSession = null;
+            sessions.Clear();
 
-                if (string.IsNullOrWhiteSpace(localRepositoryModel?.CloneUrl)) return;
+            if (localRepositoryModel != null)
+            {
+                await StatusChanged();
+            }
+        }
 
-                var modelService = await connectionManager.GetModelService(repository, modelServiceFactory);
+        async Task StatusChanged()
+        {
+            try
+            {
                 var session = CurrentSession;
 
-                if (modelService != null)
+                var pr = await service.GetPullRequestForCurrentBranch(repository).FirstOrDefaultAsync();
+                if (pr != null)
                 {
-                    var pr = await service.GetPullRequestForCurrentBranch(localRepositoryModel).FirstOrDefaultAsync();
+                    var changePR =
+                        pr.Item1 != (session?.PullRequest.Base.RepositoryCloneUrl.Owner) ||
+                        pr.Item2 != (session?.PullRequest.Number);
 
-                    if (pr?.Item1 != (CurrentSession?.PullRequest.Base.RepositoryCloneUrl.Owner) ||
-                        pr?.Item2 != (CurrentSession?.PullRequest.Number))
+                    if (changePR)
                     {
-                        var pullRequest = await GetPullRequestForTip(modelService, localRepositoryModel);
-
+                        var modelService = await connectionManager.GetModelService(repository, modelServiceFactory);
+                        var pullRequest = await modelService?.GetPullRequest(pr.Item1, repository.Name, pr.Item2);
                         if (pullRequest != null)
                         {
                             var newSession = await GetSessionInternal(pullRequest);
@@ -235,17 +237,6 @@ async Task RepoChanged(ILocalRepositoryModel localRepositoryModel)
             }
         }
 
-        async Task<IPullRequestModel> GetPullRequestForTip(IModelService modelService, ILocalRepositoryModel localRepositoryModel)
-        {
-            if (modelService != null)
-            {
-                var pr = await service.GetPullRequestForCurrentBranch(localRepositoryModel);
-                if (pr != null) return await modelService.GetPullRequest(pr.Item1, localRepositoryModel.Name, pr.Item2).ToTask();
-            }
-
-            return null;
-        }
-
         async Task<PullRequestSession> GetSessionInternal(IPullRequestModel pullRequest)
         {
             PullRequestSession session = null;

test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionManagerTests.cs

@@ -18,6 +18,7 @@
 using Microsoft.VisualStudio.Utilities;
 using NSubstitute;
 using NUnit.Framework;
+using System.ComponentModel;
 
 namespace GitHub.InlineReviews.UnitTests.Services
 {
@@ -127,6 +128,60 @@ public void CurrentSessionChangesWhenBranchChanges()
                 Assert.That(session, Is.Not.SameAs(target.CurrentSession));
             }
 
+            [Test]
+            public void LocalRepositoryModelNull()
+            {
+                var repositoryModel = null as LocalRepositoryModel;
+                var target = new PullRequestSessionManager(
+                    CreatePullRequestService(),
+                    Substitute.For<IPullRequestSessionService>(),
+                    CreateConnectionManager(),
+                    CreateModelServiceFactory(),
+                    CreateTeamExplorerContext(null));
+
+                Assert.That(target.CurrentSession, Is.Null);
+            }
+
+            [Test]
+            public void CurrentSessionChangesToNullIfNoPullRequestForCurrentBranch()
+            {
+                var service = CreatePullRequestService();
+                var teamExplorerContext = CreateTeamExplorerContext(CreateRepositoryModel());
+                var target = new PullRequestSessionManager(
+                    service,
+                    Substitute.For<IPullRequestSessionService>(),
+                    CreateConnectionManager(),
+                    CreateModelServiceFactory(),
+                    teamExplorerContext);
+                Assert.That(target.CurrentSession, Is.Not.Null);
+
+                Tuple<string, int> newPullRequest = null;
+                service.GetPullRequestForCurrentBranch(null).ReturnsForAnyArgs(Observable.Return(newPullRequest));
+                teamExplorerContext.StatusChanged += Raise.Event();
+
+                var session = target.CurrentSession;
+
+                Assert.That(session, Is.Null);
+            }
+
+            [Test]
+            public void CurrentSessionChangesToNullWhenRepoChangedToNull()
+            {
+                var teamExplorerContext = CreateTeamExplorerContext(CreateRepositoryModel());
+                var target = new PullRequestSessionManager(
+                    CreatePullRequestService(),
+                    Substitute.For<IPullRequestSessionService>(),
+                    CreateConnectionManager(),
+                    CreateModelServiceFactory(),
+                    teamExplorerContext);
+                Assert.That(target.CurrentSession, Is.Not.Null);
+
+                SetActiveRepository(teamExplorerContext, null);
+                var session = target.CurrentSession;
+
+                Assert.That(session, Is.Null);
+            }
+
             [Test]
             public void CurrentSessionChangesWhenRepoChanged()
             {
@@ -1012,7 +1067,8 @@ static ITeamExplorerContext CreateTeamExplorerContext(ILocalRepositoryModel repo
         static void SetActiveRepository(ITeamExplorerContext teamExplorerContext, ILocalRepositoryModel localRepositoryModel)
         {
             teamExplorerContext.ActiveRepository.Returns(localRepositoryModel);
-            teamExplorerContext.StatusChanged += Raise.Event();
+            var eventArgs = new PropertyChangedEventArgs(nameof(teamExplorerContext.ActiveRepository));
+            teamExplorerContext.PropertyChanged += Raise.Event<PropertyChangedEventHandler>(teamExplorerContext, eventArgs);
         }
     }
 }