Launchpad itself

Merge lp:~henninge/launchpad/bug-425645 into lp:launchpad

bug-425645
Merge into devel

Proposed by Henning Eggers on 2009-09-09

Status:

Merged

Approved by:

Gavin Panella on 2009-09-10

Approved revision:

no longer in the source branch.

Merged at revision:

not available

Proposed branch:

lp:~henninge/launchpad/bug-425645

Merge into:

lp:launchpad

Diff against target:

None lines

To merge this branch:

bzr merge lp:~henninge/launchpad/bug-425645

Related bugs:

Bug #425645: Provide a POFileBaseView

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Gavin Panella (community)		2009-09-09	Approve on 2009-09-10
Review via email: mp+11430@code.launchpad.net

Revision history for this message

Henning Eggers (henninge) wrote on 2009-09-09:

= Overview =

This branch is part of the work on the redesign of the +translate page. It adds a new view for a POFile that makes use of the new TranslatableMessage model class.

The view provides a number of basic information about the POFile as described in the doctest. It also provides a list of TranslatableMessage objects that represent the content of the file.

The view is meant to replace the existing POFileView and POFileTranslateView. At the moment it carries a lot of duplicate code from those views but that will go away in the future.

== Implementation notes ==

Part of the implementation is a new method for the POFile model class that creates a TranslatableMessage instance. LauchpadObjectFactory also reveived a new method to easily create a Translator entry as this was needed in the doc test.

The view itself is a collection of methods from existing views, namely POFileView, POFileTranslateView and BaseTranslationView with some adaptions, minor and major. One major adaption are the permission_statement and translation_groups_statement which produce (posibly HTML) strings to be inserted directly in the template. These methods take the meat out of pofile-translate-access.pt which I found quite confusing.

I had to file bug 426745 as a follow-up because I did not want to include the generation of markup for the translation group and translator using a tal formatter - mainly because don't know yet how to do that. But it should not be too hard to do.

== Test command ==

bin/test -vvt pofile-base-views -t pofile_view

== Demo/QA ==

The view is not yet connected to any page so it cannot be demoed or QA'ed.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/testing/factory.py
  lib/lp/translations/browser/pofile.py
  lib/lp/translations/browser/tests/pofile-base-views.txt
  lib/lp/translations/browser/tests/test_pofile_view.py
  lib/lp/translations/interfaces/pofile.py
  lib/lp/translations/model/pofile.py

= Overview =

This branch is part of the work on the redesign of the +translate page. It adds a new view for a POFile that makes use of the new TranslatableMessage model class.

The view provides a number of basic information about the POFile as described in the doctest. It also provides a list of TranslatableMessage objects that represent the content of the file.

The view is meant to replace the existing POFileView and POFileTranslateView. At the moment it carries a lot of duplicate code from those views but that will go away in the future.

== Implementation notes ==

== Test command ==

bin/test -vvt pofile-base-views -t pofile_view

== Demo/QA ==

The view is not yet connected to any page so it cannot be demoed or QA'ed.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Revision history for this message

Gavin Panella (allenap) wrote on 2009-09-09:

Download full text (26.2 KiB)

Hi Henning,

Because I don't really understand where this is going to fit in -
partly because my knowledge of rosetta is poor, and partly because
there's no page to look at - it was difficult to review. There are
several parts of the view that don't seem used or tested.

So, think about removing things that aren't needed, or add some simple
tests for them. I've also put some comments in the diff, but nothing
that's really going to set you back much.

Thanks!

Gavin.

> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py 2009-09-04 12:17:11 +0000
> +++ lib/lp/testing/factory.py 2009-09-09 15:11:35 +0000
> @@ -65,6 +65,7 @@
> ISpecificationSet, SpecificationDefinitionStatus)
> from lp.translations.interfaces.translationgroup import (
> ITranslationGroupSet)
> +from lp.translations.interfaces.translator import ITranslatorSet
> from canonical.launchpad.ftests._sqlobject import syncUpdate
> from lp.services.mail.signedmessage import SignedMessage
> from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
> @@ -104,6 +105,7 @@
> from lp.registry.interfaces.sourcepackagename import (
> ISourcePackageNameSet)
> from lp.registry.interfaces.ssh import ISSHKeySet, SSHKeyType
> +from lp.services.worlddata.interfaces.language import ILanguageSet
> from lp.soyuz.interfaces.component import IComponentSet
> from lp.soyuz.interfaces.packageset import IPackagesetSet
> from lp.testing import run_with_login, time_counter
> @@ -485,6 +487,15 @@
> return getUtility(ITranslationGroupSet).new(
> name, title, summary, url, owner)
>
> + def makeTranslator(self, language_code, group=None, person=None):
> + """Create a new, arbitrary `Translator`."""
> + language = getUtility(ILanguageSet).getLanguageByCode(language_code)
> + if group is None:
> + group = self.makeTranslationGroup()
> + if person is None:
> + person = self.makePerson()
> + return getUtility(ITranslatorSet).new(group, language, person)
> +
> def makeMilestone(
> self, product=None, distribution=None, productseries=None, name=None):
> if product is None and distribution is None and productseries is None:
> @@ -755,7 +766,6 @@
> :param branch: The branch that should be the default stacked-on
> branch.
> """
> - from lp.testing import run_with_login
> # 'branch' might be private, so we remove the security proxy to get at
> # the methods.
> naked_branch = removeSecurityProxy(branch)
>
> === modified file 'lib/lp/translations/browser/pofile.py'
> --- lib/lp/translations/browser/pofile.py 2009-09-02 07:13:11 +0000
> +++ lib/lp/translations/browser/pofile.py 2009-09-09 15:11:35 +0000
> @@ -49,6 +49,8 @@
> from canonical.launchpad.webapp.batching import BatchNavigator
> from canonical.launchpad.webapp.menu import structured
>
> +from canonical.launchpad import _
> +
>
> class CustomDropdownWidget(DropdownWidget):
> def _div(self, cssClass, contents, **kw):
> @@ -134,6 +136,261 @@
> links = ('description', 'translate', 'upload', 'download')
>
>
> +class POFileBaseView(Lau...

Hi Henning,

So, think about removing things that aren't needed, or add some simple
tests for them. I've also put some comments in the diff, but nothing
that's really going to set you back much.

Thanks!

Gavin.

> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py	2009-09-04 12:17:11 +0000
> +++ lib/lp/testing/factory.py	2009-09-09 15:11:35 +0000
> @@ -65,6 +65,7 @@
>      ISpecificationSet, SpecificationDefinitionStatus)
>  from lp.translations.interfaces.translationgroup import (
>      ITranslationGroupSet)
> +from lp.translations.interfaces.translator import ITranslatorSet
>  from canonical.launchpad.ftests._sqlobject import syncUpdate
>  from lp.services.mail.signedmessage import SignedMessage
>  from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
> @@ -104,6 +105,7 @@
>  from lp.registry.interfaces.sourcepackagename import (
>      ISourcePackageNameSet)
>  from lp.registry.interfaces.ssh import ISSHKeySet, SSHKeyType
> +from lp.services.worlddata.interfaces.language import ILanguageSet
>  from lp.soyuz.interfaces.component import IComponentSet
>  from lp.soyuz.interfaces.packageset import IPackagesetSet
>  from lp.testing import run_with_login, time_counter
> @@ -485,6 +487,15 @@
>          return getUtility(ITranslationGroupSet).new(
>              name, title, summary, url, owner)
>
> +    def makeTranslator(self, language_code, group=None, person=None):
> +        """Create a new, arbitrary `Translator`."""
> +        language = getUtility(ILanguageSet).getLanguageByCode(language_code)
> +        if group is None:
> +            group = self.makeTranslationGroup()
> +        if person is None:
> +            person = self.makePerson()
> +        return getUtility(ITranslatorSet).new(group, language, person)
> +
>      def makeMilestone(
>          self, product=None, distribution=None, productseries=None, name=None):
>          if product is None and distribution is None and productseries is None:
> @@ -755,7 +766,6 @@
>          :param branch: The branch that should be the default stacked-on
>              branch.
>          """
> -        from lp.testing import run_with_login
>          # 'branch' might be private, so we remove the security proxy to get at
>          # the methods.
>          naked_branch = removeSecurityProxy(branch)
>
> === modified file 'lib/lp/translations/browser/pofile.py'
> --- lib/lp/translations/browser/pofile.py	2009-09-02 07:13:11 +0000
> +++ lib/lp/translations/browser/pofile.py	2009-09-09 15:11:35 +0000
> @@ -49,6 +49,8 @@
>  from canonical.launchpad.webapp.batching import BatchNavigator
>  from canonical.launchpad.webapp.menu import structured
>
> +from canonical.launchpad import _
> +
>
>  class CustomDropdownWidget(DropdownWidget):
>      def _div(self, cssClass, contents, **kw):
> @@ -134,6 +136,261 @@
>      links = ('description', 'translate', 'upload', 'download')
>
>
> +class POFileBaseView(LaunchpadView):
> +    """A basic view for a POFile
> +
> +    This view is different from POFileView as it is the base for a new
> +    generation of POFile views that use the new TranslatableMessage class
> +    to display messages. They will eventually replace POFileView and its
> +    decendants."""
> +
> +    DEFAULT_SHOW = 'all'
> +    DEFAULT_SIZE = 10
> +
> +    def initialize(self):
> +        super(POFileBaseView, self).initialize()
> +
> +        self._initializeShowOption()
> +
> +        self.batchnav = self._buildBatchNavigator()
> +        # These two variables are stored for the sole purpose of being
> +        # output in hidden inputs that preserve the current navigation
> +        # when submitting forms.
> +        self.start = self.batchnav.start
> +        self.size = self.batchnav.currentBatch().size

Consider giving them more descriptive names, like batch_start and
batch_size. Also, they're not used or tested, so consider ditching
them.

> +
> +
> +    @cachedproperty
> +    def contributors(self):
> +        return list(self.context.contributors)

Is this premature optimisation? In any case, consider using an
immutable type like a tuple or frozenset for a cached property.

> +
> +    @property
> +    def user_can_edit(self):
> +        """Does the user have full edit rights for this translation?"""
> +        return self.context.canEditTranslations(self.user)
> +
> +    @property
> +    def user_can_suggest(self):
> +        """Is the user allowed to make suggestions here?"""
> +        return self.context.canAddSuggestions(self.user)
> +
> +    @property
> +    def permission_statement(self):
> +        """Construct the statement about permissions.
> +
> +        Explain the permissions the current user has on this pofile.
> +        """
> +
> +        if self.user_can_edit:
> +            statement = _("You have full access to this translation.")
> +        else:
> +            if self.user_can_suggest:

Could this be elif instead?

> +                statement = _("Your suggestions will be held for review by "
> +                              "the managers of this translation.")
> +            else:
> +                # Check for logged in state
> +                if self.user is None:
> +                    statement = _("You are not logged in.  Please log in to "
> +                                  "work on translations.")
> +                else:
> +                    if not self.has_translationgroup:
> +                        statement = _("This translation is not open for "
> +                                      "changes.")
> +                    else:
> +                        if self.is_managed:
> +                            statement = _("This template can be translated "
> +                                          "only by its managers.")
> +                        else:
> +                            statement = _("There is nobody to manage "
> +                                          "translation into this particular "
> +                                          "language.  If you are interested "
> +                                          "in working on it, please contact "
> +                                          "the translation group.")

