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
1=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
2--- lib/canonical/launchpad/icing/style-3-0.css.in 2010-07-15 15:10:21 +0000
3+++ lib/canonical/launchpad/icing/style-3-0.css.in 2010-07-21 16:39:54 +0000
4@@ -1806,6 +1806,12 @@
5 */
6 display: inline;
7 }
8+body.tab-bugs #duplicate-actions .sprite {
9+ /* Override sprite style for edit icon on "Mark as duplicate"
10+ to make the text not appear on a second line.
11+ */
12+ display:inline;
13+ }
14 .yui-picker-results li.sprite {
15 /* XXX: EdwinGrubbs 2009-07-27 bug=405476
16 * Override the .yui-picker-results style, so that the next icon
17
18=== modified file 'lib/lp/bugs/doc/bugactivity.txt'
19--- lib/lp/bugs/doc/bugactivity.txt 2010-06-04 17:02:01 +0000
20+++ lib/lp/bugs/doc/bugactivity.txt 2010-07-21 16:39:54 +0000
21@@ -136,7 +136,7 @@
22 True
23
24
25-== Bug report has itss duplicate marker changed to another bug report ==
26+== Bug report has its duplicate marker changed to another bug report ==
27
28 >>> edit_fields = [
29 ... "id", "title", "description", "name", "private", "duplicateof",
30@@ -173,6 +173,46 @@
31 True
32
33
34+== A bug with multiple duplicates ==
35+
36+When a bug has multiple duplicates and is itself marked a duplicate,
37+the duplicates are automatically duped to the same master bug. These changes
38+are then reflected in the activity log for each bug itself.
39+
40+ >>> edit_fields = [
41+ ... "id", "title", "description", "name", "private", "duplicateof",
42+ ... "security_related"]
43+ >>> initial_bug = factory.makeBug()
44+ >>> dupe_one = factory.makeBug()
45+ >>> dupe_two = factory.makeBug()
46+ >>> dupe_one.markAsDuplicate(initial_bug)
47+ >>> dupe_two.markAsDuplicate(initial_bug)
48+
49+After creating a few bugs to work with, we create a final bug and duplicate
50+the initial bug against it.
51+
52+ >>> final_bug = factory.makeBug()
53+ >>> initial_bug.markAsDuplicate(final_bug)
54+
55+Now, we confirm the activity log for the other bugs correctly list the
56+final_bug as their master bug.
57+
58+ >>> latest_activity = dupe_one.activity[-1]
59+ >>> print latest_activity.whatchanged
60+ changed duplicate marker
61+ >>> latest_activity.oldvalue == unicode(initial_bug.id)
62+ True
63+ >>> latest_activity.newvalue == unicode(final_bug.id)
64+ True
65+ >>> latest_activity = dupe_two.activity[-1]
66+ >>> print latest_activity.whatchanged
67+ changed duplicate marker
68+ >>> latest_activity.oldvalue == unicode(initial_bug.id)
69+ True
70+ >>> latest_activity.newvalue == unicode(final_bug.id)
71+ True
72+
73+
74 == BugActivityItem ==
75
76 BugActivityItem implements the stuff that BugActivity doesn't need to
77
78=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
79--- lib/lp/bugs/javascript/bugtask_index.js 2010-07-16 16:14:39 +0000
80+++ lib/lp/bugs/javascript/bugtask_index.js 2010-07-21 16:39:54 +0000
81@@ -165,10 +165,23 @@
82 update_dupe_url = update_dupe_link.get('href');
83 var mark_dupe_form_url = update_dupe_url + '/++form++';
84
85+ var form_header = '<p>Marking this bug as a duplicate will, by default, ' +
86+ 'hide it from search results listings.</p>';
87+
88+ var has_dupes = Y.one('#portlet-duplicates');
89+ if (has_dupes !== null) {
90+ form_header = form_header +
91+ '<p style="padding:2px 2px 0 36px;" ' +
92+ 'class="large-warning"><strong>Note:</strong> ' +
93+ 'This bug has duplicates of its own. ' +
94+ 'If you go ahead, they too will become duplicates of ' +
95+ 'the bug you specify here. This cannot be undone.' +
96+ '</p></div>';
97+ }
98+
99 duplicate_form_overlay = new Y.lazr.FormOverlay({
100 headerContent: '<h2>Mark bug report as duplicate</h2>',
101- form_header: 'Marking the bug as a duplicate will, by default, ' +
102- 'hide it from search results listings.',
103+ form_header: form_header,
104 form_submit_button: Y.Node.create(submit_button_html),
105 form_cancel_button: Y.Node.create(cancel_button_html),
106 centered: true,
107@@ -187,6 +200,7 @@
108 if (duplicate_form_overlay){
109 e.preventDefault();
110 duplicate_form_overlay.show();
111+ Y.DOM.byId('field.duplicateof').focus();
112 }
113 });
114 // Add a class denoting them as js-action links.
115@@ -366,8 +380,15 @@
116 // Hide the formoverlay:
117 duplicate_form_overlay.hide();
118
119+ // Hide the dupe edit icon if it exists.
120+ var dupe_edit_icon = Y.one('#change_duplicate_bug');
121+ if (dupe_edit_icon !== null) {
122+ dupe_edit_icon.setStyle('display', 'none');
123+ }
124+
125 // Add the spinner...
126 var dupe_span = Y.one('#mark-duplicate-text');
127+ dupe_span.removeClass('sprite bug-dupe');
128 dupe_span.addClass('update-in-progress-message');
129
130 // Set the new duplicate link on the bug entry.
131@@ -390,28 +411,35 @@
132
133 if (new_dup_url !== null) {
134 dupe_span.set('innerHTML', [
135- 'Duplicate of <a>bug #</a> ',
136- '<a class="menu-link-mark-dupe js-action sprite edit">',
137- '<span class="invisible-link">edit</span></a>'].join(""));
138+ '<a id="change_duplicate_bug" ',
139+ 'title="Edit or remove linked duplicate bug" ',
140+ 'class="sprite edit"></a>',
141+ 'Duplicate of <a>bug #</a>'].join(""));
142 dupe_span.all('a').item(0)
143+ .set('href', update_dupe_url);
144+ dupe_span.all('a').item(1)
145 .set('href', '/bugs/' + new_dup_id)
146 .appendChild(document.createTextNode(new_dup_id));
147- dupe_span.all('a').item(1)
148- .set('href', update_dupe_url);
149+ var has_dupes = Y.one('#portlet-duplicates');
150+ if (has_dupes !== null) {
151+ has_dupes.get('parentNode').removeChild(has_dupes);
152+ }
153 show_comment_on_duplicate_warning();
154 } else {
155+ dupe_span.addClass('sprite bug-dupe');
156 dupe_span.set('innerHTML', [
157- '<a class="menu-link-mark-dupe js-action ',
158- 'sprite bug-dupe">Mark as duplicate</a>'].join(""));
159+ '<a class="menu-link-mark-dupe js-action">',
160+ 'Mark as duplicate</a>'].join(""));
161 dupe_span.one('a').set('href', update_dupe_url);
162 hide_comment_on_duplicate_warning();
163 }
164 Y.lazr.anim.green_flash({node: dupe_span}).run();
165 // ensure the new link is hooked up correctly:
166- dupe_span.one('a.menu-link-mark-dupe').on(
167+ dupe_span.one('a').on(
168 'click', function(e){
169 e.preventDefault();
170 duplicate_form_overlay.show();
171+ Y.DOM.byId('field.duplicateof').focus();
172 });
173 },
174 failure: function(id, request) {
175
176=== modified file 'lib/lp/bugs/model/bug.py'
177--- lib/lp/bugs/model/bug.py 2010-07-21 12:30:46 +0000
178+++ lib/lp/bugs/model/bug.py 2010-07-21 16:39:54 +0000
179@@ -1503,7 +1503,14 @@
180 field._validate(duplicate_of)
181 if self.duplicates:
182 for duplicate in self.duplicates:
183+ # Fire a notify event in model code since moving
184+ # duplicates of a duplicate does not normally fire an
185+ # event.
186+ dupe_before = Snapshot(
187+ duplicate, providing=providedBy(duplicate))
188 duplicate.markAsDuplicate(duplicate_of)
189+ notify(ObjectModifiedEvent(
190+ duplicate, dupe_before, 'duplicateof'))
191 self.duplicateof = duplicate_of
192 except LaunchpadValidationError, validation_error:
193 raise InvalidDuplicateValue(validation_error)
194
195=== modified file 'lib/lp/bugs/templates/bug-portlet-actions.pt'
196--- lib/lp/bugs/templates/bug-portlet-actions.pt 2009-10-29 21:39:12 +0000
197+++ lib/lp/bugs/templates/bug-portlet-actions.pt 2010-07-21 16:39:54 +0000
198@@ -11,10 +11,11 @@
199 tal:define="link context_menu/markduplicate"
200 tal:condition="python: link.enabled and not
201 context.duplicateof"
202+ class="sprite bug-dupe"
203 >
204 <a
205 tal:attributes="href link/path"
206- class="menu-link-mark-dupe sprite bug-dupe">Mark as duplicate</a>
207+ class="menu-link-mark-dupe">Mark as duplicate</a>
208 </span>
209 <tal:block
210 tal:condition="context/duplicates"
211@@ -31,8 +32,7 @@
212 id="change_duplicate_bug"
213 title="Edit or remove linked duplicate bug"
214 class="sprite edit"
215- tal:attributes="href link/url"></a>
216- <span id="mark-duplicate-text">Duplicate of
217+ tal:attributes="href link/url"></a><span id="mark-duplicate-text">Duplicate of
218 <a
219 tal:condition="duplicateof/required:launchpad.View"
220 tal:attributes="href duplicateof/fmt:url; title
221
222=== modified file 'lib/lp/bugs/windmill/tests/test_mark_duplicate.py'
223--- lib/lp/bugs/windmill/tests/test_mark_duplicate.py 2010-02-18 10:51:34 +0000
224+++ lib/lp/bugs/windmill/tests/test_mark_duplicate.py 2010-07-21 16:39:54 +0000
225@@ -44,10 +44,13 @@
226 client.waits.forElement(
227 xpath=MAIN_FORM_ELEMENT, timeout=constants.FOR_ELEMENT)
228
229- # Initially the form overlay is hidden
230+ # Initially the form overlay is hidden...
231 client.asserts.assertElemJS(
232 xpath=MAIN_FORM_ELEMENT, js=FORM_NOT_VISIBLE)
233
234+ # ...and there is an expandable form for editing bug status, etc.
235+ client.asserts.assertNode(classname='bug-status-expand')
236+
237 # Clicking on the mark duplicate link brings up the formoverlay.
238 # Entering 1 as the duplicate ID changes the duplicate text.
239 client.click(classname=u'menu-link-mark-dupe')
240@@ -66,7 +69,7 @@
241 id='warning-comment-on-duplicate', timeout=constants.FOR_ELEMENT)
242
243 # The duplicate can be cleared:
244- client.click(classname=u'menu-link-mark-dupe')
245+ client.click(id=u'mark-duplicate-text')
246 client.type(text=u'', id=u'field.duplicateof')
247 client.click(xpath=CHANGE_BUTTON)
248 client.waits.forElement(
249@@ -77,7 +80,7 @@
250 client.asserts.assertNotNode(id='warning-comment-on-duplicate')
251
252 # Entering a false bug number results in input validation errors
253- client.click(classname=u'menu-link-mark-dupe')
254+ client.click(id=u'mark-duplicate-text')
255 client.type(text=u'123', id=u'field.duplicateof')
256 client.click(xpath=CHANGE_BUTTON)
257 error_xpath = (
258@@ -105,6 +108,14 @@
259 xpath=u"//h1[@id='bug-title']/span[1]",
260 validator=u'Firefox does not support SVG')
261
262+ # If someone wants to set the master to dupe another bug, there
263+ # is a warning in the dupe widget about this bug having its own
264+ # duplicates.
265+ client.click(classname='menu-link-mark-dupe')
266+ client.asserts.assertTextIn(
267+ classname='large-warning', validator=u'This bug has duplicates',
268+ timeout=constants.FOR_ELEMENT)
269+
270 # When we go back to the page for the duplicate bug...
271 client.open(url=u'http://bugs.launchpad.dev:8085/bugs/15')
272 client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
273@@ -115,6 +126,9 @@
274 # as the one we saw before.
275 client.asserts.assertNode(id='warning-comment-on-duplicate')
276
277+ # Duplicate pages also do not have the expandable form on them.
278+ client.asserts.assertNotNode(classname='bug-status-expand')
279+
280 # Once we remove the duplicate mark...
281 client.click(id=u'change_duplicate_bug')
282 client.type(text=u'', id=u'field.duplicateof')