Launchpad itself

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

  1. bug-643345
  2. Merge into devel
Proposed by Jeroen T. Vermeulen
Status: Merged
Merged at revision: 11599
Proposed branch: lp:~jtv/launchpad/bug-643345
Merge into: lp:launchpad
Diff against target: 262 lines (+108/-24)
4 files modified
lib/lp/code/model/directbranchcommit.py (+21/-4)
lib/lp/code/tests/test_directbranchcommit.py (+71/-12)
lib/lp/translations/scripts/tests/test_translations_to_branch.py (+13/-5)
lib/lp/translations/scripts/translations_to_branch.py (+3/-3)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-643345
Reviewer Review Type Date Requested Status
Tim Penhey (community) production-change Approve
Robert Collins production-change Pending
Launchpad code reviewers code Pending
Review via email: mp+36258@code.launchpad.net

Commit message

Fixes translations-to-branch exports and DirectBranchCommit.

Description of the change

= 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

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

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
Revision history for this message
Michael Nelson (michael.nelson) wrote :
Download full text (3.7 KiB)

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 <...

Read more...

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

> 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.

Revision history for this message
Tim Penhey (thumper) :
review: Approve
Revision history for this message
Tim Penhey (thumper) :
review: Approve (production-change)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the1# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""Commit files straight to bzr branch."""4"""Commit files straight to bzr branch."""
@@ -21,6 +21,7 @@
21 TransformPreview,21 TransformPreview,
22 )22 )
2323
24from canonical.config import config
24from canonical.launchpad.interfaces import IMasterObject25from canonical.launchpad.interfaces import IMasterObject
25from lp.codehosting.bzrutils import get_stacked_on_url26from lp.codehosting.bzrutils import get_stacked_on_url
26from lp.services.mail.sendmail import format_address_for_person27from lp.services.mail.sendmail import format_address_for_person
@@ -56,7 +57,7 @@
56 commit_builder = None57 commit_builder = None
5758
58 def __init__(self, db_branch, committer=None, no_race_check=False,59 def __init__(self, db_branch, committer=None, no_race_check=False,
59 merge_parents=None):60 merge_parents=None, committer_id=None):
60 """Create context for direct commit to branch.61 """Create context for direct commit to branch.
6162
62 Before constructing a `DirectBranchCommit`, set up a server that63 Before constructing a `DirectBranchCommit`, set up a server that
@@ -74,9 +75,13 @@
74 `DirectBranchCommit`.75 `DirectBranchCommit`.
7576
76 :param db_branch: a Launchpad `Branch` object.77 :param db_branch: a Launchpad `Branch` object.
77 :param committer: the `Person` writing to the branch.78 :param committer: the `Person` writing to the branch. Defaults to
79 the branch owner.
78 :param no_race_check: don't check for other commits before committing80 :param no_race_check: don't check for other commits before committing
79 our changes, for use in tests.81 our changes, for use in tests.
82 :param committer_id: Optional identification (typically with email
83 address) of the person doing the commit, for use in bzr. If not
84 given, the `committer`'s email address will be used instead.
80 """85 """
81 self.db_branch = db_branch86 self.db_branch = db_branch
8287
@@ -85,6 +90,7 @@
85 if committer is None:90 if committer is None:
86 committer = db_branch.owner91 committer = db_branch.owner
87 self.committer = committer92 self.committer = committer
93 self.committer_id = committer_id
8894
89 self.no_race_check = no_race_check95 self.no_race_check = no_race_check
9096
@@ -176,6 +182,17 @@
176 raise ConcurrentUpdateError(182 raise ConcurrentUpdateError(
177 "Branch has been changed. Not committing.")183 "Branch has been changed. Not committing.")
178184
185 def getBzrCommitterID(self):
186 """Find the committer id to pass to bzr."""
187 if self.committer_id is not None:
188 return self.committer_id
189 elif self.committer.preferredemail is not None:
190 return format_address_for_person(self.committer)
191 else:
192 return '"%s" <%s>' % (
193 self.committer.displayname,
194 config.canonical.noreply_from_address)
195
179 def commit(self, commit_message, txn=None):196 def commit(self, commit_message, txn=None):
180 """Commit to branch.197 """Commit to branch.
181198
@@ -197,7 +214,7 @@
197 if rev_id == NULL_REVISION:214 if rev_id == NULL_REVISION:
198 if list(self.transform_preview.iter_changes()) == []:215 if list(self.transform_preview.iter_changes()) == []:
199 return216 return
200 committer_id = format_address_for_person(self.committer)217 committer_id = self.getBzrCommitterID()
201 # XXX: AaronBentley 2010-08-06 bug=614404: a bzr username is218 # XXX: AaronBentley 2010-08-06 bug=614404: a bzr username is
202 # required to generate the revision-id.219 # required to generate the revision-id.
203 with override_environ(BZR_EMAIL=committer_id):220 with override_environ(BZR_EMAIL=committer_id):
204221
=== 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 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the1# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""Tests for `DirectBranchCommit`."""4"""Tests for `DirectBranchCommit`."""
55
6__metaclass__ = type6__metaclass__ = type
77
8from unittest import TestLoader8from canonical.testing.layers import (
99 DatabaseFunctionalLayer,
10from canonical.testing.layers import ZopelessDatabaseLayer10 ZopelessDatabaseLayer,
11 )
11from lp.code.model.directbranchcommit import (12from lp.code.model.directbranchcommit import (
12 ConcurrentUpdateError,13 ConcurrentUpdateError,
13 DirectBranchCommit,14 DirectBranchCommit,
@@ -16,9 +17,10 @@
16 map_branch_contents,17 map_branch_contents,
17 TestCaseWithFactory,18 TestCaseWithFactory,
18 )19 )
1920from lp.testing.fakemethod import FakeMethod
2021
21class DirectBranchCommitTestCase(TestCaseWithFactory):22
23class DirectBranchCommitTestCase:
22 """Base class for `DirectBranchCommit` tests."""24 """Base class for `DirectBranchCommit` tests."""
23 db_branch = None25 db_branch = None
24 committer = None26 committer = None
@@ -54,11 +56,16 @@
54 return map_branch_contents(self.committer.db_branch.getBzrBranch())56 return map_branch_contents(self.committer.db_branch.getBzrBranch())
5557
5658
57class TestDirectBranchCommit(DirectBranchCommitTestCase):59class TestDirectBranchCommit(DirectBranchCommitTestCase, TestCaseWithFactory):
58 """Test `DirectBranchCommit`."""60 """Test `DirectBranchCommit`."""
5961
60 layer = ZopelessDatabaseLayer62 layer = ZopelessDatabaseLayer
6163
64 def test_defaults_to_branch_owner(self):
65 # If no committer is given, DirectBranchCommits defaults to
66 # attributing changes to the branch owner.
67 self.assertEqual(self.db_branch.owner, self.committer.committer)
68
62 def test_DirectBranchCommit_empty_initial_commit_noop(self):69 def test_DirectBranchCommit_empty_initial_commit_noop(self):
63 # An empty initial commit to a branch is a no-op.70 # An empty initial commit to a branch is a no-op.
64 self.assertEqual('null:', self.tree.branch.last_revision())71 self.assertEqual('null:', self.tree.branch.last_revision())
@@ -204,8 +211,22 @@
204 revid = self.committer.commit('')211 revid = self.committer.commit('')
205 self.assertEqual(revid, self.db_branch.last_mirrored_id)212 self.assertEqual(revid, self.db_branch.last_mirrored_id)
206213
207214 def test_commit_uses_getBzrCommitterID(self):
208class TestDirectBranchCommit_getDir(DirectBranchCommitTestCase):215 # commit() passes self.getBzrCommitterID() to bzr as the
216 # committer.
217 bzr_id = self.factory.getUniqueString()
218 self.committer.getBzrCommitterID = FakeMethod(result=bzr_id)
219 fake_commit = FakeMethod()
220 self.committer.transform_preview.commit = fake_commit
221
222 self.committer.writeFile('x', 'y')
223 self.committer.commit('')
224
225 commit_args, commit_kwargs = fake_commit.calls[-1]
226 self.assertEqual(bzr_id, commit_kwargs['committer'])
227
228
229class TestGetDir(DirectBranchCommitTestCase, TestCaseWithFactory):
209 """Test `DirectBranchCommit._getDir`."""230 """Test `DirectBranchCommit._getDir`."""
210231
211 layer = ZopelessDatabaseLayer232 layer = ZopelessDatabaseLayer
@@ -259,5 +280,43 @@
259 self.assertEqual(dir_id, self.committer._getDir('foo/bar'))280 self.assertEqual(dir_id, self.committer._getDir('foo/bar'))
260281
261282
262def test_suite():283class TestGetBzrCommitterID(TestCaseWithFactory):
263 return TestLoader().loadTestsFromName(__name__)284 """Test `DirectBranchCommitter.getBzrCommitterID`."""
285
286 layer = DatabaseFunctionalLayer
287
288 def setUp(self):
289 super(TestGetBzrCommitterID, self).setUp()
290 self.useBzrBranches(direct_database=True)
291
292 def _makeBranch(self, **kwargs):
293 """Create a branch in the database and in bzr."""
294 db_branch, tree = self.create_branch_and_tree(**kwargs)
295 return db_branch
296
297 def test_uses_committer_id(self):
298 # getBzrCommitterID uses the committer string if provided.
299 bzr_id = self.factory.getUniqueString()
300 committer = DirectBranchCommit(
301 self._makeBranch(), committer_id=bzr_id)
302 self.addCleanup(committer.unlock)
303 self.assertEqual(bzr_id, committer.getBzrCommitterID())
304
305 def test_uses_committer_email(self):
306 # getBzrCommitterID returns the committing person's email address
307 # if available (and if no committer string is given).
308 branch = self._makeBranch()
309 committer = DirectBranchCommit(branch)
310 self.addCleanup(committer.unlock)
311 self.assertIn(
312 branch.owner.preferredemail.email, committer.getBzrCommitterID())
313
314 def test_falls_back_to_noreply(self):
315 # If all else fails, getBzrCommitterID uses the noreply
316 # address.
317 team = self.factory.makeTeam()
318 self.assertIs(None, team.preferredemail)
319 branch = self._makeBranch(owner=team)
320 committer = DirectBranchCommit(branch)
321 self.addCleanup(committer.unlock)
322 self.assertIn('noreply', committer.getBzrCommitterID())
264323
=== 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 @@
55
6import re6import re
7from textwrap import dedent7from textwrap import dedent
8import unittest
98
10from bzrlib.errors import NotBranchError9from bzrlib.errors import NotBranchError
11import transaction10import transaction
@@ -266,6 +265,19 @@
266 # fail either.265 # fail either.
267 transaction.commit()266 transaction.commit()
268267
268 def test_sets_bzr_id(self):
269 # The script commits to the branch under a user id that mentions
270 # the automatic translations exports as well as the Launchpad
271 # name of the branch owner.
272 self.useBzrBranches(direct_database=False)
273 exporter = ExportTranslationsToBranch(test_args=[])
274 branch, tree = self.create_branch_and_tree()
275 committer = exporter._makeDirectBranchCommit(branch)
276 committer.unlock()
277 self.assertEqual(
278 "Launchpad Translations on behalf of %s" % branch.owner.name,
279 committer.getBzrCommitterID())
280
269281
270class TestExportToStackedBranch(TestCaseWithFactory):282class TestExportToStackedBranch(TestCaseWithFactory):
271 """Test workaround for bzr bug 375013."""283 """Test workaround for bzr bug 375013."""
@@ -306,7 +318,3 @@
306 committer.commit("x!")318 committer.commit("x!")
307 finally:319 finally:
308 committer.unlock()320 committer.unlock()
309
310
311def test_suite():
312 return unittest.TestLoader().loadTestsFromName(__name__)
313321
=== 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 @@
95 def _makeDirectBranchCommit(self, db_branch):95 def _makeDirectBranchCommit(self, db_branch):
96 """Create a `DirectBranchCommit`.96 """Create a `DirectBranchCommit`.
9797
98 This factory is a mock-injection point for tests.
99
100 :param db_branch: A `Branch` object as defined in Launchpad.98 :param db_branch: A `Branch` object as defined in Launchpad.
101 :return: A `DirectBranchCommit` for `db_branch`.99 :return: A `DirectBranchCommit` for `db_branch`.
102 """100 """
103 return DirectBranchCommit(db_branch)101 committer_id = 'Launchpad Translations on behalf of %s' % (
102 db_branch.owner.name)
103 return DirectBranchCommit(db_branch, committer_id=committer_id)
104104
105 def _prepareBranchCommit(self, db_branch):105 def _prepareBranchCommit(self, db_branch):
106 """Prepare branch for use with `DirectBranchCommit`.106 """Prepare branch for use with `DirectBranchCommit`.