UI-9566 - Ensure a best effort migration of the KPI titles even when some titles could not be successfully migrated. #129

NZepeda · 2024-07-29T21:56:34Z

Summary

Issue

As the ticket states, when migrating KPIs containing custom titles, if the migration of one of the titles throws an error, none of the custom titles are migrated.

On this specific ticket, the issue was that when the KPI contained a title with an empty tupleKey as seen below, the process would error out resulting in none of the titles (even the valid ones) being migrated.

"": {
   title: "Title with empty tupleKey"
}

Solution

This PR aims to improve on that by ensuring that even if there is an error, the successfully migrated titles are persisted and the KPI widget is marked as Partially migrated.

Corresponding ticket: https://activeviam.atlassian.net/browse/UI-9566

Reviewing

Ensure the new and updated tests make sense

…e the titles migrated.

src/4.3_to_5.0/getMigratedKpiTitles.ts

NZepeda · 2024-07-30T03:50:01Z

src/4.3_to_5.0/__test_resources__/legacyKpi.ts

+            "": {
+              title: "Empty measure title",
+            },


For this fix, I went with dropping titles that have an empty tuple key and ensuring the rest of the titles are still migrated. However, I'm not 100% sure how one can even land in this state. It doesn't seem like it's valid so I dropped it, but if it is better to ensure this kind of custom title is supported, I can adapt the change to ensure it's also migrated.

…hub.com:activeviam/atoti-ui-migration into task/nze/UI-9566-support-migrating-empty-titles

src/migrateContentServer.test.ts

…is an error while migrating one of the titles the succesfully migrated titles should still be persisted.

NZepeda · 2024-07-31T18:10:28Z

src/4.3_to_5.0/accumulateMigratedKpiTitles.ts

+      if (memberUniqueName) {
+        const compoundIdentifier =
+          parse<MdxUnknownCompoundIdentifier>(memberUniqueName);
+        const specificCompoundIdentifier = getSpecificCompoundIdentifier(
+          compoundIdentifier,
+          { cube },
        );
-        tuple[hierarchyUniqueName] = specificCompoundIdentifier.path;
+        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;
+        }


Looks like a big change but all I did was wrap this logic block in an if statement.

NZepeda · 2024-07-31T18:11:34Z

src/4.3_to_5.0/accumulateMigratedKpiTitles.ts

