Code review comment for lp:~adiroiban/launchpad/bug-427319

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 <email address hidden>