Skip to content
This repository has been archived by the owner on Mar 23, 2019. It is now read-only.

Bug 1520857- add support in tc-web for displaying information from the lastFire table #413

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Bug 1520857- add support in tc-web for displaying information from the lastFire table #413

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c57174f
Initial display of lastfire fires
Biboswan Jan 23, 2019
7a33df8
Improve responsiveness and overall ui with Grid
Biboswan Jan 24, 2019
5b5c97d
Remove unwanted class
Biboswan Jan 24, 2019
a154a50
Merge two hook graphql queries into one
Biboswan Jan 25, 2019
f55678d
Replace expansion panel with datatable
Biboswan Jan 25, 2019
c5f5a16
Add pagination
Biboswan Feb 6, 2019
87993e3
minor fix
Biboswan Feb 15, 2019
f98d385
Add toggle view for error
Biboswan Feb 15, 2019
4674382
Add drawer to display error info
Biboswan Feb 22, 2019
0156365
Minor tweak
Biboswan Feb 22, 2019
e238f6f
add link icon for taskids
Biboswan Feb 23, 2019
8bb249f
minor tweak
Biboswan Feb 25, 2019
2b28062
Remove 1-20 part and text rename
Biboswan Mar 13, 2019
aa51ef9
add scrollbar
Biboswan Mar 15, 2019
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
43 changes: 42 additions & 1 deletion src/components/DataTable/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import {
oneOf,
oneOfType,
object,
bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding pagination? Is the last fire endpoint paginated (i.e., uses continuation token)?

Copy link
Author

Choose a reason for hiding this comment

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

As per discussion with Dustin in irc we are keeping as many as 100 lastfire info rows for each hookId (expiration logic). Hence I think showing all rows at once won't be good.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also only purge those once per day, so a particularly active hook might have many more rows than that at some time during the day!

} from 'prop-types';
import Table from '@material-ui/core/Table';
import TableBody from '@material-ui/core/TableBody';
import TableCell from '@material-ui/core/TableCell';
import TableHead from '@material-ui/core/TableHead';
import TableSortLabel from '@material-ui/core/TableSortLabel';
import TableRow from '@material-ui/core/TableRow';
import TablePagination from '@material-ui/core/TablePagination';

/**
* A table to display a set of data elements.
Expand All @@ -26,6 +28,12 @@ export default class DataTable extends Component {
sortByHeader: null,
sortDirection: 'desc',
noItemsMessage: 'No items for this page.',
isPaginate: false,
};

state = {
page: 0,
rowsPerPage: 10,
};

static propTypes = {
Expand Down Expand Up @@ -66,6 +74,10 @@ export default class DataTable extends Component {
* A message to display when there is no items to display.
*/
noItemsMessage: string,
/**
* Whether to paginate the table
*/
isPaginate: bool,
};

handleHeaderClick = ({ target }) => {
Expand All @@ -76,6 +88,14 @@ export default class DataTable extends Component {
}
};

handleChangePage = (event, page) => {
this.setState({ page });
};

handleChangeRowsPerPage = event => {
this.setState({ rowsPerPage: event.target.value });
};

