fix: replace datamask with key from new key value api (apache#17680)

* afirst stage to ccheck to get initial datamask * clean up code and update typescript * remove consoles * fix ts and update copy dashboard url * use key when one doesn't exists * lint clean up * fix errors * add suggested changes * remove line * add tests and add changes for copydashboard * fix lint * fix lint * fix lint * Update superset-frontend/src/dashboard/components/Header/index.jsx Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> * add timeout * fix test * fix test, add qs to cypress and add suggestions * add suggestions * fix lint * more suggested changes for backwards comapat * fix lint * cleanup naming and add qs parse to tests * Update superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> * Update superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com> * more changes and fix lint * remove nativefiler param * fix path * remove con * simplify logic Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
casual-precision · Feb 10, 2022 · cfd50eb · cfd50eb
1 parent 19772ec
commit cfd50eb
Show file tree

Hide file tree

Showing 24 changed files with 303 additions and 77 deletions.
diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/key_value.test.ts b/superset-frontend/cypress-base/cypress/integration/dashboard/key_value.test.ts
@@ -0,0 +1,54 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import qs from 'querystringify';
+import {
+  WORLD_HEALTH_DASHBOARD,
+  WORLD_HEALTH_CHARTS,
+  waitForChartLoad,
+} from './dashboard.helper';
+
+interface QueryString {
+  native_filters_key: string;
+}
+
+describe('nativefiler url param key', () => {
+  // const urlParams = { param1: '123', param2: 'abc' };
+  before(() => {
+    cy.login();
+    cy.visit(WORLD_HEALTH_DASHBOARD);
+    WORLD_HEALTH_CHARTS.forEach(waitForChartLoad);
+  });
+  beforeEach(() => {
+    cy.login();
+  });
+  let initialFilterKey: string;
+  it('should have cachekey in nativefilter param', () => {
+    cy.location().then(loc => {
+      const queryParams = qs.parse(loc.search) as QueryString;
+      expect(typeof queryParams.native_filters_key).eq('string');
+    });
+  });
+
+  it('should have different key when page reloads', () => {
+    cy.location().then(loc => {
+      const queryParams = qs.parse(loc.search) as QueryString;
+      expect(queryParams.native_filters_key).not.equal(initialFilterKey);
+    });
+  });
+});
diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts b/superset-frontend/cypress-base/cypress/integration/dashboard/nativeFilters.test.ts
@@ -16,6 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import qs from 'querystring';
 import { dashboardView, nativeFilters } from 'cypress/support/directories';
 import { testItems } from './dashboard.helper';
 import { DASHBOARD_LIST } from '../dashboard_list/dashboard_list.helper';
@@ -93,6 +94,15 @@ describe('Nativefilters Sanity test', () => {
     cy.get(nativeFilters.modal.container).should('be.visible');
   });
   it('User can add a new native filter', () => {
+    let filterKey: string;
+    const removeFirstChar = (search: string) =>
+      search.split('').slice(1, search.length).join('');
+    cy.wait(3000);
+    cy.location().then(loc => {
+      const queryParams = qs.parse(removeFirstChar(loc.search));
+      filterKey = queryParams.native_filters_key as string;
+      expect(typeof filterKey).eq('string');
+    });
     cy.get(nativeFilters.filterFromDashboardView.expand).click({ force: true });
     cy.get(nativeFilters.createFilterButton).should('be.visible').click();
     cy.get(nativeFilters.modal.container)
@@ -115,7 +125,7 @@ describe('Nativefilters Sanity test', () => {
     cy.wait(5000);
     cy.get(nativeFilters.filtersPanel.filterInfoInput)
       .last()
-      .should('be.visible')
+      .should('be.visible', { timeout: 30000 })
       .click({ force: true });
     cy.get(nativeFilters.filtersPanel.filterInfoInput)
       .last()
@@ -128,6 +138,13 @@ describe('Nativefilters Sanity test', () => {
       .contains('Save')
       .should('be.visible')
       .click();
+    cy.wait(3000);
+    cy.location().then(loc => {
+      const queryParams = qs.parse(removeFirstChar(loc.search));
+      const newfilterKey = queryParams.native_filters_key;
+      expect(newfilterKey).not.eq(filterKey);
+    });
+    cy.wait(3000);
     cy.get(nativeFilters.modal.container).should('not.exist');
   });
   it('User can delete a native filter', () => {

diff --git a/superset-frontend/cypress-base/package-lock.json b/superset-frontend/cypress-base/package-lock.json
diff --git a/superset-frontend/cypress-base/package.json b/superset-frontend/cypress-base/package.json
@@ -12,11 +12,13 @@
   "dependencies": {
     "@cypress/code-coverage": "^3.9.11",
     "@superset-ui/core": "^0.18.8",
+    "querystringify": "^2.2.0",
     "react-dom": "^16.13.0",
     "rison": "^0.1.1",
     "shortid": "^2.2.15"
   },
   "devDependencies": {
+    "@types/querystringify": "^2.0.0",
     "cypress": "^7.0.0",
     "eslint-plugin-cypress": "^2.12.1"
   },

diff --git a/superset-frontend/spec/javascripts/dashboard/util/getDashboardUrl_spec.js b/superset-frontend/spec/javascripts/dashboard/util/getDashboardUrl_spec.js
@@ -73,25 +73,21 @@ describe('getChartIdsFromLayout', () => {
     );
   });
 
-  it('should encode native filters', () => {
+  it('should process native filters key', () => {
+    const windowSpy = jest.spyOn(window, 'window', 'get');
+    windowSpy.mockImplementation(() => ({
+      location: {
+        origin: 'https://localhost',
+        search:
+          '?preselect_filters=%7B%7D&native_filters_key=024380498jdkjf-2094838',
+      },
+    }));
+
     const urlWithNativeFilters = getDashboardUrl({
       pathname: 'path',
-      dataMask: {
-        'NATIVE_FILTER-foo123': {
-          filterState: {
-            label: 'custom label',
-            value: ['a', 'b'],
-          },
-        },
-        'NATIVE_FILTER-bar456': {
-          filterState: {
-            value: undefined,
-          },
-        },
-      },
     });
     expect(urlWithNativeFilters).toBe(
-      'path?preselect_filters=%7B%7D&native_filters=%28NATIVE_FILTER-bar456%3A%28filterState%3A%28value%3A%21n%29%29%2CNATIVE_FILTER-foo123%3A%28filterState%3A%28label%3A%27custom+label%27%2Cvalue%3A%21%28a%2Cb%29%29%29%29',
+      'path?preselect_filters=%7B%7D&native_filters_key=024380498jdkjf-2094838',
     );
   });
 });
diff --git a/superset-frontend/src/common/hooks/useUrlShortener.ts b/superset-frontend/src/common/hooks/useUrlShortener.ts
@@ -23,9 +23,9 @@ export function useUrlShortener(url: string): Function {
   const [update, setUpdate] = useState(false);
   const [shortUrl, setShortUrl] = useState('');
 
-  async function getShortUrl() {
+  async function getShortUrl(urlOverride?: string) {
     if (update) {
-      const newShortUrl = await getShortUrlUtil(url);
+      const newShortUrl = await getShortUrlUtil(urlOverride || url);
       setShortUrl(newShortUrl);
       setUpdate(false);
       return newShortUrl;

diff --git a/superset-frontend/src/constants.ts b/superset-frontend/src/constants.ts
@@ -39,6 +39,10 @@ export const URL_PARAMS = {
     name: 'native_filters',
     type: 'rison',
   },
+  nativeFiltersKey: {
+    name: 'native_filters_key',
+    type: 'string',
+  },
   filterSet: {
     name: 'filter_set',
     type: 'string',

diff --git a/superset-frontend/src/dashboard/actions/hydrate.js b/superset-frontend/src/dashboard/actions/hydrate.js
@@ -67,6 +67,7 @@ export const hydrateDashboard =
     dashboardData,
     chartData,
     filterboxMigrationState = FILTER_BOX_MIGRATION_STATES.NOOP,
+    dataMaskApplied,
   ) =>
   (dispatch, getState) => {
     const { user, common } = getState();
@@ -378,10 +379,11 @@ export const hydrateDashboard =
           slice_can_edit: findPermission('can_slice', 'Superset', roles),
           common: {
             // legacy, please use state.common instead
-            flash_messages: common.flash_messages,
-            conf: common.conf,
+            flash_messages: common?.flash_messages,
+            conf: common?.conf,
           },
         },
+        dataMask: dataMaskApplied,
         dashboardFilters,
         nativeFilters,
         dashboardState: {

diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
@@ -235,7 +235,7 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
   );
 
   const dashboardRoot = dashboardLayout[DASHBOARD_ROOT_ID];
-  const rootChildId = dashboardRoot.children[0];
+  const rootChildId = dashboardRoot?.children[0];
   const topLevelTabs =
     rootChildId !== DASHBOARD_GRID_ID
       ? dashboardLayout[rootChildId]

diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/utils.ts b/superset-frontend/src/dashboard/components/DashboardBuilder/utils.ts
@@ -25,7 +25,7 @@ import findTabIndexByComponentId from 'src/dashboard/util/findTabIndexByComponen
 
 export const getRootLevelTabsComponent = (dashboardLayout: DashboardLayout) => {
   const dashboardRoot = dashboardLayout[DASHBOARD_ROOT_ID];
-  const rootChildId = dashboardRoot.children[0];
+  const rootChildId = dashboardRoot?.children[0];
   return rootChildId === DASHBOARD_GRID_ID
     ? dashboardLayout[DASHBOARD_ROOT_ID]
     : dashboardLayout[rootChildId];

diff --git a/superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/index.jsx b/superset-frontend/src/dashboard/components/Header/HeaderActionsDropdown/index.jsx
@@ -193,7 +193,6 @@ class HeaderActionsDropdown extends React.PureComponent {
       dashboardTitle,
       dashboardId,
       dashboardInfo,
-      dataMask,
       refreshFrequency,
       shouldPersistRefreshFrequency,
       editMode,
@@ -220,7 +219,6 @@ class HeaderActionsDropdown extends React.PureComponent {
     const emailBody = t('Check out this dashboard: ');
 
     const url = getDashboardUrl({
-      dataMask,
       pathname: window.location.pathname,
       filters: getActiveFilters(),
       hash: window.location.hash,
@@ -266,6 +264,7 @@ class HeaderActionsDropdown extends React.PureComponent {
             emailBody={emailBody}
             addSuccessToast={addSuccessToast}
             addDangerToast={addDangerToast}
+            dashboardId={dashboardId}
           />
         )}
         <Menu.Item

diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx
@@ -173,13 +173,15 @@ class Header extends React.PureComponent {
     this.startPeriodicRender(refreshFrequency * 1000);
     if (this.canAddReports()) {
       // this is in case there is an anonymous user.
-      this.props.fetchUISpecificReport(
-        user.userId,
-        'dashboard_id',
-        'dashboards',
-        dashboardInfo.id,
-        user.email,
-      );
+      if (Object.entries(dashboardInfo).length) {
+        this.props.fetchUISpecificReport(
+          user.userId,
+          'dashboard_id',
+          'dashboards',
+          dashboardInfo.id,
+          user.email,
+        );
+      }
     }
   }
 
@@ -211,11 +213,11 @@ class Header extends React.PureComponent {
     ) {
       // this is in case there is an anonymous user.
       this.props.fetchUISpecificReport(
-        user.userId,
+        user?.userId,
         'dashboard_id',
         'dashboards',
-        nextProps.dashboardInfo.id,
-        user.email,
+        nextProps?.dashboardInfo?.id,
+        user?.email,
       );
     }
   }
@@ -488,10 +490,10 @@ class Header extends React.PureComponent {
       filterboxMigrationState !== FILTER_BOX_MIGRATION_STATES.REVIEWING;
     const shouldShowReport = !editMode && this.canAddReports();
     const refreshLimit =
-      dashboardInfo.common.conf.SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT;
+      dashboardInfo.common?.conf?.SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT;
     const refreshWarning =
-      dashboardInfo.common.conf
-        .SUPERSET_DASHBOARD_PERIODICAL_REFRESH_WARNING_MESSAGE;
+      dashboardInfo.common?.conf
+        ?.SUPERSET_DASHBOARD_PERIODICAL_REFRESH_WARNING_MESSAGE;
 
     const handleOnPropertiesChange = updates => {
       const { dashboardInfoChanged, dashboardTitleChanged } = this.props;
@@ -529,7 +531,7 @@ class Header extends React.PureComponent {
             canEdit={userCanEdit}
             canSave={userCanSaveAs}
           />
-          {user?.userId && (
+          {user?.userId && dashboardInfo?.id && (
             <FaveStar
               itemId={dashboardInfo.id}
               fetchFaveStar={this.props.fetchFaveStar}

diff --git a/superset-frontend/src/dashboard/components/menu/ShareMenuItems/ShareMenuItems.test.tsx b/superset-frontend/src/dashboard/components/menu/ShareMenuItems/ShareMenuItems.test.tsx
@@ -27,14 +27,16 @@ import ShareMenuItems from '.';
 
 const spy = jest.spyOn(copyTextToClipboard, 'default');
 
+const DASHBOARD_ID = '26';
 const createProps = () => ({
   addDangerToast: jest.fn(),
   addSuccessToast: jest.fn(),
-  url: '/superset/dashboard/26/?preselect_filters=%7B%7D',
+  url: `/superset/dashboard/${DASHBOARD_ID}/?preselect_filters=%7B%7D`,
   copyMenuItemTitle: 'Copy dashboard URL',
   emailMenuItemTitle: 'Share dashboard by email',
   emailSubject: 'Superset dashboard COVID Vaccine Dashboard',
   emailBody: 'Check out this dashboard: ',
+  dashboardId: DASHBOARD_ID,
 });
 
 const { location } = window;