Merge lp:~jtv/launchpad/templates-listing into lp:launchpad

[Jeroen T. Vermeulen]

Move DistroSeries:+templates out of TAL.

= Bug 608349 =

Even after the phenomenal optimization job Adi and Danilo did on the DistroSeries:+templates lists, Zope and TAL are still taking too much time to make this page render comfortably. In the bug ticket they identify further optimization opportunities: avoiding the URL formatter and replacing the templating engine with a faster one.

In this hobby branch I move the rendering of the template listing rows from the TAL into the browser code. This keeps the TAL cleaner and simpler, takes the TAL parser out of the critical loop, and allows for more conventional optimization. I hope the new browser code in its own way exposes the structure more clearly than the TAL with its conditionals and irregularly-sized columns did.

On my local machine, this speeds up an Ubuntu-sized +templates page by 2—3×.

I did keep the central loop in TAL. Growing one huge string for potentially thousands of templates could get quite unwieldy; the templating engine will hopefully do things in a more "streaming" fashion. With any luck the listing's HTML can start flowing to the client before rendering has completed.

Two "conventional optimizations" are noteworthy:
 * The view classes can (and do) provide their own, faster alternatives to canonical_url.
 * Using self.user instead of context/required:launchpad.AnyPerson avoids the Zope security infrastructure.

By far most of the new code is actually in a new unit-test for the views. The browser-code approach allows for much more extensive and detailed testing. In particular of course the test verifies that the optimized URL generation code is equivalent to canonical_url. It actually produces shorter, relative URLs; this is the only semantic change and it affected two of our existing test stories in a very minor way. The rest of the HTML also got about 40% shorter (or 50% in number of lines), and I think easier to read.

Jeroen

[Robert Collins (lifeless)]

