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
1=== modified file 'lib/lp/code/model/directbranchcommit.py'
2--- lib/lp/code/model/directbranchcommit.py 2010-09-10 14:44:43 +0000
3+++ lib/lp/code/model/directbranchcommit.py 2010-09-22 07:52:42 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2009 Canonical Ltd. This software is licensed under the
6+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 """Commit files straight to bzr branch."""
10@@ -21,6 +21,7 @@
11 TransformPreview,
12 )
13
14+from canonical.config import config
15 from canonical.launchpad.interfaces import IMasterObject
16 from lp.codehosting.bzrutils import get_stacked_on_url
17 from lp.services.mail.sendmail import format_address_for_person
18@@ -56,7 +57,7 @@
19 commit_builder = None
20
21 def __init__(self, db_branch, committer=None, no_race_check=False,
22- merge_parents=None):
23+ merge_parents=None, committer_id=None):
24 """Create context for direct commit to branch.
25
26 Before constructing a `DirectBranchCommit`, set up a server that
27@@ -74,9 +75,13 @@
28 `DirectBranchCommit`.
29
30 :param db_branch: a Launchpad `Branch` object.
31- :param committer: the `Person` writing to the branch.
32+ :param committer: the `Person` writing to the branch. Defaults to
33+ the branch owner.
34 :param no_race_check: don't check for other commits before committing
35 our changes, for use in tests.
36+ :param committer_id: Optional identification (typically with email
37+ address) of the person doing the commit, for use in bzr. If not
38+ given, the `committer`'s email address will be used instead.
39 """
40 self.db_branch = db_branch
41
42@@ -85,6 +90,7 @@
43 if committer is None:
44 committer = db_branch.owner
45 self.committer = committer
46+ self.committer_id = committer_id
47
48 self.no_race_check = no_race_check
49
50@@ -176,6 +182,17 @@
51 raise ConcurrentUpdateError(
52 "Branch has been changed. Not committing.")
53
54+ def getBzrCommitterID(self):
55+ """Find the committer id to pass to bzr."""
56+ if self.committer_id is not None:
57+ return self.committer_id
58+ elif self.committer.preferredemail is not None:
59+ return format_address_for_person(self.committer)
60+ else:
61+ return '"%s" <%s>' % (
62+ self.committer.displayname,
63+ config.canonical.noreply_from_address)
64+
65 def commit(self, commit_message, txn=None):
66 """Commit to branch.
67
68@@ -197,7 +214,7 @@
69 if rev_id == NULL_REVISION:
70 if list(self.transform_preview.iter_changes()) == []:
71 return
72- committer_id = format_address_for_person(self.committer)
73+ committer_id = self.getBzrCommitterID()
74 # XXX: AaronBentley 2010-08-06 bug=614404: a bzr username is
75 # required to generate the revision-id.
76 with override_environ(BZR_EMAIL=committer_id):
77
78=== modified file 'lib/lp/code/tests/test_directbranchcommit.py'
79--- lib/lp/code/tests/test_directbranchcommit.py 2010-09-10 14:44:43 +0000
80+++ lib/lp/code/tests/test_directbranchcommit.py 2010-09-22 07:52:42 +0000
81@@ -1,13 +1,14 @@
82-# Copyright 2009 Canonical Ltd. This software is licensed under the
83+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
84 # GNU Affero General Public License version 3 (see the file LICENSE).
85
86 """Tests for `DirectBranchCommit`."""
87
88 __metaclass__ = type
89
90-from unittest import TestLoader
91-
92-from canonical.testing.layers import ZopelessDatabaseLayer
93+from canonical.testing.layers import (
94+ DatabaseFunctionalLayer,
95+ ZopelessDatabaseLayer,
96+ )
97 from lp.code.model.directbranchcommit import (
98 ConcurrentUpdateError,
99 DirectBranchCommit,
100@@ -16,9 +17,10 @@
101 map_branch_contents,
102 TestCaseWithFactory,
103 )
104-
105-
106-class DirectBranchCommitTestCase(TestCaseWithFactory):
107+from lp.testing.fakemethod import FakeMethod
108+
109+
110+class DirectBranchCommitTestCase:
111 """Base class for `DirectBranchCommit` tests."""
112 db_branch = None
113 committer = None
114@@ -54,11 +56,16 @@
115 return map_branch_contents(self.committer.db_branch.getBzrBranch())
116
117
118-class TestDirectBranchCommit(DirectBranchCommitTestCase):
119+class TestDirectBranchCommit(DirectBranchCommitTestCase, TestCaseWithFactory):
120 """Test `DirectBranchCommit`."""
121
122 layer = ZopelessDatabaseLayer
123
124+ def test_defaults_to_branch_owner(self):
125+ # If no committer is given, DirectBranchCommits defaults to
126+ # attributing changes to the branch owner.
127+ self.assertEqual(self.db_branch.owner, self.committer.committer)
128+
129 def test_DirectBranchCommit_empty_initial_commit_noop(self):
130 # An empty initial commit to a branch is a no-op.
131 self.assertEqual('null:', self.tree.branch.last_revision())
132@@ -204,8 +211,22 @@
133 revid = self.committer.commit('')
134 self.assertEqual(revid, self.db_branch.last_mirrored_id)
135
136-
137-class TestDirectBranchCommit_getDir(DirectBranchCommitTestCase):
138+ def test_commit_uses_getBzrCommitterID(self):
139+ # commit() passes self.getBzrCommitterID() to bzr as the
140+ # committer.
141+ bzr_id = self.factory.getUniqueString()
142+ self.committer.getBzrCommitterID = FakeMethod(result=bzr_id)
143+ fake_commit = FakeMethod()
144+ self.committer.transform_preview.commit = fake_commit
145+
146+ self.committer.writeFile('x', 'y')
147+ self.committer.commit('')
148+
149+ commit_args, commit_kwargs = fake_commit.calls[-1]
150+ self.assertEqual(bzr_id, commit_kwargs['committer'])
151+
152+
153+class TestGetDir(DirectBranchCommitTestCase, TestCaseWithFactory):
154 """Test `DirectBranchCommit._getDir`."""
155
156 layer = ZopelessDatabaseLayer
157@@ -259,5 +280,43 @@
158 self.assertEqual(dir_id, self.committer._getDir('foo/bar'))
159
160
161-def test_suite():
162- return TestLoader().loadTestsFromName(__name__)
163+class TestGetBzrCommitterID(TestCaseWithFactory):
164+ """Test `DirectBranchCommitter.getBzrCommitterID`."""
165+
166+ layer = DatabaseFunctionalLayer
167+
168+ def setUp(self):
169+ super(TestGetBzrCommitterID, self).setUp()
170+ self.useBzrBranches(direct_database=True)
171+
172+ def _makeBranch(self, **kwargs):
173+ """Create a branch in the database and in bzr."""
174+ db_branch, tree = self.create_branch_and_tree(**kwargs)
175+ return db_branch
176+
177+ def test_uses_committer_id(self):
178+ # getBzrCommitterID uses the committer string if provided.
179+ bzr_id = self.factory.getUniqueString()
180+ committer = DirectBranchCommit(
181+ self._makeBranch(), committer_id=bzr_id)
182+ self.addCleanup(committer.unlock)
183+ self.assertEqual(bzr_id, committer.getBzrCommitterID())
184+
185+ def test_uses_committer_email(self):
186+ # getBzrCommitterID returns the committing person's email address
187+ # if available (and if no committer string is given).
188+ branch = self._makeBranch()
189+ committer = DirectBranchCommit(branch)
190+ self.addCleanup(committer.unlock)
191+ self.assertIn(
192+ branch.owner.preferredemail.email, committer.getBzrCommitterID())
193+
194+ def test_falls_back_to_noreply(self):
195+ # If all else fails, getBzrCommitterID uses the noreply
196+ # address.
197+ team = self.factory.makeTeam()
198+ self.assertIs(None, team.preferredemail)
199+ branch = self._makeBranch(owner=team)
200+ committer = DirectBranchCommit(branch)
201+ self.addCleanup(committer.unlock)
202+ self.assertIn('noreply', committer.getBzrCommitterID())
203
204=== modified file 'lib/lp/translations/scripts/tests/test_translations_to_branch.py'
205--- lib/lp/translations/scripts/tests/test_translations_to_branch.py 2010-08-20 20:31:18 +0000
206+++ lib/lp/translations/scripts/tests/test_translations_to_branch.py 2010-09-22 07:52:42 +0000
207@@ -5,7 +5,6 @@
208
209 import re
210 from textwrap import dedent
211-import unittest
212
213 from bzrlib.errors import NotBranchError
214 import transaction
215@@ -266,6 +265,19 @@
216 # fail either.
217 transaction.commit()
218
219+ def test_sets_bzr_id(self):
220+ # The script commits to the branch under a user id that mentions
221+ # the automatic translations exports as well as the Launchpad
222+ # name of the branch owner.
223+ self.useBzrBranches(direct_database=False)
224+ exporter = ExportTranslationsToBranch(test_args=[])
225+ branch, tree = self.create_branch_and_tree()
226+ committer = exporter._makeDirectBranchCommit(branch)
227+ committer.unlock()
228+ self.assertEqual(
229+ "Launchpad Translations on behalf of %s" % branch.owner.name,
230+ committer.getBzrCommitterID())
231+
232
233 class TestExportToStackedBranch(TestCaseWithFactory):
234 """Test workaround for bzr bug 375013."""
235@@ -306,7 +318,3 @@
236 committer.commit("x!")
237 finally:
238 committer.unlock()
239-
240-
241-def test_suite():
242- return unittest.TestLoader().loadTestsFromName(__name__)
243
244=== modified file 'lib/lp/translations/scripts/translations_to_branch.py'
245--- lib/lp/translations/scripts/translations_to_branch.py 2010-08-31 23:03:45 +0000
246+++ lib/lp/translations/scripts/translations_to_branch.py 2010-09-22 07:52:42 +0000
247@@ -95,12 +95,12 @@
248 def _makeDirectBranchCommit(self, db_branch):
249 """Create a `DirectBranchCommit`.
250
251- This factory is a mock-injection point for tests.
252-
253 :param db_branch: A `Branch` object as defined in Launchpad.
254 :return: A `DirectBranchCommit` for `db_branch`.
255 """
256- return DirectBranchCommit(db_branch)
257+ committer_id = 'Launchpad Translations on behalf of %s' % (
258+ db_branch.owner.name)
259+ return DirectBranchCommit(db_branch, committer_id=committer_id)
260
261 def _prepareBranchCommit(self, db_branch):
262 """Prepare branch for use with `DirectBranchCommit`.