bulk update separation #356

derek-ho · 2023-04-14T20:09:14Z

Description

Delete fixed (minus toast for delete and rename), separate out bulk update for event analytics add viz to operational panel flow. Still need to do bulk update for SO to add new and old viz

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Derek Ho <[email protected]>

joshuali925 · 2023-04-14T21:23:49Z

public/services/saved_objects/saved_object_savers/ppl/save_as_current_vis.ts

@@ -46,9 +46,12 @@ export class SaveAsCurrentVisualization extends SavedQuerySaver {
  }

  addToPanel({ selectedPanels, saveTitle, notifications, visId }) {
+    const uuidRx = /^[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12}$/;
+    const soPanels = selectedPanels.filter((id) => id.panel.id.match(uuidRx));


duplicate and unused variable?

joshuali925 · 2023-04-14T21:29:23Z

public/services/saved_objects/saved_object_savers/ppl/save_as_current_vis.ts

@@ -46,9 +46,12 @@ export class SaveAsCurrentVisualization extends SavedQuerySaver {
  }

  addToPanel({ selectedPanels, saveTitle, notifications, visId }) {
+    const uuidRx = /^[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12}$/;
+    const soPanels = selectedPanels.filter((id) => id.panel.id.match(uuidRx));
+    const opsPanels = selectedPanels.filter((id) => !id.panel.id.match(uuidRx));


use uuidRx.test(id.panel.id)

id variable name is not accurate

i'm not sure if the logic should be here, e.g. what happens to panels in .observability index? save_as_current_vis don't support them anymore?

not sure if it helps but you can check .observability and .kibana visualizations handling here as reference, for example bulk delete

dashboards-observability/public/services/saved_objects/saved_object_client/saved_objects_actions.ts

Lines 74 to 88 in 775a77e

/**

* Delete a list of objects. Assumes object is osd visualization if id is a

* UUID. Rest and failed ids will then be deleted by PPL client.

*

* @param params - SavedObjectsDeleteBulkParams

* @returns SavedObjectsDeleteResponse

*/

static async deleteBulk(

params: SavedObjectsDeleteBulkParams

): Promise<SavedObjectsDeleteResponse> {

const idMap = params.objectIdList.reduce((prev, id) => {

const type = OSDSavedObjectClient.extractType(id);

const key = type === '' ? 'non_osd' : type;

return { ...prev, [key]: [...(prev[key] || []), id] };

}, {} as { [k in 'non_osd' | ObservabilitySavedObjectsType]: string[] });

joshuali925 · 2023-04-14T21:30:52Z

public/services/saved_objects/saved_object_savers/ppl/save_as_new_vis.ts

@@ -76,9 +76,12 @@ export class SaveAsNewVisualization extends SavedQuerySaver {
  }

  addToPanel({ selectedPanels, saveTitle, notifications, visId }) {
+    const uuidRx = /^[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12}$/;


similar comments and probably define regex globally

…vability into bulk-update Signed-off-by: Derek Ho <[email protected]>

Signed-off-by: Derek Ho <[email protected]>

public/components/custom_panels/redux/panel_slice.ts

public/services/saved_objects/saved_object_savers/ppl/save_as_current_vis.ts

Signed-off-by: Derek Ho <[email protected]>

derek-ho · 2023-04-17T18:17:00Z

public/components/custom_panels/redux/panel_slice.ts

+
+    const allVisualizations = panel!.visualizations;
+
+    const visualizationsWithNewPanel = addVisualizationPanel(vizId, undefined, allVisualizations);


@ps48 @joshuali925 can you verify this line makes sense? Since we want to add a viz to panel as a new one, pass in undefined into this function:

dashboards-observability/public/components/custom_panels/helpers/add_visualization_helper.ts

Line 57 in 5f2b777

export const addVisualizationPanel = (

?

Yes, this is correct for adding panels to a visualization. For replace workflow, you can use the same function and pass OldVisualizationId.

pjfitzgibbons · 2023-04-17T18:49:06Z

public/components/custom_panels/helpers/add_visualization_helper.ts

@@ -58,7 +58,7 @@ export const addVisualizationPanel = (
  // client: ILegacyScopedClusterClient,
  // panelId: string,
  savedVisualizationId: string,
-  oldVisualizationId?: string,
+  oldVisualizationId: string | undefined,


I thought var?: string meant string | undefined exactly?
I've even "fixed" my own bugs many times with the ? syntax.

Thuoghts?

Maybe... I think what it is doing is makes it optional, which it cannot be since a required parameter comes after. This gets rid of it but maybe the more right thing is to put it at the end...

pjfitzgibbons · 2023-04-17T18:52:16Z

public/components/custom_panels/home.tsx

@@ -358,7 +356,7 @@ export const Home = ({
                chrome={chrome}
                parentBreadcrumbs={customPanelBreadCrumbs}
                cloneCustomPanel={cloneCustomPanel}
-                deleteCustomPanel={deleteCustomPanel}
+                deleteCustomPanel={deleteCustomPanelSO}


delete savedobject should be in panel_slice. Therefore, we should collapse a reference and just call dispatch(deletePanel) (or whatever) directly from <CustomPanelViewSO>. Dont' pass it in.

We're trying to eliminate all prop-drilling, especially functions.

I can modify this in a follow up PR

pjfitzgibbons · 2023-04-17T19:01:43Z

public/services/saved_objects/saved_object_savers/ppl/save_as_new_vis.ts

@@ -76,9 +83,14 @@ export class SaveAsNewVisualization extends SavedQuerySaver {
  }

  addToPanel({ selectedPanels, saveTitle, notifications, visId }) {
+    const { dispatch } = this.dispatchers;


Please consider context here, as well as SRP-SingleResponsibilityPrinciple.

Wer are inside "saved_objects/saved_object_savers".... This, despite it's name, is the Observability index re-invention of saved objects.
We don't want to add any code here. This whole module will (eventually) be deprecated/removed.

Also, wrt. SRP, although "saving things" sounds like a single responsibliity, we can see from the code that saving to SavedObject-Core (via Redux/Dispatch) is very different from using SavedObjectSavers.
These are different responsibilities.

Recommendation :
Lift the filter/split of saved-object (UUID) and Observability (non-UUID) out of this method. Pass the non-UUID into this method, and specifically call dispatch from the caller (which is a component, which is insdie React context, which has correct context for useDispatch() ). This will also remove the prop-drilling of "dispatchers".

we can see from the code that saving to SavedObject-Core (via Redux/Dispatch) is very different from using SavedObjectSavers

Why does this happen? I didn't look into impl details but essentially the difference between saving SO vs. observability object is the client. If the client can be abstracted, then the same operation should work for both cases

@joshuali925 @pjfitzgibbons can we merge this in as is? I understand it may not be right but theres a few other p0 items today, once we make the final go-no go call I can refactor to be correct

* bulk update separation Signed-off-by: Derek Ho <[email protected]> * fix up pr Signed-off-by: Derek Ho <[email protected]> * individual panel delete SO Signed-off-by: Derek Ho <[email protected]> * also separate out on newly created ones Signed-off-by: Derek Ho <[email protected]> * resolve pr comments Signed-off-by: Derek Ho <[email protected]> * bulk update new so panels Signed-off-by: Derek Ho <[email protected]> * fix PR Signed-off-by: Derek Ho <[email protected]> --------- Signed-off-by: Derek Ho <[email protected]> (cherry picked from commit b0a87fc) Signed-off-by: Peter Fitzgibbons <[email protected]>

* bulk update separation Signed-off-by: Derek Ho <[email protected]> * fix up pr Signed-off-by: Derek Ho <[email protected]> * individual panel delete SO Signed-off-by: Derek Ho <[email protected]> * also separate out on newly created ones Signed-off-by: Derek Ho <[email protected]> * resolve pr comments Signed-off-by: Derek Ho <[email protected]> * bulk update new so panels Signed-off-by: Derek Ho <[email protected]> * fix PR Signed-off-by: Derek Ho <[email protected]> --------- Signed-off-by: Derek Ho <[email protected]>

derek-ho added 4 commits April 14, 2023 16:08

bulk update separation

8bf80a8

Signed-off-by: Derek Ho <[email protected]>

fix up pr

35f260d

Signed-off-by: Derek Ho <[email protected]>

individual panel delete SO

ea5baf7

Signed-off-by: Derek Ho <[email protected]>

also separate out on newly created ones

2b1837e

Signed-off-by: Derek Ho <[email protected]>

derek-ho marked this pull request as ready for review April 14, 2023 21:18

derek-ho requested review from pjfitzgibbons, anirudha, ps48, kavithacm, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vmmusings and Swiddis as code owners April 14, 2023 21:18

joshuali925 reviewed Apr 14, 2023

View reviewed changes

derek-ho force-pushed the bulk-update branch from 3e280e7 to 2b1837e Compare April 17, 2023 17:08

derek-ho added 3 commits April 17, 2023 13:10

Merge branch 'main' of github.com:opensearch-project/dashboards-obser…

f3ba48b

…vability into bulk-update Signed-off-by: Derek Ho <[email protected]>

resolve pr comments

d5bc709

Signed-off-by: Derek Ho <[email protected]>

bulk update new so panels

9a0d150

Signed-off-by: Derek Ho <[email protected]>

joshuali925 reviewed Apr 17, 2023

View reviewed changes

public/components/custom_panels/redux/panel_slice.ts Outdated Show resolved Hide resolved

public/services/saved_objects/saved_object_savers/ppl/save_as_current_vis.ts Outdated Show resolved Hide resolved

fix PR

19b9c9f

Signed-off-by: Derek Ho <[email protected]>

joshuali925 approved these changes Apr 17, 2023

View reviewed changes

derek-ho commented Apr 17, 2023

View reviewed changes

pjfitzgibbons suggested changes Apr 17, 2023

View reviewed changes

Merge branch 'main' into bulk-update

cedf122

pjfitzgibbons approved these changes Apr 17, 2023

View reviewed changes

derek-ho merged commit b0a87fc into opensearch-project:main Apr 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bulk update separation #356

bulk update separation #356

derek-ho commented Apr 14, 2023 •

edited

Loading

joshuali925 Apr 14, 2023

joshuali925 Apr 14, 2023

joshuali925 Apr 14, 2023

derek-ho Apr 17, 2023

ps48 Apr 17, 2023

pjfitzgibbons Apr 17, 2023

derek-ho Apr 17, 2023

pjfitzgibbons Apr 17, 2023

derek-ho Apr 17, 2023

pjfitzgibbons Apr 17, 2023

joshuali925 Apr 17, 2023

derek-ho Apr 17, 2023

	/**
	* Delete a list of objects. Assumes object is osd visualization if id is a
	* UUID. Rest and failed ids will then be deleted by PPL client.
	*
	* @param params - SavedObjectsDeleteBulkParams
	* @returns SavedObjectsDeleteResponse
	*/
	static async deleteBulk(
	params: SavedObjectsDeleteBulkParams
	): Promise<SavedObjectsDeleteResponse> {
	const idMap = params.objectIdList.reduce((prev, id) => {
	const type = OSDSavedObjectClient.extractType(id);
	const key = type === '' ? 'non_osd' : type;
	return { ...prev, [key]: [...(prev[key] \|\| []), id] };
	}, {} as { [k in 'non_osd' \| ObservabilitySavedObjectsType]: string[] });


		const allVisualizations = panel!.visualizations;

		const visualizationsWithNewPanel = addVisualizationPanel(vizId, undefined, allVisualizations);

bulk update separation #356

bulk update separation #356

Conversation

derek-ho commented Apr 14, 2023 • edited Loading

Description

Issues Resolved

Check List

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

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

derek-ho commented Apr 14, 2023 •

edited

Loading