launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

Commit message:
Allow modifying Git repository names; fix sending mail for repository deltas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1444591 in Launchpad itself: "Allow Git repository subscriptions"
  https://bugs.launchpad.net/launchpad/+bug/1444591

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-mail-fix-delta/+merge/257015

While Git repository subscriptions work now, there's no way to actually cause any mail to be sent due to various bugs elsewhere.  This makes it possible to change repository names, and fixes a missing part of GitRepositoryDelta's interface that caused the mailer to crash.  The whole assemblage is now tested.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-mail-fix-delta into lp:launchpad.

=== modified file 'lib/lp/code/adapters/gitrepository.py'
--- lib/lp/code/adapters/gitrepository.py	2015-04-16 14:35:17 +0000
+++ lib/lp/code/adapters/gitrepository.py	2015-04-21 23:25:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Components related to Git repositories."""
@@ -22,16 +22,18 @@
 
     implements(IGitRepositoryDelta)
 
-    delta_values = ('name', 'identity')
+    delta_values = ('name', 'git_identity')
+
+    new_values = ()
 
     interface = IGitRepository
 
-    def __init__(self, repository, user, name=None, identity=None):
+    def __init__(self, repository, user, name=None, git_identity=None):
         self.repository = repository
         self.user = user
 
         self.name = name
-        self.identity = identity
+        self.git_identity = git_identity
 
     @classmethod
     def construct(klass, old_repository, new_repository, user):

=== added file 'lib/lp/code/adapters/tests/test_gitrepository.py'
--- lib/lp/code/adapters/tests/test_gitrepository.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/adapters/tests/test_gitrepository.py	2015-04-21 23:25:53 +0000
@@ -0,0 +1,53 @@
+# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from lazr.lifecycle.snapshot import Snapshot
+from testtools.matchers import MatchesStructure
+from zope.interface import providedBy
+
+from lp.code.adapters.gitrepository import GitRepositoryDelta
+from lp.code.interfaces.gitrepository import GIT_FEATURE_FLAG
+from lp.services.features.testing import FeatureFixture
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import LaunchpadFunctionalLayer
+
+
+class TestGitRepositoryDelta(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestGitRepositoryDelta, self).setUp()
+        self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+    def test_no_modification(self):
+        # If there are no modifications, no delta is returned.
+        repository = self.factory.makeGitRepository(name=u"foo")
+        old_repository = Snapshot(repository, providing=providedBy(repository))
+        delta = GitRepositoryDelta.construct(
+            old_repository, repository, repository.owner)
+        self.assertIsNone(delta)
+
+    def test_modification(self):
+        # If there are modifications, the delta reflects them.
+        owner = self.factory.makePerson(name="person")
+        project = self.factory.makeProduct(name="project")
+        repository = self.factory.makeGitRepository(
+            owner=owner, target=project, name=u"foo")
+        old_repository = Snapshot(repository, providing=providedBy(repository))
+        with person_logged_in(repository.owner):
+            repository.setName(u"bar", repository.owner)
+        delta = GitRepositoryDelta.construct(old_repository, repository, owner)
+        self.assertIsNotNone(delta)
+        self.assertThat(delta, MatchesStructure.byEquality(
+            name={
+                "old": u"foo",
+                "new": u"bar",
+                },
+            git_identity={
+                "old": u"lp:~person/project/+git/foo",
+                "new": u"lp:~person/project/+git/bar",
+                }))

=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py	2015-04-17 00:01:15 +0000
+++ lib/lp/code/interfaces/gitrepository.py	2015-04-21 23:25:53 +0000
@@ -200,9 +200,9 @@
     def isPersonTrustedReviewer(reviewer):
         """Return true if the `reviewer` is a trusted reviewer.
 
