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

Soft delete of jobs and datasets. Style dialog component #2343

Merged
merged 8 commits into from
Jan 13, 2023
Merged

Soft delete of jobs and datasets. Style dialog component #2343

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
505f782
Soft delete of jobs and datasets. Style dialog component
tito12 Jan 3, 2023
209bf32
fix unit tests
tito12 Jan 5, 2023
c26958b
fix unit tests
tito12 Jan 5, 2023
a8f9bac
Fix unit web tests and lint
tito12 Jan 9, 2023
f6d4c0b
fix lint code
tito12 Jan 9, 2023
3ec0c7e
Changes after review. Button style, i18next and else
tito12 Jan 9, 2023
d3e1e03
fix unit web tests
tito12 Jan 9, 2023
ad82bba
Merge branch 'main' into web/softdelete
tito12 Jan 13, 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
27 changes: 20 additions & 7 deletions web/src/components/Dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import * as React from 'react'
import { dialogToggle } from '../store/actionCreators'
import { theme } from '../helpers/theme'
import Button from '@material-ui/core/Button'
import Dialog from '@material-ui/core/Dialog'
import DialogActions from '@material-ui/core/DialogActions'
Expand All @@ -26,16 +27,28 @@ export default function AlertDialog(props: IProps) {
<div>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to have this encapsulating tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component exist previously with encapsulating tag. Moved it to functional component as rest part components.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks

<Dialog open={props.dialogIsOpen}>
<DialogTitle>{props.title}</DialogTitle>
<DialogContent>
<DialogContentText>{props.editWarningField}</DialogContentText>
</DialogContent>
{props.editWarningField &&
<DialogContent>
<DialogContentText>{props.editWarningField}</DialogContentText>
</DialogContent>
}
<DialogActions>
<Button className='dialogButton' color='secondary' onClick={props.ignoreWarning}>
Continue
</Button>
<Button className='dialogButton' color='primary' onClick={handleClose}>
<Button
className='dialogButton'
color='primary'
onClick={handleClose}
style={{ backgroundColor: theme.palette.error.main, color: 'white' }}
>
Cancel
</Button>
<Button
className='dialogButton'
color='primary'
variant='outlined'
onClick={props.ignoreWarning}
>
Continue
</Button>
Copy link
Member

Choose a reason for hiding this comment

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

Can we make these small buttons inside the dialog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, can you explain what do you mean by that?

Copy link
Member

Choose a reason for hiding this comment

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

I think we could possibly use smaller buttons here since the dialog is so minute, but maybe it's not necessary.

</DialogActions>
</Dialog>
</div>
Expand Down
78 changes: 63 additions & 15 deletions web/src/components/datasets/DatasetDetailPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
// SPDX-License-Identifier: Apache-2.0

import * as Redux from 'redux'
import { Box, Chip, Tab, Tabs } from '@material-ui/core'
import { alpha } from '@material-ui/core/styles'
import { Box, Chip, Tab, Tabs, Button } from '@material-ui/core'
import { DatasetVersion } from '../../types/api'
import { IState } from '../../store/reducers'
import {
Expand All @@ -10,20 +11,24 @@ import {
createStyles,
withStyles
} from '@material-ui/core/styles'
import { theme } from '../../helpers/theme'
import { LineageDataset } from '../lineage/types'
import { bindActionCreators } from 'redux'
import { connect } from 'react-redux'
import {
fetchDatasetVersions,
resetDataset,
resetDatasetVersions
resetDatasetVersions,
deleteDataset,
dialogToggle
} from '../../store/actionCreators'
import { useHistory } from 'react-router-dom'
import CircularProgress from '@material-ui/core/CircularProgress/CircularProgress'
import CloseIcon from '@material-ui/icons/Close'
import DatasetColumnLineage from './DatasetColumnLineage'
import DatasetInfo from './DatasetInfo'
import DatasetVersions from './DatasetVersions'
import Dialog from '../Dialog'
import IconButton from '@material-ui/core/IconButton'
import MqText from '../core/text/MqText'
import React, { ChangeEvent, FunctionComponent, SetStateAction, useEffect } from 'react'
Expand All @@ -44,6 +49,13 @@ const styles = ({ spacing }: ITheme) => {
'&:not(:last-of-type)': {
marginRight: spacing(1)
}
},
buttonDelete: {
backgroundColor: theme.palette.error.main,
color: 'white',
'&:hover': {
backgroundColor: alpha(theme.palette.error.main, 0.9)
},
}
})
}
Expand All @@ -52,12 +64,16 @@ interface StateProps {
lineageDataset: LineageDataset
versions: DatasetVersion[]
versionsLoading: boolean
datasets: IState['datasets']
display: IState['display']
}

