Merge lp:~wallyworld/launchpad/nominations-javascript2-271697 into lp:launchpad

[Ian Booth]

Fix nominations expanders and layout issues.

The link to open the nominations form for each bugtask row was wired to old, deleted javascript. The new expander infrastructure has been wired in. The new expander uses a little triangle icon so the now obsolete brackets around the link have been wrapped in a hide-on-load span so they only show when javascript is not available.

Also fix a table cell colspan issue.

[Curtis Hovey (sinzui)]

This markup is incompatible with the expander. The expander *creates* an <a> node and wraps the label node to create the expanding link. The label node is the first node in the "collapsible" parent, and it is already an anchor. Anchors in anchors are invalid and fail in some browsers. You need to change the first node to a span., of but that markup also looks odd because we have a link in the span...

We are being stupid. This markup has been edited so many times over seven years we forgot what is happening.
1. We do not need a link to the form page, the bloody form is rendered inline.
2. The form should not be hidden, the expander adds the hidden css class when js is successful, and we care about hide-on-load
3. We already have a driver permission check in the outer nodes, we don't need checks in the subnodes.
4. the "lesser" css class that makes the small text in a for we expect to use is madness. the small text is for asides you do not need to do important work with. We can remove that.
5. I think we want to tweak the style, or find a class that does the proper alignment and spacing. The current alignment was probably designed for Lp 1.0.
6. Oh the secondary class is not used with tables anymore

This is a replacement template that removes the cruft. http://pastebin.ubuntu.com/1260024/
I would be nice if the form knew it was embedded, but that is really a seperate issue involving formoverlay.

review: Needs Fixing (code)

[Curtis Hovey (sinzui)]

PS the nomination id is not needed. If a test fails looking for it, it assumes the form was rendered or the JS was hooked up, but that is not the actual case. I would delete any test that checks for the bogus id.

[Curtis Hovey (sinzui)]

"I would be nice" should be "It would be nice ". I am never nice.

[Curtis Hovey (sinzui)]

We talked and agree that if you like the layout, my pastebin is okay to land.

review: Approve (code)

[Ian Booth (wallyworld)]

Thanks for the excellent changes. I have used those, and also added a change of my own. The accept/decline text is not needed for non js browsers since the form is now inline and visible for non js situations.
I also updated a failing doctest.

=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-nomination-table-row.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-nomination-table-row.txt	2011-12-24 17:49:30 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-nomination-table-row.txt	2012-10-04 22:36:24 +0000
@@ -37,12 +37,7 @@
     >>> user_browser.contents
     '...Nominated...for...Hoary...by...No Privileges Person...'
 
-But not the approve/decline link, nor any Approve or Decline buttons.
-
-    >>> user_browser.getLink("approve/decline")
-    Traceback (most recent call last):
-    ...
-    LinkNotFoundError
+But not the Approve or Decline buttons.
 
     >>> user_browser.getControl("Approve")
     Traceback (most recent call last):
@@ -67,8 +62,8 @@
 But if we log in as a user that does have launchpad.Driver permission,
 we will see the approve/decline links.
 
-First, when the task is in Proposed state, the (privileged) user sees an
-approve/decline link.
+First, when the task is in Proposed state, the (privileged) user sees
+Approve and Decline buttons.
 
     >>> login("[e-mail]")
     >>> check_permission("launchpad.Driver", ubuntu_hoary_nomination.target)
@@ -82,12 +77,6 @@
     ...     "http://launchpad.dev/distros/ubuntu/+source/mozilla-firefox/"
     ...     "+bug/1/nominations/2/+bugtasks-and-nominations-table-row")
 
-    >>> browser.getLink("approve/decline")
-    <Link ...>
-
-Clicking the "approve/decline" link causes a simple form to be expanded
-underneath, with "Approve" and "Decline" buttons.
-
     >>> approve_button = browser.getControl("Approve")
     >>> decline_button = browser.getControl("Decline")
 
@@ -101,8 +90,7 @@
     True
     >>> logout()
 
-Now only the "Approve" button shows, and the approve/decline link now
-reads only "approve".
+Now only the "Approve" button shows.
 
     >>> browser = setupBrowser("Basic [e-mail]:test")
     >>> browser.open(
@@ -112,13 +100,6 @@
     >>> browser.contents
     '...Declined...for...Hoary...by...Foo Bar...'
 
-    >>> browser.getLink("approve/decline")
-    Traceback (most recent call last):
-      ...
-    LinkNotFoundError
-    >>> browser.getLink("approve")
-    <Link ...>
-
     >>> approve_button = browser.getControl("Approve")
     >>> browser.getControl("Decline")
     Traceback (most recent call last):

=== modified file 'lib/lp/bugs/templates/bugnomination-tasks-and-nominations-table-row.pt'
--- lib/lp/bugs/templates/bugnomination-tasks-and-nominations-table-row.pt	2012-03-02 16:24:45 +0000
+++ lib/lp/bugs/templates/bugnomination-tasks-and-nominations-table-row.pt	2012-10-04 22:36:24 +0000
@@ -1,38 +1,21 @@
 <tal:nomination
   xmlns:tal="http://xml.zope.org/namespaces/tal"
   tal:define="user_is_driver view/userCanMakeDecisionForNomination;
-              nomination_form_id string:nomination${context/id};
               nomination_person view/getNominationPerson">
-  <tr class="secondary">
-    <td style="padding: 0.3em 0em 0.3em 1.5em" colspan="4">
-    <span class="lesser">
-      <span tal:content="context/status/title">
-        Nominated
-      </span>
-      for
-      <span tal:content="context/target/name/capitalize">
-        Warty
-      </span>
-      by
-      <a tal:replace="structure nomination_person/fmt:link">
-        Foo Bar
-      </a>
-    </span>
-    <tal:links condition="view/displayNominationEditLinks">
-      <tal:release-management condition="user_is_driver">
-          (<a href="#"
-              tal:content="view/getApproveDeclineLinkText"
-              tal:attributes="onclick string:return toggleFormVisibility('${nomination_form_id}');
-                              href view/getNominationEditLink">approve/decline</a>)
-        <div tal:attributes="id nomination_form_id"
-             tal:condition="user_is_driver"
-             class="hidden">
-            <span style="padding-left: 1.5em" tal:content="structure context/@@+edit-form">
-                Approve/Decline
-            </span>
-        </div>
-      </tal:release-management>
-    </tal:links>
+  <tr>
+    <td colspan="6">
+      <div class="subordinate">
+        <span tal:content="context/status/title">Nominated</span> for
+        <span tal:content="context/target/name/capitalize">Warty</span>
+        by <a tal:replace="structure nomination_person/fmt:link">Foo Bar</a>
+        <tal:no-cve condition="view/displayNominationEditLinks">
+          <span class="collapsible" tal:condition="user_is_driver">
+              <span class='hidden' tal:content="view/getApproveDeclineLinkText">approve/decline</span>
+              <div class="hide-on-load"
+                tal:content="structure context/@@+edit-form" />
+          </span>
+        </tal:no-cve>
+    </div>
   </td>
 </tr>
 </tal:nomination>