Launchpad itself

Merge lp:~deryck/launchpad/better-dupe-handling-ui into lp:launchpad

  1. better-dupe-handling-ui
  2. Merge into devel
Proposed by Deryck Hodge
Status: Merged
Approved by: Aaron Bentley
Approved revision: no longer in the source branch.
Merged at revision: 11190
Proposed branch: lp:~deryck/launchpad/better-dupe-handling-ui
Merge into: lp:launchpad
Prerequisite: lp:~deryck/launchpad/do-the-right-thing-dupe-move-78596
Diff against target: 282 lines (+112/-17)
6 files modified
lib/canonical/launchpad/icing/style-3-0.css.in (+6/-0)
lib/lp/bugs/doc/bugactivity.txt (+41/-1)
lib/lp/bugs/javascript/bugtask_index.js (+38/-10)
lib/lp/bugs/model/bug.py (+7/-0)
lib/lp/bugs/templates/bug-portlet-actions.pt (+3/-3)
lib/lp/bugs/windmill/tests/test_mark_duplicate.py (+17/-3)
To merge this branch: bzr merge lp:~deryck/launchpad/better-dupe-handling-ui
Reviewer Review Type Date Requested Status
Paul Hummer (community) ui Approve
Matthew Revell (community) text Approve
Aaron Bentley (community) Approve
Review via email: mp+30309@code.launchpad.net

Commit message

Update the UI for the mark as duplicate widget to understand how to dupe a bug that has duplicates itself.

Description of the change

This branch finishes off the work of my do-the-right-thing-dupe-move-78596 branch, which made us consistent in our use of markAsDuplicate and made it possible to automatically move dupes across when making a bug a dupe that itself has duplicates. That branch couldn't land because of a couple UI issues -- namely that the widget needed to warn users what was about to happen, and the activity log needed to be right when multiple dupes are moved around.

This branch does that UI work, plus fixes some other minor UI nits, namely:

 * Fixes the style rules to ensure that the icon and text appear on the same line
 * Focus the input field when the widget appears
 * Fixes the widget to not add another edit icon when the DOM changes
 * Adds a warning in the widget if the bug has dupes itself
 * Fixes the activity log and adds test coverage
 * Extends the Windmill test to cover changes to the widget
 * Extends Windmill test to cover everything that was lost when dropping page tests in the earlier branch

Test:

Windmill:
./bin/test -cvvt test_mark_duplicate

Activity log changes:
./bin/test -cvvt doc/bugactivity.txt

Dupe handling unit test:
./bin/test -cvv test_duplicate_handling

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) :
review: Approve
Revision history for this message
Deryck Hodge (deryck) wrote :

Here's a screenshot of the new warning for the UI reviewer:

http://people.canonical.com/~deryck/mark-as-dupe-warning.png

This was pre-imp'ed pretty heavily with Curtis at the Epic, almost so that I would feel okay doing ui=sinzui. However, we didn't really analyze the text of the warning together. I think the text has the word "duplicate" too much, but I cannot think of a better wording since I'm too close to the work. So I offer this for UI review, but I'll also get mrevell to take a look at the text.

Revision history for this message
Matthew Revell (matthew.revell) wrote :

I like the text and I'm not sure we can reduce the usage of "duplicate(s)" too much but I do think it should lose a clause or two, while making clearer the division between the "hide it" section

I suggest:

Marking this bug as a duplicate will remove it from future search results.

<strong>Note:</strong> This bug has duplicates of its own. If you go ahead, they'll become duplicates of the bug you specify here.

review: Needs Fixing
Revision history for this message
Matthew Revell (matthew.revell) wrote :

Sorry, I pressed "Save comment" too soon. My text suggestion is:

Marking this bug as a duplicate will remove it from future search results.

<strong>Note:</strong> This bug has duplicates of its own. If you go ahead, they too will become duplicates of the bug you specify here. This cannot be undone.

review: Needs Fixing (text)
Revision history for this message
Deryck Hodge (deryck) wrote :

Thanks, Matthew.

I like the suggestions. This better?

http://people.canonical.com/~deryck/mark-as-dupe-warning-2.png

Revision history for this message
Matthew Revell (matthew.revell) wrote :