Consider using triple-quoted strings and textwrap.dedent() next time
you need to have long strings in source.

> +        return statement
> +
> +    @property
> +    def translation_groups_statement(self):
> +        """List translation groups and translation teams for this translation.
> +
> +        Returns a HTML string that lists the translation groups and the
> +        relevant translators for this translation.
> +        """
> +        if self.translation_group is not None:
> +            language = self.context.language
> +            groups = []
> +            for group in self.context.potemplate.translationgroups:
> +                translator = group.query_translator(language)
> +                # XXX: henninge 2009-09-09 bug=426745:
> +                # The group and translator should be linkified.

I looked into doing something similar recently. The following snippet
might be useful to you:

from zope.component import getAdapter
    from zope.traversing.interfaces import IPathAdapter
    link_html = getAdapter(thing, IPathAdapter, 'fmt').link(None)

> +                if translator is None:
> +                    groups.append(_(u"%s translation group") % group.title)
> +                else:
> +                    groups.append(_(u"%s assigned by %s") % (
> +                        translator.translator.displayname, group.title))
> +            statement = (_(u"This translation is managed by ") +
> +                         _(u" and ").join(groups))+"."

There is an english_list() function in canonical.launchpad.helpers
that will do this.

> +        else:
> +            statement = _(u"No translation group has been assigned.")
> +        return statement
> +
> +    @property
> +    def has_plural_form_information(self):
> +        """Return whether we know the plural forms for this language."""
> +        if self.context.potemplate.hasPluralMessage():
> +            return self.context.language.pluralforms is not None
> +        # If there are no plural forms, we assume that we have the
> +        # plural form information for this language.

Really? I don't understand how the model works, so this is just a
sanity check.

> +        return True
> +
> +    @property
> +    def number_of_plural_forms(self):
> +        """The number of plural forms for the language or 1 if not known."""
> +        if self.context.language.pluralforms is not None:
> +            return self.context.language.pluralforms
> +        return 1
> +
> +    @property
> +    def plural_expression(self):
> +        """The plural expression for this language or the empty string."""
> +        if self.context.language.pluralexpression is not None:
> +            return self.context.language.pluralexpression
> +        return ""
> +
> +    @cachedproperty
> +    def translation_group(self):
> +        """Is there a translation group for this translation?
> +
> +        :return: TranslationGroup or None if not found.
> +        """
> +        translation_groups = self.context.potemplate.translationgroups
> +        if translation_groups is not None and len(translation_groups) > 0:
> +            group = translation_groups[0]
> +        else:
> +            group = None
> +        return group
> +
> +    def _get_translator_entry(self):
> +        """The translator entry or None if none is assigned."""
> +        group = self.translation_group
> +        if group is not None:
> +            return group.query_translator(self.context.language)
> +        return None
> +
> +    @cachedproperty
> +    def translator(self):
> +        """Who is assigned for translations to this language?"""
> +        translator_entry = self._get_translator_entry()
> +        if translator_entry is not None:
> +            return translator_entry.translator
> +        return None
> +
> +    @cachedproperty
> +    def has_any_documentation(self):
> +        """Return whether there is any documentation for this POFile."""
> +        if (self.translation_group is not None and
> +            self.translation_group.translation_guide_url is not None):
> +            return True
> +        translator_entry = self._get_translator_entry()
> +        if (translator_entry is not None and
> +            translator_entry.style_guide_url is not None):
> +            return True
> +        return False
> +
> +    def _initializeShowOption(self):
> +        # Get any value given by the user
> +        self.show = self.request.form.get('show')
> +        self.search_text = self.request.form.get('search')
> +        if self.search_text is not None:
> +            self.show = 'all'
> +
> +        # Functions that deliver the correct message counts for each
> +        # valid option value.
> +        count_functions = {
> +            'all': self.context.messageCount,
> +            'translated': self.context.translatedCount,
> +            'untranslated': self.context.untranslatedCount,
> +            'new_suggestions': self.context.unreviewedCount,
> +            'changed_in_launchpad': self.context.updatesCount,
> +            }
> +
> +        if self.show not in count_functions:
> +            self.show = self.DEFAULT_SHOW
> +
> +        self.shown_count = count_functions[self.show]()
> +
> +    def _buildBatchNavigator(self):
> +        """Construct a BatchNavigator of POTMsgSets and return it."""
> +
> +        # Changing the "show" option resets batching.
> +        old_show_option = self.request.form.get('old_show')
> +        show_option_changed = (
> +            old_show_option is not None and old_show_option != self.show)
> +        if show_option_changed:
> +            force_start = True # start will be 0, by default
> +        else:
> +            force_start = False
> +        return POFileBatchNavigator(self._getSelectedPOTMsgSets(),
> +                                    self.request, size=self.DEFAULT_SIZE,
> +                                    transient_parameters=["old_show"],
> +                                    force_start=force_start)
> +
> +    def _handleShowAll(self):
> +        """Get `POTMsgSet`s when filtering for "all" (but possibly searching).
> +
> +        Normally returns all `POTMsgSet`s for this `POFile`, but also handles
> +        search requests which act as a separate form of filtering.
> +        """
> +        if self.search_text is None:
> +            return self.context.potemplate.getPOTMsgSets()
> +
> +        if len(self.search_text) <= 1:
> +            self.request.response.addWarningNotification(
> +                "Please try searching for a longer string.")
> +            return self.context.potemplate.getPOTMsgSets()
> +
> +        return self.context.findPOTMsgSetsContaining(text=self.search_text)
> +
> +    def _getSelectedPOTMsgSets(self):
> +        """Return a list of the POTMsgSets that will be rendered."""
> +        # The set of message sets we get is based on the selection of kind
> +        # of strings we have in our form.
> +        get_functions = {
> +            'all': self._handleShowAll,
> +            'translated': self.context.getPOTMsgSetTranslated,
> +            'untranslated': self.context.getPOTMsgSetUntranslated,
> +            'new_suggestions': self.context.getPOTMsgSetWithNewSuggestions,
> +            'changed_in_launchpad':
> +                self.context.getPOTMsgSetChangedInLaunchpad,
> +            }
> +
> +        if self.show not in get_functions:
> +            raise UnexpectedFormData('show = "%s"' % self.show)
> +
> +        # We cannot listify the results to avoid additional count queries,
> +        # because we could end up with a list of more than 32000 items with
> +        # an average list of 5000 items.
> +        # The batch system will slice the list of items so we will fetch only
> +        # the exact number of entries we need to render the page.
> +        return get_functions[self.show]()
> +
> +    @property
> +    def messages(self):
> +        """The list of TranslatableMessages to show."""
> +        last = None
> +        messages = []
> +        for potmsgset in self.batchnav.currentBatch():
> +            assert (last is None or
> +                    potmsgset.getSequence(
> +                        self.context.potemplate) >= last.getSequence(
> +                            self.context.potemplate)), (
> +                "POTMsgSets on page not in ascending sequence order")

How expensive is this going to be?

> +            last = potmsgset
> +
> +            messages.append(
> +                self.context.makeTranslatableMessage(potmsgset))
> +        return messages
> +
> +
>  class POFileView(LaunchpadView):
>      """A basic view for a POFile"""
>
>
> === added file 'lib/lp/translations/browser/tests/pofile-base-views.txt'
> --- lib/lp/translations/browser/tests/pofile-base-views.txt	1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/browser/tests/pofile-base-views.txt	2009-09-09 15:11:35 +0000
> @@ -0,0 +1,119 @@
> +POFileBaseView
> +==============
> +
> +POFileBaseView provides different basic information about a POFile and a list
> +of its content as TranslatableMessage objects.
> +
> +    >>> from lp.translations.browser.pofile import POFileBaseView
> +    >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
> +
> +    >>> potemplate = factory.makePOTemplate()
> +    >>> pofile = factory.makePOFile('eo', potemplate)
> +
> +    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
> +    >>> view.initialize()
> +
> +The view provides a statement to be displayed to the user about the
> +permissions the user has on the POFile. When not logged in, the user cannot
> +work on translations.
> +
> +    >>> print view.permission_statement
> +    You are not logged in.  Please log in to work on translations.
> +
> +So we'd better log in.
> +
> +    >>> login('foo.bar@canonical.com')
> +    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
> +    >>> view.initialize()
> +    >>> print view.permission_statement
> +    You have full access to this translation.
> +
> +The view also provides more detailed information about what the user can do.
> +
> +    >>> print view.user_can_edit
> +    True
> +    >>> print view.user_can_suggest
> +    True
> +
> +The view has information about the languages plural forms:
> +
> +    >>> print view.has_plural_form_information
> +    True
> +    >>> print view.number_of_plural_forms
> +    2
> +    >>> print view.plural_expression
> +    n != 1
> +
> +The view also know about the contributers to the translations in this POFile
> +but currently there have not been any contributions yet.
> +
> +    >>> print view.contributors
> +    []
> +
> +So let's make a contribution.
> +
> +    >>> contributor = factory.makePerson(displayname="Contri Butor")
> +    >>> potmsgset = factory.makePOTMsgSet(potemplate, sequence=1)
> +    >>> translation = factory.makeTranslationMessage(pofile, potmsgset,
> +    ...     translator=contributor, reviewer=contributor,
> +    ...     translations=['A translation made by a contributor.'])
> +    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
> +    >>> view.initialize()
> +    >>> print view.contributors[0].displayname
> +    Contri Butor
> +
> +The view has a list of all translations.
> +
> +    >>> print view.messages[0].getCurrentTranslation().msgstr0.translation
> +    A translation made by a contributor.
> +
> +The view can also tell us about the translation group but the pofile is not
> +yet managed by a translation group.
> +
> +    >>> print view.translation_group
> +    None
> +
> +The view makes a nice statement about this fact, too.
> +
> +    >>> print view.translation_groups_statement
> +    No translation group has been assigned.
> +
> +So let's create one and let it manage the translations.
> +
> +    >>> group = factory.makeTranslationGroup(title="Test Translators")
> +    >>> potemplate.product.translationgroup = group
> +    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
> +    >>> view.initialize()
> +    >>> print view.translation_group.title
> +    Test Translators
> +
> +Who is assigned to do translations into this language? Nobody so far.
> +
> +    >>> print view.translator
> +    None
> +
> +Let's change that. A single person can be a tanslator
> +
> +    >>> translator = factory.makeTranslator('eo', group, contributor)
> +    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
> +    >>> view.initialize()
> +    >>> print view.translator.displayname
> +    Contri Butor
> +
> +See what statement the view makes now.
> +
> +    >>> print view.translation_groups_statement
> +    This translation is managed by Contri Butor assigned by Test Translators.

I don't really understand what this means. Does it mean that Contri
Butor is the manager of this translation because he/she is a member of
Test Translators? If so, then it implies that Contri Butor is the only
manager, not just one of many people who can manage it.

