Skip to content

Commit

Permalink
Merge pull request #1179 from HubSpot/handle_404_properly
Browse files Browse the repository at this point in the history
Handle 404 properly
  • Loading branch information
Calvinp authored Aug 2, 2016
2 parents 070836d + 7c74846 commit 373ac53
Show file tree
Hide file tree
Showing 9 changed files with 58 additions and 23 deletions.
2 changes: 1 addition & 1 deletion SingularityUI/app/actions/api/base.es6
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export function buildApiAction(actionName, opts = {}, keyFunc = undefined) {

function error(err, options, apiResponse, key = undefined) {
const action = { type: ERROR, error: err, key, statusCode: apiResponse.status };
if (Utils.isIn(apiResponse.status, options.catchStatusCodes) || apiResponse.status === 404 && options.isMainApiCall) {
if (Utils.isIn(apiResponse.status, options.catchStatusCodes) || apiResponse.status === 404 && options.renderNotFoundIf404) {
return action;
}
if (apiResponse.status === 502) { // Singularity is deploying
Expand Down
9 changes: 5 additions & 4 deletions SingularityUI/app/actions/api/history.es6
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ import Utils from '../../utils';

export const FetchTaskHistory = buildApiAction(
'FETCH_TASK_HISTORY',
(taskId) => ({
url: `/history/task/${taskId}`
(taskId, renderNotFoundIf404) => ({
url: `/history/task/${taskId}`,
renderNotFoundIf404
}),
(taskId) => taskId
);
Expand Down Expand Up @@ -41,9 +42,9 @@ export const FetchTaskHistoryForDeploy = buildApiAction(

export const FetchDeployForRequest = buildApiAction(
'FETCH_DEPLOY',
(requestId, deployId, isMainApiCall) => ({
(requestId, deployId, renderNotFoundIf404) => ({
url: `/history/request/${requestId}/deploy/${deployId}`,
isMainApiCall
renderNotFoundIf404
})
);

Expand Down
5 changes: 3 additions & 2 deletions SingularityUI/app/actions/api/requests.es6
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ export const FetchRequestsInState = buildApiAction(

export const FetchRequest = buildApiAction(
'FETCH_REQUEST',
(requestId) => ({
url: `/requests/request/${requestId}`
(requestId, renderNotFoundIf404) => ({
url: `/requests/request/${requestId}`,
renderNotFoundIf404
}),
(requestId) => requestId
);
Expand Down
9 changes: 3 additions & 6 deletions SingularityUI/app/components/deployDetail/DeployDetail.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import JSONButton from '../common/JSONButton';
import UITable from '../common/table/UITable';
import Column from '../common/table/Column';
import CollapsableSection from '../common/CollapsableSection';
import NotFound from '../common/NotFound';

import ActiveTasksTable from './ActiveTasksTable';

Expand Down Expand Up @@ -307,10 +306,7 @@ class DeployDetail extends React.Component {
}

render() {
const { notFound, deploy, activeTasks, taskHistory, latestHealthchecks } = this.props;
if (notFound) {
return <NotFound location={{pathname: this.props.location.pathname}} />;
}
const { deploy, activeTasks, taskHistory, latestHealthchecks } = this.props;
return (
<div>
{this.renderHeader(deploy)}
Expand All @@ -333,7 +329,7 @@ function mapDispatchToProps(dispatch) {
};
}

function mapStateToProps(state) {
function mapStateToProps(state, ownProps) {
let latestHealthchecks = _.mapObject(state.api.task, (val) => {
if (val.data && val.data.healthcheckResults && val.data.healthcheckResults.length > 0) {
return _.max(val.data.healthcheckResults, (hc) => {
Expand All @@ -346,6 +342,7 @@ function mapStateToProps(state) {

return {
notFound: state.api.deploy.statusCode === 404,
pathname: ownProps.location.pathname,
deploy: state.api.deploy.data,
taskHistory: state.api.taskHistoryForDeploy.data,
isTaskHistoryFetching: state.api.taskHistoryForDeploy.isFetching,
Expand Down
4 changes: 3 additions & 1 deletion SingularityUI/app/components/newDeployForm/NewDeployForm.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -1446,6 +1446,8 @@ class NewDeployForm extends Component {
function mapStateToProps(state, ownProps) {
return {
request: Utils.maybe(state.api.request, [ownProps.params.requestId, 'data']),
notFound: Utils.maybe(state.api.request, [ownProps.params.requestId, 'statusCode']) === 404,
pathname: ownProps.location.pathname,
form: state.ui.form[FORM_ID],
saveApiCall: state.api.saveDeploy
};
Expand All @@ -1464,7 +1466,7 @@ function mapDispatchToProps(dispatch, ownProps) {
});
},
fetchRequest(requestId) {
return dispatch(FetchRequest.trigger(requestId));
return dispatch(FetchRequest.trigger(requestId, true));
},
clearForm() {
return dispatch(ClearForm('newDeployForm'));
Expand Down
14 changes: 12 additions & 2 deletions SingularityUI/app/components/requestDetail/RequestDetailPage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import TaskHistoryTable from './TaskHistoryTable';
import DeployHistoryTable from './DeployHistoryTable';
import RequestHistoryTable from './RequestHistoryTable';

import Utils from '../../utils';

function refresh(props) {
props.fetchRequest(props.params.requestId);
props.fetchActiveTasksForRequest(props.params.requestId);
Expand Down Expand Up @@ -71,6 +73,14 @@ RequestDetailPage.propTypes = {
cancelRefresh: PropTypes.func.isRequired
};

const mapStateToProps = (state, ownProps) => {
const statusCode = Utils.maybe(state, ['api', 'request', ownProps.params.requestId, 'statusCode']);
return {
notFound: statusCode === 404,
pathname: ownProps.location.pathname
};
};

const mapDispatchToProps = (dispatch, ownProps) => {
const refreshActions = [
FetchRequest.trigger(ownProps.params.requestId),
Expand All @@ -87,7 +97,7 @@ const mapDispatchToProps = (dispatch, ownProps) => {
cancelRefresh: () => dispatch(
RefreshActions.CancelAutoRefresh('RequestDetailPage')
),
fetchRequest: (requestId) => dispatch(FetchRequest.trigger(requestId)),
fetchRequest: (requestId) => dispatch(FetchRequest.trigger(requestId, true)),
fetchActiveTasksForRequest: (requestId) => dispatch(FetchActiveTasksForRequest.trigger(requestId)),
fetchScheduledTasksForRequest: (requestId) => dispatch(FetchScheduledTasksForRequest.trigger(requestId)),
fetchTaskCleanups: () => dispatch(FetchTaskCleanups.trigger()),
Expand All @@ -98,6 +108,6 @@ const mapDispatchToProps = (dispatch, ownProps) => {
};

export default connect(
null,
mapStateToProps,
mapDispatchToProps
)(rootComponent(RequestDetailPage, (props) => props.params.requestId, refresh, false));
4 changes: 3 additions & 1 deletion SingularityUI/app/components/requestForm/RequestForm.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,8 @@ class RequestForm extends React.Component {
function mapStateToProps(state, ownProps) {
const request = ownProps.params.requestId && state.api.request[ownProps.params.requestId];
return {
notFound: request && request.statusCode === 404,
pathname: ownProps.location.pathname,
racks: state.api.racks.data,
request: request && request.data,
form: state.ui.form[FORM_ID],
Expand All @@ -525,7 +527,7 @@ function mapDispatchToProps(dispatch, ownProps) {
});
},
fetchRequest(requestId) {
dispatch(FetchRequest.trigger(requestId));
dispatch(FetchRequest.trigger(requestId, true));
},
fetchRacks() {
dispatch(FetchRacks.trigger());
Expand Down
17 changes: 13 additions & 4 deletions SingularityUI/app/components/taskDetail/TaskDetail.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -450,8 +450,15 @@ function mapTaskToProps(task) {
}

function mapStateToProps(state, ownProps) {
let task = state.api.task[ownProps.params.taskId];
const apiCallData = state.api.task[ownProps.params.taskId];
let task = apiCallData;
if (!(task && task.data)) return {};
if (apiCallData.statusCode === 404) {
return {
notFound: true,
pathname: ownProps.location.pathname
};
}
task = mapTaskToProps(task.data);
task = mapHealthchecksToProps(task);
return {
Expand All @@ -472,7 +479,7 @@ function mapDispatchToProps(dispatch) {
return {
runCommandOnTask: (taskId, commandName) => dispatch(RunCommandOnTask.trigger(taskId, commandName)),
killTask: (taskId, data) => dispatch(KillTask.trigger(taskId, data)),
fetchTaskHistory: (taskId) => dispatch(FetchTaskHistory.trigger(taskId)),
fetchTaskHistory: (taskId) => dispatch(FetchTaskHistory.trigger(taskId, true)),
fetchTaskStatistics: (taskId) => dispatch(FetchTaskStatistics.trigger(taskId)),
fetchTaskFiles: (taskId, path, catchStatusCodes = []) => dispatch(FetchTaskFiles.trigger(taskId, path, catchStatusCodes.concat([404]))),
fetchDeployForRequest: (taskId, deployId) => dispatch(FetchDeployForRequest.trigger(taskId, deployId)),
Expand All @@ -483,11 +490,13 @@ function mapDispatchToProps(dispatch) {
}

function refresh(props) {
props.fetchTaskFiles(props.params.taskId, props.params.splat || props.params.taskId, [400]);
props.fetchTaskFiles(props.params.taskId, props.params.splat || props.params.taskId, [400, 404]);
const promises = [];
const taskPromise = props.fetchTaskHistory(props.params.taskId);
taskPromise.then(() => {
const task = props.route.store.getState().api.task[props.params.taskId].data;
const apiData = props.route.store.getState().api.task[props.params.taskId];
if (apiData.statusCode === 404) return;
const task = apiData.data;
promises.push(props.fetchDeployForRequest(task.task.taskId.requestId, task.task.taskId.deployId));
if (task.isStillRunning) {
promises.push(props.fetchTaskStatistics(props.params.taskId));
Expand Down
17 changes: 15 additions & 2 deletions SingularityUI/app/rootComponent.jsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import React from 'react';
import React, { PropTypes, Component } from 'react';
import classNames from 'classnames';
import NotFound from 'components/common/NotFound';

const rootComponent = (Wrapped, title, refresh = _.noop, refreshInterval = true, pageMargin = true) => class extends React.Component {
const rootComponent = (Wrapped, title, refresh = _.noop, refreshInterval = true, pageMargin = true) => class extends Component {

static propTypes = {
notFound: PropTypes.bool,
pathname: PropTypes.string
}

constructor(props) {
super(props);
Expand Down Expand Up @@ -69,6 +75,13 @@ const rootComponent = (Wrapped, title, refresh = _.noop, refreshInterval = true,
}

render() {
if (this.props.notFound) {
return (
<div className={classNames({'page container-fluid': pageMargin})}>
<NotFound location={{pathname: this.props.pathname}} />
</div>
);
}
const loader = this.state.loading && <div className="page-loader fixed" />;
const page = !this.state.loading && <Wrapped {...this.props} />;
return (
Expand Down

0 comments on commit 373ac53

Please sign in to comment.