Merge "Improve fetch build actions and state-to-props handling on BuildPage"

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 => {
+function buildOutputNotAvailable() {
-  const build = state.build.builds[buildId]
+  return { type: BUILD_OUTPUT_NOT_AVAILABLE }
-  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)))
 }
 
-const fetchBuildOutput = (buildId, state, force) => dispatch => {
+function buildManifestNotAvailable() {
-  const build = state.build.builds[buildId]
+  return { type: BUILD_MANIFEST_NOT_AVAILBLE }
-  const url = build.log_url.substr(0, build.log_url.lastIndexOf('/') + 1)
+}
-  if (!force && build.output) {
+
-    return Promise.resolve()
+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)))
+function fetchBuildOutput(buildId, state) {
-    .catch(error => {
+  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
+      try {
-      Axios.get(url + 'job-output.json')
+        // Try without compression
-        .then(response => dispatch(receiveBuildOutput(
+        const response = await Axios.get(url + 'job-output.json')
-          buildId, response.data)))
+        dispatch(receiveBuildOutput(buildId, response.data))
-    })
+      } catch (error) {
-    .catch(error => dispatch(failedBuildOutput(error, url)))
+        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) => {
+export function fetchBuildAllInfo(tenant, buildId) {
-  dispatch(fetchBuild(tenant, buildId, getState(), force))
+  // This wraps the calls to fetch the build, output and manifest together as
-    .then(() => {
+  // this is the common use case we have when loading the build info.
-      dispatch(fetchBuildOutput(buildId, getState(), force))
+  return async function (dispatch, getState) {
-      dispatch(fetchBuildManifest(buildId, getState(), force))
+    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 = () => ({

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,
+    build: PropTypes.object,
-    tenant: PropTypes.object,
+    isFetching: PropTypes.bool.isRequired,
-    dispatch: PropTypes.func,
+    isFetchingManifest: PropTypes.bool.isRequired,
+    isFetchingOutput: PropTypes.bool.isRequired,
+    tenant: PropTypes.object.isRequired,
+    fetchBuildAllInfo: PropTypes.func.isRequired,
     activeTab: PropTypes.string.isRequired,
-    location: PropTypes.object,
+    location: PropTypes.object.isRequired,
-    history: PropTypes.object,
+    history: PropTypes.object.isRequired,
   }
 
-  updateData = (force) => {
+  updateData = () => {
-    this.props.dispatch(
+    if (!this.props.build) {
-      fetchBuildIfNeeded(
+      this.props.fetchBuildAllInfo(
         this.props.tenant,
-        this.props.match.params.buildId,
+        this.props.match.params.buildId
-        force
       )
-    )
+    }
   }
 
   componentDidMount() {
@@ -107,11 +109,18 @@ class BuildPage extends React.Component {
   }
 
   render() {
-    const { remoteData, activeTab, location, tenant } = this.props
+    const {
-    const build = remoteData.builds[this.props.match.params.buildId]
+      build,
+      isFetching,
+      isFetchingManifest,
+      isFetchingOutput,
+      activeTab,
+      location,
+      tenant,
+    } = this.props
     const hash = location.hash.substring(1).split('/')
 
-    if (!build && remoteData.isFetching) {
+    if (!build && isFetching) {
       return <Fetching />
     }
 
@@ -127,14 +136,11 @@ class BuildPage extends React.Component {
     }
 
     const fetchable = (
-      <Fetchable
+      <Fetchable isFetching={isFetching} fetchCallback={this.updateData} />
-        isFetching={remoteData.isFetching}
-        fetchCallback={this.updateData}
-      />
     )
 
     const resultsTabContent =
-      !build.hosts && remoteData.isFetchingOutput ? (
+      !build.hosts && isFetchingOutput ? (
         <Fetching />
       ) : build.hosts ? (
         <BuildOutput output={build.hosts} />
@@ -159,7 +165,7 @@ class BuildPage extends React.Component {
     )
 
     const logsTabContent =
-      !build.manifest && remoteData.isFetchingManifest ? (
+      !build.manifest && isFetchingManifest ? (
         <Fetching />
       ) : build.manifest ? (
         <Manifest tenant={this.props.tenant} build={build} />
@@ -173,7 +179,7 @@ class BuildPage extends React.Component {
       )
 
     const consoleTabContent =
-      !build.output && remoteData.isFetchingOutput ? (
+      !build.output && isFetchingOutput ? (
         <Fetching />
       ) : build.output ? (
         <Console
@@ -265,7 +271,24 @@ class BuildPage extends React.Component {
   }
 }
 
-export default connect((state) => ({
+function mapStateToProps(state, ownProps) {
-  tenant: state.tenant,
+  const buildId = ownProps.match.params.buildId
-  remoteData: state.build,
+  const build =
-}))(withRouter(BuildPage))
+    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))