Merge pull request #1906 from github/refactor/clone-include-owner-in-path

Include owner in default path when cloning a repository

Author: jcansdale

src/GitHub.App/ViewModels/Dialog/Clone/RepositoryCloneViewModel.cs

@@ -25,6 +25,7 @@ public class RepositoryCloneViewModel : ViewModelBase, IRepositoryCloneViewModel
         readonly IRepositoryCloneService service;
         readonly IReadOnlyList<IRepositoryCloneTabViewModel> tabs;
         string path;
+        IRepositoryModel previousRepository;
         ObservableAsPropertyHelper<string> pathError;
         int selectedTabIndex;
 
@@ -54,7 +55,7 @@ public RepositoryCloneViewModel(
 
             pathError = Observable.CombineLatest(
                 repository,
-                this.WhenAnyValue(x => x.Path), 
+                this.WhenAnyValue(x => x.Path),
                 ValidatePath)
                 .ToProperty(this, x => x.PathError);
 
@@ -138,19 +139,59 @@ void BrowseForDirectory()
             }
         }
 
-        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 = GetUpdatedBasePath(Path);
+                previousRepository = repository;
+                Path = System.IO.Path.Combine(basePath, repository.Owner, repository.Name);
+            }
+        }
+
+        string GetUpdatedBasePath(string path)
+        {
+            if (string.IsNullOrEmpty(path))
+            {
+                return service.DefaultClonePath;
+            }
+
+            if (previousRepository == null)
+            {
+                return path;
+            }
+
+            if (FindDirWithout(path, previousRepository?.Owner, 2) is string dirWithoutOwner)
+            {
+                return dirWithoutOwner;
+            }
+
+            if (FindDirWithout(path, previousRepository?.Name, 1) is string dirWithoutRepo)
+            {
+                return dirWithoutRepo;
+            }
+
+            return path;
+
+            string FindDirWithout(string dir, string match, int levels)
+            {
+                string dirWithout = null;
+                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 == match)
+                    {
+                        dirWithout = dir;
+                    }
                 }
+
+                return dirWithout;
             }
         }
 

test/GitHub.App.UnitTests/ViewModels/Dialog/Clone/RepositoryCloneViewModelTests.cs

@@ -111,15 +111,13 @@ 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();
-            var repository = Substitute.For<IRepositoryModel>();
 
-            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]
@@ -136,10 +134,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<IRepositoryModel>();
-
-            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));
@@ -149,13 +144,47 @@ 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<IRepositoryModel>();
 
-            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\\{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", "c:\\base\\owner2\\repo2",
+            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);
+            SetRepository(target.GitHubTab, CreateRepositoryModel(repo1));
+            target.Path = userPath;
+
+            SetRepository(target.GitHubTab, CreateRepositoryModel(repo2));
 
-            Assert.That(target.Path, Is.EqualTo("d:\\efault\\repo"));
+            Assert.That(target.Path, Is.EqualTo(expectPath));
         }
 
         [Test]
@@ -175,7 +204,7 @@ public async Task Clone_Is_Enabled_When_Repository_Selected()
 
             await target.InitializeAsync(null);
 
-            SetRepository(target.GitHubTab, Substitute.For<IRepositoryModel>());
+            SetRepository(target.GitHubTab, CreateRepositoryModel());
 
             Assert.That(target.Clone.CanExecute(null), Is.True);
         }
@@ -187,7 +216,7 @@ public async Task Clone_Is_Disabled_When_Has_PathError()
 
             await target.InitializeAsync(null);
 
-            SetRepository(target.GitHubTab, Substitute.For<IRepositoryModel>());
+            SetRepository(target.GitHubTab, CreateRepositoryModel());
             Assert.That(target.Clone.CanExecute(null), Is.True);
 
             target.Path = "d:\\exists";
@@ -233,10 +262,10 @@ static IRepositorySelectViewModel CreateSelectViewModel()
             return result;
         }
 
-        static IRepositoryCloneService CreateRepositoryCloneService()
+        static IRepositoryCloneService CreateRepositoryCloneService(string defaultClonePath)
         {
             var result = Substitute.For<IRepositoryCloneService>();
-            result.DefaultClonePath.Returns("d:\\efault\\path");
+            result.DefaultClonePath.Returns(defaultClonePath);
             result.DestinationExists("d:\\exists").Returns(true);
             return result;
         }
@@ -247,11 +276,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<IOperatingSystem>();
             connectionManager = connectionManager ?? CreateConnectionManager("https://github.com");
-            service = service ?? CreateRepositoryCloneService();
+            service = service ?? CreateRepositoryCloneService(defaultClonePath);
             gitHubTab = gitHubTab ?? CreateSelectViewModel();
             enterpriseTab = enterpriseTab ?? CreateSelectViewModel();
             urlTab = urlTab ?? Substitute.For<IRepositoryUrlViewModel>();
@@ -264,5 +294,20 @@ static RepositoryCloneViewModel CreateTarget(
                 enterpriseTab,
                 urlTab);
         }
+
+        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<IRepositoryModel>();
+            repository.Owner.Returns(owner);
+            repository.Name.Returns(name);
+            return repository;
+        }
     }
 }