> +
> +Neither the group nor the translator have managed to setup some documentation.
> +
> +    >>> view.has_any_documentation
> +    False
> +
> +But now the group finally got around to it.
> +
> +    >>> group.translation_guide_url = "https://launchpad.net/"
> +    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
> +    >>> view.initialize()
> +    >>> view.has_any_documentation
> +    True
>
> === added file 'lib/lp/translations/browser/tests/test_pofile_view.py'
> --- lib/lp/translations/browser/tests/test_pofile_view.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/translations/browser/tests/test_pofile_view.py	2009-09-09 15:11:35 +0000
> @@ -0,0 +1,123 @@
> +# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +__metaclass__ = type
> +
> +from datetime import datetime, timedelta
> +import pytz
> +from unittest import TestLoader
> +
> +from canonical.launchpad.webapp.servers import LaunchpadTestRequest
> +from canonical.testing import LaunchpadZopelessLayer
> +from lp.testing import TestCaseWithFactory
> +from lp.translations.browser.pofile import POFileBaseView
> +
> +
> +class TestPOFileBaseView(TestCaseWithFactory):
> +    """Test POFileBaseView."""
> +
> +    layer = LaunchpadZopelessLayer
> +
> +    def setUp(self):
> +        super(TestPOFileBaseView, self).setUp()
> +        self.potemplate = self.factory.makePOTemplate()
> +        self.pofile = self.factory.makePOFile('eo', self.potemplate)
> +
> +
> +
> +class TestPOFileBaseViewFiltering(TestCaseWithFactory):
> +    """Test POFileBaseView filtering functions."""
> +
> +    layer = LaunchpadZopelessLayer
> +
> +    def gen_now(self):
> +        now = datetime.now(pytz.UTC)
> +        while True:
> +            yield now
> +            now += timedelta(milliseconds=1)
> +
> +    def setUp(self):
> +        super(TestPOFileBaseViewFiltering, self).setUp()
> +        self.now = self.gen_now().next
> +        self.potemplate = self.factory.makePOTemplate()
> +        self.pofile = self.factory.makePOFile('eo', self.potemplate)
> +
> +        # Create a number of POTMsgsets in different states.
> +        # An untranslated message.
> +        self.untranslated = self.factory.makePOTMsgSet(
> +            self.potemplate, sequence=1)
> +        # A translated message.
> +        self.translated = self.factory.makePOTMsgSet(
> +            self.potemplate, sequence=2)
> +        self.factory.makeTranslationMessage(self.pofile, self.translated)
> +        # A translated message with a new suggestion.
> +        self.new_suggestion = self.factory.makePOTMsgSet(
> +            self.potemplate, sequence=3)
> +        self.factory.makeTranslationMessage(
> +            self.pofile, self.new_suggestion,
> +            date_updated=self.now())
> +        self.factory.makeTranslationMessage(
> +            self.pofile, self.new_suggestion, suggestion=True,
> +            date_updated=self.now())
> +        # An imported that was changed in Launchpad.
> +        self.changed = self.factory.makePOTMsgSet(
> +            self.potemplate, sequence=4)
> +        self.factory.makeTranslationMessage(
> +            self.pofile, self.changed, is_imported=True,
> +            date_updated=self.now())
> +        self.factory.makeTranslationMessage(
> +            self.pofile, self.changed,
> +            date_updated=self.now())
> +
> +    def _assertEqualPOTMsgSets(self, expected, messages):
> +        self.assertEqual(expected, [tm.potmsgset for tm in messages])
> +
> +    def test_show_all_messages(self):
> +        view = POFileBaseView(self.pofile, LaunchpadTestRequest())
> +        view.initialize()
> +        self._assertEqualPOTMsgSets(
> +            [self.untranslated, self.translated,
> +             self.new_suggestion, self.changed],
> +            view.messages)
> +
> +    def test_show_translated(self):
> +        form = {'show': 'translated'}
> +        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
> +        view.initialize()
> +        self._assertEqualPOTMsgSets(
> +            [self.translated, self.new_suggestion, self.changed],
> +            view.messages)
> +
> +    def test_show_untranslated(self):
> +        form = {'show': 'untranslated'}
> +        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
> +        view.initialize()
> +        self._assertEqualPOTMsgSets([self.untranslated], view.messages)
> +
> +    def test_show_new_suggestions(self):
> +        form = {'show': 'new_suggestions'}
> +        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
> +        view.initialize()
> +        self._assertEqualPOTMsgSets([self.new_suggestion], view.messages)
> +
> +    def test_show_changed_in_launchpad(self):
> +        form = {'show': 'changed_in_launchpad'}
> +        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
> +        view.initialize()
> +        self._assertEqualPOTMsgSets(
> +             [self.changed], view.messages)
> +
> +    def test_show_invalid_filter(self):
> +        # Invalid filter strings default to showing all messages.
> +        form = {'show': 'foo_bar'}
> +        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
> +        view.initialize()
> +        self._assertEqualPOTMsgSets(
> +            [self.untranslated, self.translated,
> +             self.new_suggestion, self.changed],
> +            view.messages)

Maybe check that view.all also gets set to DEFAULT_SHOW; someone may
come to rely on this.

Also, there are no tests for shown_count.

> +
> +
> +def test_suite():
> +    return TestLoader().loadTestsFromName(__name__)
> +
>
> === modified file 'lib/lp/translations/interfaces/pofile.py'
> --- lib/lp/translations/interfaces/pofile.py	2009-08-07 17:12:44 +0000
> +++ lib/lp/translations/interfaces/pofile.py	2009-09-09 15:11:35 +0000
> @@ -189,6 +189,12 @@
>          `date_created` with newest first.
>          """
>
> +    def makeTranslatableMessage(potmsgset):
> +        """Factory method for an `ITranslatableMessage` object.
> +
> +        :param potmsgset: The `IPOTMsgSet` to combine this pofile with.
> +        """
> +
>      def export(ignore_obsolete=False, export_utf8=False):
>          """Export this PO file as string.
>
>
> === modified file 'lib/lp/translations/model/pofile.py'
> --- lib/lp/translations/model/pofile.py	2009-08-18 11:26:49 +0000
> +++ lib/lp/translations/model/pofile.py	2009-09-09 15:11:35 +0000
> @@ -61,6 +61,7 @@
>  from lp.translations.interfaces.translationsperson import (
>      ITranslationsPerson)
>  from lp.translations.interfaces.translations import TranslationConstants
> +from lp.translations.model.translatablemessage import TranslatableMessage
>  from lp.translations.utilities.translation_common_format import (
>      TranslationMessageData)
>  from canonical.launchpad.webapp.publisher import canonical_url
> @@ -410,6 +411,10 @@
>          """See `IPOFile`."""
>          return self.language.getFullEnglishName(self.variant)
>
> +    def makeTranslatableMessage(self, potmsgset):
> +        """See `IPOFile`."""
> +        return TranslatableMessage(potmsgset, self)
> +

Even though it's trivial, there should probably be a test for this.

>
>  class POFile(SQLBase, POFileMixIn):
>      implements(IPOFile)
>

review: Needs Fixing (code)

Revision history for this message

Henning Eggers (henninge) wrote on 2009-09-09:

Download full text (13.0 KiB)

Am 09.09.2009 18:33, Gavin Panella schrieb:
> Review: Needs Fixing code
> Hi Henning,
>
> Because I don't really understand where this is going to fit in -
> partly because my knowledge of rosetta is poor, and partly because
> there's no page to look at - it was difficult to review. There are
> several parts of the view that don't seem used or tested.

I am sorry you got caught in this. I forgot to mention the blueprint to
give you a sense of the larger picture that we are working on but I
don't know if that would have helped *that* much.

This is really just about having a view class in the first place. It
will receive further addition in the future as the page is developed and
templates use it. Thank you for bearing with us here ;-)

>
> So, think about removing things that aren't needed, or add some simple
> tests for them. I've also put some comments in the diff, but nothing
> that's really going to set you back much.

I think I was able to address or at least explain away all of your
concerns. The code is better now thanks to your review.

>
> Thanks!
>
> Gavin.
>

Thank you very much. Please find my comments below. I will paste an
incremental diff.

Henning

>
>> === modified file 'lib/lp/testing/factory.py'
[...]
>> === modified file 'lib/lp/translations/browser/pofile.py'
>> --- lib/lp/translations/browser/pofile.py 2009-09-02 07:13:11 +0000
>> +++ lib/lp/translations/browser/pofile.py 2009-09-09 15:11:35 +0000
>> @@ -49,6 +49,8 @@
>> from canonical.launchpad.webapp.batching import BatchNavigator
>> from canonical.launchpad.webapp.menu import structured
>>
>> +from canonical.launchpad import _
>> +
>>
>> class CustomDropdownWidget(DropdownWidget):
>> def _div(self, cssClass, contents, **kw):
>> @@ -134,6 +136,261 @@
>> links = ('description', 'translate', 'upload', 'download')
>>
>>
>> +class POFileBaseView(LaunchpadView):
>> + """A basic view for a POFile
>> +
>> + This view is different from POFileView as it is the base for a new
>> + generation of POFile views that use the new TranslatableMessage class
>> + to display messages. They will eventually replace POFileView and its
>> + decendants."""
>> +
>> + DEFAULT_SHOW = 'all'
>> + DEFAULT_SIZE = 10
>> +
>> + def initialize(self):
>> + super(POFileBaseView, self).initialize()
>> +
>> + self._initializeShowOption()
>> +
>> + self.batchnav = self._buildBatchNavigator()
>> + # These two variables are stored for the sole purpose of being
>> + # output in hidden inputs that preserve the current navigation
>> + # when submitting forms.
>> + self.start = self.batchnav.start
>> + self.size = self.batchnav.currentBatch().size
>
> Consider giving them more descriptive names, like batch_start and
> batch_size. Also, they're not used or tested, so consider ditching
> them.

Yeah, I ditched them although they will probably come back in a later
iteration ...

>
>> +
>> +
>> + @cachedproperty
>> + def contributors(self):
>> + return list(self.context.contributors)
>
> Is this premature optimisation? In any case, consider using an
> immutable type like a tuple or frozenset for a cache...

Am 09.09.2009 18:33, Gavin Panella schrieb:
> Review: Needs Fixing code
> Hi Henning,
> 
> Because I don't really understand where this is going to fit in -
> partly because my knowledge of rosetta is poor, and partly because
> there's no page to look at - it was difficult to review. There are
> several parts of the view that don't seem used or tested.

I am sorry you got caught in this. I forgot to mention the blueprint to
give you a sense of the larger picture that we are working on but I
don't know if that would have helped *that* much.

This is really just about having a view class in the first place. It
will receive further addition in the future as the page is developed and
templates use it. Thank you for bearing with us here ;-)

> 
> So, think about removing things that aren't needed, or add some simple
> tests for them. I've also put some comments in the diff, but nothing
> that's really going to set you back much.

I think I was able to address or at least explain away all of your
concerns. The code is better now thanks to your review.

> 
> Thanks!
> 
> Gavin.
>

Thank you very much. Please find my comments below. I will paste an
incremental diff.

Henning

