Code review comment for lp:~jcsackett/launchpad/mergeproposal-status-icon-589584

Nice attention to detail.

I've spotted several problems though, and in the course of reviewing
I've come up with an alternative diff. I hope you don't mind. I've put
it at the end if you're interested.

[1]

+ <a tal:condition="link/enabled"
+ tal:attributes="class string:value mergestatus${context/queue_status/name};
+ href link/url">
+ Work in progress
+ </a>

This will always display "Work in progress"... I think a tal:content
is missing :)

[2]

         <a tal:define="link context/menu:context/edit_status"
- tal:condition="link/enabled"
            tal:attributes="href link/url">
            <img class="editicon" src="/@@/edit" alt="Edit status" />
         </a>

This means that the edit status link will always be visible. It can be
clicked by anonymous users, who are asked to log-in, but if they don't
have permission to edit the proposal status once logged-in I suspect
Launchpad will return a 403 Forbidden.

It's probably safest to just do what link/enabled dictates, and hide
the link if it's False.

[3]

         <a tal:define="link context/menu:context/edit_status"

You could use the link variable defined in tal:merge-status.

[4]

+ <tal:merge-status define="link context/menu:context/edit_status">

You could use the context_menu defined earlier in the template.

My diff:

=== modified file 'lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2010-03-15 20:28:52 +0000
+++ lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2010-12-23 11:24:23 +0000
@@ -17,15 +17,20 @@
     <tr id="summary-row-1-status">
       <th>Status:</th>
       <td id="branchmergeproposal-status-value">
- <span tal:attributes="class string:value mergestatus${context/queue_status/name}"
- tal:content="context/queue_status/title">
- Work in progress
- </span>
- <a tal:define="link context/menu:context/edit_status"
- tal:condition="link/enabled"
- tal:attributes="href link/url">
- <img class="editicon" src="/@@/edit" alt="Edit status" />
- </a>
+ <tal:merge-status define="link context_menu/edit_status">
+ <a tal:omit-tag="not:link/enabled"
+ tal:attributes="href link/url">
+ <span tal:content="context/queue_status/title"
+ tal:define="status_name context/queue_status/name"
+ tal:attributes="class string:value mergestatus${status_name}">
+ Work in progress
+ </span>
+ </a>
+ <a tal:condition="link/enabled"
+ tal:attributes="href link/url">
+ <img class="editicon" src="/@@/edit" alt="Edit status" />
+ </a>
+ </tal:merge-status>
       </td>
     </tr>
     <tal:comment condition="nothing">