launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

Commit message:
When moving a repository, check that the target_default and owner_default flags fit the target namespace.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-fix-default-flags/+merge/257708

When moving a repository, check that the target_default and owner_default flags fit the target namespace.  That is, we should refuse if you try to move the default repository for a target to a different target which already has a default, or a person's default repository for a target to a different person/target combination which already has a default.  I thought about unsetting the existing default automatically, but I think the less magical behaviour is better, at least to start with.

I added a few GitNamespace tests to go with this, which I didn't add initially due to lack of sufficient infrastructure.  We could flesh those out now, but that doesn't have to be done in this branch.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-fix-default-flags into lp:launchpad.

=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2015-04-19 12:56:32 +0000
+++ lib/lp/code/errors.py	2015-04-28 23:42:22 +0000
@@ -404,15 +404,15 @@
         params = {
             "unique_name": existing_repository.unique_name,
             "target": target.displayname,
-            "owner": owner.displayname,
             }
         if owner is None:
             message = (
                 "The default repository for '%(target)s' is already set to "
                 "%(unique_name)s." % params)
         else:
+            params["owner"] = owner.displayname
             message = (
-                "%(owner)'s default repository for '%(target)s' is already "
+                "%(owner)s's default repository for '%(target)s' is already "
                 "set to %(unique_name)s." % params)
         self.existing_repository = existing_repository
         self.target = target

=== modified file 'lib/lp/code/interfaces/gitnamespace.py'
--- lib/lp/code/interfaces/gitnamespace.py	2015-04-22 16:11:40 +0000
+++ lib/lp/code/interfaces/gitnamespace.py	2015-04-28 23:42:22 +0000
@@ -134,6 +134,16 @@
             validation constraints on IGitRepository.name.
         """
 
+    def validateDefaultFlags(repository):
+        """Check that any default flags on 'repository' fit this namespace.
+
+        :param repository: An `IGitRepository` to check.
+        :raises GitDefaultConflict: If the repository has the target_default
+            flag set but this namespace already has a target default, or if
+            the repository has the owner_default flag set but this namespace
+            already has an owner-target default.
+        """
+
     def validateMove(repository, mover, name=None):
         """Check that 'mover' can move 'repository' into this namespace.
 

=== modified file 'lib/lp/code/model/gitnamespace.py'
--- lib/lp/code/model/gitnamespace.py	2015-04-22 16:11:40 +0000
+++ lib/lp/code/model/gitnamespace.py	2015-04-28 23:42:22 +0000
@@ -34,6 +34,7 @@
     CodeReviewNotificationLevel,
     )
 from lp.code.errors import (
+    GitDefaultConflict,
     GitRepositoryCreationForbidden,
     GitRepositoryCreatorNotMemberOfOwnerTeam,
     GitRepositoryCreatorNotOwner,
@@ -47,6 +48,7 @@
     )
 from lp.code.interfaces.gitrepository import (
     IGitRepository,
+    IGitRepositorySet,
     user_has_special_git_repository_access,
     )
 from lp.code.interfaces.hasgitrepositories import IHasGitRepositories
@@ -146,12 +148,29 @@
         # "gitrepository" violates check constraint "valid_name"...'.
         IGitRepository['name'].validate(unicode(name))
 
+    def validateDefaultFlags(self, repository):
+        """See `IGitNamespace`."""
+        repository_set = getUtility(IGitRepositorySet)
+        if repository.target_default and self.target != repository.target:
+            existing = repository_set.getDefaultRepository(self.target)
+            if existing is not None:
+                raise GitDefaultConflict(existing, self.target)
+        if (repository.owner_default and
+            (self.owner != repository.owner or
+             self.target != repository.target)):
+            existing = repository_set.getDefaultRepositoryForOwner(
+                self.owner, self.target)
+            if existing is not None:
+                raise GitDefaultConflict(
+                    existing, self.target, owner=self.owner)
+
     def validateMove(self, repository, mover, name=None):
         """See `IGitNamespace`."""
         if name is None:
             name = repository.name
         self.validateRepositoryName(name)
         self.validateRegistrant(mover)
+        self.validateDefaultFlags(repository)
 
     def moveRepository(self, repository, mover, new_name=None,
                        rename_if_necessary=False):

