fix(plugins): Period over Period Plugin cleanup

superset-frontend/package.json

@@ -109,10 +109,10 @@
     "@superset-ui/legacy-preset-chart-nvd3": "file:./plugins/legacy-preset-chart-nvd3",
     "@superset-ui/plugin-chart-echarts": "file:./plugins/plugin-chart-echarts",
     "@superset-ui/plugin-chart-handlebars": "file:./plugins/plugin-chart-handlebars",
+    "@superset-ui/plugin-chart-period-over-period-kpi": "file:./plugins/plugin-chart-period-over-period-kpi",
     "@superset-ui/plugin-chart-pivot-table": "file:./plugins/plugin-chart-pivot-table",
     "@superset-ui/plugin-chart-table": "file:./plugins/plugin-chart-table",
     "@superset-ui/plugin-chart-word-cloud": "file:./plugins/plugin-chart-word-cloud",
-    "@superset-ui/plugin-chart-period-over-period-kpi": "file:./plugins/plugin-chart-period-over-period-kpi",
     "@superset-ui/switchboard": "file:./packages/superset-ui-switchboard",
     "@types/d3-format": "^3.0.1",
     "@visx/axis": "^3.0.1",

superset-frontend/plugins/plugin-chart-period-over-period-kpi/README.md

@@ -18,6 +18,7 @@ npm run dev
 ```
 
 To add the package to Superset, go to the `superset-frontend` subdirectory in your Superset source folder (assuming both the `custom-viz` plugin and `superset` repos are in the same root directory) and run
+
 ```
 npm i -S ../../custom-viz
 ```
@@ -54,6 +55,7 @@ import { CustomViz } from 'custom-viz';
 ```
 
 to import the plugin and later add the following to the array that's passed to the `plugins` property:
+
 ```js
 new CustomViz().configure({ key: 'custom-viz' }),
 ```

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/PopKPI.tsx

@@ -16,53 +16,18 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React, { useEffect, createRef } from 'react';
-import { 
-  styled,
-  } from '@superset-ui/core';
-import { PopKPIProps, PopKPIStylesProps } from './types';
+import React, { createRef } from 'react';
+import { css, styled, useTheme } from '@superset-ui/core';
+import { PopKPIComparisonValueStyleProps, PopKPIProps } from './types';
 
