Merge lp:~thumper/launchpad/use-last-rev-id into lp:launchpad

[Tim Penhey]

Send through the last scanned id when approving a merge proposal and update the table asynchronously.

[Tim Penhey (thumper)]

Need to find a good way to test this.

[Tim Penhey (thumper)]

Changed the windmill test to use the approved value and check for the approved revision id.

[Paul Hummer (rockstar)]

Think adding a few comments to the insertion code would be helpful. When I look at the patch, I know what the javascript is doing because I see the tal changes as well. Seeing the javascript by itself might be a little confusing (it is a bit hairy, but clever).

review: Approve (code js ui*)

[Tim Penhey (thumper)]

http://people.canonical.com/~tim/magic.ogv contains a movie showing the changes.

[Michael Nelson (michael.nelson)]

> Need to find a good way to test this.

Heh, a script to setup the environment that you demo'd would be great. I just created an MP for ~name12/gnome-terminal/scanned but that of course didn't have a last revision, but I could play a little with it.

UI-wise, it works really well, updating the display with the extra row when necessary. I laughed in your video near the end where you go to update the version number, but then hesitate and stop the video... made me click it ;). So I assume that'll get done later... do you think it's worth doing it before landing this branch? The non-js fallback works fine, but it does seem strange that it doesn't bring up an overlay and update inline. Up to you.

Now, nothing to do with a UI review, but I wrote a DynamicDomUpdater a while back for this *type* of thing - basically it allows you to plug-in the functionality so that a node (for example, a table node) knows how to update itself. YMMV, but you can find it at:

lib/canonical/launchpad/javascript/soyuz/lp_dynamic_dom_updater.js

tested at:

lib/canonical/launchpad/javascript/soyuz/tests/lp_dynamic_dom_updater.js

Cheers,
Michael.

review: Approve (ui)

[Tim Penhey (thumper)]

Setting the merged revision id has always behaved like that. I hesitated because it wasn't want I was wanting to demonstrate :) I'll take a look at the dynamic dom updater at some stage.

=== modified file 'lib/canonical/launchpad/javascript/code/branchmergeproposal.js'
--- lib/canonical/launchpad/javascript/code/branchmergeproposal.js	2009-11-23 19:29:02 +0000
+++ lib/canonical/launchpad/javascript/code/branchmergeproposal.js	2010-02-26 00:40:38 +0000
@@ -32,19 +32,24 @@
                     success: function(entry) {
                         var cb = status_choice_edit.get('contentBox');
                         Y.Array.each(conf.status_widget_items, function(item) {
-                            if (item.value == status_choice_edit.get(
-                                                                    'value')) {
-                                cb.query('span').addClass(item.css_class);
-                            } else {
-                                cb.query('span').removeClass(item.css_class);
-                            }
-                        });
+                                if (item.value == status_choice_edit.get('value')) {
+                                    cb.query('span').addClass(item.css_class);
+                                } else {
+                                    cb.query('span').removeClass(item.css_class);
+                                }
+                            });
+                        update_summary();
+                    },
+                    end: function() {
+                        status_content.one('img').set('src', '/@@/edit');
                     }
                 },
                 parameters: {
-                    status: status_choice_edit.get('value')
+                    status: status_choice_edit.get('value'),
+                    revid: conf.source_revid
                 }
             };
+            status_content.one('img').set('src', '/@@/spinner');
             lp_client = new LP.client.Launchpad();
             lp_client.named_post(
                 LP.client.cache.context.self_link, 'setStatus', config);
@@ -54,4 +59,72 @@
     }
 };
 
