Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit 1083f00

Browse files
shanashana
committed
ViewModels only notify if there are errors
If operations are successful, we know from the result of the command, so ViewModels only call the notification service in case of errors (so we can grab the message). Also, they shouldn't be hooking directly into the VS UI, that's for the view to handle (or whoever is controlling the view, if the view is embedded and the error needs to be shown somewhere else) Fix tests so that they look for the correct events when things change and not rely on the notification service for anything other than errors.
1 parent 023e534 commit 1083f00

File tree

4 files changed

+44
-80
lines changed

4 files changed

+44
-80
lines changed

‎src/GitHub.App/ViewModels/RepositoryPublishViewModel.cs‎

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,7 @@ public RepositoryPublishViewModel(
5555
this.repositoryPublishService = repositoryPublishService;
5656

5757
if (Connections.Any())
58-
{
5958
SelectedConnection = Connections.FirstOrDefault(x => x.HostAddress.IsGitHubDotCom()) ?? Connections[0];
60-
}
6159

6260
accounts = this.WhenAny(x => x.SelectedConnection, x => x.Value != null ? hosts.LookupHost(x.Value.HostAddress) : RepositoryHosts.DisconnectedRepositoryHost)
6361
.Where(x => !(x is DisconnectedRepositoryHost))
@@ -71,9 +69,7 @@ public RepositoryPublishViewModel(
7169
.Subscribe(accts => {
7270
var selectedAccount = accts.FirstOrDefault();
7371
if (selectedAccount != null)
74-
{
7572
SelectedAccount = accts.FirstOrDefault();
76-
}
7773
});
7874

7975
isHostComboBoxVisible = this.WhenAny(x => x.Connections, x => x.Value)
@@ -94,9 +90,7 @@ public RepositoryPublishViewModel(
9490

9591
var defaultRepositoryName = repositoryPublishService.LocalRepositoryName;
9692
if (!string.IsNullOrEmpty(defaultRepositoryName))
97-
{
98-
DefaultRepositoryName = defaultRepositoryName;
99-
}
93+
DefaultRepositoryName = defaultRepositoryName;
10094

10195
this.WhenAny(x => x.SelectedConnection, x => x.SelectedAccount,
10296
(a,b) => true)
@@ -156,11 +150,7 @@ IObservable<ProgressState> OnPublishRepository(object arg)
156150
var account = SelectedAccount;
157151

158152
return repositoryPublishService.PublishRepository(newRepository, account, SelectedHost.ApiClient)
159-
.Select(_ =>
160-
{
161-
notificationService.ShowMessage("Repository published successfully.");
162-
return ProgressState.Success;
163-
})
153+
.Select(_ => ProgressState.Success)
164154
.Catch<ProgressState, Exception>(ex =>
165155
{
166156
if (!ex.IsCriticalException())
@@ -190,21 +180,6 @@ void InitializeValidation()
190180
var parsedReference = GetSafeRepositoryName(repoName);
191181
return parsedReference != repoName ? String.Format(CultureInfo.CurrentCulture, Resources.SafeRepositoryNameWarning, parsedReference) : null;
192182
});
193-
194-
this.WhenAny(x => x.SafeRepositoryNameWarningValidator.ValidationResult, x => x.Value)
195-
.WhereNotNull() // When this is instantiated, it sends a null result.
196-
.Select(result => result?.Message)
197-
.Subscribe(message =>
198-
{
199-
if (!string.IsNullOrEmpty(message))
200-
{
201-
notificationService.ShowWarning(message);
202-
}
203-
else
204-
{
205-
notificationService.ClearNotifications();
206-
}
207-
});
208183
}
209184
}
210185
}

