Launchpad itself

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

  1. bug-487447
  2. Merge into devel
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~jtv/launchpad/bug-487447
Merge into: lp:launchpad
Prerequisite: lp:~jtv/launchpad/bug-488218
Diff against target: 357 lines (+120/-67)
10 files modified
database/schema/security.cfg (+25/-23)
lib/lp/testing/factory.py (+0/-1)
lib/lp/translations/browser/potemplate.py (+6/-5)
lib/lp/translations/interfaces/pofile.py (+3/-0)
lib/lp/translations/interfaces/potemplate.py (+1/-2)
lib/lp/translations/model/pofile.py (+13/-16)
lib/lp/translations/model/potemplate.py (+5/-11)
lib/lp/translations/model/translationimportqueue.py (+3/-2)
lib/lp/translations/scripts/tests/test_message_sharing_migration.py (+3/-1)
lib/lp/translations/tests/test_autoapproval.py (+61/-6)
To merge this branch: bzr merge lp:~jtv/launchpad/bug-487447
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Review via email: mp+15315@code.launchpad.net

Commit message

Reorganize database permissions for Translations approval. Factor canEditTranslations out of newPOFile.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

= Bugs 443129, 487447 =

We were getting some database permission errors as the two scripts that approve translations uploads created new POFiles. This has been happening a lot lately, particularly because of a fairly pointless permissions check in initializing the owner of a new POFile. POFile.owner pretty much unused anyway.

This branch does the following:

 * Reorganizes database permissions by creating a single group for "anything that approves translations uploads."

 * Lifts the setting of POFile.owner out of newPOFile and into the caller—along with the check to see whether that person is qualified to be the owner.

 * Gets rid of the parameter that told newPOFile what owner to set (if qualified).

 * Hoists some related IPOFile methods from POFile and DummyPOFile into their common base class.

 * Always makes rosetta_experts the owner of POFiles created by the approver (instead of the uploader).

 * Tests for the database privileges necessary for approving a POFile.

No lint. Main test is test_autoapproval. Q/A procedure on staging:

 - Set up a project for translation, including an import branch.
 - Push a template and a translation to the branch. The template should have a message "translation-credits"
 - See that the template and the translation get approved & imported.
 - Manually upload another translation for the same template.
 - See this get approved & imported as well.

Jeroen

Revision history for this message
Abel Deuring (adeuring) wrote :

Hi Jeroen,

your changes look good (as usual ;) Just one deatil: I think you don't need a permission to access public.project. Or do I miss anything?

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

Thanks! The Project permission is because Projects can have TranslationGroups (as can Products).