+/*
+ * Update the summary table for the merge proposal.
+ *
+ * An async request is made for the summary table, and the content is
+ * inspected. We don't modify the status row as it is in the process of having
+ * animations run on it.  Each of the table rows has an id that is strictly
+ * alphabetical.  This ordering is used to determine if a row needs to be
+ * added or removed to the table shown on the current page.  If the row
+ * appears in both, the content is checked (except for diffs as that'll never
+ * be the same due to the javascript added classes) and if it differs the
+ * shown rows are updated.
+ */
+function update_summary() {
+    var existing_summary = Y.one('#proposal-summary tbody');
+    SUMMARY_SNIPPET = '+pagelet-summary';
+    Y.io(SUMMARY_SNIPPET, {
+            on: {
+                success: function(id, response) {
+                    var new_summary = Y.Node.create(response.responseText);
+                    var new_rows = new_summary.all('tr');
+                    var old_rows = existing_summary.all('tr');
+                    // Skip over the status row (row 0).
+                    var new_pos = 1;
+                    var old_pos = 1;
+                    var new_size = new_rows.size();
+                    var old_size = old_rows.size();
+
+                    while (new_pos < new_size && old_pos < old_size) {
+                        var new_row = new_rows.item(new_pos);
+                        var old_row = old_rows.item(old_pos);
+                        var new_id = new_row.get('id');
+                        var old_id = old_row.get('id');
+                        if (new_id == old_id) {
+                            if (new_id != 'summary-row-b-diff') {
+                                // Don't mess with the diff.
+                                if (new_row.get('innerHTML') !=
+                                    old_row.get('innerHTML')) {
+                                    existing_summary.insertBefore(new_row, old_row);
+                                    old_row.remove();
+                                }
+                            }
+                            ++new_pos;
+                            ++old_pos;
+                        } else if (new_id < old_id) {
+                            ++new_pos;
+                            existing_summary.insertBefore(new_row, old_row);
+                        } else {
+                            ++old_pos;
+                            old_row.remove();
+                        }
+                    }
+                    // Remove all left over old rows, and add all left over new rows.
+                    while (old_pos < old_size) {
+                        var old_row = old_rows.item(old_pos);
+                        ++old_pos;
+                        old_row.remove();
+                    }
+                    while (new_pos < new_size) {
+                        var new_row = new_rows.item(new_pos);
+                        ++new_pos;
+                        if (new_row.get('id') != 'summary-row-b-diff') {
+                            existing_summary.append(new_row);
+                        }
+                    }
+                }
+            }});
+}
+
 }, "0.1", {"requires": ["io", "node", "lazr.choiceedit", "lp.client.plugins"]});

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2010-02-05 21:59:26 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2010-02-26 00:40:39 +0000
@@ -687,6 +687,7 @@
                 self._createStatusVocabulary(),
                 css_class_prefix='mergestatus'),
             'status_value': self.context.queue_status.title,
+            'source_revid': self.context.source_branch.last_scanned_id,
             'user_can_edit_status': check_permission(
                 'launchpad.Edit', self.context),
             })

=== modified file 'lib/lp/code/doc/branch-merge-proposals.txt'
--- lib/lp/code/doc/branch-merge-proposals.txt	2009-10-01 17:41:10 +0000
+++ lib/lp/code/doc/branch-merge-proposals.txt	2010-02-26 00:40:38 +0000
@@ -257,12 +257,11 @@
     >>> print proposal.queue_status.title
     Needs review
 
-Branches that had been approved that have subsequently been
-set back into the needs review state still retain their old reviewed
-revision id.
+Branches that had been approved that have subsequently been set back into the
+needs review state have their reviewed revision cleared.
 
-    >>> proposal.reviewed_revision_id == tip.revision_id
-    True
+    >>> print proposal.reviewed_revision_id
+    None
 
 If the target branch has specified a specific reviewer, then either the owner
 of the target branch or the reviewer is able to approve or reject the

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2010-01-11 20:07:17 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2010-02-26 00:40:38 +0000
@@ -133,7 +133,7 @@
         Text(
             title=_(
                 "The revision id that has been approved by the reviewer.")),
