launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05598
[Merge] lp:~wallyworld/launchpad/delete-all-bugtasks-889202 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/delete-all-bugtasks-889202 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #889202 in Launchpad itself: "OOPS deleting all bugtasks from bug report"
https://bugs.launchpad.net/launchpad/+bug/889202
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/delete-all-bugtasks-889202/+merge/82640
Display a user friendly error message when a user attempts to delete the last bug task.
This won't normally happen but could do if two users open the same bug and attempt to both perform a delete.
== Implementation ==
Catch the CannotDeleteBugTask exception in the view and add an error message notification to the http response.
Refactor the model code to include a specific error reason in the exception.
Perform a small drive-by improvement in the javascript to when rendering bug task rows.
When using the ajax form and an error occurs, the spinner is removed and the delete link is also left hidden so the user cannot attempt the bad delete again.
== Tests ==
Add additional tests to TestBugTaskDeleteView:
- test_delete_only_bugtask
- test_ajax_delete_only_bugtask
Add additional yui tests:
- test_process_bugtask_delete_error_response
- test_hide_spinner_restore_delete_link
- test_hide_spinner_delete_link_stays_hidden
== Lint ==
Linting changed files:
lib/lp/bugs/browser/bugtask.py
lib/lp/bugs/browser/tests/test_bugtask.py
lib/lp/bugs/javascript/bugtask_index.js
lib/lp/bugs/javascript/tests/test_bugtask_delete.js
lib/lp/bugs/model/bugtask.py
--
https://code.launchpad.net/~wallyworld/launchpad/delete-all-bugtasks-889202/+merge/82640
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/delete-all-bugtasks-889202 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-11-18 04:43:38 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-11-18 06:30:30 +0000
@@ -236,6 +236,7 @@
BugTaskStatus,
BugTaskStatusSearch,
BugTaskStatusSearchDisplay,
+ CannotDeleteBugtask,
DEFAULT_SEARCH_BUGTASK_STATUSES_FOR_DISPLAY,
IBugTask,
IBugTaskSearch,
@@ -1801,11 +1802,21 @@
bugtask = self.context
bug = bugtask.bug
deleted_bugtask_url = canonical_url(self.context, rootsite='bugs')
- message = ("This bug no longer affects %s."
+ success_message = ("This bug no longer affects %s."
% bugtask.bugtargetdisplayname)
- bugtask.delete()
- self.request.response.addNotification(message)
+ error_message = None
+
+ try:
+ bugtask.delete()
+ self.request.response.addNotification(success_message)
+ except CannotDeleteBugtask as e:
+ error_message = str(e)
+ self.request.response.addErrorNotification(error_message)
if self.request.is_ajax:
+ if error_message:
+ self.request.response.setHeader('Content-type',
+ 'application/json')
+ return dumps(None)
launchbag = getUtility(ILaunchBag)
launchbag.add(bug.default_bugtask)
# If we are deleting the current highlighted bugtask via ajax,
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2011-11-17 17:17:38 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2011-11-18 06:30:30 +0000
@@ -904,6 +904,24 @@
expected = 'This bug no longer affects %s.' % target_name
self.assertEqual(expected, notifications[0].message)
+ def test_delete_only_bugtask(self):
+ # Test that the deleting the only bugtask results in an error message.
+ bug = self.factory.makeBug()
+ with FeatureFixture(DELETE_BUGTASK_ENABLED):
+ login_person(bug.owner)
+ form = {
+ 'field.actions.delete_bugtask': 'Delete',
+ }
+ view = create_initialized_view(
+ bug.default_bugtask, name='+delete', form=form,
+ principal=bug.owner)
+ self.assertEqual([bug.default_bugtask], bug.bugtasks)
+ notifications = view.request.response.notifications
+ self.assertEqual(1, len(notifications))
+ expected = ('Cannot delete only bugtask affecting: %s.'
+ % bug.default_bugtask.target.bugtargetdisplayname)
+ self.assertEqual(expected, notifications[0].message)
+
def _create_bugtask_to_delete(self):
bug = self.factory.makeBug()
bugtask = self.factory.makeBugTask(bug=bug)
@@ -945,6 +963,38 @@
expected_url = canonical_url(bug.default_bugtask, rootsite='bugs')
self.assertEqual(dict(bugtask_url=expected_url), result_data)
+ def test_ajax_delete_only_bugtask(self):
+ # Test that deleting the only bugtask returns an empty JSON response
+ # with an error notification.
+ bug = self.factory.makeBug()
+ with FeatureFixture(DELETE_BUGTASK_ENABLED):
+ login_person(bug.owner)
+ # Set up the request so that we correctly simulate an XHR call
+ # from the URL of the bugtask we are deleting.
+ server_url = canonical_url(
+ getUtility(ILaunchpadRoot), rootsite='bugs')
+ extra = {
+ 'HTTP_X_REQUESTED_WITH': 'XMLHttpRequest',
+ }
+ form = {
+ 'field.actions.delete_bugtask': 'Delete'
+ }
+ view = create_initialized_view(
+ bug.default_bugtask, name='+delete', server_url=server_url,
+ form=form, principal=bug.owner, **extra)
+ result_data = simplejson.loads(view.render())
+ self.assertEqual([bug.default_bugtask], bug.bugtasks)
+ notifications = simplejson.loads(
+ view.request.response.getHeader('X-Lazr-Notifications'))
+ self.assertEqual(1, len(notifications))
+ expected = ('Cannot delete only bugtask affecting: %s.'
+ % bug.default_bugtask.target.bugtargetdisplayname)
+ self.assertEqual(expected, notifications[0][1])
+ self.assertEqual(
+ 'application/json',
+ view.request.response.getHeader('content-type'))
+ self.assertEqual(None, result_data)
+
def test_ajax_delete_non_current_bugtask(self):
# Test that deleting the non-current bugtask returns the new bugtasks
# table as HTML.
=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js 2011-11-14 08:30:52 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js 2011-11-18 06:30:30 +0000
@@ -681,9 +681,9 @@
if (tr !== null) {
// The row has not been deleted.
tr.all('a').each(process_link);
- // Now wire up the javascript widgets in the table row.
- namespace.setup_bugtask_row(conf);
}
+ // Now wire up the javascript widgets in the table row.
+ namespace.setup_bugtask_row(conf);
}
}
};
@@ -705,8 +705,10 @@
*
* @method _hideDeleteSpinner
*/
-namespace._hideDeleteSpinner = function(delete_link) {
- delete_link.removeClass('unseen');
+namespace._hideDeleteSpinner = function(delete_link, show_link) {
+ if( show_link ) {
+ delete_link.removeClass('unseen');
+ }
var spinner = delete_link.get('parentNode').one('.spinner');
if (spinner !== null) {
spinner.remove();
@@ -765,7 +767,8 @@
*
* @method _process_bugtask_delete_response
*/
-namespace._process_bugtask_delete_response = function(response, row_id) {
+namespace._process_bugtask_delete_response = function(
+ response, row_id, delete_link) {
// 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.
@@ -774,6 +777,10 @@
Y.lp.client.display_notifications(
response.getResponseHeader('X-Lazr-Notifications'));
var redirect = Y.JSON.parse(response.responseText);
+ if( !Y.Lang.isValue(redirect) ) {
+ namespace._hideDeleteSpinner(delete_link, false);
+ return;
+ }
Y.lp.anim.red_flash({
node: '#' + row_id,
duration: namespace.ANIM_DURATION
@@ -804,7 +811,7 @@
Y.lp.client.remove_notifications();
var error_handler = new Y.lp.client.ErrorHandler();
error_handler.showError = Y.bind(function (error_msg) {
- namespace._hideDeleteSpinner(delete_link);
+ namespace._hideDeleteSpinner(delete_link, true);
Y.lp.app.errors.display_error(undefined, error_msg);
}, this);
@@ -822,7 +829,7 @@
success:
function(id, response) {
namespace._process_bugtask_delete_response(
- response, conf.row_id);
+ response, conf.row_id, delete_link);
}
},
data: qs
@@ -868,7 +875,7 @@
if (Y.Lang.isValue(LP.cache.bug) &&
Y.Lang.isValue(LP.cache.bug.duplicate_of_link)) {
// If the bug is a duplicate, don't set the widget up and
- // canel clicks on the edit links. Users most likely don't
+ // cancel clicks on the edit links. Users most likely don't
// want to edit the bugtasks.
status_content.on('click', function(e) { e.halt(); });
importance_content.on('click', function(e) { e.halt(); });
=== modified file 'lib/lp/bugs/javascript/tests/test_bugtask_delete.js'
--- lib/lp/bugs/javascript/tests/test_bugtask_delete.js 2011-11-04 14:01:34 +0000
+++ lib/lp/bugs/javascript/tests/test_bugtask_delete.js 2011-11-18 06:30:30 +0000
@@ -45,14 +45,24 @@
Y.Assert.isTrue(this.delete_link.hasClass('unseen'));
},
- test_hide_spinner: function() {
- // Test the delete progress spinner is hidden.
+ test_hide_spinner_restore_delete_link: function() {
+ // Test the delete progress spinner is hidden and delete link is
+ // visible again.
module._showDeleteSpinner(this.delete_link);
- module._hideDeleteSpinner(this.delete_link);
+ module._hideDeleteSpinner(this.delete_link, true);
Y.Assert.isNull(this.fixture.one('.spinner'));
Y.Assert.isFalse(this.delete_link.hasClass('unseen'));
},
+ test_hide_spinner_delete_link_stays_hidden: function() {
+ // Test the delete progress spinner is hidden and delete link
+ // remains hidden.
+ module._showDeleteSpinner(this.delete_link);
+ module._hideDeleteSpinner(this.delete_link, false);
+ Y.Assert.isNull(this.fixture.one('.spinner'));
+ Y.Assert.isTrue(this.delete_link.hasClass('unseen'));
+ },
+
_test_delete_confirmation: function(click_ok) {
// Test the delete confirmation dialog when delete is clicked.
var orig_delete_bugtask = module.delete_bugtask;
@@ -146,7 +156,7 @@
responseText: '{"bugtask_url": "http://foo"}',
responseHeaders: {'Content-type': 'application/json'}});
module._process_bugtask_delete_response(
- response, this.link_conf.row_id);
+ response, this.link_conf.row_id, this.delete_link);
this.wait(function() {
// Wait for the animation to complete.
Y.Assert.isTrue(redirect_called);
@@ -154,6 +164,34 @@
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.
+ var orig_redirect = module._redirect;
+ var redirect_called = false;
+ module._redirect = function(url) {
+ redirect_called = true;
+ };
+ this.delete_link.addClass('unseen');
+ var notifications = '[ [40, "Delete Error"] ]';
+ var response = new Y.lp.testing.mockio.MockHttpResponse({
+ responseText: 'null',
+ responseHeaders: {
+ 'Content-type': 'application/json',
+ 'X-Lazr-Notifications': notifications}});
+ module._process_bugtask_delete_response(
+ response, this.link_conf.row_id, this.delete_link);
+ this.wait(function() {
+ // Wait for the animation to complete.
+ Y.Assert.isFalse(redirect_called);
+ var node = Y.one('div#request-notifications ' +
+ 'div.error.message');
+ Y.Assert.areEqual('Delete Error', node.getContent());
+ Y.Assert.isTrue(this.delete_link.hasClass('unseen'));
+ }, 50);
+ module._redirect = orig_redirect;
+ },
+
test_process_bugtask_delete_new_table_response: function() {
// Test the processing of a XHR delete result which is to
// replace the current bugtasks table.
@@ -170,7 +208,7 @@
'Content-type': 'text/html',
'X-Lazr-Notifications': notifications}});
module._process_bugtask_delete_response(
- response, this.link_conf.row_id);
+ response, this.link_conf.row_id, this.delete_link);
this.wait(function() {
// Wait for the animation to complete.
Y.Assert.isTrue(render_table_called);
@@ -189,9 +227,11 @@
var delete_response_called = false;
var self = this;
- module._process_bugtask_delete_response = function(response, id) {
+ module._process_bugtask_delete_response =
+ function(response, id, delete_link) {
Y.Assert.areEqual('<p>Foo</p>', response.responseText);
Y.Assert.areEqual(self.link_conf.row_id, id);
+ Y.Assert.areEqual(self.delete_link, delete_link);
delete_response_called = true;
};
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-11-17 12:42:16 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-11-18 06:30:30 +0000
@@ -640,19 +640,29 @@
return self._status in RESOLVED_BUGTASK_STATUSES
def canBeDeleted(self):
+ try:
+ self.checkCanBeDeleted()
+ except Exception:
+ return False
+ return True
+
+ def checkCanBeDeleted(self):
num_bugtasks = Store.of(self).find(
BugTask, bug=self.bug).count()
- return num_bugtasks > 1
+ if num_bugtasks < 2:
+ raise CannotDeleteBugtask(
+ "Cannot delete only bugtask affecting: %s."
+ % self.target.bugtargetdisplayname)
def delete(self, who=None):
"""See `IBugTask`."""
if who is None:
who = getUtility(ILaunchBag).user
- if not self.canBeDeleted():
- raise CannotDeleteBugtask(
- "Cannot delete bugtask: %s" % self.title)
+ # Raise an error if the bugtask cannot be deleted.
+ self.checkCanBeDeleted()
+
bug = self.bug
target = self.target
notify(ObjectDeletedEvent(self, who))
