Hi Henning, This is a nice branch with very good test coverage. I mostly have minor comments, but I do have one question below, so I'm marking this: needs-reply -Edwin >=== added symlink 'lib/canonical/launchpad/icing/yui' >=== target is u'../../../../lazr-js/build/yui' I think this symlink must have been for testing so should be removed. >=== modified file 'lib/lp/translations/model/translationimportqueue.py' >--- lib/lp/translations/model/translationimportqueue.py 2009-10-22 07:32:02 +0000 >+++ lib/lp/translations/model/translationimportqueue.py 2009-11-13 18:41:14 +0000 >@@ -56,11 +56,14 @@ > ITranslationImportQueueEntry, > RosettaImportStatus, > SpecialTranslationImportTargetFilter, >- TranslationImportQueueConflictError) >+ TranslationImportQueueConflictError, >+ UserCannotSetTranslationImportStatus) > from lp.translations.interfaces.potemplate import IPOTemplate > from lp.translations.interfaces.translations import TranslationConstants > from lp.translations.utilities.gettext_po_importer import ( > GettextPOImporter) >+from lp.translations.utilities.permission_helpers import ( >+ is_admin_or_rosetta_expert) > from canonical.librarian.interfaces import ILibrarianClient > > >@@ -271,9 +274,58 @@ > distroseries=self.distroseries, > sourcepackagename=self.sourcepackagename) > >- def setStatus(self, status): >- """See `ITranslationImportQueueEntry`.""" >- self.status = status >+ def isUbuntuAndIsUserTranslationGroupOwner(self, user): >+ """See `ITranslationImportQueueEntry`.""" >+ # As a special case, the Ubuntu translation group owners can >+ # manage Ubuntu uploads. >+ if self.is_targeted_to_ubuntu: >+ group = self.distroseries.distribution.translationgroup >+ if group is not None and user.inTeam(group.owner): >+ return True >+ return False >+ >+ def isUserUploaderOrOwner(self, user): >+ """See `ITranslationImportQueueEntry`.""" >+ if user.inTeam(self.importer): >+ return True >+ if self.productseries is not None: >+ return user.inTeam(self.productseries.product.owner) >+ if self.distroseries is not None: >+ return user.inTeam(self.distroseries.distribution.owner) Is there no situation where the series owner would be different from the product or distro owner and would need access? >+ return False >+ >+ def canSetStatus(self, new_status, user): >+ """See `ITranslationImportQueueEntry`.""" >+ if new_status == self.status: >+ # Leaving status as it is is always allowed. >+ return True >+ if user is None: >+ # Anonymous user cannot do anything. >+ return False >+ can_admin = (is_admin_or_rosetta_expert(user) or >+ self.isUbuntuAndIsUserTranslationGroupOwner(user)) >+ if (new_status == RosettaImportStatus.APPROVED and >+ not (self.import_into is not None and can_admin)): I think it would be clearer without the double negatives. if (new_status == RosettaImportStatus.APPROVED and (self.import_into is None or not can_admin)): > >+ # Only administrators are able to set the APPROVED status, and >+ # that's only possible if we know where to import it >+ # (import_into not None). >+ return False >+ if (new_status == RosettaImportStatus.BLOCKED and not can_admin): Extraneous parens. >+ # Only administrators are able to set an entry to BLOCKED. >+ return False >+ if (new_status in (RosettaImportStatus.FAILED, >+ RosettaImportStatus.IMPORTED) >+ and not is_admin_or_rosetta_expert(user)): >+ # Only scripts set these statuses and they report as a rosetta >+ # expert. >+ return False >+ return (self.isUserUploaderOrOwner(user) or can_admin) >+ >+ def setStatus(self, new_status, user): >+ """See `ITranslationImportQueueEntry`.""" >+ if not self.canSetStatus(new_status, user): >+ raise UserCannotSetTranslationImportStatus() >+ self.status = new_status > self.date_status_changed = UTC_NOW > > def setErrorOutput(self, output): >@@ -473,7 +525,8 @@ > if guessed_language is None: > # Custom language code says to ignore imports with this language > # code. >- self.setStatus(RosettaImportStatus.DELETED) >+ self.setStatus(RosettaImportStatus.DELETED, >+ getUtility(ILaunchpadCelebrities).rosetta_experts) > return None > elif guessed_language == '': > # We don't recognize this as a translation file with a name >@@ -877,7 +930,7 @@ > # We got an update for this entry. If the previous import is > # deleted or failed or was already imported we should retry > # the import now, just in case it can be imported now. >- entry.setStatus(RosettaImportStatus.NEEDS_REVIEW) >+ entry.setStatus(RosettaImportStatus.NEEDS_REVIEW, importer) > > entry.date_status_changed = UTC_NOW > entry.format = format >@@ -1130,7 +1183,8 @@ > > # Already know where it should be imported. The entry is approved > # automatically. >- entry.setStatus(RosettaImportStatus.APPROVED) >+ entry.setStatus(RosettaImportStatus.APPROVED, >+ getUtility(ILaunchpadCelebrities).rosetta_experts) > > if txn is not None: > txn.commit() >@@ -1165,7 +1219,9 @@ > if has_templates and not has_templates_unblocked: > # All templates on the same directory as this entry are > # blocked, so we can block it too. >- entry.setStatus(RosettaImportStatus.BLOCKED) >+ entry.setStatus( >+ RosettaImportStatus.BLOCKED, >+ getUtility(ILaunchpadCelebrities).rosetta_experts) > num_blocked += 1 > if txn is not None: > txn.commit() > >=== added file 'lib/lp/translations/tests/test_translationimportqueue.py' >--- lib/lp/translations/tests/test_translationimportqueue.py 1970-01-01 00:00:00 +0000 >+++ lib/lp/translations/tests/test_translationimportqueue.py 2009-11-13 18:41:14 +0000 >@@ -0,0 +1,125 @@ >+# Copyright 2009 Canonical Ltd. This software is licensed under the >+# GNU Affero General Public License version 3 (see the file LICENSE). >+ >+# pylint: disable-msg=C0102 >+ >+__metaclass__ = type >+ >+import unittest >+ >+from zope.component import getUtility >+ >+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities >+from lp.translations.interfaces.translationimportqueue import ( >+ ITranslationImportQueue, RosettaImportStatus) >+ >+from lp.testing import TestCaseWithFactory >+from canonical.testing import LaunchpadZopelessLayer >+ >+ >+class TestTranslationImpportQueueEntryStatus(TestCaseWithFactory): s/Impport/Import/ >+ """Test handling of the status of a queue entry.""" >+ >+ layer = LaunchpadZopelessLayer >+ >+ def setUp(self): >+ """Set up context to test in.""" >+ super(TestTranslationImpportQueueEntryStatus, self).setUp() >+ >+ self.queue = getUtility(ITranslationImportQueue) >+ self.rosetta_experts = ( >+ getUtility(ILaunchpadCelebrities).rosetta_experts) >+ self.productseries = self.factory.makeProductSeries() >+ self.uploaderperson = self.factory.makePerson() >+ self.potemplate = self.factory.makePOTemplate( >+ productseries=self.productseries) >+ self.entry = self.queue.addOrUpdateEntry( >+ 'demo.pot', '#demo', False, self.uploaderperson, >+ productseries=self.productseries, potemplate=self.potemplate) >+ >+ def _assertCanSetStatus(self, user, entry, expected_list): >+ # Helper to check for all statuses. >+ # Could iterate RosettaImportStatus.items but listing them here >+ # explicitely is better to read. They are sorted alphabetically. >+ possible_statuses = [ >+ RosettaImportStatus.APPROVED, >+ RosettaImportStatus.BLOCKED, >+ RosettaImportStatus.DELETED, >+ RosettaImportStatus.FAILED, >+ RosettaImportStatus.IMPORTED, >+ RosettaImportStatus.NEEDS_REVIEW, >+ ] >+ # Do *not* use assertContentEqual here, as the order matters. >+ self.assertEqual(expected_list, >+ [entry.canSetStatus(status, user) >+ for status in possible_statuses]) >+ >+ def test_canSetStatus_non_admin(self): >+ # A non-privileged users cannot set any status except for retaining >+ # the current status of an entry. >+ some_user = self.factory.makePerson() >+ self._assertCanSetStatus(some_user, self.entry, >+ # A B D F I NR >+ [False, False, False, False, False, True]) >+ self.entry.setStatus( >+ RosettaImportStatus.DELETED, self.rosetta_experts) >+ self._assertCanSetStatus(some_user, self.entry, >+ # A B D F I NR >+ [False, False, True, False, False, False]) >+ >+ def test_canSetStatus_rosetta_expert(self): >+ # Rosetta experts are all-powerful, didn't you know that? >+ self._assertCanSetStatus(self.rosetta_experts, self.entry, >+ # A B D F I NR >+ [True, True, True, True, True, True]) >+ >+ def test_canSetStatus_rosetta_expert_no_target(self): >+ # If the entry has no import target set, even Rosetta experts >+ # cannot set it to approved. >+ self.entry.potemplate = None >+ self._assertCanSetStatus(self.rosetta_experts, self.entry, >+ # A B D F I NR >+ [False, True, True, True, True, True]) >+ >+ def test_canSetStatus_uploader(self): >+ # The uploader can set some statuses. >+ self._assertCanSetStatus(self.uploaderperson, self.entry, >+ # A B D F I NR >+ [False, False, True, False, False, True]) >+ >+ def test_canSetStatus_owner(self): >+ # The owner gets the same permissions. >+ self._assertCanSetStatus(self.productseries.product.owner, self.entry, >+ # A B D F I NR >+ [False, False, True, False, False, True]) >+ >+ def _setUpUbuntu(self): >+ self.ubuntu = getUtility(ILaunchpadCelebrities).ubuntu >+ self.ubuntu_group_owner = self.factory.makePerson() >+ self.ubuntu.translationgroup = ( >+ self.factory.makeTranslationGroup(self.ubuntu_group_owner)) >+ >+ def test_canSetStatus_ubuntu_translation_group(self): >+ # Owners of the Ubuntu translation Groups can set entries Trailing white space. >+ # that are targeted to Ubuntu. >+ self._setUpUbuntu() >+ ubuntu_entry = self.queue.addOrUpdateEntry( >+ 'demo.pot', '#demo', False, self.uploaderperson, >+ distroseries=self.factory.makeDistroRelease(self.ubuntu), >+ sourcepackagename=self.factory.makeSourcePackageName(), >+ potemplate=self.potemplate) >+ self._assertCanSetStatus(self.ubuntu_group_owner, ubuntu_entry, >+ # A B D F I NR >+ [True, True, True, False, False, True]) >+ >+ def test_canSetStatus_ubuntu_translation_group_not_ubuntu(self): >+ # Outside of Ubuntu, owners of the Ubuntu translation Groups have no >+ # powers. >+ self._setUpUbuntu() >+ self._assertCanSetStatus(self.ubuntu_group_owner, self.entry, >+ # A B D F I NR >+ [False, False, False, False, False, True]) >+ >+ >+def test_suite(): >+ return unittest.TestLoader().loadTestsFromName(__name__) >