This isn't a review, but I'm curious how you've established that this
is faster (so that I can have a look in the same way at
ubuntu/+assignments. Also, we have a new template engine in the
pipeline :- we probably don't want a lot of procedural code doing
template-work (or we want all our code doing that).

Uhm for generating big texts in python, do this:
buffer = []
for <blah>:
   buffer.append(somestring)
...
return ''.join(buffer)

This isn't as efficient as a pure chunking end-to-end implementation,
but it is the next best thing.

Lastly, you might like to nab Gary or someone from foundations to talk
about the implications of canonical_url returning relative paths :
we've got some stuff changing in that area just recently.

-Rob

[Jeroen T. Vermeulen (jtv)]

> This isn't a review, but I'm curious how you've established that this
> is faster

I haven't actually established that it is faster; I would like to do that based on timings on actual production data on one of the test servers. In practice it's not so much the speed of producing these listings that we care about, as it is the speed of producing specifically the Ubuntu listings.

> Also, we have a new template engine in the
> pipeline :- we probably don't want a lot of procedural code doing
> template-work (or we want all our code doing that).

This is a bit of a special case given the enormous amounts of data that show up in the page. (There are good reasons for doing it this way, so "just batch it" is a poor plan B).

If the new template engine turns out to be faster than straight-through python code, then we should probably scuttle this branch.

> Uhm for generating big texts in python, do this:
> buffer = []
> for <blah>:
> buffer.append(somestring)
> ...
> return ''.join(buffer)
>
> This isn't as efficient as a pure chunking end-to-end implementation,
> but it is the next best thing.

I do that for smaller chunks. For the page as a whole however I think the templating engine is likely to be better tuned. With literally thousands of lines on a page, the ability to start sending HTML before the full list has been rendered could be a real plus.

> Lastly, you might like to nab Gary or someone from foundations to talk
> about the implications of canonical_url returning relative paths :
> we've got some stuff changing in that area just recently.

Something I didn't make clear in the cover letter: it's only a very specific use-case of canonical_url, on one kind of page and for one kind of object that it replaces. The tests should expose any changes that break the equivalence to canonical_url.

Jeroen

[Robert Collins (lifeless)]

On Sat, Sep 11, 2010 at 10:37 PM, Jeroen T. Vermeulen <email address hidden> wrote:
>> This isn't a review, but I'm curious how you've established that this
>> is faster
>
> I haven't actually established that it is faster; I would like to do that based on timings on actual production data on one of the test servers.  In practice it's not so much the speed of producing these listings that we care about, as it is the speed of producing specifically the Ubuntu listings.

Staging is currently not suitable for perf testing except under quite
controlled circumstances (see my bug on foundations - 'staging gets
overloaded') - just a heads up. If I was doing this I think I'd
compare the page before and after locally using the comment field to
get local timings.

>> Also, we have a new template engine in the
>> pipeline :- we probably don't want a lot of procedural code doing
>> template-work (or we want all our code doing that).
>
> This is a bit of a special case given the enormous amounts of data that show up in the page.  (There are good reasons for doing it this way, so "just batch it" is a poor plan B).
>
> If the new template engine turns out to be faster than straight-through python code, then we should probably scuttle this branch.

I don't really like batching most of the time either. As for template
being faster - I wouldn't necessarily expect that, but the tradeoff
between understandability and performance might be less skewed with
chameleon. You could try it - its in the tree now.

>> This isn't as efficient as a pure chunking end-to-end implementation,
>> but it is the next best thing.
>
> I do that for smaller chunks.  For the page as a whole however I think the templating engine is likely to be better tuned.  With literally thousands of lines on a page, the ability to start sending HTML before the full list has been rendered could be a real plus.

With our current design we simply cannot do that:
 - any exception before publication finishes generates an OOPS
 - so no data is sent until we know we won't have an OOPS

-> Don't worry about incremental performance here, worry about total
performance.

-Rob

[Jeroen T. Vermeulen (jtv)]

I don't know what you mean with the part about the comment field. Perhaps you're thinking of my Librarian branch that deals with discrepancies between the choices of default or slave stores?

Anyway, the reason I'm not testing locally is memory. If I create the thousands of packages and templates I need for a realistic test, the differences in caching characteristics between my local system and a production system would probably invalidate the results.

[Robert Collins (lifeless)]

On Sun, Sep 12, 2010 at 4:45 PM, Jeroen T. Vermeulen <email address hidden> wrote:
> I don't know what you mean with the part about the comment field.  Perhaps you're thinking of my Librarian branch that deals with discrepancies between the choices of default or slave stores?

No. View the source of a page - at the bottom there is a comment
listing some stats, including the time to render.

> Anyway, the reason I'm not testing locally is memory.  If I create the thousands of packages and templates I need for a realistic test, the differences in caching characteristics between my local system and a production system would probably invalidate the results.

The theory is that everything is fast except for rendering: adding
thousands of templates on on package should be fine : even at 1K per
row, that would be MB's, not 100's of MB's of increased:
 - pgsql buffer
 - storm buffer
 - objects in memory.

As long as you don't drive yourself into swap you'll be able to tell,
with a much lower turnaround time than reproducing on the actual
dataset.

If you aren't in swap, and this branch is a faster - say 15%
consistently faster - then its a pretty good sign.

-Rob

[Jeroen T. Vermeulen (jtv)]

Okay, I tried it locally. I hadn't thought of the HTML comment as a "field" before, hence my confusion.

At any rate, using "wget" to eliminate as many extraneous factors as I could, the code I present here was more than twice as fast as the code we have in devel for about 1,400 templates. When I increased the number of templates by another 1,000, my version came closer to 3× as fast as devel. I think that's worthwhile.

[Jeroen T. Vermeulen (jtv)]

I just realized I should have posted this as a reply, not a separate comment. In a nutshell: two to three times faster, gaining more with larger data sets; and 40% smaller output (though I didn't particularly aim for it, just something nicer to read) with less than half the number of lines.

I do agree that in principle we should use the TAL. In this case however, with the variable columns list, it was a small step from (pre) moving into the view all cyclomatic complexity that shouldn't have been in the TAL in the first place, to (post) rendering entire rows there.

[Aaron Bentley (abentley)]

You haven't tested with chameleon, but you've said that even if switching to chameleon gives us tech debt, this approach gets rid of some other tech debt, and I'm happy with that.

It does suggest that we should put some work into speeding up canonical_url-- perhaps we really want "canonical_urls".

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks. I wouldn't be surprised if after this, we could get yet another speedup from having some cleverly optimized bulk version of canonical_url.

=== modified file 'lib/lp/translations/browser/distroseries.py'
--- lib/lp/translations/browser/distroseries.py	2010-08-24 10:45:57 +0000
+++ lib/lp/translations/browser/distroseries.py	2010-09-13 16:27:49 +0000
@@ -178,14 +178,16 @@
         self.request.response.addInfoNotification(
             'Your changes have been applied.')
         self.next_url = canonical_url(
-            self.context, rootsite='translations', view_name='+language-packs')
+            self.context, rootsite='translations',
+            view_name='+language-packs')
 
     @action("Request", condition=is_langpack_admin)
     def request_action(self, action, data):
         self.updateContextFromData(data)
         self._request_full_export()
         self.next_url = canonical_url(
-            self.context, rootsite='translations', view_name='+language-packs')
+            self.context, rootsite='translations',
+            view_name='+language-packs')
 
 
 class DistroSeriesTemplatesView(BaseSeriesTemplatesView):
@@ -195,6 +197,11 @@
         super(DistroSeriesTemplatesView, self).initialize(
             series=self.context, is_distroseries=True)
 
+    def constructTemplateURL(self, template):
+        """See `BaseSeriesTemplatesView`."""
+        return '+source/%s/+pots/%s' % (
+            template.sourcepackagename.name, template.name)
+
 
 class DistroSeriesView(LaunchpadView, TranslationsMixin):
 

=== modified file 'lib/lp/translations/browser/potemplate.py'
--- lib/lp/translations/browser/potemplate.py	2010-09-03 13:29:58 +0000
+++ lib/lp/translations/browser/potemplate.py	2010-09-13 16:27:49 +0000
@@ -60,6 +60,7 @@
     )
 from canonical.launchpad.webapp.launchpadform import ReturnToReferrerMixin
 from canonical.launchpad.webapp.menu import structured
