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.

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

bulk update separation #356

Merged
merged 9 commits into from
Apr 17, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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
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:

?

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