render() {
const {
items,
Expand All @@ -85,8 +105,10 @@ export default class DataTable extends Component {
sortByHeader,
sortDirection,
noItemsMessage,
isPaginate,
} = this.props;
const colSpan = columnsSize || (headers && headers.length) || 0;
const { page, rowsPerPage } = this.state;

return (
<Table>
Expand Down Expand Up @@ -115,9 +137,28 @@ export default class DataTable extends Component {
</TableCell>
</TableRow>
) : (
items.map(renderRow)
items
.slice(page * rowsPerPage, page * rowsPerPage + rowsPerPage)
.map(renderRow)
)}
</TableBody>
{isPaginate && (
<TablePagination
rowsPerPageOptions={[5, 10, 25]}
component="div"
count={items.length}
rowsPerPage={rowsPerPage}
page={page}
backIconButtonProps={{
'aria-label': 'Previous Page',
}}
nextIconButtonProps={{
'aria-label': 'Next Page',
}}
onChangePage={this.handleChangePage}
onChangeRowsPerPage={this.handleChangeRowsPerPage}
/>
)}
</Table>
);
}
Expand Down
167 changes: 106 additions & 61 deletions src/components/HookForm/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,22 @@
import React, { Component, Fragment } from 'react';
import { Link } from 'react-router-dom';
import { string, bool, func, oneOfType, object } from 'prop-types';
import { string, bool, func, oneOfType, object, array } from 'prop-types';
import classNames from 'classnames';
import { equals, assocPath } from 'ramda';
import cloneDeep from 'lodash.clonedeep';
import CodeEditor from '@mozilla-frontend-infra/components/CodeEditor';
import Code from '@mozilla-frontend-infra/components/Code';
import ExpansionPanel from '@material-ui/core/ExpansionPanel';
import ExpansionPanelSummary from '@material-ui/core/ExpansionPanelSummary';
import ExpansionPanelDetails from '@material-ui/core/ExpansionPanelDetails';
import ExpandMoreIcon from '@material-ui/icons/ExpandMore';
import { withStyles } from '@material-ui/core/styles';
import List from '@material-ui/core/List';
import ListItem from '@material-ui/core/ListItem';
import ListItemText from '@material-ui/core/ListItemText';
import ListSubheader from '@material-ui/core/ListSubheader';
import TableRow from '@material-ui/core/TableRow';
import TableCell from '@material-ui/core/TableCell';
import TextField from '@material-ui/core/TextField';
import Switch from '@material-ui/core/Switch';
import FormGroup from '@material-ui/core/FormGroup';
Expand All @@ -21,15 +27,16 @@ import Typography from '@material-ui/core/Typography';
import FlashIcon from 'mdi-react/FlashIcon';
import PlusIcon from 'mdi-react/PlusIcon';
import DeleteIcon from 'mdi-react/DeleteIcon';
import LinkIcon from 'mdi-react/LinkIcon';
import ContentSaveIcon from 'mdi-react/ContentSaveIcon';
import { docs } from 'taskcluster-lib-urls';
import Label from '@mozilla-frontend-infra/components/Label';
import ErrorPanel from '../ErrorPanel';
import DataTable from '../DataTable';
import Button from '../Button';
import SpeedDial from '../SpeedDial';
import SpeedDialAction from '../SpeedDialAction';
import DialogAction from '../DialogAction';
import DateDistance from '../DateDistance';
import { HOOKS_LAST_FIRE_TYPE } from '../../utils/constants';
import { hook } from '../../utils/prop-types';
import removeKeys from '../../utils/removeKeys';

