UI-9526 - Ensures that the `behaviorOnError` flag is respected #128

NZepeda · 2024-07-29T04:07:45Z

Summary

This PR ensures that when a 4.3 widget is being migrated, if it throws an error, it is kept as is and that the remaining widgets are still migrated as expected.

Testing

All tests should pass.

…r flag

NZepeda · 2024-07-29T04:09:28Z

src/getMigrateSavedWidgets.ts

@@ -68,7 +68,7 @@ export const getMigrateSavedWidgets =

    for (const fileId in content.children) {
      if (errorReport.widgets?.[fileId] && behaviorOnError !== "keep-going") {
-        return;
+        continue;


Previously, returning here would cause the entire script to "short-circuit" causing none of the remaining widgets to be migrated.

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

…idWidget.ts

Nouzbe

I have a few questions:

src/migrateContentServer.test.ts

Nouzbe · 2024-07-31T07:41:49Z

src/4.3_to_5.0/migrate_43_to_50.ts

@@ -434,19 +434,6 @@ export async function migrate_43_to_50(
            });
            counters.widgets.success++;
          } catch (error) {
-            if (error instanceof PartialMigrationError) {


Does it make a difference to move this block after the call to _addErrorToReport? I don't mind, but just trying to understand the change.

Nouzbe · 2024-07-31T07:46:45Z

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

@@ -1336,7 +1336,7 @@ exports[`migrate_43_to_50 returns a valid ActiveUI5 /ui folder on a real life in
              "entry": {
                "canRead": true,
                "canWrite": true,
-                "content": "{}",
+                "content": "{"name":"Page filters","type":"container","value":{"style":{},"showTitleBar":true,"body":{},"containerKey":"filters"}}",


Do you understand this change?

Nouzbe · 2024-07-31T07:46:52Z

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

@@ -1608,7 +1608,7 @@ exports[`migrate_43_to_50 returns a valid ActiveUI5 /ui folder on a real life in
                  "entry": {
                    "canRead": true,
                    "canWrite": true,
-                    "content": "{"name":"Page filters","widgetKey":"filters"}",
+                    "content": "{"name":"Page filters"}",


Same question here.

Nouzbe · 2024-07-31T07:50:49Z

src/4.3_to_5.0/migrate_43_to_50.ts

+              // The migration failed.
+              // The widget is kept as is.
+              migratedWidget = bookmark;
+              widgets[fileId] = migratedWidget;


Why do we need to do this here? Isn't it already taken care of in

atoti-ui-migration/src/4.3_to_5.0/migrate_43_to_50.ts

Line 464 in c883371

widgets[fileId] = migratedWidget;

?

@Nouzbe I undid my changes within migrate_4.3_to_5.0 as indeed, preserving the bookmarks content was handled by this. There was a slight modification to one of the tests but all tests should still pass. This makes this PR more simple 👍

Co-authored-by: Benjamin Amelot <[email protected]>

…the bookmark was already handled

src/migrateContentServer.test.ts

NZepeda · 2024-07-31T14:31:25Z

@Nouzbe I'm passing this back to you. In my latest commit, I undid the changes to migrate_43_to_50 because as you pointed out, the preservation of the original bookmark content was already being handled. The difference with my previous changes, was that I was keeping the entire 4.3 bookmarks value which looks like the following:

{
  "value": {
    "style": {},
    "showTitleBar": false,
    "containerKey": "invalid-container-key",
    "body": {
      "serverUrl": "",
      "mdx": "SELECT NON EMPTY [Measures].[contributors.COUNT] ON COLUMNS FROM [EquityDerivativesCube] WHERE [Geography].[City].[ALL].[AllMember].[New York] CELL PROPERTIES VALUE, FORMATTED_VALUE, BACK_COLOR, FORE_COLOR, FONT_FLAGS",
      "contextValues": {},
      "updateMode": "once",
      "ranges": {}
    }
  }
}

The existing changes were already valid though, just keeping the value.body portion of the bookmark. So I reverted my changes which allow the migrate_43_to_50.ts file to remain untouched and thus simplify this PR a bit more. Now the changes mostly revolve around the tests ensuring that the behaviorOnError flag is respected.

Please let me know if you have any other questions or concerns, I'm happy to address them.

Nouzbe

Looks good 👍

NZepeda added 3 commits July 26, 2024 18:16

Errored widgets are left untouched

4b9f062

Add test for usage of the keep-last-successful-version behaviorOnErro…

882272c

…r flag

Small edits

0acfad1

NZepeda requested review from Nouzbe and antoinetissier as code owners July 29, 2024 04:07

NZepeda commented Jul 29, 2024

View reviewed changes

NZepeda assigned Nouzbe Jul 29, 2024

NZepeda commented Jul 30, 2024

View reviewed changes

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

Update src/4.3_to_5.0/__test_resources__/smallLegacyUIFolderWithInval…

fb12b3e

…idWidget.ts

Nouzbe reviewed Jul 31, 2024

View reviewed changes

NZepeda and others added 4 commits July 31, 2024 08:46

Update src/migrateContentServer.test.ts

cd32f76

Co-authored-by: Benjamin Amelot <[email protected]>

Update src/migrateContentServer.test.ts

39ac257

Co-authored-by: Benjamin Amelot <[email protected]>

Update src/migrateContentServer.test.ts

6ce2b64

Co-authored-by: Benjamin Amelot <[email protected]>

UI-9526 - Undo changes to the migrate_43_to_50 file since conserving …

0510f40

…the bookmark was already handled

NZepeda commented Jul 31, 2024

View reviewed changes

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

Update src/migrateContentServer.test.ts

8ffb171

NZepeda assigned NZepeda and unassigned Nouzbe Jul 31, 2024

NZepeda assigned Nouzbe and unassigned NZepeda Jul 31, 2024

Nouzbe approved these changes Aug 1, 2024

View reviewed changes

Nouzbe assigned NZepeda and unassigned Nouzbe Aug 1, 2024

Nouzbe mentioned this pull request Aug 1, 2024

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

Merged

NZepeda merged commit 5e3020e into main Aug 1, 2024
2 checks passed

NZepeda deleted the bugfix/nze/UI-9526 branch August 1, 2024 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI-9526 - Ensures that the `behaviorOnError` flag is respected #128

UI-9526 - Ensures that the `behaviorOnError` flag is respected #128

NZepeda commented Jul 29, 2024 •

edited

Loading

NZepeda Jul 29, 2024

Nouzbe left a comment

Nouzbe Jul 31, 2024

Nouzbe Jul 31, 2024

Nouzbe Jul 31, 2024

Nouzbe Jul 31, 2024

NZepeda Jul 31, 2024 •

edited

Loading

NZepeda commented Jul 31, 2024

Nouzbe left a comment

UI-9526 - Ensures that the behaviorOnError flag is respected #128

UI-9526 - Ensures that the behaviorOnError flag is respected #128

Conversation

NZepeda commented Jul 29, 2024 • edited Loading

Summary

Testing

NZepeda Jul 29, 2024

Choose a reason for hiding this comment

Nouzbe left a comment

Choose a reason for hiding this comment

Nouzbe Jul 31, 2024

Choose a reason for hiding this comment

Nouzbe Jul 31, 2024

Choose a reason for hiding this comment

Nouzbe Jul 31, 2024

Choose a reason for hiding this comment

Nouzbe Jul 31, 2024

Choose a reason for hiding this comment

NZepeda Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

NZepeda commented Jul 31, 2024

Nouzbe left a comment

Choose a reason for hiding this comment

UI-9526 - Ensures that the `behaviorOnError` flag is respected #128

UI-9526 - Ensures that the `behaviorOnError` flag is respected #128

NZepeda commented Jul 29, 2024 •

edited

Loading

NZepeda Jul 31, 2024 •

edited

Loading