interface DispatchProps {
fetchDatasetVersions: typeof fetchDatasetVersions
resetDatasetVersions: typeof resetDatasetVersions
resetDataset: typeof resetDataset
deleteDataset: typeof deleteDataset
dialogToggle: typeof dialogToggle
}

type IProps = IWithStyles<typeof styles> & StateProps & DispatchProps
Expand All @@ -72,11 +88,15 @@ function a11yProps(index: number) {
const DatasetDetailPage: FunctionComponent<IProps> = props => {
const {
classes,
datasets,
display,
fetchDatasetVersions,
resetDataset,
resetDatasetVersions,
deleteDataset,
versions,
versionsLoading
versionsLoading,
lineageDataset
} = props
const { root } = classes
const history = useHistory()
Expand All @@ -86,14 +106,17 @@ const DatasetDetailPage: FunctionComponent<IProps> = props => {
fetchDatasetVersions(props.lineageDataset.namespace, props.lineageDataset.name)
}, [props.lineageDataset.name])

useEffect(() => {
if (datasets.deletedDatasetName) {
history.push('/datasets')
}
}, [datasets.deletedDatasetName])

// unmounting
useEffect(
() => () => {
resetDataset()
resetDatasetVersions()
},
[]
)
useEffect(() => () => {
resetDataset()
resetDatasetVersions()
}, [])

const [tab, setTab] = React.useState(0)
const handleChange = (event: ChangeEvent, newValue: SetStateAction<number>) => {
Expand Down Expand Up @@ -147,9 +170,31 @@ const DatasetDetailPage: FunctionComponent<IProps> = props => {
/>
</Tabs>
</Box>
<IconButton onClick={() => history.push('/datasets')}>
<CloseIcon />
</IconButton>
<Box display={'flex'} alignItems={'center'}>
<Box mr={1}>
<Button
color='primary'
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a ghost variant... I think it's a little too strong on the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, good point

className={classes.buttonDelete}
onClick={() => {
props.dialogToggle('')
}}
>
{i18next.t('jobs.delete')}
</Button>
<Dialog
Copy link
Member

Choose a reason for hiding this comment

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

There are two instances of this dialog... Can the dialog itself just be a reusable component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component is reusable and located in "components/Dialog.tsx", and use it with different props

dialogIsOpen={display.dialogIsOpen}
dialogToggle={dialogToggle}
title={'Are you sure?'} // i18next.t('jobs.dialogTitleConfirm')
tito12 marked this conversation as resolved.
Show resolved Hide resolved
ignoreWarning={() => {
deleteDataset(lineageDataset.name, lineageDataset.namespace)
props.dialogToggle('')
}}
/>
</Box>
<IconButton onClick={() => history.push('/datasets')}>
<CloseIcon />
</IconButton>
</Box>
</Box>
<MqText heading font={'mono'}>
{name}
Expand All @@ -172,7 +217,8 @@ const DatasetDetailPage: FunctionComponent<IProps> = props => {
}

const mapStateToProps = (state: IState) => ({
datasets: state.datasets.result,
datasets: state.datasets,
display: state.display,
versions: state.datasetVersions.result.versions,
versionsLoading: state.datasetVersions.isLoading
})
Expand All @@ -182,7 +228,9 @@ const mapDispatchToProps = (dispatch: Redux.Dispatch) =>
{
fetchDatasetVersions: fetchDatasetVersions,
resetDatasetVersions: resetDatasetVersions,
resetDataset: resetDataset
resetDataset: resetDataset,
deleteDataset: deleteDataset,
dialogToggle: dialogToggle
},
dispatch
)
Expand Down
55 changes: 51 additions & 4 deletions web/src/components/jobs/JobDetailPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import React, { ChangeEvent, FunctionComponent, SetStateAction, useEffect } from

import '../../i18n/config'
import * as Redux from 'redux'
import { alpha } from '@material-ui/core/styles'
import { Box, Button, CircularProgress, Tab, Tabs } from '@material-ui/core'
import { IState } from '../../store/reducers'
import {
Expand All @@ -12,13 +13,15 @@ import {
createStyles,
withStyles
} from '@material-ui/core/styles'
import { theme } from '../../helpers/theme'
import { LineageJob } from '../lineage/types'
import { Run } from '../../types/api'
import { bindActionCreators } from 'redux'
import { connect } from 'react-redux'
import { fetchRuns, resetRuns } from '../../store/actionCreators'
import { fetchRuns, resetRuns, resetJobs, deleteJob, dialogToggle } from '../../store/actionCreators'
import { useHistory } from 'react-router-dom'
import CloseIcon from '@material-ui/icons/Close'
import Dialog from '../Dialog'
import IconButton from '@material-ui/core/IconButton'
import MqEmpty from '../core/empty/MqEmpty'
import MqText from '../core/text/MqText'
Expand All @@ -30,23 +33,35 @@ const styles = ({ spacing }: ITheme) => {
return createStyles({
root: {
padding: spacing(2)
},
buttonDelete: {
backgroundColor: theme.palette.error.main,
color: 'white',
Copy link
Member

Choose a reason for hiding this comment

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

I think I like doing the theme.palette.common.white for these in case we ever wish to support light mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added it

'&:hover': {
backgroundColor: alpha(theme.palette.error.main, 0.9)
},
}
})
}

interface DispatchProps {
fetchRuns: typeof fetchRuns
resetRuns: typeof resetRuns
resetJobs: typeof resetJobs
deleteJob: typeof deleteJob
dialogToggle: typeof dialogToggle
}

type IProps = IWithStyles<typeof styles> & {
job: LineageJob
jobs: IState['jobs']
runs: Run[]
runsLoading: boolean
display: IState['display']
} & DispatchProps

const JobDetailPage: FunctionComponent<IProps> = props => {
const { job, classes, fetchRuns, resetRuns, runs, runsLoading } = props
const { job, jobs, classes, fetchRuns, resetRuns, deleteJob, dialogToggle, runs, display, runsLoading } = props
const history = useHistory()

const [tab, setTab] = React.useState(0)
Expand All @@ -59,10 +74,17 @@ const JobDetailPage: FunctionComponent<IProps> = props => {
fetchRuns(job.name, job.namespace)
}, [job.name])

useEffect(() => {
if (jobs.deletedJobName) {
history.push('/')
}
}, [jobs.deletedJobName])

// unmounting
useEffect(() => {
return () => {
resetRuns()
resetJobs()
}
}, [])

Expand All @@ -88,6 +110,26 @@ const JobDetailPage: FunctionComponent<IProps> = props => {
<Tab label={i18next.t('jobs.history_tab')} disableRipple={true} />
</Tabs>
<Box display={'flex'} alignItems={'center'}>
<Box mr={1}>
<Button
color='primary'
className={classes.buttonDelete}
onClick={() => {
props.dialogToggle('')
}}
>
{i18next.t('jobs.delete')}
</Button>
<Dialog
dialogIsOpen={display.dialogIsOpen}
dialogToggle={dialogToggle}
title={'Are you sure?'} // i18next.t('jobs.dialogTitleConfirm')
ignoreWarning={() => {
deleteJob(job.name, job.namespace)
props.dialogToggle('')
}}
/>
</Box>
<Box mr={1}>
<Button variant='outlined' color='primary' target={'_blank'} href={job.location}>
{i18next.t('jobs.location')}
Expand Down Expand Up @@ -128,14 +170,19 @@ const JobDetailPage: FunctionComponent<IProps> = props => {

const mapStateToProps = (state: IState) => ({
runs: state.runs.result,
runsLoading: state.runs.isLoading
runsLoading: state.runs.isLoading,
display: state.display,
jobs: state.jobs
})

const mapDispatchToProps = (dispatch: Redux.Dispatch) =>
bindActionCreators(
{
fetchRuns: fetchRuns,
resetRuns: resetRuns
resetRuns: resetRuns,
resetJobs: resetJobs,
deleteJob: deleteJob,
dialogToggle: dialogToggle
},
dispatch
)
Expand Down
3 changes: 2 additions & 1 deletion web/src/i18n/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ i18next
empty_title: 'No Run Information Available',
empty_body: 'Try adding some runs for this job.',
runinfo_subhead: 'FACETS',
runs_subhead: 'FACETS'
runs_subhead: 'FACETS',
delete: "DELETE"
},
search: {
search: 'Search',
Expand Down
4 changes: 4 additions & 0 deletions web/src/store/actionCreators/actionTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ export const SET_BOTTOM_BAR_HEIGHT = 'SET_BOTTOM_BAR_HEIGHT'
export const FETCH_JOBS = 'FETCH_JOBS'
export const FETCH_JOBS_SUCCESS = 'FETCH_JOBS_SUCCESS'
export const RESET_JOBS = 'RESET_JOBS'
export const DELETE_JOB = 'DELETE_JOB'
export const DELETE_JOB_SUCCESS = 'DELETE_JOB_SUCCESS'

// datasets
export const FETCH_DATASETS = 'FETCH_DATASETS'
Expand All @@ -30,6 +32,8 @@ export const RESET_DATASET = 'RESET_DATASET'
export const FETCH_DATASET_VERSIONS = 'FETCH_DATASET_VERSIONS'
export const FETCH_DATASET_VERSIONS_SUCCESS = 'FETCH_DATASET_VERSIONS_SUCCESS'
export const RESET_DATASET_VERSIONS = 'RESET_DATASET_VERSIONS'
export const DELETE_DATASET = 'DELETE_DATASET'
export const DELETE_DATASET_SUCCESS = 'DELETE_DATASET_SUCCESS'

// events
export const FETCH_EVENTS = 'FETCH_EVENTS'
Expand Down
Loading