From 1fdfb79ddc7b99219ae7c271b27a30160f756a7b Mon Sep 17 00:00:00 2001 From: Wouter Admiraal <45544358+wouter-admiraal-sonarsource@users.noreply.github.com> Date: Tue, 24 Aug 2021 13:24:18 +0200 Subject: [PATCH] SONAR-13363 Prevent loading the component twice for nothing --- .../js/app/components/ComponentContainer.tsx | 99 ++++++++++++++----- .../__tests__/ComponentContainer-test.tsx | 69 +++++++++++-- server/sonar-web/src/main/js/types/tasks.ts | 4 +- 3 files changed, 139 insertions(+), 33 deletions(-) diff --git a/server/sonar-web/src/main/js/app/components/ComponentContainer.tsx b/server/sonar-web/src/main/js/app/components/ComponentContainer.tsx index 8453c7b8a62..ddf887c160e 100644 --- a/server/sonar-web/src/main/js/app/components/ComponentContainer.tsx +++ b/server/sonar-web/src/main/js/app/components/ComponentContainer.tsx @@ -42,7 +42,7 @@ import { } from '../../types/alm-settings'; import { BranchLike } from '../../types/branch-like'; import { ComponentQualifier, isPortfolioLike } from '../../types/component'; -import { Task, TaskStatuses, TaskWarning } from '../../types/tasks'; +import { Task, TaskStatuses, TaskTypes, TaskWarning } from '../../types/tasks'; import ComponentContainerNotFound from './ComponentContainerNotFound'; import { ComponentContext } from './ComponentContext'; import PageUnavailableDueToIndexation from './indexation/PageUnavailableDueToIndexation'; @@ -147,7 +147,7 @@ export class ComponentContainer extends React.PureComponent { loading: false }); - this.fetchStatus(componentWithQualifier); + this.fetchStatus(componentWithQualifier.key); this.fetchWarnings(componentWithQualifier, branchLike); this.fetchProjectBindingErrors(componentWithQualifier); } @@ -181,37 +181,30 @@ export class ComponentContainer extends React.PureComponent { return { branchLike, branchLikes }; }; - fetchStatus = (component: T.Component) => { - getTasksForComponent(component.key).then( + fetchStatus = (componentKey: string) => { + getTasksForComponent(componentKey).then( ({ current, queue }) => { if (this.mounted) { let shouldFetchComponent = false; this.setState( ({ branchLike, component, currentTask, tasksInProgress }) => { const newCurrentTask = this.getCurrentTask(current, branchLike); - const pendingTasks = this.getPendingTasks(queue, branchLike); - const newTasksInProgress = pendingTasks.filter( - task => task.status === TaskStatuses.InProgress + const pendingTasks = this.getPendingTasksForBranchLike(queue, branchLike); + const newTasksInProgress = this.getInProgressTasks(pendingTasks); + + shouldFetchComponent = this.computeShouldFetchComponent( + tasksInProgress, + newTasksInProgress, + currentTask, + newCurrentTask, + component ); - const currentTaskChanged = - (!currentTask && newCurrentTask) || - (currentTask && newCurrentTask && currentTask.id !== newCurrentTask.id); - const progressChanged = - tasksInProgress && - (newTasksInProgress.length !== tasksInProgress.length || - differenceBy(newTasksInProgress, tasksInProgress, 'id').length > 0); - - shouldFetchComponent = Boolean(currentTaskChanged || progressChanged); - if ( - !shouldFetchComponent && - component && - (newTasksInProgress.length > 0 || !component.analysisDate) - ) { + if (this.needsAnotherCheck(shouldFetchComponent, component, newTasksInProgress)) { // Refresh the status as long as there is tasks in progress or no analysis window.clearTimeout(this.watchStatusTimer); this.watchStatusTimer = window.setTimeout( - () => this.fetchStatus(component), + () => this.fetchStatus(componentKey), FETCH_STATUS_WAIT_TIME ); } @@ -277,7 +270,7 @@ export class ComponentContainer extends React.PureComponent { }; getCurrentTask = (current: Task, branchLike?: BranchLike) => { - if (!current) { + if (!current || !this.isReportRelatedTask(current)) { return undefined; } @@ -286,8 +279,64 @@ export class ComponentContainer extends React.PureComponent { : undefined; }; - getPendingTasks = (pendingTasks: Task[], branchLike?: BranchLike) => { - return pendingTasks.filter(task => this.isSameBranch(task, branchLike)); + getPendingTasksForBranchLike = (pendingTasks: Task[], branchLike?: BranchLike) => { + return pendingTasks.filter( + task => this.isReportRelatedTask(task) && this.isSameBranch(task, branchLike) + ); + }; + + getInProgressTasks = (pendingTasks: Task[]) => { + return pendingTasks.filter(task => task.status === TaskStatuses.InProgress); + }; + + isReportRelatedTask = (task: Task) => { + return [TaskTypes.AppRefresh, TaskTypes.Report, TaskTypes.ViewRefresh].includes(task.type); + }; + + computeShouldFetchComponent = ( + tasksInProgress: Task[] | undefined, + newTasksInProgress: Task[], + currentTask: Task | undefined, + newCurrentTask: Task | undefined, + component: T.Component | undefined + ) => { + const progressHasChanged = Boolean( + tasksInProgress && + (newTasksInProgress.length !== tasksInProgress.length || + differenceBy(newTasksInProgress, tasksInProgress, 'id').length > 0) + ); + + const currentTaskHasChanged = Boolean( + (!currentTask && newCurrentTask) || + (currentTask && newCurrentTask && currentTask.id !== newCurrentTask.id) + ); + + if (progressHasChanged) { + return true; + } else if (currentTaskHasChanged && component) { + // We return true if: + // - there was no prior analysis date (means this is an empty project, and + // a new analysis came in) + // - OR, there was a prior analysis date (non-empty project) AND there were + // some tasks in progress before + return ( + Boolean(!component.analysisDate) || + Boolean(component.analysisDate && tasksInProgress?.length) + ); + } + return false; + }; + + needsAnotherCheck = ( + shouldFetchComponent: boolean, + component: T.Component | undefined, + newTasksInProgress: Task[] + ) => { + return ( + !shouldFetchComponent && + component && + (newTasksInProgress.length > 0 || !component.analysisDate) + ); }; isSameBranch = (task: Pick, branchLike?: BranchLike) => { diff --git a/server/sonar-web/src/main/js/app/components/__tests__/ComponentContainer-test.tsx b/server/sonar-web/src/main/js/app/components/__tests__/ComponentContainer-test.tsx index 4b94887e4f4..d3e921899fe 100644 --- a/server/sonar-web/src/main/js/app/components/__tests__/ComponentContainer-test.tsx +++ b/server/sonar-web/src/main/js/app/components/__tests__/ComponentContainer-test.tsx @@ -31,7 +31,7 @@ import { mockAppState, mockComponent, mockLocation, mockRouter } from '../../../ import { waitAndUpdate } from '../../../helpers/testUtils'; import { AlmKeys } from '../../../types/alm-settings'; import { ComponentQualifier } from '../../../types/component'; -import { TaskStatuses } from '../../../types/tasks'; +import { TaskStatuses, TaskTypes } from '../../../types/tasks'; import { ComponentContainer } from '../ComponentContainer'; import PageUnavailableDueToIndexation from '../indexation/PageUnavailableDueToIndexation'; @@ -168,7 +168,7 @@ it('filters correctly the pending tasks for a main branch', () => { const branch2 = mockBranch({ name: 'branch-2' }); const pullRequest = mockPullRequest(); - expect(component.isSameBranch({} /*, undefined*/)).toBe(true); + expect(component.isSameBranch({})).toBe(true); expect(component.isSameBranch({}, mainBranch)).toBe(true); expect(component.isSameBranch({ branch: mainBranch.name }, mainBranch)).toBe(true); expect(component.isSameBranch({}, branch3)).toBe(false); @@ -182,19 +182,21 @@ it('filters correctly the pending tasks for a main branch', () => { const currentTask = mockTask({ pullRequest: pullRequest.key, status: TaskStatuses.InProgress }); const failedTask = { ...currentTask, status: TaskStatuses.Failed }; const pendingTasks = [currentTask, mockTask({ branch: branch3.name }), mockTask()]; - expect(component.getCurrentTask(currentTask, undefined)).toBeUndefined(); + expect(component.getCurrentTask(currentTask)).toBeUndefined(); expect(component.getCurrentTask(failedTask, mainBranch)).toBe(failedTask); expect(component.getCurrentTask(currentTask, mainBranch)).toBeUndefined(); expect(component.getCurrentTask(currentTask, pullRequest)).toMatchObject(currentTask); - expect(component.getPendingTasks(pendingTasks, mainBranch)).toMatchObject([{}]); - expect(component.getPendingTasks(pendingTasks, pullRequest)).toMatchObject([currentTask]); + expect(component.getPendingTasksForBranchLike(pendingTasks, mainBranch)).toMatchObject([{}]); + expect(component.getPendingTasksForBranchLike(pendingTasks, pullRequest)).toMatchObject([ + currentTask + ]); }); it('reload component after task progress finished', async () => { jest.useFakeTimers(); (getTasksForComponent as jest.Mock) .mockResolvedValueOnce({ - queue: [{ id: 'foo', status: TaskStatuses.InProgress }] + queue: [{ id: 'foo', status: TaskStatuses.InProgress, type: TaskTypes.ViewRefresh }] }) .mockResolvedValueOnce({ queue: [] @@ -237,7 +239,10 @@ it('reloads component after task progress finished, and moves straight to curren }); (getTasksForComponent as jest.Mock) .mockResolvedValueOnce({ queue: [] }) - .mockResolvedValueOnce({ queue: [], current: { id: 'foo', status: TaskStatuses.Success } }); + .mockResolvedValueOnce({ + queue: [], + current: { id: 'foo', status: TaskStatuses.Success, type: TaskTypes.AppRefresh } + }); const wrapper = shallowRender(); // First round, nothing in the queue, and component navigation was not called @@ -262,6 +267,56 @@ it('reloads component after task progress finished, and moves straight to curren expect(getTasksForComponent).toHaveBeenCalledTimes(3); }); +it('only fully loads a non-empty component once', async () => { + (getComponentData as jest.Mock).mockResolvedValueOnce({ + component: { key: 'bar', analysisDate: '2019-01-01' } + }); + (getTasksForComponent as jest.Mock).mockResolvedValueOnce({ + queue: [], + current: { id: 'foo', status: TaskStatuses.Success, type: TaskTypes.Report } + }); + const wrapper = shallowRender(); + + await waitAndUpdate(wrapper); + expect(getComponentNavigation).toHaveBeenCalledTimes(1); + expect(getTasksForComponent).toHaveBeenCalledTimes(1); +}); + +it('only fully reloads a non-empty component if there was previously some task in progress', async () => { + jest.useFakeTimers(); + (getComponentData as jest.Mock).mockResolvedValueOnce({ + component: { key: 'bar', analysisDate: '2019-01-01' } + }); + (getTasksForComponent as jest.Mock) + .mockResolvedValueOnce({ + queue: [{ id: 'foo', status: TaskStatuses.InProgress, type: TaskTypes.AppRefresh }] + }) + .mockResolvedValueOnce({ + queue: [], + current: { id: 'foo', status: TaskStatuses.Success, type: TaskTypes.AppRefresh } + }); + const wrapper = shallowRender(); + + // First round, a pending task in the queue. This should trigger a reload of the + // status endpoint. + await waitAndUpdate(wrapper); + jest.runOnlyPendingTimers(); + + // Second round, nothing in the queue, and a success task is current. This + // implies the current task was updated, and previously we displayed some information + // about a pending task. This new information must prompt the component to reload + // all data. + expect(getTasksForComponent).toHaveBeenCalledTimes(2); + + // Trigger the update. + await waitAndUpdate(wrapper); + // The component was correctly re-loaded. + expect(getComponentNavigation).toHaveBeenCalledTimes(2); + // The status API call will be called 1 final time after the component is + // fully loaded, so the total will be 3. + expect(getTasksForComponent).toHaveBeenCalledTimes(3); +}); + it('should show component not found if it does not exist', async () => { (getComponentNavigation as jest.Mock).mockRejectedValueOnce({ status: 404 }); const wrapper = shallowRender(); diff --git a/server/sonar-web/src/main/js/types/tasks.ts b/server/sonar-web/src/main/js/types/tasks.ts index 954918c2247..b23ae39d7f4 100644 --- a/server/sonar-web/src/main/js/types/tasks.ts +++ b/server/sonar-web/src/main/js/types/tasks.ts @@ -19,7 +19,9 @@ */ export enum TaskTypes { Report = 'REPORT', - IssueSync = 'ISSUE_SYNC' + IssueSync = 'ISSUE_SYNC', + AppRefresh = 'APP_REFRESH', + ViewRefresh = 'VIEW_REFRESH' } export enum TaskStatuses { -- 2.39.5