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 @@ > """Allow Launchpad's admins and Rosetta experts edit all > fields.""" > return is_admin_or_rosetta_expert(user) > > + > +class AdminDistroTranslations(OnlyRosettaExpertsAndAdmins): Do spell out full names, and do not use abbreviations like "distro". So, "AdminDistributionTranslations". > + """LP/Translations admins, distro owners and distro translations > + group owners have administrative rights over templates. > + """ Please make sure docstrings follow our style guide and PEP-257: https://dev.launchpad.net/PythonStyleGuide#Docstrings I.e. first line should be a short description all in one line, and then you can have a longer description. For instance, something like this: """Class for deciding who can administer distribution translations. This class is used for `launchpad.TranslationsAdmin` privilege on `IDistribution` and `IDistroSeries` and corresponding `IPOTemplate`s, and limits access to Rosetta experts, Launchpad admins and distribution translation group owner. """ > + > + def checkAuthenticated(self, user): > + if OnlyRosettaExpertsAndAdmins.checkAuthenticated(self, > user): > + return True > + > + template = self.obj > + if template.distroseries is not None: > + distro = template.distroseries.distribution > + if user.inTeam(distro.owner): > + return True > + translation_group = distro.translationgroup > + if translation_group and > user.inTeam(translation_group.owner): > + return True > + > + return False > + This is not really correct now that you moved the code and renamed the class. "AdminDistributionTranslations" should be usedfor IDistribution, and should set "launchpad.TranslationsAdmin" permission, just like AdminProductTranslations does. Then, in the checkAuthenticated method of AdminPOTemplateDetails do something like: template = self.obj if template.distroseries is not None: AdminDistributionTranslations( template.distroseries.distribution).checkAuthenticated(user) Then, in the future, we'd also change all of this on other distro-related permissions. > -class AdminPOTemplateSubset(OnlyRosettaExpertsAndAdmins): > +class AdminPOTemplateSubset(AdminDistroTranslations): > permission = 'launchpad.Admin' > usedfor = IPOTemplateSubset And this is again broken. It works by mere accident in that POTemplateSubset and POTemplate implement identical semantics for property "distroseries". Even though it may seem as if you are repeating the same code, considering how it's dealing with a different object, it's not exactly the same. So, you should have something in checkAuthenticated here just like above: template_subset = self.obj if template_subset.distroseries is not None: ... It was ok before because OnlyRosettaExpertsAndAdmins doesn't depend on the object (self.obj), but only on the user being passed in. Other code changes (zcml and browser view) are good. > === modified file > 'lib/lp/translations/stories/distroseries/xx-distroseries-templates.txt' > --- > lib/lp/translations/stories/distroseries/xx-distroseries-templates.txt 2009-11-30 18:18:08 +0000 > +++ > lib/lp/translations/stories/distroseries/xx-distroseries-templates.txt 2009-12-08 03:15:30 +0000 > @@ -20,6 +20,7 @@ > >>> print user_browser.url > http://translations.launchpad.dev/ubuntu/hoary/+templates > > + > The templates table > ------------------- > > @@ -35,7 +36,7 @@ > > >>> table = find_tag_by_id(anon_browser.contents, > 'templates_table') > >>> print extract_text(table) > - Source package Template name Last update > + Source package Template name Last update > evolution disabled-template 2007-01-05 > evolution evolution-2.2 2005-05-06 > evolution man 2006-08-14 Thanks for cleaning up the whitespace here :) > @@ -78,6 +79,31 @@ > pmount man 2006-08-14 Edit Upload > Download Administer > pmount pmount 2005-05-06 Edit Upload > Download Administer > > +Logging in as an Ubuntu Translations Coordinator, you should be able > to > +admin all templates, including those that are currently disabled Keep sentences more documentation like. And keep them sentences with punctuation :) I.e. "Ubuntu Translations Coordinator can administer all templates, including those that are currently disabled." You might also want to explain what is going on below, either with comments ("#...") or narrative: "New user is made an owner of an Ubuntu translation group, thus being considered a translations administrator for Ubuntu." > + >>> from zope.component import getUtility > + >>> from canonical.launchpad.ftests import login, logout > + >>> from canonical.launchpad.interfaces import ( > + ... IDistributionSet, IPersonSet) > + >>> login('