launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/sort-sharing-details into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #982807 in Launchpad itself: "Sorting is broken on sharing details page"
  https://bugs.launchpad.net/launchpad/+bug/982807

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/sort-sharing-details/+merge/102090

== Implementation ==

Ensure the 3rd party table sorting javascript is re-initialised when the sharing details widget syncUI() method is called.

The sorttable.js is 3rd party code we repackage in launchpad. It has already been previously forked to deal with our sortkey tags. The init function is now tweaked to allow the caller to force a refresh of the sorting data. This is required because the init is called before the sharing details table is loaded and needs to be called again after the table data is available.

== Tests ==

Extend the test_sharingdetails test_syncUI test to check that the sorttable init method is called correctly.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/sorttable/sorttable.js
  lib/lp/registry/javascript/sharing/sharingdetails.js
  lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html
  lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js
  lib/lp/registry/templates/pillar-sharing-details.pt
-- 
https://code.launchpad.net/~wallyworld/launchpad/sort-sharing-details/+merge/102090
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/sort-sharing-details into lp:launchpad.

=== modified file 'lib/lp/app/javascript/sorttable/sorttable.js'
--- lib/lp/app/javascript/sorttable/sorttable.js	2012-01-20 05:14:00 +0000
+++ lib/lp/app/javascript/sorttable/sorttable.js	2012-04-16 13:29:19 +0000
@@ -28,10 +28,13 @@
 
     var stIsIE = /*@cc_on!@*/false;
 
-    sorttable = {
-      init: function() {
-        // quit if this function has already been called
-        if (arguments.callee.done) return;
+    var sorttable = {
+      init: function(force_refresh) {
+        // quit if this function has already been called,
+        // unless we are forcing a refresh.
+        if (!force_refresh) {
+          if (arguments.callee.done) return;
+        }
         // flag this function so we don't do the same thing twice
         arguments.callee.done = true;
         

=== modified file 'lib/lp/registry/javascript/sharing/sharingdetails.js'
--- lib/lp/registry/javascript/sharing/sharingdetails.js	2012-04-12 09:14:58 +0000
+++ lib/lp/registry/javascript/sharing/sharingdetails.js	2012-04-16 13:29:19 +0000
@@ -197,6 +197,7 @@
         }
         this.set('bugs', model_bugs);
         this.set('branches', model_branches);
+        Y.lp.app.sorttable.SortTable.init(true);
     },
 
     // Delete the specified sharees from the table.
@@ -318,5 +319,6 @@
 namespace.SharingDetailsTable = SharingDetailsTable;
 
 }, "0.1", { "requires": [
-    'node', 'event', 'json', 'lp.mustache', 'lp.anim'
+    'node', 'event', 'json', 'lp.mustache', 'lp.anim',
+    'lp.app.sorttable'
 ] });

=== modified file 'lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html'
--- lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html	2012-04-06 17:28:25 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_sharingdetails.html	2012-04-16 13:29:19 +0000
@@ -33,6 +33,8 @@
           src="../../../../../../build/js/lp/app/anim/anim.js"></script>
       <script type="text/javascript"
           src="../../../../../../build/js/lp/app/extras/extras.js"></script>
+      <script type="text/javascript"
+          src="../../../../../../build/js/lp/app/sorttable/sorttable.js"></script>
 
       <!-- The module under test. -->
       <script type="text/javascript" src="../sharingdetails.js"></script>

=== modified file 'lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js'
--- lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js	2012-04-12 23:06:29 +0000
+++ lib/lp/registry/javascript/sharing/tests/test_sharingdetails.js	2012-04-16 13:29:19 +0000
@@ -156,6 +156,14 @@
         test_syncUI: function() {
             this.details_widget = this._create_Widget();
             this.details_widget.render();
+
+            var sorttable_ns = Y.lp.app.sorttable;
+            var orig_init = sorttable_ns.SortTable.init;
+            var sorttable_init_called = false;
+            sorttable_ns.SortTable.init = function(force_refresh) {
+                sorttable_init_called = force_refresh;
+            };
+
             // We manipulate the cached model data - delete a bug.
             var bugs = window.LP.cache.bugs;
             // Delete the first bug.
@@ -167,6 +175,9 @@
             var branch_row_selector =
                 '#sharing-table-body tr[id=shared-branch-2]';
             Y.Assert.isNotNull(Y.one(branch_row_selector));
+            // The sorting data is initialised.
+            Y.Assert.isTrue(sorttable_init_called);
+            sorttable_ns.SortTable.init = orig_init;
         },
 
         // When the branch revoke link is clicked, the correct event is
@@ -237,4 +248,6 @@
 
 }, '0.1', { 'requires':
     [ 'test', 'console', 'event', 'node-event-simulate',
-        'lp.registry.sharing.sharingdetails']});
+      'lp.registry.sharing.sharingdetails',
+      'lp.app.sorttable'
+    ]});

=== modified file 'lib/lp/registry/templates/pillar-sharing-details.pt'
--- lib/lp/registry/templates/pillar-sharing-details.pt	2012-04-12 09:22:03 +0000
+++ lib/lp/registry/templates/pillar-sharing-details.pt	2012-04-16 13:29:19 +0000
@@ -47,7 +47,7 @@
       <thead>
         <tr>
           <th colspan="4" width="">
-            <a href="#" class="sortheader">Subscribed Bug Report or Branch</a>
+            Subscribed Bug Report or Branch
           </th>
           <th>
             Information Type