Launchpad itself

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

  1. use-last-rev-id
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/use-last-rev-id
Merge into: lp:launchpad
Diff against target: 411 lines (+138/-43)
8 files modified
lib/canonical/launchpad/javascript/code/branchmergeproposal.js (+81/-8)
lib/lp/code/browser/branchmergeproposal.py (+1/-0)
lib/lp/code/doc/branch-merge-proposals.txt (+4/-5)
lib/lp/code/interfaces/branchmergeproposal.py (+5/-5)
lib/lp/code/model/branchmergeproposal.py (+5/-2)
lib/lp/code/stories/webservice/xx-branchmergeproposal.txt (+7/-7)
lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt (+27/-13)
lib/lp/code/windmill/tests/test_branchmergeproposal_commitmessage.py (+8/-3)
To merge this branch: bzr merge lp:~thumper/launchpad/use-last-rev-id
Reviewer Review Type Date Requested Status
Michael Nelson (community) ui Approve
Paul Hummer (community) code js ui* Approve
Review via email: mp+20023@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

Need to find a good way to test this.

Revision history for this message
Tim Penhey (thumper) wrote :

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

Revision history for this message
Paul Hummer (rockstar) wrote :

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*)
Revision history for this message
Tim Penhey (thumper) wrote :

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

Revision history for this message
Michael Nelson (michael.nelson) wrote :

