From 21abefd0fb6959686e466233168aa6515ff06863 Mon Sep 17 00:00:00 2001
From: Joe Farro <joef@uber.com>
Date: Fri, 27 Oct 2017 06:06:08 -0400
Subject: [PATCH] Verify stored search values and preload operations

Fix #88.

Preload operations for stored service search value.

On the search page, show a loading indicator if the services are not yet loaded.
Then, only use the values stored in localStorage if they are consistent with
what has been loaded.

Signed-off-by: Joe Farro <joef@uber.com>
---
 .../SearchTracePage/TraceSearchForm.js        | 16 +++-
 src/components/SearchTracePage/index.js       | 81 ++++++++++++-------
 src/reducers/services.js                      |  5 +-
 src/reducers/services.test.js                 | 19 ++---
 4 files changed, 77 insertions(+), 44 deletions(-)

diff --git a/src/components/SearchTracePage/TraceSearchForm.js b/src/components/SearchTracePage/TraceSearchForm.js
index 7f59a5dea6..13eb635bf1 100644
--- a/src/components/SearchTracePage/TraceSearchForm.js
+++ b/src/components/SearchTracePage/TraceSearchForm.js
@@ -223,8 +223,19 @@ const mapStateToProps = state => {
   let lastSearchOperation;
 
   if (lastSearch) {
-    lastSearchService = lastSearch.service;
-    lastSearchOperation = lastSearch.operation;
+    // last search is only valid if the service is in the list of services
+    const { operation: lastOp, service: lastSvc } = lastSearch;
+    if (lastSvc && lastSvc !== '-') {
+      if (state.services.services.includes(lastSvc)) {
+        lastSearchService = lastSvc;
+        if (lastOp && lastOp !== '-') {
+          const ops = state.services.operationsForService[lastSvc];
+          if (lastOp === 'all' || (ops && ops.includes(lastOp))) {
+            lastSearchOperation = lastOp;
+          }
+        }
+      }
+    }
   }
 
   const {
@@ -242,6 +253,7 @@ const mapStateToProps = state => {
   if (traceIDParams) {
     traceIDs = traceIDParams instanceof Array ? traceIDParams.join(',') : traceIDParams;
   }
+
   return {
     destroyOnUnmount: false,
     initialValues: {
diff --git a/src/components/SearchTracePage/index.js b/src/components/SearchTracePage/index.js
index b55dc1cf1e..65dc3ff065 100644
--- a/src/components/SearchTracePage/index.js
+++ b/src/components/SearchTracePage/index.js
@@ -12,14 +12,15 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+import React, { Component } from 'react';
 import _values from 'lodash/values';
 import PropTypes from 'prop-types';
-import React, { Component } from 'react';
 import queryString from 'query-string';
-import { bindActionCreators } from 'redux';
 import { connect } from 'react-redux';
-import { Field, reduxForm, formValueSelector } from 'redux-form';
 import { Link } from 'react-router-dom';
+import { bindActionCreators } from 'redux';
+import { Field, reduxForm, formValueSelector } from 'redux-form';
+import store from 'store';
 
 import JaegerLogo from '../../img/jaeger-logo.svg';
 
@@ -61,21 +62,27 @@ const traceResultsFiltersFormSelector = formValueSelector('traceResultsFilters')
 
 export default class SearchTracePage extends Component {
   componentDidMount() {
-    const { searchTraces, urlQueryParams, fetchServices } = this.props;
+    const { searchTraces, urlQueryParams, fetchServices, fetchServiceOperations } = this.props;
     if (urlQueryParams.service || urlQueryParams.traceID) {
       searchTraces(urlQueryParams);
     }
     fetchServices();
+    const { service } = store.get('lastSearch') || {};
+    if (service && service !== '-') {
+      fetchServiceOperations(service);
+    }
   }
+
   render() {
     const {
-      traceResults,
-      services,
-      numberOfTraceResults,
-      maxTraceDuration,
-      loading,
       errorMessage,
       isHomepage,
+      loadingServices,
+      loadingTraces,
+      maxTraceDuration,
+      numberOfTraceResults,
+      services,
+      traceResults,
     } = this.props;
     const hasTraceResults = traceResults && traceResults.length > 0;
     return (
@@ -83,13 +90,17 @@ export default class SearchTracePage extends Component {
         <div className="four wide column">
           <div className="ui tertiary segment" style={{ background: 'whitesmoke' }}>
             <h3>Find Traces</h3>
-            <TraceSearchForm services={services} />
+            {!loadingServices && services
+              ? <TraceSearchForm services={services} />
+              : <div className="m1">
+                  <div className="ui active centered inline loader" />
+                </div>}
           </div>
         </div>
         <div className="twelve wide column padded">
-          {loading && <div className="ui active centered inline loader" />}
+          {loadingTraces && <div className="ui active centered inline loader" />}
           {errorMessage &&
-            !loading &&
+            !loadingTraces &&
             <div className="ui message red trace-search--error">
               There was an error querying for traces:<br />
               {errorMessage}
@@ -103,11 +114,11 @@ export default class SearchTracePage extends Component {
             </div>}
           {!isHomepage &&
             !hasTraceResults &&
-            !loading &&
+            !loadingTraces &&
             !errorMessage &&
             <div className="ui message trace-search--no-results">No trace results. Try another query.</div>}
           {hasTraceResults &&
-            !loading &&
+            !loadingTraces &&
             <div>
               <div>
                 <div style={{ border: '1px solid #e6e6e6' }}>
@@ -165,7 +176,8 @@ SearchTracePage.propTypes = {
   traceResults: PropTypes.array,
   numberOfTraceResults: PropTypes.number,
   maxTraceDuration: PropTypes.number,
-  loading: PropTypes.bool,
+  loadingServices: PropTypes.bool,
+  loadingTraces: PropTypes.bool,
   urlQueryParams: PropTypes.shape({
     service: PropTypes.string,
     limit: PropTypes.string,
@@ -180,30 +192,38 @@ SearchTracePage.propTypes = {
   history: PropTypes.shape({
     push: PropTypes.func,
   }),
+  fetchServiceOperations: PropTypes.func,
   fetchServices: PropTypes.func,
   errorMessage: PropTypes.string,
 };
 
 const stateTraceXformer = getLastXformCacher(stateTrace => {
-  const { traces: traceMap, loading, error: traceError } = stateTrace;
+  const { traces: traceMap, loading: loadingTraces, error: traceError } = stateTrace;
   const { traces, maxDuration } = getTraceSummaries(_values(traceMap));
-  return { traces, maxDuration, loading, traceError };
+  return { traces, maxDuration, traceError, loadingTraces };
 });
 
 const stateServicesXformer = getLastXformCacher(stateServices => {
-  const { services: serviceList, operationsForService: opsBySvc, error: serviceError } = stateServices;
-  const services = serviceList.map(name => ({
-    name,
-    operations: opsBySvc[name] || [],
-  }));
-  return { services, serviceError };
+  const {
+    loading: loadingServices,
+    services: serviceList,
+    operationsForService: opsBySvc,
+    error: serviceError,
+  } = stateServices;
+  const services =
+    serviceList &&
+    serviceList.map(name => ({
+      name,
+      operations: opsBySvc[name] || [],
+    }));
+  return { loadingServices, services, serviceError };
 });
 
 function mapStateToProps(state) {
   const query = queryString.parse(state.routing.location.search);
   const isHomepage = !Object.keys(query).length;
-  const { traces, maxDuration, loading, traceError } = stateTraceXformer(state.trace);
-  const { services, serviceError } = stateServicesXformer(state.services);
+  const { traces, maxDuration, traceError, loadingTraces } = stateTraceXformer(state.trace);
+  const { loadingServices, services, serviceError } = stateServicesXformer(state.services);
   const errorMessage = serviceError || traceError ? `${serviceError || ''} ${traceError || ''}` : '';
   const sortBy = traceResultsFiltersFormSelector(state, 'sortBy');
   sortTraces(traces, sortBy);
@@ -216,16 +236,21 @@ function mapStateToProps(state) {
     maxTraceDuration: maxDuration,
     urlQueryParams: query,
     services,
-    loading,
+    loadingTraces,
+    loadingServices,
     errorMessage,
   };
 }
 
 function mapDispatchToProps(dispatch) {
-  const { searchTraces, fetchServices } = bindActionCreators(jaegerApiActions, dispatch);
+  const { searchTraces, fetchServices, fetchServiceOperations } = bindActionCreators(
+    jaegerApiActions,
+    dispatch
+  );
   return {
-    searchTraces,
+    fetchServiceOperations,
     fetchServices,
+    searchTraces,
   };
 }
 export const ConnectedSearchTracePage = connect(mapStateToProps, mapDispatchToProps)(SearchTracePage);
diff --git a/src/reducers/services.js b/src/reducers/services.js
index 3d44ee3587..a1e60d150c 100644
--- a/src/reducers/services.js
+++ b/src/reducers/services.js
@@ -18,7 +18,8 @@ import { fetchServices, fetchServiceOperations as fetchOps } from '../actions/ja
 import { baseStringComparator } from '../utils/sort';
 
 const initialState = {
-  services: [],
+  // `services` initial value of `null` indicates they haven't yet been loaded
+  services: null,
   operationsForService: {},
   loading: false,
   error: null,
@@ -48,7 +49,7 @@ function fetchOpsDone(state, { meta, payload }) {
   if (Array.isArray(operations)) {
     operations.sort(baseStringComparator);
   }
-  const operationsForService = { ...state.operationsForService, [meta.serviceName]: operations };
+  const operationsForService = { ...state.operationsForService, [meta.serviceName]: operations || [] };
   return { ...state, operationsForService };
 }
 
diff --git a/src/reducers/services.test.js b/src/reducers/services.test.js
index d2f49bcb62..92e8c37eb4 100644
--- a/src/reducers/services.test.js
+++ b/src/reducers/services.test.js
@@ -19,7 +19,7 @@ const initialState = serviceReducer(undefined, {});
 
 function verifyInitialState() {
   expect(initialState).toEqual({
-    services: [],
+    services: null,
     loading: false,
     error: null,
     operationsForService: {},
@@ -47,12 +47,8 @@ it('should handle a fetch services with loading state', () => {
   const state = serviceReducer(initialState, {
     type: `${fetchServices}_PENDING`,
   });
-  expect(state).toEqual({
-    services: [],
-    operationsForService: {},
-    loading: true,
-    error: null,
-  });
+  const expected = { ...initialState, loading: true };
+  expect(state).toEqual(expected);
 });
 
 it('should handle successful services fetch', () => {
@@ -90,12 +86,11 @@ it('should handle a successful fetching operations for a service ', () => {
     meta: { serviceName: 'serviceA' },
     payload: { data: ops.slice() },
   });
-  expect(state).toEqual({
-    services: [],
+  const expected = {
+    ...initialState,
     operationsForService: {
       serviceA: ops,
     },
-    loading: false,
-    error: null,
-  });
+  };
+  expect(state).toEqual(expected);
 });