launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-consistent-owner-target into lp:launchpad.

Commit message:
Make GitRepository.{setOwner,setTarget} behave more consistently for personal repositories.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-consistent-owner-target/+merge/261078

Make GitRepository.{setOwner,setTarget} behave more consistently for personal repositories.

setTarget needs to update the owner if you're retargeting from one personal repository namespace to another (which is a plausible thing for someone to try to do using the API since target == owner for personal repositories, and which makes edit forms in the UI easier to arrange); while setOwner needs to check the rules for private repository ownership.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-consistent-owner-target into lp:launchpad.

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-06-01 16:05:12 +0000
+++ lib/lp/code/model/gitrepository.py	2015-06-04 12:53:43 +0000
@@ -251,14 +251,16 @@
     def setTarget(self, target, user):
         """See `IGitRepository`."""
         if IPerson.providedBy(target):
-            owner = IPerson(target)
+            new_owner = IPerson(target)
             if (self.information_type in PRIVATE_INFORMATION_TYPES and
-                (not owner.is_team or
-                 owner.visibility != PersonVisibility.PRIVATE)):
+                (not new_owner.is_team or
+                 new_owner.visibility != PersonVisibility.PRIVATE)):
                 raise GitTargetError(
                     "Only private teams may have personal private "
                     "repositories.")
-        namespace = get_git_namespace(target, self.owner)
+        else:
+            new_owner = self.owner
+        namespace = get_git_namespace(target, new_owner)
         if (self.information_type not in
             namespace.getAllowedInformationTypes(user)):
             raise GitTargetError(
@@ -659,7 +661,17 @@
 
     def setOwner(self, new_owner, user):
         """See `IGitRepository`."""
-        new_namespace = get_git_namespace(self.target, new_owner)
+        if self.owner == self.target:
+            if (self.information_type in PRIVATE_INFORMATION_TYPES and
+                (not new_owner.is_team or
+                 new_owner.visibility != PersonVisibility.PRIVATE)):
+                raise GitTargetError(
+                    "Only private teams may have personal private "
+                    "repositories.")
+            new_target = new_owner
+        else:
+            new_target = self.target
+        new_namespace = get_git_namespace(new_target, new_owner)
         new_namespace.moveRepository(self, user, rename_if_necessary=True)
 
     @property

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2015-06-01 23:02:20 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2015-06-04 12:53:43 +0000
@@ -1394,12 +1394,54 @@
             repository.setOwner(person, admin)
             self.assertEqual(person, repository.owner)
 
+    def test_private_personal_forbidden_for_public_teams(self):
+        # Only private teams can have private personal repositories.
+        person = self.factory.makePerson()
+        private_team = self.factory.makeTeam(
+            owner=person, membership_policy=TeamMembershipPolicy.MODERATED,
+            visibility=PersonVisibility.PRIVATE)
+        public_team = self.factory.makeTeam(owner=person)
+        with person_logged_in(person):
+            repository = self.factory.makeGitRepository(
+                owner=private_team, target=private_team,
+                information_type=InformationType.USERDATA)
+            self.assertRaises(
+                GitTargetError, repository.setOwner, public_team, person)
+
+    def test_private_personal_allowed_for_private_teams(self):
+        # Only private teams can have private personal repositories.
+        person = self.factory.makePerson()
+        private_team_1 = self.factory.makeTeam(
+            owner=person, membership_policy=TeamMembershipPolicy.MODERATED,
+            visibility=PersonVisibility.PRIVATE)
+        private_team_2 = self.factory.makeTeam(
+            owner=person, membership_policy=TeamMembershipPolicy.MODERATED,
+            visibility=PersonVisibility.PRIVATE)
+        with person_logged_in(person):
+            repository = self.factory.makeGitRepository(
+                owner=private_team_1, target=private_team_1,
+                information_type=InformationType.USERDATA)
+            repository.setOwner(private_team_2, person)
+            self.assertEqual(private_team_2, repository.owner)
+            self.assertEqual(private_team_2, repository.target)
+
 
 class TestGitRepositorySetTarget(TestCaseWithFactory):
     """Test `IGitRepository.setTarget`."""
 
     layer = DatabaseFunctionalLayer
 
+    def test_personal_to_other_personal(self):
+        # A personal repository can be moved to a different owner.
+        person = self.factory.makePerson()
+        team = self.factory.makeTeam(owner=person)
+        repository = self.factory.makeGitRepository(
+            owner=person, target=person)
+        with person_logged_in(person):
+            repository.setTarget(target=team, user=repository.owner)
+        self.assertEqual(team, repository.owner)
+        self.assertEqual(team, repository.target)
+
     def test_personal_to_project(self):
         # A personal repository can be moved to a project.
         owner = self.factory.makePerson()