În data de Vi, 11-12-2009 la 20:18 +0000, Guilherme Salgado a scris: > Review: Approve > Hi Adi, > > Just a couple more suggestions, but these are really trivial. Once you > push the changes I'll submit your branch to ec2 to run the full test > suite. > > Once again, thanks a lot for this improvement and the very nice cleanup! > > review approve > status approved [snip] > In this case I think the best is to document that in the method's > docstring (e.g. "This method must not be called if > self.translation_group is None.") and turn the if/elif into an if/else > with an assert, like this: > > if self.translation_team: > ... > else: > assert self.translation_group is not None, ( > "Must not be called when there's no translation group.") > ... OK. Done. > This check can go away once the if/elif is converted into an if/else > with an assertion. The latter is preferred because it will fail > horribly (instead of silently, like it currently does) when the property > is used incorrectly (i.e. when translation_group is None). Yep. > > + > > if not translations_person.translations_relicensing_agreement: > > translation_license_url = PersonFormatterAPI( > > - translations_person).url() + '/translations/+licensing' > > - return ("To make translations in Launchpad you need to " + > > - "agree with the " + > > + translations_person).url( > > + view_name='+licensing', > > + rootsite='translations') > > You might not need rootsite='translations' (as this page is already on > the translations rootsite), but I might be wrong. I've checked the implementation for PersonFormatterAPI.url, and as far I can understand that code, the rootsite is not automatically detected. rootsite is required... as +licensing is not valid for main. Also tested without rootsite and it was not created for translations. > > +Users will see three references to the team managing these translations. > > + > > + >>> print user_browser.getLink( > > + ... 'Ubuntu Spanish Translator',index=0).url > > + http://launchpad.dev/~ubuntu-l10n-es > > + > > + >>> print user_browser.getLink( > > + ... 'Ubuntu Spanish Translator',index=1).url > > + http://launchpad.dev/~ubuntu-l10n-es > > + > > + >>> print user_browser.getLink( > > + ... 'Ubuntu Spanish Translator',index=2).url > > + http://launchpad.dev/~ubuntu-l10n-es > > Is it really important to show that we have 3 links to the team managing > the translations? Fixed. I added new tests for checking when a translator has not agreed the translation license and when translations are CLOSED. Merged with devel since I modified the Factory in a branch that was just commited and those changes also affects these tests. Cheers. Here is the diff. === modified file 'lib/lp/translations/browser/serieslanguage.py' --- lib/lp/translations/browser/serieslanguage.py 2009-12-11 20:11:06 +0000 +++ lib/lp/translations/browser/serieslanguage.py 2009-12-12 06:19:24 +0000 @@ -75,6 +75,8 @@ @property def access_level_description(self): + """Must not be called when there's no translation group.""" + if self.user is None: return ("You are not logged in. Please log in to work " "on translations.") @@ -88,18 +90,13 @@ elif self.translation_group: translations_contact_link = PersonFormatterAPI( self.translation_group.owner).link(None) - - if translations_contact_link is None: - #Having no translation group is a valid case, but the - #template should not call access_level_description for - #this condition. - #We return a blank screen since the information about - #missing group is displaying in a different section - return "" + else: + assert self.translation_group is not None, ( + "Must not be called when there's no translation group.") if not translations_person.translations_relicensing_agreement: translation_license_url = PersonFormatterAPI( - translations_person).url( + self.user).url( view_name='+licensing', rootsite='translations') return ("To make translations in Launchpad you need to " @@ -123,7 +120,7 @@ permission = sample_pofile.translationpermission if permission == TranslationPermission.CLOSED: return ("These templates can be translated only by " - "its managers.") + "their managers.") if self.translation_team is None: return ("Since there is nobody to manage translation " === modified file 'lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt' --- lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt 2009-12-11 20:11:06 +0000 +++ lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt 2009-12-12 06:49:16 +0000 @@ -65,12 +65,15 @@ >>> st_coordinator = factory.makePerson( ... name="ubuntu-l10n-es", ... displayname='Ubuntu Spanish Translators') - + >>> dude = factory.makePerson( + ... name="dude",