Merge lp:~adiroiban/launchpad/bug-406477 into lp:launchpad

= Bug 406477 =

Members of the ubuntu-translations-coordinators team can edit templates through the +admin pages.

However, once a template has been disabled, the +admin page is no longer accessible to them, which stops them from e.g. enabling the template again in case of a mistake.

== Implementation details ==

After asking a pre-implementation review from Danilo, he suggest I should use launchpad.TranslationsAdmin, instead of the more generic launchpad.Admin translation.

The previous administration privileges for IPOTemplate was assigned only to RosettaAdmins.
Now it was change to all RosettaAdmin + owner of distro.translationgroup

== Tests ==

There were no test covering the use case of distribution translations coordinator admin rights to templates, so this was added to distroseries-templates

Also the previous rosetta-potemplate-index was not checking the administrative links.

./bin/test -ct "distroseries-templates"
./bin/test -ct "rosetta-potemplate-index"

== Demo and Q/A ==

Make sure you are a member of Ubuntu Translation Coordinators team (ubuntu-l10n-coordinator).
https://launchpad.dev/~ubuntu-l10n-coordinator

Go to the Distro +template page
https://translations.launchpad.dev/ubuntu/hoary/+templates

You should see the Administer page for Evolution, disabled-template .

As a member of Ubuntu Translation Coordinators team you should be able to administer it, just like any other template from that table.

Login as a normal user, you should not see the administration links (only Download), and when trying to manually enter the admin url (https://translations.launchpad.dev/ubuntu/hoary/+source/evolution/+pots/disabled-template/+admin) you should see an access denied page.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/lp/translations/browser/configure.zcml
  lib/lp/translations/browser/potemplate.py
  lib/lp/translations/stories/distroseries/xx-distroseries-templates.txt
  lib/lp/translations/stories/standalone/xx-rosetta-potemplate-index.txt
  lib/lp/translations/templates/object-templates.pt
  lib/lp/translations/templates/potemplate-index.pt

Adi, thanks a lot for working on this. In general, there's one major
complaint about how you implemented permissions. There's a number of
narrative/stylistic suggestions as well, though they are obviously of
lower priority (though still important).

If I sound patronising anywhere, please forgive me: I am not sure how
much of our code base or styling guides you have already had a chance to
figure out yourself, so I might be saying things you already know :)

Anyway, this would need further improvements, but is overall a great
step in the right direction!

  review needsfixing

У уто, 08. 12 2009. у 03:15 +0000, Adi Roiban пише:

> == Implementation details ==
>
> After asking a pre-implementation review from Danilo, he suggest I
> should use launchpad.TranslationsAdmin, instead of the more generic
> launchpad.Admin translation.
>
> The previous administration privileges for IPOTemplate was assigned
> only to RosettaAdmins.
> Now it was change to all RosettaAdmin + owner of
> distro.translationgroup
>
> == Tests ==
>
> There were no test covering the use case of distribution translations
> coordinator admin rights to templates, so this was added to
> distroseries-templates
>
> Also the previous rosetta-potemplate-index was not checking the
> administrative links.

FWIW, sometimes we avoid having too many tests in "pagetests" (i.e.
stories), because they are very fragile. Just moving stuff around in
the layout sometimes breaks them, so we do not test for all possible
cases in them.

> ./bin/test -ct "distroseries-templates"
> ./bin/test -ct "rosetta-potemplate-index"

FWIW, you can pass -t to bin/test multiple times, eg.

  bin/test -vvct distroseries-templates -t rosetta-potemplate-index

(if you really must have colorized output; -vv is basically a standard
verbose mode LP developers use, and -vvv gives you test timings as well)

