UI-9384 - Preserve KPI titles when migrated KPI widgets from 4.3 to 5.0

[Nouzbe]

This ensures that KPI titles manually entered by users on Atoti UI 4.3 are still honored when they open their dashboards after a 4.3 -> 5.0 content migration.

The changes might seem bigger than expected. It is because both Atoti UI 4.3 and 5.0 use the unique names of its members in order to identify a tuple, but they don't do it in the same order unfortunately.

Atoti UI 5.0 does:

• fields on columns (including measures if present)
• fields on rows (including measures if present)

Atoti UI 4.3 does:

• fields on rows (including measures if present)
• fields on columns (including measures if present)

[antoinetissier]

Just a few questions but it's looking good !

[antoinetissier]

You could use our quote function

[antoinetissier]

Same here

[antoinetissier]

It would be nice to add this to the error report instead of console.logging IMO

[Nouzbe]

I agree. I initially did not see how to do it without refactoring, but it seems that it is possible using a PartialMigrationError.

[antoinetissier]

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 ?

[Nouzbe]

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 has doesSupportMeasuresRedirection: false, deriveMappingFromMdx does not include the ALL_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 the ALL_MEASURES tile and then to filter it out from the mapping before returning the migrated widget state.

[antoinetissier]

Thanks for the explanation.

An alternative which also keeps things simple IMO:

const measuresAxisName = getMeasuresAxisName(mdx);
const positionOfAllMeasuresWithinAxis = getMeasuresPositionOnAxis(
   mdx.axes.find((axis) => axis.name === measuresAxisName)!,
);

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.

[Nouzbe]

@antoinetissier can you have a look at the last commit and make sure this is what you had in mind?

[antoinetissier]

Thanks for assigning back to me.

Maybe I'm missing something, but I was imagining something like the following, where we compute the measures position on the axis once and for all, and insert at the right position based on it for each tuple.

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 numberOfColumnFields = mapping.columns?.length ?? 0;

  const mdx = legactWidgetState.query.mdx;
  const measuresAxisName = getMeasuresAxisName(mdx);
  const measuresAxis = mdx.axes.find((axis) => axis.name === measuresAxisName);
  let measuresPositionInTuple = -1;
  if (measuresAxis) {
    const measuresPositionOnAxis = getMeasuresPositionOnAxis(measuresAxis);
    measuresPositionInTuple =
      measuresAxisName === "columns"
        ? measuresPositionOnAxis
        : numberOfColumnFields + measuresPositionOnAxis;
  }

  // Atoti UI 5.0 KPI widgets expect tuple keys to express members in the following order:
  // - column fields first
  // - then row fields
  const ordinalFields = [...(mapping.columns ?? []), ...(mapping.rows ?? [])];

  legacyTitles.forEach(({ title, tuple }) => {
    const memberUniqueNames: string[] = [];

    ordinalFields.forEach((field) => {
      if (field.type === "hierarchy") {
        const hierarchyUniqueName = quote(
          field.dimensionName,
          field.hierarchyName,
        );
        const namePath = tuple[hierarchyUniqueName];
        if (namePath) {
          memberUniqueNames.push(
            `${hierarchyUniqueName}.${quote(...namePath)}`,
          );
        }
      }
    });

    if (measuresPositionInTuple > -1) {
      const measureName = tuple[quote("Measures", "Measures")];
      memberUniqueNames.splice(
        measuresPositionInTuple,
        0,
        quote("Measures", measureName),
      );
    }

    const tupleKey = memberUniqueNames.join(",");
    migratedTitles[tupleKey] = title;
  });

  return migratedTitles;
}

[Nouzbe]

@antoinetissier I added another commit going your way. The noticeable adaptation is that I added legacyMdx to the arguments of getMigratedKpiTitles, in order to not have to parse it twice (as it's already done in migrateKpi).

If you're happy with this, we can merge.

[antoinetissier]

Looks great !
Re-reading the code, I am just noticing this 999513e#diff-a9ab507bbcfeb1b4251d5fded05f4930172c9f29fb65a73ab51246091dea1c53R118-R123
Can it actually happen that a hierarchy is not expressed in the tuple (i.e. that namePath is undefined) ?

• If it should not legitimately happen (which I think is the case) I'd rather be explicit and throw a clear error if it does.
• If it can legitimately happen, then I don't think my proposition works, because then the index of measures within the tuple can vary.

[antoinetissier]

I think it'd be helpful to preserve the error message when wrapping it in PartialMigrationError, to help consumers of the CLI fix that error.

[Nouzbe]

Can it actually happen that a hierarchy is not expressed in the tuple (i.e. that namePath is undefined) ?

It can legitimately happen:

• add Currency and create a title
• remove Currency
• add City and create a title

=> There is a tuple in legacyTitles which does not have any entry for [Geography].[City]. If we throw an error here in this case, it means that we fail to migrate the KPI titles for this widget. So I think that we shouldn't throw here.

I also think that your suggestion still works in this case.

[antoinetissier]

Right, with this in mind we shouldn't throw, although then cleaning up the obsolete titles would be better than not doing anything IMO.

Whether you prefer to implement the cleanup or not it looks good to me !

[Nouzbe]

I'll merge this: I consider the cleanup as out of scope here. I think that it would be nice to have it as a part of Admin UI, rather than coupled to the migration script. For instance, we could have a "Clean up" button that does an audit of the content and offers clean up actions to the user, like removing stale references to tuples from widget states or deleting saved dashboards/widgets/filters that do not have an entry under structure.