Merge pull request #214 from jpmorganchase/cc_aggregate_fix

Computed columns retain persistent aggregates and show/hide
finos · Aug 27, 2018 · 81c1bcd · 81c1bcd
2 parents 5f30f89 + 485ce03
commit 81c1bcd
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 24 deletions.
diff --git a/packages/perspective-viewer/src/js/view.js b/packages/perspective-viewer/src/js/view.js
@@ -249,6 +249,7 @@ function column_visibility_clicked(ev) {
     this._update_column_view(cols);
 }
 
+// todo: apply to computed columns, separate branch
 function column_aggregate_clicked() {
     let aggregates = get_aggregate_attribute.call(this);
     let new_aggregates = this._get_view_aggregates();
@@ -339,21 +340,6 @@ async function loadTable(table) {
         table.computed_schema()
     ]);
 
-    // todo: computed columns do NOT persist across refreshes
-    // fixme: find better implementation than strip computed columns
-    if(!_.isEmpty(computed_schema)) {
-        const computed_columns = _.keys(computed_schema);
-        for (let i = 0; i < computed_columns.length; i++) {
-            const cc = computed_columns[i];
-            if (cols.includes(cc)) {
-                cols.splice(cols.indexOf(cc), 1);
-            }
-            if (_.has(schema, cc)) {
-                delete schema[cc];
-            }
-        }
-    }
-
     this._inactive_columns.innerHTML = "";
     this._active_columns.innerHTML = "";
 
@@ -377,9 +363,6 @@ async function loadTable(table) {
         return r;
     });
 
-    // fixme: better approach please
-    const computed_cols = _.pairs(computed_schema);
-
     // Update Aggregates.
     let aggregates = [];
     const found = {};
@@ -417,6 +400,22 @@ async function loadTable(table) {
     // Update column rows.
     let shown = JSON.parse(this.getAttribute('columns') || "[]").filter(x => cols.indexOf(x) > -1);
 
+    // strip computed columns from sorted columns & schema - place at end
+    if(!_.isEmpty(computed_schema)) {
+        const computed_columns = _.keys(computed_schema);
+        for (let i = 0; i < computed_columns.length; i++) {
+            const cc = computed_columns[i];
+            if (cols.includes(cc)) {
+                cols.splice(cols.indexOf(cc), 1);
+            }
+            if (_.has(schema, cc)) {
+                delete schema[cc];
+            }
+        }
+    }
+
+    const computed_cols = _.pairs(computed_schema);
+
     if (!this.hasAttribute('columns') || shown.length === 0) {
         for (let x of cols) {
             let aggregate = aggregates
@@ -430,9 +429,10 @@ async function loadTable(table) {
         for (let cc of computed_cols) {
             let cc_data = _format_computed_data(cc);
             let aggregate = aggregates
-                .filter(a => a.column === cc[0])
+                .filter(a => a.column === cc_data.column_name)
                 .map(a => a.op)[0];
-            let row = new_row.call(this, cc[0], cc[1].type, aggregate, null, null, cc_data);
+            console.log(aggregate, cc_data);
+            let row = new_row.call(this, cc_data.column_name, cc_data.type, aggregate, null, null, cc_data);
             this._inactive_columns.appendChild(row);
         }
 
@@ -450,19 +450,23 @@ async function loadTable(table) {
                 .map(a => a.op)[0];
             let row = new_row.call(this, x, schema[x], aggregate);
             this._inactive_columns.appendChild(row);
-            if (shown.indexOf(x) !== -1) {
+            if (shown.includes(x)) {
                 row.classList.add('active');
             }
         }
 
         // fixme better approach please
         for (let cc of computed_cols) {
             let cc_data = _format_computed_data(cc);
+            console.log(aggregates);
             let aggregate = aggregates
-                .filter(a => a.column === cc[0])
+                .filter(a => a.column === cc_data.column_name)
                 .map(a => a.op)[0];
-            let row = new_row.call(this, cc[0], cc[1].type, aggregate, null, null, cc_data);
+            let row = new_row.call(this, cc_data.column_name, cc_data.type, aggregate, null, null, cc_data);
             this._inactive_columns.appendChild(row);
+            if (shown.includes(cc)) {
+                row.classList.add('active');
+            }
         }
 
         for (let x of shown) {
@@ -862,7 +866,7 @@ class ViewPrivate extends HTMLElement {
         this._table.columns().then((cols) => {
             // edit overwrites last column, otherwise avoid name collision
             if(cols.includes(computed_column_name) && !data.edit) {
-                computed_column_name += (Math.round(Math.random() * 100));
+                computed_column_name += ` ${(Math.round(Math.random() * 100))}`;
             }
 
             const params = [{

diff --git a/packages/perspective-viewer/test/js/computed_column_tests.js b/packages/perspective-viewer/test/js/computed_column_tests.js
@@ -132,4 +132,14 @@ exports.default = function() {
         const viewer = await page.$('perspective-viewer');
         await page.evaluate(element => element.setAttribute('filters', '[["new_cc", "==", "2 Monday"]]'), viewer);
     });
+
+    test.capture("computed column aggregates should persist.", async page => {
+        await add_computed_column(page);
+        const viewer = await page.$('perspective-viewer');
+        await page.evaluate(element => element.setAttribute('row-pivots', '["Quantity"]'), viewer);
+        await page.evaluate(element => element.setAttribute('columns', '["Row ID", "Quantity", "new_cc"]'), viewer);
+        await page.evaluate(element => element.setAttribute('aggregates', '{"new_cc":"any"}'), viewer);
+        await page.evaluate(element => element.setAttribute('columns', '["Row ID", "Quantity"]'), viewer);
+        await page.evaluate(element => element.setAttribute('columns', '["Row ID", "Quantity", "new_cc"]'), viewer);
+    });
 };
diff --git a/packages/perspective-viewer/test/results/results.json b/packages/perspective-viewer/test/results/results.json
@@ -17,6 +17,7 @@
     "superstore.html/pivots by computed column.": "d4467ad3612e90136fa391196e61189b",
     "superstore.html/sorts by computed column.": "7a4c989e68159de9f2ed70a1e6380ef9",
     "superstore.html/filters by computed column.": "12d31929677602612fdb26ee04958eef",
+    "superstore.html/computed column aggregates should persist.": "0dbc9d15dc1a9d62d62e94b534447787",
     "blank.html/Handles reloading with a schema.": "9cda0b27c92efb59599e11dffbad169e",
     "superstore.html/pivots by a column.": "71c2f17f6ade6513574aa0143a84c634",
     "superstore.html/click on add computed column button opens the UI.": "4dd9778ed1f321d928570d7b3e60a473",