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

Hi Adi,

Thanks for working on this: it's a very welcome change! You've done a
great job here. I'll have some stylistic comments but don't let that
turn you off: it's just to better cope with development in a large team
such as Launchpad's.

I'd like this page (+templates) to become more of a maintainer-orienteed
starting point: a place they can go to look at stuff like disabled
templates and such. This branch is a step in the right direction.

 review approve code
 status approve

Let me know when you've fixed the minor points I note here so I can land
this for you (though, note that we are in "week 4", which means that our
development trees are closed for anything but "release critical"
landings).

У чет, 26. 11 2009. у 00:44 +0000, Adi Roiban пише:
> = Demo and Q/A =
>
> You should access the +templates pages for both a producseries and a distroseries.
> The table should also display the „Last update” column

As I mentioned earlier, it's useful to get direct links to pages which
you are modifying on launchpad.dev and on eg. edge.launchpad.net: while
I do know where to find them, others might need to spend more time to do
it.

> === modified file 'lib/lp/translations/stories/distroseries/xx-distroseries-templates.txt'
> --- lib/lp/translations/stories/distroseries/xx-distroseries-templates.txt 2009-10-30 10:09:17 +0000
> +++ lib/lp/translations/stories/distroseries/xx-distroseries-templates.txt 2009-11-25 17:37:01 +0000
> @@ -18,6 +18,44 @@
>
> == The templates table ==
>
> +Anonymous users will see a link from distro series

I'd rather make this something like:

"Full template listing for a distribution series is reached by following
a link from the distribution series translations page."

The idea is to make narrative in tests more like statements and like an
actual user story. In this particular case, using anonymous users just
demonstrates that *everybody* can reach this page.

> + >>> anon_browser.open(
> + ... 'http://translations.launchpad.dev/ubuntu/hoary')
> + >>> anon_browser.getLink('full list of templates').click()
> +
> +The anon page shows a table with only source package, template name and last
> +update.

And this should probably be something like:

"Full listing of templates shows source package name, template name and
the date of last update for this distribution series."

> +
> + >>> table = find_tag_by_id(anon_browser.contents, 'templates_table')
> + >>> print extract_text(table)
> + Source package Template name Last update
> + evolution disabled-template 2007-01-05
> + evolution evolution-2.2 2005-05-06
> + evolution man 2006-08-14
> + mozilla pkgconf-mozilla 2005-05-06
> + pmount man 2006-08-14
> + pmount pmount 2005-05-06
> +
> +
> +Normal users will see a link from distro series

This is relatively unneeded. The only thing you want to demonstrate
with this test is that Download links are shown. You can cut down on
the narrative, and use something as simple as:

"Logged-in users can also choose to download all translations for each
of the templates."

followed by the entire test. You can even keep it shorter (and more
readable) by including only one or two lines of the test. I.e.

    >>> user_browser.open(
    ... 'http://translations.launchpad.dev/ubuntu/hoary')
    >>> user_browser.getLink('full list of templates').click()
    >>> table = find_tag_by_id(user_browser.contents, 'templates_table')
    >>> print extract_text(table)
    Source package Template name Last update Actions
    evolution disabled-template 2007-01-05 Download
    evolution evolution-2.2 2005-05-06 Download
    ...

I think this would transfer the point much better. What do you think?

(You can use "..." to match any following text, and that's something we
really like to use :)

> === modified file 'lib/lp/translations/stories/productseries/xx-productseries-templates.txt'
> --- lib/lp/translations/stories/productseries/xx-productseries-templates.txt 2009-10-30 10:09:17 +0000
> +++ lib/lp/translations/stories/productseries/xx-productseries-templates.txt 2009-11-26 00:39:19 +0000
> @@ -6,8 +6,8 @@
>
> == Preparation ==
>
> -To test the ordering of templates in the listing, we need another template
> -that is new but must appear at the top of the list.
> +To test the ordering of templates in the listing, we need another
> +template that is new but must appear at the top of the list.
>
> >>> login('<email address hidden>')
> >>> from zope.component import getUtility
> @@ -18,6 +18,7 @@
> ... name='at-the-top')
> >>> logout()
>
> +

This entire change seems unneeded. We are generally very considerate
about the spacing, but you usually want to keep changes like these to a
minimum because it increases the diff size and makes it harder for a
reviewer to pick out the important areas to pay really close attention
to.

Rest of the changes to this file are good :)

> === modified file 'lib/lp/translations/templates/object-templates.pt'
> --- lib/lp/translations/templates/object-templates.pt 2009-10-22 11:49:30 +0000
> +++ lib/lp/translations/templates/object-templates.pt 2009-11-25 17:34:16 +0000
> @@ -75,6 +75,7 @@
> <th tal:condition="view/is_distroseries"
> class="sourcepackage_column">Source package</th>
> <th class="template_column">Template name</th>
> + <th class="lastupdate_column">Last update</th>
> <th class="actions_column"
> tal:condition="context/required:launchpad.AnyPerson">
> Actions</th>
> @@ -88,6 +89,22 @@
> </td>
> <td class="template_column"><a tal:attributes="href template/fmt:url"
> tal:content="template/name">Template name</a></td>
> + <td class="lastupdate_column">
> + <span class="sortkey"

You seem to have a TAB here: we strictly use spaces :)

> + 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"
> + >

We don't have strict HTML style rules, but I'd definitely indent this
another two spaces. Though, that's just my personal take, it's fine as
is as well :)

> + 2009-09-23
> + </span>
> + </td>
> <td class="actions_column"
> tal:condition="context/required:launchpad.AnyPerson">
> <div class="template_links">
>
> adi@adi-laptop:~/launchpad/lp-branches/template-last-update$ bzr diff -r 9941 | less

No need to append a diff: it'll be automatically generated.

Again, thanks a lot for working on this!

Cheers,
Danilo