Jeroen

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'database/schema/security.cfg'
2--- database/schema/security.cfg 2009-11-06 01:16:21 +0000
3+++ database/schema/security.cfg 2009-11-27 14:15:31 +0000
4@@ -453,29 +453,12 @@
5 [translations_import_queue_gardener]
6 # Translations import queue management
7 type=user
8-groups=script
9-public.customlanguagecode = SELECT
10-public.distribution = SELECT
11-public.distroseries = SELECT
12+groups=script,translations_approval
13 public.karma = SELECT, INSERT, UPDATE
14 public.karmaaction = SELECT
15-public.language = SELECT
16-public.person = SELECT
17-public.pofile = SELECT, INSERT, UPDATE
18-public.pomsgid = SELECT
19-public.potemplate = SELECT, INSERT, UPDATE
20-public.potmsgset = SELECT
21 public.potranslation = SELECT, INSERT
22-public.product = SELECT
23-public.productseries = SELECT
24-public.sourcepackagename = SELECT
25-public.teamparticipation = SELECT
26-public.translationgroup = SELECT
27 public.translationimportqueueentry = SELECT, DELETE, UPDATE
28-public.translationrelicensingagreement = SELECT
29-public.translationtemplateitem = SELECT
30 public.translationmessage = SELECT, INSERT, UPDATE
31-public.translator = SELECT
32 public.validpersoncache = SELECT
33
34 [poexport]
35@@ -1369,13 +1352,32 @@
36 public.validpersoncache = SELECT
37 public.translator = SELECT
38
39+# Any script that approves translation uploads.
40+[translations_approval]
41+type=group
42+public.customlanguagecode = SELECT
43+public.distribution = SELECT
44+public.distroseries = SELECT
45+public.language = SELECT
46+public.person = SELECT
47+public.pofile = SELECT, INSERT, UPDATE
48+public.pomsgid = SELECT
49+public.potemplate = SELECT, INSERT, UPDATE
50+public.potmsgset = SELECT
51+public.product = SELECT
52+public.productseries = SELECT
53+public.project = SELECT
54+public.sourcepackagename = SELECT
55+public.teamparticipation = SELECT
56+public.translationgroup = SELECT
57+public.translationimportqueueentry = SELECT, UPDATE
58+public.translationrelicensingagreement = SELECT
59+public.translationtemplateitem = SELECT
60+public.translator = SELECT
61+
62 [translationsbranchscanner]
63 type=user
64-groups=branchscanner
65-public.language = SELECT
66-public.translationrelicensingagreement = SELECT
67-public.translationgroup = SELECT
68-public.translator = SELECT
69+groups=branchscanner,translations_approval
70
71 [translationstobranch]
72 type=user
73
74=== modified file 'lib/lp/testing/factory.py'
75--- lib/lp/testing/factory.py 2009-11-23 19:56:19 +0000
76+++ lib/lp/testing/factory.py 2009-11-27 14:15:31 +0000
77@@ -1587,7 +1587,6 @@
78 if potemplate is None:
79 potemplate = self.makePOTemplate(owner=owner)
80 return potemplate.newPOFile(language_code, variant,
81- requester=potemplate.owner,
82 create_sharing=create_sharing)
83
84 def makePOTMsgSet(self, potemplate, singular=None, plural=None,
85
86=== modified file 'lib/lp/translations/browser/potemplate.py'
87--- lib/lp/translations/browser/potemplate.py 2009-09-19 21:22:13 +0000
88+++ lib/lp/translations/browser/potemplate.py 2009-11-27 14:15:31 +0000
89@@ -89,11 +89,12 @@
90 return self.context.getDummyPOFile(name, requester=user)
91 else:
92 # It's a POST.
93- # XXX CarlosPerelloMarin 2006-04-20: We should check the kind of
94- # POST we got, a Log out action will be also a POST and we should
95- # not create an IPOFile in that case. See bug #40275 for more
96- # information.
97- return self.context.newPOFile(name, requester=user)
98+ # XXX CarlosPerelloMarin 2006-04-20 bug=40275: We should
99+ # check the kind of POST we got. A logout will also be a
100+ # POST and we should not create a POFile in that case.
101+ pofile = self.context.newPOFile(name)
102+ pofile.setOwnerIfPrivileged(user)
103+ return pofile
104
105
106 class POTemplateFacets(StandardLaunchpadFacets):
107
108=== modified file 'lib/lp/translations/interfaces/pofile.py'
109--- lib/lp/translations/interfaces/pofile.py 2009-10-27 12:42:22 +0000
110+++ lib/lp/translations/interfaces/pofile.py 2009-11-27 14:15:31 +0000
111@@ -214,6 +214,9 @@
112 def canEditTranslations(person):
113 """Whether the given person is able to add/edit translations."""
114
115+ def setOwnerIfPrivileged(person):
116+ """Set `owner` to `person`, provided `person` has edit rights."""
117+
118 def canAddSuggestions(person):
119 """Whether the given person is able to add new suggestions."""
120
121
122=== modified file 'lib/lp/translations/interfaces/potemplate.py'
123--- lib/lp/translations/interfaces/potemplate.py 2009-10-30 10:09:17 +0000
124+++ lib/lp/translations/interfaces/potemplate.py 2009-11-27 14:15:31 +0000
125@@ -432,8 +432,7 @@
126 def expireAllMessages():
127 """Mark all of our message sets as not current (sequence=0)"""
128
129- def newPOFile(language_code, variant=None,
130- requester=None, create_sharing=True):
131+ def newPOFile(language_code, variant=None, create_sharing=True):
132 """Return a new `IPOFile` for the given language.
133
134 Raise LanguageNotFound if the language does not exist in the
135
136=== modified file 'lib/lp/translations/model/pofile.py'
137--- lib/lp/translations/model/pofile.py 2009-11-17 09:50:33 +0000
138+++ lib/lp/translations/model/pofile.py 2009-11-27 14:15:31 +0000
139@@ -244,6 +244,19 @@
140 forms = 2
141 return forms
142
143+ def canEditTranslations(self, person):
144+ """See `IPOFile`."""
145+ return _can_edit_translations(self, person)
146+
147+ def canAddSuggestions(self, person):
148+ """See `IPOFile`."""
149+ return _can_add_suggestions(self, person)
150+
151+ def setOwnerIfPrivileged(self, person):
152+ """See `IPOFile`."""
153+ if self.canEditTranslations(person):
154+ self.owner = person
155+
156 def getHeader(self):
157 """See `IPOFile`."""
158 translation_importer = getUtility(ITranslationImporter)
159@@ -586,14 +599,6 @@
160 "Calling prepareTranslationCredits on a message with "
161 "unknown credits type '%s'." % credits_type.title)
162
163- def canEditTranslations(self, person):
164- """See `IPOFile`."""
165- return _can_edit_translations(self, person)
166-
167- def canAddSuggestions(self, person):
168- """See `IPOFile`."""
169- return _can_add_suggestions(self, person)
170-
171 def translated(self):
172 """See `IPOFile`."""
173 raise NotImplementedError
174@@ -1365,14 +1370,6 @@
175 """See `IPOFile`."""
176 return self.potemplate.translationpermission
177
178- def canEditTranslations(self, person):
179- """See `IPOFile`."""
180- return _can_edit_translations(self, person)
181-
182- def canAddSuggestions(self, person):
183- """See `IPOFile`."""
184- return _can_add_suggestions(self, person)
185-
186 def emptySelectResults(self):
187 return POFile.select("1=2")
188
189
190=== modified file 'lib/lp/translations/model/potemplate.py'
191--- lib/lp/translations/model/potemplate.py 2009-11-17 09:50:33 +0000
192+++ lib/lp/translations/model/potemplate.py 2009-11-27 14:15:31 +0000
193@@ -734,8 +734,7 @@
194 template._cached_pofiles_by_language[language_code,
195 variant] = pofile
196
197- def newPOFile(self, language_code, variant=None, requester=None,
198- create_sharing=True):
199+ def newPOFile(self, language_code, variant=None, create_sharing=True):
200 """See `IPOTemplate`."""
201 # Make sure we don't already have a PO file for this language.
202 existingpo = self.getPOFileByLang(language_code, variant)
203@@ -763,13 +762,8 @@
204 else:
205 data['origin'] = self.sourcepackagename.name
206
207- # The default POFile owner is the Rosetta Experts team unless the
208- # requester has rights to write into that file.
209- dummy_pofile = self.getDummyPOFile(language.code, variant)
210- if dummy_pofile.canEditTranslations(requester):
211- owner = requester
212- else:
213- owner = getUtility(ILaunchpadCelebrities).rosetta_experts
214+ # The default POFile owner is the Rosetta Experts team.
215+ owner = getUtility(ILaunchpadCelebrities).rosetta_experts
216
217 path = self._composePOFilePath(language, variant)
218
219@@ -1117,8 +1111,8 @@
220 if shared_template is template:
221 continue
222 for pofile in shared_template.pofiles:
223- template.newPOFile(pofile.language.code,
224- pofile.variant, pofile.owner, False)
225+ template.newPOFile(
226+ pofile.language.code, pofile.variant, False)
227 # Do not continue, else it would trigger an existingpo assertion.
228 return
229
230
231=== modified file 'lib/lp/translations/model/translationimportqueue.py'
232--- lib/lp/translations/model/translationimportqueue.py 2009-11-19 07:45:09 +0000
233+++ lib/lp/translations/model/translationimportqueue.py 2009-11-27 14:15:31 +0000
234@@ -444,8 +444,9 @@
235 # Get or create an IPOFile based on the info we guess.
236 pofile = potemplate.getPOFileByLang(language.code, variant=variant)
237 if pofile is None:
238- pofile = potemplate.newPOFile(
239- language.code, variant=variant, requester=self.importer)
240+ pofile = potemplate.newPOFile(language.code, variant=variant)
241+ if pofile.canEditTranslations(self.importer):
242+ pofile.owner = self.importer
243
244 if self.is_published:
245 # This entry comes from upstream, which means that the path we got
246
247=== modified file 'lib/lp/translations/scripts/tests/test_message_sharing_migration.py'
248--- lib/lp/translations/scripts/tests/test_message_sharing_migration.py 2009-10-28 13:08:05 +0000
249+++ lib/lp/translations/scripts/tests/test_message_sharing_migration.py 2009-11-27 14:15:31 +0000
250@@ -514,12 +514,14 @@
251 poftset = getUtility(IPOFileTranslatorSet)
252
253 translator = self.trunk_template.owner
254+ self.trunk_pofile.owner = translator
255+ self.stable_pofile.owner = translator
256
257 contented_potmsgset = self.factory.makePOTMsgSet(
258 self.trunk_template, singular='snut', sequence=2)
259 contented_message = self._makeTranslationMessage(
260 self.trunk_pofile, contented_potmsgset, 'druf', False)
261- self.assertEqual(contented_message.submitter, translator)
262+ self.assertEqual(translator, contented_message.submitter)
263 poft = poftset.getForPersonPOFile(translator, self.trunk_pofile)
264 self.assertEqual(poft.latest_message, contented_message)
265
266
267=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
268--- lib/lp/translations/tests/test_autoapproval.py 2009-11-19 07:45:09 +0000
269+++ lib/lp/translations/tests/test_autoapproval.py 2009-11-27 14:15:31 +0000
270@@ -23,11 +23,10 @@
271 from lp.registry.model.sourcepackagename import (
272 SourcePackageName,
273 SourcePackageNameSet)
274-from lp.services.worlddata.model.language import Language
275+from lp.services.worlddata.model.language import Language, LanguageSet
276 from lp.translations.model.customlanguagecode import CustomLanguageCode
277-from lp.translations.model.potemplate import (
278- POTemplateSet,
279- POTemplateSubset)
280+from lp.translations.model.pofile import POFile
281+from lp.translations.model.potemplate import POTemplateSet, POTemplateSubset
282 from lp.translations.model.translationimportqueue import (
283 TranslationImportQueue, TranslationImportQueueEntry)
284 from lp.translations.interfaces.customlanguagecode import ICustomLanguageCode
285@@ -152,8 +151,7 @@
286
287 def _makePOFile(self, language_code):
288 """Create a translation file."""
289- file = self.template.newPOFile(
290- language_code, requester=self.product.owner)
291+ file = self.template.newPOFile(language_code)
292 file.syncUpdate()
293 return file
294
295@@ -829,5 +827,62 @@
296 self.assertFalse(self._exists(entry_id))
297
298
299+class TestAutoApprovalNewPOFile(TestCaseWithFactory):
300+ """Test creation of new `POFile`s in approval."""
301+
302+ layer = LaunchpadZopelessLayer
303+
304+ def setUp(self):
305+ super(TestAutoApprovalNewPOFile, self).setUp()
306+ self.product = self.factory.makeProduct()
307+ self.queue = TranslationImportQueue()
308+ self.language = LanguageSet().getLanguageByCode('nl')
309+
310+ def _makeTemplate(self, series):
311+ """Create a template."""
312+ return POTemplateSubset(productseries=series).new(
313+ 'test', 'test', 'test.pot', self.product.owner)
314+
315+ def _makeQueueEntry(self, series):
316+ """Create translation import queue entry."""
317+ return self.queue.addOrUpdateEntry(
318+ "%s.po" % self.language.code, 'contents', True,
319+ self.product.owner, productseries=series)
320+
321+ def test_getGuessedPOFile_creates_POFile(self):
322+ # Auto-approval may involve creating POFiles. The queue
323+ # gardener has permissions to do this. The POFile's owner is
324+ # the rosetta_experts team.
325+ trunk = self.product.getSeries('trunk')
326+ template = self._makeTemplate(trunk)
327+ entry = self._makeQueueEntry(trunk)
328+ rosetta_experts = getUtility(ILaunchpadCelebrities).rosetta_experts
329+
330+ become_the_gardener(self.layer)
331+
332+ pofile = entry.getGuessedPOFile()
333+
334+ self.assertIsInstance(pofile, POFile)
335+ self.assertNotEqual(rosetta_experts, pofile.owner)
336+
337+ def test_getGuessedPOFile_creates_POFile_with_credits(self):
338+ # When the approver creates a POFile for a template that
339+ # has a translation credits message, it also includes a
340+ # "translation" for the credits message.
341+ trunk = self.product.getSeries('trunk')
342+ template = self._makeTemplate(trunk)
343+ credits = self.factory.makePOTMsgSet(
344+ template, singular='translation-credits', sequence=1)
345+
346+ entry = self._makeQueueEntry(trunk)
347+
348+ become_the_gardener(self.layer)
349+
350+ pofile = entry.getGuessedPOFile()
351+
352+ credits.getCurrentTranslationMessage(template, self.language)
353+ self.assertNotEqual(None, credits)
354+
355+
356 def test_suite():
357 return unittest.TestLoader().loadTestsFromName(__name__)