> == Demo and Q/A ==
>
> Make sure you are a member of Ubuntu Translation Coordinators team
> (ubuntu-l10n-coordinator).
> https://launchpad.dev/~ubuntu-l10n-coordinator
>
> Go to the Distro +template page
> https://translations.launchpad.dev/ubuntu/hoary/+templates
>
> You should see the Administer page for Evolution, disabled-template .
>
> As a member of Ubuntu Translation Coordinators team you should be able
> to administer it, just like any other template from that table.
>
> Login as a normal user, you should not see the administration links
> (only Download), and when trying to manually enter the admin url
> (https://translations.launchpad.dev/ubuntu/hoary/+source/evolution/+pots/disabled-template/+admin) you should see an access denied page.

While playing with it, I discovered that you see disabled templates even
if you are not logged in, or if you don't have sufficient privileges.
Clicking on it gives you a 'not found' error. It's probably not your
code, but it'd be useful to fix it along the way and show disabled
templates only if someone has TranslationsAdmin privilege on them.

> === modified file 'lib/canonical/launchpad/security.py'
> --- lib/canonical/launchpad/security.py 2009-11-19 15:14:53 +0000
> +++ lib/canonical/launchpad/security.py 2009-12-08 03:15:30 +0000
> @@ -479,6 +479,28 @@...

Hi,

Thank you very much for taking the time and reviewing this branch.
It was really ugly. So don't worry, I deserve it :)

I agree that is hard to cover all test paths, is just that right now I don't know what should be tested and what not, and where is the border between a useful test and a bloated test.
Is a test is not needed, feel free to comment and I will delete it.

I'm testing using a single command line, but in MP I put them in different lines to improve readability... stupid thing to do :). For the future I'll put them in a single line.

Disabled templates are only displayed for TranslationsAdmins.
The previous behavior was validated by test stories.

I didn't knew Ubuntu is already a celebrity.
I prefer to keep with Ubuntu, without creating a new distribution. It will just make the story to complicated.
Added a "license=yes" to MakeTranslator , to also sign the translation license.. otherwise that translator has no edit rights.

I'm sorry for failing to write proper stories. Hope I can improve my narrator skills.

Hope the permission are right.

I added stories for testing administration and editing of POTemplate and PoTemplateSubset.

Thank you again for taking the time reviewing this branch.
If there are further issues, have no mercy! :)

It is ok to have these filenames?
xx-potemplate-admin.txt
xx-potemplate-edit.txt
xx-rosetta-potemplate-export.txt
xx-rosetta-potemplate-index.txt

I was thinking at removing rosetta from the filename.

Hi Adi,

Thanks for all the effort you are putting into this: you are not just
solving the bug, you are helping improve the Launchpad code quality as
well (which is why it's a bit harder than expected :). There's still
some work needed on this branch.

У сре, 09. 12 2009. у 01:03 +0000, Adi Roiban пише:

> Thank you very much for taking the time and reviewing this branch.
> It was really ugly. So don't worry, I deserve it :)

You are most welcome - and it wasn't ugly, it just needed further
polish :)

Btw, it's useful to post an incremental diff (i.e. only the changes
since the last review) so it's easier for a reviewer to only go over
that without going through things they have already approved. This
time it's simple because it's only one revision, but sometimes it's
harder than that and then an incremental diff helps.

> I'm testing using a single command line, but in MP I put them in
> different lines to improve readability... stupid thing to do :). For
> the future I'll put them in a single line.

Thanks!

> Disabled templates are only displayed for TranslationsAdmins.
> The previous behavior was validated by test stories.

Cool, thanks for this improvement!

> I didn't knew Ubuntu is already a celebrity.
> I prefer to keep with Ubuntu, without creating a new
> distribution. It will just make the story to complicated.

Well, "complicated" is a relative term. If you create a new
distribution and at least two new templates (one disabled, another
enabled), you'd have a fully self-contained test. That's means that
extra 5 lines of test set-up, you'll get a story which a reader can
understand fully without resorting to "make run" to see what data is
already there.

However, this branch is already too big: do not worry about this and
keep it as-is. For future work, I do suggest you keep the above in
mind.

> Added a "license=yes" to MakeTranslator , to also sign the
> translation license.. otherwise that translator has no edit rights.

Cool, thanks for doing this.

> I'm sorry for failing to write proper stories. Hope I can improve my
> narrator skills.

Don't worry about it. None of us is great at it, and we've got
years of experience. With every review though, we are always getting
better :)

There are still some suggestions on how to improve it, and some tips
on how to think about it when writing stories inline below.

> Hope the permission are right.

Not yet. I'll comment on the actual code below.

> I added stories for testing administration and editing of POTemplate
> and PoTemplateSubset.

Thanks, they look good.

> Thank you again for taking the time reviewing this branch.
> If there are further issues, have no mercy! :)

This is complicated stuff, so don't feel bad if I have no mercy :)

