Am 15.11.2009 01:10, Edwin Grubbs schrieb:
> 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

No problem. Thanks for taking this on in the first place. Let's see what
you got...

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

Oops, something went seriously wrong here. I did not do anything with
that. I now realised that I based my branch on devel and merged Maris'
branch - which is based on db-devel ...

Since this branch is not dependent on yui-3final at all, I created a new
one and merged the relevant revisions. I just don't know how to proceed
now. Should I propose that branch for merging and request a new review?
Yeah, I'll just do that.

I have a diff of the diffs attached that shows that the two branches are
identical except for the mtimes of the files and without the above
symlink addition. This diff-diff was made before I addressed any of the
following issues.

> 
> 
>> === 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?

There was a discussion about the role of the series owner with sinzui
but I forget where. The series owner has no meaning at all and is not
even accessible through the UI.

> 
> 
>> +        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)):

Actually, that is what I changed this line from (previously in the view
code) because I think "and" is easier to understand here. It derives
more directly from the definition "an entry can be approved by an admin
but only if we have an import target".

So, I like this better but if you insist, will change it back ... ;)

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

Ex.. what?  Oh, I get it, "superfluous" .. ;-)

> 
> 
>> +            # 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/

Oh right. Luckily not in a crucial spot.

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

I wish make lint would catch these. Thanks.

> 
> 
>> +        # 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__)
>>
> 

Thank you very much, this went much better than expected! (Except for
the devel - db-devel mix-up I made there).

I'll paste the incremental patch. Somewhere.

Henning