> 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)
Revision history for this message
Tim Penhey (thumper) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/javascript/code/branchmergeproposal.js'
2--- lib/canonical/launchpad/javascript/code/branchmergeproposal.js 2009-11-23 19:29:02 +0000
3+++ lib/canonical/launchpad/javascript/code/branchmergeproposal.js 2010-02-26 00:40:38 +0000
4@@ -32,19 +32,24 @@
5 success: function(entry) {
6 var cb = status_choice_edit.get('contentBox');
7 Y.Array.each(conf.status_widget_items, function(item) {
8- if (item.value == status_choice_edit.get(
9- 'value')) {
10- cb.query('span').addClass(item.css_class);
11- } else {
12- cb.query('span').removeClass(item.css_class);
13- }
14- });
15+ if (item.value == status_choice_edit.get('value')) {
16+ cb.query('span').addClass(item.css_class);
17+ } else {
18+ cb.query('span').removeClass(item.css_class);
19+ }
20+ });
21+ update_summary();
22+ },
23+ end: function() {
24+ status_content.one('img').set('src', '/@@/edit');
25 }
26 },
27 parameters: {
28- status: status_choice_edit.get('value')
29+ status: status_choice_edit.get('value'),
30+ revid: conf.source_revid
31 }
32 };
33+ status_content.one('img').set('src', '/@@/spinner');
34 lp_client = new LP.client.Launchpad();
35 lp_client.named_post(
36 LP.client.cache.context.self_link, 'setStatus', config);
37@@ -54,4 +59,72 @@
38 }
39 };
40
41+/*
42+ * Update the summary table for the merge proposal.
43+ *
44+ * An async request is made for the summary table, and the content is
45+ * inspected. We don't modify the status row as it is in the process of having
46+ * animations run on it. Each of the table rows has an id that is strictly
47+ * alphabetical. This ordering is used to determine if a row needs to be
48+ * added or removed to the table shown on the current page. If the row
49+ * appears in both, the content is checked (except for diffs as that'll never
50+ * be the same due to the javascript added classes) and if it differs the
51+ * shown rows are updated.
52+ */
53+function update_summary() {
54+ var existing_summary = Y.one('#proposal-summary tbody');
55+ SUMMARY_SNIPPET = '+pagelet-summary';
56+ Y.io(SUMMARY_SNIPPET, {
57+ on: {
58+ success: function(id, response) {
59+ var new_summary = Y.Node.create(response.responseText);
60+ var new_rows = new_summary.all('tr');
61+ var old_rows = existing_summary.all('tr');
62+ // Skip over the status row (row 0).
63+ var new_pos = 1;
64+ var old_pos = 1;
65+ var new_size = new_rows.size();
66+ var old_size = old_rows.size();
67+
68+ while (new_pos < new_size && old_pos < old_size) {
69+ var new_row = new_rows.item(new_pos);
70+ var old_row = old_rows.item(old_pos);
71+ var new_id = new_row.get('id');
72+ var old_id = old_row.get('id');
73+ if (new_id == old_id) {
74+ if (new_id != 'summary-row-b-diff') {
75+ // Don't mess with the diff.
76+ if (new_row.get('innerHTML') !=
77+ old_row.get('innerHTML')) {
78+ existing_summary.insertBefore(new_row, old_row);
79+ old_row.remove();
80+ }
81+ }
82+ ++new_pos;
83+ ++old_pos;
84+ } else if (new_id < old_id) {
85+ ++new_pos;
86+ existing_summary.insertBefore(new_row, old_row);
87+ } else {
88+ ++old_pos;
89+ old_row.remove();
90+ }
91+ }
92+ // Remove all left over old rows, and add all left over new rows.
93+ while (old_pos < old_size) {
94+ var old_row = old_rows.item(old_pos);
95+ ++old_pos;
96+ old_row.remove();
97+ }
98+ while (new_pos < new_size) {
99+ var new_row = new_rows.item(new_pos);
100+ ++new_pos;
101+ if (new_row.get('id') != 'summary-row-b-diff') {
102+ existing_summary.append(new_row);
103+ }
104+ }
105+ }
106+ }});
107+}
108+
109 }, "0.1", {"requires": ["io", "node", "lazr.choiceedit", "lp.client.plugins"]});
110
111=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
112--- lib/lp/code/browser/branchmergeproposal.py 2010-02-05 21:59:26 +0000
113+++ lib/lp/code/browser/branchmergeproposal.py 2010-02-26 00:40:39 +0000
114@@ -687,6 +687,7 @@
115 self._createStatusVocabulary(),
116 css_class_prefix='mergestatus'),
117 'status_value': self.context.queue_status.title,
118+ 'source_revid': self.context.source_branch.last_scanned_id,
119 'user_can_edit_status': check_permission(
120 'launchpad.Edit', self.context),
121 })
122
123=== modified file 'lib/lp/code/doc/branch-merge-proposals.txt'
124--- lib/lp/code/doc/branch-merge-proposals.txt 2009-10-01 17:41:10 +0000
125+++ lib/lp/code/doc/branch-merge-proposals.txt 2010-02-26 00:40:38 +0000
126@@ -257,12 +257,11 @@
127 >>> print proposal.queue_status.title
128 Needs review
129
130-Branches that had been approved that have subsequently been
131-set back into the needs review state still retain their old reviewed
132-revision id.
133+Branches that had been approved that have subsequently been set back into the
134+needs review state have their reviewed revision cleared.
135
136- >>> proposal.reviewed_revision_id == tip.revision_id
137- True
138+ >>> print proposal.reviewed_revision_id
139+ None
140
141 If the target branch has specified a specific reviewer, then either the owner
142 of the target branch or the reviewer is able to approve or reject the
143
144=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
145--- lib/lp/code/interfaces/branchmergeproposal.py 2010-01-11 20:07:17 +0000
146+++ lib/lp/code/interfaces/branchmergeproposal.py 2010-02-26 00:40:38 +0000
147@@ -133,7 +133,7 @@
148 Text(
149 title=_(
150 "The revision id that has been approved by the reviewer.")),
151- exported_as='reviewed_revno')
152+ exported_as='reviewed_revid')
153
154 commit_message = exported(
155 Summary(
156@@ -159,7 +159,7 @@
157 required=False,
158 description=_("The revision id that has been queued for "
159 "landing.")),
160- exported_as='queued_revno')
161+ exported_as='queued_revid')
162
163 merged_revno = exported(
164 Int(
165@@ -269,7 +269,7 @@
166 """True if it is valid for user update the proposal to next_state."""
167
168 @call_with(user=REQUEST_USER)
169- @rename_parameters_as(revision_id='revno')
170+ @rename_parameters_as(revision_id='revid')
171 @operation_parameters(
172 status=Choice(
173 title=_("The new status of the merge proposal."),
174@@ -284,8 +284,8 @@
175
176 :param status: The new status of the merge proposal.
177 :param user: The user making the change.
178- :param revision_id: The revno to provide to the underlying status
179- change method.
180+ :param revision_id: The revision id to provide to the underlying
181+ status change method.
182 """
183
184 def setAsWorkInProgress():
185
186=== modified file 'lib/lp/code/model/branchmergeproposal.py'
187--- lib/lp/code/model/branchmergeproposal.py 2009-12-18 20:14:21 +0000
188+++ lib/lp/code/model/branchmergeproposal.py 2010-02-26 00:40:39 +0000
189@@ -340,8 +340,6 @@
190 self.setAsWorkInProgress()
191 elif status == BranchMergeProposalStatus.NEEDS_REVIEW:
192 self.requestReview()
193- elif status == BranchMergeProposalStatus.NEEDS_REVIEW:
194- self.requestReview()
195 elif status == BranchMergeProposalStatus.CODE_APPROVED:
196 # Other half of the edge case. If the status is currently queued,
197 # we need to dequeue, otherwise we just approve the branch.
198@@ -378,6 +376,11 @@
199 if self.queue_status != BranchMergeProposalStatus.NEEDS_REVIEW:
200 self._transitionToState(BranchMergeProposalStatus.NEEDS_REVIEW)
201 self.date_review_requested = _date_requested
202+ # Clear out any reviewed or queued values.
203+ self.reviewer = None
204+ self.reviewed_revision_id = None
205+ self.queuer = None
206+ self.queued_revision_id = None
207
208 def isMergable(self):
209 """See `IBranchMergeProposal`."""
210
211=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
212--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2010-02-04 16:52:05 +0000
213+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt 2010-02-26 00:40:39 +0000
214@@ -42,12 +42,12 @@
215 private: False
216 queue_position: None
217 queue_status: u'Needs review'
218- queued_revno: None
219+ queued_revid: None
220 queuer_link: None
221 registrant_link: u'http://api.launchpad.dev/beta/~person-name...'
222 resource_type_link:
223 u'http://api.launchpad.dev/beta/#branch_merge_proposal'
224- reviewed_revno: None
225+ reviewed_revid: None
226 reviewer_link: None
227 self_link: u'http://api.launchpad.dev/beta/~.../+merge/...'
228 source_branch_link: u'http://api.launchpad.dev/beta/~...'
229@@ -136,11 +136,11 @@
230 private: False
231 queue_position: None
232 queue_status: u'Work in progress'
233- queued_revno: None
234+ queued_revid: None
235 queuer_link: None
236 registrant_link: u'http://.../~person-name...'
237 resource_type_link: u'http://.../#branch_merge_proposal'
238- reviewed_revno: None
239+ reviewed_revid: None
240 reviewer_link: None
241 self_link: u'http://.../~source/fooix/fix-it/+merge/...'
242 source_branch_link: u'http://.../~source/fooix/fix-it'
243@@ -259,13 +259,13 @@
244
245 >>> _unused = reviewer_webservice.named_post(
246 ... merge_proposal['self_link'], 'setStatus',
247- ... status=u'Approved', revno=u'25')
248+ ... status=u'Approved', revid=u'25')
249 >>> merge_proposal = reviewer_webservice.get(
250 ... merge_proposal['self_link']).jsonBody()
251
252 >>> print merge_proposal['queue_status']
253 Approved
254- >>> print merge_proposal['reviewed_revno']
255+ >>> print merge_proposal['reviewed_revid']
256 25
257
258 However, there may have been breakage in the branch, and we need to revert back
259@@ -279,7 +279,7 @@
260
261 >>> print merge_proposal['queue_status']
262 Work in progress
263- >>> print merge_proposal['reviewed_revno']
264+ >>> print merge_proposal['reviewed_revid']
265 None
266
267
268
269=== modified file 'lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt'
270--- lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2010-02-02 17:19:05 +0000
271+++ lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt 2010-02-26 00:40:39 +0000
272@@ -6,7 +6,15 @@
273 tal:define="context_menu context/menu:context">
274
275 <tbody>
276- <tr>
277+ <tal:comment condition="nothing">
278+ <!--
279+ Each of the rows in this table have an id that is strictly
280+ alphabetical (according to ASCII). This is used in the javascript
281+ processing of the summary to determine which rows have been added,
282+ and removed through AJAX requests.
283+ -->
284+ </tal:comment>
285+ <tr id="summary-row-1-status">
286 <th>Status:</th>
287 <td id="branchmergeproposal-status-value">
288 <span tal:attributes="class string:value mergestatus${context/queue_status/name}"
289@@ -21,12 +29,14 @@
290 </td>
291 </tr>
292 <tal:comment condition="nothing">
293- Only show the reviewed section if the state is not superseded.
294- Only show the reviewed revision if not rejected.
295+ <!--
296+ Only show the reviewed section if the state is not superseded.
297+ Only show the reviewed revision if not rejected.
298+ -->
299 </tal:comment>
300 <tal:not-superseded condition="not: context/queue_status/enumvalue:SUPERSEDED">
301 <tal:reviewed condition="context/reviewer">
302- <tr>
303+ <tr id="summary-row-2-reviewer">
304 <th>
305 <tal:rejected condition="context/queue_status/enumvalue:REJECTED">
306 Rejected by:
307@@ -42,7 +52,8 @@
308 <tal:modified replace="context/date_reviewed/fmt:displaydate" />
309 </td>
310 </tr>
311- <tr tal:condition="not: context/queue_status/enumvalue:REJECTED">
312+ <tr id="summary-row-3-approved-revision"
313+ tal:condition="not: context/queue_status/enumvalue:REJECTED">
314 <th>Approved revision:</th>
315 <td>
316 <tal:not-available condition="not: context/reviewed_revision_id">
317@@ -57,11 +68,11 @@
318 </tal:reviewed>
319 </tal:not-superseded>
320 <tal:queued condition="context/queue_status/enumvalue:QUEUED">
321- <tr>
322+ <tr id="summary-row-4-queued-by">
323 <th>Queued by:</th>
324 <td tal:content="structure context/queuer/fmt:link">Some User</td>
325 </tr>
326- <tr>
327+ <tr id="summary-row-5-queued-revision">
328 <th>Queued revision:</th>
329 <td>
330 <tal:not-available condition="not: context/queued_revision_id">
331@@ -75,11 +86,12 @@
332 </tr>
333 </tal:queued>
334 <tal:merged condition="context/queue_status/enumvalue:MERGED">
335- <tr tal:condition="context/merge_reporter">
336+ <tr id="summary-row-6-merge-reporter"
337+ tal:condition="context/merge_reporter">
338 <th>Merge reported by:</th>
339 <td tal:content="structure context/merge_reporter/fmt:link">Some User</td>
340 </tr>
341- <tr>
342+ <tr id="summary-row-7-merged-revision">
343 <th>Merged at revision:</th>
344 <td>
345 <tal:not-available condition="not: context/merged_revno">
346@@ -97,19 +109,21 @@
347 </td>
348 </tr>
349 </tal:merged>
350- <tr>
351+ <tr id="summary-row-8-source-branch">
352 <th>Proposed branch:</th>
353 <td tal:content="structure context/source_branch/fmt:bzr-link">lp:~foo/bar/baz</td>
354 </tr>
355- <tr>
356+ <tr id="summary-row-9-target-branch">
357 <th>Merge into:</th>
358 <td tal:content="structure context/target_branch/fmt:bzr-link">lp:~foo/bar/baz</td>
359 </tr>
360- <tr tal:condition="context/prerequisite_branch">
361+ <tr id="summary-row-a-prerequisite-branch"
362+ tal:condition="context/prerequisite_branch">
363 <th>Prerequisite:</th>
364 <td tal:content="structure context/prerequisite_branch/fmt:bzr-link">lp:~foo/bar/baz</td>
365 </tr>
366- <tr tal:condition="context/preview_diff">
367+ <tr id="summary-row-b-diff"
368+ tal:condition="context/preview_diff">
369 <th>Diff against target:</th>
370 <td>
371 <tal:diff replace="structure context/preview_diff/fmt:link"/>
372
373=== modified file 'lib/lp/code/windmill/tests/test_branchmergeproposal_commitmessage.py'
374--- lib/lp/code/windmill/tests/test_branchmergeproposal_commitmessage.py 2010-02-01 18:37:00 +0000
375+++ lib/lp/code/windmill/tests/test_branchmergeproposal_commitmessage.py 2010-02-26 00:40:38 +0000
376@@ -81,6 +81,7 @@
377 email="mike@example.com")
378 branch = self.factory.makeBranch(owner=mike)
379 second_branch = self.factory.makeBranch(product=branch.product)
380+ self.factory.makeRevisionsForBranch(second_branch)
381 merge_proposal = second_branch.addLandingTarget(mike, branch)
382 transaction.commit()
383
384@@ -99,12 +100,16 @@
385 xpath=u'//div[contains(@class, "yui-ichoicelist-content")]')
386
387 # Change the status to experimental.
388- client.click(link=u'Rejected')
389+ client.click(link=u'Approved')
390 client.waits.sleep(milliseconds=SLEEP)
391
392 client.asserts.assertText(
393 xpath=u'//td[@id="branchmergeproposal-status-value"]/span',
394- validator=u'Rejected')
395+ validator=u'Approved')
396+
397+ client.asserts.assertText(
398+ xpath=u'//tr[@id="summary-row-3-approved-revision"]/td',
399+ validator=u'5')
400
401 # Reload the page and make sure the change sticks.
402 client.open(url=merge_url)
403@@ -114,7 +119,7 @@
404 timeout=FOR_ELEMENT)
405 client.asserts.assertText(
406 xpath=u'//td[@id="branchmergeproposal-status-value"]/span',
407- validator=u'Rejected')
408+ validator=u'Approved')
409
410
411 def test_suite():