launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/dupe-project-warning-317258 into lp:launchpad.

Requested reviews:
  Curtis Hovey (sinzui)
Related bugs:
  Bug #317258 in Launchpad itself: "Warn users when trying to dupe a bug in a different project"
  https://bugs.launchpad.net/launchpad/+bug/317258

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/dupe-project-warning-317258/+merge/118287

== Implementation ==

When a bug search is done for the bug picker, a new optional related_bug parameter is passed in. The json search results data contains a new 'different_pillars' attribute which is true if the search result bug and related bug do not have any target pillars in common.

The ui displays a warning if the bug data says that the proposed dupe and the context bug have no pillars in common.

== Demo and QA ==

http://people.canonical.com/~ianb/different-project-warning.png
http://people.canonical.com/~ianb/different-project-warning2.png

== Tests ==

Enhance bug picker and duplicate bug yui tests.
Enhance getBugData() unit tests.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/systemhomes.py
  lib/lp/bugs/browser/tests/test_bugs.py
  lib/lp/bugs/interfaces/malone.py
  lib/lp/bugs/javascript/bug_picker.js
  lib/lp/bugs/javascript/duplicates.js
  lib/lp/bugs/javascript/tests/test_bug_picker.js
  lib/lp/bugs/javascript/tests/test_duplicates.js

-- 
https://code.launchpad.net/~wallyworld/launchpad/dupe-project-warning-317258/+merge/118287
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/bugs/browser/tests/test_bugs.py'
--- lib/lp/bugs/browser/tests/test_bugs.py	2012-07-27 08:01:54 +0000
+++ lib/lp/bugs/browser/tests/test_bugs.py	2012-08-06 03:03:22 +0000
@@ -117,16 +117,19 @@
         # we should get some valid content out of this
         self.assertIn('Search all bugs', content)
 
-    def test_getBugData(self):
+    def _assert_getBugData(self, related_bug=None):
         # The getBugData method works as expected.
         owner = self.factory.makePerson()
+        product = self.factory.makeProduct()
         bug = self.factory.makeBug(
+            product=product,
             owner=owner,
             status=BugTaskStatus.INPROGRESS,
             title='title', description='description',
             information_type=InformationType.PRIVATESECURITY)
         with person_logged_in(owner):
-            bug_data = getUtility(IMaloneApplication).getBugData(owner, bug.id)
+            bug_data = getUtility(IMaloneApplication).getBugData(
+                owner, bug.id, related_bug)
             expected_bug_data = {
                     'id': bug.id,
                     'information_type': 'Private Security',
@@ -137,6 +140,16 @@
                     'status_class': 'statusINPROGRESS',
                     'bug_summary': 'title',
                     'description': 'description',
-                    'bug_url': canonical_url(bug.default_bugtask)
+                    'bug_url': canonical_url(bug.default_bugtask),
+                    'different_pillars': related_bug is not None
         }
         self.assertEqual([expected_bug_data], bug_data)
+
+    def test_getBugData(self):
+        # The getBugData method works as expected without a related_bug.
+        self._assert_getBugData()
+
+    def test_getBugData_with_related_bug(self):
+        # The getBugData method works as expected if related bug is specified.
+        related_bug = self.factory.makeBug()
+        self._assert_getBugData(related_bug)

=== modified file 'lib/lp/bugs/interfaces/malone.py'
--- lib/lp/bugs/interfaces/malone.py	2012-07-27 08:01:54 +0000
+++ lib/lp/bugs/interfaces/malone.py	2012-08-06 03:03:22 +0000
@@ -41,11 +41,12 @@
 
     @call_with(user=REQUEST_USER)
     @operation_parameters(
-        bug_id=copy_field(IBug['id'])
+        bug_id=copy_field(IBug['id']),
+        related_bug=Reference(schema=IBug)
     )
     @export_read_operation()
     @operation_for_version('devel')
