launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05976
[Merge] lp:~wallyworld/launchpad/delete-bugtask-stale-page-904890 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/delete-bugtask-stale-page-904890 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #904890 in Launchpad itself: "delete bugtask in stale page cause errors"
https://bugs.launchpad.net/launchpad/+bug/904890
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/delete-bugtask-stale-page-904890/+merge/86205
== Implementation ==
When a bug task is to be deleted, a POST is made to a URL like:
https://bugs.launchpad.dev/ubuntu/grumpy/+bug/5/+delete
If the bugtask has already been deleted, the bug traversal code catches this error and redirects to the default bug task (with the +delete sill appended). The browser intercepts the redirect and renders the resulting URL. The XHR call receives the result with status 200 and assumes the repsonse.responseText is actually the new bugtask table being returned from a successful delete call. So the HTML of an entire LP "Do you really want to delete this bugtask" page is rendered in place of the bugtask tabble.
Solution: modify the bug traversal code to raise a NotFound (404) if an attempt is made to delete a non-existent bugtask (instead of redirecting). The client code receives the 404 and displays a user friendly message, as well as deleting the bugtask row from the ui and deleting the bugtask entry from the bugtask_data json cache.
The standard LP client ErrorHandler needed to be modified. A handleError() hook returning boolean was added. If a subclass wants to process any error itself and stop subsequent processing which would normally display an error message, it implements handleError() and returns true.
== QA ==
As per the original bug report.
== Tests ==
I couldn't find any existing tests for the specific bug traversal behaviour altered in this mp so I added a new test module: test_bugtask_navigation
TestBugtaskTraversal:
- test_traversal_to_nonexistent_bugtask
- test_traversal_to_bugtask_delete_url
There were no tests at all for the LP client ErrorHandler. I added new tests to the test_bugtask_delete yui tests which test the new error handling and will fail if ErrorHandler is broken:
- test_process_bugtask_delete_not_found_response
- test_delete_non_existent_bugtask
== Lint ==
Drive by lint fix for a comment in bugtask.
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/client.js
lib/lp/bugs/browser/bugtask.py
lib/lp/bugs/browser/tests/test_bugtask_navigation.py
lib/lp/bugs/javascript/bugtask_index.js
lib/lp/bugs/javascript/tests/test_bugtask_delete.js
--
https://code.launchpad.net/~wallyworld/launchpad/delete-bugtask-stale-page-904890/+merge/86205
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/delete-bugtask-stale-page-904890 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/client.js'
--- lib/lp/app/javascript/client.js 2011-11-21 14:37:25 +0000
+++ lib/lp/app/javascript/client.js 2011-12-19 10:34:26 +0000
@@ -886,6 +886,21 @@
showError: function (error_msg) {},
/**
+ * Handle an error from an XHR request.
+ *
+ * This method is invoked before any generic error handling is done.
+ *
+ * @method handleError
+ * @param ioId The request id.
+ * @param response The XHR call response object.
+ * @return {Boolean} Return true if the error has been fully processed and
+ * any further generic error handling is not required.
+ */
+ handleError: function(ioId, response) {
+ return false;
+ },
+
+ /**
* Return a failure handler function for XHR requests.
*
* Assign the result of this function as the failure handler when
@@ -897,6 +912,11 @@
var self = this;
return function(ioId, o) {
self.clearProgressUI();
+ // Perform any user specified error handling. If true is returned,
+ // we do not do any further processing.
+ if( self.handleError(ioId, o) ) {
+ return;
+ }
// If it was a timeout...
if (o.status === 503) {
self.showError(
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-12-18 03:35:49 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-12-19 10:34:26 +0000
@@ -559,8 +559,13 @@
# Security proxy this object on the way out.
return getUtility(IBugTaskSet).get(bugtask.id)
- # If we've come this far, there's no task for the requested
- # context. Redirect to one that exists.
+ # If we've come this far, there's no task for the requested context.
+ # If we are attempting to delete a bug task, we raise NotFound error.
+ # Otherwise we are simply navigating to a non-existent task and so we
+ # redirect to one that exists.
+ travseral_stack = self.request.getTraversalStack()
+ if len(travseral_stack) > 0 and travseral_stack[-1] == '+delete':
+ raise NotFoundError
return self.redirectSubTree(canonical_url(bug.default_bugtask))
@@ -2253,9 +2258,9 @@
}
# This is a total hack, but pystache will run both truth/false values
- # for an empty list for some reason, and it "works" if it's just a flag
- # like this. We need this value for the mustache template to be able
- # to tell that there are no tags without looking at the list.
+ # for an empty list for some reason, and it "works" if it's just a
+ # flag like this. We need this value for the mustache template to be
+ # able to tell that there are no tags without looking at the list.
flattened['has_tags'] = True if len(flattened['tags']) else False
return flattened
=== added file 'lib/lp/bugs/browser/tests/test_bugtask_navigation.py'
--- lib/lp/bugs/browser/tests/test_bugtask_navigation.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask_navigation.py 2011-12-19 10:34:26 +0000
@@ -0,0 +1,54 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test `BugTargetTraversalMixin`."""
+
+__metaclass__ = type
+
+from zope.publisher.interfaces import NotFound
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.services.features.testing import FeatureFixture
+from lp.testing import (
+ login_person,
+ TestCaseWithFactory,
+ )
+from lp.testing.publication import test_traverse
+
+
+class TestBugtaskTraversal(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_traversal_to_nonexistent_bugtask(self):
+ # Test that a traversing to a non-existent bugtask redirects to the
+ # bug's default bugtask.
+ bug = self.factory.makeBug()
+ bugtask = self.factory.makeBugTask(bug=bug)
+ bugtask_url = canonical_url(bugtask, rootsite='bugs')
+ flags = {u"disclosure.delete_bugtask.enabled": u"on"}
+ login_person(bugtask.owner)
+ with FeatureFixture(flags):
+ bugtask.delete()
+ obj, view, request = test_traverse(bugtask_url)
+ view()
+ naked_view = removeSecurityProxy(view)
+ self.assertEqual(301, request.response.getStatus())
+ self.assertEqual(
+ naked_view.target,
+ canonical_url(bug.default_bugtask, rootsite='bugs'))
+
+ def test_traversal_to_bugtask_delete_url(self):
+ # Test that a traversing to the delete URL of a non-existent bugtask
+ # raises a NotFound error.
+ bug = self.factory.makeBug()
+ bugtask = self.factory.makeBugTask(bug=bug)
+ bugtask_delete_url = canonical_url(
+ bugtask, rootsite='bugs', view_name='+delete')
+ flags = {u"disclosure.delete_bugtask.enabled": u"on"}
+ login_person(bugtask.owner)
+ with FeatureFixture(flags):
+ bugtask.delete()
+ self.assertRaises(NotFound, test_traverse, bugtask_delete_url)
=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js 2011-12-16 03:14:17 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js 2011-12-19 10:34:26 +0000
@@ -31,7 +31,6 @@
var bug_repr;
// Overlay related vars.
-var error_overlay;
var submit_button_html =
'<button type="submit" name="field.actions.change" ' +
'value="Change" class="lazr-pos lazr-btn" >OK</button>';
@@ -784,6 +783,26 @@
row_fade_out.run();
};
+ // If the bugtask has already been deleted, the HTTP response status will
+ // be 404. In this case, we remove the affected row and display a
+ // informational message to the user.
+ if( response.status === 404) {
+ var message = Y.Lang.substitute(
+ "Bug task affecting {targetname} has already been deleted.",
+ {targetname: LP.cache.bugtask_data[bugtask_id].targetname});
+ var notification = Y.Lang.substitute(
+ '[[20, "{message}"]]', {message: message});
+ Y.lp.client.display_notifications(notification);
+ animate_deletion(function() {
+ var tr = Y.one('#' + row_id);
+ if( tr !== null ) {
+ tr.remove(true);
+ }
+ delete LP.cache.bugtask_data[bugtask_id];
+ });
+ return;
+ }
+
// If the result is json, then we need to perform a redirect to a new
// bugtask URL. This happens when the current bugtask is deleted and we
// need to ensure all link URLS are correctly reset.
@@ -818,10 +837,18 @@
namespace.delete_bugtask = function (delete_link, conf) {
Y.lp.client.remove_notifications();
var error_handler = new Y.lp.client.ErrorHandler();
+ var module = this;
error_handler.showError = Y.bind(function (error_msg) {
namespace._hideDeleteSpinner(delete_link, true);
Y.lp.app.errors.display_error(undefined, error_msg);
}, this);
+ error_handler.handleError = Y.bind(function(id, response) {
+ if( response.status === 404 ) {
+ module._process_bugtask_delete_response(
+ response, conf.id, conf.row_id, delete_link);
+ return true;
+ }
+ }, this);
var submit_url = delete_link.get('href');
var qs = Y.lp.client.append_qs(
=== modified file 'lib/lp/bugs/javascript/tests/test_bugtask_delete.js'
--- lib/lp/bugs/javascript/tests/test_bugtask_delete.js 2011-12-16 03:14:17 +0000
+++ lib/lp/bugs/javascript/tests/test_bugtask_delete.js 2011-12-19 10:34:26 +0000
@@ -15,7 +15,8 @@
id: '49',
row_id: 'tasksummary49',
form_row_id: 'tasksummary49',
- user_can_delete: true
+ user_can_delete: true,
+ targetname: 'bugtarget'
};
this.no_form_link_conf = {
id: '50',
@@ -190,6 +191,47 @@
module._redirect = orig_redirect;
},
+ test_process_bugtask_delete_not_found_response: function() {
+ // Test the processing of a XHR delete request which is for a
+ // bugtask which has already been deleted displays an
+ // informational message and deletes the affected bugtask row.
+
+ var orig_render_bugtask_table = module._render_bugtask_table;
+ var render_table_called = false;
+ module._render_bugtask_table = function(new_table) {
+ render_table_called = true;
+ };
+ var orig_redirect = module._redirect;
+ var redirect_called = false;
+ module._redirect = function(url) {
+ redirect_called = true;
+ };
+ var response = new Y.lp.testing.mockio.MockHttpResponse({
+ responseText: '<html></html>',
+ status: 404,
+ responseHeaders: {
+ 'Content-type': 'text/html'}});
+ Y.Assert.isNotNull(Y.one('#' + this.link_conf.row_id));
+ module._process_bugtask_delete_response(
+ response, this.link_conf.id, this.link_conf.row_id,
+ this.delete_link);
+ this.wait(function() {
+ // Wait for the animation to complete.
+ var node = Y.one('div#request-notifications ' +
+ 'div.informational.message');
+ Y.Assert.isFalse(render_table_called);
+ Y.Assert.isFalse(redirect_called);
+ Y.Assert.areEqual(
+ 'Bug task affecting bugtarget has already been deleted.',
+ node.getContent());
+ Y.Assert.isNull(Y.one('#' + this.link_conf.row_id));
+ Y.Assert.isUndefined(
+ LP.cache.bugtask_data[this.link_conf.id]);
+ }, 50);
+ module._render_bugtask_table = orig_render_bugtask_table;
+ module._redirect = orig_redirect;
+ },
+
test_process_bugtask_delete_error_response: function() {
// Test the processing of a XHR delete request which results in
// an error message to be displayed.
@@ -287,6 +329,44 @@
Y.Assert.isTrue(delete_response_called);
module._process_bugtask_delete_response = orig_delete_repsonse;
+ },
+
+ test_delete_non_existent_bugtask: function() {
+ // Test that when delete_bugtask is called and a 404 error occurs,
+ // no error processing occurs (since it's not an error as such)
+ // and the correct call is made to process the result.
+
+ var orig_display_error = Y.lp.app.errors.display_error;
+ var display_error_called = false;
+ Y.lp.app.errors.display_error = function(flash_node, msg) {
+ display_error_called = true;
+ };
+ var orig_delete_repsonse =
+ module._process_bugtask_delete_response;
+
+ var delete_response_called = false;
+ var self = this;
+ module._process_bugtask_delete_response =
+ function(response, bugtask_id, row_id, delete_link) {
+ Y.Assert.areEqual(response.status, 404);
+ Y.Assert.areEqual(self.link_conf.id, bugtask_id);
+ Y.Assert.areEqual(self.link_conf.row_id, row_id);
+ Y.Assert.areEqual(self.delete_link, delete_link);
+ delete_response_called = true;
+ };
+
+ var mockio = new Y.lp.testing.mockio.MockIo();
+ var conf = Y.merge(this.link_conf, {io_provider: mockio});
+ module.delete_bugtask(this.delete_link, conf);
+ mockio.failure({
+ responseText: '<html></html>',
+ status: 404,
+ responseHeaders: {'Content-Type': 'text/html'}});
+ Y.Assert.isTrue(delete_response_called);
+ Y.Assert.isFalse(display_error_called);
+
+ module._process_bugtask_delete_response = orig_delete_repsonse;
+ Y.lp.app.errors.display_error = orig_display_error;
}
}));