-// The following Styles component is a <div> element, which has been styled using Emotion
-// For docs, visit https://emotion.sh/docs/styled
-
-// Theming variables are provided for your use via a ThemeProvider
-// imported from @superset-ui/core. For variables available, please visit
-// https://github.com/apache-superset/superset-ui/blob/master/packages/superset-ui-core/src/style/index.ts
-
-const Styles = styled.div<PopKPIStylesProps>`
-
-  font-family: ${({ theme }) => theme.typography.families.sansSerif};
-  position: relative;
-  display: flex;
-  flex-direction: column;
-  justify-content: center;
-  padding: ${({ theme }) => theme.tdUnit * 4}px;
-  border-radius: ${({ theme }) => theme.tdUnit * 2}px;
-  height: ${({ height }) => height}px;
-  width: ${({ width }) => width}px;
-`;
-
-const BigValueContainer = styled.div`
-  font-size: ${props=> props.headerFontSize ? props.headerFontSize : 60}px;
-  font-weight: ${({ theme }) => theme.typography.weights.normal};
-  text-align: center;
-`;
-
-const TableContainer = styled.div`
-  width: 100%;
-  display: table;
-`
-
-const ComparisonContainer = styled.div`
-  display: table-row;
-`;
-
-const ComparisonValue = styled.div`
-  font-weight: ${({ theme }) => theme.typography.weights.light};
-  width: 33%;
-  display: table-cell;
-  font-size: ${props=> props.subheaderFontSize ? props.subheaderFontSize : 20}px;
-  text-align: center;
+const ComparisonValue = styled.div<PopKPIComparisonValueStyleProps>`
+  ${({ theme, subheaderFontSize }) => `
+    font-weight: ${theme.typography.weights.light};
+    width: 33%;
+    display: table-cell;
+    font-size: ${subheaderFontSize || 20}px;
+    text-align: center;
+  `}
 `;
 
 export default function PopKPI(props: PopKPIProps) {
@@ -78,28 +43,54 @@ export default function PopKPI(props: PopKPIProps) {
   } = props;
 
   const rootElem = createRef<HTMLDivElement>();
+  const theme = useTheme();
 
-  useEffect(() => {
-    const root = rootElem.current as HTMLElement;
-  });
+  const wrapperDivStyles = css`
+    font-family: ${theme.typography.families.sansSerif};
+    position: relative;
+    display: flex;
+    flex-direction: column;
+    justify-content: center;
+    padding: ${theme.gridUnit * 4}px;
+    border-radius: ${theme.gridUnit * 2}px;
+    height: ${height}px;
+    width: ${width}px;
+  `;
 
-  return (
-    <Styles
-      ref={rootElem}
-      boldText={props.boldText}
-      headerFontSize={props.headerFontSize}
-      height={height}
-      width={width}
-    >
+  const bigValueContainerStyles = css`
+    font-size: ${headerFontSize || 60}px;
+    font-weight: ${theme.typography.weights.normal};
+    text-align: center;
+  `;
 
-      <BigValueContainer headerFontSize={headerFontSize}>{bigNumber}</BigValueContainer>
-      <TableContainer>
-        <ComparisonContainer>
-          <ComparisonValue subheaderFontSize={subheaderFontSize}> #: {prevNumber}</ComparisonValue>
-          <ComparisonValue subheaderFontSize={subheaderFontSize}> Δ: {valueDifference}</ComparisonValue>
-          <ComparisonValue subheaderFontSize={subheaderFontSize}> %: {percentDifference}</ComparisonValue>
-        </ComparisonContainer>
-      </TableContainer>
-    </Styles>
+  return (
+    <div ref={rootElem} css={wrapperDivStyles}>
+      <div css={bigValueContainerStyles}>{bigNumber}</div>
+      <div
+        css={css`
+          width: 100%;
+          display: table;
+        `}
+      >
+        <div
+          css={css`
+            display: table-row;
+          `}
+        >
+          <ComparisonValue subheaderFontSize={subheaderFontSize}>
+            {' '}
+            #: {prevNumber}
+          </ComparisonValue>
+          <ComparisonValue subheaderFontSize={subheaderFontSize}>
+            {' '}
+            Δ: {valueDifference}
+          </ComparisonValue>
+          <ComparisonValue subheaderFontSize={subheaderFontSize}>
+            {' '}
+            %: {percentDifference}
+          </ComparisonValue>
+        </div>
+      </div>
+    </div>
   );
 }

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/buildQuery.ts

@@ -16,7 +16,11 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { buildQueryContext, QueryFormData } from '@superset-ui/core';
+import {
+  AdhocFilter,
+  buildQueryContext,
+  QueryFormData,
+} from '@superset-ui/core';
 import moment, { Moment } from 'moment';
 
 /**
@@ -42,25 +46,25 @@ function getSinceUntil(
   relativeEnd: string | null = null,
 ): MomentTuple {
   const separator = ' : ';
-  const _relativeStart = relativeStart || "today";
-  const _relativeEnd = relativeEnd || "today";
+  const effectiveRelativeStart = relativeStart || 'today';
+  const effectiveRelativeEnd = relativeEnd || 'today';
 
   if (!timeRange) {
     return [null, null];
   }
 
   let modTimeRange: string | null = timeRange;
 
-  if (timeRange === 'NO_TIME_RANGE' || timeRange === '_(NO_TIME_RANGE)'){
+  if (timeRange === 'NO_TIME_RANGE' || timeRange === '_(NO_TIME_RANGE)') {
     return [null, null];
   }
 
   if (timeRange?.startsWith('last') && !timeRange.includes(separator)) {
-    modTimeRange = timeRange + separator + _relativeEnd;
+    modTimeRange = timeRange + separator + effectiveRelativeEnd;
   }
 
   if (timeRange?.startsWith('next') && !timeRange.includes(separator)) {
-    modTimeRange = _relativeStart + separator + timeRange;
+    modTimeRange = effectiveRelativeStart + separator + timeRange;
   }
 
   if (
@@ -179,12 +183,15 @@ function getSinceUntil(
   return [_since, _until];
 }
 
-function calculatePrev(startDate: Moment | null, endDate: Moment | null, calcType: String) {
-
-  if (!startDate || !endDate){
-    return [null, null]
+function calculatePrev(
+  startDate: Moment | null,
+  endDate: Moment | null,
+  calcType: String,
+) {
+  if (!startDate || !endDate) {
+    return [null, null];
   }
-  
+
   const daysBetween = endDate.diff(startDate, 'days');
 
   let startDatePrev = moment();
@@ -219,53 +226,63 @@ export default function buildQuery(formData: QueryFormData) {
     },
   ]);
 
+  const timeFilterIndex: number =
+    formData.adhoc_filters?.findIndex(
+      filter => 'operator' in filter && filter.operator === 'TEMPORAL_RANGE',
+    ) ?? -1;
 
-  const timeFilter: any = formData.adhoc_filters?.find(
-    ({ operator }: { operator: string }) => operator === 'TEMPORAL_RANGE',
-  );
+  const timeFilter: AdhocFilter | null =
+    timeFilterIndex !== -1 && formData.adhoc_filters
+      ? formData.adhoc_filters[timeFilterIndex]
+      : null;
 
-  const timeFilterIndex: any = formData.adhoc_filters?.findIndex(
-    ({ operator }: { operator: string }) => operator === 'TEMPORAL_RANGE',
-  );
+  let testSince = null;
+  let testUntil = null;
 
-  const [testSince, testUntil] = getSinceUntil(
-    timeFilter.comparator.toLowerCase(),
-  );
-
-  let formDataB: QueryFormData;
+  if (
+    timeFilter &&
+    'comparator' in timeFilter &&
+    typeof timeFilter.comparator === 'string'
+  ) {
+    [testSince, testUntil] = getSinceUntil(
+      timeFilter.comparator.toLocaleLowerCase(),
+    );
+  }
 
-  if (timeComparison!='c'){
+  let formDataB: QueryFormData;
 
-  const [prevStartDateMoment, prevEndDateMoment] = calculatePrev(
-    testSince,
-    testUntil,
-    timeComparison,
-  );
+  if (timeComparison !== 'c') {
+    const [prevStartDateMoment, prevEndDateMoment] = calculatePrev(
+      testSince,
+      testUntil,
+      timeComparison,
+    );
 
     const queryBComparator = `${prevStartDateMoment?.format(
-    'YYYY-MM-DDTHH:mm:ss',
-  )} : ${prevEndDateMoment?.format('YYYY-MM-DDTHH:mm:ss')}`;
+      'YYYY-MM-DDTHH:mm:ss',
+    )} : ${prevEndDateMoment?.format('YYYY-MM-DDTHH:mm:ss')}`;
 
