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

Update metadata #3361

Merged
merged 6 commits into from
Mar 28, 2023
Merged

Update metadata #3361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
45f443e
updating metadata
MVarshini Mar 25, 2023
eb17937
uncomment mine param
MVarshini Mar 25, 2023
0efec32
display metadata errors on update
MVarshini Mar 27, 2023
82fa71d
border top for checkbox
MVarshini Mar 27, 2023
c4e0a68
selected runs array
MVarshini Mar 27, 2023
30cd88e
prettier error
MVarshini Mar 27, 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
33 changes: 26 additions & 7 deletions dashboard/src/actions/overviewActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import * as CONSTANTS from "assets/constants/overviewConstants";
import * as TYPES from "./types";

import { DANGER, ERROR_MSG } from "assets/constants/toastConstants";

import API from "../utils/axiosInstance";
import { DANGER } from "assets/constants/toastConstants";
import { expandUriTemplate } from "../utils/helper";
import { findNoOfDays } from "utils/dateFunctions";
import { showToast } from "./toastActions";
import { expandUriTemplate } from "../utils/helper";

export const getDatasets = () => async (dispatch, getState) => {
const alreadyRendered = getState().overview.loadingDone;
Expand Down Expand Up @@ -41,7 +42,7 @@ export const getDatasets = () => async (dispatch, getState) => {
}
}
} catch (error) {
dispatch(showToast(DANGER, error?.response?.data?.message));
dispatch(showToast(DANGER, error?.response?.data?.message ?? ERROR_MSG));
dispatch({ type: TYPES.NETWORK_ERROR });
}
if (alreadyRendered) {
Expand Down Expand Up @@ -129,14 +130,26 @@ export const updateDataset =
(item) => item.resource_id === dataset.resource_id
);
runs[dataIndex].metadata[metaDataActions[actionType]] =
response.data[metaDataActions[actionType]];
response.data.metadata[metaDataActions[actionType]];
dispatch({
type: TYPES.USER_RUNS,
payload: runs,
});
dispatch(initializeRuns());

const errors = response.data?.errors;
if (errors && Object.keys(errors).length > 0) {
let errorText = "";

for (const [key, value] of Object.entries(errors)) {
errorText += `${key} : ${value} \n`;
}
Comment on lines +142 to +146
Copy link
Member

Choose a reason for hiding this comment

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

Alternately,

          const errorText = Object.entries(errors).reduce(
            (a, [k, v]) => a += `${k} : ${v} \n`,
            ""
            );

dispatch(
showToast("warning", "Problem updating metadata", errorText)
);
}
} else {
dispatch(showToast(DANGER, response?.data?.message));
dispatch(showToast(DANGER, response?.data?.message ?? ERROR_MSG));
}
} catch (error) {
dispatch(showToast(DANGER, error?.response?.data?.message));
Expand Down Expand Up @@ -173,7 +186,7 @@ export const deleteDataset = (dataset) => async (dispatch, getState) => {
dispatch(showToast(CONSTANTS.SUCCESS, "Deleted!"));
}
} catch (error) {
dispatch(showToast(DANGER, error?.response?.data?.message));
dispatch(showToast(DANGER, error?.response?.data?.message ?? ERROR_MSG));
dispatch({ type: TYPES.NETWORK_ERROR });
}
dispatch({ type: TYPES.COMPLETED });
Expand All @@ -193,6 +206,12 @@ export const setSelectedRuns = (rows) => {
};
};

export const setSelectedSavedRuns = (rows) => {
return {
type: TYPES.SELECTED_SAVED_RUNS,
payload: rows,
};
};
Comment on lines +209 to +214
Copy link
Member

Choose a reason for hiding this comment

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

Alternately,

export const setSelectedSavedRuns = (rows) => (
  {
    type: TYPES.SELECTED_SAVED_RUNS,
    payload: rows,
  }
);

That is, we only need to return a value which is an object -- we don't need any actual code (like a return statement).

export const updateMultipleDataset =
(method, value) => (dispatch, getState) => {
const selectedRuns = getState().overview.selectedRuns;
Expand Down Expand Up @@ -245,7 +264,7 @@ export const publishDataset =
dispatch(showToast(CONSTANTS.SUCCESS, "Updated!"));
}
} catch (error) {
dispatch(showToast(DANGER, error?.response?.data?.message));
dispatch(showToast(DANGER, error?.response?.data?.message ?? ERROR_MSG));
dispatch({ type: TYPES.NETWORK_ERROR });
}
dispatch({ type: TYPES.COMPLETED });
Expand Down
1 change: 1 addition & 0 deletions dashboard/src/actions/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export const SELECTED_NEW_RUNS = "SELECTED_NEW_RUNS";
export const EXPIRING_RUNS = "EXPIRING_RUNS";
export const SET_DASHBOARD_LOADING = "SET_DASHBOARD_LOADING";
export const SET_LOADING_FLAG = "SET_LOADING_FLAG";
export const SELECTED_SAVED_RUNS = "SELECTED_SAVED_RUNS";

/* TABLE OF CONTENT */
export const GET_TOC_DATA = "GET_TOC_DATA";
Expand Down
1 change: 1 addition & 0 deletions dashboard/src/assets/constants/toastConstants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export const DANGER = "danger";
export const ERROR_MSG = "Something went wrong!";
Copy link
Member

Choose a reason for hiding this comment

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

This is expedient, but it would be really handy if we could come up with something less generic. Any context would potentially be helpful.