> 
>> === modified file 'lib/lp/testing/factory.py'
[...]
>> === modified file 'lib/lp/translations/browser/pofile.py'
>> --- lib/lp/translations/browser/pofile.py	2009-09-02 07:13:11 +0000
>> +++ lib/lp/translations/browser/pofile.py	2009-09-09 15:11:35 +0000
>> @@ -49,6 +49,8 @@
>>  from canonical.launchpad.webapp.batching import BatchNavigator
>>  from canonical.launchpad.webapp.menu import structured
>>
>> +from canonical.launchpad import _
>> +
>>
>>  class CustomDropdownWidget(DropdownWidget):
>>      def _div(self, cssClass, contents, **kw):
>> @@ -134,6 +136,261 @@
>>      links = ('description', 'translate', 'upload', 'download')
>>
>>
>> +class POFileBaseView(LaunchpadView):
>> +    """A basic view for a POFile
>> +
>> +    This view is different from POFileView as it is the base for a new
>> +    generation of POFile views that use the new TranslatableMessage class
>> +    to display messages. They will eventually replace POFileView and its
>> +    decendants."""
>> +
>> +    DEFAULT_SHOW = 'all'
>> +    DEFAULT_SIZE = 10
>> +
>> +    def initialize(self):
>> +        super(POFileBaseView, self).initialize()
>> +
>> +        self._initializeShowOption()
>> +
>> +        self.batchnav = self._buildBatchNavigator()
>> +        # These two variables are stored for the sole purpose of being
>> +        # output in hidden inputs that preserve the current navigation
>> +        # when submitting forms.
>> +        self.start = self.batchnav.start
>> +        self.size = self.batchnav.currentBatch().size
> 
> Consider giving them more descriptive names, like batch_start and
> batch_size. Also, they're not used or tested, so consider ditching
> them.

Yeah, I ditched them although they will probably come back in a later
iteration ...

> 
>> +
>> +
>> +    @cachedproperty
>> +    def contributors(self):
>> +        return list(self.context.contributors)
> 
> Is this premature optimisation? In any case, consider using an
> immutable type like a tuple or frozenset for a cached property.

Copied code. I changed it to tuple now.

> 
>> +
>> +    @property
>> +    def user_can_edit(self):
>> +        """Does the user have full edit rights for this translation?"""
>> +        return self.context.canEditTranslations(self.user)
>> +
>> +    @property
>> +    def user_can_suggest(self):
>> +        """Is the user allowed to make suggestions here?"""
>> +        return self.context.canAddSuggestions(self.user)
>> +
>> +    @property
>> +    def permission_statement(self):
>> +        """Construct the statement about permissions.
>> +
>> +        Explain the permissions the current user has on this pofile.
>> +        """
>> +
>> +        if self.user_can_edit:
>> +            statement = _("You have full access to this translation.")
>> +        else:
>> +            if self.user_can_suggest:
> 
> Could this be elif instead?

I had thought about this method, too, and came up with eliminating the
statement variable and using direct returns. Hope you like it, too. This
is transformed from a series of tal:condition statements and only when I
was done the cascading shape became obvious.

> 
>> +                statement = _("Your suggestions will be held for review by "
>> +                              "the managers of this translation.")
>> +            else:
>> +                # Check for logged in state
>> +                if self.user is None:
>> +                    statement = _("You are not logged in.  Please log in to "
>> +                                  "work on translations.")
>> +                else:
>> +                    if not self.has_translationgroup:
>> +                        statement = _("This translation is not open for "
>> +                                      "changes.")
>> +                    else:
>> +                        if self.is_managed:
>> +                            statement = _("This template can be translated "
>> +                                          "only by its managers.")
>> +                        else:
>> +                            statement = _("There is nobody to manage "
>> +                                          "translation into this particular "
>> +                                          "language.  If you are interested "
>> +                                          "in working on it, please contact "
>> +                                          "the translation group.")
> 
> Consider using triple-quoted strings and textwrap.dedent() next time
> you need to have long strings in source.

Hm, I don't know what the advantage of that is and I don't know how that
interacts with the _() function. Sooner or later Launchpad will need to
be localized, going by the Top Three Wishes survey (and what I
constantly hear from users). So I am leaving that as it is now.

> 
>> +        return statement
>> +
>> +    @property
>> +    def translation_groups_statement(self):
>> +        """List translation groups and translation teams for this translation.
>> +
>> +        Returns a HTML string that lists the translation groups and the
>> +        relevant translators for this translation.
>> +        """
>> +        if self.translation_group is not None:
>> +            language = self.context.language
>> +            groups = []
>> +            for group in self.context.potemplate.translationgroups:
>> +                translator = group.query_translator(language)
>> +                # XXX: henninge 2009-09-09 bug=426745:
>> +                # The group and translator should be linkified.
> 
> I looked into doing something similar recently. The following snippet
> might be useful to you:
> 
>     from zope.component import getAdapter
>     from zope.traversing.interfaces import IPathAdapter
>     link_html = getAdapter(thing, IPathAdapter, 'fmt').link(None)
>

Thanks, I took a note of that in the bug. There is no formatter for
translation groups yet, so that would have to be done, too.

>> +                if translator is None:
>> +                    groups.append(_(u"%s translation group") % group.title)
>> +                else:
>> +                    groups.append(_(u"%s assigned by %s") % (
>> +                        translator.translator.displayname, group.title))
>> +            statement = (_(u"This translation is managed by ") +
>> +                         _(u" and ").join(groups))+"."
> 
> There is an english_list() function in canonical.launchpad.helpers
> that will do this.

Thanks, I did not know about this. Problem is, again, localization which
would have to be build into english_list first. Also, this list is never
longer than two items. I added a comment about the latter to the code.

> 
>> +        else:
>> +            statement = _(u"No translation group has been assigned.")
>> +        return statement
>> +
>> +    @property
>> +    def has_plural_form_information(self):
>> +        """Return whether we know the plural forms for this language."""
>> +        if self.context.potemplate.hasPluralMessage():
>> +            return self.context.language.pluralforms is not None
>> +        # If there are no plural forms, we assume that we have the
>> +        # plural form information for this language.
> 
> Really? I don't understand how the model works, so this is just a
> sanity check.

Copied code, again. This is used to display a warning if the file
contains strings with plural forms but we do not have information about
how many there should be. If the file does not contain any such strings,
the warning is suppressed this way.

> 
>> +        return True
>> +
[...]
>> +
>> +    @property
>> +    def messages(self):
>> +        """The list of TranslatableMessages to show."""
>> +        last = None
>> +        messages = []
>> +        for potmsgset in self.batchnav.currentBatch():
>> +            assert (last is None or
>> +                    potmsgset.getSequence(
>> +                        self.context.potemplate) >= last.getSequence(
>> +                            self.context.potemplate)), (
>> +                "POTMsgSets on page not in ascending sequence order")
> 
> How expensive is this going to be?

Dunno but this is copied from current code and seems to not be a problem
there.

> 
>> +            last = potmsgset
>> +
>> +            messages.append(
>> +                self.context.makeTranslatableMessage(potmsgset))
>> +        return messages
>> +
>> +
>>  class POFileView(LaunchpadView):
>>      """A basic view for a POFile"""
>>
>>
>> === added file 'lib/lp/translations/browser/tests/pofile-base-views.txt'
>> --- lib/lp/translations/browser/tests/pofile-base-views.txt	1970-01-01 00:00:00 +0000
>> +++ lib/lp/translations/browser/tests/pofile-base-views.txt	2009-09-09 15:11:35 +0000
[...]
>> +
>> +See what statement the view makes now.
>> +
>> +    >>> print view.translation_groups_statement
>> +    This translation is managed by Contri Butor assigned by Test Translators.
> 
> I don't really understand what this means. Does it mean that Contri
> Butor is the manager of this translation because he/she is a member of
> Test Translators? If so, then it implies that Contri Butor is the only
> manager, not just one of many people who can manage it.

A translation group (Test Translators) is responsible for managing
translations for a product or distro. It assigns the management of
translations for specific languages to translators (person/team).

So Contri Butor is the manager of the Esperanto (eo) translations for
this product and has been assigned by Test Translators which is managing
translations into all languages for this product.

This wording is from the current templates. It may well be re-considered
in the future.

> 
>> +
>> +Neither the group nor the translator have managed to setup some documentation.
>> +
>> +    >>> view.has_any_documentation
>> +    False
>> +
>> +But now the group finally got around to it.
>> +
>> +    >>> group.translation_guide_url = "https://launchpad.net/"
>> +    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
>> +    >>> view.initialize()
>> +    >>> view.has_any_documentation
>> +    True
>>
>> === added file 'lib/lp/translations/browser/tests/test_pofile_view.py'
[...]
>> +
>> +    def test_show_invalid_filter(self):
>> +        # Invalid filter strings default to showing all messages.
>> +        form = {'show': 'foo_bar'}
>> +        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
>> +        view.initialize()
>> +        self._assertEqualPOTMsgSets(
>> +            [self.untranslated, self.translated,
>> +             self.new_suggestion, self.changed],
>> +            view.messages)
> 
> Maybe check that view.all also gets set to DEFAULT_SHOW; someone may
> come to rely on this.

Good point! I added checks for that.

> 
> Also, there are no tests for shown_count.
>

Another good catch! I added that, too.

>> +
>> +
>> +def test_suite():
>> +    return TestLoader().loadTestsFromName(__name__)
>> +
>>
>> === modified file 'lib/lp/translations/interfaces/pofile.py'
[...]
>> === modified file 'lib/lp/translations/model/pofile.py'
>> --- lib/lp/translations/model/pofile.py	2009-08-18 11:26:49 +0000
>> +++ lib/lp/translations/model/pofile.py	2009-09-09 15:11:35 +0000
>> @@ -61,6 +61,7 @@
>>  from lp.translations.interfaces.translationsperson import (
>>      ITranslationsPerson)
>>  from lp.translations.interfaces.translations import TranslationConstants
>> +from lp.translations.model.translatablemessage import TranslatableMessage
>>  from lp.translations.utilities.translation_common_format import (
>>      TranslationMessageData)
>>  from canonical.launchpad.webapp.publisher import canonical_url
>> @@ -410,6 +411,10 @@
>>          """See `IPOFile`."""
>>          return self.language.getFullEnglishName(self.variant)
>>
>> +    def makeTranslatableMessage(self, potmsgset):
>> +        """See `IPOFile`."""
>> +        return TranslatableMessage(potmsgset, self)
>> +
> 
> Even though it's trivial, there should probably be a test for this.

Yeah, you're right. I added the test.

> 
>>  class POFile(SQLBase, POFileMixIn):
>>      implements(IPOFile)
>>
>

Revision history for this message

Henning Eggers (henninge) wrote on 2009-09-09:

Download full text (8.2 KiB)

self.batchnav = self._buildBatchNavigator()
- # These two variables are stored for the sole purpose of being
- # output in hidden inputs that preserve the current navigation
- # when submitting forms.
- self.start = self.batchnav.start
- self.size = self.batchnav.currentBatch().size

@cachedproperty
def contributors(self):
- return list(self.context.contributors)
+ return tuple(self.context.contributors)

     @property
     def user_can_edit(self):
