launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-fix-clone-from-owner-default into lp:launchpad.

Commit message:
Fix code to get the owner default repository if there is no target default.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-fix-clone-from-owner-default/+merge/257622

The code to find the owner default if there's no target default used the wrong IGitRepositorySet method.  This fixes it and adds a test.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-fix-clone-from-owner-default into lp:launchpad.

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2015-04-28 04:48:55 +0000
+++ lib/lp/code/xmlrpc/git.py	2015-04-28 10:32:26 +0000
@@ -200,12 +200,15 @@
             # If repository has target_default, clone from default.
             target_path = None
             try:
-                target_default = self.repository_set.getDefaultRepository(
-                    repository.target) or (
-                        self.repository_set.getDefaultRepository(
-                    repository.owner))
-                if target_default and target_default.visibleByUser(requester):
-                    target_path = target_default.getInternalPath()
+                default = self.repository_set.getDefaultRepository(
+                    repository.target)
+                if default is not None and default.visibleByUser(requester):
+                    target_path = default.getInternalPath()
+                else:
+                    default = self.repository_set.getDefaultRepositoryForOwner(
+                        repository.owner, repository.target)
+                    if default is not None and default.visibleByUser(requester):
+                        target_path = default.getInternalPath()
             except GitTargetError:
                 pass  # Ignore Personal repositories.
 

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2015-04-28 04:48:55 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2015-04-28 10:32:26 +0000
@@ -217,13 +217,12 @@
             self.git_api.hosting_client.calls[0][0:2])
         return repository
 
-    def assertCreatesFromClone(self, requester, path, can_authenticate=False):
-        repository = self.assertCreates(requester, path, can_authenticate)
-        target_default = removeSecurityProxy(
-            self.repository_set.getDefaultRepository(repository.target))
-        self.assertIsNotNone(target_default)
-        self.assertEqual(target_default.getInternalPath(),
-                         self.git_api.hosting_client.calls[0][2])
+    def assertCreatesFromClone(self, requester, path, cloned_from,
+                               can_authenticate=False):
+        self.assertCreates(requester, path, can_authenticate)
+        self.assertEqual(
+            cloned_from.getInternalPath(),
+            self.git_api.hosting_client.calls[0][2])
 
     def test_translatePath_private_repository(self):
         requester = self.factory.makePerson()
@@ -427,17 +426,32 @@
         self.assertCreates(
             requester, u"/~%s/%s/+git/random" % (requester.name, project.name))
 
-    def test_translatePath_create_project_with_default_target(self):
-        # translatePath creates a project repository cloned from the
-        # target default if it exists.
+    def test_translatePath_create_project_clone_from_target_default(self):
+        # translatePath creates a project repository cloned from the target
+        # default if it exists.
         target = self.factory.makeProduct()
         repository = self.factory.makeGitRepository(
             owner=target.owner, target=target)
         with person_logged_in(target.owner):
             self.repository_set.setDefaultRepository(target, repository)
             self.assertCreatesFromClone(
-                target.owner, u"/~%s/%s/+git/random" % (target.owner.name,
-                                                        target.name))
+                target.owner,
+                u"/~%s/%s/+git/random" % (target.owner.name, target.name),
+                repository)
+
+    def test_translatePath_create_project_clone_from_owner_default(self):
+        # translatePath creates a project repository cloned from the owner
+        # default if it exists and the target default does not.
+        target = self.factory.makeProduct()
+        repository = self.factory.makeGitRepository(
+            owner=target.owner, target=target)
+        with person_logged_in(target.owner):
+            self.repository_set.setDefaultRepositoryForOwner(
+                target.owner, target, repository)
+            self.assertCreatesFromClone(
+                target.owner,
+                u"/~%s/%s/+git/random" % (target.owner.name, target.name),
+                repository)
 
     def test_translatePath_create_package(self):
         # translatePath creates a package repository that doesn't exist, if