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 On Fri, 2009-12-11 at 19:54 +0000, Adi Roiban wrote: > În data de Vi, 11-12-2009 la 17:48 +0000, Guilherme Salgado a scris: > > > + > > > + translations_person = ITranslationsPerson(self.user) > > > + translations_contact_link = None > > > + > > > + if self.translation_team: > > > + translations_contact_link = PersonFormatterAPI( > > > + self.translation_team.translator).link(None) > > > + elif self.translation_group: > > > + translations_contact_link = PersonFormatterAPI( > > > + self.translation_group.owner).link(None) > > > > Is it possible that self.translation_team and self.translation_group are > > both None, thus leaving translations_contact_link set to None as well? > > I hope that's not possible as it'd cause the code below to crash, and in > > that case we can make this assumption (that we expect at least one of > > them to be non-None) clear in the code by adding an else block with an > > AssertionError. > > > > else: > > raise AssertionError("No translation team/group found.") > > > > And then you can also remove the "translations_contact_link = None" > > line. > > It is possible and it should not be an error. > In such case no information should be display. > > In a normal use case we should not reach this line, as that check is > already done in the template. > > I added a check and returned "". 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.") ... > > > +class DistroSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView): > > > + """View class to render translation status for an `IDistroSeries`.""" > > > + > > > + def initialize(self): > > > + series = self.context.distroseries > > > + super(DistroSeriesLanguageView, self).initialize( > > > + series=series, > > > + translationgroup=series.distribution.translationgroup) > > > + self.parent = self.series.distribution > > > + > > > + > > > +class ProductSeriesLanguageView(BaseSeriesLanguageView, LaunchpadView): > > > + """View class to render translation status for an `IProductSeries`.""" > > > + > > > + def initialize(self): > > > + series = self.context.productseries > > > + super(ProductSeriesLanguageView, self).initialize( > > > + series=series, > > > > I'd drop the 'series = self.context....' line in both classes above and > > just pass series=self.context... to super's initialize(). > > I have leave that attribution to avoid a long line that will follow. > > translationgroup=self.context.productseries.product.translationgroup Oh right, I didn't notice that the first time. > === modified file 'lib/lp/translations/browser/serieslanguage.py' > --- lib/lp/translations/browser/serieslanguage.py 2009-12-11 16:09:04 +0000 > +++ lib/lp/translations/browser/serieslanguage.py 2009-12-11 19:53:07 +0000 > @@ -76,7 +76,7 @@ > @property > def access_level_description(self): > if self.user is None: > - return ("You are not logged in. Please log in to work " + > + return ("You are not logged in. Please log in to work " > "on translations.") > > translations_person = ITranslationsPerson(self.user) > @@ -89,11 +89,21 @@ > 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 "" 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). > + > 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. > + return ("To make translations in Launchpad you need to " > + "agree with the " > === modified file 'lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt' > --- lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt 2009-12-11 16:15:33 +0000 > +++ lib/lp/translations/stories/standalone/xx-serieslanguage-index.txt 2009-12-11 19:45:10 +0000 > @@ -46,7 +46,7 @@ > ... find_tag_by_id(user_browser.contents, 'group-team-info')) > There is no translation group to manage Ubuntu translations. > > -Create a translation group for Ubuntu, togheter with a translation > +Create a translation group for Ubuntu, together with a translation > person for managing Ubuntu Spanish translations and set translation > policy to RESTRICTED. > This is done to so see what the page will look like when they exist. > @@ -63,7 +63,8 @@ > >>> utg = factory.makeTranslationGroup( > ... owner=utc_team, name='utg', title='Ubuntu Translation Group') > >>> st_coordinator = factory.makePerson( > - ... displayname='Ubuntu Spanish Translator') > + ... name="ubuntu-l10n-es", > + ... displayname='Ubuntu Spanish Translators') > > >>> ubuntu = getUtility(ILaunchpadCelebrities).ubuntu > >>> ubuntu.translationgroup = utg > @@ -84,7 +85,7 @@ > > >>> print extract_text( > ... find_tag_by_id(user_browser.contents, 'group-team-info')) > - These Ubuntu translations are managed by Ubuntu Spanish Translator. > + These Ubuntu translations are managed by Ubuntu Spanish Translators. > > Authenticated users can add suggestion but will be held for review by > the members of Spanish translations team. > @@ -92,33 +93,77 @@ > >>> print extract_text( > ... find_tag_by_id( > ... user_browser.contents, 'translation-access-level')) > - Your suggestions will be held for review by the managers of > - these translations. If you need help, ... > - please get in touch with Ubuntu Spanish Translator... > + Your suggestions will be held for review by > + Ubuntu Spanish Translator... > + please get in touch with Ubuntu Spanish Translators... > + > +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? > > Catalan has no translation team for managing translations and since > there is no one to review the work, authenticated users can not add > suggestions. > > - >>> user_browser.open('http://translations.launchpad.dev/ubuntu/hoary/') > + >>> user_browser.open( > + ... 'http://translations.launchpad.dev/ubuntu/hoary/') > >>> user_browser.getLink('Catalan').click() > >>> print user_browser.url > http://translations.launchpad.dev/ubuntu/hoary/+lang/ca > > - >>> print extract_text(find_tag_by_id(user_browser.contents, 'group-team-info')) > + >>> print extract_text( > + ... find_tag_by_id(user_browser.contents, 'group-team-info')) > There is no team to manage ... To set one up, please get in touch > with Ubuntu Translation Coordinators. > > - >>> print extract_text(find_tag_by_id(user_browser.contents, 'translation-access-level')) > + >>> print extract_text(find_tag_by_id( > + ... user_browser.contents, 'translation-access-level')) > Since there is nobody to manage translation ... > your cannot add new suggestions. If you are interested in making > translations, please contact Ubuntu Translation Coordinators... > > + >>> print user_browser.getLink( > + ... 'Ubuntu Translation Coordinators',index=0).url > + http://launchpad.dev/~utc-team > + >>> print user_browser.getLink( > + ... 'Ubuntu Translation Coordinators',index=1).url > + http://launchpad.dev/~utc-team Same here; I think showing just the first link is equally good. > + > Members of translation team and translations admins have full access to > translations. They can add and review translations. > -- Guilherme Salgado