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 7 commits
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
78 changes: 38 additions & 40 deletions public/components/custom_panels/home.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,12 @@ import {
ObservabilityPanelAttrs,
PanelType,
} from '../../../common/types/custom_panels';
import { ObservabilitySideBar } from '../common/side_nav';
import { CustomPanelTable } from './custom_panel_table';
import { CustomPanelView } from './custom_panel_view';
import { isNameValid } from './helpers/utils';
import { CustomPanelViewSO } from './custom_panel_view_so';
import { coreRefs } from '../../framework/core_refs';
import { fetchPanels } from './redux/panel_slice';
import { deletePanel, fetchPanels, uuidRx } from './redux/panel_slice';

// import { ObjectFetcher } from '../common/objectFetcher';

Expand Down Expand Up @@ -143,8 +142,6 @@ export const Home = ({
});
};

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 isUuid = (id) => !!id.match(uuidRx);

const fetchSavedObjectPanel = async (id: string) => {
Expand Down Expand Up @@ -226,44 +223,46 @@ export const Home = ({

// Deletes multiple existing Operational Panels
const deleteCustomPanelList = (customPanelIdList: string[], toastMessage: string) => {
// Promise.all([
// deletePanelSO(customPanelIdList),
// deletePanels(customPanelIdList)
// ]).then((res) => {
// setcustomPanelData((prevCustomPanelData) => {
// return prevCustomPanelData.filter(
// (customPanel) => !customPanelIdList.includes(customPanel.id)
// );
// });
// setToast(toastMessage);
// })
// .catch((err) => {
// setToast(
// 'Error deleting Operational Panels, please make sure you have the correct permission.',
// 'danger'
// );
// console.error(err.body.message);
// });
Promise.all([deletePanelSO(customPanelIdList), deletePanels(customPanelIdList)])
.then((res) => {
// setcustomPanelData((prevCustomPanelData) => {
// return prevCustomPanelData.filter(
// (customPanel) => !customPanelIdList.includes(customPanel.id)
// );
// });
// setToast(toastMessage);
})
.catch((err) => {
setToast(
'Error deleting Operational Panels, please make sure you have the correct permission.',
'danger'
);
console.error(err.body.message);
});
};

// Deletes an existing Operational Panel
const deleteCustomPanel = async (customPanelId: string, customPanelName: string) => {
// return http
// .delete(`${CUSTOM_PANELS_API_PREFIX}/panels/` + customPanelId)
// .then((res) => {
// setcustomPanelData((prevCustomPanelData) => {
// return prevCustomPanelData.filter((customPanel) => customPanel.id !== customPanelId);
// });
// setToast(`Operational Panel "${customPanelName}" successfully deleted!`);
// return res;
// })
// .catch((err) => {
// setToast(
// 'Error deleting Operational Panel, please make sure you have the correct permission.',
// 'danger'
// );
// console.error(err.body.message);
// });
return http
.delete(`${CUSTOM_PANELS_API_PREFIX}/panels/` + customPanelId)
.then((res) => {
dispatch(fetchPanels());
setToast(`Operational Panel "${customPanelName}" successfully deleted!`);
return res;
})
.catch((err) => {
setToast(
'Error deleting Operational Panel, please make sure you have the correct permission.',
'danger'
);
console.error(err.body.message);
});
};

// Deletes an existing SO Operational Panel
const deleteCustomPanelSO = async (customPanelId: string, customPanelName: string) => {
dispatch(deletePanel(customPanelId));
// TODO: toast here
};

const addSamplePanels = async () => {
Expand Down Expand Up @@ -306,7 +305,6 @@ export const Home = ({
})
.then((res) => {
dispatch(fetchPanels());
// setcustomPanelData([...customPanelData, ...res.demoPanelsData]);
});
setToast(`Sample panels successfully added.`);
} catch (err: any) {
Expand Down Expand Up @@ -358,7 +356,7 @@ export const Home = ({
chrome={chrome}
parentBreadcrumbs={customPanelBreadCrumbs}
cloneCustomPanel={cloneCustomPanel}
deleteCustomPanel={deleteCustomPanel}
deleteCustomPanel={deleteCustomPanelSO}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can modify this in a follow up PR

setToast={setToast}
onEditClick={onEditClick}
page="operationalPanels"
Expand Down
70 changes: 44 additions & 26 deletions 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 @@ -56,10 +58,7 @@ export const panelReducer = panelSlice.reducer;

export const selectPanel = (rootState): CustomPanelType => rootState.customPanel.panel;

export const selectPanelList = (rootState): CustomPanelType[] => {
// console.log('selectPanelList', { rootState, panelList: rootState.customPanel.panelList });
return rootState.customPanel.panelList;
};
export const selectPanelList = (rootState): CustomPanelType[] => rootState.customPanel.panelList;

// export const selectPanelList = createSelector(
// rootState => { console.log("selectPanelList", { rootState }); return rootState.customPanel.panelList },
Expand Down Expand Up @@ -90,16 +89,18 @@ const fetchCustomPanels = async () => {
const panels$: Observable<CustomPanelListType> = concat(
fetchSavedObjectPanels$(),
fetchObservabilityPanels$()
).pipe(map((res) => {
console.log("fetchCustomPanels", res);
return res as CustomPanelListType
}));
).pipe(
map((res) => {
console.log('fetchCustomPanels', res);
return res as CustomPanelListType;
})
);

return panels$.pipe(toArray()).toPromise();
};

export const fetchPanels = () => async (dispatch, getState) => {
const panels = await fetchCustomPanels()
const panels = await fetchCustomPanels();
console.log('fetchPanels', { panels });
dispatch(setPanelList(panels));
};
Expand All @@ -112,34 +113,49 @@ export const fetchPanel = (id) => async (dispatch, getState) => {

export const fetchVisualization = () => (dispatch, getState) => {};

const updateLegacyPanel = (panel: CustomPanelType) => coreRefs.http!
.post(`${CUSTOM_PANELS_API_PREFIX}/panels/update`, {
const updateLegacyPanel = (panel: CustomPanelType) =>
coreRefs.http!.post(`${CUSTOM_PANELS_API_PREFIX}/panels/update`, {
body: JSON.stringify({ panelId: panel.id, panel: panel as PanelType }),
});

const updateSavedObjectPanel = (panel: CustomPanelType) => savedObjectPanelsClient.update(panel);


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}$/;
export 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 isUuid = (id) => !!id.match(uuidRx);


export const updatePanel = (panel: CustomPanelType) => async (dispatch, getState) => {
try {
if (isUuid(panel.id))
await updateSavedObjectPanel(panel)
else
await updateLegacyPanel(panel)
if (isUuid(panel.id)) await updateSavedObjectPanel(panel);
else await updateLegacyPanel(panel);

dispatch(setPanel(panel));
const panelList = getState().customPanel.panelList.map((p) => (p.id === panel.id ? panel : p));
dispatch(setPanelList(panelList));
} catch (err) {
console.log("Error updating panel", { err, panel })
console.log('Error updating panel', { err, panel });
}
};

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 All @@ -152,7 +168,6 @@ export const createPanel = (panel) => async (dispatch, getState) => {
dispatch(setPanelList([...panelList, newPanel]));
};


const saveRenamedPanel = async (id, name) => {
const renamePanelObject = {
panelId: id,
Expand All @@ -174,17 +189,20 @@ const saveRenamedPanelSO = async (id, name) => {
};

// Renames an existing CustomPanel
export const renameCustomPanel = (editedCustomPanelName: string, id: string) => async (dispatch, getState) => {
console.log("renameCustomPanel dispatched", { editedCustomPanelName, id })
export const renameCustomPanel = (editedCustomPanelName: string, id: string) => async (
dispatch,
getState
) => {
console.log('renameCustomPanel dispatched', { editedCustomPanelName, id });

if (!isNameValid(editedCustomPanelName)) {
console.log('Invalid Custom Panel name', 'danger');
return Promise.reject();
}

const panel = getState().customPanel.panelList.find(p => p.id === id)
const updatedPanel = { ...panel, title: editedCustomPanelName }
dispatch(updatePanel(updatedPanel))
const panel = getState().customPanel.panelList.find((p) => p.id === id);
const updatedPanel = { ...panel, title: editedCustomPanelName };
dispatch(updatePanel(updatedPanel));

// try {
// // await savePanelFn(editedCustomPanelId, editedCustomPanelName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ export const SavePanel = ({
setSubType,
isSaveAsMetricEnabled,
}: ISavedPanelProps) => {
const [options, setOptions] = useState([]);
const [checked, setChecked] = useState(false);
const [svpnlError, setSvpnlError] = useState(null);

Expand All @@ -62,20 +61,6 @@ export const SavePanel = ({
dispatch(fetchPanels());
}, []);

const getCustomPabnelList = async (svobj: SavedObjects) => {
const optionRes = await svobj
.fetchCustomPanels()
.then((res: any) => {
return res;
})
.catch((error: any) => setSvpnlError(error));
setOptions(optionRes?.panels || []);
};

useEffect(() => {
getCustomPabnelList(savedObjects);
}, []);

const onToggleChange = (e: { target: { checked: React.SetStateAction<boolean> } }) => {
setChecked(e.target.checked);
if (e.target.checked) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
* SPDX-License-Identifier: Apache-2.0
*/

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 @@ -46,9 +50,14 @@ 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: selectedPanels,
selectedCustomPanels: opsPanels,
savedVisualizationId: visId,
})
.then((res: any) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,20 @@
* SPDX-License-Identifier: Apache-2.0
*/

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 @@ -76,9 +83,14 @@ 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: selectedPanels,
selectedCustomPanels: opsPanels,
savedVisualizationId: visId,
})
.then((res: any) => {
Expand Down