Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bulk update separation #356

Merged
merged 9 commits into from
Apr 17, 2023
Merged

bulk update separation #356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8bf80a8
bulk update separation
derek-ho Apr 14, 2023
35f260d
fix up pr
derek-ho Apr 14, 2023
ea5baf7
individual panel delete SO
derek-ho Apr 14, 2023
2b1837e
also separate out on newly created ones
derek-ho Apr 14, 2023
f3ba48b
Merge branch 'main' of github.com:opensearch-project/dashboards-obser…
derek-ho Apr 17, 2023
d5bc709
resolve pr comments
derek-ho Apr 17, 2023
9a0d150
bulk update new so panels
derek-ho Apr 17, 2023
19b9c9f
fix PR
derek-ho Apr 17, 2023
cedf122
Merge branch 'main' into bulk-update
derek-ho Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion public/components/custom_panels/helpers/add_visualization_helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export const addVisualizationPanel = (
// client: ILegacyScopedClusterClient,
// panelId: string,
savedVisualizationId: string,
oldVisualizationId?: string,
oldVisualizationId: string | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Thuoghts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

allPanelVisualizations: VisualizationType[]
) => {
try {
Expand Down
23 changes: 22 additions & 1 deletion public/components/custom_panels/redux/panel_slice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { createSelector, createSlice } from '@reduxjs/toolkit';
import { concat, from, Observable, of } from 'rxjs';
import { async, concat, from, Observable, of } from 'rxjs';
import { map, mergeMap, tap, toArray } from 'rxjs/operators';
import { forEach } from 'lodash';
import {
CUSTOM_PANELS_API_PREFIX,
CUSTOM_PANELS_SAVED_OBJECT_TYPE,
Expand All @@ -16,6 +17,7 @@ import {
import { coreRefs } from '../../../framework/core_refs';
import { SavedObject, SimpleSavedObject } from '../../../../../../src/core/public';
import { isNameValid } from '../helpers/utils';
import { addVisualizationPanel } from '../helpers/add_visualization_helper';

interface InitialState {
id: string;
Expand Down Expand Up @@ -135,6 +137,25 @@ export const updatePanel = (panel: CustomPanelType) => async (dispatch, getState
}
};

export const addVizToPanels = (panels, vizId) => async (dispatch, getState) => {
forEach(panels, (oldPanel) => {
console.log(oldPanel);
console.log(getState().customPanel.panelList);
derek-ho marked this conversation as resolved.
Show resolved Hide resolved
const panel = getState().customPanel.panelList.find((p) => p.id === oldPanel.panel.id);

const allVisualizations = panel!.visualizations;

const visualizationsWithNewPanel = addVisualizationPanel(vizId, undefined, allVisualizations);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 = (
?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const updatedPanel = { ...panel, visualizations: visualizationsWithNewPanel };
try {
dispatch(updatePanel(updatedPanel));
} catch (err) {
console.error(err?.body?.message || err);
}
});
};

export const deletePanel = (id) => async (dispatch, getState) => {
await savedObjectPanelsClient.delete(id);
const panelList: CustomPanelType[] = getState().panelList.filter((p) => p.id !== id);
Expand Down
8 changes: 7 additions & 1 deletion public/services/saved_objects/saved_object_savers/ppl/save_as_current_vis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { uuidRx } from '../../../../../public/components/custom_panels/redux/panel_slice';
import {
addVizToPanels,
uuidRx,
} from '../../../../../public/components/custom_panels/redux/panel_slice';
import { SavedQuerySaver } from './saved_query_saver';

export class SaveAsCurrentVisualization extends SavedQuerySaver {
Expand Down Expand Up @@ -47,8 +50,11 @@ export class SaveAsCurrentVisualization extends SavedQuerySaver {
}

addToPanel({ selectedPanels, saveTitle, notifications, visId }) {
const { dispatch } = this.dispatchers;
const soPanels = selectedPanels.filter((panel) => panel.panel.id.test(uuidRx));
const opsPanels = selectedPanels.filter((panel) => !panel.panel.id.test(uuidRx));
derek-ho marked this conversation as resolved.
Show resolved Hide resolved
dispatch(addVizToPanels(soPanels, visId));

this.panelClient
.updateBulk({
selectedCustomPanels: opsPanels,
Expand Down
11 changes: 10 additions & 1 deletion public/services/saved_objects/saved_object_savers/ppl/save_as_new_vis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,20 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { uuidRx } from '../../../../../public/components/custom_panels/redux/panel_slice';
import { forEach } from 'lodash';
import {
addVizToPanels,
fetchPanel,
uuidRx,
} from '../../../../../public/components/custom_panels/redux/panel_slice';
import {
SAVED_OBJECT_ID,
SAVED_OBJECT_TYPE,
SAVED_VISUALIZATION,
} from '../../../../../common/constants/explorer';
import { ISavedObjectsClient } from '../../saved_object_client/client_interface';
import { SavedQuerySaver } from './saved_query_saver';
import { addVisualizationPanel } from '../../../../../public/components/custom_panels/helpers/add_visualization_helper';

export class SaveAsNewVisualization extends SavedQuerySaver {
constructor(
Expand Down Expand Up @@ -77,8 +83,11 @@ export class SaveAsNewVisualization extends SavedQuerySaver {
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

const soPanels = selectedPanels.filter((panel) => panel.panel.id.test(uuidRx));
const opsPanels = selectedPanels.filter((panel) => !panel.panel.id.test(uuidRx));

dispatch(addVizToPanels(soPanels, visId));
this.panelClient
.updateBulk({
selectedCustomPanels: opsPanels,
Expand Down