fix(ui): Change Issue details to not force global selection values [SEN-658] (#13554)

We save last selected projects + envs (+ dates) in local storage. High up in the component tree, we initialize our global selection store with these values from local storage. This means when we view issue details with a direct URL that does not contain a project or environment, it will load values from the store. So you could end up in a weird state if it tries to load last used environment.

This makes sure that 1) the component tree for GroupDetails prop drills `environments` so that it does not directly read from the store and
2) does not pass down the environments value from the store for the component that uses `withGlobalSelection` (controlled by a prop).

This will maintain the current behavior for components that do not specify the `disableLoadFromStore` prop on the `withGlobalSelection` HoC. So if you were to navigate from issue details back to issue stream, it will have values from store.

Another issue this fixes is that when GlobalSelectionHeader changed params, it would update ALL url params (e.g. projects, envs, dates) - which was not a problem before because these values would usually be synced, but because we initially do not use values from store, it is a bit out of sync so if you were to change environments, it would also incorrectly overwrite projects and dates as well.

Author: billyvg

src/sentry/static/sentry/app/components/organizations/globalSelectionHeader/index.jsx

@@ -50,6 +50,11 @@ class GlobalSelectionHeader extends React.Component {
      */
     forceProject: SentryTypes.Project,
 
+    /**
+     * If true, do not initially update URL with values from store
+     */
+    disableLoadFromStore: PropTypes.bool,
+
     /**
      * Currently selected values(s)
      */
@@ -109,8 +114,7 @@ class GlobalSelectionHeader extends React.Component {
     const hasMultipleProjectFeature = this.hasMultipleProjectSelection();
 
     const stateFromRouter = getStateFromQuery(location.query);
-    // We should update store if there are any relevant URL parameters when component
-    // is mounted
+    // We should update store if there are any relevant URL parameters when component is mounted
     if (Object.values(stateFromRouter).some(i => !!i)) {
       if (!stateFromRouter.start && !stateFromRouter.end && !stateFromRouter.period) {
         stateFromRouter.period = DEFAULT_STATS_PERIOD;
@@ -143,6 +147,10 @@ class GlobalSelectionHeader extends React.Component {
       // update URL parameters to reflect current store
       const {datetime, environments, projects} = selection;
 
+      if (this.props.disableLoadFromStore) {
+        return;
+      }
+
       if (hasMultipleProjectFeature || projects.length === 1) {
         updateParamsWithoutHistory(
           {project: projects, environment: environments, ...datetime},
@@ -166,7 +174,7 @@ class GlobalSelectionHeader extends React.Component {
     }
 
     // Update if URL parameters change
-    if (this.didQueryChange(this.props, nextProps)) {
+    if (this.changedQueryKeys(this.props, nextProps).length > 0) {
       return true;
     }
 
@@ -219,35 +227,57 @@ class GlobalSelectionHeader extends React.Component {
     return new Set(this.props.organization.features).has('global-views');
   };
 
-  didQueryChange = (prevProps, nextProps) => {
+  /**
+   * Identifies if query string has changed (with query params that this component cares about)
+   *
+   *
+   * @return {String[]} Returns `false` if did not change, otherwise return an array of params that have changed
+   */
+  changedQueryKeys = (prevProps, nextProps) => {
     const urlParamKeys = Object.values(URL_PARAM);
     const prevQuery = pick(prevProps.location.query, urlParamKeys);
     const nextQuery = pick(nextProps.location.query, urlParamKeys);
 
     // If no next query is specified keep the previous global selection values
     if (Object.keys(prevQuery).length === 0 && Object.keys(nextQuery).length === 0) {
-      return false;
+      return [];
     }
 
-    return !isEqual(prevQuery, nextQuery);
+    const changedKeys = Object.values(urlParamKeys).filter(
+      key => !isEqual(prevQuery[key], nextQuery[key])
+    );
+
+    return changedKeys;
   };
 
   updateStoreIfChange = (prevProps, nextProps) => {
     // Don't do anything if query parameters have not changed
     //
     // e.g. if selection store changed, don't trigger more actions
     // to update global selection store (otherwise we'll get recursive updates)
-    if (!this.didQueryChange(prevProps, nextProps)) {
+    const changedKeys = this.changedQueryKeys(prevProps, nextProps);
+
+    if (!changedKeys.length) {
       return;
     }
 
     const {project, environment, period, start, end, utc} = getStateFromQuery(
       nextProps.location.query
     );
 
-    updateDateTime({start, end, period, utc});
-    updateEnvironments(environment || []);
-    updateProjects(project || []);
+    if (changedKeys.includes(URL_PARAM.PROJECT)) {
+      updateProjects(project || []);
+    }
+    if (changedKeys.includes(URL_PARAM.ENVIRONMENT)) {
+      updateEnvironments(environment || []);
+    }
+    if (
+      [URL_PARAM.START, URL_PARAM.END, URL_PARAM.UTC, URL_PARAM.PERIOD].find(key =>
+        changedKeys.includes(key)
+      )
+    ) {
+      updateDateTime({start, end, period, utc});
+    }
   };
 
   // Returns `router` from props if `hasCustomRouting` property is false

src/sentry/static/sentry/app/utils/withGlobalSelection.jsx

@@ -1,9 +1,10 @@
+import PropTypes from 'prop-types';
 import React from 'react';
 import Reflux from 'reflux';
 import createReactClass from 'create-react-class';
 
-import getDisplayName from 'app/utils/getDisplayName';
 import GlobalSelectionStore from 'app/stores/globalSelectionStore';
+import getDisplayName from 'app/utils/getDisplayName';
 
 /**
  * Higher order component that uses GlobalSelectionStore and provides the
@@ -12,10 +13,17 @@ import GlobalSelectionStore from 'app/stores/globalSelectionStore';
 const withGlobalSelection = WrappedComponent =>
   createReactClass({
     displayName: `withGlobalSelection(${getDisplayName(WrappedComponent)})`,
+    propTypes: {
+      // Does not initially load values from the store
+      // However any following updates to store should work
+      disableLoadFromStore: PropTypes.bool,
+    },
     mixins: [Reflux.listenTo(GlobalSelectionStore, 'onUpdate')],
     getInitialState() {
       return {
-        selection: GlobalSelectionStore.get(),
+        selection: this.props.disableLoadFromStore
+          ? {projects: [], environments: [], datetime: {}}
+          : GlobalSelectionStore.get(),
       };
     },
 

src/sentry/static/sentry/app/views/organizationGroupDetails/groupDetails.jsx

@@ -33,6 +33,9 @@ const GroupDetails = createReactClass({
     environments: PropTypes.arrayOf(PropTypes.string),
     enableSnuba: PropTypes.bool,
     showGlobalHeader: PropTypes.bool,
+
+    // This gets passed to global selection header
+    disableLoadFromStore: PropTypes.bool,
   },
 
   childContextTypes: {
@@ -229,7 +232,7 @@ const GroupDetails = createReactClass({
   },
 
   render() {
-    const {organization, showGlobalHeader} = this.props;
+    const {organization, disableLoadFromStore, showGlobalHeader} = this.props;
     const {group, project, loading} = this.state;
 
     if (this.state.error) {
@@ -251,6 +254,7 @@ const GroupDetails = createReactClass({
       <React.Fragment>
         {showGlobalHeader && (
           <GlobalSelectionHeader
+            disableLoadFromStore={disableLoadFromStore}
             organization={organization}
             forceProject={project}
             showDateSelector={false}

src/sentry/static/sentry/app/views/organizationGroupDetails/groupEventDetails.jsx

@@ -13,7 +13,6 @@ import ResolutionBox from 'app/components/resolutionBox';
 import MutedBox from 'app/components/mutedBox';
 import withApi from 'app/utils/withApi';
 import withOrganization from 'app/utils/withOrganization';
-import withGlobalSelection from 'app/utils/withGlobalSelection';
 import fetchSentryAppInstallations from 'app/utils/fetchSentryAppInstallations';
 import {fetchSentryAppComponents} from 'app/actionCreators/sentryAppComponents';
 import OrganizationEnvironmentsStore from 'app/stores/organizationEnvironmentsStore';
@@ -178,17 +177,17 @@ class GroupEventDetails extends React.Component {
   }
 }
 
-export {GroupEventDetails};
+function GroupEventDetailsContainer(props) {
+  const environments = OrganizationEnvironmentsStore.getActive().filter(env =>
+    props.environments.includes(env.name)
+  );
+
+  return <GroupEventDetails {...props} environments={environments} />;
+}
 
-export default withApi(
-  withOrganization(
-    withGlobalSelection(props => {
-      const {selection, ...otherProps} = props;
-      const environments = OrganizationEnvironmentsStore.getActive().filter(env =>
-        selection.environments.includes(env.name)
-      );
-
-      return <GroupEventDetails {...otherProps} environments={environments} />;
-    })
-  )
-);
+GroupEventDetailsContainer.propTypes = {
+  environments: PropTypes.arrayOf(SentryTypes.Environment).isRequired,
+};
+
+export default withApi(withOrganization(GroupEventDetailsContainer));
+export {GroupEventDetails};

src/sentry/static/sentry/app/views/organizationGroupDetails/index.jsx

@@ -21,18 +21,23 @@ class OrganizationGroupDetails extends React.Component {
   }
 
   render() {
-    // eslint-disable-next-line no-unused-vars
     const {selection, ...props} = this.props;
 
     return (
       <GroupDetails
         environments={selection.environments}
-        enableSnuba={true}
-        showGlobalHeader={true}
+        enableSnuba
+        showGlobalHeader
         {...props}
       />
     );
   }
 }
 
-export default withOrganization(withGlobalSelection(OrganizationGroupDetails));
+const OrganizationGroupDetailsHoC = withOrganization(
+  withGlobalSelection(OrganizationGroupDetails)
+);
+
+export default function OrganizationGroupDetailsContainer(props) {
+  return <OrganizationGroupDetailsHoC disableLoadFromStore {...props} />;
+}

tests/js/spec/components/organizations/globalSelectionHeader.spec.jsx

@@ -111,6 +111,20 @@ describe('GlobalSelectionHeader', function() {
       })
     );
 
+    // component will initially try to sync router + stores
+    expect(globalActions.updateDateTime).toHaveBeenCalledWith({
+      period: '7d',
+      utc: null,
+      start: null,
+      end: null,
+    });
+    expect(globalActions.updateProjects).toHaveBeenCalledWith([]);
+    expect(globalActions.updateEnvironments).toHaveBeenCalledWith([]);
+
+    globalActions.updateDateTime.mockClear();
+    globalActions.updateProjects.mockClear();
+    globalActions.updateEnvironments.mockClear();
+
     wrapper.setContext(
       changeQuery(routerContext, {
         statsPeriod: '21d',
@@ -125,8 +139,10 @@ describe('GlobalSelectionHeader', function() {
       start: null,
       end: null,
     });
-    expect(globalActions.updateProjects).toHaveBeenCalledWith([]);
-    expect(globalActions.updateEnvironments).toHaveBeenCalledWith([]);
+
+    // These should not be called because they have not changed, only date has changed
+    expect(globalActions.updateProjects).not.toHaveBeenCalled();
+    expect(globalActions.updateEnvironments).not.toHaveBeenCalled();
 
     expect(GlobalSelectionStore.get()).toEqual({
       datetime: {
@@ -248,6 +264,51 @@ describe('GlobalSelectionHeader', function() {
     });
   });
 
+  it('updates store when there are no query params in URL and `disableLoadFromStore` is false', function() {
+    const initializationObj = initializeOrg({
+      organization: {
+        features: ['global-views'],
+      },
+      router: {
+        params: {orgId: 'org-slug'}, // we need this to be set to make sure org in context is same as current org in URL
+        location: {query: {project: [1, 2]}},
+      },
+    });
+
+    mount(
+      <GlobalSelectionHeader organization={initializationObj.organization} />,
+      initializationObj.routerContext
+    );
+
+    expect(globalActions.updateProjects).toHaveBeenCalledWith([1, 2]);
+    expect(globalActions.updateEnvironments).toHaveBeenCalledWith([]);
+    expect(globalActions.updateDateTime).toHaveBeenCalled();
+  });
+
+  it('does not update store when there are no query params in URL and `disableLoadFromStore` is true', function() {
+    const initializationObj = initializeOrg({
+      organization: {
+        features: ['global-views'],
+      },
+      router: {
+        params: {orgId: 'org-slug'}, // we need this to be set to make sure org in context is same as current org in URL
+        location: {query: {}},
+      },
+    });
+
+    mount(
+      <GlobalSelectionHeader
+        organization={initializationObj.organization}
+        disableLoadFromStore={true}
+      />,
+      initializationObj.routerContext
+    );
+
+    expect(globalActions.updateProjects).not.toHaveBeenCalled();
+    expect(globalActions.updateEnvironments).not.toHaveBeenCalled();
+    expect(globalActions.updateDateTime).not.toHaveBeenCalled();
+  });
+
   describe('Single project selection mode', function() {
     it('selects first project if more than one is requested', function() {
       const initializationObj = initializeOrg({

tests/js/spec/views/organizationDashboard/overviewDashboard.spec.jsx

@@ -92,7 +92,6 @@ describe('OverviewDashboard', function() {
 
     createWrapper(dashboardData);
 
-    // TODO(billy): Figure out why releases gets called twice
     expect(discoverMock).toHaveBeenCalledTimes(4);
 
     expect(releasesMock).toHaveBeenCalledTimes(1);