feat: Duplicate dashboard visualize #148

yuye-aws · 2023-09-12T10:15:31Z

Description

Implement duplicate on dashboard
Implement duplicate on visualization
Bug fix: re-duplicate when failed to duplicate some of the saved objects

Issues Resolved

Screenshot

Testing the changes

Check List

codecov-commenter · 2023-09-13T03:31:15Z

Codecov Report

Merging #148 (3c76545) into workspace (3ecaf7d) will increase coverage by 25.76%.
Report is 3 commits behind head on workspace.
The diff coverage is 2.94%.

@@              Coverage Diff               @@
##           workspace     #148       +/-   ##
==============================================
+ Coverage      34.82%   60.59%   +25.76%     
==============================================
  Files           1942     2992     +1050     
  Lines          38428    58595    +20167     
  Branches        5499     9527     +4028     
==============================================
+ Hits           13384    35506    +22122     
+ Misses         24438    21002     -3436     
- Partials         606     2087     +1481

Flag	Coverage Δ
Linux_1	`34.17% <1.47%> (-0.66%)`	⬇️
Linux_2	`55.05% <ø> (?)`
Linux_3	`42.76% <2.94%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/core/public/workspace/workspaces_service.ts	`37.03% <ø> (+37.03%)`	⬆️
...components/dashboard_top_nav/dashboard_top_nav.tsx	`87.50% <ø> (+82.50%)`	⬆️
...ts/dashboard_top_nav/top_nav/get_top_nav_config.ts	`46.66% <0.00%> (+46.66%)`	⬆️
...omponents/dashboard_top_nav/top_nav/top_nav_ids.ts	`100.00% <ø> (ø)`
...ns/navigation/public/top_nav_menu/top_nav_menu.tsx	`16.66% <ø> (ø)`
...c/plugins/saved_objects_management/public/index.ts	`0.00% <ø> (ø)`
...s_management/public/lib/duplicate_saved_objects.ts	`0.00% <ø> (ø)`
...ction/objects_table/components/duplicate_modal.tsx	`0.00% <0.00%> (ø)`
...gement_section/objects_table/components/header.tsx	`16.66% <ø> (ø)`
.../objects_table/components/show_duplicate_modal.tsx	`0.00% <0.00%> (ø)`
... and 4 more

... and 1640 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

wanglam · 2023-09-13T06:43:50Z

src/plugins/dashboard/public/application/utils/get_nav_actions.tsx

+    const getDuplicateWorkspaces = async (): Promise<WorkspaceAttribute[]> => {
+      let result;
+      try {
+        result = await getWorkspacesWithWritePermission(http);


@ruanyl @SuZhou-Joe How do you think about add a permitted workspace list in the core public workspace service? Not sure if suitable to call workspace plugin API via this way. Another option will be expose this method in workspace start service result.

src/plugins/dashboard/public/application/utils/get_nav_actions.tsx

wanglam · 2023-09-13T08:06:11Z

src/plugins/dashboard/public/application/utils/get_nav_actions.tsx

+    dashboardSavedObject.meta = { title: savedDashboard.title };
+
+    const duplicateModal = (
+      <SavedObjectsDuplicateModal


Can we move these duplicateModal create logic to show_duplicate_modal.tsx file?

wanglam · 2023-09-13T08:26:04Z

src/plugins/visualize/public/application/utils/get_top_nav_config.tsx

+              ) {
+                createVisReference();
+              } else if (savedVis) {
+                showDuplicateModal(duplicateModal, I18nContext);


It seems we are using the same SavedObjectsDuplicateModal with src/plugins/dashboard/public/application/utils/get_nav_actions.tsx, why we need to pass I18nContext here?

It seems the differences between two showDuplicateModal are error messages. Could we merge these two showDuplicateModal into one with SavedObjectsDuplicateModal component? We can pass different messages from outside.

ruanyl · 2023-09-13T02:35:51Z

...gins/dashboard/public/application/components/dashboard_top_nav/top_nav/get_top_nav_config.ts

@@ -55,6 +55,7 @@ export function getTopNavConfig(
            getFullScreenConfig(actions[TopNavIds.FULL_SCREEN]),
            getShareConfig(actions[TopNavIds.SHARE]),
            getCloneConfig(actions[TopNavIds.CLONE]),
+            getDuplicateConfig(actions[TopNavIds.DUPLICATE]),


Is duplicate supposed to replace clone? If so, can we remove clone?

Yes, clone should be replaced

ruanyl · 2023-09-13T04:08:29Z

...s/dashboard/public/application/components/dashboard_top_nav/top_nav/show_duplicate_modal.tsx

+  onClose: () => void;
+}
+
+export function showDuplicateModal(duplicateModal: React.ReactElement<MinimalDuplicateModalProps>) {


I see two showDuplicateModal function and they are almost the same except one accepts i18n, I think we can reuse the showDuplicateModal in src/plugins/saved_objects/public/duplicate_modal/show_saved_object_duplicate_modal.tsx.
And i18n should also be passed to duplicate dashboard modal, am I right?

ruanyl · 2023-09-13T04:17:01Z

...s/dashboard/public/application/components/dashboard_top_nav/top_nav/show_duplicate_modal.tsx

+import { WorkspaceAttribute, WorkspaceStart } from 'opensearch-dashboards/public';
+import { SavedObjectWithMetadata } from '../../../../../../saved_objects_management/public';
+
+interface MinimalDuplicateModalProps {


What does Minimal mean in this context?

It means minimal required props for DuplicateModal. I think this file can be deleted and we use src/plugins/saved_objects/public/duplicate_modal/show_saved_object_duplicate_modal.tsx instead.

MinimalDuplicateModalProps has been replaced by ShowDuplicateModalProps

ruanyl · 2023-09-13T04:24:26Z

...s/dashboard/public/application/components/dashboard_top_nav/top_nav/show_duplicate_modal.tsx

+interface MinimalDuplicateModalProps {
+  selectedSavedObjects: SavedObjectWithMetadata[];
+  workspaces: WorkspaceStart;
+  getDuplicateWorkspaces: (...args: any[]) => Promise<WorkspaceAttribute[]>;


We should avoid any[] and it seems getDuplicateWorkspaces doesn't necessarily to have arguments.
Also the function name sounds a bit misleading, what about getTargetWorkspaces?

ruanyl · 2023-09-13T04:25:53Z

...s/dashboard/public/application/components/dashboard_top_nav/top_nav/show_duplicate_modal.tsx

+  selectedSavedObjects: SavedObjectWithMetadata[];
+  workspaces: WorkspaceStart;
+  getDuplicateWorkspaces: (...args: any[]) => Promise<WorkspaceAttribute[]>;
+  onDuplicate: (...args: any[]) => Promise<void>;


No any, let's get it properly typed

ruanyl · 2023-09-13T10:10:06Z

src/plugins/visualize/public/application/utils/get_top_nav_config.tsx

+              const getDuplicateWorkspaces = async (): Promise<WorkspaceAttribute[]> => {
+                let result;
+                try {
+                  result = await getWorkspacesWithWritePermission(http);
+                } catch (error) {
+                  notifications?.toasts.addDanger({
+                    title: i18n.translate(
+                      'savedObjectsManagement.objectsTable.duplicateWorkspaces.dangerNotification',
+                      {
+                        defaultMessage: 'Unable to get workspaces with write permission',
+                      }
+                    ),
+                    text: error instanceof Error ? error.message : JSON.stringify(error),
+                  });
+                }
+                if (result?.success) {
+                  return result.result?.workspaces ?? [];
+                } else {
+                  return [];
+                }
+              };


Suggested change

const getDuplicateWorkspaces = async (): Promise<WorkspaceAttribute[]> => {

let result;

try {

result = await getWorkspacesWithWritePermission(http);

} catch (error) {

notifications?.toasts.addDanger({

title: i18n.translate(

'savedObjectsManagement.objectsTable.duplicateWorkspaces.dangerNotification',

{

defaultMessage: 'Unable to get workspaces with write permission',

}

),

text: error instanceof Error ? error.message : JSON.stringify(error),

});

}

if (result?.success) {

return result.result?.workspaces ?? [];

} else {

return [];

}

};

const getDuplicateWorkspaces = async (): Promise<WorkspaceAttribute[]> => {

try {

const result = await getWorkspacesWithWritePermission(http);

if (result?.success) {

return result.result?.workspaces ?? [];

}

} catch (error) {

notifications?.toasts.addDanger({

title: i18n.translate(

'savedObjectsManagement.objectsTable.duplicateWorkspaces.dangerNotification',

{

defaultMessage: 'Unable to get workspaces with write permission',

}

),

text: error instanceof Error ? error.message : JSON.stringify(error),

});

}

return [];

};

ruanyl · 2023-09-13T10:11:06Z

src/plugins/visualize/public/application/utils/get_top_nav_config.tsx

+                const objectsToDuplicate = visualizationSavedObjects.map((obj) => ({
+                  id: obj.id,
+                  type: 'visualization',
+                }));


Suggested change

const objectsToDuplicate = visualizationSavedObjects.map((obj) => ({

id: obj.id,

type: 'visualization',

}));

// object must be visualization type

const objectsToDuplicate = visualizationSavedObjects.filter(obj => obj.type === 'visualization').map((obj) => ({

id: obj.id,

type: obj.type,

}));

src/plugins/visualize/public/application/utils/get_top_nav_config.tsx

ruanyl · 2023-09-13T10:15:59Z

src/plugins/visualize/public/application/utils/get_top_nav_config.tsx

+                });
+              };
+
+              const visualizationSavedObject = (savedVis || {}) as SavedObjectWithMetadata;


If savedVis is undefined, it should display error toast, now it creates an empty object which I think is not right.

Duplicate behavior for visualization will be disabled if savedVis is undefined.

ruanyl · 2023-09-13T10:20:26Z

src/plugins/visualize/public/application/utils/get_top_nav_config.tsx

+                dashboard.dashboardFeatureFlagConfig.allowByValueEmbeddables &&
+                !isSaveAsButton
+              ) {
+                createVisReference();


Q: Could you leave a comment on when and why it should call createVisReference?