-  const queryBFilter = {
+    const queryBFilter: any = {
       ...timeFilter,
-      comparator: queryBComparator.replace(/Z/g, '')
-    }
+      comparator: queryBComparator.replace(/Z/g, ''),
+    };
 
-    const otherFilters = formData.adhoc_filters?.filter((_value: any, index: number) => timeFilterIndex !== index);
-    const queryBFilters = otherFilters ? [queryBFilter, ...otherFilters] : [queryBFilter];
-
-    formDataB= {
+    const otherFilters = formData.adhoc_filters?.filter(
+      (_value: any, index: number) => timeFilterIndex !== index,
+    );
+    const queryBFilters = otherFilters
+      ? [queryBFilter, ...otherFilters]
+      : [queryBFilter];
+
+    formDataB = {
       ...formData,
       adhoc_filters: queryBFilters,
-    }
-
+    };
   } else {
-
-    formDataB= {
+    formDataB = {
       ...formData,
       adhoc_filters: formData.adhoc_custom,
-    }
-
+    };
   }
 
   const queryContextB = buildQueryContext(formDataB, baseQueryObject => [
@@ -275,7 +292,6 @@ export default function buildQuery(formData: QueryFormData) {
     },
   ]);
 
-
   return {
     ...queryContextA,
     queries: [...queryContextA.queries, ...queryContextB.queries],

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/plugin/transformProps.ts

@@ -16,15 +16,26 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import moment from 'moment';
 import {
   ChartProps,
-  TimeseriesDataRecord,
   getMetricLabel,
   getValueFormatter,
   NumberFormats,
   getNumberFormatter,
 } from '@superset-ui/core';
 
+export const parseMetricValue = (metricValue: number | string | null) => {
+  if (typeof metricValue === 'string') {
+    const dateObject = moment.utc(metricValue, moment.ISO_8601, true);
+    if (dateObject.isValid()) {
+      return dateObject.valueOf();
+    }
+    return 0;
+  }
+  return metricValue ?? 0;
+};
+
 export default function transformProps(chartProps: ChartProps) {
   /**
    * This function is called after a successful response has been
@@ -71,12 +82,14 @@ export default function transformProps(chartProps: ChartProps) {
     currencyFormat,
     subheaderFontSize,
   } = formData;
-  const dataA = queriesData[0].data as TimeseriesDataRecord[];
-  const dataB = queriesData[1].data as TimeseriesDataRecord[];
+  const { data: dataA = [] } = queriesData[0];
+  const { data: dataB = [] } = queriesData[1];
   const data = dataA;
   const metricName = getMetricLabel(metrics[0]);
-  let bigNumber = data.length === 0 ? null : data[0][metricName];
-  let prevNumber = dataB.length === 0 ? null : dataB[0][metricName];
+  let bigNumber: number | string =
+    data.length === 0 ? 0 : parseMetricValue(data[0][metricName]);
+  let prevNumber: number | string =
+    data.length === 0 ? 0 : parseMetricValue(dataB[0][metricName]);
 
   const numberFormatter = getValueFormatter(
     metrics[0],
@@ -97,13 +110,13 @@ export default function transformProps(chartProps: ChartProps) {
     NumberFormats.PERCENT_SIGNED_1_POINT,
   );
 
-  let valueDifference = bigNumber - prevNumber;
+  let valueDifference: number | string = bigNumber - prevNumber;
 
   const percentDifferenceNum = prevNumber
     ? (bigNumber - prevNumber) / Math.abs(prevNumber)
     : 0;
 
-  const compType= compTitles[formData.timeComparison]
+  const compType = compTitles[formData.timeComparison];
   bigNumber = numberFormatter(bigNumber);
   prevNumber = numberFormatter(prevNumber);
   valueDifference = numberFormatter(valueDifference);

superset-frontend/plugins/plugin-chart-period-over-period-kpi/src/types.ts

@@ -35,6 +35,10 @@ interface PopKPICustomizeProps {
   headerText: string;
 }
 
+export interface PopKPIComparisonValueStyleProps {
+  subheaderFontSize?: keyof typeof supersetTheme.typography.sizes;
+}
+
 export type PopKPIQueryFormData = QueryFormData &
   PopKPIStylesProps &
   PopKPICustomizeProps;

superset-frontend/plugins/plugin-chart-period-over-period-kpi/tsconfig.json

@@ -22,4 +22,4 @@
       "path": "../../packages/superset-ui-core"
     }
   ]
-}
+}

superset-frontend/src/visualizations/presets/MainPreset.js

@@ -76,8 +76,8 @@ import {
 } from 'src/filters/components';
 import { PivotTableChartPlugin as PivotTableChartPluginV2 } from '@superset-ui/plugin-chart-pivot-table';
 import { HandlebarsChartPlugin } from '@superset-ui/plugin-chart-handlebars';
+import { PopKPIPlugin } from '@superset-ui/plugin-chart-period-over-period-kpi';
 import TimeTableChartPlugin from '../TimeTable';
-import { PopKPIPlugin } from '@superset-ui/plugin-chart-period-over-period-kpi'
 
 export default class MainPreset extends Preset {
   constructor() {
@@ -156,7 +156,7 @@ export default class MainPreset extends Preset {
         new EchartsSunburstChartPlugin().configure({ key: 'sunburst_v2' }),
         new HandlebarsChartPlugin().configure({ key: 'handlebars' }),
         new EchartsBubbleChartPlugin().configure({ key: 'bubble_v2' }),
-        new PopKPIPlugin().configure({ key: 'pop_kpi'}),
+        new PopKPIPlugin().configure({ key: 'pop_kpi' }),
       ],
     });
   }