Browse Source

fix(issues): persist state so that cache doesnt override it (#60857)

Subsequent changes to the store might change the ordering of items,
meaning we shouldn't rely on the cached state from that point in time
and need to persist it across requests instead.

The stat request loading now works for all cases and the data is
persisted in the state local to the provider
Jonas 1 year ago
parent
commit
420ca1ad35

+ 6 - 3
static/app/views/issueList/groupStatsProvider.tsx

@@ -1,4 +1,4 @@
-import {createContext, useContext, useEffect} from 'react';
+import {createContext, useContext, useEffect, useState} from 'react';
 import * as Sentry from '@sentry/react';
 import {dropUndefinedKeys} from '@sentry/utils';
 import * as reactQuery from '@tanstack/react-query';
@@ -78,6 +78,7 @@ export interface GroupStatsProviderProps {
 
 export function GroupStatsProvider(props: GroupStatsProviderProps) {
   const api = useApi();
+  const [groupStats, setGroupStats] = useState<Record<string, GroupStats>>({});
 
   const queryFn = (): Promise<Record<string, GroupStats>> => {
     const promise = api
@@ -92,13 +93,14 @@ export function GroupStatsProvider(props: GroupStatsProviderProps) {
         includeAllArgs: true,
       })
       .then((resp: ApiResult<GroupStats[]>): Record<string, GroupStats> => {
-        const map: Record<string, GroupStats> = {};
+        const map: Record<string, GroupStats> = {...groupStats};
         if (!resp || !Array.isArray(resp[0])) {
           return map;
         }
         for (const stat of resp[0]) {
           map[stat.id] = stat;
         }
+        setGroupStats(map);
         return map;
       })
       .catch(e => {
@@ -132,7 +134,8 @@ export function GroupStatsProvider(props: GroupStatsProviderProps) {
   }, [statsQuery.status, onStatsQuery]);
 
   return (
-    <GroupStatsContext.Provider value={statsQuery}>
+    // @ts-expect-error we are overriding data with the stored version
+    <GroupStatsContext.Provider value={{...statsQuery, data: groupStats}}>
       {props.children}
     </GroupStatsContext.Provider>
   );

+ 225 - 35
static/app/views/issueList/overview.spec.tsx

@@ -1,7 +1,10 @@
 import {browserHistory} from 'react-router';
 import merge from 'lodash/merge';
+import {Group} from 'sentry-fixture/group';
 import {GroupStats} from 'sentry-fixture/groupStats';
+import {Member} from 'sentry-fixture/member';
 import {Organization} from 'sentry-fixture/organization';
+import {Project} from 'sentry-fixture/project';
 import {Search} from 'sentry-fixture/search';
 import {Tags} from 'sentry-fixture/tags';
 
@@ -16,7 +19,7 @@ import {
 } from 'sentry-test/reactTestingLibrary';
 import {textWithMarkupMatcher} from 'sentry-test/utils';
 
-import StreamGroup from 'sentry/components/stream/group';
+import GroupStore from 'sentry/stores/groupStore';
 import ProjectsStore from 'sentry/stores/projectsStore';
 import TagStore from 'sentry/stores/tagStore';
 import {SavedSearchVisibility} from 'sentry/types';
@@ -26,17 +29,16 @@ import IssueListWithStores, {IssueListOverview} from 'sentry/views/issueList/ove
 
 // Mock <IssueListActions>
 jest.mock('sentry/views/issueList/actions', () => jest.fn(() => null));
-jest.mock('sentry/components/stream/group', () => jest.fn(() => null));
 
 const DEFAULT_LINKS_HEADER =
   '<http://127.0.0.1:8000/api/0/organizations/org-slug/issues/?cursor=1443575731:0:1>; rel="previous"; results="false"; cursor="1443575731:0:1", ' +
   '<http://127.0.0.1:8000/api/0/organizations/org-slug/issues/?cursor=1443575000:0:0>; rel="next"; results="true"; cursor="1443575000:0:0"';
 
-const project = TestStubs.Project({
+const project = Project({
   id: '3559',
   name: 'Foo Project',
   slug: 'project-slug',
-  firstEvent: true,
+  firstEvent: new Date().toISOString(),
 });
 
 const {organization, router, routerContext} = initializeOrg({
@@ -62,7 +64,7 @@ describe('IssueList', function () {
   let props;
 
   const tags = Tags();
-  const group = TestStubs.Group({project});
+  const group = Group({project});
   const groupStats = GroupStats();
   const savedSearch = Search({
     id: '789',
@@ -131,7 +133,7 @@ describe('IssueList', function () {
     fetchMembersRequest = MockApiClient.addMockResponse({
       url: '/organizations/org-slug/users/',
       method: 'GET',
-      body: [TestStubs.Member({projects: [project.slug]})],
+      body: [Member({projects: [project.slug]})],
     });
     MockApiClient.addMockResponse({
       url: '/organizations/org-slug/sent-first-event/',
@@ -177,8 +179,6 @@ describe('IssueList', function () {
     let issuesRequest: jest.Mock;
 
     beforeEach(function () {
-      jest.mocked(StreamGroup).mockClear();
-
       recentSearchesRequest = MockApiClient.addMockResponse({
         url: '/organizations/org-slug/recent-searches/',
         method: 'GET',
@@ -437,9 +437,11 @@ describe('IssueList', function () {
     });
 
     it('caches the search results', async function () {
+      act(() => ProjectsStore.loadInitialData([project]));
+
       issuesRequest = MockApiClient.addMockResponse({
         url: '/organizations/org-slug/issues/',
-        body: [...new Array(25)].map((_, i) => ({id: i})),
+        body: [...new Array(25)].map((_, i) => Group({id: `${i}`})),
         headers: {
           Link: DEFAULT_LINKS_HEADER,
           'X-Hits': '500',
@@ -1164,23 +1166,23 @@ describe('IssueList', function () {
 
     it('displays when no projects selected and all projects user is member of, async does not have first event', async function () {
       const projects = [
-        TestStubs.Project({
+        Project({
           id: '1',
           slug: 'foo',
           isMember: true,
-          firstEvent: false,
+          firstEvent: undefined,
         }),
-        TestStubs.Project({
+        Project({
           id: '2',
           slug: 'bar',
           isMember: true,
-          firstEvent: false,
+          firstEvent: undefined,
         }),
-        TestStubs.Project({
+        Project({
           id: '3',
           slug: 'baz',
           isMember: true,
-          firstEvent: false,
+          firstEvent: undefined,
         }),
       ];
       MockApiClient.addMockResponse({
@@ -1210,23 +1212,23 @@ describe('IssueList', function () {
 
     it('does not display when no projects selected and any projects have a first event', async function () {
       const projects = [
-        TestStubs.Project({
+        Project({
           id: '1',
           slug: 'foo',
           isMember: true,
-          firstEvent: false,
+          firstEvent: '',
         }),
-        TestStubs.Project({
+        Project({
           id: '2',
           slug: 'bar',
           isMember: true,
-          firstEvent: true,
+          firstEvent: new Date().toISOString(),
         }),
-        TestStubs.Project({
+        Project({
           id: '3',
           slug: 'baz',
           isMember: true,
-          firstEvent: false,
+          firstEvent: new Date().toISOString(),
         }),
       ];
       MockApiClient.addMockResponse({
@@ -1251,23 +1253,23 @@ describe('IssueList', function () {
 
     it('displays when all selected projects do not have first event', async function () {
       const projects = [
-        TestStubs.Project({
+        Project({
           id: '1',
           slug: 'foo',
           isMember: true,
-          firstEvent: false,
+          firstEvent: undefined,
         }),
-        TestStubs.Project({
+        Project({
           id: '2',
           slug: 'bar',
           isMember: true,
-          firstEvent: false,
+          firstEvent: undefined,
         }),
-        TestStubs.Project({
+        Project({
           id: '3',
           slug: 'baz',
           isMember: true,
-          firstEvent: false,
+          firstEvent: undefined,
         }),
       ];
       MockApiClient.addMockResponse({
@@ -1302,23 +1304,23 @@ describe('IssueList', function () {
 
     it('does not display when any selected projects have first event', async function () {
       const projects = [
-        TestStubs.Project({
+        Project({
           id: '1',
           slug: 'foo',
           isMember: true,
-          firstEvent: false,
+          firstEvent: undefined,
         }),
-        TestStubs.Project({
+        Project({
           id: '2',
           slug: 'bar',
           isMember: true,
-          firstEvent: true,
+          firstEvent: new Date().toISOString(),
         }),
-        TestStubs.Project({
+        Project({
           id: '3',
           slug: 'baz',
           isMember: true,
-          firstEvent: true,
+          firstEvent: new Date().toISOString(),
         }),
       ];
       MockApiClient.addMockResponse({
@@ -1349,9 +1351,11 @@ describe('IssueList', function () {
   });
 
   it('displays a count that represents the current page', function () {
+    act(() => ProjectsStore.loadInitialData([project]));
+
     MockApiClient.addMockResponse({
       url: '/organizations/org-slug/issues/',
-      body: [...new Array(25)].map((_, i) => ({id: i})),
+      body: [...new Array(25)].map((_, i) => Group({id: `${i}`})),
       headers: {
         Link: DEFAULT_LINKS_HEADER,
         'X-Hits': '500',
@@ -1440,7 +1444,7 @@ describe('IssueList', function () {
       });
 
       it('for multiple projects', function () {
-        const projectBar = TestStubs.Project({
+        const projectBar = Project({
           id: '3560',
           name: 'Bar Project',
           slug: 'project-slug-bar',
@@ -1485,3 +1489,189 @@ describe('IssueList', function () {
     });
   });
 });
+
+describe('stats', () => {
+  const tags = Tags();
+  const groupStats = GroupStats();
+
+  const savedSearch = Search({
+    id: '789',
+    query: 'is:unresolved TypeError',
+    sort: 'date',
+    name: 'Unresolved TypeErrors',
+  });
+
+  beforeEach(() => {
+    GroupStore.loadInitialData([]);
+    MockApiClient.clearMockResponses();
+    MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/searches/',
+      body: [savedSearch],
+    });
+    MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/recent-searches/',
+      body: [],
+    });
+    MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/recent-searches/',
+      method: 'POST',
+      body: [],
+    });
+    MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/issues-count/',
+      method: 'GET',
+      body: [{}],
+    });
+    MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/processingissues/',
+      method: 'GET',
+      body: [
+        {
+          project: 'test-project',
+          numIssues: 1,
+          hasIssues: true,
+          lastSeen: '2019-01-16T15:39:11.081Z',
+        },
+      ],
+    });
+    MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/tags/',
+      method: 'GET',
+      body: tags,
+    });
+    MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/users/',
+      method: 'GET',
+      body: [Member({projects: [project.slug]})],
+    });
+    MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/sent-first-event/',
+      body: {sentFirstEvent: true},
+    });
+    MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/projects/',
+      body: [project],
+    });
+  });
+
+  it('fetches stats for route', async () => {
+    const customRouterProps = {
+      params: router.params,
+      location: router.location,
+    };
+
+    const groups: any[] = [];
+    for (let i = 0; i < 25; i++) {
+      groups.push(Group({project, id: `${i}`}));
+    }
+
+    const issuesRequest = MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/issues/',
+      body: groups,
+      headers: {
+        Link: DEFAULT_LINKS_HEADER,
+      },
+    });
+
+    const statsRequest = MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/issues-stats/',
+      body: [groupStats],
+    });
+
+    render(<IssueListWithStores {...customRouterProps} />, {
+      context: routerContext,
+    });
+
+    await waitForElementToBeRemoved(() => screen.getByTestId('loading-indicator'));
+
+    expect(issuesRequest).toHaveBeenCalled();
+    expect(statsRequest.mock.calls[0][1].query).toEqual(
+      expect.objectContaining({
+        groups: groups.map(g => g.id),
+      })
+    );
+
+    expect(await screen.findAllByTestId('group')).toHaveLength(25);
+    expect(screen.queryAllByTestId('loading-placeholder')).toHaveLength(0);
+  });
+
+  it('refetches stats for another route', async () => {
+    const customRouterProps = {
+      params: router.params,
+      location: router.location,
+    };
+
+    const groups: any[] = [];
+    const statsForGroups: any[] = [];
+
+    for (let i = 0; i < 25; i++) {
+      groups.push(Group({project, id: `${i}`}));
+      statsForGroups.push(GroupStats({id: `${i}`}));
+    }
+
+    const forReviewGroups: any[] = [];
+    const forReviewGroupStats: any[] = [];
+    for (let i = 0; i < 25; i++) {
+      forReviewGroups.push(Group({project, id: `${groups.length + i}`}));
+      forReviewGroupStats.push(GroupStats({id: `${groups.length + i}`}));
+    }
+
+    const issuesRequest = MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/issues/',
+      body: groups,
+      headers: {
+        Link: DEFAULT_LINKS_HEADER,
+      },
+    });
+
+    const statsRequest = MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/issues-stats/',
+      body: [statsForGroups],
+    });
+
+    const {rerender} = render(<IssueListWithStores {...customRouterProps} />, {
+      context: routerContext,
+    });
+
+    await waitForElementToBeRemoved(() => screen.getByTestId('loading-indicator'));
+
+    expect(issuesRequest).toHaveBeenCalled();
+    expect(statsRequest).toHaveBeenCalled();
+
+    expect(await screen.findAllByTestId('group')).toHaveLength(25);
+    expect(screen.queryAllByTestId('loading-placeholder')).toHaveLength(0);
+
+    const newRouteProps = {
+      ...customRouterProps,
+      location: {
+        ...customRouterProps.location,
+        query: {
+          query:
+            'is:unresolved is:for_review assigned_or_suggested:[me, my_teams, none] ',
+        },
+        hash: '',
+      },
+    };
+
+    const forReviewGroupsRequest = MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/issues/',
+      body: forReviewGroups,
+      headers: {
+        Link: DEFAULT_LINKS_HEADER,
+      },
+    });
+
+    const forReviewStatsRequest = MockApiClient.addMockResponse({
+      url: '/organizations/org-slug/issues-stats/',
+      body: [forReviewGroupStats],
+    });
+
+    rerender(<IssueListWithStores {...newRouteProps} />);
+
+    expect(forReviewGroupsRequest).toHaveBeenCalled();
+    expect(forReviewStatsRequest).toHaveBeenCalled();
+
+    expect(await screen.findAllByTestId('group')).toHaveLength(25);
+    expect(screen.queryAllByTestId('loading-placeholder')).toHaveLength(0);
+  });
+});

+ 1 - 0
static/app/views/issueList/overview.tsx

@@ -558,6 +558,7 @@ class IssueListOverview extends Component<Props, State> {
     this.setState({
       itemsRemoved: 0,
       error: null,
+      statsForGroupIds: [],
     });
 
     // Used for Issue Stream Performance project, enabled means we are doing saved search look up in the backend