+from canonical.launchpad.webapp.tales import DateTimeFormatterAPI
 from canonical.lazr.utils import smartquote
 from lp.app.enums import service_uses_launchpad
 from lp.app.errors import NotFoundError
@@ -838,6 +839,8 @@
         self.can_edit = (
             self.can_admin or check_permission('launchpad.Edit', series))
 
+        self.user_is_logged_in = (self.user is not None)
+
     def iter_templates(self):
         potemplateset = getUtility(IPOTemplateSet)
         return potemplateset.getSubset(
@@ -850,3 +853,142 @@
             return "active-template"
         else:
             return "inactive-template"
+
+    def _renderSourcePackage(self, template):
+        """Render the `SourcePackageName` for `template`."""
+        if self.is_distroseries:
+            return cgi.escape(template.sourcepackagename.name)
+        else:
+            return None
+
+    def _renderTemplateLink(self, template, url):
+        """Render a link to `template`.
+
+        :param template: The target `POTemplate`.
+        :param url: The cached URL for `template`.
+        :return: HTML for a link to `template`.
+        """
+        text = '<a href="%s">%s</a>' % (url, cgi.escape(template.name))
+        if not template.iscurrent:
+            text += ' (inactive)'
+        return text
+
+    def _renderLastUpdateDate(self, template):
+        """Render a template's "last updated" column."""
+        formatter = DateTimeFormatterAPI(template.date_last_updated)
+        full_time = formatter.datetime()
+        date = formatter.approximatedate()
+        return ''.join([
+            '<span class="sortkey">%s</span>' % full_time,
+            '<span class="lastupdate_column" title="%s">%s</span>' % (
+                full_time, date),
+            ])
+
+    def _renderAction(self, base_url, name, path, sprite, enabled):
+        """Render an action for the "actions" column.
+
+        :param base_url: The cached URL for `template`.
+        :param name: Action name for display in the UI.
+        :param path: Path suffix for the action (relative to `base_url`).
+        :param sprite: Sprite class for the action.
+        :param enabled: Show this action?  If not, return empty string.
+        :return: HTML for the contents of the "actions" column.
+        """
+        if not enabled:
+            return ''
+
+        parameters = {
+            'base_url': base_url,
+            'name': name,
+            'path': path,
+            'sprite': sprite,
+        }
+        return (
+            '<a class="sprite %(sprite)s" href="%(base_url)s/%(path)s">'
+            '%(name)s'
+            '</a>') % parameters
+
+    def _renderActionsColumn(self, template, base_url):
+        """Render a template's "actions" column."""
+        if not self.user_is_logged_in:
+            return None
+
+        actions = [
+            ('Edit', '+edit', 'edit', self.can_edit),
+            ('Upload', '+upload', 'add', self.can_edit),
+            ('Download', '+export', 'download', self.user_is_logged_in),
+            ('Administer', '+admin', 'edit', self.can_admin),
+        ]
+        links = [
+            self._renderAction(base_url, *action) for action in actions]
+        html = '<div class="template_links">\n%s</div>'
+        return html % '\n'.join(links)
+
+    def _renderField(self, column_class, content, tag='td'):
+        """Create a table field of the given class and contents.
+
+        :param column_class: CSS class for this column.
+        :param content: HTML to go into the column.  If None, the field
+            will be omitted entirely.  (To produce an empty column, pass
+            the empty string instead.)
+        :param tag: The HTML tag to surround the field in.
+        :return: HTML for the entire table field, or the empty string if
+            `content` was None.
+        """
+        if content is None:
+            return ''
+        else:
+            return '<%s class="%s">%s</%s>' % (
+                tag, column_class, content, tag)
+
+    def constructTemplateURL(self, template):
+        """Build the URL for `template`.
+
+        Since this is performance-critical, views are allowed to
+        override it with optimized implementations.
+        """
+        return canonical_url(template)
+
+    def renderTemplatesHeader(self):
+        """Render HTML for the templates table header."""
+        if self.is_distroseries:
+            sourcepackage_header = "Source package"
+        else:
+            sourcepackage_header = None
+        if self.user_is_logged_in:
+            actions_header = "Actions"
+        else:
+            actions_header = None
+        columns = [
+            ('priority_column', "Priority"),
+            ('sourcepackage_column', sourcepackage_header),
+            ('template_column', "Template name"),
+            ('length_column', "Length"),
+            ('lastupdate_column', "Updated"),
+            ('actions_column', actions_header),
+            ]
+        return '\n'.join([
+            self._renderField(*column, tag='th') for column in columns])
+
+    def renderTemplateRow(self, template):
+        """Render HTML for an entire template row."""
+        if not self.can_edit and not template.iscurrent:
+            return ""
+
+        # Cached URL for template.
+        base_url = self.constructTemplateURL(template)
+
+        fields = [
+            ('priority_column', template.priority),
+            ('sourcepackage_column', self._renderSourcePackage(template)),
+            ('template_column', self._renderTemplateLink(template, base_url)),
+            ('length_column', template.messagecount),
+            ('lastupdate_column', self._renderLastUpdateDate(template)),
+            ('actions_column', self._renderActionsColumn(template, base_url)),
+        ]
+
+        tds = [self._renderField(*field) for field in fields]
+
+        css = self.rowCSSClass(template)
+        return '<tr class="template_row %s">\n%s</tr>\n' % (
+            css, '\n'.join(tds))

=== modified file 'lib/lp/translations/browser/productseries.py'
--- lib/lp/translations/browser/productseries.py	2010-08-24 10:45:57 +0000
+++ lib/lp/translations/browser/productseries.py	2010-09-13 16:27:49 +0000
@@ -250,7 +250,8 @@
                     'administrator in the coming few days.  You can track '
                     'your upload\'s status in the '
                     '<a href="%s/+imports">Translation Import Queue</a>' %(
-                        canonical_url(self.context, rootsite='translations'))))
+                        canonical_url(self.context,
+                        rootsite='translations'))))
 
         elif is_tar_filename(filename):
             # Add the whole tarball to the import queue.
