fix(ui): Fix <GlobalSelectionHeader> race conditions (#17126)

Use `shouldComponentUpdate` to enforce checks while organization is still changing, i.e. do not update component if current organization !== orgId in URL

src/sentry/static/sentry/app/actionCreators/organization.jsx

@@ -1,11 +1,12 @@
-import {setActiveOrganization} from 'app/actionCreators/organizations';
-
 import {Client} from 'app/api';
+import {setActiveOrganization} from 'app/actionCreators/organizations';
+import GlobalSelectionActions from 'app/actions/globalSelectionActions';
 import OrganizationActions from 'app/actions/organizationActions';
 import ProjectActions from 'app/actions/projectActions';
 import ProjectsStore from 'app/stores/projectsStore';
 import TeamActions from 'app/actions/teamActions';
 import TeamStore from 'app/stores/teamStore';
+
 /**
  * Fetches an organization's details with an option for the detailed representation
  * with teams and projects
@@ -21,6 +22,7 @@ export async function fetchOrganizationDetails(api, slug, detailed, silent) {
   if (!silent) {
     OrganizationActions.fetchOrg();
     ProjectActions.reset();
+    GlobalSelectionActions.reset();
   }
 
   try {

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

@@ -157,6 +157,10 @@ class GlobalSelectionHeader extends React.Component {
 
     const hasMultipleProjectFeature = this.hasMultipleProjectSelection();
 
+    if (organization?.slug && organization?.slug !== params?.orgId) {
+      return;
+    }
+
     const stateFromRouter = getStateFromQuery(location.query);
     // We should update store if there are any relevant URL parameters when component is mounted
     if (Object.values(stateFromRouter).some(i => !!i)) {
@@ -206,6 +210,13 @@ class GlobalSelectionHeader extends React.Component {
       return true;
     }
 
+    if (
+      nextProps.organization?.slug &&
+      nextProps.organization?.slug !== nextProps.params?.orgId
+    ) {
+      return false;
+    }
+
     // Update if `forceProject` changes or loading state changes
     if (
       this.props.forceProject !== nextProps.forceProject ||
@@ -246,6 +257,10 @@ class GlobalSelectionHeader extends React.Component {
       return true;
     }
 
+    if (this.props.organization !== nextProps.organization) {
+      return true;
+    }
+
     return false;
   }
 
@@ -335,14 +350,13 @@ class GlobalSelectionHeader extends React.Component {
       return;
     }
 
-    const {organization, params} = this.props;
     if (forceProject) {
       // this takes precendence over the other options
       newProject = [getProjectIdFromProject(forceProject)];
     } else if (requestedProjects && requestedProjects.length > 0) {
       // If there is a list of projects from URL params, select first project from that list
       newProject = [requestedProjects[0]];
-    } else if (organization?.slug && organization?.slug === params?.orgId) {
+    } else {
       // When we have finished loading the organization into the props,  i.e. the organization slug is consistent with
       // the URL param--Sentry will get the first project from the organization that the user is a member of.
       newProject = this.getFirstProject();

tests/js/sentry-test/initializeOrg.jsx

@@ -20,7 +20,13 @@ export function initializeOrg({
     projects,
     ...additionalOrg,
   });
-  const router = TestStubs.router(additionalRouter);
+  const router = TestStubs.router({
+    ...additionalRouter,
+    params: {
+      orgId: organization.slug,
+      ...additionalRouter?.params,
+    },
+  });
 
   const routerContext = TestStubs.routerContext([
     {

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

@@ -6,6 +6,7 @@ import {mountWithTheme} from 'sentry-test/enzyme';
 import ConfigStore from 'app/stores/configStore';
 import GlobalSelectionHeader from 'app/components/organizations/globalSelectionHeader';
 import GlobalSelectionStore from 'app/stores/globalSelectionStore';
+import OrganizationActions from 'app/actions/organizationActions';
 import ProjectsStore from 'app/stores/projectsStore';
 import * as globalActions from 'app/actionCreators/globalSelection';
 
@@ -44,6 +45,7 @@ describe('GlobalSelectionHeader', function() {
     ],
     router: {
       location: {query: {}},
+      params: {orgId: 'org-slug'},
     },
   });
 
@@ -67,6 +69,7 @@ describe('GlobalSelectionHeader', function() {
       router.push,
       router.replace,
     ].forEach(mock => mock.mockClear());
+
     MockApiClient.addMockResponse({
       url: '/organizations/org-slug/projects/',
       body: [],
@@ -103,7 +106,10 @@ describe('GlobalSelectionHeader', function() {
 
   it('only updates GlobalSelection store when mounted with query params', async function() {
     mountWithTheme(
-      <GlobalSelectionHeader organization={organization} />,
+      <GlobalSelectionHeader
+        organization={organization}
+        params={{orgId: organization.slug}}
+      />,
       changeQuery(routerContext, {
         statsPeriod: '7d',
       })
@@ -270,6 +276,7 @@ describe('GlobalSelectionHeader', function() {
       ],
       router: {
         location: {query: {project: [1]}},
+        params: {orgId: 'org-slug'},
       },
     });
     jest.spyOn(ProjectsStore, 'getState').mockImplementation(() => ({
@@ -476,7 +483,81 @@ describe('GlobalSelectionHeader', function() {
     });
   });
 
+  /**
+   * GSH: (no global-views)
+   * - mounts with no state from router
+   *   - params org id === org.slug
+   * - updateProjects should not be called (enforceSingleProject should not be called)
+   * - componentDidUpdate with loadingProjects === true, and pass in list of projects (via projects store)
+   * - enforceProject should be called and updateProjects() called with the new project
+   *   - variation:
+   *     - params.orgId !== org.slug (e.g. just switched orgs)
+   *
+   * When switching orgs when not in Issues view, the issues view gets rendered
+   * with params.orgId !== org.slug
+   * Global selection header gets unmounted and mounted, and in this case nothing should be done
+   * until it gets updated and params.orgId === org.slug
+   *
+   * Separate issue:
+   * IssuesList ("child view") renders before a single project is enforced, will require refactoring
+   * views so that they depend on GSH enforcing a single project first IF they don't have required feature
+   * (and no project id in URL).
+   */
   describe('Single project selection mode', function() {
+    it('does not do anything while organization is switching in single project', async function() {
+      const initialData = initializeOrg({
+        organization: {slug: 'old-org-slug'},
+        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]}},
+        },
+      });
+      ProjectsStore.getState.mockRestore();
+      ProjectsStore.getAll.mockRestore();
+
+      MockApiClient.addMockResponse({
+        url: '/organizations/old-org-slug/projects/',
+        body: [],
+      });
+
+      // This can happen when you switch organization so params.orgId !== the current org in context
+      // In this case params.orgId = 'org-slug'
+      const wrapper = mountWithTheme(
+        <GlobalSelectionHeader organization={initialData.organization} />,
+        initialData.routerContext
+      );
+      expect(globalActions.updateProjects).not.toHaveBeenCalled();
+
+      const updatedOrganization = {
+        ...organization,
+        slug: 'org-slug',
+        features: [],
+        projects: [TestStubs.Project({id: '123', slug: 'org-slug-project1'})],
+      };
+
+      MockApiClient.addMockResponse({
+        url: '/organizations/org-slug/',
+        body: updatedOrganization,
+      });
+
+      // Eventually OrganizationContext will fetch org details for `org-slug` and update `organization` prop
+      // emulate fetchOrganizationDetails
+      OrganizationActions.update(updatedOrganization);
+      wrapper.setContext({
+        organization: updatedOrganization,
+        location: {query: {}},
+        router: {
+          ...initialData.router,
+          location: {query: {}},
+        },
+      });
+      wrapper.setProps({organization: updatedOrganization});
+
+      ProjectsStore.loadInitialData(updatedOrganization.projects);
+
+      expect(globalActions.updateProjects).toHaveBeenLastCalledWith([123]);
+    });
+
     it('selects first project if more than one is requested', function() {
       const initializationObj = initializeOrg({
         router: {
@@ -863,18 +944,23 @@ describe('GlobalSelectionHeader', function() {
     beforeEach(function() {
       memberProject = TestStubs.Project({id: '3', isMember: true});
       nonMemberProject = TestStubs.Project({id: '4', isMember: false});
-      const org = TestStubs.Organization({projects: [memberProject, nonMemberProject]});
-      jest
-        .spyOn(ProjectsStore, 'getState')
-        .mockImplementation(() => ({projects: org.projects, loading: false}));
-
       initialData = initializeOrg({
-        organization: org,
+        organization: {
+          projects: [memberProject, nonMemberProject],
+        },
         router: {
           location: {query: {}},
+          params: {
+            orgId: 'org-slug',
+          },
         },
       });
 
+      jest.spyOn(ProjectsStore, 'getState').mockImplementation(() => ({
+        projects: initialData.organization.projects,
+        loading: false,
+      }));
+
       wrapper = mountWithTheme(
         <GlobalSelectionHeader organization={initialData.organization} />,
         initialData.routerContext

tests/js/spec/views/discover/discover.spec.jsx

@@ -1,6 +1,7 @@
 import {browserHistory} from 'react-router';
 import React from 'react';
 
+import {initializeOrg} from 'sentry-test/initializeOrg';
 import {mountWithTheme} from 'sentry-test/enzyme';
 import ConfigStore from 'app/stores/configStore';
 import Discover from 'app/views/discover/discover';
@@ -8,10 +9,10 @@ import GlobalSelectionStore from 'app/stores/globalSelectionStore';
 import createQueryBuilder from 'app/views/discover/queryBuilder';
 
 describe('Discover', function() {
-  let organization, project, queryBuilder;
+  let organization, project, queryBuilder, routerContext;
   beforeEach(function() {
-    project = TestStubs.Project();
-    organization = TestStubs.Organization({projects: [project]});
+    ({organization, project, routerContext} = initializeOrg());
+
     queryBuilder = createQueryBuilder({}, organization);
     GlobalSelectionStore.reset();
     MockApiClient.addMockResponse({
@@ -59,18 +60,19 @@ describe('Discover', function() {
 
     it('auto-runs saved query after tags are loaded', async function() {
       const savedQuery = TestStubs.DiscoverSavedQuery();
+      const params = {savedQueryId: savedQuery.id, orgId: organization.slug};
       wrapper = mountWithTheme(
         <Discover
           location={{query: {}}}
           queryBuilder={queryBuilder}
           organization={organization}
           savedQuery={savedQuery}
-          params={{savedQueryId: savedQuery.id}}
+          params={params}
           updateSavedQueryData={jest.fn()}
           toggleEditMode={jest.fn()}
           isLoading
         />,
-        TestStubs.routerContext([{organization}])
+        routerContext
       );
       await tick();
       expect(wrapper.state().data.baseQuery.query).toBe(null);
@@ -97,7 +99,7 @@ describe('Discover', function() {
           toggleEditMode={jest.fn()}
           isLoading
         />,
-        TestStubs.routerContext([{organization}])
+        routerContext
       );
       await tick();
       expect(wrapper.state().data.baseQuery.query).toBe(null);
@@ -123,7 +125,7 @@ describe('Discover', function() {
           toggleEditMode={jest.fn()}
           isLoading={false}
         />,
-        TestStubs.routerContext([{organization}])
+        routerContext
       );
       await tick();
       expect(wrapper.state().data.baseQuery.query).toBe(null);
@@ -143,7 +145,7 @@ describe('Discover', function() {
           toggleEditMode={jest.fn()}
           isLoading={false}
         />,
-        TestStubs.routerContext([{organization}])
+        routerContext
       );
       expect(wrapper.find('NewQuery')).toHaveLength(1);
       expect(wrapper.find('EditSavedQuery')).toHaveLength(0);
@@ -159,17 +161,18 @@ describe('Discover', function() {
     });
 
     it('handles navigating to new date', async function() {
+      const params = {orgId: organization.slug};
       const wrapper = mountWithTheme(
         <Discover
           queryBuilder={queryBuilder}
           organization={organization}
           updateSavedQueryData={jest.fn()}
           location={{query: {}, search: ''}}
-          params={{}}
+          params={params}
           toggleEditMode={jest.fn()}
           isLoading={false}
         />,
-        TestStubs.routerContext([{organization}])
+        routerContext
       );
 
       expect(wrapper.find('TimeRangeSelector').text()).toEqual('Last 14 days');
@@ -218,7 +221,7 @@ describe('Discover', function() {
           toggleEditMode={jest.fn()}
           isLoading={false}
         />,
-        TestStubs.routerContext()
+        routerContext
       );
     });
 
@@ -280,7 +283,7 @@ describe('Discover', function() {
           toggleEditMode={jest.fn()}
           isLoading={false}
         />,
-        TestStubs.routerContext()
+        routerContext
       );
     });
 
@@ -352,7 +355,7 @@ describe('Discover', function() {
           toggleEditMode={jest.fn()}
           isLoading={false}
         />,
-        TestStubs.routerContext()
+        routerContext
       );
       const createMock = MockApiClient.addMockResponse({
         url: '/organizations/org-slug/discover/saved/',
@@ -401,7 +404,7 @@ describe('Discover', function() {
             toggleEditMode={jest.fn()}
             isLoading={false}
           />,
-          TestStubs.routerContext()
+          routerContext
         );
 
         wrapper.instance().updateField('fields', ['message']);
@@ -483,7 +486,7 @@ describe('Discover', function() {
           toggleEditMode={jest.fn()}
           isLoading={false}
         />,
-        TestStubs.routerContext()
+        routerContext
       );
 
       deleteMock = MockApiClient.addMockResponse({
@@ -567,7 +570,7 @@ describe('Discover', function() {
           toggleEditMode={jest.fn()}
           isLoading={false}
         />,
-        TestStubs.routerContext()
+        routerContext
       );
 
       const mockResponse = Promise.resolve({timing: {}, data: [], meta: []});
@@ -622,7 +625,7 @@ describe('Discover', function() {
           toggleEditMode={jest.fn()}
           isLoading={false}
         />,
-        TestStubs.routerContext()
+        routerContext
       );
     });
 
@@ -660,8 +663,6 @@ describe('Discover', function() {
         body: {timing: {}, data: [], meta: []},
       });
 
-      const routerContext = TestStubs.routerContext([{organization}]);
-
       wrapper = mountWithTheme(
         <Discover
           queryBuilder={queryBuilder}

tests/js/spec/views/events/events.spec.jsx

@@ -40,6 +40,7 @@ describe('EventsErrors', function() {
         pathname: '/organizations/org-slug/events/',
         query: {},
       },
+      params: {orgId: 'org-slug'},
     },
   });
 
@@ -315,6 +316,7 @@ describe('EventsContainer', function() {
         pathname: '/organizations/org-slug/events/',
         query: {},
       },
+      params: {orgId: 'org-slug'},
     },
   });
 

tests/js/spec/views/events/index.spec.jsx

@@ -11,6 +11,10 @@ import ProjectsStore from 'app/stores/projectsStore';
 describe('EventsContainer', function() {
   let wrapper;
   const environments = ['production', 'staging'];
+  const params = {
+    orgId: 'org-slug',
+  };
+
   const {organization, router, routerContext} = initializeOrg({
     projects: [
       {isMember: true, environments, isBookmarked: true},
@@ -24,6 +28,7 @@ describe('EventsContainer', function() {
         pathname: '/organizations/org-slug/events/',
         query: {},
       },
+      params,
     },
   });