Fix Pivot Visualization should not be saving data (#4174)

getredash · Sep 25, 2019 · 780fbce · 780fbce
1 parent 2c77c21
commit 780fbce
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 25 deletions.
diff --git a/client/app/visualizations/pivot/Renderer.jsx b/client/app/visualizations/pivot/Renderer.jsx
@@ -1,4 +1,4 @@
-import React, { useState, useEffect } from 'react';
+import React, { useState, useEffect, useMemo } from 'react';
 import { get, find, pick, map, mapValues } from 'lodash';
 import PivotTableUI from 'react-pivottable/PivotTableUI';
 import { RendererPropTypes } from '@/visualizations';
@@ -8,7 +8,6 @@ import 'react-pivottable/pivottable.css';
 import './renderer.less';
 
 const VALID_OPTIONS = [
-  'data',
   'rows',
   'cols',
   'vals',
@@ -33,15 +32,16 @@ function formatRows({ rows, columns }) {
 }
 
 export default function Renderer({ data, options, onOptionsChange }) {
-  const [config, setConfig] = useState({ data: formatRows(data), ...options });
+  const [config, setConfig] = useState({ ...options });
+  const dataRows = useMemo(() => formatRows(data), [data]);
 
   useEffect(() => {
-    setConfig({ data: formatRows(data), ...options });
-  }, [data, options]);
+    setConfig({ ...options });
+  }, [options]);
 
   const onChange = (updatedOptions) => {
     const validOptions = pick(updatedOptions, VALID_OPTIONS);
-    setConfig(validOptions);
+    setConfig({ ...validOptions });
     onOptionsChange(validOptions);
   };
 
@@ -57,7 +57,7 @@ export default function Renderer({ data, options, onOptionsChange }) {
       data-hide-column-totals={hideColumnTotals || null}
       data-test="PivotTableVisualization"
     >
-      <PivotTableUI {...pick(config, VALID_OPTIONS)} onChange={onChange} />
+      <PivotTableUI {...pick(config, VALID_OPTIONS)} data={dataRows} onChange={onChange} />
     </div>
   );
 }

diff --git a/client/cypress/integration/visualizations/pivot_spec.js b/client/cypress/integration/visualizations/pivot_spec.js
@@ -16,9 +16,25 @@ const SQL = `
   SELECT 'c' AS stage1, 'c1' AS stage2, 19 AS value UNION ALL
   SELECT 'c' AS stage1, 'c2' AS stage2, 92 AS value UNION ALL
   SELECT 'c' AS stage1, 'c3' AS stage2, 63 AS value UNION ALL
-  SELECT 'c' AS stage1, 'c4' AS stage2, 44 AS value
+  SELECT 'c' AS stage1, 'c4' AS stage2, 44 AS value\
 `;
 
+function createPivotThroughUI(visualizationName, options = {}) {
+  cy.getByTestId('NewVisualization').click();
+  cy.getByTestId('VisualizationType').click();
+  cy.getByTestId('VisualizationType.PIVOT').click();
+  cy.getByTestId('VisualizationName').clear().type(visualizationName);
+  if (options.hideControls) {
+    cy.getByTestId('PivotEditor.HideControls').click();
+    cy.getByTestId('VisualizationPreview')
+      .find('table')
+      .find('.pvtAxisContainer, .pvtRenderer, .pvtVals')
+      .should('be.not.visible');
+  }
+  cy.getByTestId('VisualizationPreview').find('table').should('exist');
+  cy.getByTestId('EditVisualizationDialog').contains('button', 'Save').click();
+}
+
 describe('Pivot', () => {
   beforeEach(() => {
     cy.login();
@@ -32,13 +48,8 @@ describe('Pivot', () => {
     cy.getByTestId('ExecuteButton').click();
 
     const visualizationName = 'Pivot';
+    createPivotThroughUI(visualizationName);
 
-    cy.getByTestId('NewVisualization').click();
-    cy.getByTestId('VisualizationType').click();
-    cy.getByTestId('VisualizationType.PIVOT').click();
-    cy.getByTestId('VisualizationName').clear().type(visualizationName);
-    cy.getByTestId('VisualizationPreview').find('table').should('exist');
-    cy.getByTestId('EditVisualizationDialog').contains('button', 'Save').click();
     cy.getByTestId('QueryPageVisualizationTabs').contains('li', visualizationName).should('exist');
   });
 
@@ -51,18 +62,8 @@ describe('Pivot', () => {
     cy.server();
     cy.route('POST', 'api/visualizations').as('SaveVisualization');
 
-    cy.getByTestId('NewVisualization').click();
-    cy.getByTestId('VisualizationType').click();
-    cy.getByTestId('VisualizationType.PIVOT').click();
-    cy.getByTestId('VisualizationName').clear().type(visualizationName);
+    createPivotThroughUI(visualizationName, { hideControls: true });
 
-    cy.getByTestId('PivotEditor.HideControls').click();
-    cy.getByTestId('VisualizationPreview')
-      .find('table')
-      .find('.pvtAxisContainer, .pvtRenderer, .pvtVals')
-      .should('be.not.visible');
-
-    cy.getByTestId('EditVisualizationDialog').contains('button', 'Save').click();
     cy.wait('@SaveVisualization').then((xhr) => {
       const visualizationId = get(xhr, 'response.body.id');
       // Added visualization should also have hidden controls
@@ -73,6 +74,39 @@ describe('Pivot', () => {
     });
   });
 
+  it('updates the visualization when results change', function () {
+    const options = {
+      aggregatorName: 'Count',
+      data: [], // force it to have a data object, although it shouldn't
+      controls: { enabled: false },
+      cols: ['stage1'],
+      rows: ['stage2'],
+      vals: ['value'],
+    };
+
+    createVisualization(this.queryId, 'PIVOT', 'Pivot', options)
+      .then((visualization) => {
+        cy.visit(`queries/${this.queryId}/source#${visualization.id}`);
+        cy.getByTestId('ExecuteButton').click();
+
+        cy.getByTestId(`QueryPageVisualization${visualization.id}`)
+          .find('.pvtGrandTotal')
+          .should('have.text', '11'); // assert number of rows is 11
+
+        cy.getByTestId('QueryEditor')
+          .get('.ace_text-input')
+          .first()
+          .focus()
+          .type(" UNION ALL\nSELECT 'c' AS stage1, 'c5' AS stage2, 55 AS value");
+        cy.getByTestId('SaveButton').click();
+        cy.getByTestId('ExecuteButton').click();
+
+        cy.getByTestId(`QueryPageVisualization${visualization.id}`)
+          .find('.pvtGrandTotal')
+          .should('have.text', '12'); // assert number of rows is 12
+      });
+  });
+
   it('takes a snapshot with different configured Pivots', function () {
     const options = {
       aggregatorName: 'Sum',