=== added file 'lib/lp/code/model/tests/test_gitnamespace.py'
--- lib/lp/code/model/tests/test_gitnamespace.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/model/tests/test_gitnamespace.py	2015-04-28 23:42:22 +0000
@@ -0,0 +1,145 @@
+# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `IGitNamespace` implementations."""
+
+from zope.component import getUtility
+
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.code.errors import (
+    GitDefaultConflict,
+    GitRepositoryExists,
+    )
+from lp.code.interfaces.gitrepository import (
+    GIT_FEATURE_FLAG,
+    IGitRepositorySet,
+    )
+from lp.code.model.gitnamespace import ProjectGitNamespace
+from lp.services.features.testing import FeatureFixture
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import DatabaseFunctionalLayer
+
+
+class TestGitNamespaceMoveRepository(TestCaseWithFactory):
+    """Test the IGitNamespace.moveRepository method."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGitNamespaceMoveRepository, self).setUp()
+        self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+    def assertNamespacesEqual(self, expected, result):
+        """Assert that the namespaces refer to the same thing.
+
+        The name of the namespace contains the user name and the context
+        parts, so is the easiest thing to check.
+        """
+        self.assertEqual(expected.name, result.name)
+
+    def test_move_to_same_namespace(self):
+        # Moving to the same namespace is effectively a no-op.  No
+        # exceptions about matching repository names should be raised.
+        repository = self.factory.makeGitRepository()
+        namespace = repository.namespace
+        namespace.moveRepository(repository, repository.owner)
+        self.assertNamespacesEqual(namespace, repository.namespace)
+
+    def test_name_clash_raises(self):
+        # A name clash will raise an exception.
+        repository = self.factory.makeGitRepository(name=u"test")
+        another = self.factory.makeGitRepository(
+            owner=repository.owner, name=u"test")
+        namespace = another.namespace
+        self.assertRaises(
+            GitRepositoryExists, namespace.moveRepository,
+            repository, repository.owner)
+
+    def test_move_with_rename(self):
+        # A name clash with 'rename_if_necessary' set to True will cause the
+        # repository to be renamed instead of raising an error.
+        repository = self.factory.makeGitRepository(name=u"test")
+        another = self.factory.makeGitRepository(
+            owner=repository.owner, name=u"test")
+        namespace = another.namespace
+        namespace.moveRepository(
+            repository, repository.owner, rename_if_necessary=True)
+        self.assertEqual("test-1", repository.name)
+        self.assertNamespacesEqual(namespace, repository.namespace)
+
+    def test_move_with_new_name(self):
+        # A new name for the repository can be specified as part of the move.
+        repository = self.factory.makeGitRepository(name=u"test")
+        another = self.factory.makeGitRepository(
+            owner=repository.owner, name=u"test")
+        namespace = another.namespace
+        namespace.moveRepository(repository, repository.owner, new_name=u"foo")
+        self.assertEqual("foo", repository.name)
+        self.assertNamespacesEqual(namespace, repository.namespace)
+
+    def test_sets_repository_owner(self):
+        # Moving to a new namespace may change the owner of the repository
+        # if the owner of the namespace is different.
+        repository = self.factory.makeGitRepository(name=u"test")
+        team = self.factory.makeTeam(repository.owner)
+        project = self.factory.makeProduct()
+        namespace = ProjectGitNamespace(team, project)
+        namespace.moveRepository(repository, repository.owner)
+        self.assertEqual(team, repository.owner)
+        # And for paranoia.
+        self.assertNamespacesEqual(namespace, repository.namespace)
+
+    def test_target_default_clash_raises(self):
+        # A clash between target_default repositories will raise an exception.
+        repository = self.factory.makeGitRepository()
+        repository.setTargetDefault(True)
+        another = self.factory.makeGitRepository()
+        another.setTargetDefault(True)
+        self.assertRaisesWithContent(
+            GitDefaultConflict,
+            "The default repository for '%s' is already set to %s." % (
+                another.target.displayname, another.unique_name),
+            another.namespace.moveRepository,
+            repository, getUtility(ILaunchpadCelebrities).admin.teamowner)
+
+    def test_owner_default_clash_raises(self):
+        # A clash between owner_default repositories will raise an exception.
+        repository = self.factory.makeGitRepository()
+        repository.setOwnerDefault(True)
+        another = self.factory.makeGitRepository()
+        another.setOwnerDefault(True)
+        self.assertRaisesWithContent(
+            GitDefaultConflict,
+            "%s's default repository for '%s' is already set to %s." % (
+                another.owner.displayname, another.target.displayname,
+                another.unique_name),
+            another.namespace.moveRepository,
+            repository, getUtility(ILaunchpadCelebrities).admin.teamowner)
+
+    def test_preserves_target_default(self):
+        # If there is no clash, target_default is preserved.
+        repository = self.factory.makeGitRepository()
+        repository.setTargetDefault(True)
+        another = self.factory.makeGitRepository()
+        namespace = another.namespace
+        namespace.moveRepository(
+            repository, getUtility(ILaunchpadCelebrities).admin.teamowner)
+        self.assertNamespacesEqual(namespace, repository.namespace)
+        repository_set = getUtility(IGitRepositorySet)
+        self.assertEqual(
+            repository, repository_set.getDefaultRepository(another.target))
+
+    def test_preserves_owner_default(self):
+        # If there is no clash, owner_default is preserved.
+        repository = self.factory.makeGitRepository()
+        repository.setOwnerDefault(True)
+        another = self.factory.makeGitRepository()
+        namespace = another.namespace
+        namespace.moveRepository(
+            repository, getUtility(ILaunchpadCelebrities).admin.teamowner)
+        self.assertNamespacesEqual(namespace, repository.namespace)
+        repository_set = getUtility(IGitRepositorySet)
+        self.assertEqual(
+            repository,
+            repository_set.getDefaultRepositoryForOwner(
+                another.owner, another.target))