From 7d3e80e983ab3039bb6329cd6ddb8ead579ee3e8 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 10 Sep 2018 09:38:05 +0100 Subject: [PATCH 1/9] Check owner is included in default repository path Expect test to fail until functionality implemented. --- .../Clone/RepositoryCloneViewModelTests.cs | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs index 4e4eb56579..b9c976cdce 100644 --- a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs +++ b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs @@ -113,12 +113,10 @@ public async Task Path_Is_Initialized() public async Task Repository_Name_Is_Appended_To_Base_Path() { var target = CreateTarget(); - var repository = Substitute.For(); - repository.Name.Returns("repo"); - SetRepository(target.GitHubTab, repository); + SetRepository(target.GitHubTab, CreateRepositoryModel("owner", "repo")); - Assert.That(target.Path, Is.EqualTo("d:\\efault\\path\\repo")); + Assert.That(target.Path, Is.EqualTo("d:\\efault\\path\\owner\\repo")); } [Test] @@ -135,10 +133,7 @@ public async Task PathError_Is_Not_Set_When_No_Repository_Selected() public async Task PathError_Is_Set_For_Existing_Destination() { var target = CreateTarget(); - var repository = Substitute.For(); - - repository.Name.Returns("repo"); - SetRepository(target.GitHubTab, repository); + SetRepository(target.GitHubTab, CreateRepositoryModel("owner", "repo")); target.Path = "d:\\exists"; Assert.That(target.PathError, Is.EqualTo(Resources.DestinationAlreadyExists)); @@ -148,13 +143,14 @@ public async Task PathError_Is_Set_For_Existing_Destination() public async Task Repository_Name_Replaces_Last_Part_Of_Non_Base_Path() { var target = CreateTarget(); - var repository = Substitute.For(); - target.Path = "d:\\efault\\foo"; - repository.Name.Returns("repo"); - SetRepository(target.GitHubTab, repository); + var owner = "owner"; + target.Path = "d:\\efault"; + SetRepository(target.GitHubTab, CreateRepositoryModel(owner, "name")); + target.Path = $"d:\\efault\\{owner}\\foo"; + SetRepository(target.GitHubTab, CreateRepositoryModel(owner, "repo")); - Assert.That(target.Path, Is.EqualTo("d:\\efault\\repo")); + Assert.That(target.Path, Is.EqualTo($"d:\\efault\\{owner}\\repo")); } [Test] @@ -174,7 +170,7 @@ public async Task Clone_Is_Enabled_When_Repository_Selected() await target.InitializeAsync(null); - SetRepository(target.GitHubTab, Substitute.For()); + SetRepository(target.GitHubTab, CreateRepositoryModel()); Assert.That(target.Clone.CanExecute(null), Is.True); } @@ -186,7 +182,7 @@ public async Task Clone_Is_Disabled_When_Has_PathError() await target.InitializeAsync(null); - SetRepository(target.GitHubTab, Substitute.For()); + SetRepository(target.GitHubTab, CreateRepositoryModel()); Assert.That(target.Clone.CanExecute(null), Is.True); target.Path = "d:\\exists"; @@ -260,5 +256,13 @@ static RepositoryCloneViewModel CreateTarget( enterpriseTab, urlTab); } + + static IRepositoryModel CreateRepositoryModel(string owner = "owner", string name = "repo") + { + var repository = Substitute.For(); + repository.Owner.Returns(owner); + repository.Name.Returns(name); + return repository; + } } } From e248f52d0b703b663d779994917c0575e684bac0 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 10 Sep 2018 10:07:55 +0100 Subject: [PATCH 2/9] Include owner in default clone path Keep the base path consistent if user edits the fully qualified path. --- .../Dialog/Clone/RepositoryCloneViewModel.cs | 39 ++++++++++++++----- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs b/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs index 0bb06dbd1c..f82a05fcc4 100644 --- a/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs +++ b/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs @@ -23,6 +23,7 @@ public class RepositoryCloneViewModel : ViewModelBase, IRepositoryCloneViewModel readonly IRepositoryCloneService service; readonly IReadOnlyList tabs; string path; + string previousOwner; ObservableAsPropertyHelper pathError; int selectedTabIndex; @@ -50,7 +51,7 @@ public RepositoryCloneViewModel( pathError = Observable.CombineLatest( repository, - this.WhenAnyValue(x => x.Path), + this.WhenAnyValue(x => x.Path), ValidatePath) .ToProperty(this, x => x.PathError); @@ -113,20 +114,38 @@ public async Task InitializeAsync(IConnection connection) this.WhenAnyValue(x => x.SelectedTabIndex).Subscribe(x => tabs[x].Activate().Forget()); } - void UpdatePath(IRepositoryModel x) + void UpdatePath(IRepositoryModel repository) { - if (x != null) + if (repository != null) { - if (Path == service.DefaultClonePath) - { - Path = System.IO.Path.Combine(Path, x.Name); - } - else + var basePath = GetBasePath(Path, previousOwner); + previousOwner = repository.Owner; + Path = System.IO.Path.Combine(basePath, repository.Owner, repository.Name); + } + } + + static string GetBasePath(string path, string owner) + { + if (owner != null) + { + var dir = path; + for (var i = 0; i < 2; i++) { - var basePath = System.IO.Path.GetDirectoryName(Path); - Path = System.IO.Path.Combine(basePath, x.Name); + if (string.IsNullOrEmpty(dir)) + { + break; + } + + var name = System.IO.Path.GetFileName(dir); + dir = System.IO.Path.GetDirectoryName(dir); + if (name == owner) + { + return dir; + } } } + + return path; } string ValidatePath(IRepositoryModel repository, string path) From 5cd8a9109c28c9e0b6878ca9562a80996a43fb8a Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Mon, 10 Sep 2018 10:49:06 +0100 Subject: [PATCH 3/9] Add tests for when user edits path Check different combinations of repository and path changes. --- .../Clone/RepositoryCloneViewModelTests.cs | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs index b9c976cdce..2997e0fab8 100644 --- a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs +++ b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs @@ -153,6 +153,38 @@ public async Task Repository_Name_Replaces_Last_Part_Of_Non_Base_Path() Assert.That(target.Path, Is.EqualTo($"d:\\efault\\{owner}\\repo")); } + [TestCase("c:\\base", "owner1/repo1", "c:\\base\\owner1\\repo1", "owner2/repo2", "c:\\base\\owner2\\repo2", + Description = "Path unchanged")] + [TestCase("c:\\base", "owner1/repo1", "c:\\base\\owner1\\changed", "owner2/repo2", "c:\\base\\owner2\\repo2", + Description = "Repo name changed")] + [TestCase("c:\\base", "owner1/repo1", "c:\\base\\owner1", "owner2/repo2", "c:\\base\\owner2\\repo2", + Description = "Repo name deleted")] + [TestCase("c:\\base", "owner1/repo1", "c:\\base", "owner2/repo2", "c:\\base\\owner2\\repo2", + Description = "Base path reverted")] + + [TestCase("c:\\base", "owner1/repo1", "c:\\new\\base\\owner1\\changed", "owner2/repo2", "c:\\new\\base\\owner2\\repo2", + Description = "Base path and repo name changed")] + [TestCase("c:\\base", "owner1/repo1", "c:\\new\\base\\owner1", "owner2/repo2", "c:\\new\\base\\owner2\\repo2", + Description = "Base path changed and repo name deleted")] + [TestCase("c:\\base", "owner1/repo1", "c:\\new\\base", "owner2/repo2", "c:\\new\\base\\owner2\\repo2", + Description = "Base path changed and repo owner/name deleted")] + + [TestCase("c:\\base", "owner1/repo1", "", "owner2/repo2", "owner2\\repo2", + Description = "Base path cleared")] + [TestCase("c:\\base", "owner1/repo1", "c:\\base\\repo1", "owner2/repo2", "c:\\base\\repo1\\owner2\\repo2", + Description = "Owner deleted looks like base path change")] + public async Task User_Edits_Path(string basePath, string repo1, string userPath, string repo2, string expectPath) + { + var target = CreateTarget(); + + target.Path = basePath; + SetRepository(target.GitHubTab, CreateRepositoryModel(repo1)); + target.Path = userPath; + SetRepository(target.GitHubTab, CreateRepositoryModel(repo2)); + + Assert.That(target.Path, Is.EqualTo(expectPath)); + } + [Test] public async Task Clone_Is_Initially_Disabled() { @@ -257,7 +289,14 @@ static RepositoryCloneViewModel CreateTarget( urlTab); } - static IRepositoryModel CreateRepositoryModel(string owner = "owner", string name = "repo") + static IRepositoryModel CreateRepositoryModel(string repo = "owner/repo") + { + var split = repo.Split('/'); + var (owner, name) = (split[0], split[1]); + return CreateRepositoryModel(owner, name); + } + + static IRepositoryModel CreateRepositoryModel(string owner, string name) { var repository = Substitute.For(); repository.Owner.Returns(owner); From 0e885b2b581a3ba19d6d3abe8ba4577c593a6722 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 11 Sep 2018 10:35:04 +0100 Subject: [PATCH 4/9] Check restore DefaultClonePath when cleared Test that if the target Path is cleared, DefaultClonePath will be used as the base path on next selection. --- .../Clone/RepositoryCloneViewModelTests.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs index e8dffd44ab..d19a8eb47f 100644 --- a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs +++ b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs @@ -170,17 +170,16 @@ public async Task Repository_Name_Replaces_Last_Part_Of_Non_Base_Path() [TestCase("c:\\base", "owner1/repo1", "c:\\new\\base", "owner2/repo2", "c:\\new\\base\\owner2\\repo2", Description = "Base path changed and repo owner/name deleted")] - [TestCase("c:\\base", "owner1/repo1", "", "owner2/repo2", "owner2\\repo2", + [TestCase("c:\\base", "owner1/repo1", "", "owner2/repo2", "c:\\base\\owner2\\repo2", Description = "Base path cleared")] [TestCase("c:\\base", "owner1/repo1", "c:\\base\\repo1", "owner2/repo2", "c:\\base\\repo1\\owner2\\repo2", Description = "Owner deleted looks like base path change")] - public async Task User_Edits_Path(string basePath, string repo1, string userPath, string repo2, string expectPath) + public async Task User_Edits_Path(string defaultClonePath, string repo1, string userPath, string repo2, string expectPath) { - var target = CreateTarget(); - - target.Path = basePath; + var target = CreateTarget(defaultClonePath: defaultClonePath); SetRepository(target.GitHubTab, CreateRepositoryModel(repo1)); target.Path = userPath; + SetRepository(target.GitHubTab, CreateRepositoryModel(repo2)); Assert.That(target.Path, Is.EqualTo(expectPath)); @@ -261,10 +260,10 @@ static IRepositorySelectViewModel CreateSelectViewModel() return result; } - static IRepositoryCloneService CreateRepositoryCloneService() + static IRepositoryCloneService CreateRepositoryCloneService(string defaultClonePath) { var result = Substitute.For(); - result.DefaultClonePath.Returns("d:\\efault\\path"); + result.DefaultClonePath.Returns(defaultClonePath); result.DestinationExists("d:\\exists").Returns(true); return result; } @@ -275,11 +274,12 @@ static RepositoryCloneViewModel CreateTarget( IRepositoryCloneService service = null, IRepositorySelectViewModel gitHubTab = null, IRepositorySelectViewModel enterpriseTab = null, - IRepositoryUrlViewModel urlTab = null) + IRepositoryUrlViewModel urlTab = null, + string defaultClonePath = "d:\\efault\\path") { os = os ?? Substitute.For(); connectionManager = connectionManager ?? CreateConnectionManager("https://github.com"); - service = service ?? CreateRepositoryCloneService(); + service = service ?? CreateRepositoryCloneService(defaultClonePath); gitHubTab = gitHubTab ?? CreateSelectViewModel(); enterpriseTab = enterpriseTab ?? CreateSelectViewModel(); urlTab = urlTab ?? Substitute.For(); From 54503025a1f6058b269737b31f7b91d7568d7801 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 11 Sep 2018 10:38:01 +0100 Subject: [PATCH 5/9] Restore DefaultClonePath if Path is cleared If Path is cleared, restore the DefaultClonePath on next selection. --- .../Dialog/Clone/RepositoryCloneViewModel.cs | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs b/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs index 4580755430..19125c7952 100644 --- a/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs +++ b/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs @@ -149,24 +149,31 @@ void UpdatePath(IRepositoryModel repository) } } - static string GetBasePath(string path, string owner) + string GetBasePath(string path, string owner) { - if (owner != null) + if (string.IsNullOrEmpty(path)) { - var dir = path; - for (var i = 0; i < 2; i++) + return service.DefaultClonePath; + } + + if (string.IsNullOrEmpty(owner)) + { + return path; + } + + var dir = path; + for (var i = 0; i < 2; i++) + { + if (string.IsNullOrEmpty(dir)) + { + break; + } + + var name = System.IO.Path.GetFileName(dir); + dir = System.IO.Path.GetDirectoryName(dir); + if (name == owner) { - if (string.IsNullOrEmpty(dir)) - { - break; - } - - var name = System.IO.Path.GetFileName(dir); - dir = System.IO.Path.GetDirectoryName(dir); - if (name == owner) - { - return dir; - } + return dir; } } From fca3a96eee66417ff8ccf3d1ed25e399115be31a Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 11 Sep 2018 11:06:10 +0100 Subject: [PATCH 6/9] Check when just owner has been deleted Test that we don't duplicate the repo on next selection. --- .../ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs index d19a8eb47f..56dd3836e1 100644 --- a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs +++ b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs @@ -172,8 +172,8 @@ public async Task Repository_Name_Replaces_Last_Part_Of_Non_Base_Path() [TestCase("c:\\base", "owner1/repo1", "", "owner2/repo2", "c:\\base\\owner2\\repo2", Description = "Base path cleared")] - [TestCase("c:\\base", "owner1/repo1", "c:\\base\\repo1", "owner2/repo2", "c:\\base\\repo1\\owner2\\repo2", - Description = "Owner deleted looks like base path change")] + [TestCase("c:\\base", "owner1/repo1", "c:\\base\\repo1", "owner2/repo2", "c:\\base\\owner2\\repo2", + Description = "Owner deleted")] public async Task User_Edits_Path(string defaultClonePath, string repo1, string userPath, string repo2, string expectPath) { var target = CreateTarget(defaultClonePath: defaultClonePath); From 785c5f6659d3dab4ab6780363342f1f3e3ec6a94 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 11 Sep 2018 11:12:28 +0100 Subject: [PATCH 7/9] Handle when just owner has been deleted Don't include old the repository name when next repository is selected. --- .../Dialog/Clone/RepositoryCloneViewModel.cs | 48 ++++++++++++------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs b/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs index 19125c7952..3a2a3c67f4 100644 --- a/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs +++ b/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs @@ -25,7 +25,7 @@ public class RepositoryCloneViewModel : ViewModelBase, IRepositoryCloneViewModel readonly IRepositoryCloneService service; readonly IReadOnlyList tabs; string path; - string previousOwner; + IRepositoryModel previousRepository; ObservableAsPropertyHelper pathError; int selectedTabIndex; @@ -143,41 +143,55 @@ void UpdatePath(IRepositoryModel repository) { if (repository != null) { - var basePath = GetBasePath(Path, previousOwner); - previousOwner = repository.Owner; + var basePath = GetUpdatedBasePath(Path); + previousRepository = repository; Path = System.IO.Path.Combine(basePath, repository.Owner, repository.Name); } } - string GetBasePath(string path, string owner) + string GetUpdatedBasePath(string path) { if (string.IsNullOrEmpty(path)) { return service.DefaultClonePath; } - if (string.IsNullOrEmpty(owner)) + if (previousRepository == null) { return path; } - var dir = path; - for (var i = 0; i < 2; i++) + if (FindDirWithout(path, previousRepository?.Owner, 2) is string dirWithoutOwner) { - if (string.IsNullOrEmpty(dir)) - { - break; - } + return dirWithoutOwner; + } - var name = System.IO.Path.GetFileName(dir); - dir = System.IO.Path.GetDirectoryName(dir); - if (name == owner) - { - return dir; - } + if (FindDirWithout(path, previousRepository?.Name, 1) is string dirWithoutRepo) + { + return dirWithoutRepo; } return path; + + string FindDirWithout(string dir, string match, int levels) + { + for (var i = 0; i < 2; i++) + { + if (string.IsNullOrEmpty(dir)) + { + break; + } + + var name = System.IO.Path.GetFileName(dir); + dir = System.IO.Path.GetDirectoryName(dir); + if (name == match) + { + return dir; + } + } + + return null; + } } string ValidatePath(IRepositoryModel repository, string path) From 37d479060260ce0cc1b2a22d9933fc0edf0bf503 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Tue, 11 Sep 2018 11:18:57 +0100 Subject: [PATCH 8/9] Handle case where owner and repo have same name --- .../ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs | 5 +++-- .../ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs b/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs index 3a2a3c67f4..c4a2581d51 100644 --- a/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs +++ b/src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs @@ -175,6 +175,7 @@ string GetUpdatedBasePath(string path) string FindDirWithout(string dir, string match, int levels) { + string dirWithout = null; for (var i = 0; i < 2; i++) { if (string.IsNullOrEmpty(dir)) @@ -186,11 +187,11 @@ string FindDirWithout(string dir, string match, int levels) dir = System.IO.Path.GetDirectoryName(dir); if (name == match) { - return dir; + dirWithout = dir; } } - return null; + return dirWithout; } } diff --git a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs index 56dd3836e1..eec924f739 100644 --- a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs +++ b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs @@ -174,6 +174,8 @@ public async Task Repository_Name_Replaces_Last_Part_Of_Non_Base_Path() Description = "Base path cleared")] [TestCase("c:\\base", "owner1/repo1", "c:\\base\\repo1", "owner2/repo2", "c:\\base\\owner2\\repo2", Description = "Owner deleted")] + [TestCase("c:\\base", "same/same", "c:\\base\\same\\same", "owner2/repo2", "c:\\base\\owner2\\repo2", + Description = "Owner and repo have same name")] public async Task User_Edits_Path(string defaultClonePath, string repo1, string userPath, string repo2, string expectPath) { var target = CreateTarget(defaultClonePath: defaultClonePath); From 48b369c1f5be759369f5c0961801e7bb1299b5e6 Mon Sep 17 00:00:00 2001 From: Jamie Cansdale Date: Thu, 13 Sep 2018 11:27:09 +0100 Subject: [PATCH 9/9] Correct test name We're now adding the owner and repository name to the path. --- .../ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs index eec924f739..24ee79aa47 100644 --- a/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs +++ b/test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs @@ -111,7 +111,7 @@ public async Task Path_Is_Initialized() } [Test] - public async Task Repository_Name_Is_Appended_To_Base_Path() + public async Task Owner_And_Repository_Name_Is_Appended_To_Base_Path() { var target = CreateTarget();