-        exported_as='reviewed_revno')
+        exported_as='reviewed_revid')
 
     commit_message = exported(
         Summary(
@@ -159,7 +159,7 @@
             required=False,
             description=_("The revision id that has been queued for "
                           "landing.")),
-        exported_as='queued_revno')
+        exported_as='queued_revid')
 
     merged_revno = exported(
         Int(
@@ -269,7 +269,7 @@
         """True if it is valid for user update the proposal to next_state."""
 
     @call_with(user=REQUEST_USER)
-    @rename_parameters_as(revision_id='revno')
+    @rename_parameters_as(revision_id='revid')
     @operation_parameters(
         status=Choice(
             title=_("The new status of the merge proposal."),
@@ -284,8 +284,8 @@
 
         :param status: The new status of the merge proposal.
         :param user: The user making the change.
-        :param revision_id: The revno to provide to the underlying status
-            change method.
+        :param revision_id: The revision id to provide to the underlying
+            status change method.
         """
 
     def setAsWorkInProgress():

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2009-12-18 20:14:21 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2010-02-26 00:40:39 +0000
@@ -340,8 +340,6 @@
             self.setAsWorkInProgress()
         elif status == BranchMergeProposalStatus.NEEDS_REVIEW:
             self.requestReview()
-        elif status == BranchMergeProposalStatus.NEEDS_REVIEW:
-            self.requestReview()
         elif status == BranchMergeProposalStatus.CODE_APPROVED:
             # Other half of the edge case.  If the status is currently queued,
             # we need to dequeue, otherwise we just approve the branch.
@@ -378,6 +376,11 @@
         if self.queue_status != BranchMergeProposalStatus.NEEDS_REVIEW:
             self._transitionToState(BranchMergeProposalStatus.NEEDS_REVIEW)
             self.date_review_requested = _date_requested
+            # Clear out any reviewed or queued values.
+            self.reviewer = None
+            self.reviewed_revision_id = None
+            self.queuer = None
+            self.queued_revision_id = None
 
     def isMergable(self):
         """See `IBranchMergeProposal`."""

=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2010-02-04 16:52:05 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2010-02-26 00:40:39 +0000
@@ -42,12 +42,12 @@
     private: False
     queue_position: None
     queue_status: u'Needs review'
-    queued_revno: None
+    queued_revid: None
     queuer_link: None
     registrant_link: u'http://api.launchpad.dev/beta/~person-name...'
     resource_type_link:
         u'http://api.launchpad.dev/beta/#branch_merge_proposal'
-    reviewed_revno: None
+    reviewed_revid: None
     reviewer_link: None
     self_link: u'http://api.launchpad.dev/beta/~.../+merge/...'
     source_branch_link: u'http://api.launchpad.dev/beta/~...'
@@ -136,11 +136,11 @@
     private: False
     queue_position: None
     queue_status: u'Work in progress'
-    queued_revno: None
+    queued_revid: None
     queuer_link: None
     registrant_link: u'http://.../~person-name...'
     resource_type_link: u'http://.../#branch_merge_proposal'
-    reviewed_revno: None
+    reviewed_revid: None
     reviewer_link: None
     self_link: u'http://.../~source/fooix/fix-it/+merge/...'
     source_branch_link: u'http://.../~source/fooix/fix-it'
@@ -259,13 +259,13 @@
 
     >>> _unused = reviewer_webservice.named_post(
     ...     merge_proposal['self_link'], 'setStatus',
-    ...     status=u'Approved', revno=u'25')
+    ...     status=u'Approved', revid=u'25')
     >>> merge_proposal = reviewer_webservice.get(
     ...     merge_proposal['self_link']).jsonBody()
 
     >>> print merge_proposal['queue_status']
     Approved
-    >>> print merge_proposal['reviewed_revno']
+    >>> print merge_proposal['reviewed_revid']
     25
 
 However, there may have been breakage in the branch, and we need to revert back
@@ -279,7 +279,7 @@
 
     >>> print merge_proposal['queue_status']
     Work in progress
-    >>> print merge_proposal['reviewed_revno']
+    >>> print merge_proposal['reviewed_revid']
     None
 
 

=== modified file 'lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt'
--- lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt	2010-02-02 17:19:05 +0000
+++ lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt	2010-02-26 00:40:39 +0000
@@ -6,7 +6,15 @@
   tal:define="context_menu context/menu:context">
 
   <tbody>
-    <tr>
+    <tal:comment condition="nothing">
+      <!--
+          Each of the rows in this table have an id that is strictly
+          alphabetical (according to ASCII).  This is used in the javascript
+          processing of the summary to determine which rows have been added,
+          and removed through AJAX requests.
+      -->
+    </tal:comment>
+    <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}"
@@ -21,12 +29,14 @@
       </td>
     </tr>
     <tal:comment condition="nothing">
-      Only show the reviewed section if the state is not superseded.
-      Only show the reviewed revision if not rejected.
+      <!--
+          Only show the reviewed section if the state is not superseded.
+          Only show the reviewed revision if not rejected.
+      -->
     </tal:comment>
     <tal:not-superseded condition="not: context/queue_status/enumvalue:SUPERSEDED">
       <tal:reviewed condition="context/reviewer">
-        <tr>
+        <tr id="summary-row-2-reviewer">
           <th>
             <tal:rejected condition="context/queue_status/enumvalue:REJECTED">
               Rejected by:
@@ -42,7 +52,8 @@
             <tal:modified replace="context/date_reviewed/fmt:displaydate" />
           </td>
         </tr>
-        <tr tal:condition="not: context/queue_status/enumvalue:REJECTED">
+        <tr id="summary-row-3-approved-revision"
+            tal:condition="not: context/queue_status/enumvalue:REJECTED">
           <th>Approved revision:</th>
           <td>
             <tal:not-available condition="not: context/reviewed_revision_id">
@@ -57,11 +68,11 @@
       </tal:reviewed>
     </tal:not-superseded>
     <tal:queued condition="context/queue_status/enumvalue:QUEUED">
-      <tr>
+      <tr id="summary-row-4-queued-by">
         <th>Queued by:</th>
         <td tal:content="structure context/queuer/fmt:link">Some User</td>
       </tr>
-      <tr>
+      <tr id="summary-row-5-queued-revision">
         <th>Queued revision:</th>
         <td>
           <tal:not-available condition="not: context/queued_revision_id">
@@ -75,11 +86,12 @@
       </tr>
     </tal:queued>
     <tal:merged condition="context/queue_status/enumvalue:MERGED">
-      <tr tal:condition="context/merge_reporter">
+      <tr id="summary-row-6-merge-reporter"
+          tal:condition="context/merge_reporter">
         <th>Merge reported by:</th>
         <td tal:content="structure context/merge_reporter/fmt:link">Some User</td>
       </tr>
-      <tr>
+      <tr id="summary-row-7-merged-revision">
         <th>Merged at revision:</th>
         <td>
           <tal:not-available condition="not: context/merged_revno">
@@ -97,19 +109,21 @@
         </td>
       </tr>
     </tal:merged>
-    <tr>
+    <tr id="summary-row-8-source-branch">
       <th>Proposed branch:</th>
       <td tal:content="structure context/source_branch/fmt:bzr-link">lp:~foo/bar/baz</td>
     </tr>
-    <tr>
+    <tr id="summary-row-9-target-branch">
       <th>Merge into:</th>
       <td tal:content="structure context/target_branch/fmt:bzr-link">lp:~foo/bar/baz</td>
     </tr>
-    <tr tal:condition="context/prerequisite_branch">
+    <tr id="summary-row-a-prerequisite-branch"
+        tal:condition="context/prerequisite_branch">
       <th>Prerequisite:</th>
       <td tal:content="structure context/prerequisite_branch/fmt:bzr-link">lp:~foo/bar/baz</td>
     </tr>
-    <tr tal:condition="context/preview_diff">
+    <tr id="summary-row-b-diff"
+        tal:condition="context/preview_diff">
       <th>Diff against target:</th>
       <td>
         <tal:diff replace="structure context/preview_diff/fmt:link"/>

=== modified file 'lib/lp/code/windmill/tests/test_branchmergeproposal_commitmessage.py'
--- lib/lp/code/windmill/tests/test_branchmergeproposal_commitmessage.py	2010-02-01 18:37:00 +0000
+++ lib/lp/code/windmill/tests/test_branchmergeproposal_commitmessage.py	2010-02-26 00:40:38 +0000
@@ -81,6 +81,7 @@
             email="[e-mail]")
         branch = self.factory.makeBranch(owner=mike)
         second_branch = self.factory.makeBranch(product=branch.product)
+        self.factory.makeRevisionsForBranch(second_branch)
         merge_proposal = second_branch.addLandingTarget(mike, branch)
         transaction.commit()
 
@@ -99,12 +100,16 @@
             xpath=u'//div[contains(@class, "yui-ichoicelist-content")]')
 
         # Change the status to experimental.
-        client.click(link=u'Rejected')
+        client.click(link=u'Approved')
         client.waits.sleep(milliseconds=SLEEP)
 
         client.asserts.assertText(
             xpath=u'//td[@id="branchmergeproposal-status-value"]/span',
-            validator=u'Rejected')
+            validator=u'Approved')
+
+        client.asserts.assertText(
+            xpath=u'//tr[@id="summary-row-3-approved-revision"]/td',
+            validator=u'5')
 
         # Reload the page and make sure the change sticks.
         client.open(url=merge_url)
@@ -114,7 +119,7 @@
             timeout=FOR_ELEMENT)
         client.asserts.assertText(
             xpath=u'//td[@id="branchmergeproposal-status-value"]/span',
-            validator=u'Rejected')
+            validator=u'Approved')
 
 
 def test_suite():