@@ -277,7 +278,8 @@
                     'your upload\'s status in the '
                     '<a href="%s/+imports">Translation Import Queue</a>' %(
                         num, plural_s, plural_s, itthey,
-                        canonical_url(self.context, rootsite='translations'))))
+                        canonical_url(self.context,
+                        rootsite='translations'))))
                 if len(conflicts) > 0:
                     if len(conflicts) == 1:
                         warning = (
@@ -492,6 +494,10 @@
         super(ProductSeriesTemplatesView, self).initialize(
             series=self.context, is_distroseries=False)
 
+    def constructTemplateURL(self, template):
+        """See `BaseSeriesTemplatesView`."""
+        return '+pots/%s' % template.name
+
 
 class LinkTranslationsBranchView(LaunchpadEditFormView):
     """View to set the series' translations export branch."""

=== added file 'lib/lp/translations/browser/tests/test_seriestemplatesview.py'
--- lib/lp/translations/browser/tests/test_seriestemplatesview.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/browser/tests/test_seriestemplatesview.py	2010-09-13 16:27:49 +0000
@@ -0,0 +1,332 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `BaseSeriesTemplatesView` and descendants."""
+
+__metaclass__ = type
+
+import re
+
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.ftests import login
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing import DatabaseFunctionalLayer
+from lp.registry.model.distroseries import DistroSeries
+from lp.registry.model.productseries import ProductSeries
+from lp.translations.browser.distroseries import DistroSeriesTemplatesView
+from lp.translations.browser.productseries import ProductSeriesTemplatesView
+from lp.testing import (
+    login_person,
+    TestCaseWithFactory,
+    )
+from lp.testing.sampledata import ADMIN_EMAIL
+
+
+class SeriesTemplatesViewScenario:
+
+    # The view class to test.
+    view_class = None
+
+    # The expected list of columns for the view_class, each shown as a
+    # list holding the column's CSS class.
+    columns = None
+
+    def makeTemplateContext(self):
+        """Create makePOTemplate arguments to create a series."""
+        raise NotImplementedError()
+
+    def _makeTemplate(self, **kwargs):
+        """Create a distro- or productseries for the view."""
+        args_dict = self.makeTemplateContext()
+        args_dict.update(kwargs)
+        return self.factory.makePOTemplate(**args_dict)
+
+    def _getSeries(self, template):
+        """Get `template`'s distro- or productseries."""
+        return template.distroseries or template.productseries
+
+    def _makeView(self, template=None):
+        """Create a `BaseTemplatesView` containing `template`."""
+        if template is None:
+            template = self._makeTemplate()
+        request = LaunchpadTestRequest()
+        view = self.view_class(self._getSeries(template), request)
+        view.initialize()
+        return view
+
+    def _findTagClasses(self, html, tag):
+        """Find the CSS classes for all instances of `tag`s in `html`.
+
+        Returns a list of lists.  The outer list represents instances of
+        `tag`, in the order in which they are found.  The inner lists
+        hold the respective sets of HTML classes for these tags, sorted
+        alphabetically.
+        """
+        regex = '<%s [^>]*class="([^"]*)"' % tag
+        return [
+            sorted(css_class.split())
+            for css_class in re.findall(regex, html)]
+
+    def _findActions(self, html):
+        """Find the available actions in an HTML actions column."""
+        return re.findall('<[^>]*>([^<]*)</[^>]*', html)
+
+    def test_has_the_right_columns(self):
+        # Test the column headers against the expected list.
+        view = self._makeView()
+        header = view.renderTemplatesHeader()
+        self.assertEqual(self.columns, self._findTagClasses(header, 'th'))
+
+    def test_logging_in_adds_actions_column(self):
+        # A logged-in user gets to see an extra "actions" column.
+        template = self._makeTemplate()
+        login_person(self.factory.makePerson())
+        view = self._makeView(template)
+        columns = self.columns + [['actions_column']]
+        header = view.renderTemplatesHeader()
+        self.assertEqual(columns, self._findTagClasses(header, 'th'))
+        row = view.renderTemplateRow(template)
+        self.assertEqual(columns, self._findTagClasses(row, 'td'))
+
+    def test_user_actions(self):
+        # The only action offered to regular users is Download.
+        template = self._makeTemplate()
+        url = canonical_url(template)
+        login_person(self.factory.makePerson())
+        view = self._makeView(template)
+
+        self.assertEqual(
+            ['Download'],
+            self._findActions(view._renderActionsColumn(template, url)))
+
+    def test_admin_actions(self):
+        # An administrator gets to see all actions on a template.
+        template = self._makeTemplate()
+        url = canonical_url(template)
+        login(ADMIN_EMAIL)
+        view = self._makeView(template)
+
+        self.assertEqual(
+            ['Edit', 'Upload', 'Download', 'Administer'],
+            self._findActions(view._renderActionsColumn(template, url)))
+
+    def test_edit_actions(self):
+        # A non-admin user with edit rights gets the Edit, Upload, and
+        # Download actions.
+        template = self._makeTemplate()
+        url = canonical_url(template)
+        login_person(self.factory.makePerson())
+        view = self._makeView(template)
+        view.can_edit = True
+
+        self.assertEqual(
+            ['Edit', 'Upload', 'Download'],
+            self._findActions(view._renderActionsColumn(template, url)))
+
+    def test_constructs_correct_urls(self):
+        # The view classes can override constructTemplateURL with
+        # optimized versions.  These can produce either an absolute URL
+        # that exactly matches the template's canonical_url, or a
+        # relative one starting from the series' canonical_url.
+        template = self._makeTemplate()
+        view = self._makeView(template)
+
+        series_url = canonical_url(
+            self._getSeries(template), rootsite='translations')
+        constructed_url = view.constructTemplateURL(template)
+
+        self.assertIn(
+            canonical_url(template),
+            (constructed_url, '/'.join([series_url, constructed_url])))
+
+    def test_renderTemplateLink(self):
+        # _renderTemplateLink renders a link to the template.
+        template = self._makeTemplate()
+        view = self._makeView(template)
+
+        url = view.constructTemplateURL(template)
+        link = view._renderTemplateLink(template, url)
+
+        self.assertIn('<a ', link)
+        self.assertIn('href="%s"' % url, link)
+        self.assertIn('>%s<' % template.name, link)
+
+    def test_renderTemplateLink_marks_disabled(self):
+        # _renderTemplateLinks marks disabled templates as "(inactive)."
+        template = self._makeTemplate()
+        view = self._makeView(template)
+        url = canonical_url(template)
+
+        removeSecurityProxy(template).iscurrent = True
+        self.assertNotIn(
+            '(inactive)', view._renderTemplateLink(template, url))
+        removeSecurityProxy(template).iscurrent = False
+        self.assertIn('(inactive)', view._renderTemplateLink(template, url))
+
+    def test_renderLastUpdateDate_sets_sortkey(self):
+        # _renderLastUpdateDate sets the full date as the column's sort
+        # key, so that clicking on the column header sorts by date (even
+        # if sorting alphabetically by the visible date might produce a
+        # different ordering).
+        template = self._makeTemplate()
+        view = self._makeView(template)
+
+        date_field = view._renderLastUpdateDate(template)
+
+        # The sort key is set in a span of class "sortkey."
+        sortkey_match = re.findall(
+            '<span class="sortkey">([^<]*)</span>', date_field)
+        self.assertIsNot(None, sortkey_match)
+        self.assertEqual(1, len(sortkey_match))
+
+        # The column also has the same full date as a tooltip.
+        full_date = sortkey_match[0].strip()
+        self.assertIn('title="%s"' % full_date, date_field)
+
+    def test_renderAction_returns_empty_string_if_not_enabled(self):
+        view = self._makeView()
+        self.assertEqual(
+            '',
+            view._renderAction('url', 'name', 'path', 'sprite', False))
+
+    def test_renderAction(self):
+        # If enabled, _renderAction produces a link to an action form
+        # for a given template.
+        view = self._makeView()
+
+        url = self.factory.getUniqueString()
+        name = self.factory.getUniqueString()
+        path = self.factory.getUniqueString()
+        sprite = self.factory.getUniqueString()
+
+        action = view._renderAction(url, name, path, sprite, True)
+
+        self.assertIn('<a ', action)
+        self.assertIn('href="%s/%s"' % (url, path), action)
+        self.assertIn(name, action)
+        self.assertIn('class="sprite %s"' % sprite, action)
+
+    def test_renderField_returns_empty_string_if_no_content(self):
+        view = self._makeView()
+        self.assertEqual('', view._renderField('x', None, tag='y'))
+
+    def test_renderField_returns_empty_field_for_empty_content(self):
+        field = self._makeView()._renderField('class', '', tag='tag')
+        self.assertIn('<tag class="class">', field)
+        self.assertIn('</tag>', field)
+
+    def test_renderField(self):
+        column_class = self.factory.getUniqueString()
+        content = self.factory.getUniqueString()
+        tag = self.factory.getUniqueString()
+
+        field = self._makeView()._renderField(column_class, content, tag=tag)
+
+        self.assertIn('<%s class="%s">' % (tag, column_class), field)
+        self.assertIn(content, field)
+        self.assertIn('</%s>' % tag, field)
+
+    def test_renderTemplateRow(self):
+        template = self._makeTemplate()
+        view = self._makeView(template)
+
+        row = view.renderTemplateRow(template)
+
+        self.assertEqual(
+            [sorted(['template_row', view.rowCSSClass(template)])],
+            self._findTagClasses(row, 'tr'))
+
+        self.assertEqual(self.columns, self._findTagClasses(row, 'td'))
+
+
+class TestDistroSeriesTemplatesView(SeriesTemplatesViewScenario,
+                                    TestCaseWithFactory):
+    """Run the test scenario against `DistroSeriesTemplatesView`."""
+
+    layer = DatabaseFunctionalLayer
+
+    view_class = DistroSeriesTemplatesView
+
+    columns = [
+        ['priority_column'],
+        ['sourcepackage_column'],
+        ['template_column'],
+        ['length_column'],
+        ['lastupdate_column'],
+    ]
+
+    def makeTemplateContext(self):
+        """See `SeriesTemplatesViewScenario`."""
+        return dict(
+            sourcepackagename=self.factory.makeSourcePackageName(),
+            distroseries=self.factory.makeDistroSeries())
+
+    def test_makeTemplate(self):
+        # In this test case, _makeTemplate produces a distroseries
+        # template.
+        template = self._makeTemplate()
+        self.assertIsInstance(template.distroseries, DistroSeries)
+        self.assertIs(None, template.productseries)
+
+    def test_findTagClasses(self):
+        # Tested here arbitrarily (no need to repeat it): the
+        # _findTagClasses helper.
+        self.assertEqual(
+            [['b', 'c'], ['a']],
+            self._findTagClasses('<x class="c b" /><x class="a">', 'x'))
+
+    def test_findActions(self):
+        # Tested here arbitrarily (no need to repeat it): the
+        # _findActions helper.
+        self.assertEqual(['Foo'], self._findActions('<a class="bar">Foo</a>'))
+
+    def test_is_distroseries(self):
+        self.assertTrue(self._makeView().is_distroseries)
+
+    def test_renderSourcePackage(self):
+        # _renderSourcePackage returns the template's source-package
+        # name for a productseries view.
+        template = self._makeTemplate()
+        view = self._makeView(template)
+
+        return self.assertEqual(
+            template.sourcepackagename.name,
+            view._renderSourcePackage(template))
+
+
+class TestProductSeriesTemplatesView(SeriesTemplatesViewScenario,
+                                     TestCaseWithFactory):
+    """Run the test scenario against `ProductSeriesTemplatesView`."""
+
+    layer = DatabaseFunctionalLayer
+
+    view_class = ProductSeriesTemplatesView
+
+    columns = [
+        ['priority_column'],
+        ['template_column'],
+        ['length_column'],
+        ['lastupdate_column'],
+    ]
+
+    def makeTemplateContext(self):
+        """See `SeriesTemplatesViewScenario`."""
+        return dict(productseries=self.factory.makeProductSeries())
+
+    def test_makeTemplate(self):
+        # In this test case, _makeTemplate produces a productseries
+        # template.
+        template = self._makeTemplate()
+        self.assertIs(None, template.distroseries)
+        self.assertIsInstance(template.productseries, ProductSeries)
+
+    def test_is_distroseries(self):
+        self.assertFalse(self._makeView().is_distroseries)
+
+    def test_renderSourcePackage(self):
+        # _renderSourcePackage returns None for a productseries view.
+        template = self._makeTemplate()
+        view = self._makeView(template)
+        self.assertIs(None, view._renderSourcePackage(template))

=== modified file 'lib/lp/translations/stories/standalone/xx-potemplate-edit.txt'
--- lib/lp/translations/stories/standalone/xx-potemplate-edit.txt	2010-04-22 17:07:56 +0000
+++ lib/lp/translations/stories/standalone/xx-potemplate-edit.txt	2010-09-13 16:27:49 +0000
@@ -183,9 +183,7 @@
 
   >>> referrer = 'http://translations.launchpad.dev/evolution/trunk/+templates'
   >>> admin_browser.open(referrer)
-  >>> admin_browser.getLink(
-  ...   url='/evolution/trunk/+pots/'
-  ...       'evolution-2.2/+edit').click()
+  >>> admin_browser.getLink(url='+pots/evolution-2.2/+edit').click()
   >>> admin_browser.getControl('Change').click()
   >>> admin_browser.url == referrer
   True

=== modified file 'lib/lp/translations/stories/standalone/xx-series-templates.txt'
--- lib/lp/translations/stories/standalone/xx-series-templates.txt	2010-07-15 13:57:23 +0000
+++ lib/lp/translations/stories/standalone/xx-series-templates.txt	2010-09-13 16:27:49 +0000
@@ -85,16 +85,14 @@
     >>> utc_browser.open(
     ...     'http://translations.launchpad.dev/ubuntu/hoary/+templates')
     >>> utc_browser.getLink(
-    ...     url='ubuntu/hoary/+source/evolution/'
-    ...         '+pots/evolution-2.2/+edit').click()
+    ...     url='+source/evolution/+pots/evolution-2.2/+edit').click()
     >>> print utc_browser.url
     http://.../ubuntu/hoary/+source/evolution/+pots/evolution-2.2/+edit
 
     >>> utc_browser.open(
     ...     'http://translations.launchpad.dev/ubuntu/hoary/+templates')
     >>> utc_browser.getLink(
-    ...     url='/ubuntu/hoary/+source/evolution/'
-    ...         '+pots/evolution-2.2/+admin').click()
+    ...     url='+source/evolution/+pots/evolution-2.2/+admin').click()
     >>> print utc_browser.url
     http://.../ubuntu/hoary/+source/evolution/+pots/evolution-2.2/+admin
 
