UI-7229 - Option to remove widgets of a given type in the CLI #21

antoinetissier · 2022-04-19T16:54:36Z

The rationale is explained in UI-7229, which comes from a client request at CIBC: NAS-3463

You can review commit by commit.

src/__test_resources__/legacyUIFolder.ts

antoinetissier · 2022-04-19T17:05:25Z

src/__snapshots__/migrateUIFolder.test.ts.snap

@@ -2128,6 +2249,18 @@ Object {
        ],
      },
    },
+    "organization_permissions": Object {


It seems that I forgot to update the tests last PR.
This has happened for a lot of devs already, and the only way to make it poka-yoke would be to add a CI.
We could even test the CLI in the CI once #18 and #4 are merged.

Nouzbe

A few suggestions. Also, have you been able to try it for real and make sure that the migrated dashboards work as expected? I am thinking in particular about edge cases where a page contains a single widget, which is removed. Or even worse: a dashboard contains a single page, which contains a single widget, which is removed.

Asking to know whether I should invest the time to do it on my side.

Nouzbe · 2022-04-20T12:42:06Z

src/migrateDashboard.test.ts

+
+    const { content, layout } = dashboard.pages["p-0"];
+    const widgetPluginKeys = _map(content, ({ widgetKey }) => widgetKey);
+    expect(


It would be nice to add another assertion here with the legacy dashboard state:

expect( _intersection(widgetKeysInLegacyDashboard, keysOfWidgetPluginsToRemove) ).toHaveLength(1);

This way, we are guaranteed that this test will remain useful, even if future devs change the content of mocks.

Nouzbe · 2022-04-20T12:56:23Z

src/migrateDashboard.ts

+
+  const deserializedDashboard = deserializeDashboardState(dashboard);
+
+  const dashboardWithWidgetsRemoved = _reduce(


I try to avoid reduce because it tends to be less legible than other methods to people who didn't write them (and the future-self of people who wrote them). I am not the only one feeling this way. See eslint-plugin-unicorn/no-array-reduce.md.

In this case, I believe we could do:

const dashboardWithWidgetsRemoved = produce(deserializedDashboard, draft => { for (let i = 0; i < draft.pages.length; i++) { keysOfLeavesToRemove.forEach(leafKey => { // the call to getLayoutPath could even be internalized within removeWidget draft.pages[i] = removeWidget(draft.pages[i], layoutPath); }); ) })

That would be ideal, but this is not the signature of removeWidget:

removeWidget({ dashboardState, pageKey, leafKey, layoutPath, });

And since removeWidget can modify the layout of a given page, then when calling several removeWidget in a row, one has to get the latest dashboardState at each iteration (rather than the initial dashboardState before any removal).
So typically I feel this is a good use case for reduce: you want to feed the intermediate result to the next iteration step.

An alternative would be to refactor things a bit in the SDK and export a removeWidgetFromPage function that mutates its input page, but that would mean:

making the API surface bigger

requiring to bump the SDK dependency to 5.0.11 in activeui-migration before proceeding with this dev
This does not seem very appealing to me.

Or am I missing another way to write it ?

I am pushing an attempt. Let me know what you think.

src/migrateUIFolder.test.ts

Nouzbe · 2022-04-20T12:59:08Z

src/migrateUIFolder.test.ts

+    const widgetPluginKeys = _map(content, ({ widgetKey }) => widgetKey);
+    expect(
+      _intersection(widgetPluginKeys, keysOfWidgetPluginsToRemove)
+    ).toHaveLength(0);


Same remark here.

Don't forget this one either 🙏

antoinetissier · 2022-04-20T14:48:00Z

Also, have you been able to try it for real and make sure that the migrated dashboards work as expected?

I have tested that

npx migrate -i "aui4_ui_folder_with_page_filters_widget.json" -o migrated-content.json -s servers.json --remove-widgets "filters"

works well with the attached input aui4 folder (GitHub does not support attaching JSONs, so you'll have to change the extension if you want to open it in an IDE and have it formatted properly)
aui4_ui_folder_with_page_filters_widget.txt and the servers.json from #4
servers.txt, and that indeed in the output JSON:

the saved widgets (=0xb) matching the given keys have been removed
widgets matching the given keys within saved dashboards (= "Page filters" widget in dashboard eef) have been removed

I am thinking in particular about edge cases where a page contains a single widget, which is removed. Or even worse: a dashboard contains a single page, which contains a single widget, which is removed.

While I agree that it would be nice to do end-to-end tests for edge cases, here I feel this is almost out of scope: the removal job is the responsability of removeWidget from activeui-sdk.
If there is a problem with it for some edge cases, then it should be fixed in the SDK, and there should be nothing more to do here than to bump the dependency version.

Nouzbe

I cannot build the repo. I get 40 instances of the following error:

ERROR in ./node_modules/katex/dist/fonts/KaTeX_Typewriter-Regular.woff 1:4
Module parse failed: Unexpected character '' (1:4)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
(Source code omitted for this binary file)
@ ./node_modules/css-loader/dist/cjs.js!./node_modules/katex/dist/katex.css 62:0-83 123:74-104
@ ./node_modules/katex/dist/katex.css 2:26-79
@ ./node_modules/@activeviam/plugin-widget-text-editor/dist/esm/DangerousMarkdownPreview-0f2462ba.js 5:0-30
@ ./node_modules/@activeviam/plugin-widget-text-editor/dist/esm/index.js 19:45-93
@ ./node_modules/@activeviam/activeui-sdk/dist/esm/index.js 81:0-79 81:0-79
@ ./src/migrateDashboard.ts 8:23-58
@ ./src/index.ts 5:25-54

Can you build the repo?

Nouzbe · 2022-05-05T12:48:24Z

src/migrateDashboard.ts

+/**
+ * Returns the layout path to the leaf uniquely identified by `leafKey`, or `undefined` if no leaf has this key.
+ */
+function findPathToLeaf(layout: Layout, leafKey: string): number[] | undefined {


Can this be removed in favor of getLayoutPath from @activeviam/activeui-sdk?

src/migrateUIFolder.test.ts

Nouzbe · 2022-05-05T12:54:17Z

src/migrateUIFolder.test.ts

+    );
+    const { content, layout } = migratedDashboard.pages["p-0"];
+    const widgetPluginKeys = _map(content, ({ widgetKey }) => widgetKey);
+    expect(


This seems like a simpler way of testing the same thing:

Suggested change

expect(

expect(widgetPluginKeysInLegacyDashboard).toContain("filters");

If you agree, same suggestion below.

Don't forget this comment 🙏

Nouzbe · 2022-05-05T13:05:29Z

src/migrateDashboard.ts

+
+  const deserializedDashboard = deserializeDashboardState(dashboard);
+
+  const dashboardWithWidgetsRemoved = _reduce(


I am pushing an attempt. Let me know what you think.

antoinetissier · 2022-05-05T13:44:26Z

Can you build the repo?

I get the same errors but it's only for fonts.
They have been here for a while and do not seem to block the build.
I do agree that we should keep track of these errors and fix them though.

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

antoinetissier · 2022-05-09T14:24:55Z

Thanks for the commit, it works 👍

src/migrateUIFolder.test.ts

Nouzbe · 2022-05-24T14:36:09Z

src/migrateUIFolder.test.ts

+    );
+    const { content, layout } = migratedDashboard.pages["p-0"];
+    const widgetPluginKeys = _map(content, ({ widgetKey }) => widgetKey);
+    expect(


Don't forget this comment 🙏

Nouzbe · 2022-05-24T14:36:17Z

src/migrateUIFolder.test.ts

+    const widgetPluginKeys = _map(content, ({ widgetKey }) => widgetKey);
+    expect(
+      _intersection(widgetPluginKeys, keysOfWidgetPluginsToRemove)
+    ).toHaveLength(0);


Don't forget this one either 🙏

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

antoinetissier · 2022-05-24T14:44:32Z

I would like to keep the abstraction of "output plugin keys do (not) contain the plugins keys to remove", rather than projecting it on "filters".
I'll propose another way of writing with jest assertions, and if you still don't like it I'll go for your suggestion.

Nouzbe

Looks good 👍

antoinetissier added 3 commits April 19, 2022 18:30

UI-7229 - Allow to optionally remove some widgets

85cc182

CLI option

3fd30f1

Add README

84313b4

antoinetissier requested a review from Nouzbe as a code owner April 19, 2022 16:54

antoinetissier self-assigned this Apr 19, 2022

Enrich warning message

36f8b1a

antoinetissier assigned Nouzbe and unassigned antoinetissier Apr 19, 2022

antoinetissier commented Apr 19, 2022

View reviewed changes

Nouzbe reviewed Apr 20, 2022

View reviewed changes

Nouzbe assigned antoinetissier and unassigned Nouzbe Apr 20, 2022

PR feedback

84560e3

antoinetissier assigned Nouzbe and unassigned antoinetissier Apr 20, 2022

Nouzbe reviewed May 5, 2022

View reviewed changes

Simplification attempt

a5aa13f

Nouzbe assigned antoinetissier and unassigned Nouzbe May 5, 2022

Update src/migrateUIFolder.test.ts

9199c5b

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

PR feedback

a583efd

antoinetissier assigned Nouzbe and unassigned antoinetissier May 9, 2022

feedback in other place

dae69c4

Nouzbe reviewed May 24, 2022

View reviewed changes

Nouzbe assigned antoinetissier and unassigned Nouzbe May 24, 2022

Update src/migrateUIFolder.test.ts

8fc1a30

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

antoinetissier added 2 commits May 24, 2022 16:53

other way of writing the assertion

2e67750

Fix

dbe80e1

antoinetissier assigned Nouzbe and unassigned antoinetissier May 24, 2022

Nouzbe approved these changes May 24, 2022

View reviewed changes

Nouzbe assigned antoinetissier and unassigned Nouzbe May 24, 2022

antoinetissier merged commit d8d51c4 into main May 24, 2022

antoinetissier deleted the personal/a.tissier/UI-7229-optionnally-remove-widgets branch May 24, 2022 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI-7229 - Option to remove widgets of a given type in the CLI #21

UI-7229 - Option to remove widgets of a given type in the CLI #21

antoinetissier commented Apr 19, 2022 •

edited

Loading

antoinetissier Apr 19, 2022

Nouzbe left a comment

Nouzbe Apr 20, 2022

Nouzbe Apr 20, 2022

antoinetissier Apr 20, 2022 •

edited

Loading

Nouzbe May 5, 2022

Nouzbe Apr 20, 2022

Nouzbe May 24, 2022

antoinetissier commented Apr 20, 2022 •

edited

Loading

Nouzbe left a comment

Nouzbe May 5, 2022

Nouzbe May 5, 2022

Nouzbe May 24, 2022

Nouzbe May 5, 2022

antoinetissier commented May 5, 2022 •

edited

Loading

antoinetissier commented May 9, 2022

Nouzbe May 24, 2022

Nouzbe May 24, 2022

antoinetissier commented May 24, 2022

Nouzbe left a comment


		const deserializedDashboard = deserializeDashboardState(dashboard);

		const dashboardWithWidgetsRemoved = _reduce(

	expect(
	expect(widgetPluginKeysInLegacyDashboard).toContain("filters");

UI-7229 - Option to remove widgets of a given type in the CLI #21

UI-7229 - Option to remove widgets of a given type in the CLI #21

Conversation

antoinetissier commented Apr 19, 2022 • edited Loading

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

Choose a reason for hiding this comment

antoinetissier Apr 20, 2022 • edited Loading

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

antoinetissier commented Apr 20, 2022 • edited Loading

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

antoinetissier commented May 5, 2022 • edited Loading

antoinetissier commented May 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoinetissier commented May 24, 2022

Nouzbe left a comment

Choose a reason for hiding this comment

antoinetissier commented Apr 19, 2022 •

edited

Loading

antoinetissier Apr 20, 2022 •

edited

Loading

antoinetissier commented Apr 20, 2022 •

edited

Loading

antoinetissier commented May 5, 2022 •

edited

Loading