Browse Source

ref(ui): Strict config store state (#72174)

Scott Cooper 9 months ago
parent
commit
5c1c8c43e2

+ 25 - 15
static/app/components/acl/access.spec.tsx

@@ -128,9 +128,11 @@ describe('Access', function () {
 
     it('handles no user', function () {
       // Regression test for the share sheet.
-      ConfigStore.config = ConfigFixture({
-        user: undefined,
-      });
+      ConfigStore.loadInitialData(
+        ConfigFixture({
+          user: undefined,
+        })
+      );
 
       render(<Access>{childrenMock}</Access>, {organization});
 
@@ -141,9 +143,11 @@ describe('Access', function () {
     });
 
     it('is superuser', function () {
-      ConfigStore.config = ConfigFixture({
-        user: UserFixture({isSuperuser: true}),
-      });
+      ConfigStore.loadInitialData(
+        ConfigFixture({
+          user: UserFixture({isSuperuser: true}),
+        })
+      );
 
       render(<Access isSuperuser>{childrenMock}</Access>, {
         organization,
@@ -156,9 +160,11 @@ describe('Access', function () {
     });
 
     it('is not superuser', function () {
-      ConfigStore.config = ConfigFixture({
-        user: UserFixture({isSuperuser: false}),
-      });
+      ConfigStore.loadInitialData(
+        ConfigFixture({
+          user: UserFixture({isSuperuser: false}),
+        })
+      );
 
       render(<Access isSuperuser>{childrenMock}</Access>, {
         organization,
@@ -195,9 +201,11 @@ describe('Access', function () {
     });
 
     it('has superuser', function () {
-      ConfigStore.config = ConfigFixture({
-        user: UserFixture({isSuperuser: true}),
-      });
+      ConfigStore.loadInitialData(
+        ConfigFixture({
+          user: UserFixture({isSuperuser: true}),
+        })
+      );
 
       render(
         <Access isSuperuser>
@@ -210,9 +218,11 @@ describe('Access', function () {
     });
 
     it('has no superuser', function () {
-      ConfigStore.config = ConfigFixture({
-        user: UserFixture({isSuperuser: false}),
-      });
+      ConfigStore.loadInitialData(
+        ConfigFixture({
+          user: UserFixture({isSuperuser: false}),
+        })
+      );
 
       render(
         <Access isSuperuser>

+ 5 - 3
static/app/components/acl/feature.spec.tsx

@@ -182,9 +182,11 @@ describe('Feature', function () {
     });
 
     it('checks ConfigStore.config.features (e.g. `organizations:create`)', function () {
-      ConfigStore.config = ConfigFixture({
-        features: new Set(['organizations:create']),
-      });
+      ConfigStore.loadInitialData(
+        ConfigFixture({
+          features: new Set(['organizations:create']),
+        })
+      );
 
       render(
         <WrappedFeature features="organizations:create">{childrenMock}</WrappedFeature>,

+ 7 - 7
static/app/components/acl/role.spec.tsx

@@ -1,6 +1,7 @@
 import {OrganizationFixture} from 'sentry-fixture/organization';
+import {UserFixture} from 'sentry-fixture/user';
 
-import {render, screen} from 'sentry-test/reactTestingLibrary';
+import {act, render, screen} from 'sentry-test/reactTestingLibrary';
 
 import {Role} from 'sentry/components/acl/role';
 import ConfigStore from 'sentry/stores/configStore';
@@ -92,8 +93,8 @@ describe('Role', function () {
     });
 
     it('handles no user', function () {
-      const user = {...ConfigStore.config.user};
-      ConfigStore.config.user = undefined as any;
+      const user = {...ConfigStore.get('user')};
+      ConfigStore.set('user', undefined as any);
       render(<Role role="member">{childrenMock}</Role>, {
         organization,
       });
@@ -101,12 +102,11 @@ describe('Role', function () {
       expect(childrenMock).toHaveBeenCalledWith({
         hasRole: false,
       });
-      ConfigStore.config.user = user;
+      act(() => ConfigStore.set('user', user));
     });
 
     it('updates if user changes', function () {
-      const user = {...ConfigStore.config.user};
-      ConfigStore.config.user = undefined as any;
+      ConfigStore.set('user', undefined as any);
       const {rerender} = render(<Role role="member">{childrenMock}</Role>, {
         organization,
       });
@@ -114,7 +114,7 @@ describe('Role', function () {
       expect(childrenMock).toHaveBeenCalledWith({
         hasRole: false,
       });
-      ConfigStore.config.user = user;
+      act(() => ConfigStore.set('user', UserFixture()));
 
       rerender(<Role role="member">{childrenMock}</Role>);
       expect(childrenMock).toHaveBeenCalledWith({

+ 8 - 6
static/app/components/assistant/guideAnchor.spec.tsx

@@ -17,12 +17,14 @@ describe('GuideAnchor', function () {
   const firstGuideHeader = 'How bad is it?';
 
   beforeEach(function () {
-    ConfigStore.config = ConfigFixture({
-      user: UserFixture({
-        isSuperuser: false,
-        dateJoined: '2020-01-01T00:00:00',
-      }),
-    });
+    ConfigStore.loadInitialData(
+      ConfigFixture({
+        user: UserFixture({
+          isSuperuser: false,
+          dateJoined: '2020-01-01T00:00:00',
+        }),
+      })
+    );
   });
 
   it('renders, async advances, async and finishes', async function () {

+ 3 - 3
static/app/components/contextPickerModal.spec.tsx

@@ -194,7 +194,7 @@ describe('ContextPickerModal', function () {
   it('isSuperUser and selects an integrationConfig and calls `onFinish` with URL to that configuration', async function () {
     OrganizationsStore.load([org]);
     OrganizationStore.onUpdate(org);
-    ConfigStore.config.user = UserFixture({isSuperuser: true});
+    ConfigStore.set('user', UserFixture({isSuperuser: true}));
 
     const provider = {slug: 'github'};
     const configUrl = `/api/0/organizations/${org.slug}/integrations/?provider_key=${provider.slug}&includeConfig=0`;
@@ -238,7 +238,7 @@ describe('ContextPickerModal', function () {
   it('not superUser and cannot select an integrationConfig and calls `onFinish` with URL to integration overview page', async function () {
     OrganizationsStore.load([org]);
     OrganizationStore.onUpdate(org);
-    ConfigStore.config.user = UserFixture({isSuperuser: false});
+    ConfigStore.set('user', UserFixture({isSuperuser: false}));
 
     const provider = {slug: 'github'};
     const configUrl = `/api/0/organizations/${org.slug}/integrations/?provider_key=${provider.slug}&includeConfig=0`;
@@ -270,7 +270,7 @@ describe('ContextPickerModal', function () {
   it('is superUser and no integration configurations and calls `onFinish` with URL to integration overview page', async function () {
     OrganizationsStore.load([org]);
     OrganizationStore.onUpdate(org);
-    ConfigStore.config.user = UserFixture({isSuperuser: false});
+    ConfigStore.set('user', UserFixture({isSuperuser: false}));
 
     const provider = {slug: 'github'};
     const configUrl = `/api/0/organizations/${org.slug}/integrations/?provider_key=${provider.slug}&includeConfig=0`;

+ 6 - 0
static/app/stores/configStore.spec.tsx

@@ -26,4 +26,10 @@ describe('ConfigStore', () => {
       sentryUrl: 'https://sentry.io',
     });
   });
+
+  it('returns a stable reference from getState()', () => {
+    ConfigStore.set('theme', 'dark');
+    const state = ConfigStore.getState();
+    expect(Object.is(state, ConfigStore.getState())).toBe(true);
+  });
 });

+ 8 - 15
static/app/stores/configStore.tsx

@@ -3,17 +3,10 @@ import {createStore} from 'reflux';
 
 import type {Config} from 'sentry/types/system';
 
-import type {CommonStoreDefinition} from './types';
+import type {StrictStoreDefinition} from './types';
 
-interface InternalConfigStore {
-  config: Config;
-}
-
-interface ConfigStoreDefinition
-  extends CommonStoreDefinition<Config>,
-    InternalConfigStore {
+interface ConfigStoreDefinition extends StrictStoreDefinition<Config> {
   get<K extends keyof Config>(key: K): Config[K];
-  init(): void;
   loadInitialData(config: Config): void;
   set<K extends keyof Config>(key: K, value: Config[K]): void;
 }
@@ -21,28 +14,28 @@ interface ConfigStoreDefinition
 const storeConfig: ConfigStoreDefinition = {
   // When the app is booted we will _immediately_ hydrate the config store,
   // effecively ensuring this is not empty.
-  config: {} as Config,
+  state: {} as Config,
 
   init(): void {
     // XXX: Do not use `this.listenTo` in this store. We avoid usage of reflux
     // listeners due to their leaky nature in tests.
 
-    this.config = {} as Config;
+    this.state = {} as Config;
   },
 
   get(key) {
-    return this.config[key];
+    return this.state[key];
   },
 
   set(key, value) {
-    this.config = {...this.config, [key]: value};
+    this.state = {...this.state, [key]: value};
     this.trigger({[key]: value});
   },
 
   loadInitialData(config): void {
     const shouldUseDarkMode = config.user?.options.theme === 'dark';
 
-    this.config = {
+    this.state = {
       ...config,
       features: new Set(config.features || []),
       theme: shouldUseDarkMode ? 'dark' : 'light',
@@ -58,7 +51,7 @@ const storeConfig: ConfigStoreDefinition = {
   },
 
   getState() {
-    return this.config;
+    return this.state;
   },
 };
 

+ 9 - 7
static/app/stores/guideStore.spec.tsx

@@ -13,13 +13,15 @@ describe('GuideStore', function () {
 
   beforeEach(function () {
     jest.clearAllMocks();
-    ConfigStore.config = ConfigFixture({
-      user: UserFixture({
-        id: '5',
-        isSuperuser: false,
-        dateJoined: '2020-01-01T00:00:00',
-      }),
-    });
+    ConfigStore.loadInitialData(
+      ConfigFixture({
+        user: UserFixture({
+          id: '5',
+          isSuperuser: false,
+          dateJoined: '2020-01-01T00:00:00',
+        }),
+      })
+    );
     GuideStore.init();
     data = [
       {

+ 39 - 33
static/app/utils/useIsSentryEmployee.spec.tsx

@@ -8,17 +8,19 @@ import {useIsSentryEmployee} from 'sentry/utils/useIsSentryEmployee';
 
 describe('useIsSentryEmployee', () => {
   it('should return true if the user is a Sentry employee', () => {
-    ConfigStore.config = ConfigFixture({
-      user: UserFixture({
-        emails: [
-          {
-            email: 'jenn@sentry.io',
-            is_verified: true,
-            id: '1',
-          },
-        ],
-      }),
-    });
+    ConfigStore.loadInitialData(
+      ConfigFixture({
+        user: UserFixture({
+          emails: [
+            {
+              email: 'jenn@sentry.io',
+              is_verified: true,
+              id: '1',
+            },
+          ],
+        }),
+      })
+    );
 
     const {result} = renderHook(() => useIsSentryEmployee());
 
@@ -27,17 +29,19 @@ describe('useIsSentryEmployee', () => {
 
   it('should return false if the user is not a Sentry employee', () => {
     // Mock ConfigStore to simulate a non-Sentry employee
-    ConfigStore.config = ConfigFixture({
-      user: UserFixture({
-        emails: [
-          {
-            email: 'jenn@not-sentry.com',
-            is_verified: true,
-            id: '1',
-          },
-        ],
-      }),
-    });
+    ConfigStore.loadInitialData(
+      ConfigFixture({
+        user: UserFixture({
+          emails: [
+            {
+              email: 'jenn@not-sentry.com',
+              is_verified: true,
+              id: '1',
+            },
+          ],
+        }),
+      })
+    );
 
     const {result} = renderHook(() => useIsSentryEmployee());
 
@@ -45,17 +49,19 @@ describe('useIsSentryEmployee', () => {
   });
 
   it('should return false if the email is not verified', () => {
-    ConfigStore.config = ConfigFixture({
-      user: UserFixture({
-        emails: [
-          {
-            email: 'jenn@sentry.io',
-            is_verified: false,
-            id: '1',
-          },
-        ],
-      }),
-    });
+    ConfigStore.loadInitialData(
+      ConfigFixture({
+        user: UserFixture({
+          emails: [
+            {
+              email: 'jenn@sentry.io',
+              is_verified: false,
+              id: '1',
+            },
+          ],
+        }),
+      })
+    );
 
     const {result} = renderHook(() => useIsSentryEmployee());
 

+ 2 - 2
static/app/views/app/index.spec.tsx

@@ -2,7 +2,7 @@ import {InstallWizardFixture} from 'sentry-fixture/installWizard';
 import {OrganizationFixture} from 'sentry-fixture/organization';
 
 import {initializeOrg} from 'sentry-test/initializeOrg';
-import {render, screen, waitFor} from 'sentry-test/reactTestingLibrary';
+import {act, render, screen, waitFor} from 'sentry-test/reactTestingLibrary';
 
 import ConfigStore from 'sentry/stores/configStore';
 import HookStore from 'sentry/stores/hookStore';
@@ -237,7 +237,7 @@ describe('App', function () {
         <div>placeholder content</div>
       </App>
     );
-    ConfigStore.config.isSelfHosted = restore;
+    act(() => ConfigStore.set('isSelfHosted', restore));
 
     await waitFor(() => OrganizationsStore.getAll().length === 1);
 

Some files were not shown because too many files changed in this diff