@@ -104,16 +102,14 @@
     >>> utc_browser.open(
     ...     'http://translations.launchpad.dev/ubuntu/hoary/+templates')
     >>> utc_browser.getLink(
-    ...     url='ubuntu/hoary/+source/evolution/'
-    ...         '+pots/disabled-template/+edit').click()
+    ...     url='+source/evolution/+pots/disabled-template/+edit').click()
     >>> print utc_browser.url
     http://.../ubuntu/hoary/+source/evolution/+pots/disabled-template/+edit
 
     >>> utc_browser.open(
     ...     'http://translations.launchpad.dev/ubuntu/hoary/+templates')
     >>> utc_browser.getLink(
-    ...     url='/ubuntu/hoary/+source/evolution/'
-    ...         '+pots/disabled-template/+admin').click()
+    ...     url='+source/evolution/+pots/disabled-template/+admin').click()
     >>> print utc_browser.url
     http://.../ubuntu/hoary/+source/evolution/+pots/disabled-template/+admin
 

=== modified file 'lib/lp/translations/templates/object-templates.pt'
--- lib/lp/translations/templates/object-templates.pt	2010-07-15 09:41:27 +0000
+++ lib/lp/translations/templates/object-templates.pt	2010-09-13 16:27:49 +0000
@@ -73,80 +73,13 @@
       <div class="yui-b">
       <table class="sortable listing" id="templates_table">
         <thead>