+    try {
+      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)}`,
+      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")][0];
+        memberUniqueNames.splice(
+          measuresPositionInTuple,
+          0,
+          quote("Measures", measureName),
+        );
      }
-    });
-
-    if (measuresPositionInTuple > -1) {
-      const measureName = tuple[quote("Measures", "Measures")][0];
-      memberUniqueNames.splice(
-        measuresPositionInTuple,
-        0,
-        quote("Measures", measureName),
-      );
-    }

-    const tupleKey = memberUniqueNames.join(",");
-    migratedTitles[tupleKey] = title;
+      const tupleKey = memberUniqueNames.join(",");
+      migratedTitles[tupleKey] = title;
+    } catch (error) {
+      errors.push(error as Error);
+    }


Again, looks like a big change but I just wrapped this logic block in a try-catch.

NZepeda · 2024-07-31T18:14:09Z

src/4.3_to_5.0/accumulateMigratedKpiTitles.ts

+  if (errors.length > 0) {
+    throw new Error(
+      `One or more errors occurred while migrating the titles for the kpi named ${legacyKpiState.name}: ` +
+        errors.map((e) => e.message).join(", "),
+    );
+  }


This function "swallows" any error until all legacyTitles have been attempted to be migrated. This throws at the end to ensure that the kpi widget that is being migrated is marked as partially migrated.

src/migrateContentServer.test.ts

…hub.com:activeviam/atoti-ui-migration into task/nze/UI-9566-support-migrating-empty-titles

Nouzbe

Could you please merge #128 into main, rebase this branch and take the following comment into consideration before reassigning?

Nouzbe · 2024-08-01T10:15:59Z

src/4.3_to_5.0/__test_resources__/smallLegacyUIFolderWithInvalidKpiTitle.ts

+ * The shortened version of the content of the /ui folder on a Content Server, useful for unit tests.
+ * Contains a KPI widget containing a custom title with an empty `tupleKey` for its location.
+ */
+export const smallLegacyUIFolderWithInvalidKpiTitle = {


It would be nice to factorize the common part between this and the test resource introduced in #128, in order to avoid adding redundant content to this repo.

For instance, we could have a emptyLegacyUIFolder test resource and a addLegacyBookmarkToUIFolder test resource function that takes a map of id to legacy bookmark as argument and returns the "small legacy UI folder with ..." object, without mutating emptyLegacyFolder.

…s folder

…n test resource files

…files

…UI folder function

NZepeda · 2024-09-18T21:17:22Z

It would be great to use our new addLegacyBookmarkToUIFolder in order to generate our existing:

smallLegacyUIFolder

smallLegacyUIFolderWithInvalidDashboard

smallLegacyUIFolderWithInvalidFilter

It would still be nice to have them as their own test resources, but this way we would cut down some duplicate content and lower the entropy of this repo.

This is done via one of the latest commits 🫡

NZepeda · 2024-09-18T21:30:06Z

@Nouzbe Your latest suggestions have been addressed in the latest commits 👍

src/4.3_to_5.0/__test_resources__/legacyUIFolderWithInvalidKpi.ts

src/4.3_to_5.0/__test_resources__/legacyInvalidDashboardBookmark.ts

src/4.3_to_5.0/__test_resources__/legacyInvalidFilterBookmark.ts

src/4.3_to_5.0/__test_resources__/legacyKpiWithInvalidTitle.ts

Nouzbe

The KPI titles change looks good. The test resources refactor looks good too. There's just a couple instances where we've lost some content in our snapshots (I assume because we've lost them in some test resources). See comments below.

Otherwise, good to go!

Nouzbe · 2024-09-25T08:32:16Z

src/4.3_to_5.0/__snapshots__/migrate_43_to_50.test.ts.snap

-            },
-            "settings": {
-              "entry": {
-                "content": "{"theme":"dark-activeviam","search.maxResults":10,"userFilters.areEnabled":true,"drillthrough.defaultSelectedColumns":{"EquityDerivativesCube":["delta","gamma","pnlVega","Desk","Currency","Date","HostName"],"EquityDerivativesCubeDist":["BumpedMtmDown","ProductQtyMultiplier","vega","rho","productId","pnlVega","pnlDelta","pnl","gamma","delta","TradeId","ProductBaseMtm"]}}",


Oops, it looks like we're losing the settings conversion part of this test snapshot. It's probably because some settings that were part of a legacy UI folder test resource were lost in the process. Can you please make sure that we don't lose this?

I've added a new commit to avoid losing this information. However, it means re-adding the settings to the emptyLegacyUIFolder since we are now generating test resources such as smallLegacyUIFolder via the emptyLegacyUIFolder and addLegacyBookmarkToUIFolder.

Nouzbe · 2024-09-25T08:33:16Z

src/4.3_to_5.0/__snapshots__/migrate_43_to_50.test.ts.snap

@@ -2247,12 +2247,8 @@ exports[`migrate_43_to_50 returns a valid ActiveUI5 /ui folder on a small input
                    "content": "{"name":"hidden grand totals"}",
                    "isDirectory": true,
                    "lastEditor": "admin",
