RN-702: Re-instate excluding columns when using the 'exclude' merge f…

…unction (#4248)
beyondessential · Jan 10, 2023 · 301e3b9 · 301e3b9
1 parent 0ec18ac
commit 301e3b9
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 52 deletions.
diff --git a/packages/report-server/src/__tests__/reportBuilder/transform/mergeRows.test.ts b/packages/report-server/src/__tests__/reportBuilder/transform/mergeRows.test.ts
@@ -283,10 +283,11 @@ describe('mergeRows', () => {
         },
       ]);
       expect(transform(TransformTable.fromRows(MERGEABLE_ANALYTICS))).toStrictEqual(
-        TransformTable.fromRows(
-          [{ period: '20200101' }, { period: '20200102' }, { period: '20200103' }],
-          ['period', 'organisationUnit', 'BCD1', 'BCD2'], // excludes values, but keeps columns
-        ),
+        TransformTable.fromRows([
+          { period: '20200101' },
+          { period: '20200102' },
+          { period: '20200103' },
+        ]),
       );
     });
 

diff --git a/packages/report-server/src/__tests__/reportBuilder/transform/parser/parser.test.ts b/packages/report-server/src/__tests__/reportBuilder/transform/parser/parser.test.ts
@@ -121,31 +121,6 @@ describe('parser', () => {
   });
 
   describe('in transforms', () => {
-    it('mergeRows supports parser lookups on where', () => {
-      const transform = buildTransform([
-        {
-          transform: 'mergeRows',
-          using: {
-            organisationUnit: 'exclude',
-            period: 'exclude',
-            '*': 'sum',
-          },
-          where: "=eq($organisationUnit, 'TO')",
-        },
-      ]);
-      expect(transform(TransformTable.fromRows(PARSABLE_ANALYTICS))).toStrictEqual(
-        TransformTable.fromRows(
-          [
-            { BCD1: 11 },
-            { period: '20200101', organisationUnit: 'PG', BCD1: 7 },
-            { period: '20200102', organisationUnit: 'PG', BCD1: 8 },
-            { period: '20200103', organisationUnit: 'PG', BCD1: 2 },
-          ],
-          ['period', 'organisationUnit', 'BCD1'],
-        ),
-      );
-    });
-
     it('excludeRows supports parser lookups on where', () => {
       const transform = buildTransform([
         {

diff --git a/packages/report-server/src/reportBuilder/transform/functions/mergeRows/mergeRows.ts b/packages/report-server/src/reportBuilder/transform/functions/mergeRows/mergeRows.ts
@@ -6,10 +6,7 @@
 import { yup } from '@tupaia/utils';
 import { yupTsUtils } from '@tupaia/tsutils';
 
-import { Context } from '../../../context';
-import { TransformParser } from '../../parser';
 import { mergeStrategies } from './mergeStrategies';
-import { buildWhere } from '../where';
 import { FieldValue, Row } from '../../../types';
 import { buildCreateGroupKey } from './createGroupKey';
 import { buildGetMergeStrategy } from './getMergeStrategy';
@@ -19,7 +16,6 @@ import { TransformTable } from '../../table';
 type MergeRowsParams = {
   createGroupKey: (row: Row) => string;
   getMergeStrategy: (field: string) => keyof typeof mergeStrategies;
-  where: (parser: TransformParser) => boolean;
 };
 
 const optionalMergeStrategyNameValidator = yup
@@ -65,23 +61,15 @@ type Group = {
   [columnName: string]: FieldValue[];
 };
 
-const groupRows = (table: TransformTable, params: MergeRowsParams, context: Context) => {
+const groupRows = (table: TransformTable, params: MergeRowsParams) => {
   const groupsByKey: Record<string, Group> = {};
-  const parser = new TransformParser(table, context);
-  const ungroupedRows: Row[] = []; // Rows that don't match the 'where' clause are left ungrouped
 
   table.getRows().forEach((row: Row) => {
-    if (!params.where(parser)) {
-      ungroupedRows.push(row);
-      parser.next();
-      return;
-    }
     const groupKey = params.createGroupKey(row);
     addRowToGroup(groupsByKey, groupKey, row); // mutates groupsByKey
-    parser.next();
   });
 
-  return { groups: Object.values(groupsByKey), ungroupedRows };
+  return Object.values(groupsByKey);
 };
 
 const addRowToGroup = (groupsByKey: Record<string, Group>, groupKey: string, row: Row) => {
@@ -102,24 +90,32 @@ const addRowToGroup = (groupsByKey: Record<string, Group>, groupKey: string, row
 };
 
 const mergeGroups = (groups: Group[], params: MergeRowsParams) => {
-  return groups.map(group => {
+  const excludedColumns = new Set<string>();
+  const mergedRows = groups.map(group => {
     const mergedRow: Row = {};
     Object.entries(group).forEach(([columnName, groupValues]) => {
       const mergeStrategy = params.getMergeStrategy(columnName);
+
+      // We track the excluded columns so that we can remove them from the TransformTable later
+      if (mergeStrategy === 'exclude') {
+        excludedColumns.add(columnName);
+      }
+
       const mergedValue = mergeStrategies[mergeStrategy](groupValues);
       if (mergedValue !== undefined) {
         mergedRow[columnName] = mergedValue;
       }
     });
     return mergedRow;
   });
+  return { mergedRows, excludedColumns: Array.from(excludedColumns) };
 };
 
-const mergeRows = (table: TransformTable, params: MergeRowsParams, context: Context) => {
-  const { groups, ungroupedRows } = groupRows(table, params, context);
-  const mergedRows = mergeGroups(groups, params);
-  const newRowData = mergedRows.concat(ungroupedRows);
-  return new TransformTable(table.getColumns(), newRowData);
+const mergeRows = (table: TransformTable, params: MergeRowsParams) => {
+  const groups = groupRows(table, params);
+  const { mergedRows, excludedColumns } = mergeGroups(groups, params);
+  const columns = table.getColumns().filter(columnName => !excludedColumns.includes(columnName));
+  return new TransformTable(columns, mergedRows);
 };
 
 const buildParams = (params: unknown): MergeRowsParams => {
@@ -130,11 +126,10 @@ const buildParams = (params: unknown): MergeRowsParams => {
   return {
     createGroupKey: buildCreateGroupKey(groupBy),
     getMergeStrategy: buildGetMergeStrategy(groupBy, using),
-    where: buildWhere(params),
   };
 };
 
-export const buildMergeRows = (params: unknown, context: Context) => {
+export const buildMergeRows = (params: unknown) => {
   const builtParams = buildParams(params);
-  return (table: TransformTable) => mergeRows(table, builtParams, context);
+  return (table: TransformTable) => mergeRows(table, builtParams);
 };