Merge lp:~jtv/launchpad/bug-643345 into lp:launchpad

[Jeroen T. Vermeulen]

Fixes translations-to-branch exports and DirectBranchCommit.

= 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

[Tim Penhey (thumper)]

For the TestGetBzrCommitterID test case, remove the tearDown, and instead
do something like:

committer = DirectBranchCommit(...)
self.addCleanup(committer.unlock)

The test should also work in the DatabaseFunctionalLayer rather than the
ZopelessDatabaseLayer. Unless there is a reason why you are running it in
that layer, in which case, document it.

How about "Launchpad Translations on behalf of %s"?

review: Needs Fixing

[Michael Nelson (michael.nelson)]

I didn't realise Tim was already reviewing this, but here were my notes up until I did:

> === 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 07:16:44 +0000
> @@ -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.

It wasn't obvious to me without looking below that this isn't an actual commit
string provided by the committer.

> """
> 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)

The bug history shows Aaron changing the title of the bug to:
"Using preferredemail as a public email id is wrong and broken."

If so, should the above elif be included?

> + else:
> + return '"%s" <%s>' % (
> + self.committer.displayname,
> + config.canonical.noreply_from_address)
> +
> def commit(self, commit_message, txn=None):
> """Commit to branch.
>

IRC log:
09:26 < noodles775> jtv: only things I've thought so far are:
09:27 < noodles775> 1) It wasn't obvious to me without reading the code below that committer_string isn't an actual commit string provided (somehow) by the committer. Not sure if there's a better name, or maybe its just me.
09:28 < noodles775> 2) In getBzrCommitterID you're using self.committer.preferredemail, even though abentley's change of the related bug title seems to indicate this is wrong?
09:28 < noodles775> (but I don't know the background)
09:29 < jtv> noodles775: I'd be happy to come up with a better name for 1. As for 2, that falls under the "laying groundwork" part—there must be better fixes, but for now I just want the failures out of the way.
09:30 < noodles775> Yes, but (regarding 2) couldn't you just remove that elif for the moment to get the failures out of the way?
09:30 < thumper> jtv: reviewed
09:30 < jtv> noodles775: well AIUI it's wrong _because_ it's not guaranteed to be present.
09:31 <...

[Jeroen T. Vermeulen (jtv)]

> For the TestGetBzrCommitterID test case, remove the tearDown, and instead
> do something like:
>
> committer = DirectBranchCommit(...)
> self.addCleanup(committer.unlock)

Great! Done.

> The test should also work in the DatabaseFunctionalLayer rather than the
> ZopelessDatabaseLayer. Unless there is a reason why you are running it in
> that layer, in which case, document it.

Changed the layer.

> How about "Launchpad Translations on behalf of %s"?

I was hesitant to put it like that because it's longer, but agree it's better. Done.

[Tim Penhey (thumper)]

review: Approve

[Tim Penhey (thumper)]

review: Approve (production-change)

=== 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 07:52:42 +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_id=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_id: Optional identification (typically with email
+            address) of the person doing the commit, 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_id = committer_id
 
         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_id is not None:
+            return self.committer_id
+        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 07:52:42 +0000
@@ -1,13 +1,14 @@
-# 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 canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    ZopelessDatabaseLayer,
+    )
 from lp.code.model.directbranchcommit import (
     ConcurrentUpdateError,
     DirectBranchCommit,
@@ -16,9 +17,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 +56,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 +211,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 +280,43 @@
         self.assertEqual(dir_id, self.committer._getDir('foo/bar'))
 
 
-def test_suite():
-    return TestLoader().loadTestsFromName(__name__)
+class TestGetBzrCommitterID(TestCaseWithFactory):
+    """Test `DirectBranchCommitter.getBzrCommitterID`."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGetBzrCommitterID, self).setUp()
+        self.useBzrBranches(direct_database=True)
+
+    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_id(self):
+        # getBzrCommitterID uses the committer string if provided.
+        bzr_id = self.factory.getUniqueString()
+        committer = DirectBranchCommit(
+            self._makeBranch(), committer_id=bzr_id)
+        self.addCleanup(committer.unlock)
+        self.assertEqual(bzr_id, 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()
+        committer = DirectBranchCommit(branch)
+        self.addCleanup(committer.unlock)
+        self.assertIn(
+            branch.owner.preferredemail.email, 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)
+        committer = DirectBranchCommit(branch)
+        self.addCleanup(committer.unlock)
+        self.assertIn('noreply', 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 07:52:42 +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 on behalf of %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 07:52:42 +0000
@@ -95,12 +95,12 @@
     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_id = 'Launchpad Translations on behalf of %s' % (
+            db_branch.owner.name)
+        return DirectBranchCommit(db_branch, committer_id=committer_id)
 
     def _prepareBranchCommit(self, db_branch):
         """Prepare branch for use with `DirectBranchCommit`.