-
Notifications
You must be signed in to change notification settings - Fork 1
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-9384 - Preserve KPI titles when migrated KPI widgets from 4.3 to 5.0 #124
Merged
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
61ad7e1
UI-9384 - Preserve KPI titles when migrated KPI widgets from 4.3 to 5.0
Nouzbe 22c597c
Use quote
Nouzbe 027dea4
Add error to report
Nouzbe 6c22730
Explicitly re-add the allMeasures tile at the correct position outsid…
Nouzbe 999513e
Antoine's way
Nouzbe 28d43d7
Antoine's suggestion and fix test
Nouzbe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import { dataModelsForTests } from "@activeviam/data-model-5.0"; | ||
import { getMigratedKpiTitles } from "./getMigratedKpiTitles"; | ||
import { legacyKpi } from "./__test_resources__/legacyKpi"; | ||
|
||
const cube = dataModelsForTests.sandbox.catalogs[0].cubes[0]; | ||
|
||
describe("getMigratedKpiTitles", () => { | ||
it("returns the migrated KPI titles corresponding to the legacy KPI state, ready to be used in Atoti UI 5.0", () => { | ||
const migratedKpiTitles = getMigratedKpiTitles(legacyKpi, { | ||
cube, | ||
mapping: { | ||
columns: [{ type: "allMeasures" }], | ||
measures: [{ type: "measure", measureName: "contributors.COUNT" }], | ||
rows: [ | ||
{ | ||
type: "hierarchy", | ||
dimensionName: "Currency", | ||
hierarchyName: "Currency", | ||
levelName: "Currency", | ||
}, | ||
], | ||
}, | ||
}); | ||
expect(migratedKpiTitles).toMatchInlineSnapshot(` | ||
{ | ||
"[Measures].[contributors.COUNT],[Currency].[Currency].[AllMember].[EUR, USD]": "Hello World", | ||
} | ||
`); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
import { | ||
Cube, | ||
DataVisualizationWidgetMapping, | ||
KpiWidgetState, | ||
MdxUnknownCompoundIdentifier, | ||
parse, | ||
} from "@activeviam/activeui-sdk-5.0"; | ||
import { getSpecificCompoundIdentifier, quote } from "@activeviam/mdx-5.0"; | ||
|
||
interface LegacyKpiTitle { | ||
title: string; | ||
tuple: { [hierarchyUniqueName: string]: string[] }; | ||
} | ||
|
||
/** | ||
* Returns the legacy KPI titles attached to `legacyKpiState`. | ||
*/ | ||
function _getLegacyKpiTitles( | ||
// Legacy widget states are not typed. | ||
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types | ||
legacyKpiState: any, | ||
{ cube }: { cube: Cube }, | ||
): LegacyKpiTitle[] { | ||
const locations = | ||
legacyKpiState?.value?.body?.configuration?.featuredValues?.locations; | ||
|
||
if (!locations) { | ||
return []; | ||
} | ||
|
||
const legacyTitles: LegacyKpiTitle[] = []; | ||
|
||
for (const tupleKey in locations) { | ||
const { title } = locations[tupleKey]; | ||
const tuple: { | ||
[hierarchyUniqueName: string]: string[]; | ||
} = {}; | ||
const memberUniqueNames = tupleKey.split(/,(?![^\[]*\])/); | ||
for (const memberUniqueName of memberUniqueNames) { | ||
const compoundIdentifier = | ||
parse<MdxUnknownCompoundIdentifier>(memberUniqueName); | ||
const specificCompoundIdentifier = getSpecificCompoundIdentifier( | ||
compoundIdentifier, | ||
{ cube }, | ||
); | ||
if (specificCompoundIdentifier.type === "measure") { | ||
tuple[`[Measures].[Measures]`] = [ | ||
specificCompoundIdentifier.measureName, | ||
]; | ||
} else if (specificCompoundIdentifier.type === "member") { | ||
const hierarchyUniqueName = quote( | ||
specificCompoundIdentifier.dimensionName, | ||
specificCompoundIdentifier.hierarchyName, | ||
); | ||
tuple[hierarchyUniqueName] = specificCompoundIdentifier.path; | ||
} | ||
} | ||
legacyTitles.push({ title, tuple }); | ||
} | ||
|
||
return legacyTitles; | ||
} | ||
|
||
/** | ||
* Returns the migrated KPI widget titles corresponding to legacyKpiState. | ||
*/ | ||
export function getMigratedKpiTitles( | ||
// Legacy widget states are not typed. | ||
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types | ||
legacyKpiState: any, | ||
{ mapping, cube }: { mapping: DataVisualizationWidgetMapping; cube: Cube }, | ||
): KpiWidgetState["titles"] { | ||
const legacyTitles = _getLegacyKpiTitles(legacyKpiState, { cube }); | ||
const migratedTitles: KpiWidgetState["titles"] = {}; | ||
|
||
const memberUniqueNames: string[] = []; | ||
legacyTitles.forEach(({ title, tuple }) => { | ||
// Atoti UI 5.0 KPI widgets expect tuple keys to express members in the following order: | ||
// - column fields first | ||
// - then row fields | ||
const fields = [...(mapping.columns ?? []), ...(mapping.rows ?? [])]; | ||
fields.forEach((field) => { | ||
if (field.type === "allMeasures") { | ||
const measureName = tuple[`[Measures].[Measures]`]?.[0]; | ||
if (measureName) { | ||
memberUniqueNames.push(`[Measures].[${measureName}]`); | ||
} | ||
} else if (field.type === "hierarchy") { | ||
const hierarchyUniqueName = quote( | ||
field.dimensionName, | ||
field.hierarchyName, | ||
); | ||
const namePath = tuple[hierarchyUniqueName]; | ||
if (namePath) { | ||
memberUniqueNames.push( | ||
`${hierarchyUniqueName}.${quote(...namePath)}`, | ||
); | ||
} | ||
} | ||
}); | ||
const tupleKey = memberUniqueNames.join(","); | ||
migratedTitles[tupleKey] = title; | ||
}); | ||
|
||
return migratedTitles; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,8 @@ import { _getQueryInLegacyWidgetState } from "./_getQueryInLegacyWidgetState"; | |
import { _getTargetCubeFromServerUrl } from "./_getTargetCubeFromServerUrl"; | ||
import { _migrateQuery } from "./_migrateQuery"; | ||
import { produce } from "immer"; | ||
import { getMigratedKpiTitles } from "./getMigratedKpiTitles"; | ||
import { PartialMigrationError } from "../PartialMigrationError"; | ||
|
||
const moveExpressionToWithClause = ( | ||
draft: any, | ||
|
@@ -189,7 +191,13 @@ export function migrateKpi( | |
const mapping = deriveMappingFromMdx({ | ||
mdx: query.mdx, | ||
cube, | ||
widgetPlugin: pluginWidgetKpi, | ||
widgetPlugin: { | ||
...pluginWidgetKpi, | ||
// This makes the "allMeasures" tile part of the generated mapping. | ||
// This tile indicates the position of the measures on the queries axes, necessary to migrate KPI titles. | ||
// It is then removed from the mapping, as Atoti UI 5.0 does not expect it. | ||
doesSupportMeasuresRedirection: true, | ||
}, | ||
}); | ||
|
||
const migratedWidgetState: KpiWidgetState = { | ||
|
@@ -206,6 +214,29 @@ export function migrateKpi( | |
}), | ||
}; | ||
|
||
try { | ||
const migratedTitles = getMigratedKpiTitles(legacyKpiState, { | ||
cube, | ||
mapping, | ||
}); | ||
if (migratedTitles && Object.keys(migratedTitles).length > 0) { | ||
migratedWidgetState.titles = migratedTitles; | ||
} | ||
} catch (error) { | ||
// Migrating the KPI titles is a best effort. | ||
// The migration script should not fail if this part errors. | ||
throw new PartialMigrationError( | ||
`Could not migrate the titles of the featured values widget named "${legacyKpiState.name}"`, | ||
serializeWidgetState(migratedWidgetState), | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be helpful to preserve the error message when wrapping it in |
||
} | ||
|
||
Object.keys(mapping).forEach((attributeName) => { | ||
mapping[attributeName] = mapping[attributeName].filter( | ||
(field) => field.type !== "allMeasures", | ||
); | ||
}); | ||
|
||
const serializedWidgetState = serializeWidgetState(migratedWidgetState); | ||
|
||
if (isUsingUnsupportedUpdateMode) { | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand: can't we just recognize for each post-widget-state-migration mapping attribute whether there are measures mapped to it ? And then migrate the titles based on that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is the position of
ALL_MEASURES
relative to level fields in the rows and columns axes. Indeed, the order of the measure or level identifiers in the tuple matters for KPI titles to work in Atoti UI 5. They have to correspond to their order on the cellset axes.But since
pluginWidgetKpi
hasdoesSupportMeasuresRedirection: false
,deriveMappingFromMdx
does not include theALL_MEASURES
tile. This prevents us from knowing where the measure unique name should be placed in the tuple.The simplest solution I found to that problem is to derive the mapping with
doesSupportMeasuresRedirection: true
in order to receive theALL_MEASURES
tile and then to filter it out from the mapping before returning the migrated widget state.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
An alternative which also keeps things simple IMO:
This is what we do in
deriveGenericMappingFromMdx
.I have a preference for it because it's more explicit and avoids relying on a "trick".
But feel free to keep the current version if you like it better.