launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00166
[Merge] lp:~deryck/launchpad/better-dupe-handling-ui into lp:launchpad/devel
Deryck Hodge has proposed merging lp:~deryck/launchpad/better-dupe-handling-ui into lp:launchpad/devel with lp:~deryck/launchpad/do-the-right-thing-dupe-move-78596 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#502881 Clicking "Mark as duplicate" doesn't focus the bug number field
https://bugs.launchpad.net/bugs/502881
#591705 Add confirmation dialog when marking duplicate a bug that itself has duplicates
https://bugs.launchpad.net/bugs/591705
#604419 changing the master bug of a duplicate bug will make a second 'edit' button appear
https://bugs.launchpad.net/bugs/604419
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
--
https://code.launchpad.net/~deryck/launchpad/better-dupe-handling-ui/+merge/30309
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~deryck/launchpad/better-dupe-handling-ui into lp:launchpad/devel.
=== 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-19 17:41:52 +0000
@@ -1806,6 +1806,12 @@
*/
display: inline;
}
+body.tab-bugs #duplicate-actions .sprite {
+ /* Override sprite style for edit icon on "Mark as duplicate"
+ to make the text not appear on a second line.
+ */
+ display:inline;
+ }
.yui-picker-results li.sprite {
/* XXX: EdwinGrubbs 2009-07-27 bug=405476
* Override the .yui-picker-results style, so that the next icon
=== modified file 'lib/lp/bugs/doc/bugactivity.txt'
--- lib/lp/bugs/doc/bugactivity.txt 2010-07-19 17:41:50 +0000
+++ lib/lp/bugs/doc/bugactivity.txt 2010-07-19 17:41:52 +0000
@@ -136,7 +136,7 @@
True
-== Bug report has itss duplicate marker changed to another bug report ==
+== Bug report has its duplicate marker changed to another bug report ==
>>> edit_fields = [
... "id", "title", "description", "name", "private", "duplicateof",
@@ -173,6 +173,46 @@
True
+== A bug with multiple duplicates ==
+
+When a bug has multiple duplicates and is itself marked a duplicate,
+the duplicates are automatically duped to the same master bug. These changes
+are then reflected in the activity log for each bug itself.
+
+ >>> edit_fields = [
+ ... "id", "title", "description", "name", "private", "duplicateof",
+ ... "security_related"]
+ >>> initial_bug = factory.makeBug()
+ >>> dupe_one = factory.makeBug()
+ >>> dupe_two = factory.makeBug()
+ >>> dupe_one.markAsDuplicate(initial_bug)
+ >>> dupe_two.markAsDuplicate(initial_bug)
+
+After creating a few bugs to work with, we create a final bug and duplicate
+the initial bug against it.
+
+ >>> final_bug = factory.makeBug()
+ >>> initial_bug.markAsDuplicate(final_bug)
+
+Now, we confirm the activity log for the other bugs correctly list the
+final_bug as their master bug.
+
+ >>> latest_activity = dupe_one.activity[-1]
+ >>> print latest_activity.whatchanged
+ changed duplicate marker
+ >>> latest_activity.oldvalue == unicode(initial_bug.id)
+ True
+ >>> latest_activity.newvalue == unicode(final_bug.id)
+ True
+ >>> latest_activity = dupe_two.activity[-1]
+ >>> print latest_activity.whatchanged
+ changed duplicate marker
+ >>> latest_activity.oldvalue == unicode(initial_bug.id)
+ True
+ >>> latest_activity.newvalue == unicode(final_bug.id)
+ True
+
+
== BugActivityItem ==
BugActivityItem implements the stuff that BugActivity doesn't need to
=== 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-19 17:41:52 +0000
@@ -165,10 +165,21 @@
update_dupe_url = update_dupe_link.get('href');
var mark_dupe_form_url = update_dupe_url + '/++form++';
+ var form_header = '<p>Marking this bug as a duplicate will, by default, ' +
+ 'hide it from search results listings.</p>';
+
+ var has_dupes = Y.one('#portlet-duplicates');
+ if (has_dupes !== null) {
+ form_header = form_header +
+ '<p style="padding:2px 2px 0 36px;" ' +
+ 'class="large-warning">This bug has duplicates. ' +
+ 'These duplicates will be moved to become duplicates of any ' +
+ 'bug ID provided here. This cannot be undone.</p></div>';
+ }
+
duplicate_form_overlay = new Y.lazr.FormOverlay({
headerContent: '<h2>Mark bug report as duplicate</h2>',
- form_header: 'Marking the bug as a duplicate will, by default, ' +
- 'hide it from search results listings.',
+ form_header: form_header,
form_submit_button: Y.Node.create(submit_button_html),
form_cancel_button: Y.Node.create(cancel_button_html),
centered: true,
@@ -187,6 +198,7 @@
if (duplicate_form_overlay){
e.preventDefault();
duplicate_form_overlay.show();
+ Y.DOM.byId('field.duplicateof').focus();
}
});
// Add a class denoting them as js-action links.
@@ -366,8 +378,15 @@
// Hide the formoverlay:
duplicate_form_overlay.hide();
+ // Hide the dupe edit icon if it exists.
+ var dupe_edit_icon = Y.one('#change_duplicate_bug');
+ if (dupe_edit_icon !== null) {
+ dupe_edit_icon.setStyle('display', 'none');
+ }
+
// Add the spinner...
var dupe_span = Y.one('#mark-duplicate-text');
+ dupe_span.removeClass('sprite bug-dupe');
dupe_span.addClass('update-in-progress-message');
// Set the new duplicate link on the bug entry.
@@ -390,28 +409,35 @@
if (new_dup_url !== null) {
dupe_span.set('innerHTML', [
- 'Duplicate of <a>bug #</a> ',
- '<a class="menu-link-mark-dupe js-action sprite edit">',
- '<span class="invisible-link">edit</span></a>'].join(""));
+ '<a id="change_duplicate_bug" ',
+ 'title="Edit or remove linked duplicate bug" ',
+ 'class="sprite edit"></a>',
+ 'Duplicate of <a>bug #</a>'].join(""));
dupe_span.all('a').item(0)
+ .set('href', update_dupe_url);
+ dupe_span.all('a').item(1)
.set('href', '/bugs/' + new_dup_id)
.appendChild(document.createTextNode(new_dup_id));
- dupe_span.all('a').item(1)
- .set('href', update_dupe_url);
+ var has_dupes = Y.one('#portlet-duplicates');
+ if (has_dupes !== null) {
+ has_dupes.get('parentNode').removeChild(has_dupes);
+ }
show_comment_on_duplicate_warning();
} else {
+ dupe_span.addClass('sprite bug-dupe');
dupe_span.set('innerHTML', [
- '<a class="menu-link-mark-dupe js-action ',
- 'sprite bug-dupe">Mark as duplicate</a>'].join(""));
+ '<a class="menu-link-mark-dupe js-action">',
+ 'Mark as duplicate</a>'].join(""));
dupe_span.one('a').set('href', update_dupe_url);
hide_comment_on_duplicate_warning();
}
Y.lazr.anim.green_flash({node: dupe_span}).run();
// ensure the new link is hooked up correctly:
- dupe_span.one('a.menu-link-mark-dupe').on(
+ dupe_span.one('a').on(
'click', function(e){
e.preventDefault();
duplicate_form_overlay.show();
+ Y.DOM.byId('field.duplicateof').focus();
});
},
failure: function(id, request) {
=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py 2010-07-19 17:41:50 +0000
+++ lib/lp/bugs/model/bug.py 2010-07-19 17:41:52 +0000
@@ -1503,7 +1503,14 @@
field._validate(duplicate_of)
if self.duplicates:
for duplicate in self.duplicates:
+ # Fire a notify event in model code since moving
+ # duplicates of a duplicate does not normally fire an
+ # event.
+ dupe_before = Snapshot(
+ duplicate, providing=providedBy(duplicate))
duplicate.markAsDuplicate(duplicate_of)
+ notify(
+ ObjectModifiedEvent(duplicate, dupe_before, 'duplicateof'))
self.duplicateof = duplicate_of
except LaunchpadValidationError, validation_error:
raise InvalidDuplicateValue(validation_error)
=== 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-19 17:41:52 +0000
@@ -11,10 +11,11 @@
tal:define="link context_menu/markduplicate"
tal:condition="python: link.enabled and not
context.duplicateof"
+ class="sprite bug-dupe"
>
<a
tal:attributes="href link/path"
- class="menu-link-mark-dupe sprite bug-dupe">Mark as duplicate</a>
+ class="menu-link-mark-dupe">Mark as duplicate</a>
</span>
<tal:block
tal:condition="context/duplicates"
@@ -31,8 +32,7 @@
id="change_duplicate_bug"
title="Edit or remove linked duplicate bug"
class="sprite edit"
- tal:attributes="href link/url"></a>
- <span id="mark-duplicate-text">Duplicate of
+ tal:attributes="href link/url"></a><span id="mark-duplicate-text">Duplicate of
<a
tal:condition="duplicateof/required:launchpad.View"
tal:attributes="href duplicateof/fmt:url; title
=== 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-19 17:41:52 +0000
@@ -44,10 +44,13 @@
client.waits.forElement(
xpath=MAIN_FORM_ELEMENT, timeout=constants.FOR_ELEMENT)
- # Initially the form overlay is hidden
+ # Initially the form overlay is hidden...
client.asserts.assertElemJS(
xpath=MAIN_FORM_ELEMENT, js=FORM_NOT_VISIBLE)
+ # ...and there is an expandable form for editing bug status, etc.
+ client.asserts.assertNode(classname='bug-status-expand')
+
# Clicking on the mark duplicate link brings up the formoverlay.
# Entering 1 as the duplicate ID changes the duplicate text.
client.click(classname=u'menu-link-mark-dupe')
@@ -66,7 +69,7 @@
id='warning-comment-on-duplicate', timeout=constants.FOR_ELEMENT)
# The duplicate can be cleared:
- client.click(classname=u'menu-link-mark-dupe')
+ client.click(id=u'mark-duplicate-text')
client.type(text=u'', id=u'field.duplicateof')
client.click(xpath=CHANGE_BUTTON)
client.waits.forElement(
@@ -77,7 +80,7 @@
client.asserts.assertNotNode(id='warning-comment-on-duplicate')
# Entering a false bug number results in input validation errors
- client.click(classname=u'menu-link-mark-dupe')
+ client.click(id=u'mark-duplicate-text')
client.type(text=u'123', id=u'field.duplicateof')
client.click(xpath=CHANGE_BUTTON)
error_xpath = (
@@ -105,6 +108,14 @@
xpath=u"//h1[@id='bug-title']/span[1]",
validator=u'Firefox does not support SVG')
+ # If someone wants to set the master to dupe another bug, there
+ # is a warning in the dupe widget about this bug having its own
+ # duplicates.
+ client.click(classname='menu-link-mark-dupe')
+ client.asserts.assertTextIn(
+ classname='large-warning', validator=u'This bug has duplicates.',
+ timeout=constants.FOR_ELEMENT)
+
# When we go back to the page for the duplicate bug...
client.open(url=u'http://bugs.launchpad.dev:8085/bugs/15')
client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
@@ -115,6 +126,9 @@
# as the one we saw before.
client.asserts.assertNode(id='warning-comment-on-duplicate')
+ # Duplicate pages also do not have the expandable form on them.
+ client.asserts.assertNotNode(classname='bug-status-expand')
+
# Once we remove the duplicate mark...
client.click(id=u'change_duplicate_bug')
client.type(text=u'', id=u'field.duplicateof')