1 change: 1 addition & 0 deletions dashboard/src/modules/components/OverviewComponent/NewRunsComponent.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ const NewRunsComponent = () => {
selectAllRuns(isSelecting),
isSelected: areAllRunsSelected,
}}
style={{ borderTop: "1px solid #d2d2d2" }}
></Th>
<Th width={35}>{columnNames.result}</Th>
<Th width={25}>{columnNames.endtime}</Th>
Expand Down
17 changes: 10 additions & 7 deletions dashboard/src/modules/components/OverviewComponent/SavedRunsComponent.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
editMetadata,
publishDataset,
setRowtoEdit,
setSelectedRuns,
setSelectedSavedRuns,
updateDataset,
} from "actions/overviewActions";
import { useDispatch, useSelector } from "react-redux";
Expand All @@ -29,23 +29,25 @@ import { SavedRunsRow } from "./common-component";

const SavedRunsComponent = () => {
const dispatch = useDispatch();
const { savedRuns, selectedRuns } = useSelector((state) => state.overview);
const { savedRuns, selectedSavedRuns } = useSelector(
(state) => state.overview
);

/* Selecting */
const areAllRunsSelected =
savedRuns?.length > 0 && savedRuns?.length === selectedRuns?.length;
savedRuns?.length > 0 && savedRuns?.length === selectedSavedRuns?.length;
Comment on lines 37 to +38
Copy link
Member

Choose a reason for hiding this comment

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

The ?. operator needs to be used with considerable care.

For instance, while savedRuns?.length > 0 will "do the right thing" if length is not present, the inverse, savedRuns?.length <= 0, probably won't (that is, they both return false if length is not present, which might be counterintuitive).

Also, we can (and should) use . instead of ?. in the second reference to savedRuns.length, because we know that the field is present if we get past the first test.

So, this code should do an explicit test for the presence of the field, and then test it's value:

    savedRuns?.length && savedRuns.length > 0 && savedRuns.length === selectedSavedRuns?.length;

However, if saved?.length is true-y, then I'm pretty sure that it is greater than zero, so we can omit the middle comparison.

const selectAllRuns = (isSelecting) => {
dispatch(setSelectedRuns(isSelecting ? [...savedRuns] : []));
dispatch(setSelectedSavedRuns(isSelecting ? [...savedRuns] : []));
};
const onSelectRuns = (run, _rowIndex, isSelecting) => {
const otherSelectedRuns = selectedRuns.filter(
const otherSelectedRuns = selectedSavedRuns.filter(
(r) => r.resource_id !== run.resource_id
);
const c = isSelecting ? [...otherSelectedRuns, run] : otherSelectedRuns;
dispatch(setSelectedRuns(c));
dispatch(setSelectedSavedRuns(c));
};
const isRowSelected = (run) =>
selectedRuns.filter((item) => item.name === run.name).length > 0;
selectedSavedRuns.filter((item) => item.name === run.name).length > 0;
/* Selecting */

/* Actions Row */
Expand Down Expand Up @@ -107,6 +109,7 @@ const SavedRunsComponent = () => {
selectAllRuns(isSelecting),
isSelected: areAllRunsSelected,
}}
style={{ borderTop: "1px solid #d2d2d2" }}
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why are we specifying the style in-line instead of putting it in the .less file?

></Th>
<Th>{columnNames.result}</Th>
<Th>{columnNames.uploadedtime}</Th>
Expand Down
13 changes: 4 additions & 9 deletions dashboard/src/modules/components/OverviewComponent/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@
.bordered {
border: 1px solid #d2d2d2;
}
.pf-c-table__check:first-child {
border-top: 1px solid #d2d2d2;
}
.pf-c-accordion__expanded-content.pf-m-expanded {
height: 100%;
}
Expand Down Expand Up @@ -50,20 +47,18 @@
}
.newruns-table-container {
height: 90%;
.pf-c-table__check {
border-top: 1px solid #d2d2d2;
}

.pf-c-scroll-outer-wrapper {
min-height: 100%;
}
.unseen-row {
background-color: #efefef;
}
}
.pf-c-pagination {
padding: 0;
}
}
.unseen-row {
background-color: #efefef;
}
}
.separator {
margin: 3vh 0;
Expand Down
5 changes: 4 additions & 1 deletion dashboard/src/modules/components/ToastNotificationComponent/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ const ToastComponent = () => {
/>
}
>
{item?.message && <p>{item?.message}</p>}
{item?.message &&
item?.message.split("\n").map((i, key) => {
Comment on lines +36 to +37
Copy link
Member

Choose a reason for hiding this comment

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

We don't need (and therefore shouldn't use) the ?. operator in the second reference.

return <p key={i}>{i}</p>;
})}
</Alert>
))}
</AlertGroup>
Expand Down
8 changes: 7 additions & 1 deletion dashboard/src/reducers/overviewReducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const initialState = {
defaultPerPage: 5,
initNewRuns: [],
selectedRuns: [],
selectedSavedRuns: [],
expiringRuns: [],
loadingDone: !!sessionStorage.getItem("loadingDone"),
};
Expand Down Expand Up @@ -37,7 +38,12 @@ const OverviewReducer = (state = initialState, action = {}) => {
case TYPES.SELECTED_NEW_RUNS:
return {
...state,
selectedRuns: payload,
selectedRuns: [...payload],
};
case TYPES.SELECTED_SAVED_RUNS:
return {
...state,
selectedSavedRuns: [...payload],
};
case TYPES.EXPIRING_RUNS:
return {
Expand Down