-        The reviewer is trusted if they either own the repository, or are in the
-        team that owns the repository, or they are in the review team for the
-        repository.
+        The reviewer is trusted if they either own the repository, or are in
+        the team that owns the repository, or they are in the review team
+        for the repository.
         """
 
     git_identity = exported(Text(
@@ -490,6 +490,15 @@
 class IGitRepositoryEdit(Interface):
     """IGitRepository methods that require launchpad.Edit permission."""
 
+    @mutator_for(IGitRepositoryView["name"])
+    @call_with(user=REQUEST_USER)
+    @operation_parameters(
+        new_name=TextLine(title=_("The new name of the repository.")))
+    @export_write_operation()
+    @operation_for_version("devel")
+    def setName(new_name, user):
+        """Set the name of the repository to be `new_name`."""
+
     @mutator_for(IGitRepositoryView["owner"])
     @call_with(user=REQUEST_USER)
     @operation_parameters(

=== modified file 'lib/lp/code/model/gitnamespace.py'
--- lib/lp/code/model/gitnamespace.py	2015-04-16 23:56:21 +0000
+++ lib/lp/code/model/gitnamespace.py	2015-04-21 23:25:53 +0000
@@ -152,9 +152,10 @@
     def moveRepository(self, repository, mover, new_name=None,
                        rename_if_necessary=False):
         """See `IGitNamespace`."""
-        # Check to see if the repository is already in this namespace.
+        # Check to see if the repository is already in this namespace with
+        # this name.
         old_namespace = repository.namespace
-        if self.name == old_namespace.name:
+        if self.name == old_namespace.name and new_name is None:
             return
         if new_name is None:
             new_name = repository.name
@@ -164,10 +165,13 @@
         # Remove the security proxy of the repository as the owner and
         # target attributes are read-only through the interface.
         naked_repository = removeSecurityProxy(repository)
-        naked_repository.owner = self.owner
-        self._retargetRepository(naked_repository)
-        del get_property_cache(naked_repository).target
-        naked_repository.name = new_name
+        if self.owner != repository.owner:
+            naked_repository.owner = self.owner
+        if self.target != repository.target:
+            self._retargetRepository(naked_repository)
+            del get_property_cache(naked_repository).target
+        if new_name != repository.name:
+            naked_repository.name = new_name
 
     def getRepositories(self):
         """See `IGitNamespace`."""

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-04-21 11:39:09 +0000
+++ lib/lp/code/model/gitrepository.py	2015-04-21 23:25:53 +0000
@@ -622,6 +622,10 @@
         # subscriptions.
         getUtility(IRemoveArtifactSubscriptionsJobSource).create(user, [self])
 
+    def setName(self, new_name, user):
+        """See `IGitRepository`."""
+        self.namespace.moveRepository(self, user, new_name=new_name)
+
     def setOwner(self, new_owner, user):
         """See `IGitRepository`."""
         new_namespace = get_git_namespace(self.target, new_owner)

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2015-04-21 11:39:09 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2015-04-21 23:25:53 +0000
@@ -6,11 +6,14 @@
 __metaclass__ = type
 
 from datetime import datetime
+import email
 from functools import partial
 import hashlib
 import json
 
 from lazr.lifecycle.event import ObjectModifiedEvent
+from lazr.lifecycle.snapshot import Snapshot
+import transaction
 import pytz
 from testtools.matchers import (
     EndsWith,
@@ -19,6 +22,8 @@
     )
 from zope.component import getUtility
 from zope.event import notify
+from zope.interface import providedBy
+from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.enums import (
@@ -37,6 +42,7 @@
     GitFeatureDisabled,
     GitRepositoryCreatorNotMemberOfOwnerTeam,
     GitRepositoryCreatorNotOwner,
+    GitRepositoryExists,
     GitTargetError,
     )
 from lp.code.interfaces.defaultgit import ICanHasDefaultGitRepository
@@ -68,6 +74,7 @@
 from lp.registry.tests.test_accesspolicy import get_policies_for_artifact
 from lp.services.database.constants import UTC_NOW
 from lp.services.features.testing import FeatureFixture
+from lp.services.mail import stub
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.testing import (
@@ -288,16 +295,16 @@
             repository.getRepositoryIdentities())
 
 
-class TestGitRepositoryDateLastModified(TestCaseWithFactory):
-    """Exercise the situations where date_last_modified is updated."""
+class TestGitRepositoryModifications(TestCaseWithFactory):
+    """Tests for Git repository modification notifications."""
 
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        super(TestGitRepositoryDateLastModified, self).setUp()
+        super(TestGitRepositoryModifications, self).setUp()
         self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
 
-    def test_initial_value(self):
+    def test_date_last_modified_initial_value(self):
         # The initial value of date_last_modified is date_created.
         repository = self.factory.makeGitRepository()
         self.assertEqual(
@@ -314,6 +321,34 @@
         self.assertSqlAttributeEqualsDate(
             repository, "date_last_modified", UTC_NOW)
 
+    def test_sends_notifications(self):
+        # Attribute modifications send mail to subscribers.
+        self.assertEqual(0, len(stub.test_emails))
+        repository = self.factory.makeGitRepository(name=u"foo")
+        repository_before_modification = Snapshot(
+            repository, providing=providedBy(repository))
+        with person_logged_in(repository.owner):
+            repository.subscribe(
+                repository.owner,
+                BranchSubscriptionNotificationLevel.ATTRIBUTEONLY,
+                BranchSubscriptionDiffSize.NODIFF,
+                CodeReviewNotificationLevel.NOEMAIL,
+                repository.owner)
+            repository.setName(u"bar", repository.owner)
+            notify(ObjectModifiedEvent(
+                repository, repository_before_modification, ["name"],
+                user=repository.owner))
+        transaction.commit()
+        self.assertEqual(1, len(stub.test_emails))
+        message = email.message_from_string(stub.test_emails[0][2])
+        body = message.get_payload(decode=True)
+        self.assertIn("Name: foo => bar\n", body)
+        self.assertIn(
+            "Git identity: lp:~{person}/{project}/+git/foo => "
+            "lp:~{person}/{project}/+git/bar\n".format(
+                person=repository.owner.name, project=repository.target.name),
+            body)
+
     # XXX cjwatson 2015-02-04: This will need to be expanded once Launchpad
     # actually notices any interesting kind of repository modifications.
 
@@ -892,6 +927,39 @@
         self.assertNotTrustedReviewer(repository, reviewer)
 
 
+class TestGitRepositorySetName(TestCaseWithFactory):
+    """Test `IGitRepository.setName`."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGitRepositorySetName, self).setUp()
+        self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+    def test_not_owner(self):
+        # A non-owner non-admin user cannot rename a repository.
+        repository = self.factory.makeGitRepository()
+        with person_logged_in(self.factory.makePerson()):
+            self.assertRaises(Unauthorized, getattr, repository, "setName")
+
+    def test_name_clash(self):
+        # Name clashes are refused.
+        repository = self.factory.makeGitRepository(name=u"foo")
+        self.factory.makeGitRepository(
+            owner=repository.owner, target=repository.target, name=u"bar")
+        with person_logged_in(repository.owner):
+            self.assertRaises(
+                GitRepositoryExists, repository.setName,
+                u"bar", repository.owner)
+
+    def test_rename(self):
+        # A non-clashing rename request works.
+        repository = self.factory.makeGitRepository(name=u"foo")
+        with person_logged_in(repository.owner):
+            repository.setName(u"bar", repository.owner)
+        self.assertEqual(u"bar", repository.name)
+
+
 class TestGitRepositorySetOwner(TestCaseWithFactory):
     """Test `IGitRepository.setOwner`."""