Expand Down Expand Up @@ -114,12 +121,24 @@ const initialHook = {
subheader: {
fontSize: theme.typography.pxToRem(16),
},
displayBlock: {
display: 'block',
},
errorTableCell: {
whiteSpace: 'normal',
},
errorPanel: {
maxHeight: '300px',
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to put quotations when the units are px.

maxHeight: 300,

maxWidth: '75ch',
overflowY: 'scroll',
},
}))
/** A form to view/edit/create a hook */
export default class HookForm extends Component {
static defaultProps = {
isNewHook: false,
hook: initialHook,
hookLastFires: null,
onTriggerHook: null,
onCreateHook: null,
onUpdateHook: null,
Expand All @@ -129,8 +148,12 @@ export default class HookForm extends Component {
};

static propTypes = {
/** A GraphQL hook response. Not needed when creating a new hook */
/** Part of a GraphQL hook response containing info about that hook.
Not needed when creating a new hook */
hook: hook.isRequired,
/** Part of the same Grahql hook response as above containing info
about some last hook fired attempts */
hookLastFires: array,
/** Set to `true` when creating a new hook. */
isNewHook: bool,
/** Callback function fired when a hook is triggered. */
Expand All @@ -153,6 +176,7 @@ export default class HookForm extends Component {

state = {
hook: null,
hookLastFires: null,
// eslint-disable-next-line react/no-unused-state
previousHook: null,
taskInput: '',
Expand All @@ -164,15 +188,21 @@ export default class HookForm extends Component {
validation: {},
};

expandsionSummary = React.createRef();

static getDerivedStateFromProps(props, state) {
if (equals(props.hook, state.previousHook)) {
if (
equals(props.hook, state.previousHook) &&
equals(props.hookLastFires, state.hookLastFires)
) {
return null;
}

const hook = props.isNewHook ? initialHook : props.hook;

return {
hook: props.hook,
hookLastFires: props.hookLastFires,
previousHook: props.hook,
taskInput: JSON.stringify(
removeKeys(cloneDeep(hook.task), ['__typename']),
Expand Down Expand Up @@ -412,11 +442,11 @@ export default class HookForm extends Component {
triggerSchemaInput,
triggerContextInput,
hook,
hookLastFires,
validation,
} = this.state;
/* eslint-disable-next-line no-underscore-dangle */
const lastFireTypeName = !isNewHook && hook.status.lastFire.__typename;

/* eslint-disable-next-line no-underscore-dangle */
return (
<Fragment>
<List>
Expand Down Expand Up @@ -508,61 +538,76 @@ export default class HookForm extends Component {
/>
</FormGroup>
</ListItem>
<ListItem className={classes.displayBlock}>
<ListItemText
primary={
<Typography variant="subtitle1">
Last Hook Fire Attempts
</Typography>
}
/>
{hookLastFires && (
<DataTable
items={hookLastFires}
headers={['Task ID', 'FiredBy', 'Result', 'Attempted', 'Error']}
isPaginate
renderRow={hookFire => (
<TableRow key={hookFire.taskId}>
<TableCell>
{(hookFire.result === 'SUCCESS' && (
<Link to={`/tasks/${hookFire.taskId}`}>
<Typography>{hookFire.taskId}</Typography>
</Link>
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now it looks like:

screen shot 2019-02-07 at 1 22 09 pm

It should look like this:

screen shot 2019-02-07 at 1 23 11 pm

This will make it look like any other link in a table to the rest of the app. Here's an example: The Worker ID column in the Workers view.

Copy link
Author

Choose a reason for hiding this comment

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

If the lastfire result is success then the taskId links to another route. But if result is error then no task of the mentioned taskId is actually created hence there is no link to the corresponding task. So I'm underlinking taskId for the sucess ones and no underline for error ones to distinguish them. Shall I change it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

From https://github.com/taskcluster/taskcluster-web/pull/413/files#r254810845, we could remove the link icon when the task has an error.

)) || <Typography>{hookFire.taskId}</Typography>}
</TableCell>
<TableCell>
<Typography>{hookFire.firedBy}</Typography>
</TableCell>
<TableCell>
<Label mini status={hookFire.result.toLowerCase()}>
{hookFire.result}
</Label>
</TableCell>
<TableCell>
<DateDistance from={hookFire.taskCreateTime} />
</TableCell>
<TableCell className={classes.errorTableCell}>
{(hookFire.result === 'ERROR' && (
<ExpansionPanel>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a hook I can see where hookFire.result === 'ERROR'? I want to see how it looks in the UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having an expansion panel for each error, we should instead have a button that opens a drawer with the error on click. This is similar to https://taskcluster-web.netlify.com/provisioners/aws-provisioner-v1 when clicking the info icon. The drawer can display the ErrorPanel component.

screen shot 2019-02-11 at 10 59 46 am

<ExpansionPanelSummary
expandIcon={<ExpandMoreIcon />}>
<Typography className={classes.heading}>
Show/Hide
</Typography>
</ExpansionPanelSummary>
<ExpansionPanelDetails>
<ErrorPanel
className={classes.errorPanel}
error={hookFire.error}
onClose={null}
/>
</ExpansionPanelDetails>
</ExpansionPanel>
)) || <Typography>N/A</Typography>}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use lower case n/a.

</TableCell>
</TableRow>
)}
/>
)}
</ListItem>
{!isNewHook && (
<Fragment>
<ListItem>
<ListItemText
primary="Last Fired"
secondary={
lastFireTypeName === HOOKS_LAST_FIRE_TYPE.NO_FIRE ? (
'n/a'
) : (
<DateDistance from={hook.status.lastFire.time} />
)
}
/>
</ListItem>
{lastFireTypeName === HOOKS_LAST_FIRE_TYPE.SUCCESSFUL_FIRE ? (
<ListItem
button
component={Link}
className={classes.listItemButton}
to={`/tasks/${hook.status.lastFire.taskId}`}>
<ListItemText
primary="Last Fired Result"
secondary={hook.status.lastFire.taskId}
/>
<LinkIcon />
</ListItem>
) : (
<ListItem>
<ListItemText
primary="Last Fired Result"
secondary={
lastFireTypeName === HOOKS_LAST_FIRE_TYPE.NO_FIRE ? (
'n/a'
) : (
<pre>
{JSON.stringify(hook.status.lastFire.error, null, 2)}
</pre>
)
}
/>
</ListItem>
)}
<ListItem>
<ListItemText
primary="Next Scheduled Fire"
secondary={
hook.status.nextScheduledDate ? (
<DateDistance from={hook.status.nextScheduledDate} />
) : (
'n/a'
)
}
/>
</ListItem>
</Fragment>
<ListItem>
<ListItemText
primary="Next Scheduled Fire"
secondary={
hook.status.nextScheduledDate ? (
<DateDistance from={hook.status.nextScheduledDate} />
) : (
'n/a'
)
}
/>
</ListItem>
)}
<List>
<ListItem>
Expand Down
7 changes: 7 additions & 0 deletions src/views/Hooks/ViewHook/hook.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,12 @@ query Hook($hookGroupId: ID!, $hookId: ID!) {
tags
extra
}
},
hookLastFires(hookGroupId: $hookGroupId, hookId: $hookId) {
taskId
firedBy
taskCreateTime
result
error
}
}
4 changes: 4 additions & 0 deletions src/views/Hooks/ViewHook/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@ export default class ViewHook extends Component {
dialogError={dialogError}
actionLoading={actionLoading}
hook={data.hook}
hookLastFires={data.hookLastFires.sort(
(a, b) =>
new Date(b.taskCreateTime) - new Date(a.taskCreateTime)
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Extract logic from jsx:

const hookLastFires = // ...

// ...
hookLastFires={hookLastFires}

dialogOpen={dialogOpen}
onTriggerHook={this.handleTriggerHook}
onUpdateHook={this.handleUpdateHook}
Expand Down