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
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2009-11-06 01:16:21 +0000
+++ database/schema/security.cfg 2009-11-27 14:15:31 +0000
@@ -453,29 +453,12 @@
453[translations_import_queue_gardener]453[translations_import_queue_gardener]
454# Translations import queue management454# Translations import queue management
455type=user455type=user
456groups=script456groups=script,translations_approval
457public.customlanguagecode = SELECT
458public.distribution = SELECT
459public.distroseries = SELECT
460public.karma = SELECT, INSERT, UPDATE457public.karma = SELECT, INSERT, UPDATE
461public.karmaaction = SELECT458public.karmaaction = SELECT
462public.language = SELECT
463public.person = SELECT
464public.pofile = SELECT, INSERT, UPDATE
465public.pomsgid = SELECT
466public.potemplate = SELECT, INSERT, UPDATE
467public.potmsgset = SELECT
468public.potranslation = SELECT, INSERT459public.potranslation = SELECT, INSERT
469public.product = SELECT
470public.productseries = SELECT
471public.sourcepackagename = SELECT
472public.teamparticipation = SELECT
473public.translationgroup = SELECT
474public.translationimportqueueentry = SELECT, DELETE, UPDATE460public.translationimportqueueentry = SELECT, DELETE, UPDATE
475public.translationrelicensingagreement = SELECT
476public.translationtemplateitem = SELECT
477public.translationmessage = SELECT, INSERT, UPDATE461public.translationmessage = SELECT, INSERT, UPDATE
478public.translator = SELECT
479public.validpersoncache = SELECT462public.validpersoncache = SELECT
480463
481[poexport]464[poexport]
@@ -1369,13 +1352,32 @@
1369public.validpersoncache = SELECT1352public.validpersoncache = SELECT
1370public.translator = SELECT1353public.translator = SELECT
13711354
1355# Any script that approves translation uploads.
1356[translations_approval]
1357type=group
1358public.customlanguagecode = SELECT
1359public.distribution = SELECT
1360public.distroseries = SELECT
1361public.language = SELECT
1362public.person = SELECT
1363public.pofile = SELECT, INSERT, UPDATE
1364public.pomsgid = SELECT
1365public.potemplate = SELECT, INSERT, UPDATE
1366public.potmsgset = SELECT
1367public.product = SELECT
1368public.productseries = SELECT
1369public.project = SELECT
1370public.sourcepackagename = SELECT
1371public.teamparticipation = SELECT
1372public.translationgroup = SELECT
1373public.translationimportqueueentry = SELECT, UPDATE
1374public.translationrelicensingagreement = SELECT
1375public.translationtemplateitem = SELECT
1376public.translator = SELECT
1377
1372[translationsbranchscanner]1378[translationsbranchscanner]
1373type=user1379type=user
1374groups=branchscanner1380groups=branchscanner,translations_approval
1375public.language = SELECT
1376public.translationrelicensingagreement = SELECT
1377public.translationgroup = SELECT
1378public.translator = SELECT
13791381
1380[translationstobranch]1382[translationstobranch]
1381type=user1383type=user
13821384
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2009-11-23 19:56:19 +0000
+++ lib/lp/testing/factory.py 2009-11-27 14:15:31 +0000
@@ -1587,7 +1587,6 @@
1587 if potemplate is None:1587 if potemplate is None:
1588 potemplate = self.makePOTemplate(owner=owner)1588 potemplate = self.makePOTemplate(owner=owner)
1589 return potemplate.newPOFile(language_code, variant,1589 return potemplate.newPOFile(language_code, variant,
1590 requester=potemplate.owner,
1591 create_sharing=create_sharing)1590 create_sharing=create_sharing)
15921591
1593 def makePOTMsgSet(self, potemplate, singular=None, plural=None,1592 def makePOTMsgSet(self, potemplate, singular=None, plural=None,
15941593
=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py 2009-09-19 21:22:13 +0000
+++ lib/lp/translations/browser/potemplate.py 2009-11-27 14:15:31 +0000
@@ -89,11 +89,12 @@
89 return self.context.getDummyPOFile(name, requester=user)89 return self.context.getDummyPOFile(name, requester=user)
90 else:90 else:
91 # It's a POST.91 # It's a POST.
92 # XXX CarlosPerelloMarin 2006-04-20: We should check the kind of92 # XXX CarlosPerelloMarin 2006-04-20 bug=40275: We should
93 # POST we got, a Log out action will be also a POST and we should93 # check the kind of POST we got. A logout will also be a
94 # not create an IPOFile in that case. See bug #40275 for more94 # POST and we should not create a POFile in that case.
95 # information.95 pofile = self.context.newPOFile(name)
96 return self.context.newPOFile(name, requester=user)96 pofile.setOwnerIfPrivileged(user)
97 return pofile
9798
9899
99class POTemplateFacets(StandardLaunchpadFacets):100class POTemplateFacets(StandardLaunchpadFacets):
100101
=== modified file 'lib/lp/translations/interfaces/pofile.py'
--- lib/lp/translations/interfaces/pofile.py 2009-10-27 12:42:22 +0000
+++ lib/lp/translations/interfaces/pofile.py 2009-11-27 14:15:31 +0000
@@ -214,6 +214,9 @@
214 def canEditTranslations(person):214 def canEditTranslations(person):
215 """Whether the given person is able to add/edit translations."""215 """Whether the given person is able to add/edit translations."""
216216
217 def setOwnerIfPrivileged(person):
218 """Set `owner` to `person`, provided `person` has edit rights."""
219
217 def canAddSuggestions(person):220 def canAddSuggestions(person):
218 """Whether the given person is able to add new suggestions."""221 """Whether the given person is able to add new suggestions."""
219222
220223
=== modified file 'lib/lp/translations/interfaces/potemplate.py'
--- lib/lp/translations/interfaces/potemplate.py 2009-10-30 10:09:17 +0000
+++ lib/lp/translations/interfaces/potemplate.py 2009-11-27 14:15:31 +0000
@@ -432,8 +432,7 @@
432 def expireAllMessages():432 def expireAllMessages():
433 """Mark all of our message sets as not current (sequence=0)"""433 """Mark all of our message sets as not current (sequence=0)"""
434434
435 def newPOFile(language_code, variant=None,435 def newPOFile(language_code, variant=None, create_sharing=True):
436 requester=None, create_sharing=True):
437 """Return a new `IPOFile` for the given language.436 """Return a new `IPOFile` for the given language.
438437
439 Raise LanguageNotFound if the language does not exist in the438 Raise LanguageNotFound if the language does not exist in the
440439
=== modified file 'lib/lp/translations/model/pofile.py'
--- lib/lp/translations/model/pofile.py 2009-11-17 09:50:33 +0000
+++ lib/lp/translations/model/pofile.py 2009-11-27 14:15:31 +0000
@@ -244,6 +244,19 @@
244 forms = 2244 forms = 2
245 return forms245 return forms
246246
247 def canEditTranslations(self, person):
248 """See `IPOFile`."""
249 return _can_edit_translations(self, person)
250
251 def canAddSuggestions(self, person):
252 """See `IPOFile`."""
253 return _can_add_suggestions(self, person)
254
255 def setOwnerIfPrivileged(self, person):
256 """See `IPOFile`."""
257 if self.canEditTranslations(person):
258 self.owner = person
259
247 def getHeader(self):260 def getHeader(self):
248 """See `IPOFile`."""261 """See `IPOFile`."""
249 translation_importer = getUtility(ITranslationImporter)262 translation_importer = getUtility(ITranslationImporter)
@@ -586,14 +599,6 @@
586 "Calling prepareTranslationCredits on a message with "599 "Calling prepareTranslationCredits on a message with "
587 "unknown credits type '%s'." % credits_type.title)600 "unknown credits type '%s'." % credits_type.title)
588601
589 def canEditTranslations(self, person):
590 """See `IPOFile`."""
591 return _can_edit_translations(self, person)
592
593 def canAddSuggestions(self, person):
594 """See `IPOFile`."""
595 return _can_add_suggestions(self, person)
596
597 def translated(self):602 def translated(self):
598 """See `IPOFile`."""603 """See `IPOFile`."""
599 raise NotImplementedError604 raise NotImplementedError
@@ -1365,14 +1370,6 @@
1365 """See `IPOFile`."""1370 """See `IPOFile`."""
1366 return self.potemplate.translationpermission1371 return self.potemplate.translationpermission
13671372
1368 def canEditTranslations(self, person):
1369 """See `IPOFile`."""
1370 return _can_edit_translations(self, person)
1371
1372 def canAddSuggestions(self, person):
1373 """See `IPOFile`."""
1374 return _can_add_suggestions(self, person)
1375
1376 def emptySelectResults(self):1373 def emptySelectResults(self):
1377 return POFile.select("1=2")1374 return POFile.select("1=2")
13781375
13791376
=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py 2009-11-17 09:50:33 +0000
+++ lib/lp/translations/model/potemplate.py 2009-11-27 14:15:31 +0000
@@ -734,8 +734,7 @@
734 template._cached_pofiles_by_language[language_code,734 template._cached_pofiles_by_language[language_code,
735 variant] = pofile735 variant] = pofile
736736
737 def newPOFile(self, language_code, variant=None, requester=None,737 def newPOFile(self, language_code, variant=None, create_sharing=True):
738 create_sharing=True):
739 """See `IPOTemplate`."""738 """See `IPOTemplate`."""
740 # Make sure we don't already have a PO file for this language.739 # Make sure we don't already have a PO file for this language.
741 existingpo = self.getPOFileByLang(language_code, variant)740 existingpo = self.getPOFileByLang(language_code, variant)
@@ -763,13 +762,8 @@
763 else:762 else:
764 data['origin'] = self.sourcepackagename.name763 data['origin'] = self.sourcepackagename.name
765764
766 # The default POFile owner is the Rosetta Experts team unless the765 # The default POFile owner is the Rosetta Experts team.
767 # requester has rights to write into that file.766 owner = getUtility(ILaunchpadCelebrities).rosetta_experts
768 dummy_pofile = self.getDummyPOFile(language.code, variant)
769 if dummy_pofile.canEditTranslations(requester):
770 owner = requester
771 else:
772 owner = getUtility(ILaunchpadCelebrities).rosetta_experts
773767
774 path = self._composePOFilePath(language, variant)768 path = self._composePOFilePath(language, variant)
775769
@@ -1117,8 +1111,8 @@
1117 if shared_template is template:1111 if shared_template is template:
1118 continue1112 continue
1119 for pofile in shared_template.pofiles:1113 for pofile in shared_template.pofiles:
1120 template.newPOFile(pofile.language.code,1114 template.newPOFile(
1121 pofile.variant, pofile.owner, False)1115 pofile.language.code, pofile.variant, False)
1122 # Do not continue, else it would trigger an existingpo assertion.1116 # Do not continue, else it would trigger an existingpo assertion.
1123 return1117 return
11241118
11251119
=== modified file 'lib/lp/translations/model/translationimportqueue.py'
--- lib/lp/translations/model/translationimportqueue.py 2009-11-19 07:45:09 +0000
+++ lib/lp/translations/model/translationimportqueue.py 2009-11-27 14:15:31 +0000
@@ -444,8 +444,9 @@
444 # Get or create an IPOFile based on the info we guess.444 # Get or create an IPOFile based on the info we guess.
445 pofile = potemplate.getPOFileByLang(language.code, variant=variant)445 pofile = potemplate.getPOFileByLang(language.code, variant=variant)
446 if pofile is None:446 if pofile is None:
447 pofile = potemplate.newPOFile(447 pofile = potemplate.newPOFile(language.code, variant=variant)
448 language.code, variant=variant, requester=self.importer)448 if pofile.canEditTranslations(self.importer):
449 pofile.owner = self.importer
449450
450 if self.is_published:451 if self.is_published:
451 # This entry comes from upstream, which means that the path we got452 # This entry comes from upstream, which means that the path we got
452453
=== modified file 'lib/lp/translations/scripts/tests/test_message_sharing_migration.py'
--- lib/lp/translations/scripts/tests/test_message_sharing_migration.py 2009-10-28 13:08:05 +0000
+++ lib/lp/translations/scripts/tests/test_message_sharing_migration.py 2009-11-27 14:15:31 +0000
@@ -514,12 +514,14 @@
514 poftset = getUtility(IPOFileTranslatorSet)514 poftset = getUtility(IPOFileTranslatorSet)
515515
516 translator = self.trunk_template.owner516 translator = self.trunk_template.owner
517 self.trunk_pofile.owner = translator
518 self.stable_pofile.owner = translator
517519
518 contented_potmsgset = self.factory.makePOTMsgSet(520 contented_potmsgset = self.factory.makePOTMsgSet(
519 self.trunk_template, singular='snut', sequence=2)521 self.trunk_template, singular='snut', sequence=2)
520 contented_message = self._makeTranslationMessage(522 contented_message = self._makeTranslationMessage(
521 self.trunk_pofile, contented_potmsgset, 'druf', False)523 self.trunk_pofile, contented_potmsgset, 'druf', False)
522 self.assertEqual(contented_message.submitter, translator)524 self.assertEqual(translator, contented_message.submitter)
523 poft = poftset.getForPersonPOFile(translator, self.trunk_pofile)525 poft = poftset.getForPersonPOFile(translator, self.trunk_pofile)
524 self.assertEqual(poft.latest_message, contented_message)526 self.assertEqual(poft.latest_message, contented_message)
525527
526528
=== modified file 'lib/lp/translations/tests/test_autoapproval.py'
--- lib/lp/translations/tests/test_autoapproval.py 2009-11-19 07:45:09 +0000
+++ lib/lp/translations/tests/test_autoapproval.py 2009-11-27 14:15:31 +0000
@@ -23,11 +23,10 @@
23from lp.registry.model.sourcepackagename import (23from lp.registry.model.sourcepackagename import (
24 SourcePackageName,24 SourcePackageName,
25 SourcePackageNameSet)25 SourcePackageNameSet)
26from lp.services.worlddata.model.language import Language26from lp.services.worlddata.model.language import Language, LanguageSet
27from lp.translations.model.customlanguagecode import CustomLanguageCode27from lp.translations.model.customlanguagecode import CustomLanguageCode
28from lp.translations.model.potemplate import (28from lp.translations.model.pofile import POFile
29 POTemplateSet,29from lp.translations.model.potemplate import POTemplateSet, POTemplateSubset
30 POTemplateSubset)
31from lp.translations.model.translationimportqueue import (30from lp.translations.model.translationimportqueue import (
32 TranslationImportQueue, TranslationImportQueueEntry)31 TranslationImportQueue, TranslationImportQueueEntry)
33from lp.translations.interfaces.customlanguagecode import ICustomLanguageCode32from lp.translations.interfaces.customlanguagecode import ICustomLanguageCode
@@ -152,8 +151,7 @@
152151
153 def _makePOFile(self, language_code):152 def _makePOFile(self, language_code):
154 """Create a translation file."""153 """Create a translation file."""
155 file = self.template.newPOFile(154 file = self.template.newPOFile(language_code)
156 language_code, requester=self.product.owner)
157 file.syncUpdate()155 file.syncUpdate()
158 return file156 return file
159157
@@ -829,5 +827,62 @@
829 self.assertFalse(self._exists(entry_id))827 self.assertFalse(self._exists(entry_id))
830828
831829
830class TestAutoApprovalNewPOFile(TestCaseWithFactory):
831 """Test creation of new `POFile`s in approval."""
832
833 layer = LaunchpadZopelessLayer
834
835 def setUp(self):
836 super(TestAutoApprovalNewPOFile, self).setUp()
837 self.product = self.factory.makeProduct()
838 self.queue = TranslationImportQueue()
839 self.language = LanguageSet().getLanguageByCode('nl')
840
841 def _makeTemplate(self, series):
842 """Create a template."""
843 return POTemplateSubset(productseries=series).new(
844 'test', 'test', 'test.pot', self.product.owner)
845
846 def _makeQueueEntry(self, series):
847 """Create translation import queue entry."""
848 return self.queue.addOrUpdateEntry(
849 "%s.po" % self.language.code, 'contents', True,
850 self.product.owner, productseries=series)
851
852 def test_getGuessedPOFile_creates_POFile(self):
853 # Auto-approval may involve creating POFiles. The queue
854 # gardener has permissions to do this. The POFile's owner is
855 # the rosetta_experts team.
856 trunk = self.product.getSeries('trunk')
857 template = self._makeTemplate(trunk)
858 entry = self._makeQueueEntry(trunk)
859 rosetta_experts = getUtility(ILaunchpadCelebrities).rosetta_experts
860
861 become_the_gardener(self.layer)
862
863 pofile = entry.getGuessedPOFile()
864
865 self.assertIsInstance(pofile, POFile)
866 self.assertNotEqual(rosetta_experts, pofile.owner)
867
868 def test_getGuessedPOFile_creates_POFile_with_credits(self):
869 # When the approver creates a POFile for a template that
870 # has a translation credits message, it also includes a
871 # "translation" for the credits message.
872 trunk = self.product.getSeries('trunk')
873 template = self._makeTemplate(trunk)
874 credits = self.factory.makePOTMsgSet(
875 template, singular='translation-credits', sequence=1)
876
877 entry = self._makeQueueEntry(trunk)
878
879 become_the_gardener(self.layer)
880
881 pofile = entry.getGuessedPOFile()
882
883 credits.getCurrentTranslationMessage(template, self.language)
884 self.assertNotEqual(None, credits)
885
886
832def test_suite():887def test_suite():
833 return unittest.TestLoader().loadTestsFromName(__name__)888 return unittest.TestLoader().loadTestsFromName(__name__)