@@ -180,31 +175,21 @@
         """

if self.user_can_edit:
- statement = _("You have full access to this translation.")
- else:
- if self.user_can_suggest:
- statement = _("Your suggestions will be held for review by "
- "the managers of this translation.")
- else:
- # Check for logged in state
- if self.user is None:
- statement = _("You are not logged in. Please log in to "
- "work on translations.")
- else:
- if not self.has_translationgroup:
- statement = _("This translation is not open for "
- "changes.")
- else:
- if self.is_managed:
- statement = _("This template can be translated "
- "only by its managers.")
- else:
- statement = _("There is nobody to manage "
- "translation into this particular "
- "language. If you are interested "
- "in working on it, please contact "
- "the translation group.")
- return statement
+ return _("You have full access to this translation.")
+ if self.user_can_suggest:
+ return _("Your suggestions will be held for review by "
+ "the managers of this translation.")
+ # Check for logged in state
+ if self.user is None:
+ return _("You are not logged in. Please log in to "
+ "work on translations.")
+ if not self.has_translationgroup:
+ return _("This translation is not open for changes.")
+ if self.is_managed:
+ return _("This template can be translated only by its managers.")
+ return _("There is nobody to manage translation into this particular "
+ "language. If you are interested in working on it, please "
+ "contact the translation group.")

     @property
     def translation_groups_statement(self):
@@ -225,6 +210,8 @@
                 else:
                     groups.appen...

=== modified file 'lib/lp/translations/browser/pofile.py'
--- lib/lp/translations/browser/pofile.py	2009-09-09 11:04:58 +0000
+++ lib/lp/translations/browser/pofile.py	2009-09-09 17:16:12 +0000
@@ -151,16 +151,11 @@
         self._initializeShowOption()
 
         self.batchnav = self._buildBatchNavigator()
-        # These two variables are stored for the sole purpose of being
-        # output in hidden inputs that preserve the current navigation
-        # when submitting forms.
-        self.start = self.batchnav.start
-        self.size = self.batchnav.currentBatch().size
 
 
     @cachedproperty
     def contributors(self):
-        return list(self.context.contributors)
+        return tuple(self.context.contributors)
 
     @property
     def user_can_edit(self):
@@ -180,31 +175,21 @@
         """
 
         if self.user_can_edit:
-            statement = _("You have full access to this translation.")
-        else:
-            if self.user_can_suggest:
-                statement = _("Your suggestions will be held for review by "
-                              "the managers of this translation.")
-            else:
-                # Check for logged in state
-                if self.user is None:
-                    statement = _("You are not logged in.  Please log in to "
-                                  "work on translations.")
-                else:
-                    if not self.has_translationgroup:
-                        statement = _("This translation is not open for "
-                                      "changes.")
-                    else:
-                        if self.is_managed:
-                            statement = _("This template can be translated "
-                                          "only by its managers.")
-                        else:
-                            statement = _("There is nobody to manage "
-                                          "translation into this particular "
-                                          "language.  If you are interested "
-                                          "in working on it, please contact "
-                                          "the translation group.")
-        return statement
+            return _("You have full access to this translation.")
+        if self.user_can_suggest:
+            return _("Your suggestions will be held for review by "
+                     "the managers of this translation.")
+        # Check for logged in state
+        if self.user is None:
+            return _("You are not logged in.  Please log in to "
+                     "work on translations.")
+        if not self.has_translationgroup:
+            return _("This translation is not open for changes.")
+        if self.is_managed:
+            return _("This template can be translated only by its managers.")
+        return _("There is nobody to manage translation into this particular "
+                 "language.  If you are interested in working on it, please "
+                 "contact the translation group.")
 
     @property
     def translation_groups_statement(self):
@@ -225,6 +210,8 @@
                 else:
                     groups.append(_(u"%s assigned by %s") % (
                         translator.translator.displayname, group.title))
+            # There are at most two translation groups, so just using 'and'
+            # is fine here.
             statement = (_(u"This translation is managed by ") +
                          _(u" and ").join(groups))+"."
         else:

=== modified file 'lib/lp/translations/browser/tests/pofile-base-views.txt'
--- lib/lp/translations/browser/tests/pofile-base-views.txt	2009-09-09 11:04:58 +0000
+++ lib/lp/translations/browser/tests/pofile-base-views.txt	2009-09-09 17:10:33 +0000
@@ -48,7 +48,7 @@
 but currently there have not been any contributions yet.
 
     >>> print view.contributors
-    []
+    ()
 
 So let's make a contribution.

=== modified file 'lib/lp/translations/browser/tests/test_pofile_view.py'
--- lib/lp/translations/browser/tests/test_pofile_view.py	2009-09-09 11:04:58 +0000
+++ lib/lp/translations/browser/tests/test_pofile_view.py	2009-09-09 17:36:17 +0000
@@ -69,12 +69,19 @@
             self.pofile, self.changed,
             date_updated=self.now())
 
+        # Update statistics so that shown_count returns correct values.
+        self.pofile.updateStatistics()
+
     def _assertEqualPOTMsgSets(self, expected, messages):
         self.assertEqual(expected, [tm.potmsgset for tm in messages])
 
     def test_show_all_messages(self):
+        # The default is to show all messages.
         view = POFileBaseView(self.pofile, LaunchpadTestRequest())
         view.initialize()
+        self.assertEqual('all', view.DEFAULT_SHOW)
+        self.assertEqual(view.DEFAULT_SHOW, view.show)
+        self.assertEqual(4, view.shown_count)
         self._assertEqualPOTMsgSets(
             [self.untranslated, self.translated,
              self.new_suggestion, self.changed],
@@ -84,6 +91,7 @@
         form = {'show': 'translated'}
         view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
         view.initialize()
+        self.assertEqual(3, view.shown_count)
         self._assertEqualPOTMsgSets(
             [self.translated, self.new_suggestion, self.changed],
             view.messages)
@@ -92,18 +100,21 @@
         form = {'show': 'untranslated'}
         view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
         view.initialize()
+        self.assertEqual(1, view.shown_count)
         self._assertEqualPOTMsgSets([self.untranslated], view.messages)
 
     def test_show_new_suggestions(self):
         form = {'show': 'new_suggestions'}
         view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
         view.initialize()
+        self.assertEqual(1, view.shown_count)
         self._assertEqualPOTMsgSets([self.new_suggestion], view.messages)
 
     def test_show_changed_in_launchpad(self):
         form = {'show': 'changed_in_launchpad'}
         view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
         view.initialize()
+        self.assertEqual(1, view.shown_count)
         self._assertEqualPOTMsgSets(
              [self.changed], view.messages)
 
@@ -112,6 +123,7 @@
         form = {'show': 'foo_bar'}
         view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
         view.initialize()
+        self.assertEqual(view.DEFAULT_SHOW, view.show)
         self._assertEqualPOTMsgSets(
             [self.untranslated, self.translated,
              self.new_suggestion, self.changed],

=== modified file 'lib/lp/translations/tests/test_pofile.py'
--- lib/lp/translations/tests/test_pofile.py	2009-08-18 11:26:49 +0000
+++ lib/lp/translations/tests/test_pofile.py	2009-09-09 17:54:03 +0000
@@ -13,11 +13,13 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.translations.interfaces.pofile import IPOFileSet
+from lp.translations.interfaces.translatablemessage import (
+    ITranslatableMessage)
 from lp.translations.interfaces.translationmessage import (
     TranslationValidationStatus)
 from lp.translations.interfaces.translationcommonformat import (
     ITranslationFileData)
-from lp.testing import TestCaseWithFactory
+from lp.testing import TestCaseWithFactory, verifyObject
 from canonical.testing import ZopelessDatabaseLayer
 from canonical.launchpad.webapp.publisher import canonical_url
 
@@ -1445,5 +1447,26 @@
         self.assertEquals(self.pofile.updatesCount(), 1)
 
 
+class TestPOFile(TestCaseWithFactory):
+    """Test PO file methods."""
+
+    layer = ZopelessDatabaseLayer
+
+    def setUp(self):
+        # Create a POFile to calculate statistics on.
+        super(TestPOFile, self).setUp()
+        self.pofile = self.factory.makePOFile('sr')
+        self.potemplate = self.pofile.potemplate
+
+        # Create a single POTMsgSet that is used across all tests.
+        self.potmsgset = self.factory.makePOTMsgSet(self.potemplate,
+                                                    sequence=1)
+
+    def test_makeTranslatableMessage(self):
+        # TranslatableMessages can be created from the PO file
+        message = self.pofile.makeTranslatableMessage(self.potmsgset)
+        verifyObject(ITranslatableMessage, message)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

Revision history for this message

Gavin Panella (allenap) wrote on 2009-09-10:

Download full text (14.5 KiB)

On Wed, 09 Sep 2009 18:03:08 -0000
Henning Eggers <email address hidden> wrote:

> Am 09.09.2009 18:33, Gavin Panella schrieb:
> > Review: Needs Fixing code
> > Hi Henning,
> >
> > Because I don't really understand where this is going to fit in -
> > partly because my knowledge of rosetta is poor, and partly because
> > there's no page to look at - it was difficult to review. There are
> > several parts of the view that don't seem used or tested.
>
> I am sorry you got caught in this. I forgot to mention the blueprint to
> give you a sense of the larger picture that we are working on but I
> don't know if that would have helped *that* much.
>
> This is really just about having a view class in the first place. It
> will receive further addition in the future as the page is developed and
> templates use it. Thank you for bearing with us here ;-)

No worries, I wasn't grumbling, just giving you fair warning that I
might have missed the point on several occassions during the review.

>
> >
> > So, think about removing things that aren't needed, or add some simple
> > tests for them. I've also put some comments in the diff, but nothing
> > that's really going to set you back much.
>
> I think I was able to address or at least explain away all of your
> concerns. The code is better now thanks to your review.

Excellent :)

>
> >
> > Thanks!
> >
> > Gavin.
> >
>
> Thank you very much. Please find my comments below. I will paste an
> incremental diff.

The diff all looks good.

review approve
merge approve

>
> Henning
>
>
>
> >
> >> === modified file 'lib/lp/testing/factory.py'
> [...]
> >> === modified file 'lib/lp/translations/browser/pofile.py'
> >> --- lib/lp/translations/browser/pofile.py 2009-09-02 07:13:11 +0000
> >> +++ lib/lp/translations/browser/pofile.py 2009-09-09 15:11:35 +0000
> >> @@ -49,6 +49,8 @@
> >> from canonical.launchpad.webapp.batching import BatchNavigator
> >> from canonical.launchpad.webapp.menu import structured
> >>
> >> +from canonical.launchpad import _
> >> +
> >>
> >> class CustomDropdownWidget(DropdownWidget):
> >> def _div(self, cssClass, contents, **kw):
> >> @@ -134,6 +136,261 @@
> >> links = ('description', 'translate', 'upload', 'download')
> >>
> >>
> >> +class POFileBaseView(LaunchpadView):
> >> + """A basic view for a POFile
> >> +
> >> + This view is different from POFileView as it is the base for a new
> >> + generation of POFile views that use the new TranslatableMessage class
> >> + to display messages. They will eventually replace POFileView and its
> >> + decendants."""
> >> +
> >> + DEFAULT_SHOW = 'all'
> >> + DEFAULT_SIZE = 10
> >> +
> >> + def initialize(self):
> >> + super(POFileBaseView, self).initialize()
> >> +
> >> + self._initializeShowOption()
> >> +
> >> + self.batchnav = self._buildBatchNavigator()
> >> + # These two variables are stored for the sole purpose of being
> >> + # output in hidden inputs that preserve the current navigation
> >> + # when submitting forms.
> >> + self.start = self.batchnav.start
> >> + self.size = self.batchnav.currentBatch().size
> >
> > Consider giving them mor...

On Wed, 09 Sep 2009 18:03:08 -0000
Henning Eggers <henning.eggers@canonical.com> wrote:

