launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-643345 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
  #643345 Using preferredemail as a public email id is wrong and broken.
  https://bugs.launchpad.net/bugs/643345


= Bug 643345 =

For cherrypicking.

DirectBranchCommit has been changed in such a way that it errors out when the committer has no preferred email (which is often the case with teams).  This is breaking our daily translations-to-branch exports for at least three dozen productseries.

This branch fixes that.  It makes several changes:
 * Allows a user of DirectBranchCommit to specify a bzr committer id as a string.
 * Makes DirectBranchCommit fall back to a safe default for the committer id.
 * Documents and tests the choice of default committer (it's the branch owner).
 * Sets a more accurate committer string in translations exports.
 * Cleans out the obsolete boilerplate that we used to need at the bottom of each unit test.
 * Sanitizes the inheritance tree for some test classes.

There are probably better fixes for the general case.  This branch lays some groundwork for them, but is primarily intended to solve our immediate problem.


To test:
{{{
./bin/test -vvc lp.code.tests.test_directbranchcommit
./bin/test -vvc lp.translations -t branch
}}}


No lint,

Jeroen
-- 
https://code.launchpad.net/~jtv/launchpad/bug-643345/+merge/36258
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-643345 into lp:launchpad/devel.

=== modified file 'lib/lp/code/model/directbranchcommit.py'
--- lib/lp/code/model/directbranchcommit.py	2010-09-10 14:44:43 +0000
+++ lib/lp/code/model/directbranchcommit.py	2010-09-22 06:21:06 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Commit files straight to bzr branch."""
@@ -21,6 +21,7 @@
     TransformPreview,
     )
 
+from canonical.config import config
 from canonical.launchpad.interfaces import IMasterObject
 from lp.codehosting.bzrutils import get_stacked_on_url
 from lp.services.mail.sendmail import format_address_for_person
@@ -56,7 +57,7 @@
     commit_builder = None
 
     def __init__(self, db_branch, committer=None, no_race_check=False,
-            merge_parents=None):
+                 merge_parents=None, committer_string=None):
         """Create context for direct commit to branch.
 
         Before constructing a `DirectBranchCommit`, set up a server that
@@ -74,9 +75,13 @@
         `DirectBranchCommit`.
 
         :param db_branch: a Launchpad `Branch` object.
-        :param committer: the `Person` writing to the branch.
+        :param committer: the `Person` writing to the branch.  Defaults to
+            the branch owner.
         :param no_race_check: don't check for other commits before committing
             our changes, for use in tests.
+        :param committer_string: Optional description (typically with email
+            address) of the committer for use in bzr.  If not given, the
+            `committer`'s email address will be used instead.
         """
         self.db_branch = db_branch
 
@@ -85,6 +90,7 @@
         if committer is None:
             committer = db_branch.owner
         self.committer = committer
+        self.committer_string = committer_string
 
         self.no_race_check = no_race_check
 
@@ -176,6 +182,17 @@
             raise ConcurrentUpdateError(
                 "Branch has been changed.  Not committing.")
 
+    def getBzrCommitterID(self):
+        """Find the committer id to pass to bzr."""
+        if self.committer_string is not None:
+            return self.committer_string
+        elif self.committer.preferredemail is not None:
+            return format_address_for_person(self.committer)
+        else:
+            return '"%s" <%s>' % (
+                self.committer.displayname,
+                config.canonical.noreply_from_address)
+
     def commit(self, commit_message, txn=None):
         """Commit to branch.
 
@@ -197,7 +214,7 @@
             if rev_id == NULL_REVISION:
                 if list(self.transform_preview.iter_changes()) == []:
                     return
-            committer_id = format_address_for_person(self.committer)
+            committer_id = self.getBzrCommitterID()
             # XXX: AaronBentley 2010-08-06 bug=614404: a bzr username is
             # required to generate the revision-id.
             with override_environ(BZR_EMAIL=committer_id):

=== modified file 'lib/lp/code/tests/test_directbranchcommit.py'
--- lib/lp/code/tests/test_directbranchcommit.py	2010-09-10 14:44:43 +0000
+++ lib/lp/code/tests/test_directbranchcommit.py	2010-09-22 06:21:06 +0000
@@ -1,12 +1,10 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `DirectBranchCommit`."""
 
 __metaclass__ = type
 
-from unittest import TestLoader
-
 from canonical.testing.layers import ZopelessDatabaseLayer
 from lp.code.model.directbranchcommit import (
     ConcurrentUpdateError,
@@ -16,9 +14,10 @@
     map_branch_contents,
     TestCaseWithFactory,
     )
-
-
-class DirectBranchCommitTestCase(TestCaseWithFactory):
+from lp.testing.fakemethod import FakeMethod
+
+
+class DirectBranchCommitTestCase:
     """Base class for `DirectBranchCommit` tests."""
     db_branch = None
     committer = None
@@ -54,11 +53,16 @@
         return map_branch_contents(self.committer.db_branch.getBzrBranch())
 
 
-class TestDirectBranchCommit(DirectBranchCommitTestCase):
+class TestDirectBranchCommit(DirectBranchCommitTestCase, TestCaseWithFactory):
     """Test `DirectBranchCommit`."""
 
     layer = ZopelessDatabaseLayer
 
+    def test_defaults_to_branch_owner(self):
+        # If no committer is given, DirectBranchCommits defaults to
+        # attributing changes to the branch owner.
+        self.assertEqual(self.db_branch.owner, self.committer.committer)
+
     def test_DirectBranchCommit_empty_initial_commit_noop(self):
         # An empty initial commit to a branch is a no-op.
         self.assertEqual('null:', self.tree.branch.last_revision())
@@ -204,8 +208,22 @@
         revid = self.committer.commit('')
         self.assertEqual(revid, self.db_branch.last_mirrored_id)
 
-
-class TestDirectBranchCommit_getDir(DirectBranchCommitTestCase):
+    def test_commit_uses_getBzrCommitterID(self):
+        # commit() passes self.getBzrCommitterID() to bzr as the
+        # committer.
+        bzr_id = self.factory.getUniqueString()
+        self.committer.getBzrCommitterID = FakeMethod(result=bzr_id)
+        fake_commit = FakeMethod()
+        self.committer.transform_preview.commit = fake_commit
+
+        self.committer.writeFile('x', 'y')
+        self.committer.commit('')
+
+        commit_args, commit_kwargs = fake_commit.calls[-1]
+        self.assertEqual(bzr_id, commit_kwargs['committer'])
+
+
+class TestGetDir(DirectBranchCommitTestCase, TestCaseWithFactory):
     """Test `DirectBranchCommit._getDir`."""
 
     layer = ZopelessDatabaseLayer
@@ -259,5 +277,47 @@
         self.assertEqual(dir_id, self.committer._getDir('foo/bar'))
 
 
-def test_suite():
-    return TestLoader().loadTestsFromName(__name__)
+class TestGetBzrCommitterID(TestCaseWithFactory):
+    """Test `DirectBranchCommitter.getBzrCommitterID`."""
+
+    layer = ZopelessDatabaseLayer
+
+    def setUp(self):
+        super(TestGetBzrCommitterID, self).setUp()
+        self.useBzrBranches(direct_database=True)
+        self.committer = None
+
+    def tearDown(self):
+        if self.committer:
+            self.committer.unlock()
+        super(TestGetBzrCommitterID, self).tearDown()
+
+    def _makeBranch(self, **kwargs):
+        """Create a branch in the database and in bzr."""
+        db_branch, tree = self.create_branch_and_tree(**kwargs)
+        return db_branch
+
+    def test_uses_committer_string(self):
+        # getBzrCommitterID uses the committer string if provided.
+        bzr_id = self.factory.getUniqueString()
+        self.committer = DirectBranchCommit(
+            self._makeBranch(), committer_string=bzr_id)
+        self.assertEqual(bzr_id, self.committer.getBzrCommitterID())
+
+    def test_uses_committer_email(self):
+        # getBzrCommitterID returns the committing person's email address
+        # if available (and if no committer string is given).
+        branch = self._makeBranch()
+        self.committer = DirectBranchCommit(branch)
+        self.assertIn(
+            branch.owner.preferredemail.email,
+            self.committer.getBzrCommitterID())
+
+    def test_falls_back_to_noreply(self):
+        # If all else fails, getBzrCommitterID uses the noreply
+        # address.
+        team = self.factory.makeTeam()
+        self.assertIs(None, team.preferredemail)
+        branch = self._makeBranch(owner=team)
+        self.committer = DirectBranchCommit(branch)
+        self.assertIn('noreply', self.committer.getBzrCommitterID())

=== modified file 'lib/lp/translations/scripts/tests/test_translations_to_branch.py'
--- lib/lp/translations/scripts/tests/test_translations_to_branch.py	2010-08-20 20:31:18 +0000
+++ lib/lp/translations/scripts/tests/test_translations_to_branch.py	2010-09-22 06:21:06 +0000
@@ -5,7 +5,6 @@
 
 import re
 from textwrap import dedent
-import unittest
 
 from bzrlib.errors import NotBranchError
 import transaction
@@ -266,6 +265,19 @@
         # fail either.
         transaction.commit()
 
+    def test_sets_bzr_id(self):
+        # The script commits to the branch under a user id that mentions
+        # the automatic translations exports as well as the Launchpad
+        # name of the branch owner.
+        self.useBzrBranches(direct_database=False)
+        exporter = ExportTranslationsToBranch(test_args=[])
+        branch, tree = self.create_branch_and_tree()
+        committer = exporter._makeDirectBranchCommit(branch)
+        committer.unlock()
+        self.assertEqual(
+            "Launchpad Translations for %s" % branch.owner.name,
+            committer.getBzrCommitterID())
+
 
 class TestExportToStackedBranch(TestCaseWithFactory):
     """Test workaround for bzr bug 375013."""
@@ -306,7 +318,3 @@
             committer.commit("x!")
         finally:
             committer.unlock()
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
--- lib/lp/translations/scripts/translations_to_branch.py	2010-08-31 23:03:45 +0000
+++ lib/lp/translations/scripts/translations_to_branch.py	2010-09-22 06:21:06 +0000
@@ -95,12 +95,11 @@
     def _makeDirectBranchCommit(self, db_branch):
         """Create a `DirectBranchCommit`.
 
-        This factory is a mock-injection point for tests.
-
         :param db_branch: A `Branch` object as defined in Launchpad.
         :return: A `DirectBranchCommit` for `db_branch`.
         """
-        return DirectBranchCommit(db_branch)
+        committer = 'Launchpad Translations for %s' % db_branch.owner.name
+        return DirectBranchCommit(db_branch, committer_string=committer)
 
     def _prepareBranchCommit(self, db_branch):
         """Prepare branch for use with `DirectBranchCommit`.