Merge pull request #445 from jpmorganchase/fix-expand

Fix regression on view expand
finos · Feb 22, 2019 · d0e63ff · d0e63ff
2 parents 982e5e5 + 9545402
commit d0e63ff
Show file tree

Hide file tree

Showing 6 changed files with 54 additions and 21 deletions.
diff --git a/cpp/perspective/src/cpp/view.cpp b/cpp/perspective/src/cpp/view.cpp
@@ -52,28 +52,26 @@ View<CTX_T>::get_row_expanded(std::int32_t idx) {
     return m_ctx->unity_get_row_expanded(idx);
 }
 
-template <typename CTX_T>
-t_index
-View<CTX_T>::expand(std::int32_t idx) {
-    return m_ctx->open(idx);
-}
-
 template <>
 t_index
-View<t_ctx0>::expand(std::int32_t idx) {
+View<t_ctx0>::expand(std::int32_t idx, std::int32_t row_pivot_length) {
     return idx;
 }
 
 template <>
 t_index
-View<t_ctx2>::expand(std::int32_t idx) {
-    return m_ctx->open(t_header::HEADER_ROW, idx);
+View<t_ctx1>::expand(std::int32_t idx, std::int32_t row_pivot_length) {
+    return m_ctx->open(idx);
 }
 
-template <typename CTX_T>
+template <>
 t_index
-View<CTX_T>::collapse(std::int32_t idx) {
-    return m_ctx->close(idx);
+View<t_ctx2>::expand(std::int32_t idx, std::int32_t row_pivot_length) {
+    if (m_ctx->unity_get_row_depth(idx) < t_uindex(row_pivot_length)) {
+        return m_ctx->open(t_header::HEADER_ROW, idx);
+    } else {
+        return idx;
+    }
 }
 
 template <>
@@ -82,26 +80,32 @@ View<t_ctx0>::collapse(std::int32_t idx) {
     return idx;
 }
 
+template <>
+t_index
+View<t_ctx1>::collapse(std::int32_t idx) {
+    return m_ctx->close(idx);
+}
+
 template <>
 t_index
 View<t_ctx2>::collapse(std::int32_t idx) {
     return m_ctx->close(t_header::HEADER_ROW, idx);
 }
 
-template <typename CTX_T>
+template <>
 void
-View<CTX_T>::set_depth(std::int32_t depth, std::int32_t row_pivot_length) {
+View<t_ctx0>::set_depth(std::int32_t depth, std::int32_t row_pivot_length) {}
+
+template <>
+void
+View<t_ctx1>::set_depth(std::int32_t depth, std::int32_t row_pivot_length) {
     if (row_pivot_length >= depth) {
         m_ctx->set_depth(depth);
     } else {
         std::cout << "Cannot expand past " << std::to_string(row_pivot_length) << std::endl;
     }
 }
 
-template <>
-void
-View<t_ctx0>::set_depth(std::int32_t depth, std::int32_t row_pivot_length) {}
-
 template <>
 void
 View<t_ctx2>::set_depth(std::int32_t depth, std::int32_t row_pivot_length) {

diff --git a/cpp/perspective/src/include/perspective/view.h b/cpp/perspective/src/include/perspective/view.h
@@ -38,7 +38,7 @@ class PERSPECTIVE_EXPORT View {
 
     std::map<std::string, std::string> schema();
 
-    t_index expand(std::int32_t idx);
+    t_index expand(std::int32_t idx, std::int32_t row_pivot_length);
     t_index collapse(std::int32_t idx);
     void set_depth(std::int32_t depth, std::int32_t row_pivot_length);
 

diff --git a/packages/perspective-viewer-hypergrid/test/js/superstore.spec.js b/packages/perspective-viewer-hypergrid/test/js/superstore.spec.js
@@ -32,6 +32,21 @@ utils.with_server({}, () => {
             simple_tests.default();
 
             describe("expand/collapse", () => {
+                test.capture("should not be able to expand past number of row pivots", async page => {
+                    const viewer = await page.$("perspective-viewer");
+                    await page.shadow_click("perspective-viewer", "#config_button");
+                    await page.evaluate(element => element.setAttribute("row-pivots", '["Region"]'), viewer);
+                    await page.waitForSelector("perspective-viewer:not([updating])");
+                    await page.evaluate(element => element.setAttribute("column-pivots", '["Sub-Category"]'), viewer);
+
+                    await page.evaluate(element => {
+                        // 2 is greater than no. of row pivots
+                        element.view.expand(2);
+                        element.notifyResize();
+                    }, viewer);
+                    await page.waitForSelector("perspective-viewer:not([updating])");
+                });
+
                 test.capture("collapses to depth smaller than viewport", async page => {
                     const viewer = await page.$("perspective-viewer");
                     await page.evaluate(element => element.setAttribute("row-pivots", '["Category","State"]'), viewer);

diff --git a/packages/perspective-viewer-hypergrid/test/results/results.json b/packages/perspective-viewer-hypergrid/test/results/results.json
@@ -21,5 +21,6 @@
     "hypergrid.html/perspective dispatches perspective-click event with correct properties.": "f5d6bf82299b2e5f403cdb69b67645fe",
     "hypergrid.html/perspective dispatches perspective-click event with one filter.": "b7a10ece1d5084742f9a2d88bb68984d",
     "hypergrid.html/perspective dispatches perspective-click event with filters.": "3025f83c828d468207b04d8454e94fda",
-    "superstore.html/handles flush": "a5d1bad309edf83ceef190dd19d867ec"
+    "superstore.html/handles flush": "a5d1bad309edf83ceef190dd19d867ec",
+    "superstore.html/should not be able to expand past number of row pivots": "2db398671e7d98f2fa3c7ebabf4a3176"
 }
diff --git a/packages/perspective/src/js/perspective.js b/packages/perspective/src/js/perspective.js
@@ -586,7 +586,7 @@ export default function(Module) {
      * @returns {Promise<void>}
      */
     view.prototype.expand = async function(idx) {
-        return this._View.expand(idx);
+        return this._View.expand(idx, this.config.row_pivot.length);
     };
 
     /**

diff --git a/packages/perspective/test/js/pivots.js b/packages/perspective/test/js/pivots.js
@@ -574,4 +574,17 @@ module.exports = perspective => {
             table.delete();
         });
     });
+
+    describe("Pivot table operations", function() {
+        it("Should not expand past number of row pivots", async function() {
+            var table = perspective.table(data);
+            var view = table.view({
+                row_pivot: ["x"],
+                column_pivot: ["y"]
+            });
+            var expanded_idx = await view.expand(2);
+            // invalid expands return the index
+            expect(expanded_idx).toEqual(2);
+        });
+    });
 };