launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/delete-bugtask-red-flash-903110 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #903110 in Launchpad itself: "Red flash when removing project/package from a bug report"
  https://bugs.launchpad.net/launchpad/+bug/903110

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/delete-bugtask-red-flash-903110/+merge/85592

Flashing the deleted bug task red was wrong since red mean failure.

== Implementation ==

Alter the animation to fade out the deleted row.
In the process of testing, an error was discovered in setting up the table after deleting the row. To prevent the error, the cached bugtask data for the deleted bugtask id was removed after the delete succeeded.

== Tests ==

Modify the test_bugtask_delete yui test to check that the bugtask row is deleted from the cache.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/javascript/bugtask_index.js
  lib/lp/bugs/javascript/tests/test_bugtask_delete.j
-- 
https://code.launchpad.net/~wallyworld/launchpad/delete-bugtask-red-flash-903110/+merge/85592
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/delete-bugtask-red-flash-903110 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-12-13 15:32:36 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-12-14 03:52:25 +0000
@@ -4104,6 +4104,7 @@
             (user is None or user.teams_participated_in.count() == 0))
         cx = self.context
         return dict(
+            id=cx.id,
             row_id=self.data['row_id'],
             form_row_id=self.data['form_row_id'],
             bugtask_path='/'.join([''] + self.data['link'].split('/')[3:]),

=== modified file 'lib/lp/bugs/javascript/bugtask_index.js'
--- lib/lp/bugs/javascript/bugtask_index.js	2011-11-18 06:26:35 +0000
+++ lib/lp/bugs/javascript/bugtask_index.js	2011-12-14 03:52:25 +0000
@@ -678,10 +678,7 @@
             // embedded in the picker tales so we need to ensure we handle
             // this case.
             var tr = Y.one('#' + conf.form_row_id);
-            if (tr !== null) {
-                // The row has not been deleted.
-                tr.all('a').each(process_link);
-            }
+            tr.all('a').each(process_link);
             // Now wire up the javascript widgets in the table row.
             namespace.setup_bugtask_row(conf);
         }
@@ -768,7 +765,23 @@
  * @method _process_bugtask_delete_response
  */
 namespace._process_bugtask_delete_response = function(
-        response, row_id, delete_link) {
+        response, bugtask_id, row_id, delete_link) {
+    // The deleted row will fade out before being removed from the table.
+    var animate_deletion = function(after_callback) {
+        var row = Y.one('#' + row_id);
+        row.setStyle('opacity', 1);
+        row.addClass('transparent');
+        var row_fade_out = new Y.Anim({
+            node: row,
+            to: {opacity: 0},
+            duration: namespace.ANIM_DURATION
+        });
+        if( Y.Lang.isFunction(after_callback) ) {
+            row_fade_out.on('end', after_callback);
+        }
+        row_fade_out.run();
+    };
+
     // 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.
@@ -781,25 +794,18 @@
             namespace._hideDeleteSpinner(delete_link, false);
             return;
         }
-        Y.lp.anim.red_flash({
-            node: '#' + row_id,
-            duration: namespace.ANIM_DURATION
-        }).run();
+        animate_deletion();
         namespace._redirect(redirect.bugtask_url);
         return;
     }
     // We have received HTML, so we replace the current bugtask table with a
     // new one.
-    var anim = Y.lp.anim.red_flash({
-        node: '#' + row_id,
-        duration: namespace.ANIM_DURATION
-    });
-    anim.on('end', function() {
+    animate_deletion(function() {
+        delete LP.cache.bugtask_data[bugtask_id];
         namespace._render_bugtask_table(response.responseText);
         Y.lp.client.display_notifications(
             response.getResponseHeader('X-Lazr-Notifications'));
     });
-    anim.run();
 };
 
 /**
@@ -829,7 +835,7 @@
             success:
                 function(id, response) {
                     namespace._process_bugtask_delete_response(
-                            response, conf.row_id, delete_link);
+                            response, conf.id, conf.row_id, delete_link);
                 }
         },
         data: qs

=== modified file 'lib/lp/bugs/javascript/tests/test_bugtask_delete.js'
--- lib/lp/bugs/javascript/tests/test_bugtask_delete.js	2011-11-18 06:18:53 +0000
+++ lib/lp/bugs/javascript/tests/test_bugtask_delete.js	2011-12-14 03:52:25 +0000
@@ -12,6 +12,7 @@
         setUp: function() {
             module.ANIM_DURATION = 0;
             this.link_conf = {
+                id: '49',
                 row_id: 'tasksummary49',
                 form_row_id: 'tasksummary49',
                 user_can_delete: true
@@ -156,7 +157,8 @@
                 responseText: '{"bugtask_url": "http://foo"}',
                 responseHeaders: {'Content-type': 'application/json'}});
             module._process_bugtask_delete_response(
-                response, this.link_conf.row_id, this.delete_link);
+                response, this.link_conf.id, this.link_conf.row_id,
+                this.delete_link);
             this.wait(function() {
                 // Wait for the animation to complete.
                 Y.Assert.isTrue(redirect_called);
@@ -180,7 +182,8 @@
                     'Content-type': 'application/json',
                     'X-Lazr-Notifications': notifications}});
             module._process_bugtask_delete_response(
-                response, this.link_conf.row_id, this.delete_link);
+                response, this.link_conf.id, this.link_conf.row_id,
+                this.delete_link);
             this.wait(function() {
                 // Wait for the animation to complete.
                 Y.Assert.isFalse(redirect_called);
@@ -208,13 +211,16 @@
                     'Content-type': 'text/html',
                     'X-Lazr-Notifications': notifications}});
             module._process_bugtask_delete_response(
-                response, this.link_conf.row_id, this.delete_link);
+                response, this.link_conf.id, this.link_conf.row_id,
+                this.delete_link);
             this.wait(function() {
                 // Wait for the animation to complete.
                 Y.Assert.isTrue(render_table_called);
                 var node = Y.one('div#request-notifications ' +
                                     'div.informational.message');
                 Y.Assert.areEqual('Delete Success', node.getContent());
+                Y.Assert.isUndefined(
+                    LP.cache.bugtask_data[this.link_conf.id]);
             }, 50);
             module._render_bugtask_table = orig_render_bugtask_table;
         },
@@ -228,9 +234,10 @@
             var delete_response_called = false;
             var self = this;
             module._process_bugtask_delete_response =
-                    function(response, id, delete_link) {
+                    function(response, bugtask_id, row_id, delete_link) {
                 Y.Assert.areEqual('<p>Foo</p>', response.responseText);
-                Y.Assert.areEqual(self.link_conf.row_id, id);
+                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;
             };