launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10336
[Merge] lp:~wallyworld/launchpad/private-dupe-bug-warning3 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/private-dupe-bug-warning3 into lp:launchpad with lp:~wallyworld/launchpad/private-dupe-bug-warning2-943497 as a prerequisite.
Requested reviews:
Curtis Hovey (sinzui)
Related bugs:
Bug #190635 in Launchpad itself: "Cannot tell that you chose the right bug when marking as a duplicate"
https://bugs.launchpad.net/launchpad/+bug/190635
Bug #767084 in Launchpad itself: ""Mark as duplicate" popup gives an unhelpful error when attempted to dupe against a dupe"
https://bugs.launchpad.net/launchpad/+bug/767084
Bug #943497 in Launchpad itself: "warn when you are going to mark a public bug as a duplicate of a private one"
https://bugs.launchpad.net/launchpad/+bug/943497
Bug #1023425 in Launchpad itself: "Duplicate error "not a valid bug number or nickname" when nicknames are defunct"
https://bugs.launchpad.net/launchpad/+bug/1023425
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/private-dupe-bug-warning3/+merge/116997
== Implementation ==
Thus branch completes the first round of work to improve the bug dupe form. Main new features compared with previous branch:
- Short circuit some checks
If the same bug number as the current bug is entered, of the same bug number as the current dupe, no search is done and a suitable error message is displayed iommediately.
- Display privacy warning
If the current bug is public and the proposed dupe bug is private, display a warning
(improvements to message text welcome)
- All correct bug data retrieved from server
The implementation adds a new method getBugData() to IMaloneApplication and exposes it as a named ws op on the /bugs web service interface. The method currently takes just the one search criteria (bug_id) but can be extended if we want to allow searching on pillar and key words. The getBugData method returns a list of json bug data.
==Demo and QA ==
Here's a screenshot of the privacy warning.
http://people.canonical.com/~ianb/bug-dupe-privacy-warning.png
== Tests ==
Extend yui tests
Add tests for getBugData (for direct call and via web service)
== 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/bugtask_index.js
lib/lp/bugs/javascript/duplicates.js
lib/lp/bugs/javascript/tests/test_duplicates.js
lib/lp/bugs/tests/test_searchtasks_webserv
--
https://code.launchpad.net/~wallyworld/launchpad/private-dupe-bug-warning3/+merge/116997
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-02-28 04:24:19 +0000
+++ lib/lp/bugs/browser/tests/test_bugs.py 2012-07-27 08:04:37 +0000
@@ -9,12 +9,15 @@
from zope.component import getUtility
+from lp.bugs.interfaces.bugtask import BugTaskStatus
from lp.bugs.interfaces.malone import IMaloneApplication
from lp.bugs.publisher import BugsLayer
+from lp.registry.enums import InformationType
from lp.services.webapp.publisher import canonical_url
from lp.testing import (
celebrity_logged_in,
feature_flags,
+ person_logged_in,
set_feature_flag,
TestCaseWithFactory,
)
@@ -113,3 +116,27 @@
# we should get some valid content out of this
self.assertIn('Search all bugs', content)
+
+ def test_getBugData(self):
+ # The getBugData method works as expected.
+ owner = self.factory.makePerson()
+ bug = self.factory.makeBug(
+ 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)
+ expected_bug_data = {
+ 'id': bug.id,
+ 'information_type': 'Private Security',
+ 'is_private': True,
+ 'importance': 'Undecided',
+ 'importance_class': 'importanceUNDECIDED',
+ 'status': 'In Progress',
+ 'status_class': 'statusINPROGRESS',
+ 'bug_summary': 'title',
+ 'description': 'description',
+ 'bug_url': canonical_url(bug.default_bugtask)
+ }
+ self.assertEqual([expected_bug_data], bug_data)
=== modified file 'lib/lp/bugs/interfaces/malone.py'
--- lib/lp/bugs/interfaces/malone.py 2012-05-11 06:29:42 +0000
+++ lib/lp/bugs/interfaces/malone.py 2012-07-27 08:04:37 +0000
@@ -12,10 +12,13 @@
collection_default_content,
export_as_webservice_collection,
export_factory_operation,
+ export_read_operation,
+ operation_for_version,
operation_parameters,
REQUEST_USER,
)
from lazr.restful.fields import Reference
+from lazr.restful.interface import copy_field
from zope.interface import Attribute
from lp.bugs.interfaces.bug import IBug
@@ -36,6 +39,21 @@
def searchTasks(search_params):
"""Search IBugTasks with the given search parameters."""
+ @call_with(user=REQUEST_USER)
+ @operation_parameters(
+ bug_id=copy_field(IBug['id'])
+ )
+ @export_read_operation()
+ @operation_for_version('devel')
+ def getBugData(user, bug_id):
+ """Search bugtasks matching the specified criteria.
+
+ The only criteria currently supported is to search for a bugtask with
+ the specified bug id.
+
+ :return: a list of matching bugs represented as json data
+ """
+
bug_count = Attribute("The number of bugs recorded in Launchpad")
bugwatch_count = Attribute("The number of links to external bug trackers")
bugtask_count = Attribute("The number of bug tasks in Launchpad")
=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js 2012-07-25 00:31:04 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js 2012-07-27 08:04:37 +0000
@@ -41,7 +41,11 @@
setup_client_and_bug();
var dup_widget = new Y.lp.bugs.duplicates.MarkBugDuplicate({
srcNode: '#duplicate-actions',
- lp_bug_entry: lp_bug_entry
+ lp_bug_entry: lp_bug_entry,
+ private_warning_message:
+ 'This is a private bug. If you select this bug as ' +
+ 'the duplicate, it may not be visible to some users.'
+
});
dup_widget.render();
=== modified file 'lib/lp/bugs/javascript/duplicates.js'
--- lib/lp/bugs/javascript/duplicates.js 2012-07-27 08:04:36 +0000
+++ lib/lp/bugs/javascript/duplicates.js 2012-07-27 08:04:37 +0000
@@ -138,16 +138,35 @@
* @private
*/
_find_duplicate_bug: function(data) {
- var new_dup_id = Y.Lang.trim(data['field.duplicateof'][0]);
+ var new_dup_id = Y.Lang.trim(data['field.duplicateof'][0]).trim();
// If there's no bug data entered then we are deleting the duplicate
// link.
if (new_dup_id === '') {
this._update_bug_duplicate(new_dup_id);
return;
}
+
+ // Do some quick checks before we submit.
+ if (new_dup_id === LP.cache.bug.id.toString()) {
+ this.duplicate_form.showError(
+ 'A bug cannot be marked as a duplicate of itself.');
+ return;
+ }
+ var duplicate_of_link = LP.cache.bug.duplicate_of_link;
+ var new_dupe_link
+ = Y.lp.client.get_absolute_uri("/api/devel/bugs/" + new_dup_id);
+ if (new_dupe_link === duplicate_of_link) {
+ this.duplicate_form.showError(
+ 'This bug is already marked as a duplicate of bug ' +
+ new_dup_id + '.');
+ return;
+ }
+
var that = this;
var qs_data
= 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", new_dup_id);
var bug_field = this.duplicate_form.form_node
.one('[id="field.duplicateof"]');
@@ -165,7 +184,12 @@
return;
}
var bug_data = Y.JSON.parse(response.responseText);
- that._confirm_selected_bug(bug_data);
+ if (!Y.Lang.isArray(bug_data) || bug_data.length === 0) {
+ return;
+ }
+ // The server may return multiple bugs but for now we only
+ // support displaying one of them.
+ that._confirm_selected_bug(bug_data[0]);
},
failure: function(id, response) {
var error_msg;
@@ -181,13 +205,14 @@
data: qs_data
};
var uri
- = Y.lp.client.get_absolute_uri("/api/devel/bugs/" + new_dup_id);
+ = Y.lp.client.get_absolute_uri("/api/devel/bugs");
this.io_provider.io(uri, config);
},
// Template for rendering the bug details.
_bug_details_template: function() {
return [
+ '<table><tbody><tr><td>',
'<div id="client-listing">',
' <div class="buglisting-col1">',
' <div class="importance {{importance_class}}">',
@@ -210,8 +235,22 @@
' {{description}}</p>',
' </div>',
' </div>',
- '</div>'
- ].join(' ');
+ '</div></td></tr>',
+ '{{> private_warning}}',
+ '</tbody></table>'
+ ].join(' ');
+ },
+
+ _private_warning_template: function(message) {
+ var template = [
+ '{{#private_warning}}',
+ '<tr><td><p id="privacy-warning" ',
+ 'class="block-sprite large-warning">',
+ '{message}',
+ '</p></td></tr>',
+ '{{/private_warning}}'
+ ].join(' ');
+ return Y.Lang.substitute(template, {message: message});
},
// Template for the bug confirmation form.
@@ -236,18 +275,18 @@
* @private
*/
_confirm_selected_bug: function(bug_data) {
- // TODO - get real data from the server
- bug_data.importance = 'High';
- bug_data.importance_class = 'importanceHIGH';
- bug_data.status = 'Triaged';
- bug_data.status_class = 'statusTRIAGED';
- bug_data.bug_summary = bug_data.title;
- bug_data.bug_url = bug_data.web_link;
-
var bug_id = bug_data.id;
+ bug_data.private_warning
+ = this.get('public_context') && bug_data.is_private;
+ var private_warning_message
+ = this.get('private_warning_message');
var html = Y.lp.mustache.to_html(
this._bug_confirmation_form_template(), bug_data,
- {bug_details: this._bug_details_template()});
+ {
+ bug_details: this._bug_details_template(),
+ private_warning:
+ this._private_warning_template(private_warning_message)
+ });
var confirm_node = Y.Node.create(html);
this._show_confirm_node(confirm_node);
var that = this;
@@ -521,6 +560,16 @@
dupe_span: '#mark-duplicate-text'
},
ATTRS: {
+ // Is the context in which this form being used public.
+ public_context: {
+ getter: function() {
+ return !Y.one(document.body).hasClass('private');
+ }
+ },
+ // Warning to display if we select a private bug from a public context.
+ private_warning_message: {
+ value: 'You are selecting a private bug.'
+ },
// The launchpad client entry for the current bug.
lp_bug_entry: {
value: null
=== modified file 'lib/lp/bugs/javascript/tests/test_duplicates.js'
--- lib/lp/bugs/javascript/tests/test_duplicates.js 2012-07-27 08:04:36 +0000
+++ lib/lp/bugs/javascript/tests/test_duplicates.js 2012-07-27 08:04:37 +0000
@@ -13,8 +13,9 @@
links: {},
cache: {
bug: {
+ id: 1,
self_link: 'api/devel/bugs/1',
- duplicate_of_link: null
+ duplicate_of_link: ''
}
}
};
@@ -52,7 +53,8 @@
srcNode: '#duplicate-actions',
lp_bug_entry: this.lp_bug_entry,
use_animation: false,
- io_provider: this.mockio
+ io_provider: this.mockio,
+ private_warning_message: 'Privacy warning'
});
widget.render();
Y.Assert.areEqual(
@@ -109,11 +111,18 @@
.hasClass('yui3-lazr-formoverlay-hidden'));
Y.DOM.byId('field.duplicateof').value = bug_id;
form.one('[name="field.actions.change"]').simulate('click');
- var expected_url = '/api/devel/bugs/1';
if (bug_id !== '') {
- expected_url = 'file:///api/devel/bugs/' + bug_id;
+ Y.Assert.areEqual(
+ 'file:///api/devel/bugs',
+ this.mockio.last_request.url);
+ Y.Assert.areEqual(
+ this.mockio.last_request.config.data,
+ 'ws.accept=application.json&ws.op=getBugData&' +
+ 'bug_id=' + bug_id);
+ } else {
+ Y.Assert.areEqual(
+ '/api/devel/bugs/1', this.mockio.last_request.url);
}
- Y.Assert.areEqual(expected_url, this.mockio.last_request.url);
},
// The bug entry form is visible and the confirmation form is invisible
@@ -134,15 +143,49 @@
// Invoke a successful search operation and check the form state.
_assert_search_form_success: function(bug_id) {
- var expected_updated_entry = {
+ var is_private = bug_id === 4;
+ var expected_updated_entry = [{
id: bug_id,
uri: 'api/devel/bugs/' + bug_id,
+ 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}];
this.mockio.last_request.successJSON(expected_updated_entry);
this._assert_form_state(true);
},
+ // 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() {
+ this.widget = this._createWidget(false);
+ this.widget.get('update_dupe_link').simulate('click');
+ this.mockio.last_request = null;
+ Y.DOM.byId('field.duplicateof').value = 1;
+ var form = Y.one('#duplicate-form-container');
+ form.one('[name="field.actions.change"]').simulate('click');
+ Y.Assert.isNull(this.mockio.last_request);
+ this._assert_error_display(
+ 'A bug cannot be marked as a duplicate of itself.');
+ this._assert_form_state(false);
+ },
+
+ // Attempt to make a bug as a duplicate of it's existing dupe is
+ // detected and an error is displayed immediately.
+ test_mark_bug_as_dupe_of_existing_dupe: function() {
+ this.widget = this._createWidget(false);
+ this.widget.get('update_dupe_link').simulate('click');
+ this.mockio.last_request = null;
+ window.LP.cache.bug.duplicate_of_link
+ = 'file:///api/devel/bugs/4';
+ Y.DOM.byId('field.duplicateof').value = 4;
+ var form = Y.one('#duplicate-form-container');
+ form.one('[name="field.actions.change"]').simulate('click');
+ Y.Assert.isNull(this.mockio.last_request);
+ this._assert_error_display(
+ 'This bug is already marked as a duplicate of bug 4.');
+ this._assert_form_state(false);
+ },
+
// A successful search for a bug displays the confirmation form.
test_initial_bug_search_success: function() {
this.widget = this._createWidget(false);
@@ -150,6 +193,34 @@
this._assert_search_form_success(3);
},
+ // No privacy warning when marking a bug as a dupe a public one.
+ test_public_dupe: function() {
+ this.widget = this._createWidget(false);
+ this._assert_search_form_submission(3);
+ this._assert_search_form_success(3);
+ Y.Assert.isNull(Y.one('#privacy-warning'));
+ },
+
+ // Privacy warning when marking a public bug as a dupe of private one.
+ test_public_bug_private_dupe: function() {
+ this.widget = this._createWidget(false);
+ this._assert_search_form_submission(4);
+ this._assert_search_form_success(4);
+ var privacy_message = Y.one('#privacy-warning');
+ Y.Assert.areEqual(
+ 'Privacy warning', privacy_message.get('text').trim());
+ },
+
+ // No privacy warning when marking a private bug as a dupe of another
+ // private bug.
+ test_private_bug_private_dupe: function() {
+ Y.one(document.body).addClass('private');
+ this.widget = this._createWidget(false);
+ this._assert_search_form_submission(4);
+ this._assert_search_form_success(4);
+ Y.Assert.isNull(Y.one('#privacy-warning'));
+ },
+
// After a successful search, hitting the Search Again button takes us
// back to the bug details entry form.
test_initial_bug_search_try_again: function() {
@@ -260,7 +331,7 @@
function(response, old_dup_url, new_dup_id) {
Y.Assert.areEqual(
'There was an error', response.responseText);
- Y.Assert.areEqual(null, old_dup_url);
+ Y.Assert.areEqual('', old_dup_url);
Y.Assert.areEqual(3, new_dup_id);
failure_called = true;
};
=== modified file 'lib/lp/bugs/tests/test_searchtasks_webservice.py'
--- lib/lp/bugs/tests/test_searchtasks_webservice.py 2012-07-17 14:13:55 +0000
+++ lib/lp/bugs/tests/test_searchtasks_webservice.py 2012-07-27 08:04:37 +0000
@@ -115,3 +115,36 @@
# A non-matching search returns no results.
response = self.search("devel", information_type="Private")
self.assertEqual(response['total_size'], 0)
+
+
+class TestGetBugData(TestCaseWithFactory):
+ """Tests for the /bugs getBugData operation."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestGetBugData, self).setUp()
+ self.owner = self.factory.makePerson()
+ with person_logged_in(self.owner):
+ self.product = self.factory.makeProduct()
+ self.bug = self.factory.makeBug(
+ product=self.product,
+ information_type=InformationType.PRIVATESECURITY)
+ self.webservice = LaunchpadWebServiceCaller(
+ 'launchpad-library', 'salgado-change-anything')
+
+ def search(self, api_version, **kwargs):
+ return self.webservice.named_get(
+ '/bugs', 'getBugData',
+ api_version=api_version, **kwargs).jsonBody()
+
+ def test_search_returns_results(self):
+ # A matching search returns results.
+ response = self.search(
+ "devel", bug_id=self.bug.id)
+ self.assertEqual(self.bug.id, response[0]['id'])
+
+ def test_search_returns_no_results(self):
+ # A non-matching search returns no results.
+ response = self.search("devel", bug_id=0)
+ self.assertEqual(len(response), 0)
=== modified file 'lib/lp/systemhomes.py'
--- lib/lp/systemhomes.py 2012-05-11 06:36:01 +0000
+++ lib/lp/systemhomes.py 2012-07-27 08:04:37 +0000
@@ -58,6 +58,7 @@
IHWVendorIDSet,
ParameterError,
)
+from lp.registry.enums import PRIVATE_INFORMATION_TYPES
from lp.registry.interfaces.distribution import IDistribution
from lp.registry.interfaces.distributionsourcepackage import (
IDistributionSourcePackage,
@@ -77,6 +78,7 @@
ICanonicalUrlData,
ILaunchBag,
)
+from lp.services.webapp.publisher import canonical_url
from lp.services.webservice.interfaces import IWebServiceApplication
from lp.services.worlddata.interfaces.language import ILanguageSet
from lp.testopenid.interfaces.server import ITestOpenIDApplication
@@ -126,6 +128,30 @@
"""See `IMaloneApplication`."""
return getUtility(IBugTaskSet).search(search_params)
+ def getBugData(self, user, bug_id):
+ """See `IMaloneApplication`."""
+ search_params = BugTaskSearchParams(user, bug=bug_id)
+ bugtasks = getUtility(IBugTaskSet).search(search_params)
+ if not bugtasks:
+ return []
+ bugs = [task.bug for task in bugtasks]
+ data = []
+ for bug in bugs:
+ bugtask = bug.default_bugtask
+ data.append({
+ 'id': bug_id,
+ 'information_type': bug.information_type.title,
+ 'is_private':
+ bug.information_type in PRIVATE_INFORMATION_TYPES,
+ 'importance': bugtask.importance.title,
+ 'importance_class': 'importance' + bugtask.importance.name,
+ 'status': bugtask.status.title,
+ 'status_class': 'status' + bugtask.status.name,
+ 'bug_summary': bug.title,
+ 'description': bug.description,
+ 'bug_url': canonical_url(bugtask)})
+ return data
+
def createBug(self, owner, title, description, target,
security_related=False, private=False, tags=None):
"""See `IMaloneApplication`."""
Follow ups