-    def getBugData(user, bug_id):
+    def getBugData(user, bug_id, related_bug=None):
         """Search bugtasks matching the specified criteria.
 
         The only criteria currently supported is to search for a bugtask with

=== modified file 'lib/lp/bugs/javascript/bug_picker.js'
--- lib/lp/bugs/javascript/bug_picker.js	2012-08-03 12:47:11 +0000
+++ lib/lp/bugs/javascript/bug_picker.js	2012-08-06 03:03:22 +0000
@@ -116,6 +116,8 @@
             = Y.lp.client.append_qs("", "ws.accept", "application.json");
         qs_data = Y.lp.client.append_qs(qs_data, "ws.op", "getBugData");
         qs_data = Y.lp.client.append_qs(qs_data, "bug_id", bug_id);
+        qs_data = Y.lp.client.append_qs(
+            qs_data, "related_bug", LP.cache.bug.self_link);
 
         var that = this;
         var config = {
@@ -152,7 +154,7 @@
     // Template for rendering the bug details.
     _bug_details_template: function() {
         return [
-        '<table><tbody><tr><td>',
+        '<table class="confirm-bug-details"><tbody><tr><td>',
         '<div id="client-listing">',
         '  <div class="buglisting-col1">',
         '      <div class="importance {{importance_class}}">',

=== modified file 'lib/lp/bugs/javascript/duplicates.js'
--- lib/lp/bugs/javascript/duplicates.js	2012-08-03 12:47:11 +0000
+++ lib/lp/bugs/javascript/duplicates.js	2012-08-06 03:03:22 +0000
@@ -41,6 +41,32 @@
         });
     },
 
+    _syncResultsUI: function() {
+        Y.lp.bugs.bug_picker.BugPicker.prototype._syncResultsUI.call(this);
+        // Display warning about different project if required.
+        var bug_data = this.get('results');
+        if (!bug_data.length) {
+            this._hide_bug_results();
+            return;
+        }
+        bug_data = bug_data[0];
+        if (!bug_data.different_pillars) {
+            return;
+        }
+        var bug_details_table = this._results_box.one(
+            'table.confirm-bug-details tbody');
+        var different_project_warning =
+            '<p id="different-project-warning" ' +
+            'class="block-sprite large-warning"><strong>' +
+            'This bug affects a different project to the bug you ' +
+            'are specifying here.' +
+            '</strong></p>';
+        var warning_node_row = Y.Node.create('<tr></tr>');
+        warning_node_row.appendChild(
+            Y.Node.create('<td></td>').setContent(different_project_warning));
+        bug_details_table.appendChild(warning_node_row);
+    },
+
     _bug_search_header: function() {
         var search_header = '<p class="search-header">' +
             'Marking this bug as a duplicate will, ' +

=== modified file 'lib/lp/bugs/javascript/tests/test_bug_picker.js'
--- lib/lp/bugs/javascript/tests/test_bug_picker.js	2012-08-03 12:47:11 +0000
+++ lib/lp/bugs/javascript/tests/test_bug_picker.js	2012-08-06 03:03:22 +0000
@@ -17,10 +17,12 @@
                 Y.Assert.areEqual(
                     'file:///api/devel/bugs',
                     this.mockio.last_request.url);
+                var bug_uri = encodeURIComponent('api/devel/bugs/1');
                 Y.Assert.areEqual(
                     this.mockio.last_request.config.data,
                     'ws.accept=application.json&ws.op=getBugData&' +
-                    'bug_id=' + bug_id);
+                    'bug_id=' + bug_id +
+                    '&related_bug=' + bug_uri);
             } else {
                 Y.Assert.areEqual(
                     '/api/devel/bugs/1', this.mockio.last_request.url);
@@ -47,7 +49,8 @@
                 bug_summary: 'dupe title',
                 is_private: is_private,
                 duplicate_of_link: 'api/devel/bugs/' + bug_id,
-                self_link: 'api/devel/bugs/' + bug_id}];
+                self_link: 'api/devel/bugs/' + bug_id,
+                different_pillars: this.different_pillars}];
             this.mockio.last_request.successJSON(expected_updated_entry);
             this._assert_form_state(true);
         },

=== modified file 'lib/lp/bugs/javascript/tests/test_duplicates.js'
--- lib/lp/bugs/javascript/tests/test_duplicates.js	2012-08-02 13:52:58 +0000
+++ lib/lp/bugs/javascript/tests/test_duplicates.js	2012-08-06 03:03:22 +0000
@@ -127,6 +127,20 @@
             this._assert_form_state(false);
         },
 
+        // A warning is displayed for search results which are not targeted to
+        // the same project.
+        test_different_pillars: function() {
+            this.widget = this._createWidget(false);
+            this.different_pillars = true;
+            this._assert_search_form_submission(4);
+            this._assert_search_form_success(4);
+            var privacy_message = Y.one('#different-project-warning');
+            Y.Assert.areEqual(
+                'This bug affects a different project to the bug ' +
+                'you are specifying here.',
+                privacy_message.get('text').trim());
+        },
+
         // The expected data is submitted after searching for and selecting a
         // bug.
         _assert_dupe_submission: function(bug_id) {

=== modified file 'lib/lp/systemhomes.py'
--- lib/lp/systemhomes.py	2012-07-27 08:01:54 +0000
+++ lib/lp/systemhomes.py	2012-08-06 03:03:22 +0000
@@ -128,7 +128,7 @@
         """See `IMaloneApplication`."""
         return getUtility(IBugTaskSet).search(search_params)
 
-    def getBugData(self, user, bug_id):
+    def getBugData(self, user, bug_id, related_bug=None):
         """See `IMaloneApplication`."""
         search_params = BugTaskSearchParams(user, bug=bug_id)
         bugtasks = getUtility(IBugTaskSet).search(search_params)
@@ -138,6 +138,9 @@
         data = []
         for bug in bugs:
             bugtask = bug.default_bugtask
+            different_pillars = related_bug and (
+                set(bug.affected_pillars).isdisjoint(
+                    related_bug.affected_pillars)) or False
             data.append({
                 'id': bug_id,
                 'information_type': bug.information_type.title,
@@ -149,7 +152,8 @@
                 'status_class': 'status' + bugtask.status.name,
                 'bug_summary': bug.title,
                 'description': bug.description,
-                'bug_url': canonical_url(bugtask)})
+                'bug_url': canonical_url(bugtask),
+                'different_pillars': different_pillars})
         return data
 
     def createBug(self, owner, title, description, target,