No worries, I wasn't grumbling, just giving you fair warning that I
might have missed the point on several occassions during the review.

Excellent :)

>
> >
> > Thanks!
> >
> > Gavin.
> >
>
> Thank you very much. Please find my comments below. I will paste an
> incremental diff.

The diff all looks good.

review approve
 merge approve

>
> Henning
>
>
>
> >
> >> === modified file 'lib/lp/testing/factory.py'
> [...]
> >> === modified file 'lib/lp/translations/browser/pofile.py'
> >> --- lib/lp/translations/browser/pofile.py	2009-09-02 07:13:11 +0000
> >> +++ lib/lp/translations/browser/pofile.py	2009-09-09 15:11:35 +0000
> >> @@ -49,6 +49,8 @@
> >>  from canonical.launchpad.webapp.batching import BatchNavigator
> >>  from canonical.launchpad.webapp.menu import structured
> >>
> >> +from canonical.launchpad import _
> >> +
> >>
> >>  class CustomDropdownWidget(DropdownWidget):
> >>      def _div(self, cssClass, contents, **kw):
> >> @@ -134,6 +136,261 @@
> >>      links = ('description', 'translate', 'upload', 'download')
> >>
> >>
> >> +class POFileBaseView(LaunchpadView):
> >> +    """A basic view for a POFile
> >> +
> >> +    This view is different from POFileView as it is the base for a new
> >> +    generation of POFile views that use the new TranslatableMessage class
> >> +    to display messages. They will eventually replace POFileView and its
> >> +    decendants."""
> >> +
> >> +    DEFAULT_SHOW = 'all'
> >> +    DEFAULT_SIZE = 10
> >> +
> >> +    def initialize(self):
> >> +        super(POFileBaseView, self).initialize()
> >> +
> >> +        self._initializeShowOption()
> >> +
> >> +        self.batchnav = self._buildBatchNavigator()
> >> +        # These two variables are stored for the sole purpose of being
> >> +        # output in hidden inputs that preserve the current navigation
> >> +        # when submitting forms.
> >> +        self.start = self.batchnav.start
> >> +        self.size = self.batchnav.currentBatch().size
> >
> > Consider giving them more descriptive names, like batch_start and
> > batch_size. Also, they're not used or tested, so consider ditching
> > them.
>
> Yeah, I ditched them although they will probably come back in a later
> iteration ...

Ah, okay, if you know you're going to need them then I didn't mean to
press you to remove them. But, as they're simple, it probably is
better to wait until you know exactly what you need, then implement
and test only that.

>
> >
> >> +
> >> +
> >> +    @cachedproperty
> >> +    def contributors(self):
> >> +        return list(self.context.contributors)
> >
> > Is this premature optimisation? In any case, consider using an
> > immutable type like a tuple or frozenset for a cached property.
>
> Copied code. I changed it to tuple now.
>
> >
> >> +
> >> +    @property
> >> +    def user_can_edit(self):
> >> +        """Does the user have full edit rights for this translation?"""
> >> +        return self.context.canEditTranslations(self.user)
> >> +
> >> +    @property
> >> +    def user_can_suggest(self):
> >> +        """Is the user allowed to make suggestions here?"""
> >> +        return self.context.canAddSuggestions(self.user)
> >> +
> >> +    @property
> >> +    def permission_statement(self):
> >> +        """Construct the statement about permissions.
> >> +
> >> +        Explain the permissions the current user has on this pofile.
> >> +        """
> >> +
> >> +        if self.user_can_edit:
> >> +            statement = _("You have full access to this translation.")
> >> +        else:
> >> +            if self.user_can_suggest:
> >
> > Could this be elif instead?
>
> I had thought about this method, too, and came up with eliminating the
> statement variable and using direct returns. Hope you like it, too. This
> is transformed from a series of tal:condition statements and only when I
> was done the cascading shape became obvious.

The diff looks good here. I think I would have done it as
if/elif/.../else, but that's personal preference, not something that
I'm asking you to change :)

>
> >
> >> +                statement = _("Your suggestions will be held for review by "
> >> +                              "the managers of this translation.")
> >> +            else:
> >> +                # Check for logged in state
> >> +                if self.user is None:
> >> +                    statement = _("You are not logged in.  Please log in to "
> >> +                                  "work on translations.")
> >> +                else:
> >> +                    if not self.has_translationgroup:
> >> +                        statement = _("This translation is not open for "
> >> +                                      "changes.")
> >> +                    else:
> >> +                        if self.is_managed:
> >> +                            statement = _("This template can be translated "
> >> +                                          "only by its managers.")
> >> +                        else:
> >> +                            statement = _("There is nobody to manage "
> >> +                                          "translation into this particular "
> >> +                                          "language.  If you are interested "
> >> +                                          "in working on it, please contact "
> >> +                                          "the translation group.")
> >
> > Consider using triple-quoted strings and textwrap.dedent() next time
> > you need to have long strings in source.
>
> Hm, I don't know what the advantage of that is and I don't know how that
> interacts with the _() function. Sooner or later Launchpad will need to
> be localized, going by the Top Three Wishes survey (and what I
> constantly hear from users). So I am leaving that as it is now.

Ah, you're totally right. Multi-line triple-quoted strings are bad
news for localisation, with or without dedent.

>
> >
> >> +        return statement
> >> +
> >> +    @property
> >> +    def translation_groups_statement(self):
> >> +        """List translation groups and translation teams for this translation.
> >> +
> >> +        Returns a HTML string that lists the translation groups and the
> >> +        relevant translators for this translation.
> >> +        """
> >> +        if self.translation_group is not None:
> >> +            language = self.context.language
> >> +            groups = []
> >> +            for group in self.context.potemplate.translationgroups:
> >> +                translator = group.query_translator(language)
> >> +                # XXX: henninge 2009-09-09 bug=426745:
> >> +                # The group and translator should be linkified.
> >
> > I looked into doing something similar recently. The following snippet
> > might be useful to you:
> >
> >     from zope.component import getAdapter
> >     from zope.traversing.interfaces import IPathAdapter
> >     link_html = getAdapter(thing, IPathAdapter, 'fmt').link(None)
> >
>
> Thanks, I took a note of that in the bug. There is no formatter for
> translation groups yet, so that would have to be done, too.
>
> >> +                if translator is None:
> >> +                    groups.append(_(u"%s translation group") % group.title)
> >> +                else:
> >> +                    groups.append(_(u"%s assigned by %s") % (
> >> +                        translator.translator.displayname, group.title))
> >> +            statement = (_(u"This translation is managed by ") +
> >> +                         _(u" and ").join(groups))+"."
> >
> > There is an english_list() function in canonical.launchpad.helpers
> > that will do this.
>
> Thanks, I did not know about this. Problem is, again, localization which
> would have to be build into english_list first. Also, this list is never
> longer than two items. I added a comment about the latter to the code.

Thanks, that's great.

>
> >
> >> +        else:
> >> +            statement = _(u"No translation group has been assigned.")
> >> +        return statement
> >> +
> >> +    @property
> >> +    def has_plural_form_information(self):
> >> +        """Return whether we know the plural forms for this language."""
> >> +        if self.context.potemplate.hasPluralMessage():
> >> +            return self.context.language.pluralforms is not None
> >> +        # If there are no plural forms, we assume that we have the
> >> +        # plural form information for this language.
> >
> > Really? I don't understand how the model works, so this is just a
> > sanity check.
>
> Copied code, again. This is used to display a warning if the file
> contains strings with plural forms but we do not have information about
> how many there should be. If the file does not contain any such strings,
> the warning is suppressed this way.

Thanks for the explanation.

>
> >
> >> +        return True
> >> +
> [...]
> >> +
> >> +    @property
> >> +    def messages(self):
> >> +        """The list of TranslatableMessages to show."""
> >> +        last = None
> >> +        messages = []
> >> +        for potmsgset in self.batchnav.currentBatch():
> >> +            assert (last is None or
> >> +                    potmsgset.getSequence(
> >> +                        self.context.potemplate) >= last.getSequence(
> >> +                            self.context.potemplate)), (
> >> +                "POTMsgSets on page not in ascending sequence order")
> >
> > How expensive is this going to be?
>
> Dunno but this is copied from current code and seems to not be a problem
> there.

Okay.

>
> >
> >> +            last = potmsgset
> >> +
> >> +            messages.append(
> >> +                self.context.makeTranslatableMessage(potmsgset))
> >> +        return messages
> >> +
> >> +
> >>  class POFileView(LaunchpadView):
> >>      """A basic view for a POFile"""
> >>
> >>
> >> === added file 'lib/lp/translations/browser/tests/pofile-base-views.txt'
> >> --- lib/lp/translations/browser/tests/pofile-base-views.txt	1970-01-01 00:00:00 +0000
> >> +++ lib/lp/translations/browser/tests/pofile-base-views.txt	2009-09-09 15:11:35 +0000
> [...]
> >> +
> >> +See what statement the view makes now.
> >> +
> >> +    >>> print view.translation_groups_statement
> >> +    This translation is managed by Contri Butor assigned by Test Translators.
> >
> > I don't really understand what this means. Does it mean that Contri
> > Butor is the manager of this translation because he/she is a member of
> > Test Translators? If so, then it implies that Contri Butor is the only
> > manager, not just one of many people who can manage it.
>
> A translation group (Test Translators) is responsible for managing
> translations for a product or distro. It assigns the management of
> translations for specific languages to translators (person/team).
>
> So Contri Butor is the manager of the Esperanto (eo) translations for
> this product and has been assigned by Test Translators which is managing
> translations into all languages for this product.

Okay, that makes sense now.

>
> This wording is from the current templates. It may well be re-considered
> in the future.
>
> >
> >> +
> >> +Neither the group nor the translator have managed to setup some documentation.
> >> +
> >> +    >>> view.has_any_documentation
> >> +    False
> >> +
> >> +But now the group finally got around to it.
> >> +
> >> +    >>> group.translation_guide_url = "https://launchpad.net/"
> >> +    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
> >> +    >>> view.initialize()
> >> +    >>> view.has_any_documentation
> >> +    True
> >>
> >> === added file 'lib/lp/translations/browser/tests/test_pofile_view.py'
> [...]
> >> +
> >> +    def test_show_invalid_filter(self):
> >> +        # Invalid filter strings default to showing all messages.
> >> +        form = {'show': 'foo_bar'}
> >> +        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
> >> +        view.initialize()
> >> +        self._assertEqualPOTMsgSets(
> >> +            [self.untranslated, self.translated,
> >> +             self.new_suggestion, self.changed],
> >> +            view.messages)
> >
> > Maybe check that view.all also gets set to DEFAULT_SHOW; someone may
> > come to rely on this.
>
> Good point! I added checks for that.
>
> >
> > Also, there are no tests for shown_count.
> >
>
> Another good catch! I added that, too.
>
> >> +
> >> +
> >> +def test_suite():
> >> +    return TestLoader().loadTestsFromName(__name__)
> >> +
> >>
> >> === modified file 'lib/lp/translations/interfaces/pofile.py'
> [...]
> >> === modified file 'lib/lp/translations/model/pofile.py'
> >> --- lib/lp/translations/model/pofile.py	2009-08-18 11:26:49 +0000
> >> +++ lib/lp/translations/model/pofile.py	2009-09-09 15:11:35 +0000
> >> @@ -61,6 +61,7 @@
> >>  from lp.translations.interfaces.translationsperson import (
> >>      ITranslationsPerson)
> >>  from lp.translations.interfaces.translations import TranslationConstants
> >> +from lp.translations.model.translatablemessage import TranslatableMessage
> >>  from lp.translations.utilities.translation_common_format import (
> >>      TranslationMessageData)
> >>  from canonical.launchpad.webapp.publisher import canonical_url
> >> @@ -410,6 +411,10 @@
> >>          """See `IPOFile`."""
> >>          return self.language.getFullEnglishName(self.variant)
> >>
> >> +    def makeTranslatableMessage(self, potmsgset):
> >> +        """See `IPOFile`."""
> >> +        return TranslatableMessage(potmsgset, self)
> >> +
> >
> > Even though it's trivial, there should probably be a test for this.
>
> Yeah, you're right. I added the test.
>
> >
> >>  class POFile(SQLBase, POFileMixIn):
> >>      implements(IPOFile)
> >>
> >

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Barki Mustapha