-          <tr>
-            <th class="priority_column">Priority</th>
-            <th tal:condition="view/is_distroseries"
-                class="sourcepackage_column">Source package</th>
-            <th class="template_column">Template name</th>
-            <th class="length_column">Length</th>
-            <th class="lastupdate_column">Updated</th>
-            <th class="actions_column"
-                tal:condition="context/required:launchpad.AnyPerson">
-              Actions</th>
+          <tr tal:content="structure view/renderTemplatesHeader">
           </tr>
         </thead>
         <tbody>
-          <tal:templates repeat="template view/iter_templates">
-            <tr tal:define="
-                  inactive_css_class python:view.rowCSSClass(template);
-                  template_url template/fmt:url"
-                tal:condition="python: view.can_edit or template.iscurrent"
-                tal:attributes="
-                  class string: template_row ${inactive_css_class}">
-              <td class="priority_column"
-                  tal:content="template/priority">7</td>
-              <td tal:condition="view/is_distroseries"
-                  tal:content="template/sourcepackagename/name"
-                  class="sourcepackage_column">Source package
-              </td>
-              <td class="template_column">
-                <a tal:attributes="href template_url">
-                  <span tal:content="template/name">Template name</span>
-                </a>
-                <tal:inactive condition="not: template/iscurrent">
-                  (inactive)
-                </tal:inactive>
-              </td>
-              <td class="length_column"
-                  tal:content="template/messagecount">1777</td>
-              <td class="lastupdate_column">
-                <span class="sortkey"
-                      tal:condition="template/date_last_updated"
-                      tal:content="template/date_last_updated/fmt:datetime">
-                      time sort key
-                </span>
-                <span class="lastupdate_column"
-                      tal:condition="template/date_last_updated"
-                      tal:attributes="
-                        title template/date_last_updated/fmt:datetime"
-                      tal:content="
-                        template/date_last_updated/fmt:approximatedate"
-                >
-                   2009-09-23
-                </span>
-              </td>
-              <td class="actions_column"
-                  tal:condition="context/required:launchpad.AnyPerson">
-              <div class="template_links">
-                <a class="sprite edit"
-                    tal:condition="view/can_edit"
-                    tal:attributes="
-                        href string: ${template_url}/+edit">Edit</a>
-                <a class="sprite add"
-                    tal:condition="view/can_edit"
-                    tal:attributes="
-                        href string: ${template_url}/+upload">Upload</a>
-                <a class="sprite download"
-                    tal:attributes="
-                        href string: ${template_url}/+export">Download</a>
-                <a class="sprite edit"
-                    tal:condition="view/can_admin"
-                    tal:attributes="
-                        href string: ${template_url}/+admin">Administer</a>
-              </div>
-              </td>
-            </tr>
-          </tal:templates>
+          <tal:templates
+              repeat="template view/iter_templates"
+              content="structure python:view.renderTemplateRow(template)"/>
         </tbody>
       </table>
       </div>