From 17072178fa9a64aa99fde55cf3efeae75593a84e Mon Sep 17 00:00:00 2001 From: 7PH Date: Tue, 21 Nov 2023 15:43:04 +0100 Subject: [PATCH] SONAR-20975 Change IssuesPage issue loading logic to always be to able fetch open issue --- .../main/js/api/mocks/IssuesServiceMock.ts | 6 + .../src/main/js/api/mocks/data/issues.ts | 3 +- .../js/apps/issues/__tests__/IssueApp-it.tsx | 8 + .../js/apps/issues/__tests__/IssuesApp-it.tsx | 6 +- .../js/apps/issues/components/IssuesApp.tsx | 236 ++++++++---------- .../SubnavigationIssue.tsx | 2 +- .../src/main/js/apps/issues/test-utils.tsx | 5 + .../main/js/components/common/PageCounter.tsx | 2 +- .../resources/org/sonar/l10n/core.properties | 1 - 9 files changed, 136 insertions(+), 133 deletions(-) diff --git a/server/sonar-web/src/main/js/api/mocks/IssuesServiceMock.ts b/server/sonar-web/src/main/js/api/mocks/IssuesServiceMock.ts index f2d889a0d01..382bac185e7 100644 --- a/server/sonar-web/src/main/js/api/mocks/IssuesServiceMock.ts +++ b/server/sonar-web/src/main/js/api/mocks/IssuesServiceMock.ts @@ -462,6 +462,12 @@ export default class IssuesServiceMock { return item.issue.codeVariants.some( (codeVariant) => query.codeVariants?.split(',').includes(codeVariant), ); + }) + .filter((item) => { + if (!query.issues) { + return true; + } + return query.issues.split(',').includes(item.issue.key); }); // Splice list items according to paging using a fixed page size diff --git a/server/sonar-web/src/main/js/api/mocks/data/issues.ts b/server/sonar-web/src/main/js/api/mocks/data/issues.ts index d2634d338e8..863c582e055 100644 --- a/server/sonar-web/src/main/js/api/mocks/data/issues.ts +++ b/server/sonar-web/src/main/js/api/mocks/data/issues.ts @@ -387,6 +387,7 @@ export function mockIssuesList(baseComponentKey = PARENT_COMPONENT_KEY): IssueDa project: 'org.project2', assignee: 'email1@sonarsource.com', author: 'email3@sonarsource.com', + issueStatus: IssueStatus.Confirmed, }), snippets: keyBy( [ @@ -438,7 +439,7 @@ export function mockIssuesList(baseComponentKey = PARENT_COMPONENT_KEY): IssueDa key: ISSUE_1103, component: `${baseComponentKey}:${ISSUE_TO_FILES[ISSUE_1103][0]}`, creationDate: '2022-01-15T09:36:01+0100', - message: 'Issue inside folderA', + message: 'Issue 2 inside folderA', type: IssueType.CodeSmell, rule: ISSUE_TO_RULE[ISSUE_1103], textRange: { diff --git a/server/sonar-web/src/main/js/apps/issues/__tests__/IssueApp-it.tsx b/server/sonar-web/src/main/js/apps/issues/__tests__/IssueApp-it.tsx index 3d0555f1af1..c610490fa88 100644 --- a/server/sonar-web/src/main/js/apps/issues/__tests__/IssueApp-it.tsx +++ b/server/sonar-web/src/main/js/apps/issues/__tests__/IssueApp-it.tsx @@ -64,6 +64,14 @@ beforeEach(() => { }); describe('issue app', () => { + it('should always be able to render the open issue', async () => { + renderProjectIssuesApp('project/issues?issueStatuses=CONFIRMED&open=issue2&id=myproject&why=1'); + + expect(await ui.conciseIssueTotal.find()).toHaveTextContent('3'); + expect(ui.conciseIssueItem4.get()).toBeInTheDocument(); + expect(ui.conciseIssueItem2.get()).toBeInTheDocument(); + }); + it('should navigate to Why is this an issue tab', async () => { renderProjectIssuesApp('project/issues?issues=issue2&open=issue2&id=myproject&why=1'); diff --git a/server/sonar-web/src/main/js/apps/issues/__tests__/IssuesApp-it.tsx b/server/sonar-web/src/main/js/apps/issues/__tests__/IssuesApp-it.tsx index 23152f04b84..6c4457bbeae 100644 --- a/server/sonar-web/src/main/js/apps/issues/__tests__/IssuesApp-it.tsx +++ b/server/sonar-web/src/main/js/apps/issues/__tests__/IssuesApp-it.tsx @@ -233,14 +233,14 @@ describe('issues app', () => { renderIssueApp(); expect(await ui.issueItems.findAll()).toHaveLength(7); - expect(ui.issueItem8.query()).not.toBeInTheDocument(); + expect(ui.issueItem9.query()).not.toBeInTheDocument(); await act(async () => { await user.click(screen.getByRole('button', { name: 'show_more' })); }); - expect(ui.issueItems.getAll()).toHaveLength(10); - expect(ui.issueItem8.get()).toBeInTheDocument(); + expect(ui.issueItems.getAll()).toHaveLength(9); + expect(ui.issueItem9.get()).toBeInTheDocument(); }); it('should be able to select issues for bulk change', async () => { diff --git a/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx b/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx index 768480535f2..2a3f7907d4a 100644 --- a/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx +++ b/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx @@ -121,7 +121,6 @@ interface Props extends WithIndexationContextProps { } export interface State { bulkChangeModal: boolean; - cannotShowOpenIssue?: boolean; checkAll?: boolean; checked: string[]; effortTotal?: number; @@ -150,7 +149,8 @@ export interface State { selectedLocationIndex?: number; } -const MAX_INITAL_FETCH = 1000; +// When opening a specific issue, number of issues to fetch through pagination before loading it specifically +const MAX_INITAL_FETCH = 400; const VARIANTS_FACET = 'codeVariants'; const ISSUES_PAGE_SIZE = 100; @@ -469,32 +469,24 @@ export class App extends React.PureComponent { createdAfterIncludesTime = () => Boolean(this.props.location.query.createdAfter?.includes('T')); - fetchIssuesHelper = (query: RawQuery) => { + fetchIssuesHelper = async (query: RawQuery) => { if (this.props.component?.needIssueSync) { - return listIssues({ - ...query, - }).then((response) => { - const { components, issues, rules } = response; - - const parsedIssues = issues.map((issue) => - parseIssueFromResponse(issue, components, undefined, rules), - ); - - return { ...response, issues: parsedIssues } as FetchIssuesPromise; - }); + const response = await listIssues(query); + const parsedIssues = response.issues.map((issue) => + parseIssueFromResponse(issue, response.components, undefined, response.rules), + ); + return { ...response, issues: parsedIssues } as FetchIssuesPromise; } - return searchIssues({ + const response = await searchIssues({ ...query, additionalFields: '_all', timeZone: Intl.DateTimeFormat().resolvedOptions().timeZone, - }).then((response) => { - const parsedIssues = response.issues.map((issue) => - parseIssueFromResponse(issue, response.components, response.users, response.rules), - ); - - return { ...response, issues: parsedIssues } as FetchIssuesPromise; }); + const parsedIssues = response.issues.map((issue) => + parseIssueFromResponse(issue, response.components, response.users, response.rules), + ); + return { ...response, issues: parsedIssues } as FetchIssuesPromise; }; fetchIssues = ( @@ -549,123 +541,131 @@ export class App extends React.PureComponent { return this.fetchIssuesHelper(parameters); }; - fetchFirstIssues(firstRequest: boolean) { + async fetchFirstIssues(firstRequest: boolean) { const prevQuery = this.props.location.query; const openIssueKey = getOpen(this.props.location.query); - let fetchPromise; this.setState({ checked: [], loading: true }); + let response: FetchIssuesPromise; if (openIssueKey !== undefined) { - fetchPromise = this.fetchIssuesUntil(1, (pageIssues: Issue[], paging: Paging) => { - if ( - paging.total <= paging.pageIndex * paging.pageSize || - paging.pageIndex * paging.pageSize >= MAX_INITAL_FETCH - ) { - return true; - } - - return pageIssues.some((issue) => issue.key === openIssueKey); - }); + response = await this.fetchIssuesUntil(openIssueKey); } else { - fetchPromise = this.fetchIssues({}, true, firstRequest); + response = await this.fetchIssues({}, true, firstRequest); } - return fetchPromise.then(this.parseFirstIssues(firstRequest, openIssueKey, prevQuery), () => { + try { + return this.parseFirstIssues(response, firstRequest, prevQuery); + } catch (error) { if (this.mounted && areQueriesEqual(prevQuery, this.props.location.query)) { this.setState({ loading: false }); } - return []; - }); + } } - parseFirstIssues = - (firstRequest: boolean, openIssueKey: string | undefined, prevQuery: RawQuery) => - ({ effortTotal, facets, issues, paging, ...other }: FetchIssuesPromise) => { - if (this.mounted && areQueriesEqual(prevQuery, this.props.location.query)) { - const openIssue = getOpenIssue(this.props, issues); - let selected: string | undefined = undefined; + parseFirstIssues = (response: FetchIssuesPromise, firstRequest: boolean, prevQuery: RawQuery) => { + const { effortTotal, facets, issues, paging, ...other } = response; - if (issues.length > 0) { - selected = openIssue ? openIssue.key : issues[0].key; - } + if (this.mounted && areQueriesEqual(prevQuery, this.props.location.query)) { + const openIssue = getOpenIssue(this.props, issues); + let selected: string | undefined = undefined; - this.setState(({ showVariantsFilter }) => ({ - cannotShowOpenIssue: Boolean(openIssueKey) && !openIssue, - effortTotal, - facets: parseFacets(facets), - issues, - loading: false, - locationsNavigator: true, - openIssue, - paging, - referencedComponentsById: keyBy(other.components, 'uuid'), - referencedComponentsByKey: keyBy(other.components, 'key'), - referencedLanguages: keyBy(other.languages, 'key'), - referencedRules: keyBy(other.rules, 'key'), - referencedUsers: keyBy(other.users, 'login'), - selected, - selectedFlowIndex: 0, - selectedLocationIndex: undefined, - showVariantsFilter: firstRequest - ? Boolean(facets?.find((f) => f.property === VARIANTS_FACET)?.values.length) - : showVariantsFilter, - })); + if (issues.length > 0) { + selected = openIssue ? openIssue.key : issues[0].key; } - return issues; - }; + this.setState(({ showVariantsFilter }) => ({ + effortTotal, + facets: parseFacets(facets), + issues, + loading: false, + locationsNavigator: true, + openIssue, + paging, + referencedComponentsById: keyBy(other.components, 'uuid'), + referencedComponentsByKey: keyBy(other.components, 'key'), + referencedLanguages: keyBy(other.languages, 'key'), + referencedRules: keyBy(other.rules, 'key'), + referencedUsers: keyBy(other.users, 'login'), + selected, + selectedFlowIndex: 0, + selectedLocationIndex: undefined, + showVariantsFilter: firstRequest + ? Boolean(facets?.find((f) => f.property === VARIANTS_FACET)?.values.length) + : showVariantsFilter, + })); + } - fetchIssuesPage = (p: number) => { - return this.fetchIssues({ p }); + return issues; }; - fetchIssuesUntil = ( - page: number, - done: (pageIssues: Issue[], paging: Paging) => boolean, - ): Promise => { - const recursiveFetch = (p: number, prevIssues: Issue[]): Promise => { - return this.fetchIssuesPage(p).then(({ issues: pageIssues, paging, ...other }) => { - const issues = [...prevIssues, ...pageIssues]; - - // eslint-disable-next-line promise/no-callback-in-promise - return done(pageIssues, paging) - ? { issues, paging, ...other } - : recursiveFetch(p + 1, issues); - }); - }; + fetchIssuesUntil = async (issueKey: string): Promise => { + let issueOfInterest: Issue | undefined; + const allIssues: Issue[] = []; + + // Try and find issue of interest in the first pages of issues. Stop if we find it. + let currentPage = 1; + let lastResponse: FetchIssuesPromise; + do { + // eslint-disable-next-line no-await-in-loop + const response = await this.fetchIssues({ p: currentPage }); + allIssues.push(...response.issues); + lastResponse = response; + + issueOfInterest = response.issues.find((issue) => issue.key === issueKey); + if (issueOfInterest) { + return { ...response, issues: allIssues }; + } + + currentPage++; + } while (currentPage <= MAX_INITAL_FETCH / ISSUES_PAGE_SIZE); - return recursiveFetch(page, []); + // If we could not find the issue, we load it specifically and prepend it to the list + const { + issues: [issue], + } = await this.fetchIssuesHelper({ issues: issueKey }); + return { + ...lastResponse, + issues: [issue, ...allIssues], + // Use last paging object we got from the previous requests and patch it with this issue + paging: { + ...lastResponse.paging, + total: lastResponse.paging.total + 1, + }, + }; }; - fetchMoreIssues = () => { + fetchMoreIssues = async () => { const { paging } = this.state; if (!paging) { - return Promise.reject(); + throw new Error('Paging is not defined'); } - const p = paging.pageIndex + 1; - this.setState({ checkAll: false, loadingMore: true }); - return this.fetchIssuesPage(p).then( - (response) => { - if (this.mounted) { - this.setState((state) => ({ - issues: [...state.issues, ...response.issues], - loadingMore: false, - paging: response.paging, - })); - } - }, - () => { - if (this.mounted) { - this.setState({ loadingMore: false }); - } - }, - ); + try { + const response = await this.fetchIssues({ p: paging.pageIndex + 1 }); + + // In some cases, we can get an issue that we already have in the list as the first issue + // When this happens, we filter it out + // @see this.fetchIssuesUntil + const firstIssueKey = response.issues[0]?.key; + response.issues = response.issues.filter((issue) => issue.key !== firstIssueKey); + + if (this.mounted) { + this.setState((state) => ({ + issues: [...state.issues, ...response.issues], + loadingMore: false, + paging: response.paging, + })); + } + } catch (error) { + if (this.mounted) { + this.setState({ loadingMore: false }); + } + } }; fetchFacet = (facet: string) => { @@ -1082,7 +1082,8 @@ export class App extends React.PureComponent { {({ top }) => (