Merge lp:~wallyworld/launchpad/remove-dupe-link-133622 into lp:launchpad

[Ian Booth]

Add a Reemove button/link to the HTML bug +duplicate form and the Javascript bug duplicate popup.

== Implementation ==

The bug duplicate forms (both Javascript and HTML) required the entry of a blank duplicate followed by clicking Save/Change in order to remove a bug duplicate. The bug id field on the form was optional.

The branch makes the bug id field mandatory and adds an explicit 'Remove Duplicate' link. For the Javascript form, the link is centred and rendered similar to the equivalent link in the person picker. For the HTML form, a submit button is rendered.

1. Javascript form

I'm not sure if the link should be centred. But I'm not sure it looks great left aligned either. Thoughts?

2. HTML form

I changed the validation processing for the form so that the bug id field is only validated when 'Save' is clicked. No error is raised if 'Remove' is clicked. This allows the confusing "Optional" label to be removed.

The save button was called 'Change' but I renamed it to 'Set Duplicate'. This pairs up nicely with the 'Remove Duplicate' button. The remove button is only rendered if the bug has a duplicate. Also, I don't understand why the Cancel link was missing so I added that too.

== Demo and QA ==

Screenshots of the new forms:

http://people.canonical.com/~ianb/bug-duplicate-form.png
http://people.canonical.com/~ianb/bug-duplicate-popup.png

== Tests ==

I found some incidental doctests for the +duplicate functionality but nothing that seemed specifically targetted to test that functionality. I created a new test case TestBugMarkAsDuplicateView and some tests

The yui tests for the javascript duplicate form were also updated, as well as some doctests.

== Lint ==

Clean except for some doctest noise.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bug.py
  lib/lp/bugs/browser/tests/test_bug_views.py
  lib/lp/bugs/javascript/bug_picker.js
  lib/lp/bugs/javascript/bugtask_index.js
  lib/lp/bugs/javascript/tests/test_duplicates.js
  lib/lp/bugs/model/bugsubscriptionfilterimportance.py
  lib/lp/bugs/model/bugsubscriptionfilterstatus.py
  lib/lp/bugs/model/tests/test_bugsubscriptionfilterimportance.py
  lib/lp/bugs/model/tests/test_bugsubscriptionfilterstatus.py
  lib/lp/bugs/stories/bugs/xx-duplicate-of-private-bug.txt
  lib/lp/bugs/stories/guided-filebug/xx-displaying-similar-bugs.txt

[Curtis Hovey (sinzui)]

Thank you. This looks fine to land I have one suggestion

Replace
    <div style="text-align: center">
with
    <div class="center">

review: Approve (code)

=== modified file 'lib/lp/bugs/browser/bug.py'
--- lib/lp/bugs/browser/bug.py	2012-07-27 04:24:59 +0000
+++ lib/lp/bugs/browser/bug.py	2012-07-31 23:26:22 +0000
@@ -782,12 +782,17 @@
     label = "Mark bug report as a duplicate"
     page_title = label
 
+    @property
+    def cancel_url(self):
+        """See `LaunchpadFormView`."""
+        return canonical_url(self.context)
+
     def setUpFields(self):
         """Make the readonly version of duplicateof available."""
         super(BugMarkAsDuplicateView, self).setUpFields()
 
         duplicateof_field = DuplicateBug(
-            __name__='duplicateof', title=_('Duplicate Of'), required=False)
+            __name__='duplicateof', title=_('Duplicate Of'), required=True)
 
         self.form_fields = self.form_fields.omit('duplicateof')
         self.form_fields = formlib.form.Fields(duplicateof_field)
@@ -797,7 +802,12 @@
         """See `LaunchpadFormView.`"""
         return {'duplicateof': self.context.bug.duplicateof}
 
-    @action('Change', name='change')
+    def _validate(self, action, data):
+        if action.name != 'remove':
+            return super(BugMarkAsDuplicateView, self)._validate(action, data)
+        return []
+
+    @action('Set Duplicate', name='change')
     def change_action(self, action, data):
         """Update the bug."""
         data = dict(data)
@@ -812,6 +822,19 @@
         # Apply other changes.
         self.updateBugFromData(data)
 