Celso Providelo

Christian Reis

Christy Awad

Colin Watson

Harpianto,ANDI

Henning Eggers

James Troup

John A Meinel

Kevin bush

Launchpad code reviewers

Launchpad code reviewers from Canonical

Matthew Tanner

Maximiliano Bertacchini

Oguz Ersoz

Simon Brakhane

Ubuntu-BR DevOps

William Grant

alhawiti

api.ng

pedro cavazos

todaioan

wenjingwen

to status/vote changes:

Tzaddi

Tzaddi Belding

Launchpad itself

Merge lp:~henninge/launchpad/bug-425645 into lp:launchpad

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'lib/lp/testing/factory.py'
 --- lib/lp/testing/factory.py	2009-09-04 12:17:11 +0000
 +++ lib/lp/testing/factory.py	2009-09-09 11:04:58 +0000
@@ -65,6 +65,7 @@
      ISpecificationSet, SpecificationDefinitionStatus)
  from lp.translations.interfaces.translationgroup import (
      ITranslationGroupSet)
++from lp.translations.interfaces.translator import ITranslatorSet
  from canonical.launchpad.ftests._sqlobject import syncUpdate
  from lp.services.mail.signedmessage import SignedMessage
  from canonical.launchpad.webapp.dbpolicy import MasterDatabasePolicy
@@ -104,6 +105,7 @@
  from lp.registry.interfaces.sourcepackagename import (
      ISourcePackageNameSet)
  from lp.registry.interfaces.ssh import ISSHKeySet, SSHKeyType
++from lp.services.worlddata.interfaces.language import ILanguageSet
  from lp.soyuz.interfaces.component import IComponentSet
  from lp.soyuz.interfaces.packageset import IPackagesetSet
  from lp.testing import run_with_login, time_counter
@@ -485,6 +487,15 @@
          return getUtility(ITranslationGroupSet).new(
              name, title, summary, url, owner)
++    def makeTranslator(self, language_code, group=None, person=None):
++        """Create a new, arbitrary `Translator`."""
++        language = getUtility(ILanguageSet).getLanguageByCode(language_code)
++        if group is None:
++            group = self.makeTranslationGroup()
++        if person is None:
++            person = self.makePerson()
++        return getUtility(ITranslatorSet).new(group, language, person)
++
      def makeMilestone(
          self, product=None, distribution=None, productseries=None, name=None):
          if product is None and distribution is None and productseries is None:
