Code review comment for lp:~adiroiban/launchpad/bug-406477

У чет, 10. 12 2009. у 12:44 +0000, Adi Roiban пише:
> Here is the diff from the last commit.
> I will leave the other tasks for another branch.

Cool, thanks. We are almost there, only a few bits remaining.

> === modified file 'lib/canonical/launchpad/icing/style-3-0.css'
> --- lib/canonical/launchpad/icing/style-3-0.css 2009-12-05 03:11:48 +0000
> +++ lib/canonical/launchpad/icing/style-3-0.css 2009-12-10 12:03:04 +0000
> @@ -674,6 +674,18 @@
> padding: 2px 1em 2px 2px;
> }
>
> +/* Templates listing.
> + *
> + * Examples:
> + * https://translations.launchpad.dev/ubuntu/hoary/+templates
> + * https://translations.launchpad.dev/evolution/trunk/+templates
> + */
> +
> +.inactive-template td {
> + background-color: #DCDCDC;
> +}
> +
> +

I tried it out: this looks great. I've also played with

.inactive-template td {
    background-color: #fee;
    color: #855;
}

(to give it a slight redish appearance, to indicate something is wrong
with it), but I am not so sure about it. Your call about that, but if
you are in doubt about something, JFDI. :)

> === modified file 'lib/canonical/launchpad/security.py'
> --- lib/canonical/launchpad/security.py 2009-12-09 01:03:22 +0000
> +++ lib/canonical/launchpad/security.py 2009-12-10 11:31:11 +0000

> @@ -1123,13 +1123,21 @@
> to edit distribution details are able to change translation settings
> for a distribution.
> """
> - return (
> + # Translation group owner for a distribution is also a
> + # translations administrator for it.
> + translation_group = self.obj.translationgroup
> + if translation_group and user.inTeam(translation_group.owner):
> + return True
> + else:
> + return (
> OnlyRosettaExpertsAndAdmins.checkAuthenticated(self, user) or
> EditDistributionByDistroOwnersOrAdmins.checkAuthenticated(
> self, user))

A minor nitpick: please fix the indentation here as well. In general,
if you are using Emacs, python-mode should do it correctly for you if
you just press Tab key once on each of these lines.

> +# Please keep AdminPOTemplateSubset in sync with this, unless you
> +# know exactly what you are doing.
> +class AdminPOTemplateDetails(OnlyRosettaExpertsAndAdmins):
> """Controls administration of an `IPOTemplate`.
>
> Allow all persons that can also administer the translations to
> @@ -1143,16 +1151,17 @@
> def checkAuthenticated(self, user):
> template = self.obj
> if template.distroseries is not None:
> - distro = template.distroseries.distribution
> - translation_group = distro.translationgroup
> - if translation_group and
> user.inTeam(translation_group.owner):
> - return True
> -
> + # Template is on a distribution.
> + distribution = template.distroseries.distribution
> return (
> AdminDistributionTranslations(
> -
> template.distroseries.distribution).checkAuthenticated(user))
> +
> template.distroseries.distribution).checkAuthenticated(
> + user))

Let's replace this `template.distroseries.distribution` reference with a
simpler `distribution`. (I know I gave you this code :)

> -class AdminPOTemplateSubset(AdminDistributionTranslations):
> +# Please keep this in sync with AdminPOTemplateDetails. Note that
> +# this permission controls access to browsing into individual
> +# potemplates, but it's on a different object (POTemplateSubset)
> +# from AdminPOTemplateDetails, even though it looks almost identical
> +class AdminPOTemplateSubset(OnlyRosettaExpertsAndAdmins):
> """Controls administration of an `IPOTemplateSubset`.
>
> Allow all persons that can also administer the translations to
> @@ -1648,16 +1661,16 @@
> usedfor = IPOTemplateSubset
>
> def checkAuthenticated(self, user):
> - template = self.obj
> - if template.distroseries is not None:
> - distro = template.distroseries.distribution
> + template_set = self.obj
> + if template_set.distroseries is not None:
> + distro = template_set.distroseries.distribution

Heh, let's make this "distribution = ..." :)

> translation_group = distro.translationgroup
> if translation_group and user.inTeam(translation_group.owner):
> return True

And remove this bit again. This is checked with
AdminDistributionTranslations now. Basically, this method should be
identical to checkAuthenticated for AdminPOTemplateDetails, with
s/template/template_set/.

> return (
> AdminDistributionTranslations(
> - template.distroseries.distribution).checkAuthenticated(user))
> + template_set.distroseries.distribution).checkAuthenticated(user))

We can make use of the distro/distribution variable here as well,
instead of the longer template_set.distroseries.distribution.

> return False

You are still missing the privilege for products here. Eg. try going to
https://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2-test/+admin and disabling the template and then administering it again. If you made this exactly the same as checkAuthenticated from AdminPOTemplateDetails, you'd have no such issues.

Basically, you can copy entire checkAuthenticated from above, and
replace `template` with `template_set` (just so it's clear what is it
about).

All else is great, thanks!

Cheers,
Danilo