feat(ui): Fix OrganizationContext sometimes double fetching for org details (#13655)

Fixes the race condition where `OrganizationContext` gets loaded twice if `OrganizationStore` finishes loading *after* `OrganizationDetails.fetchData` finishes.

Author: billyvg

src/sentry/static/sentry/app/views/organizationContext.jsx

@@ -71,13 +71,16 @@ const OrganizationContext = createReactClass({
       this.props.params.orgId &&
       prevProps.params.orgId !== this.props.params.orgId;
 
+    // protect against the case where we finish fetching org details
+    // and then `OrganizationsStore` finishes loading:
+    // only fetch in the case where we don't have an orgId
     const organizationLoadingChanged =
       prevProps.organizationsLoading !== this.props.organizationsLoading &&
       this.props.organizationsLoading === false;
 
     if (
       hasOrgIdAndChanged ||
-      organizationLoadingChanged ||
+      (!this.props.params.orgId && organizationLoadingChanged) ||
       (this.props.location.state === 'refresh' && prevProps.location.state !== 'refresh')
     ) {
       this.remountComponent();
@@ -146,6 +149,7 @@ const OrganizationContext = createReactClass({
       })
       .catch(err => {
         let errorType = null;
+
         switch (err.statusText) {
           case 'NOT FOUND':
             errorType = ERROR_TYPES.ORG_NOT_FOUND;
@@ -161,6 +165,10 @@ const OrganizationContext = createReactClass({
         // If user is superuser, open sudo window
         const user = ConfigStore.get('user');
         if (!user || !user.isSuperuser || err.status !== 403) {
+          // This `catch` can swallow up errors in development (and tests)
+          // So let's log them. This may create some noise, especially the test case where
+          // we specifically test this branch
+          console.error(err); // eslint-disable-line no-console
           return;
         }
         openSudo({

tests/js/spec/views/organizationContext.spec.jsx

@@ -87,6 +87,10 @@ describe('OrganizationContext', function() {
 
   it('fetches new org when router params change', function() {
     wrapper = createWrapper();
+    MockApiClient.addMockResponse({
+      url: '/organizations/new-slug/environments/',
+      body: TestStubs.Environments(),
+    });
     const mock = MockApiClient.addMockResponse({
       url: '/organizations/new-slug/',
       body: org,
@@ -140,8 +144,13 @@ describe('OrganizationContext', function() {
   });
 
   it('uses last organization from ConfigStore', function() {
+    MockApiClient.addMockResponse({
+      url: '/organizations/lastOrganization/environments/',
+      body: TestStubs.Environments(),
+    });
     getOrgMock = MockApiClient.addMockResponse({
       url: '/organizations/lastOrganization/',
+      body: org,
     });
     // mocking `.get('lastOrganization')`
     ConfigStore.get.mockImplementation(() => 'lastOrganization');
@@ -153,8 +162,13 @@ describe('OrganizationContext', function() {
   });
 
   it('uses last organization from `organizations` prop', async function() {
+    MockApiClient.addMockResponse({
+      url: '/organizations/foo/environments/',
+      body: TestStubs.Environments(),
+    });
     getOrgMock = MockApiClient.addMockResponse({
       url: '/organizations/foo/',
+      body: org,
     });
     ConfigStore.get.mockImplementation(() => '');
 
@@ -182,4 +196,28 @@ describe('OrganizationContext', function() {
 
     expect(getOrgMock).toHaveBeenLastCalledWith('/organizations/foo/', expect.anything());
   });
+
+  it('fetches org details only once if organizations loading store changes', async function() {
+    wrapper = createWrapper({
+      params: {orgId: 'org-slug'},
+      organizationsLoading: true,
+      organizations: [],
+    });
+    await tick();
+    wrapper.update();
+    expect(wrapper.find('LoadingIndicator')).toHaveLength(0);
+    expect(getOrgMock).toHaveBeenCalledTimes(1);
+
+    // Simulate OrganizationsStore being loaded *after* `OrganizationContext` finishes
+    // org details fetch
+    wrapper.setProps({
+      organizationsLoading: false,
+      organizations: [
+        TestStubs.Organization({slug: 'foo'}),
+        TestStubs.Organization({slug: 'bar'}),
+      ],
+    });
+
+    expect(getOrgMock).toHaveBeenCalledTimes(1);
+  });
 });