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

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);