‎src/GitHub.VisualStudio/TeamExplorer/Connect/GitHubConnectSection.cs‎

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -326,22 +326,7 @@ void StartFlow(UIControllerFlow controllerFlow)
326326
{
327327
var notifications = ServiceProvider.GetExportedValue<INotificationDispatcher>();
328328
var teServices = ServiceProvider.GetExportedValue<ITeamExplorerServices>();
329-
var messages = Observable.Merge(
330-
notifications.Listen()
331-
.Where(n => n.Type == Notification.NotificationType.Message)
332-
.Do(m => teServices.ShowMessage(m.Message)),
333-
notifications.Listen()
334-
.Where(n => n.Type == Notification.NotificationType.MessageCommand)
335-
.Do(m => teServices.ShowMessage(m.Message, m.Command)),
336-
notifications.Listen()
337-
.Where(n => n.Type == Notification.NotificationType.Warning)
338-
.Do(m => teServices.ShowWarning(m.Message)),
339-
notifications.Listen()
340-
.Where(n => n.Type == Notification.NotificationType.Error)
341-
.Do(m => teServices.ShowError(m.Message))
342-
)
343-
.Subscribe();
344-
329+
notifications.AddListener(teServices);
345330

346331
var uiProvider = ServiceProvider.GetExportedValue<IUIProvider>();
347332
uiProvider.GitServiceProvider = ServiceProvider;
@@ -355,7 +340,7 @@ void StartFlow(UIControllerFlow controllerFlow)
355340
});
356341
uiProvider.RunUI();
357342

358-
messages.Dispose();
343+
notifications.RemoveListener();
359344
}
360345

361346
bool disposed;

‎src/GitHub.VisualStudio/UI/Views/Controls/RepositoryPublishControl.xaml.cs‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
using NullGuard;
1010
using ReactiveUI;
1111
using System.ComponentModel.Composition;
12+
using GitHub.Extensions.Reactive;
13+
using GitHub.Services;
1214

1315
namespace GitHub.VisualStudio.UI.Views.Controls
1416
{
@@ -19,7 +21,8 @@ public class GenericRepositoryPublishControl : SimpleViewUserControl<IRepository
1921
[PartCreationPolicy(CreationPolicy.NonShared)]
2022
public partial class RepositoryPublishControl : GenericRepositoryPublishControl
2123
{
22-
public RepositoryPublishControl()
24+
[ImportingConstructor]
25+
public RepositoryPublishControl(ITeamExplorerServices teServices)
2326
{
2427
InitializeComponent();
2528

@@ -51,7 +54,18 @@ public RepositoryPublishControl()
5154
});
5255

5356
d(this.WhenAny(x => x.ViewModel.IsPublishing, x => x.Value)
54-
.Subscribe(x => NotifyIsBusy(x)));
57+
.Subscribe(x => NotifyIsBusy(x)));
58+
59+
d(this.WhenAny(x => x.ViewModel.SafeRepositoryNameWarningValidator.ValidationResult, x => x.Value)
60+
.WhereNotNull()
61+
.Select(result => result?.Message)
62+
.Subscribe(message =>
63+
{
64+
if (!string.IsNullOrEmpty(message))
65+
teServices.ShowWarning(message);
66+
else
67+
teServices.ClearNotifications();
68+
}));
5569

5670
nameText.Text = ViewModel.DefaultRepositoryName;
5771
});

‎src/UnitTests/GitHub.App/ViewModels/RepositoryPublishViewModelTests.cs‎

Lines changed: 24 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -298,52 +298,29 @@ public void IsFalseWhenRepoNameIsNotSafe()
298298
}
299299

300300
[Fact]
301-
public void DisplaysWarningWhenRepoNameNotSafeAndClearsItWhenSafeAgain()
301+
public void ResetsSafeNameValidator()
302302
{
303303
var cm = Substitutes.ConnectionManager;
304304
var hosts = Substitute.For<IRepositoryHosts>();
305-
var notificationService = Substitute.For<INotificationService>();
306-
var vm = Helpers.SetupConnectionsAndViewModel(hosts, notificationService: notificationService, cm: cm);
305+
var vm = Helpers.SetupConnectionsAndViewModel(hosts, cm: cm);
307306

308-
notificationService.DidNotReceive().ShowWarning(Args.String);
307+
vm.RepositoryName = "this";
308+
Assert.True(vm.SafeRepositoryNameWarningValidator.ValidationResult.IsValid);
309309

310310
vm.RepositoryName = "this is bad";
311311
Assert.Equal("this-is-bad", vm.SafeRepositoryName);
312-
313-
notificationService.Received().ShowWarning("Will be created as this-is-bad");
314-
notificationService.DidNotReceive().ClearNotifications();
312+
Assert.False(vm.SafeRepositoryNameWarningValidator.ValidationResult.IsValid);
315313

316314
vm.RepositoryName = "this";
317315

318-
notificationService.Received().ClearNotifications();
316+
Assert.True(vm.SafeRepositoryNameWarningValidator.ValidationResult.IsValid);
319317
}
320318
}
321319