> It is ok to have these filenames?
> xx-potemplate-admin.txt
> xx-potemplate-edit.txt
> xx-rosetta-potemplate-export.txt
> xx-rosetta-potemplate-index.txt
>
> I was thinking at removing rosetta from the filename.

Sure, that'd be great :)

And now, your code with my comments inline:

=== modified file 'lib/canonical/launchpad/security.py'
> --- lib/canonical/launchpad/security.py 2009-12-07 21:58:53 +0000
> +++ lib/canonical/launchpad/secur...

Here is the diff from the last commit.
I will leave the other tasks for another branch.

I looked for all the other places where factory.makeTranslator is used and the change should not affect the current tests.

bin/test -m translations looks ok.

=== 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;
+}
+
+
 /* Translations statistics and legend.
  *
  * Examples:

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

-class AdminPOTemplateDetails(AdminDistributionTranslations):
+# 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))

- return False
+ else:
+ # Template is on a product.
+ return OnlyRosettaExpertsAndAdmins.checkAuthenticated(
+ self, user)

 class EditPOTemplateDetails(AdminPOTemplateDetails, EditByOwnersOrAdmins):
@@ -1636,7 +1645,11 @@
                 user.inTeam(celebs.bazaar_experts))

-class AdminPOTemplateSubset(AdminDistributionTranslations):
+# Please keep this in sync with AdminPOTemplateDetails. Note that
+# th...

У чет, 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.d...

Many thanks for the review... there we a couple of stupid errors :(
For the future I will try to read the whole diff at least twice.

Latest diff.

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css 2009-12-10 12:46:11 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css 2009-12-11 11:33:39 +0000
@@ -682,7 +682,8 @@
  */

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

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2009-12-10 12:46:11 +0000
+++ lib/canonical/launchpad/security.py 2009-12-11 11:33:39 +0000
@@ -1130,9 +1130,9 @@
             return True
         else:
             return (
- OnlyRosettaExpertsAndAdmins.checkAuthenticated(self, user) or
- EditDistributionByDistroOwnersOrAdmins.checkAuthenticated(
- self, user))
+ OnlyRosettaExpertsAndAdmins.checkAuthenticated(self, user) or
+ EditDistributionByDistroOwnersOrAdmins.checkAuthenticated(
+ self, user))

 # Please keep AdminPOTemplateSubset in sync with this, unless you
@@ -1155,8 +1155,7 @@
             distribution = template.distroseries.distribution
             return (
                 AdminDistributionTranslations(
- template.distroseries.distribution).checkAuthenticated(
- user))
+ distribution).checkAuthenticated(user))

         else:
             # Template is on a product.
@@ -1663,16 +1662,13 @@
     def checkAuthenticated(self, user):
         template_set = self.obj
         if template_set.distroseries is not None:
- distro = template_set.distroseries.distribution
- translation_group = distro.translationgroup
- if translation_group and user.inTeam(translation_group.owner):
- return True
-
+ distribution = template_set.distroseries.distribution
             return (
                 AdminDistributionTranslations(
- template_set.distroseries.distribution).checkAuthenticated(user))
-
- return False
+ distribution).checkAuthenticated(user))
+ else:
+ # Template is on a product.
+ return OnlyRosettaExpertsAndAdmins.checkAuthenticated(self, user)

 class AdminDistroSeriesLanguage(OnlyRosettaExpertsAndAdmins):

=== modified file 'lib/lp/translations/stories/standalone/xx-potemplate-admin.txt'
--- lib/lp/translations/stories/standalone/xx-potemplate-admin.txt 2009-08-12 05:09:36 +0000
+++ lib/lp/translations/stories/standalone/xx-potemplate-admin.txt 2009-12-11 11:33:39 +0000
@@ -1,3 +1,10 @@
+Administering POTemplates
+=========================
+
+
+Product templates
+-----------------
+
 The POTemplate admin page lets us to edit any aspect of that object, that's
 why we need to be a Rosetta Expert or a Launchpad admin to use it.

@@ -155,8 +162,29 @@
   >>> print admin_browser.url
   http://translations.launchpad.dev/evolution/trunk/+pots/evolution-renamed

-
-== Distribution templates ==
+Administrators can disable and t...

У пет, 11. 12 2009. у 11:40 +0000, Adi Roiban пише:
> Many thanks for the review... there we a couple of stupid errors :(
> For the future I will try to read the whole diff at least twice.

Great work, thanks again!

 review approve
 merge approve