+1

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

Looks fine.

review: Approve (ui)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in 2010-07-15 15:10:21 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in 2010-07-21 16:39:54 +0000
@@ -1806,6 +1806,12 @@
1806 */1806 */
1807 display: inline;1807 display: inline;
1808 }1808 }
1809body.tab-bugs #duplicate-actions .sprite {
1810 /* Override sprite style for edit icon on "Mark as duplicate"
1811 to make the text not appear on a second line.
1812 */
1813 display:inline;
1814 }
1809.yui-picker-results li.sprite {1815.yui-picker-results li.sprite {
1810 /* XXX: EdwinGrubbs 2009-07-27 bug=4054761816 /* XXX: EdwinGrubbs 2009-07-27 bug=405476
1811 * Override the .yui-picker-results style, so that the next icon1817 * Override the .yui-picker-results style, so that the next icon
18121818
=== modified file 'lib/lp/bugs/doc/bugactivity.txt'
--- lib/lp/bugs/doc/bugactivity.txt 2010-06-04 17:02:01 +0000
+++ lib/lp/bugs/doc/bugactivity.txt 2010-07-21 16:39:54 +0000
@@ -136,7 +136,7 @@
136 True136 True
137137
138138
139== Bug report has itss duplicate marker changed to another bug report ==139== Bug report has its duplicate marker changed to another bug report ==
140140
141 >>> edit_fields = [141 >>> edit_fields = [
142 ... "id", "title", "description", "name", "private", "duplicateof",142 ... "id", "title", "description", "name", "private", "duplicateof",
@@ -173,6 +173,46 @@
173 True173 True
174174
175175
176== A bug with multiple duplicates ==
177
178When a bug has multiple duplicates and is itself marked a duplicate,
179the duplicates are automatically duped to the same master bug. These changes
180are then reflected in the activity log for each bug itself.
181
182 >>> edit_fields = [
183 ... "id", "title", "description", "name", "private", "duplicateof",
184 ... "security_related"]
185 >>> initial_bug = factory.makeBug()
186 >>> dupe_one = factory.makeBug()
187 >>> dupe_two = factory.makeBug()
188 >>> dupe_one.markAsDuplicate(initial_bug)
189 >>> dupe_two.markAsDuplicate(initial_bug)
190
191After creating a few bugs to work with, we create a final bug and duplicate
192the initial bug against it.
193
194 >>> final_bug = factory.makeBug()
195 >>> initial_bug.markAsDuplicate(final_bug)
196
197Now, we confirm the activity log for the other bugs correctly list the
198final_bug as their master bug.
199
200 >>> latest_activity = dupe_one.activity[-1]
201 >>> print latest_activity.whatchanged
202 changed duplicate marker
203 >>> latest_activity.oldvalue == unicode(initial_bug.id)
204 True
205 >>> latest_activity.newvalue == unicode(final_bug.id)
206 True
207 >>> latest_activity = dupe_two.activity[-1]
208 >>> print latest_activity.whatchanged
209 changed duplicate marker
210 >>> latest_activity.oldvalue == unicode(initial_bug.id)
211 True
212 >>> latest_activity.newvalue == unicode(final_bug.id)
213 True
214
215
176== BugActivityItem ==216== BugActivityItem ==
177217
178BugActivityItem implements the stuff that BugActivity doesn't need to218BugActivityItem implements the stuff that BugActivity doesn't need to
179219
=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js 2010-07-16 16:14:39 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js 2010-07-21 16:39:54 +0000
@@ -165,10 +165,23 @@
165 update_dupe_url = update_dupe_link.get('href');165 update_dupe_url = update_dupe_link.get('href');
166 var mark_dupe_form_url = update_dupe_url + '/++form++';166 var mark_dupe_form_url = update_dupe_url + '/++form++';
167167
168 var form_header = '<p>Marking this bug as a duplicate will, by default, ' +
169 'hide it from search results listings.</p>';
170
171 var has_dupes = Y.one('#portlet-duplicates');
172 if (has_dupes !== null) {
173 form_header = form_header +
174 '<p style="padding:2px 2px 0 36px;" ' +
175 'class="large-warning"><strong>Note:</strong> ' +
176 'This bug has duplicates of its own. ' +
177 'If you go ahead, they too will become duplicates of ' +
178 'the bug you specify here. This cannot be undone.' +
179 '</p></div>';
180 }
181
168 duplicate_form_overlay = new Y.lazr.FormOverlay({182 duplicate_form_overlay = new Y.lazr.FormOverlay({
169 headerContent: '<h2>Mark bug report as duplicate</h2>',183 headerContent: '<h2>Mark bug report as duplicate</h2>',
170 form_header: 'Marking the bug as a duplicate will, by default, ' +184 form_header: form_header,
171 'hide it from search results listings.',
172 form_submit_button: Y.Node.create(submit_button_html),185 form_submit_button: Y.Node.create(submit_button_html),
173 form_cancel_button: Y.Node.create(cancel_button_html),186 form_cancel_button: Y.Node.create(cancel_button_html),
174 centered: true,187 centered: true,
@@ -187,6 +200,7 @@
187 if (duplicate_form_overlay){200 if (duplicate_form_overlay){
188 e.preventDefault();201 e.preventDefault();
189 duplicate_form_overlay.show();202 duplicate_form_overlay.show();
203 Y.DOM.byId('field.duplicateof').focus();
190 }204 }
191 });205 });
192 // Add a class denoting them as js-action links.206 // Add a class denoting them as js-action links.
@@ -366,8 +380,15 @@
366 // Hide the formoverlay:380 // Hide the formoverlay:
367 duplicate_form_overlay.hide();381 duplicate_form_overlay.hide();
368382
383 // Hide the dupe edit icon if it exists.
384 var dupe_edit_icon = Y.one('#change_duplicate_bug');
385 if (dupe_edit_icon !== null) {
386 dupe_edit_icon.setStyle('display', 'none');
387 }
388
369 // Add the spinner...389 // Add the spinner...
370 var dupe_span = Y.one('#mark-duplicate-text');390 var dupe_span = Y.one('#mark-duplicate-text');
391 dupe_span.removeClass('sprite bug-dupe');
371 dupe_span.addClass('update-in-progress-message');392 dupe_span.addClass('update-in-progress-message');
372393
373 // Set the new duplicate link on the bug entry.394 // Set the new duplicate link on the bug entry.
@@ -390,28 +411,35 @@
390411
391 if (new_dup_url !== null) {412 if (new_dup_url !== null) {
392 dupe_span.set('innerHTML', [413 dupe_span.set('innerHTML', [
393 'Duplicate of <a>bug #</a> ',414 '<a id="change_duplicate_bug" ',
394 '<a class="menu-link-mark-dupe js-action sprite edit">',415 'title="Edit or remove linked duplicate bug" ',
395 '<span class="invisible-link">edit</span></a>'].join(""));416 'class="sprite edit"></a>',
417 'Duplicate of <a>bug #</a>'].join(""));
396 dupe_span.all('a').item(0)418 dupe_span.all('a').item(0)
419 .set('href', update_dupe_url);
420 dupe_span.all('a').item(1)
397 .set('href', '/bugs/' + new_dup_id)421 .set('href', '/bugs/' + new_dup_id)
398 .appendChild(document.createTextNode(new_dup_id));422 .appendChild(document.createTextNode(new_dup_id));
399 dupe_span.all('a').item(1)423 var has_dupes = Y.one('#portlet-duplicates');
400 .set('href', update_dupe_url);424 if (has_dupes !== null) {
425 has_dupes.get('parentNode').removeChild(has_dupes);
426 }
401 show_comment_on_duplicate_warning();427 show_comment_on_duplicate_warning();
402 } else {428 } else {
429 dupe_span.addClass('sprite bug-dupe');
403 dupe_span.set('innerHTML', [430 dupe_span.set('innerHTML', [
404 '<a class="menu-link-mark-dupe js-action ',431 '<a class="menu-link-mark-dupe js-action">',
405 'sprite bug-dupe">Mark as duplicate</a>'].join(""));432 'Mark as duplicate</a>'].join(""));
406 dupe_span.one('a').set('href', update_dupe_url);433 dupe_span.one('a').set('href', update_dupe_url);
407 hide_comment_on_duplicate_warning();434 hide_comment_on_duplicate_warning();
408 }435 }
409 Y.lazr.anim.green_flash({node: dupe_span}).run();436 Y.lazr.anim.green_flash({node: dupe_span}).run();
410 // ensure the new link is hooked up correctly:437 // ensure the new link is hooked up correctly:
411 dupe_span.one('a.menu-link-mark-dupe').on(438 dupe_span.one('a').on(
412 'click', function(e){439 'click', function(e){
413 e.preventDefault();440 e.preventDefault();
414 duplicate_form_overlay.show();441 duplicate_form_overlay.show();
442 Y.DOM.byId('field.duplicateof').focus();
415 });443 });
416 },444 },
417 failure: function(id, request) {445 failure: function(id, request) {
418446
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2010-07-21 12:30:46 +0000
+++ lib/lp/bugs/model/bug.py 2010-07-21 16:39:54 +0000
@@ -1503,7 +1503,14 @@
1503 field._validate(duplicate_of)1503 field._validate(duplicate_of)
1504 if self.duplicates:1504 if self.duplicates:
1505 for duplicate in self.duplicates:1505 for duplicate in self.duplicates:
1506 # Fire a notify event in model code since moving
1507 # duplicates of a duplicate does not normally fire an
1508 # event.
1509 dupe_before = Snapshot(
1510 duplicate, providing=providedBy(duplicate))
1506 duplicate.markAsDuplicate(duplicate_of)1511 duplicate.markAsDuplicate(duplicate_of)
1512 notify(ObjectModifiedEvent(
1513 duplicate, dupe_before, 'duplicateof'))
1507 self.duplicateof = duplicate_of1514 self.duplicateof = duplicate_of
1508 except LaunchpadValidationError, validation_error:1515 except LaunchpadValidationError, validation_error:
1509 raise InvalidDuplicateValue(validation_error)1516 raise InvalidDuplicateValue(validation_error)
15101517
=== modified file 'lib/lp/bugs/templates/bug-portlet-actions.pt'
--- lib/lp/bugs/templates/bug-portlet-actions.pt 2009-10-29 21:39:12 +0000
+++ lib/lp/bugs/templates/bug-portlet-actions.pt 2010-07-21 16:39:54 +0000
@@ -11,10 +11,11 @@
11 tal:define="link context_menu/markduplicate"11 tal:define="link context_menu/markduplicate"
12 tal:condition="python: link.enabled and not12 tal:condition="python: link.enabled and not
13 context.duplicateof"13 context.duplicateof"
14 class="sprite bug-dupe"
14 >15 >
15 <a16 <a
16 tal:attributes="href link/path"17 tal:attributes="href link/path"
17 class="menu-link-mark-dupe sprite bug-dupe">Mark as duplicate</a>18 class="menu-link-mark-dupe">Mark as duplicate</a>
18 </span>19 </span>
19 <tal:block20 <tal:block
20 tal:condition="context/duplicates"21 tal:condition="context/duplicates"
@@ -31,8 +32,7 @@
31 id="change_duplicate_bug"32 id="change_duplicate_bug"
32 title="Edit or remove linked duplicate bug"33 title="Edit or remove linked duplicate bug"
33 class="sprite edit"34 class="sprite edit"
34 tal:attributes="href link/url"></a>35 tal:attributes="href link/url"></a><span id="mark-duplicate-text">Duplicate of
35 <span id="mark-duplicate-text">Duplicate of
36 <a36 <a
37 tal:condition="duplicateof/required:launchpad.View"37 tal:condition="duplicateof/required:launchpad.View"
38 tal:attributes="href duplicateof/fmt:url; title38 tal:attributes="href duplicateof/fmt:url; title
3939
=== modified file 'lib/lp/bugs/windmill/tests/test_mark_duplicate.py'
--- lib/lp/bugs/windmill/tests/test_mark_duplicate.py 2010-02-18 10:51:34 +0000
+++ lib/lp/bugs/windmill/tests/test_mark_duplicate.py 2010-07-21 16:39:54 +0000
@@ -44,10 +44,13 @@
44 client.waits.forElement(44 client.waits.forElement(
45 xpath=MAIN_FORM_ELEMENT, timeout=constants.FOR_ELEMENT)45 xpath=MAIN_FORM_ELEMENT, timeout=constants.FOR_ELEMENT)
4646
47 # Initially the form overlay is hidden47 # Initially the form overlay is hidden...
48 client.asserts.assertElemJS(48 client.asserts.assertElemJS(
49 xpath=MAIN_FORM_ELEMENT, js=FORM_NOT_VISIBLE)49 xpath=MAIN_FORM_ELEMENT, js=FORM_NOT_VISIBLE)
5050
51 # ...and there is an expandable form for editing bug status, etc.
52 client.asserts.assertNode(classname='bug-status-expand')
53
51 # Clicking on the mark duplicate link brings up the formoverlay.54 # Clicking on the mark duplicate link brings up the formoverlay.
52 # Entering 1 as the duplicate ID changes the duplicate text.55 # Entering 1 as the duplicate ID changes the duplicate text.
53 client.click(classname=u'menu-link-mark-dupe')56 client.click(classname=u'menu-link-mark-dupe')
@@ -66,7 +69,7 @@
66 id='warning-comment-on-duplicate', timeout=constants.FOR_ELEMENT)69 id='warning-comment-on-duplicate', timeout=constants.FOR_ELEMENT)
6770
68 # The duplicate can be cleared:71 # The duplicate can be cleared:
69 client.click(classname=u'menu-link-mark-dupe')72 client.click(id=u'mark-duplicate-text')
70 client.type(text=u'', id=u'field.duplicateof')73 client.type(text=u'', id=u'field.duplicateof')
71 client.click(xpath=CHANGE_BUTTON)74 client.click(xpath=CHANGE_BUTTON)
72 client.waits.forElement(75 client.waits.forElement(
@@ -77,7 +80,7 @@
77 client.asserts.assertNotNode(id='warning-comment-on-duplicate')80 client.asserts.assertNotNode(id='warning-comment-on-duplicate')
7881
79 # Entering a false bug number results in input validation errors82 # Entering a false bug number results in input validation errors
80 client.click(classname=u'menu-link-mark-dupe')83 client.click(id=u'mark-duplicate-text')
81 client.type(text=u'123', id=u'field.duplicateof')84 client.type(text=u'123', id=u'field.duplicateof')
82 client.click(xpath=CHANGE_BUTTON)85 client.click(xpath=CHANGE_BUTTON)
83 error_xpath = (86 error_xpath = (
@@ -105,6 +108,14 @@
105 xpath=u"//h1[@id='bug-title']/span[1]",108 xpath=u"//h1[@id='bug-title']/span[1]",
106 validator=u'Firefox does not support SVG')109 validator=u'Firefox does not support SVG')
107110
111 # If someone wants to set the master to dupe another bug, there
112 # is a warning in the dupe widget about this bug having its own
113 # duplicates.
114 client.click(classname='menu-link-mark-dupe')
115 client.asserts.assertTextIn(
116 classname='large-warning', validator=u'This bug has duplicates',
117 timeout=constants.FOR_ELEMENT)
118
108 # When we go back to the page for the duplicate bug...119 # When we go back to the page for the duplicate bug...
109 client.open(url=u'http://bugs.launchpad.dev:8085/bugs/15')120 client.open(url=u'http://bugs.launchpad.dev:8085/bugs/15')
110 client.waits.forPageLoad(timeout=constants.PAGE_LOAD)121 client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
@@ -115,6 +126,9 @@
115 # as the one we saw before.126 # as the one we saw before.
116 client.asserts.assertNode(id='warning-comment-on-duplicate')127 client.asserts.assertNode(id='warning-comment-on-duplicate')
117128
129 # Duplicate pages also do not have the expandable form on them.
130 client.asserts.assertNotNode(classname='bug-status-expand')
131
118 # Once we remove the duplicate mark...132 # Once we remove the duplicate mark...
119 client.click(id=u'change_duplicate_bug')133 client.click(id=u'change_duplicate_bug')
120 client.type(text=u'', id=u'field.duplicateof')134 client.type(text=u'', id=u'field.duplicateof')