-                    "owners": [
-                      "admin",


Here too, it would be nice to not lose this, in order to ensure that the migration script does not erase bookmark permissions.

Nouzbe · 2024-09-25T08:41:59Z

src/4.3_to_5.0/migrateKpi.ts

      });
      if (migratedTitles && Object.keys(migratedTitles).length > 0) {
        migratedWidgetState.titles = migratedTitles;
      }
    }
  } catch (error) {
    // Migrating the KPI titles is a best effort.
+    // If there is an error while migrating one of the titles, the successfully migrated titles are still persisted.
+    if (migratedTitles && Object.keys(migratedTitles).length > 0) {
+      migratedWidgetState.titles = migratedTitles;


Got it now, thank you 👍

NZepeda · 2024-09-25T15:38:31Z

src/4.3_to_5.0/__snapshots__/migrate_43_to_50.test.ts.snap

@@ -2397,6 +2404,86 @@ exports[`migrate_43_to_50 returns a valid ActiveUI5 /ui folder on a small input
      },
    },
    "users": {
+      "children": {


This change results from re-adding the settings to the emptyLegacyUIFolder. With the changes in this commit, this file remains unchanged in this PR.

NZepeda · 2024-09-25T15:47:21Z

@Nouzbe With the latest commit, I've ensured that we are not losing any data in the migrate_43_to_50 test snapshot. However, that meant re-adding the previously removed settings to the emptyLegacyUIFolder. This is required since test resources such as the smallLegacyUIFolder are being generated via the emptyLegacyUIFolder test resource and the addLegacyBookmarkToFolder function.

Nouzbe

A different approach could be to have a addLegacySettings function, which adds given settings to a legacy UI folder. This way, smallLegacyUIFolder could be generated from an actually empty emptyLegacyUIFolder using this function.

If you like this way better, I'll leave it up to you if you prefer to do it in this PR or separately.

This is good to go, thank you for bearing with me 👍

NZepeda · 2024-09-25T17:27:18Z

A different approach could be to have a addLegacySettings function, which adds given settings to a legacy UI folder. This way, smallLegacyUIFolder could be generated from an actually empty emptyLegacyUIFolder using this function.

If you like this way better, I'll leave it up to you if you prefer to do it in this PR or separately.

This is good to go, thank you for bearing with me 👍

I'll address this in a separate PR 👍

UI-9566 - Ensure kpis with titles corresponding to empty measures hav…

dd3f796

…e the titles migrated.

NZepeda requested review from Nouzbe and antoinetissier as code owners July 29, 2024 21:56

NZepeda added 2 commits July 29, 2024 23:18

Minor improvement

620462a

Drop kpi titles that have an empty location.

88bab6b

NZepeda commented Jul 30, 2024

View reviewed changes

src/4.3_to_5.0/getMigratedKpiTitles.ts Outdated Show resolved Hide resolved

Update src/4.3_to_5.0/getMigratedKpiTitles.ts

5e66d80

NZepeda commented Jul 30, 2024

View reviewed changes

NZepeda added 2 commits July 30, 2024 00:00

Update test resource

206fc72

Merge branch 'task/nze/UI-9566-support-migrating-empty-titles' of git…

fd9896d

…hub.com:activeviam/atoti-ui-migration into task/nze/UI-9566-support-migrating-empty-titles

NZepeda assigned Nouzbe and NZepeda and unassigned Nouzbe Jul 30, 2024

NZepeda commented Jul 31, 2024

View reviewed changes

src/migrateContentServer.test.ts Outdated Show resolved Hide resolved

NZepeda commented Jul 31, 2024

View reviewed changes

src/migrateContentServer.test.ts Show resolved Hide resolved

NZepeda added 3 commits July 31, 2024 11:53

Apply suggestions from code review

3876da1

Update tests

85700d8

UI-9566 - Ensure a best effort migration of the kpi titles. If there …

96a710a

…is an error while migrating one of the titles the succesfully migrated titles should still be persisted.

NZepeda commented Jul 31, 2024

View reviewed changes

src/migrateContentServer.test.ts Outdated Show resolved Hide resolved

Update src/migrateContentServer.test.ts

6f50c45

NZepeda changed the title ~~UI-9566 - Ensure kpis with titles corresponding to empty measures have the titles migrated.~~ UI-9566 - Ensure a best effort migration of the KPI titles even when some titles could not be successfully migrated. Jul 31, 2024

NZepeda added 2 commits July 31, 2024 14:30

Fixups

71fbb2d

Merge branch 'task/nze/UI-9566-support-migrating-empty-titles' of git…

1d5c86c

…hub.com:activeviam/atoti-ui-migration into task/nze/UI-9566-support-migrating-empty-titles

NZepeda assigned Nouzbe and unassigned NZepeda Jul 31, 2024

Nouzbe reviewed Aug 1, 2024

View reviewed changes

Nouzbe assigned NZepeda Aug 1, 2024

Nouzbe assigned NZepeda and unassigned Nouzbe Sep 12, 2024

NZepeda added 5 commits September 18, 2024 15:54

Move the location of addLegacyBookmarkToUIFolder to the test resource…

bdcdec4

…s folder

The empty legacy ui folder test resource is truly empty

1eb0b41

Split out the invalid legacy bookmarks and their folder into their ow…

22ecfc2

…n test resource files

Split out the legacy kpi bookmark and folder into their own resource …

1cde91f

…files

Create the smallLegacy ui folder with the new add legacy bookmark to …

2280099

…UI folder function

NZepeda commented Sep 18, 2024

View reviewed changes

src/4.3_to_5.0/__test_resources__/legacyUIFolderWithInvalidKpi.ts Outdated Show resolved Hide resolved

NZepeda commented Sep 18, 2024

View reviewed changes

src/4.3_to_5.0/__test_resources__/legacyInvalidDashboardBookmark.ts Outdated Show resolved Hide resolved

NZepeda commented Sep 18, 2024

View reviewed changes

src/4.3_to_5.0/__test_resources__/legacyInvalidFilterBookmark.ts Outdated Show resolved Hide resolved

NZepeda commented Sep 18, 2024

View reviewed changes

src/4.3_to_5.0/__test_resources__/legacyKpiWithInvalidTitle.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

26adf4b

NZepeda assigned Nouzbe and unassigned NZepeda Sep 18, 2024

Nouzbe reviewed Sep 25, 2024

View reviewed changes

Nouzbe assigned NZepeda and unassigned Nouzbe Sep 25, 2024

Ensure the test snapshots have not lost data

dee3129

NZepeda commented Sep 25, 2024

View reviewed changes

NZepeda assigned Nouzbe and unassigned NZepeda Sep 25, 2024

Nouzbe approved these changes Sep 25, 2024

View reviewed changes

Nouzbe assigned NZepeda and unassigned Nouzbe Sep 25, 2024

NZepeda merged commit 5c4f836 into main Sep 25, 2024
2 checks passed

NZepeda deleted the task/nze/UI-9566-support-migrating-empty-titles branch September 25, 2024 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI-9566 - Ensure a best effort migration of the KPI titles even when some titles could not be successfully migrated. #129

UI-9566 - Ensure a best effort migration of the KPI titles even when some titles could not be successfully migrated. #129

NZepeda commented Jul 29, 2024 •

edited

Loading

NZepeda Jul 30, 2024

NZepeda Jul 31, 2024

NZepeda Jul 31, 2024

NZepeda Jul 31, 2024

Nouzbe left a comment

Nouzbe Aug 1, 2024

NZepeda commented Sep 18, 2024

NZepeda commented Sep 18, 2024

Nouzbe left a comment

Nouzbe Sep 25, 2024

NZepeda Sep 25, 2024

Nouzbe Sep 25, 2024

Nouzbe Sep 25, 2024

NZepeda Sep 25, 2024

NZepeda commented Sep 25, 2024

Nouzbe left a comment

NZepeda commented Sep 25, 2024

UI-9566 - Ensure a best effort migration of the KPI titles even when some titles could not be successfully migrated. #129

UI-9566 - Ensure a best effort migration of the KPI titles even when some titles could not be successfully migrated. #129

Conversation

NZepeda commented Jul 29, 2024 • edited Loading

Summary

Issue

Solution

Reviewing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nouzbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NZepeda commented Sep 18, 2024

NZepeda commented Sep 18, 2024

Nouzbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NZepeda commented Sep 25, 2024

Nouzbe left a comment

Choose a reason for hiding this comment

NZepeda commented Sep 25, 2024

NZepeda commented Jul 29, 2024 •

edited

Loading