+    def shouldShowRemoveButton(self, action):
+        return self.context.bug.duplicateof is not None
+
+    @action('Remove Duplicate', name='remove',
+        condition=shouldShowRemoveButton)
+    def remove_action(self, action, data):
+        """Update the bug."""
+        bug = self.context.bug
+        bug_before_modification = Snapshot(bug, providing=providedBy(bug))
+        bug.markAsDuplicate(None)
+        notify(
+            ObjectModifiedEvent(bug, bug_before_modification, 'duplicateof'))
+
 
 class BugSecrecyEditView(LaunchpadFormView, BugSubscriptionPortletDetails):
     """Form for marking a bug as a private/public."""

=== modified file 'lib/lp/bugs/browser/tests/test_bug_views.py'
--- lib/lp/bugs/browser/tests/test_bug_views.py	2012-07-26 07:54:40 +0000
+++ lib/lp/bugs/browser/tests/test_bug_views.py	2012-07-31 23:26:22 +0000
@@ -515,3 +515,55 @@
         view = create_initialized_view(
             bug.default_bugtask, '+addcomment', form=form)
         self.assertEqual(0, len(view.errors))
+
+
+class TestBugMarkAsDuplicateView(TestCaseWithFactory):
+    """Tests for marking a bug as a duplicate."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBugMarkAsDuplicateView, self).setUp()
+        self.bug_owner = self.factory.makePerson()
+        self.bug = self.factory.makeBug(owner=self.bug_owner)
+        self.duplicate_bug = self.factory.makeBug(owner=self.bug_owner)
+
+    def test_remove_link_not_shown_if_no_duplicate(self):
+        with person_logged_in(self.bug_owner):
+            view = create_initialized_view(
+                self.bug.default_bugtask, name="+duplicate",
+                principal=self.bug_owner)
+            soup = BeautifulSoup(view.render())
+        self.assertIsNone(soup.find(attrs={'id': 'field.actions.remove'}))
+
+    def test_remove_link_shown_if_duplicate(self):
+        with person_logged_in(self.bug_owner):
+            self.bug.markAsDuplicate(self.duplicate_bug)
+            view = create_initialized_view(
+                self.bug.default_bugtask, name="+duplicate",
+                principal=self.bug_owner)
+            soup = BeautifulSoup(view.render())
+        self.assertIsNotNone(
+            soup.find(attrs={'id': 'field.actions.remove'}))
+
+    def test_create_duplicate(self):
+        with person_logged_in(self.bug_owner):
+            form = {
+                'field.actions.change': u'Set Duplicate',
+                'field.duplicateof': u'%s' % self.duplicate_bug.id
+                }
+            create_initialized_view(
+                self.bug.default_bugtask, name="+duplicate",
+                principal=self.bug_owner, form=form)
+        self.assertEqual(self.duplicate_bug, self.bug.duplicateof)
+
+    def test_remove_duplicate(self):
+        with person_logged_in(self.bug_owner):
+            self.bug.markAsDuplicate(self.duplicate_bug)
+            form = {
+                'field.actions.remove': u'Remove Duplicate',
+                }
+            create_initialized_view(
+                self.bug.default_bugtask, name="+duplicate",
+                principal=self.bug_owner, form=form)
+        self.assertIsNone(self.bug.duplicateof)

=== modified file 'lib/lp/bugs/javascript/bug_picker.js'
--- lib/lp/bugs/javascript/bug_picker.js	2012-07-30 23:55:05 +0000
+++ lib/lp/bugs/javascript/bug_picker.js	2012-07-31 23:26:22 +0000
@@ -33,6 +33,15 @@
         }
     },
 
+    _remove_link_html: function() {
+        return [
+            '<div class="centered">',
+            '<a class="sprite remove ',
+            'js-action" href="javascript:void(0)">',
+            this.get('remove_link_text'),
+            '</a></div>'].join('');
+    },
+
     renderUI: function() {
         var select_bug_link = this.get('select_bug_link');
         if (!Y.Lang.isValue(select_bug_link)) {
@@ -56,6 +65,7 @@
                 'the bug you specify here.  This cannot be undone.' +
                 '</p>';
         }
+        form_header = form_header + this._remove_link_html();
 
         this.find_button = Y.Node.create(find_button_html);
         this.picker_form = new Y.lazr.FormOverlay({
@@ -92,12 +102,31 @@
                 Y.DOM.byId('field.duplicateof').focus();
             }
         });
+        // Wire up the Remove link.
+        this.picker_form.after('contentUpdate', function() {
+            var remove_link = that.picker_form.form_header_node
+                .one('a.remove');
+            if (Y.Lang.isValue(remove_link)) {
+                remove_link.on('click', function(e) {
+                    e.halt();
+                    that._submit_bug('');
+                });
+            }
+        });
         // When the dupe form overlay is hidden, we need to reset the form.
         this.picker_form.after('visibleChange', function() {
             if (!this.get('visible')) {
                 that._hide_bug_details_node();
             } else {
                 Y.DOM.byId('field.duplicateof').value = '';
+                var remove_link = that.picker_form.form_header_node
+                    .one('a.remove');
+                var existing_dupe = LP.cache.bug.duplicate_of_link;
+                if (Y.Lang.isString(existing_dupe) && existing_dupe !== '') {
+                    remove_link.removeClass('hidden');
+                } else {
+                    remove_link.addClass('hidden');
+                }
             }
         });
     },
@@ -130,7 +159,8 @@
         // If there's no bug data entered then we are deleting the duplicate
         // link.
         if (new_dup_id === '') {
-            this._submit_bug(new_dup_id);
+            this.picker_form.showError(
+                'Please enter a valid bug number.');
             return;
         }
 
@@ -364,6 +394,7 @@
                                            new_dup_id) {
         this.picker_form.hide();
         updated_entry.set('duplicate_of_link', new_dup_url);
+        LP.cache.bug.duplicate_of_link = updated_entry.duplicate_of_link;
         this.set('lp_bug_entry', updated_entry);
 
         var dupe_span = this.get('dupe_span').ancestor('li');
@@ -572,6 +603,10 @@
                 return Y.one('#portlet-duplicates');
             }
         },
+        // The text used for the remove link.
+        remove_link_text: {
+            value: "Remove this bug"
+        },
         // Override for testing.
         use_animation: {
             value: true

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2012-07-30 23:49:42 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2012-07-31 23:26:22 +0000
@@ -42,6 +42,7 @@
         var dup_widget = new Y.lp.bugs.bug_picker.BugPicker({
             srcNode: '#duplicate-actions',
             lp_bug_entry: lp_bug_entry,
+            remove_link_text: 'Bug is not a duplicate',
             private_warning_message:
                 'Marking this bug as a duplicate of a private bug means '+
                 'that it won\'t be visible to contributors and encourages '+

=== modified file 'lib/lp/bugs/javascript/tests/test_duplicates.js'
--- lib/lp/bugs/javascript/tests/test_duplicates.js	2012-07-30 23:49:42 +0000
+++ lib/lp/bugs/javascript/tests/test_duplicates.js	2012-07-31 23:26:22 +0000
@@ -86,10 +86,14 @@
             Y.Assert.areEqual('http://foo/+duplicate', url);
             Y.Assert.isNotNull(
                 Y.one('#mark-duplicate-text a.menu-link-mark-dupe'));
+            this.widget.get('select_bug_link').simulate('click');
+            var remove_dupe = Y.one('#duplicate-form-container a.remove');
+            Y.Assert.isTrue(remove_dupe.hasClass('hidden'));
         },
 
         // The widget is created when there are bug duplicates.
         test_widget_creation_duplicate_exists: function() {
+            window.LP.cache.bug.duplicate_of_link = 'bug/5';
             this.widget = this._createWidget(true);
             Y.Assert.isInstanceOf(
                 Y.lp.bugs.bug_picker.BugPicker,
@@ -97,6 +101,9 @@
                 "Mark bug duplicate failed to be instantiated");
             var url = this.widget.get('select_bug_link').get('href');
             Y.Assert.areEqual('http://foo/+duplicate', url);
+            this.widget.get('select_bug_link').simulate('click');
+            var remove_dupe = Y.one('#duplicate-form-container a.remove');
+            Y.Assert.isFalse(remove_dupe.hasClass('hidden'));
         },
 
         // The search form renders and submits the expected data.
@@ -149,6 +156,20 @@
             this._assert_form_state(true);
         },
 
+        // Attempt to enter an empty bug number and an error is displayed.
+        test_no_bug_id_entered: function() {
+            this.widget = this._createWidget(false);
+            this.widget.get('select_bug_link').simulate('click');
+            this.mockio.last_request = null;
+            Y.DOM.byId('field.duplicateof').value = '';
+            var form = Y.one('#duplicate-form-container');
+            form.one('[name="field.actions.find"]').simulate('click');
+            Y.Assert.isNull(this.mockio.last_request);
+            this._assert_error_display(
+                'Please enter a valid bug number.');
+            this._assert_form_state(false);
+        },
+
         // Attempt to make a bug as a duplicate of itself is detected and an
         // error is displayed immediately.
         test_mark_bug_as_dupe_of_self: function() {
@@ -342,7 +363,6 @@
         // Submitting a dupe removal request works as expected.
         test_picker_form_submission_remove_dupe: function() {
             this.widget = this._createWidget(false);
-            this._assert_search_form_submission('');
             var success_called = false;
             this.widget._submit_bug_success =
                 function(updated_entry, new_dup_url, new_dup_id) {
@@ -351,6 +371,8 @@
                     Y.Assert.areEqual('', new_dup_id);
                     success_called = true;
                 };
+            var remove_dupe = Y.one('#duplicate-form-container a.remove');
+            remove_dupe.simulate('click');
             var expected_updated_entry =
                 '{"duplicate_of_link":""}';
             this.mockio.success({

=== modified file 'lib/lp/bugs/stories/bugs/xx-duplicate-of-private-bug.txt'
--- lib/lp/bugs/stories/bugs/xx-duplicate-of-private-bug.txt	2012-07-17 14:29:17 +0000
+++ lib/lp/bugs/stories/bugs/xx-duplicate-of-private-bug.txt	2012-07-31 23:26:22 +0000
@@ -26,7 +26,7 @@
     ...     'http://bugs.launchpad.dev/'
     ...         'ubuntu/+source/linux-source-2.6.15/+bug/10/+duplicate')
     >>> admin_browser.getControl('Duplicate Of').value = '8'
-    >>> admin_browser.getControl('Change').click()
+    >>> admin_browser.getControl('Set Duplicate').click()
 
 As a privileged user the title of the private bug can be found in the
 duplicate bug page:

=== modified file 'lib/lp/bugs/stories/guided-filebug/xx-displaying-similar-bugs.txt'
--- lib/lp/bugs/stories/guided-filebug/xx-displaying-similar-bugs.txt	2011-07-19 10:17:47 +0000
+++ lib/lp/bugs/stories/guided-filebug/xx-displaying-similar-bugs.txt	2012-07-31 23:26:22 +0000
@@ -66,7 +66,7 @@
     >>> user_browser.open(
     ...     "http://bugs.launchpad.dev/firefox/+bug/4/+duplicate")
     >>> user_browser.getControl('Duplicate Of').value = '8'
-    >>> user_browser.getControl('Change').click()
+    >>> user_browser.getControl('Set Duplicate').click()
 
 Then, if we match bug 1, only basic details of bug 8 are displayed:
 
@@ -110,7 +110,7 @@
         >>> user_browser.open(
         ...     "http://bugs.launchpad.dev/evolution/+bug/7/+duplicate")
         >>> user_browser.getControl('Duplicate Of').value = '8'
-        >>> user_browser.getControl('Change').click()
+        >>> user_browser.getControl('Set Duplicate').click()
 
         >>> user_browser.open("http://bugs.launchpad.dev/gnome/+filebug")
         >>> user_browser.getControl("Project", index=0).value = ['evolution']
@@ -145,7 +145,7 @@
         ...     "http://bugs.launchpad.dev/ubuntu/+source/"
         ...     "thunderbird/+bug/9/+duplicate")
         >>> user_browser.getControl('Duplicate Of').value = '8'
-        >>> user_browser.getControl('Change').click()
+        >>> user_browser.getControl('Set Duplicate').click()
 
         >>> user_browser.open("http://launchpad.dev/ubuntu/+filebug")
         >>> user_browser.getControl("Summary", index=0).value = 'crashes'
@@ -181,7 +181,7 @@
         >>> user_browser.open(
         ...     "http://bugs.launchpad.dev/firefox/+bug/1/+duplicate")
         >>> user_browser.getControl('Duplicate Of').value = '8'
-        >>> user_browser.getControl('Change').click()
+        >>> user_browser.getControl('Set Duplicate').click()
 
         >>> user_browser.open(
         ...     "http://launchpad.dev/ubuntu/+source/"