@@ -755,7 +766,6 @@
          :param branch: The branch that should be the default stacked-on
              branch.
          """
--        from lp.testing import run_with_login
          # 'branch' might be private, so we remove the security proxy to get at
          # the methods.
          naked_branch = removeSecurityProxy(branch)
 === modified file 'lib/lp/translations/browser/pofile.py'
 --- lib/lp/translations/browser/pofile.py	2009-08-31 13:06:42 +0000
 +++ lib/lp/translations/browser/pofile.py	2009-09-09 11:04:58 +0000
@@ -47,6 +47,8 @@
  from canonical.launchpad.webapp.batching import BatchNavigator
  from canonical.launchpad.webapp.menu import structured
++from canonical.launchpad import _
++
  class CustomDropdownWidget(DropdownWidget):
      def _div(self, cssClass, contents, **kw):
@@ -132,6 +134,261 @@
      links = ('description', 'translate', 'upload', 'download')
++class POFileBaseView(LaunchpadView):
++    """A basic view for a POFile
++
++    This view is different from POFileView as it is the base for a new
++    generation of POFile views that use the new TranslatableMessage class
++    to display messages. They will eventually replace POFileView and its
++    decendants."""
++
++    DEFAULT_SHOW = 'all'
++    DEFAULT_SIZE = 10
++
++    def initialize(self):
++        super(POFileBaseView, self).initialize()
++
++        self._initializeShowOption()
++
++        self.batchnav = self._buildBatchNavigator()
++        # These two variables are stored for the sole purpose of being
++        # output in hidden inputs that preserve the current navigation
++        # when submitting forms.
++        self.start = self.batchnav.start
++        self.size = self.batchnav.currentBatch().size
++
++
++    @cachedproperty
++    def contributors(self):
++        return list(self.context.contributors)
++
++    @property
++    def user_can_edit(self):
++        """Does the user have full edit rights for this translation?"""
++        return self.context.canEditTranslations(self.user)
++
++    @property
++    def user_can_suggest(self):
++        """Is the user allowed to make suggestions here?"""
++        return self.context.canAddSuggestions(self.user)
++
++    @property
++    def permission_statement(self):
++        """Construct the statement about permissions.
++
++        Explain the permissions the current user has on this pofile.
++        """
++
++        if self.user_can_edit:
++            statement = _("You have full access to this translation.")
++        else:
++            if self.user_can_suggest:
++                statement = _("Your suggestions will be held for review by "
++                              "the managers of this translation.")
++            else:
++                # Check for logged in state
++                if self.user is None:
++                    statement = _("You are not logged in.  Please log in to "
++                                  "work on translations.")
++                else:
++                    if not self.has_translationgroup:
++                        statement = _("This translation is not open for "
++                                      "changes.")
++                    else:
++                        if self.is_managed:
++                            statement = _("This template can be translated "
++                                          "only by its managers.")
++                        else:
++                            statement = _("There is nobody to manage "
++                                          "translation into this particular "
++                                          "language.  If you are interested "
++                                          "in working on it, please contact "
++                                          "the translation group.")
++        return statement
++
++    @property
++    def translation_groups_statement(self):
++        """List translation groups and translation teams for this translation.
++
++        Returns a HTML string that lists the translation groups and the
++        relevant translators for this translation.
++        """
++        if self.translation_group is not None:
++            language = self.context.language
++            groups = []
++            for group in self.context.potemplate.translationgroups:
++                translator = group.query_translator(language)
++                # XXX: henninge 2009-09-09 bug=426745:
++                # The group and translator should be linkified.
++                if translator is None:
++                    groups.append(_(u"%s translation group") % group.title)
++                else:
++                    groups.append(_(u"%s assigned by %s") % (
++                        translator.translator.displayname, group.title))
++            statement = (_(u"This translation is managed by ") +
++                         _(u" and ").join(groups))+"."
++        else:
++            statement = _(u"No translation group has been assigned.")
++        return statement
++
++    @property
++    def has_plural_form_information(self):
++        """Return whether we know the plural forms for this language."""
++        if self.context.potemplate.hasPluralMessage():
++            return self.context.language.pluralforms is not None
++        # If there are no plural forms, we assume that we have the
++        # plural form information for this language.
++        return True
++
++    @property
++    def number_of_plural_forms(self):
++        """The number of plural forms for the language or 1 if not known."""
++        if self.context.language.pluralforms is not None:
++            return self.context.language.pluralforms
++        return 1
++
++    @property
++    def plural_expression(self):
++        """The plural expression for this language or the empty string."""
++        if self.context.language.pluralexpression is not None:
++            return self.context.language.pluralexpression
++        return ""
++
++    @cachedproperty
++    def translation_group(self):
++        """Is there a translation group for this translation?
++
++        :return: TranslationGroup or None if not found.
++        """
++        translation_groups = self.context.potemplate.translationgroups
++        if translation_groups is not None and len(translation_groups) > 0:
++            group = translation_groups[0]
++        else:
++            group = None
++        return group
++
++    def _get_translator_entry(self):
++        """The translator entry or None if none is assigned."""
++        group = self.translation_group
++        if group is not None:
++            return group.query_translator(self.context.language)
++        return None
++
++    @cachedproperty
++    def translator(self):
++        """Who is assigned for translations to this language?"""
++        translator_entry = self._get_translator_entry()
++        if translator_entry is not None:
++            return translator_entry.translator
++        return None
++
++    @cachedproperty
++    def has_any_documentation(self):
++        """Return whether there is any documentation for this POFile."""
++        if (self.translation_group is not None and
++            self.translation_group.translation_guide_url is not None):
++            return True
++        translator_entry = self._get_translator_entry()
++        if (translator_entry is not None and
++            translator_entry.style_guide_url is not None):
++            return True
++        return False
++
++    def _initializeShowOption(self):
++        # Get any value given by the user
++        self.show = self.request.form.get('show')
++        self.search_text = self.request.form.get('search')
++        if self.search_text is not None:
++            self.show = 'all'
++
++        # Functions that deliver the correct message counts for each
++        # valid option value.
++        count_functions = {
++            'all': self.context.messageCount,
++            'translated': self.context.translatedCount,
++            'untranslated': self.context.untranslatedCount,
++            'new_suggestions': self.context.unreviewedCount,
++            'changed_in_launchpad': self.context.updatesCount,
++            }
++
++        if self.show not in count_functions:
++            self.show = self.DEFAULT_SHOW
++
++        self.shown_count = count_functions[self.show]()
++
++    def _buildBatchNavigator(self):
++        """Construct a BatchNavigator of POTMsgSets and return it."""
++
++        # Changing the "show" option resets batching.
++        old_show_option = self.request.form.get('old_show')
++        show_option_changed = (
++            old_show_option is not None and old_show_option != self.show)
++        if show_option_changed:
++            force_start = True # start will be 0, by default
++        else:
++            force_start = False
++        return POFileBatchNavigator(self._getSelectedPOTMsgSets(),
++                                    self.request, size=self.DEFAULT_SIZE,
++                                    transient_parameters=["old_show"],
++                                    force_start=force_start)
++
++    def _handleShowAll(self):
++        """Get `POTMsgSet`s when filtering for "all" (but possibly searching).
++
++        Normally returns all `POTMsgSet`s for this `POFile`, but also handles
++        search requests which act as a separate form of filtering.
++        """
++        if self.search_text is None:
++            return self.context.potemplate.getPOTMsgSets()
++
++        if len(self.search_text) <= 1:
++            self.request.response.addWarningNotification(
++                "Please try searching for a longer string.")
++            return self.context.potemplate.getPOTMsgSets()
++
++        return self.context.findPOTMsgSetsContaining(text=self.search_text)
++
++    def _getSelectedPOTMsgSets(self):
++        """Return a list of the POTMsgSets that will be rendered."""
++        # The set of message sets we get is based on the selection of kind
++        # of strings we have in our form.
++        get_functions = {
++            'all': self._handleShowAll,
++            'translated': self.context.getPOTMsgSetTranslated,
++            'untranslated': self.context.getPOTMsgSetUntranslated,
++            'new_suggestions': self.context.getPOTMsgSetWithNewSuggestions,
++            'changed_in_launchpad':
++                self.context.getPOTMsgSetChangedInLaunchpad,
++            }
++
++        if self.show not in get_functions:
++            raise UnexpectedFormData('show = "%s"' % self.show)
++
++        # We cannot listify the results to avoid additional count queries,
++        # because we could end up with a list of more than 32000 items with
++        # an average list of 5000 items.
++        # The batch system will slice the list of items so we will fetch only
++        # the exact number of entries we need to render the page.
++        return get_functions[self.show]()
++
++    @property
++    def messages(self):
++        """The list of TranslatableMessages to show."""
++        last = None
++        messages = []
++        for potmsgset in self.batchnav.currentBatch():
++            assert (last is None or
++                    potmsgset.getSequence(
++                        self.context.potemplate) >= last.getSequence(
++                            self.context.potemplate)), (
++                "POTMsgSets on page not in ascending sequence order")
++            last = potmsgset
++
++            messages.append(
++                self.context.makeTranslatableMessage(potmsgset))
++        return messages
++
++
  class POFileView(LaunchpadView):
      """A basic view for a POFile"""
 === added file 'lib/lp/translations/browser/tests/pofile-base-views.txt'
 --- lib/lp/translations/browser/tests/pofile-base-views.txt	1970-01-01 00:00:00 +0000
 +++ lib/lp/translations/browser/tests/pofile-base-views.txt	2009-09-09 11:04:58 +0000
@@ -0,0 +1,119 @@
++POFileBaseView
++==============
++
++POFileBaseView provides different basic information about a POFile and a list
++of its content as TranslatableMessage objects.
++
++    >>> from lp.translations.browser.pofile import POFileBaseView
++    >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
++
++    >>> potemplate = factory.makePOTemplate()
++    >>> pofile = factory.makePOFile('eo', potemplate)
++
++    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
++    >>> view.initialize()
++
++The view provides a statement to be displayed to the user about the
++permissions the user has on the POFile. When not logged in, the user cannot
++work on translations.
++
++    >>> print view.permission_statement
++    You are not logged in.  Please log in to work on translations.
++
++So we'd better log in.
++
++    >>> login('foo.bar@canonical.com')
++    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
++    >>> view.initialize()
++    >>> print view.permission_statement
++    You have full access to this translation.
++
++The view also provides more detailed information about what the user can do.
++
++    >>> print view.user_can_edit
++    True
++    >>> print view.user_can_suggest
++    True
++
++The view has information about the languages plural forms:
++
++    >>> print view.has_plural_form_information
++    True
++    >>> print view.number_of_plural_forms
++    2
++    >>> print view.plural_expression
++    n != 1
++
++The view also know about the contributers to the translations in this POFile
++but currently there have not been any contributions yet.
++
++    >>> print view.contributors
++    []
++
++So let's make a contribution.
++
++    >>> contributor = factory.makePerson(displayname="Contri Butor")
++    >>> potmsgset = factory.makePOTMsgSet(potemplate, sequence=1)
++    >>> translation = factory.makeTranslationMessage(pofile, potmsgset,
++    ...     translator=contributor, reviewer=contributor,
++    ...     translations=['A translation made by a contributor.'])
++    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
++    >>> view.initialize()
++    >>> print view.contributors[0].displayname
++    Contri Butor
++
++The view has a list of all translations.
++
++    >>> print view.messages[0].getCurrentTranslation().msgstr0.translation
++    A translation made by a contributor.
++
++The view can also tell us about the translation group but the pofile is not
++yet managed by a translation group.
++
++    >>> print view.translation_group
++    None
++
++The view makes a nice statement about this fact, too.
++
++    >>> print view.translation_groups_statement
++    No translation group has been assigned.
++
++So let's create one and let it manage the translations.
++
++    >>> group = factory.makeTranslationGroup(title="Test Translators")
++    >>> potemplate.product.translationgroup = group
++    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
++    >>> view.initialize()
++    >>> print view.translation_group.title
++    Test Translators
++
++Who is assigned to do translations into this language? Nobody so far.
++
++    >>> print view.translator
++    None
++
++Let's change that. A single person can be a tanslator
++
++    >>> translator = factory.makeTranslator('eo', group, contributor)
++    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
++    >>> view.initialize()
++    >>> print view.translator.displayname
++    Contri Butor
++
++See what statement the view makes now.
++
++    >>> print view.translation_groups_statement
++    This translation is managed by Contri Butor assigned by Test Translators.
++
++Neither the group nor the translator have managed to setup some documentation.
++
++    >>> view.has_any_documentation
++    False
++
++But now the group finally got around to it.
++
++    >>> group.translation_guide_url = "https://launchpad.net/"
++    >>> view = POFileBaseView(pofile, LaunchpadTestRequest())
++    >>> view.initialize()
++    >>> view.has_any_documentation
++    True
 === added file 'lib/lp/translations/browser/tests/test_pofile_view.py'
 --- lib/lp/translations/browser/tests/test_pofile_view.py	1970-01-01 00:00:00 +0000
 +++ lib/lp/translations/browser/tests/test_pofile_view.py	2009-09-09 11:04:58 +0000
@@ -0,0 +1,123 @@
++# Copyright 2009 Canonical Ltd.  This software is licensed under the
++# GNU Affero General Public License version 3 (see the file LICENSE).
++
++__metaclass__ = type
++
++from datetime import datetime, timedelta
++import pytz
++from unittest import TestLoader
++
++from canonical.launchpad.webapp.servers import LaunchpadTestRequest
++from canonical.testing import LaunchpadZopelessLayer
++from lp.testing import TestCaseWithFactory
++from lp.translations.browser.pofile import POFileBaseView
++
++
++class TestPOFileBaseView(TestCaseWithFactory):
++    """Test POFileBaseView."""
++
++    layer = LaunchpadZopelessLayer
++
++    def setUp(self):
++        super(TestPOFileBaseView, self).setUp()
++        self.potemplate = self.factory.makePOTemplate()
++        self.pofile = self.factory.makePOFile('eo', self.potemplate)
++
++
++
++class TestPOFileBaseViewFiltering(TestCaseWithFactory):
++    """Test POFileBaseView filtering functions."""
++
++    layer = LaunchpadZopelessLayer
++
++    def gen_now(self):
++        now = datetime.now(pytz.UTC)
++        while True:
++            yield now
++            now += timedelta(milliseconds=1)
++
++    def setUp(self):
++        super(TestPOFileBaseViewFiltering, self).setUp()
++        self.now = self.gen_now().next
++        self.potemplate = self.factory.makePOTemplate()
++        self.pofile = self.factory.makePOFile('eo', self.potemplate)
++
++        # Create a number of POTMsgsets in different states.
++        # An untranslated message.
++        self.untranslated = self.factory.makePOTMsgSet(
++            self.potemplate, sequence=1)
++        # A translated message.
++        self.translated = self.factory.makePOTMsgSet(
++            self.potemplate, sequence=2)
++        self.factory.makeTranslationMessage(self.pofile, self.translated)
++        # A translated message with a new suggestion.
++        self.new_suggestion = self.factory.makePOTMsgSet(
++            self.potemplate, sequence=3)
++        self.factory.makeTranslationMessage(
++            self.pofile, self.new_suggestion,
++            date_updated=self.now())
++        self.factory.makeTranslationMessage(
++            self.pofile, self.new_suggestion, suggestion=True,
++            date_updated=self.now())
++        # An imported that was changed in Launchpad.
++        self.changed = self.factory.makePOTMsgSet(
++            self.potemplate, sequence=4)
++        self.factory.makeTranslationMessage(
++            self.pofile, self.changed, is_imported=True,
++            date_updated=self.now())
++        self.factory.makeTranslationMessage(
++            self.pofile, self.changed,
++            date_updated=self.now())
++
++    def _assertEqualPOTMsgSets(self, expected, messages):
++        self.assertEqual(expected, [tm.potmsgset for tm in messages])
++
++    def test_show_all_messages(self):
++        view = POFileBaseView(self.pofile, LaunchpadTestRequest())
++        view.initialize()
++        self._assertEqualPOTMsgSets(
++            [self.untranslated, self.translated,
++             self.new_suggestion, self.changed],
++            view.messages)
++
++    def test_show_translated(self):
++        form = {'show': 'translated'}
++        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
++        view.initialize()
++        self._assertEqualPOTMsgSets(
++            [self.translated, self.new_suggestion, self.changed],
++            view.messages)
++
++    def test_show_untranslated(self):
++        form = {'show': 'untranslated'}
++        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
++        view.initialize()
++        self._assertEqualPOTMsgSets([self.untranslated], view.messages)
++
++    def test_show_new_suggestions(self):
++        form = {'show': 'new_suggestions'}
++        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
++        view.initialize()
++        self._assertEqualPOTMsgSets([self.new_suggestion], view.messages)
++
++    def test_show_changed_in_launchpad(self):
++        form = {'show': 'changed_in_launchpad'}
++        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
++        view.initialize()
++        self._assertEqualPOTMsgSets(
++             [self.changed], view.messages)
++
++    def test_show_invalid_filter(self):
++        # Invalid filter strings default to showing all messages.
++        form = {'show': 'foo_bar'}
++        view = POFileBaseView(self.pofile, LaunchpadTestRequest(form=form))
++        view.initialize()
++        self._assertEqualPOTMsgSets(
++            [self.untranslated, self.translated,
++             self.new_suggestion, self.changed],
++            view.messages)
++
++
++def test_suite():
++    return TestLoader().loadTestsFromName(__name__)
++
 === modified file 'lib/lp/translations/interfaces/pofile.py'
 --- lib/lp/translations/interfaces/pofile.py	2009-08-07 17:12:44 +0000
 +++ lib/lp/translations/interfaces/pofile.py	2009-09-09 11:04:58 +0000
@@ -189,6 +189,12 @@
          `date_created` with newest first.
          """
++    def makeTranslatableMessage(potmsgset):
++        """Factory method for an `ITranslatableMessage` object.
++
++        :param potmsgset: The `IPOTMsgSet` to combine this pofile with.
++        """
++
      def export(ignore_obsolete=False, export_utf8=False):
          """Export this PO file as string.
 === modified file 'lib/lp/translations/model/pofile.py'
 --- lib/lp/translations/model/pofile.py	2009-08-18 11:26:49 +0000
 +++ lib/lp/translations/model/pofile.py	2009-09-09 11:04:58 +0000
@@ -61,6 +61,7 @@
  from lp.translations.interfaces.translationsperson import (
      ITranslationsPerson)
  from lp.translations.interfaces.translations import TranslationConstants
++from lp.translations.model.translatablemessage import TranslatableMessage
  from lp.translations.utilities.translation_common_format import (
      TranslationMessageData)
  from canonical.launchpad.webapp.publisher import canonical_url
@@ -410,6 +411,10 @@
          """See `IPOFile`."""
          return self.language.getFullEnglishName(self.variant)
++    def makeTranslatableMessage(self, potmsgset):
++        """See `IPOFile`."""
++        return TranslatableMessage(potmsgset, self)
++
  class POFile(SQLBase, POFileMixIn):
      implements(IPOFile)