From 116b7e9b8eae95ce5aae0194b1fedf60802539e0 Mon Sep 17 00:00:00 2001 From: Felix Edel Date: Mon, 28 Sep 2020 14:22:05 +0200 Subject: [PATCH] Improve fetch build actions and state-to-props handling on BuildPage This removes some unused parameters from the various fetchBuild...() actions, introduces async/await to avoid function chaining and improves the state-to-props handling on the BuildPage. The last one is done by using the mapStateToProps() function which allows us to explicitly map the parts of the redux state in which we are interested to properties of our React component (BuildPage). This way we keep the mapping in a single place and don't need to "unpack" the redux state in various places in our component. In addition we use mapDispatchToProps to automatically wrap all actions we use in our React component into a dispatch call. This change also fixe a bug when trying to call substr() on a missing log_url. Additionally, a missing log url and a missing manifest file are no longer treated as failure to avoid showing toast notifications for those. This mainly applies to noop builds, which by definition don't provide any log at all. Change-Id: I056448b8735ac4ed5d271a7f55ca558fe9bf317f --- web/src/actions/build.js | 124 +++++++++++++++++++++++++++------------ web/src/pages/Build.jsx | 75 +++++++++++++++-------- 2 files changed, 135 insertions(+), 64 deletions(-) diff --git a/web/src/actions/build.js b/web/src/actions/build.js index 41a5b1b3b1..9b810e6aea 100644 --- a/web/src/actions/build.js +++ b/web/src/actions/build.js @@ -27,10 +27,12 @@ export const BUILDSET_FETCH_FAIL = 'BUILDSET_FETCH_FAIL' export const BUILD_OUTPUT_REQUEST = 'BUILD_OUTPUT_FETCH_REQUEST' export const BUILD_OUTPUT_SUCCESS = 'BUILD_OUTPUT_FETCH_SUCCESS' export const BUILD_OUTPUT_FAIL = 'BUILD_OUTPUT_FETCH_FAIL' +export const BUILD_OUTPUT_NOT_AVAILABLE = 'BUILD_OUTPUT_NOT_AVAILABLE' export const BUILD_MANIFEST_REQUEST = 'BUILD_MANIFEST_FETCH_REQUEST' export const BUILD_MANIFEST_SUCCESS = 'BUILD_MANIFEST_FETCH_SUCCESS' export const BUILD_MANIFEST_FAIL = 'BUILD_MANIFEST_FETCH_FAIL' +export const BUILD_MANIFEST_NOT_AVAILBLE = 'BUILD_MANIFEST_NOT_AVAILABLE' export const requestBuild = () => ({ type: BUILD_FETCH_REQUEST @@ -249,46 +251,81 @@ const failedBuildManifest = (error, url) => { } } -export const fetchBuild = (tenant, buildId, state, force) => dispatch => { - const build = state.build.builds[buildId] - if (!force && build) { - return Promise.resolve() - } - dispatch(requestBuild()) - return API.fetchBuild(tenant.apiPrefix, buildId) - .then(response => { - dispatch(receiveBuild(buildId, response.data)) - }) - .catch(error => dispatch(failedBuild(error, tenant.apiPrefix))) +function buildOutputNotAvailable() { + return { type: BUILD_OUTPUT_NOT_AVAILABLE } } -const fetchBuildOutput = (buildId, state, force) => dispatch => { - const build = state.build.builds[buildId] - const url = build.log_url.substr(0, build.log_url.lastIndexOf('/') + 1) - if (!force && build.output) { - return Promise.resolve() +function buildManifestNotAvailable() { + return { type: BUILD_MANIFEST_NOT_AVAILBLE } +} + +export function fetchBuild(tenant, buildId, state) { + return async function (dispatch) { + // Although it feels a little weird to not do anything in an action creator + // based on the redux state, we do this in here because the function is + // called from multiple places and it's easier to check for the build in + // here than in all the other places before calling this function. + if (state.build.builds[buildId]) { + return Promise.resolve() + } + + dispatch(requestBuild()) + try { + const response = await API.fetchBuild(tenant.apiPrefix, buildId) + dispatch(receiveBuild(buildId, response.data)) + } catch (error) { + dispatch(failedBuild(error, tenant.apiPrefix)) + // Raise the error again, so fetchBuildAllInfo() doesn't call the + // remaining fetch methods. + throw error + } } - dispatch(requestBuildOutput()) - return Axios.get(url + 'job-output.json.gz') - .then(response => dispatch(receiveBuildOutput(buildId, response.data))) - .catch(error => { +} + +function fetchBuildOutput(buildId, state) { + return async function (dispatch) { + // As this function is only called after fetchBuild() we can assume that + // the build is in the state. Otherwise an error would have been thrown and + // this function wouldn't be called. + const build = state.build.builds[buildId] + if (!build.log_url) { + // Don't treat a missing log URL as failure as we don't want to show a + // toast for that. The UI already informs about the missing log URL in + // multiple places. + dispatch(buildOutputNotAvailable()) + return Promise.resolve() + } + const url = build.log_url.substr(0, build.log_url.lastIndexOf('/') + 1) + dispatch(requestBuildOutput()) + try { + const response = await Axios.get(url + 'job-output.json.gz') + dispatch(receiveBuildOutput(buildId, response.data)) + } catch (error) { if (!error.request) { + dispatch(failedBuildOutput(error, url)) + // Raise the error again, so fetchBuildAllInfo() doesn't call the + // remaining fetch methods. throw error } - // Try without compression - Axios.get(url + 'job-output.json') - .then(response => dispatch(receiveBuildOutput( - buildId, response.data))) - }) - .catch(error => dispatch(failedBuildOutput(error, url))) + try { + // Try without compression + const response = await Axios.get(url + 'job-output.json') + dispatch(receiveBuildOutput(buildId, response.data)) + } catch (error) { + dispatch(failedBuildOutput(error, url)) + // Raise the error again, so fetchBuildAllInfo() doesn't call the + // remaining fetch methods. + throw error + } + } + } } -export const fetchBuildManifest = (buildId, state, force) => dispatch => { +export const fetchBuildManifest = (buildId, state) => (dispatch) => { + // As this function is only called after fetchBuild() we can assume that + // the build is in the state. Otherwise an error would have been thrown and + // this function wouldn't be called. const build = state.build.builds[buildId] - if (!force && build.manifest) { - return Promise.resolve() - } - dispatch(requestBuildManifest()) for (let artifact of build.artifacts) { if ('metadata' in artifact && @@ -301,15 +338,26 @@ export const fetchBuildManifest = (buildId, state, force) => dispatch => { .catch(error => dispatch(failedBuildManifest(error, artifact.url))) } } - dispatch(failedBuildManifest('no manifest found')) + // Don't treat a missing manifest file as failure as we don't want to show a + // toast for that. + dispatch(buildManifestNotAvailable()) } -export const fetchBuildIfNeeded = (tenant, buildId, force) => (dispatch, getState) => { - dispatch(fetchBuild(tenant, buildId, getState(), force)) - .then(() => { - dispatch(fetchBuildOutput(buildId, getState(), force)) - dispatch(fetchBuildManifest(buildId, getState(), force)) - }) +export function fetchBuildAllInfo(tenant, buildId) { + // This wraps the calls to fetch the build, output and manifest together as + // this is the common use case we have when loading the build info. + return async function (dispatch, getState) { + try { + // Wait for the build info to be available and provide the current status + // to the fetchBuildOutput and fetchBuildManifest so they can get the log + // url from the fetched build. + await dispatch(fetchBuild(tenant, buildId, getState())) + dispatch(fetchBuildOutput(buildId, getState())) + dispatch(fetchBuildManifest(buildId, getState())) + } catch (error) { + dispatch(failedBuild(error, tenant.apiPrefix)) + } + } } export const requestBuildset = () => ({ diff --git a/web/src/pages/Build.jsx b/web/src/pages/Build.jsx index 3ac98a4620..fcd9c2c953 100644 --- a/web/src/pages/Build.jsx +++ b/web/src/pages/Build.jsx @@ -36,7 +36,7 @@ import { PollIcon, } from '@patternfly/react-icons' -import { fetchBuildIfNeeded } from '../actions/build' +import { fetchBuildAllInfo } from '../actions/build' import { EmptyPage } from '../containers/Errors' import { Fetchable, Fetching } from '../containers/Fetching' import ArtifactList from '../containers/build/Artifact' @@ -48,22 +48,24 @@ import Manifest from '../containers/build/Manifest' class BuildPage extends React.Component { static propTypes = { match: PropTypes.object.isRequired, - remoteData: PropTypes.object, - tenant: PropTypes.object, - dispatch: PropTypes.func, + build: PropTypes.object, + isFetching: PropTypes.bool.isRequired, + isFetchingManifest: PropTypes.bool.isRequired, + isFetchingOutput: PropTypes.bool.isRequired, + tenant: PropTypes.object.isRequired, + fetchBuildAllInfo: PropTypes.func.isRequired, activeTab: PropTypes.string.isRequired, - location: PropTypes.object, - history: PropTypes.object, + location: PropTypes.object.isRequired, + history: PropTypes.object.isRequired, } - updateData = (force) => { - this.props.dispatch( - fetchBuildIfNeeded( + updateData = () => { + if (!this.props.build) { + this.props.fetchBuildAllInfo( this.props.tenant, - this.props.match.params.buildId, - force + this.props.match.params.buildId ) - ) + } } componentDidMount() { @@ -107,11 +109,18 @@ class BuildPage extends React.Component { } render() { - const { remoteData, activeTab, location, tenant } = this.props - const build = remoteData.builds[this.props.match.params.buildId] + const { + build, + isFetching, + isFetchingManifest, + isFetchingOutput, + activeTab, + location, + tenant, + } = this.props const hash = location.hash.substring(1).split('/') - if (!build && remoteData.isFetching) { + if (!build && isFetching) { return } @@ -127,14 +136,11 @@ class BuildPage extends React.Component { } const fetchable = ( - + ) const resultsTabContent = - !build.hosts && remoteData.isFetchingOutput ? ( + !build.hosts && isFetchingOutput ? ( ) : build.hosts ? ( @@ -159,7 +165,7 @@ class BuildPage extends React.Component { ) const logsTabContent = - !build.manifest && remoteData.isFetchingManifest ? ( + !build.manifest && isFetchingManifest ? ( ) : build.manifest ? ( @@ -173,7 +179,7 @@ class BuildPage extends React.Component { ) const consoleTabContent = - !build.output && remoteData.isFetchingOutput ? ( + !build.output && isFetchingOutput ? ( ) : build.output ? ( ({ - tenant: state.tenant, - remoteData: state.build, -}))(withRouter(BuildPage)) +function mapStateToProps(state, ownProps) { + const buildId = ownProps.match.params.buildId + const build = + buildId && Object.keys(state.build.builds).length > 0 + ? state.build.builds[buildId] + : null + return { + build, + tenant: state.tenant, + isFetching: state.build.isFetching, + isFetchingManifest: state.build.isFetchingManifest, + isFetchingOutput: state.build.isFetchingOutput, + } +} + +const mapDispatchToProps = { fetchBuildAllInfo } + +export default connect( + mapStateToProps, + mapDispatchToProps +)(withRouter(BuildPage))