322320
public class ThePublishRepositoryCommand : TestBaseClass
323321
{
324322
[Fact]
325-
public async Task DisplaysSuccessMessageWhenCompletedWithoutError()
326-
{
327-
var cm = Substitutes.ConnectionManager;
328-
var hosts = Substitute.For<IRepositoryHosts>();
329-
var notificationService = Substitute.For<INotificationService>();
330-
331-
var repositoryPublishService = Substitute.For<IRepositoryPublishService>();
332-
repositoryPublishService.PublishRepository(Args.NewRepository, Args.Account, Args.ApiClient)
333-
.Returns(Observable.Return(new Octokit.Repository()));
334-
335-
var vm = Helpers.SetupConnectionsAndViewModel(hosts, repositoryPublishService, notificationService, cm);
336-
337-
vm.RepositoryName = "repo-name";
338-
339-
await vm.PublishRepository.ExecuteAsync().Catch(Observable.Return(ProgressState.Success));
340-
341-
notificationService.Received().ShowMessage("Repository published successfully.");
342-
notificationService.DidNotReceive().ShowError(Args.String);
343-
}
344-
345-
[Fact]
346-
public async Task DisplaysRepositoryExistsErrorWithVisualStudioNotifications()
323+
public async Task RepositoryExistsCallsNotificationServiceWithError()
347324
{
348325
var cm = Substitutes.ConnectionManager;
349326
var hosts = Substitute.For<IRepositoryHosts>();
@@ -357,12 +334,13 @@ public async Task DisplaysRepositoryExistsErrorWithVisualStudioNotifications()
357334

358335
await vm.PublishRepository.ExecuteAsync().Catch(Observable.Return(ProgressState.Fail));
359336

337+
Assert.NotNull(vm.SafeRepositoryNameWarningValidator.ValidationResult.Message);
360338
notificationService.DidNotReceive().ShowMessage(Args.String);
361339
notificationService.Received().ShowError("There is already a repository named 'repo-name' for the current account.");
362340
}
363341

364342
[Fact]
365-
public async Task ClearsErrorsWhenSwitchingHosts()
343+
public async Task ResetsWhenSwitchingHosts()
366344
{
367345
var args = Helpers.GetArgs(GitHubUrls.GitHub, "https://ghe.io");
368346

@@ -390,13 +368,19 @@ public async Task ClearsErrorsWhenSwitchingHosts()
390368

391369
await vm.PublishRepository.ExecuteAsync().Catch(Observable.Return(ProgressState.Fail));
392370

371+
Assert.Equal("repo-name", vm.RepositoryName);
372+
notificationService.Received().ShowError("There is already a repository named 'repo-name' for the current account.");
373+
374+
var wasCalled = false;
375+
vm.SafeRepositoryNameWarningValidator.PropertyChanged += (s, e) => wasCalled = true;
376+
393377
vm.SelectedConnection = conns.First(x => x != vm.SelectedConnection);
394378

395-
notificationService.Received().ClearNotifications();
379+
Assert.True(wasCalled);
396380
}
397381

398382
[Fact]
399-
public async Task ClearsErrorsWhenSwitchingAccounts()
383+
public async Task ResetsWhenSwitchingAccounts()
400384
{
401385
var cm = Substitutes.ConnectionManager;
402386
var hosts = Substitute.For<IRepositoryHosts>();
@@ -421,9 +405,15 @@ public async Task ClearsErrorsWhenSwitchingAccounts()
421405

422406
await vm.PublishRepository.ExecuteAsync().Catch(Observable.Return(ProgressState.Fail));
423407

408+
Assert.Equal("repo-name", vm.RepositoryName);
409+
notificationService.Received().ShowError("There is already a repository named 'repo-name' for the current account.");
410+
411+
var wasCalled = false;
412+
vm.SafeRepositoryNameWarningValidator.PropertyChanged += (s, e) => wasCalled = true;
413+
424414
vm.SelectedAccount = accounts[1];
425415

426-
notificationService.Received().ClearNotifications();
416+
Assert.True(wasCalled);
427417
}
428418
}
429419
}

0 commit comments

Comments
 (0)