From d36c33edce3210693f4846cde56bac7e6fd1f538 Mon Sep 17 00:00:00 2001 From: Stanley Goldman Date: Wed, 12 Jun 2019 10:42:16 -0400 Subject: [PATCH] Moving UsageService from PullRequestService to PullRequestCreationViewModel --- src/GitHub.App/Services/PullRequestService.cs | 6 +----- .../PullRequestCreationViewModel.cs | 21 +++++++++++++++---- .../Services/PullRequestServiceTests.cs | 16 +++++--------- .../PullRequestCreationViewModelTests.cs | 20 +++++++++--------- 4 files changed, 33 insertions(+), 30 deletions(-) diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs index 7c143409ba..8b794a8735 100644 --- a/src/GitHub.App/Services/PullRequestService.cs +++ b/src/GitHub.App/Services/PullRequestService.cs @@ -60,7 +60,6 @@ public class PullRequestService : IssueishService, IPullRequestService, IStaticR readonly IVSGitExt gitExt; readonly IGraphQLClientFactory graphqlFactory; readonly IOperatingSystem os; - readonly IUsageTracker usageTracker; readonly IDictionary tempFileMappings; @@ -71,8 +70,7 @@ public PullRequestService( IVSGitExt gitExt, IApiClientFactory apiClientFactory, IGraphQLClientFactory graphqlFactory, - IOperatingSystem os, - IUsageTracker usageTracker) + IOperatingSystem os) : base(apiClientFactory, graphqlFactory) { this.gitClient = gitClient; @@ -80,7 +78,6 @@ public PullRequestService( this.gitExt = gitExt; this.graphqlFactory = graphqlFactory; this.os = os; - this.usageTracker = usageTracker; this.tempFileMappings = new Dictionary(StringComparer.OrdinalIgnoreCase); } @@ -1011,7 +1008,6 @@ async Task PushAndCreatePR(IModelService modelService, var ret = await modelService.CreatePullRequest(sourceRepository, targetRepository, sourceBranch, targetBranch, title, body); await MarkBranchAsPullRequest(repo, sourceBranch.Name, targetRepository.CloneUrl.Owner, ret.Number); gitExt.RefreshActiveRepositories(); - await usageTracker.IncrementCounter(x => x.NumberOfUpstreamPullRequests); return ret; } } diff --git a/src/GitHub.App/ViewModels/GitHubPane/PullRequestCreationViewModel.cs b/src/GitHub.App/ViewModels/GitHubPane/PullRequestCreationViewModel.cs index bd25c85c52..b0c4717f74 100644 --- a/src/GitHub.App/ViewModels/GitHubPane/PullRequestCreationViewModel.cs +++ b/src/GitHub.App/ViewModels/GitHubPane/PullRequestCreationViewModel.cs @@ -41,6 +41,7 @@ public class PullRequestCreationViewModel : PanePageViewModelBase, IPullRequestC readonly IMessageDraftStore draftStore; readonly IGitService gitService; readonly IScheduler timerScheduler; + readonly IUsageTracker usageTracker; readonly CompositeDisposable disposables = new CompositeDisposable(); LocalRepositoryModel activeLocalRepo; ObservableAsPropertyHelper githubRepository; @@ -53,8 +54,9 @@ public PullRequestCreationViewModel( INotificationService notifications, IMessageDraftStore draftStore, IGitService gitService, - IAutoCompleteAdvisor autoCompleteAdvisor) - : this(modelServiceFactory, service, notifications, draftStore, gitService, autoCompleteAdvisor, DefaultScheduler.Instance) + IAutoCompleteAdvisor autoCompleteAdvisor, + IUsageTracker usageTracker) + : this(modelServiceFactory, service, notifications, draftStore, gitService, autoCompleteAdvisor, usageTracker, DefaultScheduler.Instance) { } @@ -65,6 +67,7 @@ public PullRequestCreationViewModel( IMessageDraftStore draftStore, IGitService gitService, IAutoCompleteAdvisor autoCompleteAdvisor, + IUsageTracker usageTracker, IScheduler timerScheduler) { Guard.ArgumentNotNull(modelServiceFactory, nameof(modelServiceFactory)); @@ -81,6 +84,7 @@ public PullRequestCreationViewModel( this.gitService = gitService; this.AutoCompleteAdvisor = autoCompleteAdvisor; this.timerScheduler = timerScheduler; + this.usageTracker = usageTracker; this.WhenAnyValue(x => x.Branches) .WhereNotNull() @@ -107,8 +111,16 @@ public PullRequestCreationViewModel( .Subscribe(x => notifications.ShowError(BranchValidator.ValidationResult.Message)); CreatePullRequest = ReactiveCommand.CreateFromObservable( - () => service - .CreatePullRequest(modelService, activeLocalRepo, TargetBranch.Repository, SourceBranch, TargetBranch, PRTitle, Description ?? String.Empty) + () => Observable.FromAsync(async () => + { + var pullRequestModel = await service + .CreatePullRequest(modelService, activeLocalRepo, TargetBranch.Repository, SourceBranch, + TargetBranch, PRTitle, Description ?? String.Empty); + + await usageTracker.IncrementCounter(x => x.NumberOfUpstreamPullRequests); + + return pullRequestModel; + }) .Catch(ex => { log.Error(ex, "Error creating pull request"); @@ -120,6 +132,7 @@ public PullRequestCreationViewModel( return Observable.Empty(); }), whenAnyValidationResultChanges); + CreatePullRequest.Subscribe(pr => { notifications.ShowMessage(String.Format(CultureInfo.CurrentCulture, Resources.PRCreatedUpstream, SourceBranch.DisplayName, TargetBranch.Repository.Owner + "/" + TargetBranch.Repository.Name + "#" + pr.Number, diff --git a/test/GitHub.App.UnitTests/Services/PullRequestServiceTests.cs b/test/GitHub.App.UnitTests/Services/PullRequestServiceTests.cs index f56ab23f5d..1a57d6f5f3 100644 --- a/test/GitHub.App.UnitTests/Services/PullRequestServiceTests.cs +++ b/test/GitHub.App.UnitTests/Services/PullRequestServiceTests.cs @@ -578,8 +578,7 @@ static PullRequestService CreatePullRequestService(Repository repo) Substitute.For(), Substitute.For(), Substitute.For(), - serviceProvider.GetOperatingSystem(), - Substitute.For()); + serviceProvider.GetOperatingSystem()); return service; } @@ -692,8 +691,7 @@ public void CreatePullRequestAllArgsMandatory() Substitute.For(), Substitute.For(), Substitute.For(), - serviceProvider.GetOperatingSystem(), - Substitute.For()); + serviceProvider.GetOperatingSystem()); IModelService ms = null; LocalRepositoryModel sourceRepo = null; @@ -906,8 +904,7 @@ public async Task ShouldReturnCorrectDefaultLocalBranchNameForPullRequestsWithNo Substitute.For(), Substitute.For(), Substitute.For(), - Substitute.For(), - Substitute.For()); + Substitute.For()); var localRepo = new LocalRepositoryModel { }; var result = await service.GetDefaultLocalBranchName(localRepo, 123, "コードをレビューする準備ができたこと"); @@ -1064,8 +1061,7 @@ static PullRequestService CreateTarget( IVSGitExt gitExt = null, IApiClientFactory apiClientFactory = null, IGraphQLClientFactory graphqlFactory = null, - IOperatingSystem os = null, - IUsageTracker usageTracker = null) + IOperatingSystem os = null) { gitClient = gitClient ?? Substitute.For(); gitService = gitService ?? Substitute.For(); @@ -1073,7 +1069,6 @@ static PullRequestService CreateTarget( apiClientFactory = apiClientFactory ?? Substitute.For(); graphqlFactory = graphqlFactory ?? Substitute.For(); os = os ?? Substitute.For(); - usageTracker = usageTracker ?? Substitute.For(); return new PullRequestService( gitClient, @@ -1081,8 +1076,7 @@ static PullRequestService CreateTarget( gitExt, apiClientFactory, graphqlFactory, - os, - usageTracker); + os); } static BranchCollection MockBranches(params string[] names) diff --git a/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestCreationViewModelTests.cs b/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestCreationViewModelTests.cs index 2c2df0a648..dcd8eeaeae 100644 --- a/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestCreationViewModelTests.cs +++ b/test/GitHub.App.UnitTests/ViewModels/GitHubPane/PullRequestCreationViewModelTests.cs @@ -147,10 +147,10 @@ static RemoteRepositoryModel CreateRemoteRepositoryModel(Repository repository) public async Task TargetBranchDisplayNameIncludesRepoOwnerWhenForkAsync() { var data = PrepareTestData("octokit.net", "shana", "master", "octokit", "master", "origin", true, true); - var prservice = new PullRequestService(data.GitClient, data.GitService, Substitute.For(), Substitute.For(), Substitute.For(), data.ServiceProvider.GetOperatingSystem(), Substitute.For()); + var prservice = new PullRequestService(data.GitClient, data.GitService, Substitute.For(), Substitute.For(), Substitute.For(), data.ServiceProvider.GetOperatingSystem()); prservice.GetPullRequestTemplate(data.ActiveRepo).Returns(Observable.Empty()); var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, - Substitute.For(), data.GitService, data.AutoCompleteAdvisor); + Substitute.For(), data.GitService, data.AutoCompleteAdvisor, Substitute.For()); await vm.InitializeAsync(data.ActiveRepo, data.Connection); Assert.That("octokit/master", Is.EqualTo(vm.TargetBranch.DisplayName)); } @@ -184,9 +184,9 @@ public async Task CreatingPRsAsync( var targetBranch = data.TargetBranch; var ms = data.ModelService; - var prservice = new PullRequestService(data.GitClient, data.GitService, Substitute.For(), Substitute.For(), Substitute.For(), data.ServiceProvider.GetOperatingSystem(), Substitute.For()); + var prservice = new PullRequestService(data.GitClient, data.GitService, Substitute.For(), Substitute.For(), Substitute.For(), data.ServiceProvider.GetOperatingSystem()); var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, - Substitute.For(), data.GitService, data.AutoCompleteAdvisor); + Substitute.For(), data.GitService, data.AutoCompleteAdvisor, Substitute.For()); await vm.InitializeAsync(data.ActiveRepo, data.Connection); // the TargetBranch property gets set to whatever the repo default is (we assume master here), @@ -229,7 +229,7 @@ public async Task TemplateIsUsedIfPresentAsync() prservice.GetPullRequestTemplate(data.ActiveRepo).Returns(Observable.Return("Test PR template")); var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, - Substitute.For(), data.GitService, data.AutoCompleteAdvisor); + Substitute.For(), data.GitService, data.AutoCompleteAdvisor, Substitute.For()); await vm.InitializeAsync(data.ActiveRepo, data.Connection); Assert.That("Test PR template", Is.EqualTo(vm.Description)); @@ -249,7 +249,7 @@ public async Task LoadsDraft() var prservice = Substitute.For(); var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, - draftStore, data.GitService, data.AutoCompleteAdvisor); + draftStore, data.GitService, data.AutoCompleteAdvisor, Substitute.For()); await vm.InitializeAsync(data.ActiveRepo, data.Connection); Assert.That(vm.PRTitle, Is.EqualTo("This is a Title.")); @@ -264,7 +264,7 @@ public async Task UpdatesDraftWhenDescriptionChanges() var draftStore = Substitute.For(); var prservice = Substitute.For(); var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, - draftStore, data.GitService, data.AutoCompleteAdvisor, scheduler); + draftStore, data.GitService, data.AutoCompleteAdvisor, Substitute.For(), scheduler); await vm.InitializeAsync(data.ActiveRepo, data.Connection); vm.Description = "Body changed."; @@ -287,7 +287,7 @@ public async Task UpdatesDraftWhenTitleChanges() var draftStore = Substitute.For(); var prservice = Substitute.For(); var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, - draftStore, data.GitService, data.AutoCompleteAdvisor, scheduler); + draftStore, data.GitService, data.AutoCompleteAdvisor, Substitute.For(), scheduler); await vm.InitializeAsync(data.ActiveRepo, data.Connection); vm.PRTitle = "Title changed."; @@ -310,7 +310,7 @@ public async Task DeletesDraftWhenPullRequestSubmitted() var draftStore = Substitute.For(); var prservice = Substitute.For(); var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, draftStore, - data.GitService, data.AutoCompleteAdvisor, scheduler); + data.GitService, data.AutoCompleteAdvisor, Substitute.For(), scheduler); await vm.InitializeAsync(data.ActiveRepo, data.Connection); await vm.CreatePullRequest.Execute(); @@ -326,7 +326,7 @@ public async Task DeletesDraftWhenCanceled() var draftStore = Substitute.For(); var prservice = Substitute.For(); var vm = new PullRequestCreationViewModel(data.GetModelServiceFactory(), prservice, data.NotificationService, draftStore, - data.GitService, data.AutoCompleteAdvisor, scheduler); + data.GitService, data.AutoCompleteAdvisor, Substitute.For(), scheduler); await vm.InitializeAsync(data.ActiveRepo, data.Connection); await vm.Cancel.Execute();