Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI-8857 - Fix unexpected filter MDX update during 4.3 to 5.0 migration #118

Merged
merged 6 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/4.3_to_5.0/_migrateQuery.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ describe("_migrateQuery", () => {
const [migratedQuery, isUsingUnsupportedUpdateMode] = _migrateQuery({
legacyQuery,
cube,
shouldUpdateFiltersMdx: true,
});
expect(migratedQuery).toMatchInlineSnapshot(`
{
Expand Down Expand Up @@ -42,6 +43,7 @@ describe("_migrateQuery", () => {
const [migratedQuery, isUsingUnsupportedUpdateMode] = _migrateQuery({
legacyQuery: { mdx: "" },
cube,
shouldUpdateFiltersMdx: false,
});
expect(migratedQuery).toEqual({
query: { updateMode: "once" },
Expand All @@ -55,8 +57,15 @@ describe("_migrateQuery", () => {
const legacyQuery = {
mdx: "SELECT FROM [EquityDerivativesCube] WHERE [Currency].[Currency].[AllMember].[EUR]",
};
const [{ query, filters }] = _migrateQuery({ legacyQuery, cube })!;
const [{ query, filters }] = _migrateQuery({
legacyQuery,
cube,
shouldUpdateFiltersMdx: true,
})!;

// Filters are unchanged in the query.
expect(stringify(query.mdx!)).toBe("SELECT FROM [EquityDerivativesCube]");

expect(filters.length).toBe(1);
expect(stringify(filters[0])).toBe(
"[Currency].[Currency].[AllMember].[EUR]",
Expand Down Expand Up @@ -99,7 +108,7 @@ describe("_migrateQuery", () => {
{
query: { mdx },
},
] = _migrateQuery({ legacyQuery, cube })!;
] = _migrateQuery({ legacyQuery, cube, shouldUpdateFiltersMdx: true })!;

expect(stringify(mdx!, { indent: true })).toMatchInlineSnapshot(`
"SELECT
Expand Down
11 changes: 9 additions & 2 deletions src/4.3_to_5.0/_migrateQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,11 @@ export interface LegacyQuery {
export const _migrateQuery = <T extends MdxSelect | MdxDrillthrough>({
legacyQuery,
cube,
shouldUpdateFiltersMdx,
}: {
legacyQuery: LegacyQuery;
cube: Cube;
shouldUpdateFiltersMdx: boolean;
}): [
{
query: { mdx?: T; updateMode: UpdateMode };
Expand Down Expand Up @@ -91,13 +93,18 @@ export const _migrateQuery = <T extends MdxSelect | MdxDrillthrough>({
const filters = getFilters(improvedMdx, { cube }).map(
({ mdx: filterMdx }) => filterMdx,
);
const mdxWithoutFilters = setFilters(improvedMdx, { filters: [], cube });

const finalMdx = shouldUpdateFiltersMdx
? // The filters part of the MDX is removed from the query and stored in state.filters.
setFilters(improvedMdx, { filters: [], cube })
: // The filters part of the MDX remains stored as is, in state.query.
improvedMdx;

// TODO UI-5036 Migrate query ranges.
return [
{
// Migrating the query preserves its type.
query: { mdx: mdxWithoutFilters as T, updateMode: migratedUpdateMode },
query: { mdx: finalMdx as T, updateMode: migratedUpdateMode },
filters,
queryContext,
},
Expand Down
47 changes: 37 additions & 10 deletions src/4.3_to_5.0/migrateChart.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ import { servers } from "./__test_resources__/servers";

describe("migrateChart", () => {
it("returns the ActiveUI5 chart widget state corresponding to the given ActiveUI4 chart widget state", () => {
expect(migrateChart(legacyChart, servers)).toMatchInlineSnapshot(`
expect(migrateChart(legacyChart, { servers, shouldUpdateFiltersMdx: true }))
.toMatchInlineSnapshot(`
{
"filters": [],
"mapping": {
Expand Down Expand Up @@ -81,7 +82,9 @@ describe("migrateChart", () => {
});

it("migrates a legacy line chart to a Plotly line chart", () => {
expect(migrateChart(legacyLineChart, servers)).toMatchInlineSnapshot(`
expect(
migrateChart(legacyLineChart, { servers, shouldUpdateFiltersMdx: true }),
).toMatchInlineSnapshot(`
{
"filters": [],
"mapping": {
Expand Down Expand Up @@ -111,7 +114,9 @@ describe("migrateChart", () => {
});

it("migrates a legacy area chart to a Plotly area chart", () => {
expect(migrateChart(legacyAreaChart, servers)).toMatchInlineSnapshot(`
expect(
migrateChart(legacyAreaChart, { servers, shouldUpdateFiltersMdx: true }),
).toMatchInlineSnapshot(`
{
"filters": [],
"mapping": {
Expand Down Expand Up @@ -141,7 +146,12 @@ describe("migrateChart", () => {
});

it("migrates a legacy column chart to a Plotly column chart", () => {
expect(migrateChart(legacyColumnChart, servers)).toMatchInlineSnapshot(`
expect(
migrateChart(legacyColumnChart, {
servers,
shouldUpdateFiltersMdx: true,
}),
).toMatchInlineSnapshot(`
{
"filters": [],
"mapping": {
Expand Down Expand Up @@ -170,7 +180,9 @@ describe("migrateChart", () => {
});

it("migrates a legacy bar chart to a Plotly bar chart", () => {
expect(migrateChart(legacyBarChart, servers)).toMatchInlineSnapshot(`
expect(
migrateChart(legacyBarChart, { servers, shouldUpdateFiltersMdx: true }),
).toMatchInlineSnapshot(`
{
"filters": [],
"mapping": {
Expand Down Expand Up @@ -199,7 +211,9 @@ describe("migrateChart", () => {
});

it("migrates a legacy pie chart to a Plotly pie chart", () => {
expect(migrateChart(legacyPieChart, servers)).toMatchInlineSnapshot(`
expect(
migrateChart(legacyPieChart, { servers, shouldUpdateFiltersMdx: true }),
).toMatchInlineSnapshot(`
{
"filters": [],
"mapping": {
Expand All @@ -225,7 +239,12 @@ describe("migrateChart", () => {
});

it("migrates a legacy scatter plot to a Plotly scatter plot", () => {
expect(migrateChart(legacyScatterPlot, servers)).toMatchInlineSnapshot(`
expect(
migrateChart(legacyScatterPlot, {
servers,
shouldUpdateFiltersMdx: true,
}),
).toMatchInlineSnapshot(`
{
"filters": [],
"mapping": {
Expand Down Expand Up @@ -260,7 +279,9 @@ describe("migrateChart", () => {
});

it("migrates an empty chart widget to a plotly-line-chart", () => {
expect(migrateChart(emptyLegacyChart, servers)).toMatchInlineSnapshot(`
expect(
migrateChart(emptyLegacyChart, { servers, shouldUpdateFiltersMdx: true }),
).toMatchInlineSnapshot(`
{
"filters": [],
"mapping": {
Expand Down Expand Up @@ -298,7 +319,10 @@ describe("migrateChart", () => {
},
);

const chartState = migrateChart(partialLegacyChartState, servers);
const chartState = migrateChart(partialLegacyChartState, {
servers,
shouldUpdateFiltersMdx: true,
});

// Notice that `splitBy`, `horizontalSubplots` and `verticalSubplots` are present in the migrated chart state,
// even though they are missing in the input legacy chart state.
Expand Down Expand Up @@ -330,7 +354,10 @@ describe("migrateChart", () => {
// ActiveUI 4 charts did not have this capability: "all measures" could never be moved.
// For this ActiveUI 5 feature to work, charts supporting measures redirection in ActiveUI 5 should see the "ALL_MEASURES" tile added to their mapping during the migration.

const migratedChartState = migrateChart(legacyChart, servers);
const migratedChartState = migrateChart(legacyChart, {
servers,
shouldUpdateFiltersMdx: true,
});

// Even though the ALL_MEASURES tile is missing in the legacy chart state, the migrated state does contain it.
expect(migratedChartState.mapping.splitBy).toContain("ALL_MEASURES");
Expand Down
13 changes: 11 additions & 2 deletions src/4.3_to_5.0/migrateChart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,13 @@ export function migrateChart(
// Legacy widget states are not typed.
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
legacyChartState: any,
servers: { [serverKey: string]: { dataModel: DataModel; url: string } },
{
servers,
shouldUpdateFiltersMdx,
}: {
servers: { [serverKey: string]: { dataModel: DataModel; url: string } };
shouldUpdateFiltersMdx: boolean;
},
): PlotlyWidgetState<"serialized"> {
const widgetName = legacyChartState?.name;
const {
Expand Down Expand Up @@ -267,7 +273,7 @@ export function migrateChart(
const [
{ query: migratedQuery, filters: extractedFilters, queryContext },
isUsingUnsupportedUpdateMode,
] = _migrateQuery<MdxSelect>({ legacyQuery, cube });
] = _migrateQuery<MdxSelect>({ legacyQuery, cube, shouldUpdateFiltersMdx });

// If there is no MDX in the query, the type does not matter: it can be considered a stringified query.
const query = (
Expand Down Expand Up @@ -299,6 +305,9 @@ export function migrateChart(
serverKey,
name: widgetName,
widgetKey: migratedWidgetKey,
...(!shouldUpdateFiltersMdx && {
areFiltersDrivenByMdx: true,
}),
};

if (isUsingUnsupportedUpdateMode) {
Expand Down
7 changes: 7 additions & 0 deletions src/4.3_to_5.0/migrateDashboard.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ describe("migrateDashboard", () => {
it("turns the pages content from arrays into maps", () => {
const [dashboard] = migrateDashboard(legacyDashboard, {
servers,
shouldUpdateFiltersMdx: true,
});

expect(dashboard.pages["p-0"].content).toMatchInlineSnapshot(`
Expand Down Expand Up @@ -143,6 +144,7 @@ describe("migrateDashboard", () => {
it("flattens the page layouts", () => {
const [dashboard] = migrateDashboard(legacyDashboard, {
servers,
shouldUpdateFiltersMdx: true,
});
expect(dashboard.pages["p-0"].layout).toMatchInlineSnapshot(`
{
Expand Down Expand Up @@ -179,6 +181,7 @@ describe("migrateDashboard", () => {
// Due to the flattening, the context values from the first cube that are also defined in the second cube are overriden.
const [dashboard] = migrateDashboard(legacyDashboard, {
servers,
shouldUpdateFiltersMdx: true,
});
expect(dashboard.queryContext).toMatchInlineSnapshot(`
[
Expand All @@ -201,6 +204,7 @@ describe("migrateDashboard", () => {
it("migrates page context values", () => {
const [dashboard] = migrateDashboard(legacyDashboard, {
servers,
shouldUpdateFiltersMdx: true,
});
expect(_mapValues(dashboard.pages, ({ queryContext }) => queryContext))
.toMatchInlineSnapshot(`
Expand Down Expand Up @@ -240,6 +244,7 @@ describe("migrateDashboard", () => {
} as unknown as LegacyDashboardState;
const [emptyDashboard] = migrateDashboard(legacyEmptyDashboard, {
servers,
shouldUpdateFiltersMdx: true,
});
expect(emptyDashboard).toMatchInlineSnapshot(`
{
Expand Down Expand Up @@ -283,6 +288,7 @@ describe("migrateDashboard", () => {
const [dashboard] = migrateDashboard(legacyDashboard, {
servers,
keysOfWidgetPluginsToRemove,
shouldUpdateFiltersMdx: true,
});

const { content, layout } = dashboard.pages["p-0"];
Expand Down Expand Up @@ -334,6 +340,7 @@ describe("migrateDashboard", () => {
legacyDashboardWithDisconnectedWidget,
{
servers,
shouldUpdateFiltersMdx: true,
},
);

Expand Down
8 changes: 5 additions & 3 deletions src/4.3_to_5.0/migrateDashboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,13 @@ export function migrateDashboard(
servers,
keysOfWidgetPluginsToRemove,
doesReportIncludeStacks,
shouldUpdateFiltersMdx,
treeTableColumnWidth,
}: {
servers: { [serverKey: string]: { dataModel: DataModel; url: string } };
keysOfWidgetPluginsToRemove?: string[];
doesReportIncludeStacks?: boolean;
shouldUpdateFiltersMdx: boolean;
treeTableColumnWidth?: [number, number];
},
): [DashboardState<"serialized">, PartialDashboardErrorReport?] {
Expand Down Expand Up @@ -93,11 +95,11 @@ export function migrateDashboard(
} else {
let migratedWidget: AWidgetState<"serialized"> | undefined = undefined;
try {
migratedWidget = migrateWidget(
widget.bookmark,
migratedWidget = migrateWidget(widget.bookmark, {
servers,
treeTableColumnWidth,
);
shouldUpdateFiltersMdx,
});
if (isDisconnectedWidget(widget.bookmark)) {
keysOfPageDisconnectedWidgets.add(leafKey);
}
Expand Down
16 changes: 12 additions & 4 deletions src/4.3_to_5.0/migrateDrillthrough.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@ import { servers } from "./__test_resources__/servers";

describe("migrateDrillthrough", () => {
it("returns the ActiveUI5 drillthrough-table widget state corresponding to the given ActiveUI4 drillthrough widget state", () => {
expect(migrateDrillthrough(legacyDrillthrough, servers))
.toMatchInlineSnapshot(`
expect(
migrateDrillthrough(legacyDrillthrough, {
servers,
shouldUpdateFiltersMdx: true,
}),
).toMatchInlineSnapshot(`
{
"columnWidths": {},
"filters": [
Expand All @@ -25,8 +29,12 @@ describe("migrateDrillthrough", () => {
});

it("migrates an empty drillthrough widget", () => {
expect(migrateDrillthrough(emptyLegacyDrillthrough, servers))
.toMatchInlineSnapshot(`
expect(
migrateDrillthrough(emptyLegacyDrillthrough, {
servers,
shouldUpdateFiltersMdx: true,
}),
).toMatchInlineSnapshot(`
{
"columnWidths": {},
"filters": [],
Expand Down
12 changes: 11 additions & 1 deletion src/4.3_to_5.0/migrateDrillthrough.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,13 @@ export function migrateDrillthrough(
// Legacy widget states are not typed.
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
legacyDrillthroughState: any,
servers: { [serverKey: string]: { dataModel: DataModel; url: string } },
{
servers,
shouldUpdateFiltersMdx,
}: {
servers: { [serverKey: string]: { dataModel: DataModel; url: string } };
shouldUpdateFiltersMdx: boolean;
},
): AWidgetState<"serialized"> {
const legacyQuery = _getQueryInLegacyWidgetState(legacyDrillthroughState);
const legacyMdx = legacyQuery.mdx
Expand All @@ -36,6 +42,7 @@ export function migrateDrillthrough(
_migrateQuery<MdxDrillthrough>({
legacyQuery,
cube,
shouldUpdateFiltersMdx,
});

const legacyColumns =
Expand All @@ -62,6 +69,9 @@ export function migrateDrillthrough(
serverKey,
widgetKey: "drillthrough-table",
columnWidths,
...(!shouldUpdateFiltersMdx && {
areFiltersDrivenByMdx: true,
}),
};

const serializedWidgetState = serializeWidgetState(migratedWidgetState);
Expand Down
Loading
Loading