Merge pull request #482 from github/fixes/477-crash-pr-empty-body

grokys · web-flow · commit 7481136f31fd · 2016-08-09T13:30:20.000+02:00
Fix crasher when creating a PR with nothing in the body
diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs
@@ -4,12 +4,20 @@
 
 namespace GitHub.Services
 {
+    [NullGuard.NullGuard(NullGuard.ValidationFlags.None)]
     [Export(typeof(IPullRequestService))]
     [PartCreationPolicy(CreationPolicy.Shared)]
     public class PullRequestService : IPullRequestService
     {
         public IObservable<IPullRequestModel> CreatePullRequest(IRepositoryHost host, ISimpleRepositoryModel repository, string title, string body, IBranch source, IBranch target)
         {
+            Extensions.Guard.ArgumentNotNull(host, nameof(host));
+            Extensions.Guard.ArgumentNotNull(repository, nameof(repository));
+            Extensions.Guard.ArgumentNotNull(title, nameof(title));
+            Extensions.Guard.ArgumentNotNull(body, nameof(body));
+            Extensions.Guard.ArgumentNotNull(source, nameof(source));
+            Extensions.Guard.ArgumentNotNull(target, nameof(target));
+
             return host.ModelService.CreatePullRequest(repository, title, body, source, target);
         }
     }
diff --git a/src/GitHub.App/ViewModels/PullRequestCreationViewModel.cs b/src/GitHub.App/ViewModels/PullRequestCreationViewModel.cs
@@ -17,6 +17,7 @@
 using System.Reactive;
 using System.Diagnostics.CodeAnalysis;
 using Octokit;
+using NLog;
 
 namespace GitHub.ViewModels
 {
@@ -25,6 +26,8 @@ namespace GitHub.ViewModels
     [SuppressMessage("Microsoft.Design", "CA1001:TypesThatOwnDisposableFieldsShouldBeDisposable")]
     public class PullRequestCreationViewModel : BaseViewModel, IPullRequestCreationViewModel
     {
+        static readonly Logger log = LogManager.GetCurrentClassLogger();
+
         readonly IRepositoryHost repositoryHost;
         readonly ISimpleRepositoryModel activeRepo;
         readonly Subject<Unit> initializationComplete = new Subject<Unit>();
@@ -36,7 +39,7 @@ public class PullRequestCreationViewModel : BaseViewModel, IPullRequestCreationV
              IPullRequestService service, INotificationService notifications)
              : this(connectionRepositoryHostMap.CurrentRepositoryHost, teservice.ActiveRepo, service, notifications)
          {}
-        
+
         public PullRequestCreationViewModel(IRepositoryHost repositoryHost, ISimpleRepositoryModel activeRepo,
             IPullRequestService service, INotificationService notifications)
         {
@@ -78,18 +81,18 @@ public PullRequestCreationViewModel(IRepositoryHost repositoryHost, ISimpleRepos
                 .Subscribe(x => notifications.ShowError(BranchValidator.ValidationResult.Message));
 
             createPullRequest = ReactiveCommand.CreateAsyncObservable(whenAnyValidationResultChanges,
-                _ => service.CreatePullRequest(repositoryHost, activeRepo, PRTitle, Description, SourceBranch, TargetBranch)
-            );
-            createPullRequest.ThrownExceptions.Subscribe(ex =>
-            {
-                if (!ex.IsCriticalException())
-                {
-                    //TODO:Will need a uniform solution to HTTP exception message handling
-                    var apiException = ex as ApiValidationException;
-                    var error = apiException?.ApiError?.Errors?.FirstOrDefault();
-                    notifications.ShowError(error?.Message ?? ex.Message);
-                }
-            });
+                _ => service
+                    .CreatePullRequest(repositoryHost, activeRepo, PRTitle, Description ?? String.Empty, SourceBranch, TargetBranch)
+                    .Catch<IPullRequestModel, Exception>(ex =>
+                    {
+                        log.Error(ex);
+
+                        //TODO:Will need a uniform solution to HTTP exception message handling
+                        var apiException = ex as ApiValidationException;
+                        var error = apiException?.ApiError?.Errors?.FirstOrDefault();
+                        notifications.ShowError(error?.Message ?? ex.Message);
+                        return Observable.Empty<IPullRequestModel>();
+                    }));
         }
         
         public override void Initialize([AllowNull] ViewWithData data)
diff --git a/src/UnitTests/GitHub.App/Models/RepositoryModelTests.cs b/src/UnitTests/GitHub.App/Models/RepositoryModelTests.cs
@@ -60,7 +60,6 @@ public class PathConstructorTests : TempFileBaseClass
         public void NoRemoteUrl()
         {
             var provider = Substitutes.ServiceProvider;
-            Services.PackageServiceProvider = provider;
             var gitservice = provider.GetGitService();
             var repo = Substitute.For<IRepository>();
             var path = Directory.CreateSubdirectory("repo-name");
@@ -73,7 +72,6 @@ public void NoRemoteUrl()
         public void WithRemoteUrl()
         {
             var provider = Substitutes.ServiceProvider;
-            Services.PackageServiceProvider = provider;
             var gitservice = provider.GetGitService();
             var repo = Substitute.For<IRepository>();
             var path = Directory.CreateSubdirectory("repo-name");
diff --git a/src/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs b/src/UnitTests/GitHub.App/Services/PullRequestServiceTests.cs
@@ -0,0 +1,48 @@
+﻿using System.Reactive.Linq;
+using System.Threading.Tasks;
+using NSubstitute;
+using Xunit;
+using UnitTests;
+using GitHub.Models;
+using System;
+using GitHub.Services;
+
+public class PullRequestServiceTests : TestBaseClass
+{
+    [Fact]
+    public async Task CreatePullRequestAllArgsMandatory()
+    {
+        var serviceProvider = Substitutes.ServiceProvider;
+        var service = new PullRequestService();
+
+        IRepositoryHost host = null;
+        ISimpleRepositoryModel repository = null;
+        string title = null;
+        string body = null;
+        IBranch source = null;
+        IBranch target = null;
+
+        Assert.Throws<ArgumentNullException>(() => service.CreatePullRequest(host, repository, title, body, source, target));
+
+        host = serviceProvider.GetRepositoryHosts().GitHubHost;
+        Assert.Throws<ArgumentNullException>(() => service.CreatePullRequest(host, repository, title, body, source, target));
+
+        repository = new SimpleRepositoryModel("name", new GitHub.Primitives.UriString("http://github.com/github/stuff"));
+        Assert.Throws<ArgumentNullException>(() => service.CreatePullRequest(host, repository, title, body, source, target));
+
+        title = "a title";
+        Assert.Throws<ArgumentNullException>(() => service.CreatePullRequest(host, repository, title, body, source, target));
+
+        body = "a body";
+        Assert.Throws<ArgumentNullException>(() => service.CreatePullRequest(host, repository, title, body, source, target));
+
+        source = new BranchModel() { Name = "source" };
+        Assert.Throws<ArgumentNullException>(() => service.CreatePullRequest(host, repository, title, body, source, target));
+
+        target = new BranchModel() { Name = "target" };
+        var pr = await service.CreatePullRequest(host, repository, title, body, source, target);
+
+        Assert.NotNull(pr);
+    }
+
+}
diff --git a/src/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs b/src/UnitTests/GitHub.App/ViewModels/PullRequestCreationViewModelTests.cs
@@ -0,0 +1,36 @@
+﻿using System.Reactive.Linq;
+using System.Threading.Tasks;
+using NSubstitute;
+using Xunit;
+using UnitTests;
+using GitHub.Models;
+using System;
+using GitHub.Services;
+using GitHub.ViewModels;
+
+public class PullRequestCreationViewModelTests : TempFileBaseClass
+{
+    [Fact]
+    public async Task NullDescriptionBecomesEmptyBody()
+    {
+        var serviceProvider = Substitutes.ServiceProvider;
+        var service = new PullRequestService();
+        var notifications = Substitute.For<INotificationService>();
+
+        var host = serviceProvider.GetRepositoryHosts().GitHubHost;
+        var ms = Substitute.For<IModelService>();
+        host.ModelService.Returns(ms);
+
+        var repository = new SimpleRepositoryModel("name", new GitHub.Primitives.UriString("http://github.com/github/stuff"));
+        var title = "a title";
+
+        var vm = new PullRequestCreationViewModel(host, repository, service, notifications);
+        vm.SourceBranch = new BranchModel() { Name = "source" };
+        vm.TargetBranch = new BranchModel() { Name = "target" };
+        vm.PRTitle = title;
+
+        await vm.CreatePullRequest.ExecuteAsync();
+        var unused = ms.Received().CreatePullRequest(repository, vm.PRTitle, String.Empty, vm.SourceBranch, vm.TargetBranch);
+    }
+
+}
diff --git a/src/UnitTests/GitHub.Exports/SimpleRepositoryModelTests.cs b/src/UnitTests/GitHub.Exports/SimpleRepositoryModelTests.cs
@@ -13,7 +13,6 @@ public class SimpleRepositoryModelTests : TempFileBaseClass
     void SetupRepository(string sha)
     {
         var provider = Substitutes.ServiceProvider;
-        Services.PackageServiceProvider = provider;
         var gitservice = provider.GetGitService();
         var repo = Substitute.For<IRepository>();
         gitservice.GetRepo(Args.String).Returns(repo);
diff --git a/src/UnitTests/Properties/AssemblyInfo.cs b/src/UnitTests/Properties/AssemblyInfo.cs
@@ -1,6 +1,7 @@
 ﻿using System.Reflection;
 using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
+using Xunit;
 
 // General Information about an assembly is controlled through the following 
 // set of attributes. Change these attribute values to modify the information
@@ -34,3 +35,4 @@
 // [assembly: AssemblyVersion("1.0.*")]
 [assembly: AssemblyVersion("1.0.0.0")]
 [assembly: AssemblyFileVersion("1.0.0.0")]
+[assembly: CollectionBehavior(CollectionBehavior.CollectionPerAssembly, DisableTestParallelization = true)]
diff --git a/src/UnitTests/Substitutes.cs b/src/UnitTests/Substitutes.cs
@@ -2,6 +2,7 @@
 using GitHub.Authentication;
 using GitHub.Models;
 using GitHub.Services;
+using GitHub.VisualStudio;
 using Microsoft.VisualStudio.ComponentModelHost;
 using NSubstitute;
 using Rothko;
@@ -65,6 +66,7 @@ public static IOperatingSystem OperatingSystem
         public static IConnectionManager ConnectionManager { get { return Substitute.For<IConnectionManager>(); } }
         public static ITwoFactorChallengeHandler TwoFactorChallengeHandler { get { return Substitute.For<ITwoFactorChallengeHandler>(); } }
         public static IGistPublishService GistPublishService { get { return Substitute.For<IGistPublishService>(); } }
+        public static IPullRequestService PullRequestService { get { return Substitute.For<IPullRequestService>(); } }
 
         /// <summary>
         /// This returns a service provider with everything mocked except for 
@@ -106,6 +108,7 @@ public static IServiceProvider GetServiceProvider(
             cc.ComposeExportedValue(gitservice);
             ((IComponentModel)cm).DefaultExportProvider.Returns(cc);
             ret.GetService(typeof(SComponentModel)).Returns(cm);
+            Services.PackageServiceProvider = ret;
 
             var os = OperatingSystem;
             var vs = IVSServices;
@@ -126,6 +129,7 @@ public static IServiceProvider GetServiceProvider(
             ret.GetService(typeof(IAvatarProvider)).Returns(avatarProvider);
             ret.GetService(typeof(ITwoFactorChallengeHandler)).Returns(TwoFactorChallengeHandler);
             ret.GetService(typeof(IGistPublishService)).Returns(GistPublishService);
+            ret.GetService(typeof(IPullRequestService)).Returns(PullRequestService);
             return ret;
         }
 
@@ -197,5 +201,10 @@ public static IGistPublishService GetGistPublishService(this IServiceProvider pr
         {
             return provider.GetService(typeof(IGistPublishService)) as IGistPublishService;
         }
+
+        public static IPullRequestService GetPullRequestsService(this IServiceProvider provider)
+        {
+            return provider.GetService(typeof(IPullRequestService)) as IPullRequestService;
+        }
     }
 }
diff --git a/src/UnitTests/UnitTests.csproj b/src/UnitTests/UnitTests.csproj
@@ -166,11 +166,13 @@
     <Compile Include="GitHub.App\Models\RepositoryModelTests.cs" />
     <Compile Include="GitHub.App\Services\AvatarProviderTests.cs" />
     <Compile Include="GitHub.App\Services\GitClientTests.cs" />
+    <Compile Include="GitHub.App\Services\PullRequestServiceTests.cs" />
     <Compile Include="GitHub.App\Services\RepositoryCloneServiceTests.cs" />
     <Compile Include="GitHub.App\Services\RepositoryCreationServiceTests.cs" />
     <Compile Include="GitHub.App\ViewModels\GistCreationViewModelTests.cs" />
     <Compile Include="GitHub.App\ViewModels\LoginControlViewModelTests.cs" />
     <Compile Include="GitHub.App\ViewModels\LoginToGitHubViewModelTests.cs" />
+    <Compile Include="GitHub.App\ViewModels\PullRequestCreationViewModelTests.cs" />
     <Compile Include="GitHub.App\ViewModels\PullRequestListViewModelTests.cs" />
     <Compile Include="GitHub.App\ViewModels\RepositoryCloneViewModelTests.cs" />
     <Compile Include="GitHub.App\ViewModels\RepositoryCreationViewModelTests.cs" />

Original file line number	Diff line number	Diff line change
`@@ -4,12 +4,20 @@`
`4`	`4`
`5`	`5`	`namespace GitHub.Services`
`6`	`6`	`{`
	`7`	`+ [NullGuard.NullGuard(NullGuard.ValidationFlags.None)]`
`7`	`8`	`[Export(typeof(IPullRequestService))]`
`8`	`9`	`[PartCreationPolicy(CreationPolicy.Shared)]`
`9`	`10`	`public class PullRequestService : IPullRequestService`
`10`	`11`	`{`
`11`	`12`	`public IObservable<IPullRequestModel> CreatePullRequest(IRepositoryHost host, ISimpleRepositoryModel repository, string title, string body, IBranch source, IBranch target)`
`12`	`13`	`{`
	`14`	`+ Extensions.Guard.ArgumentNotNull(host, nameof(host));`
	`15`	`+ Extensions.Guard.ArgumentNotNull(repository, nameof(repository));`
	`16`	`+ Extensions.Guard.ArgumentNotNull(title, nameof(title));`
	`17`	`+ Extensions.Guard.ArgumentNotNull(body, nameof(body));`
	`18`	`+ Extensions.Guard.ArgumentNotNull(source, nameof(source));`
	`19`	`+ Extensions.Guard.ArgumentNotNull(target, nameof(target));`
	`20`	`+`
`13`	`21`	`return host.ModelService.CreatePullRequest(repository, title, body, source, target);`
`14`	`22`	`}`
`15`	`23`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,6 @@ public class SimpleRepositoryModelTests : TempFileBaseClass`
`13`	`13`	`void SetupRepository(string sha)`
`14`	`14`	`{`
`15`	`15`	`var provider = Substitutes.ServiceProvider;`
`16`		`- Services.PackageServiceProvider = provider;`
`17`	`16`	`var gitservice = provider.GetGitService();`
`18`	`17`	`var repo = Substitute.For<IRepository>();`
`19`	`18`	`gitservice.GetRepo(Args.String).Returns(repo);`