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/security.py 2009-12-09 01:03:22 +0000 > @@ -480,27 +480,6 @@ class OnlyRosettaExpertsAndAdmins(Author > return is_admin_or_rosetta_expert(user) > > > -class AdminDistroTranslations(OnlyRosettaExpertsAndAdmins): > - """LP/Translations admins, distro owners and distro translations > - group owners have administrative rights over templates. > - """ > - > - 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 > - > - > class AdminProductTranslations(AuthorizationBase): > permission = 'launchpad.TranslationsAdmin' > usedfor = IProduct > @@ -1125,10 +1104,56 @@ class EditCodeImportMachine(OnlyVcsImpor > usedfor = ICodeImportMachine > > > -class AdminPOTemplateDetails(AdminDistroTranslations): > +class AdminDistributionTranslations(OnlyRosettaExpertsAndAdmins, > + EditDistributionByDistroOwnersOrAdmins): > + """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. > + """ > + permission = 'launchpad.TranslationsAdmin' > + usedfor = IDistribution > + > + def checkAuthenticated(self, user): > + """Is the user able to manage `IDistribution` translations settings? > + > + Any Launchpad/Launchpad Translations administrator or people allowed > + to edit distribution details are able to change translation settings > + for a distribution. > + """ > + return ( > + OnlyRosettaExpertsAndAdmins.checkAuthenticated(self, user) or > + EditDistributionByDistroOwnersOrAdmins.checkAuthenticated( > + self, user)) What you want to check in AdminDistributionTranslations is if user is a member of the translation group as well, as I advised in my previous review: that's what gives someone the permission on entire distribution, so that's where it should live. (and not only on potemplate admin) I.e. you'd move that bit from AdminPOTemplateDetails here, and the method above would become: def checkAuthenticated(self, user): """Is the user able to manage `IDistribution` translations settings? Any Launchpad/Launchpad Translations administrator or people allowed to edit distribution details are able to change translation settings for a distribution. """ # 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 return ( OnlyRosettaExpertsAndAdmins.checkAuthenticated(self, user) or EditDistributionByDistroOwnersOrAdmins.checkAuthenticated( self, user)) (I am not sure about allowing distribution owners to touch this: most of Ubuntu drivers have no idea about translations setup, so giving them those links is probably a bad idea) And then, you'd use this privilege to check for access on a distribution below. > +class AdminPOTemplateDetails(AdminDistributionTranslations): Derive from OnlyRosettaExpertsAndAdmins, not AdminDistributionTranslations here: since POTemplate can be both on a product and on a distribution, it makes no sense to limit handling only to distribution templates. Also, because you'll use rosetta-experts check below. > + """Controls administration of an `IPOTemplate`. > + > + Allow all persons that can also administer the translations to > + which this template belongs to and also translation group owners. > + > + Product owners does not have administrative privileges. > + """ > permission = 'launchpad.TranslationsAdmin' > usedfor = IPOTemplate > > + 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 > + > + return ( > + AdminDistributionTranslations( > + template.distroseries.distribution).checkAuthenticated(user)) > + > + return False I.e. this would become: def checkAuthenticated(self, user): template = self.obj if template.distroseries is not None: # Template is on a distribution. distribution = template.distroseries.distribution return ( AdminDistributionTranslations( template.distroseries.distribution).checkAuthenticated( user)) else: # Template is on a product. return OnlyRosettaExpertsAndAdmins.checkAuthenticated( self, user) If it's not clear, you can use the approach like I am using above for OnlyRosettaExpertsAndAdmins only when the two classes are context-independent (i.e. check the value of "user" but not of self.obj) or have the same context (same type of self.obj). If they don't, you have to construct a privilege as above by passing in the right context (template.distroseries.distribution is now self.obj in AdminDistributionTranslations). Also note that my code above is not using "distro = ..." but "distribution = ..." :) While it may be easier to type out "distro", "distribution" is so much easier to read when you come back to it later. > class EditPOTemplateDetails(AdminPOTemplateDetails, EditByOwnersOrAdmins): > permission = 'launchpad.Edit' > @@ -1611,10 +1636,31 @@ class AdminBranch(AuthorizationBase): > user.inTeam(celebs.bazaar_experts)) > > > -class AdminPOTemplateSubset(AdminDistroTranslations): > - permission = 'launchpad.Admin' > +class AdminPOTemplateSubset(AdminDistributionTranslations): > + """Controls administration of an `IPOTemplateSubset`. You'd do exactly the same changes as for AdminPOTemplateDetails permission above for this class: namely, use OnlyRosettaExpertsAndAdmins to derive from and use almost identical checkAuthenticated method (except that the initial variable would be called `template_set` instead of `template` :). Considering how they are similar, it'd be useful to put a comment indicating what exact thing this permission is controlling, and how one should keep it in sync with the above unless it's explicitely desired not to be in sync. I.e. add a comment to AdminPOTemplateDetails permission a comment saying: # Please keep AdminPOTemplateSubset in sync with this, unless you # know exactly what you are doing. And then add a comment to AdminPOTemplateSubset saying: # 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. Ideally, we would have something like a IPOTemplateGrouping which defines an interface with (distroseries, sourcepackagename, productseries) which would be shared between IPOTemplate and IPOTemplateSubset, and then we'd have only one permission on it, but that's again very involved and not worth the effort. > === modified file 'lib/lp/testing/factory.py' > --- lib/lp/testing/factory.py 2009-12-07 02:59:18 +0000 > +++ lib/lp/testing/factory.py 2009-12-09 01:03:22 +0000 > @@ -76,6 +76,7 @@ from lp.blueprints.interfaces.specificat > from lp.translations.interfaces.translationgroup import ( > ITranslationGroupSet) > from lp.translations.interfaces.translator import ITranslatorSet > +from lp.translations.interfaces.translationsperson import ITranslationsPerson > from canonical.launchpad.ftests._sqlobject import syncUpdate > from lp.services.mail.signedmessage import SignedMessage > from lp.services.worlddata.interfaces.country import ICountrySet > @@ -502,13 +503,15 @@ class LaunchpadObjectFactory(ObjectFacto > return getUtility(ITranslationGroupSet).new( > name, title, summary, url, owner) > > - def makeTranslator(self, language_code, group=None, person=None): > + def makeTranslator( > + self, language_code, group=None, person=None, license=True): By changing the default behaviour, you risk breaking existing tests which may rely on makeTranslator producing a user who has not signed the license agreement. However, license=True is a good default. If it breaks any existing tests, let's just make it license=False, and be explicit where it's needed. > === modified file 'lib/lp/translations/browser/potemplate.py' > --- lib/lp/translations/browser/potemplate.py 2009-12-07 21:58:53 +0000 > +++ lib/lp/translations/browser/potemplate.py 2009-12-09 01:03:22 +0000 > @@ -688,7 +688,7 @@ class POTemplateSubsetNavigation(Navigat > raise AssertionError('Unknown context for %s' % potemplate.title) > > if ((official_rosetta and potemplate.iscurrent) or > - check_permission('launchpad.Admin', self.context)): > + check_permission('launchpad.TranslationsAdmin', self.context)): > # The target is using officially Launchpad Translations and the > # template is available to be translated, or the user is a is a > # Launchpad administrator in which case we show everything. So, just to make it clear: we write bad code too. I.e. all the stuff like this should move into permission (launchpad.View for POTemplateSubset and POTemplate) itself. But, do not try to fix up all of this now :) > === modified file 'lib/lp/translations/stories/distroseries/xx-distroseries-templates.txt' ... > -Logged-in users will see a link from distro series > +Log in as any user and an the distribution serie page you will see a > +link to the page listing all active templates for that series Again, full stop at the end of the sentence. And it's still better to state the fact then direct users what they should do. I.e. your previous sentence started better, and I'd make it: "Logged-in users see a link to all the active translation templates on a distribution series translation page." > >>> user_browser.open( > ... 'http://translations.launchpad.dev/ubuntu/hoary') > >>> user_browser.getLink('full list of templates').click() > > -Logged-in users can also choose to download all translations for each > -of the templates. > +From this page you can choose to download all translations for each > +active template. "Regular users only see the option to download translations for each of the active templates." Try to think of it as a story, and not a dialog: you, as the writer of the story, are not talking to the reader. It's a story for the reader. > +Visit the Ubuntu Hoary templates page as on translation administrator > +You will see "Edit" and "Administer" for all templates. > + > +Try editing and administering an active template. I'll let you rephrase this yourself. Basic trick: "you" is a bad word to have in a story. Also, don't talk to the reader ("Visit..."). Say who get what (eg. "Translation administrators see... on..."). > +Try editing and administering a disabled template. "Trying to edit/administer disabled templates brings them to the appropriate page." > === modified file 'lib/lp/translations/stories/standalone/xx-rosetta-potemplate-index.txt' > --- lib/lp/translations/stories/standalone/xx-rosetta-potemplate-index.txt 2009-12-08 02:56:31 +0000 > +++ lib/lp/translations/stories/standalone/xx-rosetta-potemplate-index.txt 2009-12-09 01:03:22 +0000 > @@ -1,5 +1,3 @@ > - > - > POTemplate index page > ===================== > > @@ -82,7 +80,7 @@ English translations, but they are not d > Finnish ... ... ... ... P\xf6ll\xe4 > > > -DistroSeries and ProductSeries interlinks > +DistroSeries and ProductSeries links to related templates > ----------------------------------------- You want to extend the "---" markers as well :) > We are presented not only with links to alternate templates from the same > @@ -107,12 +105,11 @@ The template in the other distroseries l > > > -Administration section > +Administering templates > ---------------------- And here. > > -Accessing the POTemplate index as anonymous user, the Administration > -section should not be displayed, and there should be no Download or > -Upload links > +Anonymous visitors see only a list of all existing templates, with no > +administration or download/upload links Sounds good, but don't forget the full stop. :) > === modified file 'lib/lp/translations/templates/object-templates.pt' > --- lib/lp/translations/templates/object-templates.pt 2009-12-07 22:18:35 +0000 > +++ lib/lp/translations/templates/object-templates.pt 2009-12-09 01:03:22 +0000 > @@ -82,7 +82,16 @@ > > > > - > + > + > + > + > + > + > + > + > + > + You should not use procedural programming in TAL :) I.e. do not assign variables when you can avoid it. Especially since if you do it in a better way, it's going to be much nicer. For instance, you can put this check into the view, and you'd avoid checking for permissions with every single template, increasing speed. Or, you could keep it similar to what you've got, but instead show disabled templates as greyed out to admins, eg. ... ... Also, you'd have to define CSS class for inactive-template in lib/canonical/launchpad/icing/style-3.0.css (look for translations section to do it in). With this approach, you'd repeat almost exactly the same code as part of the template row TR. This is not optimal, bit this branch is already too big (and taking too long), so don't worry about it (or we are never going to land it): we can just file a bug tagged with "tech-debt" to clean it up later. I.e. it'd be great if you do that cleanup as well, but in a separate branch (to be honest, I shouldn't have asked you to fix these disabled templates links being shown in this branch: I apologize for that). Cheers, Danilo