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

Hi Adi.

This is a good start. I was please to see a presentation that deemphasised
the action to improve the readability of the page.

The behaviour of these actions is disconcerting. When I edit a a template,
the next_url is the template, but I expected to return to the all templates
view. I know that this behaviour has not changed in this branch. I do not
see any bugs that pertain to this surprise--maybe I do not know what to
search on. Here is an example of how to negotiate multiple origins:

    @property
    def next_url(self):
        referrer = self.request.getHeader('referer')
        if (referrer is not None
            and referrer.startswith(self.request.getApplicationURL())):
            return referrer
        else:
            return canonical_url(self.context)

We should discuss if this is in scope. If not this should be a bug reported
against translations.

See below for some 3.0 style concerns.

=== modified file 'lib/lp/translations/templates/object-templates.pt'
--- lib/lp/translations/templates/object-templates.pt 2009-12-28 22:58:18 +0000
+++ lib/lp/translations/templates/object-templates.pt 2010-02-10 13:48:35 +0000
...

+ <td class="actions_column"
+ tal:condition="context/required:launchpad.AnyPerson">
+ <div class="template_links">
+ <tal:maintainer condition="template/required:launchpad.Edit">
+ <a tal:attributes="href string:${template/fmt:url}/+edit;
+ title string:Edit ${template/name}'s details">
+ <img src="/@@/edit" />&nbsp;Edit</a>
+ <a tal:attributes="href string:${template/fmt:url}/+upload;
+ title string:Upload translations to ${template/name}">
+ <img src="/@@/add" />&nbsp;Upload</a>
+ </tal:maintainer>
+ <a tal:attributes="href string:${template/fmt:url}/+export;
+ title string:Download translations from ${template/name}">
+ <img src="/@@/download" />&nbsp;Download</a>
+ <tal:admin condition="template/required:launchpad.TranslationsAdmin">
+ <a tal:attributes="href string:${template/fmt:url}/+admin;
+ title string:Administer ${template/name}">
+ <img src="/@@/edit" />&nbsp;Administer</a>
+ </tal:admin>

I an uncomfortable with these hand crafted links. We usually need extra tests
to verify that the permission and URL that is in the same as the canonical
definitions used by menus, and render the correct markup.

This rendered markup is not 3.0 style. This should use a sprite, which will
fix the strange underlining issues I see under the icon eg:

  class="sprint edit"

All these links appear to be defined in POTemplateMenu, so I think you can
avoid writing lots of tests by removing this exceptional markup with:

    <a tal:replace="structure template/menu:translations/edit/fmt:link" />

The URL is guaranteed by menu:translations. The markup is guaranteed by
fmt:link. This does permission checking for you too. This will be consistent
with other links...

...Which leads me to ask why do we use "Edit" when the canonical link uses
"Settings"? I am certain they should be the same. If settings is wrong,
should we change the link defined in POTemplateMenu?

...