-
Notifications
You must be signed in to change notification settings - Fork 4k
Filter task search rows by live report status instead of the stale snapshot #100069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -438,6 +438,13 @@ const expenseStatusActionMapping: Record<string, ExpenseStatusPredicate> = { | |
| [CONST.SEARCH.STATUS.EXPENSE.DELETED]: (_expenseReport, transactionReportID) => transactionReportID === CONST.REPORT.TRASH_REPORT_ID, | ||
| }; | ||
|
|
||
| type TaskStatusPredicate = (taskReport?: OnyxTypes.Report | SearchTask) => boolean; | ||
|
|
||
| const taskStatusActionMapping: Record<string, TaskStatusPredicate> = { | ||
| [CONST.SEARCH.STATUS.TASK.OUTSTANDING]: (taskReport) => taskReport?.stateNum === CONST.REPORT.STATE_NUM.OPEN && taskReport.statusNum === CONST.REPORT.STATUS_NUM.OPEN, | ||
| [CONST.SEARCH.STATUS.TASK.COMPLETED]: (taskReport) => taskReport?.stateNum === CONST.REPORT.STATE_NUM.APPROVED && taskReport.statusNum === CONST.REPORT.STATUS_NUM.APPROVED, | ||
| }; | ||
|
|
||
| const nonSortableColumns = new Set<SearchColumnType>([ | ||
| CONST.SEARCH.TABLE_COLUMNS.RECEIPT, | ||
| CONST.SEARCH.TABLE_COLUMNS.TYPE, | ||
|
|
@@ -450,6 +457,10 @@ function isValidExpenseStatus(status: unknown): status is ValueOf<typeof CONST.S | |
| return typeof status === 'string' && status in expenseStatusActionMapping; | ||
| } | ||
|
|
||
| function isValidTaskStatus(status: unknown): status is ValueOf<typeof CONST.SEARCH.STATUS.TASK> { | ||
| return typeof status === 'string' && status in taskStatusActionMapping; | ||
| } | ||
|
|
||
| // Statuses a freshly created expense can never be in. The tracked optimistic item is kept visible | ||
| // before its server snapshot arrives, but it must not be force-shown under these terminal status | ||
| // filters (e.g. a brand-new "Draft" expense leaking into the "Deleted" tab). | ||
|
|
@@ -2165,6 +2176,32 @@ function isEligibleForStatus(currentQueryJSON: SearchQueryJSON | undefined, repo | |
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Whether a task still belongs under the active `status:` filter, judged against the live report rather than the | ||
| * search snapshot. Completing or reopening a task does not patch the snapshot, so without this a completed task | ||
| * lingers under `status:outstanding` (and vice versa) until the next server fetch. Mirrors `isEligibleForStatus`. | ||
| */ | ||
| function isEligibleForTaskStatus(currentQueryJSON: SearchQueryJSON | undefined, report: OnyxEntry<OnyxTypes.Report> | SearchTask) { | ||
| const status = getFilterFromQuery(currentQueryJSON, CONST.SEARCH.SYNTAX_FILTER_KEYS.STATUS); | ||
| if (!status.value) { | ||
| return true; | ||
| } | ||
|
|
||
| if (status.isNegated) { | ||
| return Object.keys(taskStatusActionMapping).some((taskStatus) => { | ||
| const isExcluded = status.value?.includes(taskStatus); | ||
| return !isExcluded && taskStatusActionMapping[taskStatus](report); | ||
| }); | ||
| } | ||
|
|
||
| // A status we don't model (e.g. a hand-typed `status:all`) must not silently empty the list, so leave the row visible. | ||
| if (!status.value.every(isValidTaskStatus)) { | ||
| return true; | ||
| } | ||
|
|
||
| return status.value.some((taskStatus) => taskStatusActionMapping[taskStatus](report)); | ||
| } | ||
|
|
||
| /** | ||
| * Whether the tracked optimistic (just-created) expense may be kept visible under the active status | ||
| * filter. A newly created expense can plausibly belong to "all", "unreported", "draft" or | ||
|
|
@@ -2729,8 +2766,10 @@ function getTaskSections( | |
| conciergeReportID: string | undefined, | ||
| reportNameValuePairs?: OnyxCollection<OnyxTypes.ReportNameValuePairs>, | ||
| reportAttributesDerivedValue?: OnyxTypes.ReportAttributesDerivedValue['reports'], | ||
| queryJSON?: SearchQueryJSON, | ||
| ): [TaskListItemType[], number] { | ||
| const {shouldShowYearCreated} = shouldShowYear(data); | ||
| const currentQueryJSON = queryJSON ?? getCurrentSearchQueryJSON(); | ||
| const tasks = Object.keys(data) | ||
| .filter(isReportEntry) | ||
| // Ensure that the reports that were passed are tasks, and not some other | ||
|
|
@@ -2761,6 +2800,10 @@ function getTaskSections( | |
| formattedCreatedBy, | ||
| keyForList: taskItem.reportID, | ||
| shouldShowYear: shouldShowYearCreated, | ||
| // The snapshot is not patched when a task is completed or reopened, so prefer the live report's status. | ||
| // Otherwise the row keeps rendering the Complete button instead of the Completed badge until a refetch. | ||
| statusNum: report.statusNum ?? taskItem.statusNum, | ||
| stateNum: report.stateNum ?? taskItem.stateNum, | ||
| }; | ||
|
|
||
| if (parentReport && personalDetails) { | ||
|
|
@@ -2795,7 +2838,10 @@ function getTaskSections( | |
| } | ||
|
|
||
| return result; | ||
| }); | ||
| }) | ||
| // Drop tasks whose live status no longer matches the active `status:` filter — the snapshot still lists a | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-16 (docs)This newly added comment uses an em dash ( Split it into two sentences instead: // Drop tasks whose live status no longer matches the active `status:` filter. The snapshot still lists a
// just-completed task under `status:outstanding` because `completeTask` never writes to it.
.filter((task) => isEligibleForTaskStatus(currentQueryJSON, task));Reviewed at: d9d0aad | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| // just-completed task under `status:outstanding` because `completeTask` never writes to it. | ||
| .filter((task) => isEligibleForTaskStatus(currentQueryJSON, task)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a paginated task search has more results, completing or reopening any loaded task makes this filter shrink Useful? React with 👍 / 👎. |
||
| return [tasks, tasks.length]; | ||
| } | ||
|
|
||
|
|
@@ -3997,7 +4043,7 @@ function getSections({ | |
| return [...getReportActionsSections(data, reportAttributesDerivedValue, visibleReportActionsData), false]; | ||
| } | ||
| if (type === CONST.SEARCH.DATA_TYPES.TASK) { | ||
| return [...getTaskSections(data, formatPhoneNumber, translate, conciergeReportID, reportNameValuePairs, reportAttributesDerivedValue), false]; | ||
| return [...getTaskSections(data, formatPhoneNumber, translate, conciergeReportID, reportNameValuePairs, reportAttributesDerivedValue, queryJSON), false]; | ||
| } | ||
|
|
||
| if (type === CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5751,6 +5751,231 @@ describe('SearchUIUtils', () => { | |
| expect(result.at(0)?.reportName).toBe('Task without concierge'); | ||
| }); | ||
|
|
||
| describe('task status filter uses the live report instead of the stale snapshot', () => { | ||
| const staleTaskReportID = 'task_report_700'; | ||
| const staleCreatorID = 121212; | ||
| const staleAssigneeID = 131313; | ||
|
|
||
| // The snapshot still says the task is open — this is exactly what `completeTask` leaves behind, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-16 (docs)This newly added comment uses an em dash ( // The snapshot still says the task is open. This is exactly what `completeTask` leaves behind,
// because it only writes to `report_<taskID>` and never patches `snapshot_<hash>`.Reviewed at: d9d0aad | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| // because it only writes to `report_<taskID>` and never patches `snapshot_<hash>`. | ||
| const snapshotTask = createMock<SearchTask>({ | ||
| type: CONST.REPORT.TYPE.TASK, | ||
| accountID: staleCreatorID, | ||
| reportID: staleTaskReportID, | ||
| reportName: 'Stale outstanding task', | ||
| description: 'Completed but still in the snapshot as open', | ||
| managerID: staleAssigneeID, | ||
| parentReportID: 'parent_stale', | ||
| stateNum: CONST.REPORT.STATE_NUM.OPEN, | ||
| statusNum: CONST.REPORT.STATUS_NUM.OPEN, | ||
| created: '2025-02-05 10:00:00', | ||
| }); | ||
|
|
||
| const staleTaskData = createMock<OnyxTypes.SearchResults['data']>({ | ||
| personalDetailsList: { | ||
| [staleCreatorID]: { | ||
| accountID: staleCreatorID, | ||
| avatar: '', | ||
| displayName: 'Stale Creator', | ||
| login: 'creator@test.com', | ||
| }, | ||
| [staleAssigneeID]: { | ||
| accountID: staleAssigneeID, | ||
| avatar: '', | ||
| displayName: 'Stale Assignee', | ||
| login: 'assignee@test.com', | ||
| }, | ||
| }, | ||
| [`report_${staleTaskReportID}`]: snapshotTask, | ||
| }); | ||
|
|
||
| const getStaleTaskSections = (query: string) => | ||
| getSectionsByType( | ||
| SearchUIUtils.getSections({ | ||
| dateFnsLocale: undefined, | ||
| type: CONST.SEARCH.DATA_TYPES.TASK, | ||
| data: staleTaskData, | ||
| currentAccountID: staleCreatorID, | ||
| currentUserEmail: 'creator@test.com', | ||
| translate: translateLocal, | ||
| formatPhoneNumber, | ||
| bankAccountList: {}, | ||
| conciergeReportID: '999', | ||
| convertToDisplayString, | ||
| reportAttributesDerivedValue: {}, | ||
| queryJSON: buildSearchQueryJSON(query), | ||
| }), | ||
| SearchUIUtils.isTaskListItemType, | ||
| ); | ||
|
|
||
| beforeEach(async () => { | ||
| // The live report has been completed, mirroring what `completeTask` merges into Onyx. | ||
| await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${staleTaskReportID}`, { | ||
| ...snapshotTask, | ||
| stateNum: CONST.REPORT.STATE_NUM.APPROVED, | ||
| statusNum: CONST.REPORT.STATUS_NUM.APPROVED, | ||
| }); | ||
| await waitForBatchedUpdates(); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${staleTaskReportID}`, null); | ||
| await waitForBatchedUpdates(); | ||
| }); | ||
|
|
||
| it('drops a completed task from the outstanding filter', () => { | ||
| const [result, count] = getStaleTaskSections('type:task status:outstanding'); | ||
|
|
||
| expect(result).toHaveLength(0); | ||
| expect(count).toBe(0); | ||
| }); | ||
|
|
||
| it('keeps a completed task in the completed filter and reports its live status', () => { | ||
| const [result, count] = getStaleTaskSections('type:task status:completed'); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(count).toBe(1); | ||
| expect(result.at(0)?.statusNum).toBe(CONST.REPORT.STATUS_NUM.APPROVED); | ||
| expect(result.at(0)?.stateNum).toBe(CONST.REPORT.STATE_NUM.APPROVED); | ||
| }); | ||
|
|
||
| it('keeps a still-open task in the outstanding filter', async () => { | ||
| await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${staleTaskReportID}`, { | ||
| stateNum: CONST.REPORT.STATE_NUM.OPEN, | ||
| statusNum: CONST.REPORT.STATUS_NUM.OPEN, | ||
| }); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| const [result, count] = getStaleTaskSections('type:task status:outstanding'); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(count).toBe(1); | ||
| expect(result.at(0)?.reportName).toBe('Stale outstanding task'); | ||
| }); | ||
|
|
||
| // A negated filter (`-status:completed`) keeps every status *except* the excluded ones, so it has to be | ||
| // judged against the live report too. `-` is the negation prefix and `status` is negatable, so these | ||
| // queries are reachable by typing them into the search router. | ||
| it('drops a completed task from a negated completed filter', () => { | ||
| const [result, count] = getStaleTaskSections('type:task -status:completed'); | ||
|
|
||
| expect(result).toHaveLength(0); | ||
| expect(count).toBe(0); | ||
| }); | ||
|
|
||
| it('keeps a completed task in a negated outstanding filter', () => { | ||
| const [result, count] = getStaleTaskSections('type:task -status:outstanding'); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(count).toBe(1); | ||
| expect(result.at(0)?.statusNum).toBe(CONST.REPORT.STATUS_NUM.APPROVED); | ||
| }); | ||
|
|
||
| it('keeps a still-open task in a negated completed filter', async () => { | ||
| await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${staleTaskReportID}`, { | ||
| stateNum: CONST.REPORT.STATE_NUM.OPEN, | ||
| statusNum: CONST.REPORT.STATUS_NUM.OPEN, | ||
| }); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| const [result, count] = getStaleTaskSections('type:task -status:completed'); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(count).toBe(1); | ||
| expect(result.at(0)?.statusNum).toBe(CONST.REPORT.STATUS_NUM.OPEN); | ||
| }); | ||
| }); | ||
|
|
||
| describe('reopening a task re-filters against the live report', () => { | ||
| const reopenedTaskReportID = 'task_report_701'; | ||
| const reopenedCreatorID = 141414; | ||
| const reopenedAssigneeID = 151515; | ||
|
|
||
| // Mirror image of the complete case: `reopenTask` writes only to `report_<taskID>`, so the snapshot is | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-16 (docs)This newly added comment uses an em dash ( // Mirror image of the complete case. `reopenTask` writes only to `report_<taskID>`, so the snapshot is
// left claiming the task is still completed.Reviewed at: d9d0aad | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| // left claiming the task is still completed. | ||
| const completedSnapshotTask = createMock<SearchTask>({ | ||
| type: CONST.REPORT.TYPE.TASK, | ||
| accountID: reopenedCreatorID, | ||
| reportID: reopenedTaskReportID, | ||
| reportName: 'Stale completed task', | ||
| description: 'Reopened but still in the snapshot as completed', | ||
| managerID: reopenedAssigneeID, | ||
| parentReportID: 'parent_reopened', | ||
| stateNum: CONST.REPORT.STATE_NUM.APPROVED, | ||
| statusNum: CONST.REPORT.STATUS_NUM.APPROVED, | ||
| created: '2025-02-06 10:00:00', | ||
| }); | ||
|
|
||
| const reopenedTaskData = createMock<OnyxTypes.SearchResults['data']>({ | ||
| personalDetailsList: { | ||
| [reopenedCreatorID]: { | ||
| accountID: reopenedCreatorID, | ||
| avatar: '', | ||
| displayName: 'Reopened Creator', | ||
| login: 'reopencreator@test.com', | ||
| }, | ||
| [reopenedAssigneeID]: { | ||
| accountID: reopenedAssigneeID, | ||
| avatar: '', | ||
| displayName: 'Reopened Assignee', | ||
| login: 'reopenassignee@test.com', | ||
| }, | ||
| }, | ||
| [`report_${reopenedTaskReportID}`]: completedSnapshotTask, | ||
| }); | ||
|
|
||
| const getReopenedTaskSections = (query: string) => | ||
| getSectionsByType( | ||
| SearchUIUtils.getSections({ | ||
| dateFnsLocale: undefined, | ||
| type: CONST.SEARCH.DATA_TYPES.TASK, | ||
| data: reopenedTaskData, | ||
| currentAccountID: reopenedCreatorID, | ||
| currentUserEmail: 'reopencreator@test.com', | ||
| translate: translateLocal, | ||
| formatPhoneNumber, | ||
| bankAccountList: {}, | ||
| conciergeReportID: '999', | ||
| convertToDisplayString, | ||
| reportAttributesDerivedValue: {}, | ||
| queryJSON: buildSearchQueryJSON(query), | ||
| }), | ||
| SearchUIUtils.isTaskListItemType, | ||
| ); | ||
|
|
||
| beforeEach(async () => { | ||
| // The live report has been reopened, mirroring what `reopenTask` merges into Onyx. | ||
| await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reopenedTaskReportID}`, { | ||
| ...completedSnapshotTask, | ||
| stateNum: CONST.REPORT.STATE_NUM.OPEN, | ||
| statusNum: CONST.REPORT.STATUS_NUM.OPEN, | ||
| }); | ||
| await waitForBatchedUpdates(); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| await Onyx.set(`${ONYXKEYS.COLLECTION.REPORT}${reopenedTaskReportID}`, null); | ||
| await waitForBatchedUpdates(); | ||
| }); | ||
|
|
||
| it('drops a reopened task from the completed filter', () => { | ||
| const [result, count] = getReopenedTaskSections('type:task status:completed'); | ||
|
|
||
| expect(result).toHaveLength(0); | ||
| expect(count).toBe(0); | ||
| }); | ||
|
|
||
| it('shows a reopened task under the outstanding filter with its live status', () => { | ||
| const [result, count] = getReopenedTaskSections('type:task status:outstanding'); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| expect(count).toBe(1); | ||
| expect(result.at(0)?.reportName).toBe('Stale completed task'); | ||
| expect(result.at(0)?.statusNum).toBe(CONST.REPORT.STATUS_NUM.OPEN); | ||
| expect(result.at(0)?.stateNum).toBe(CONST.REPORT.STATE_NUM.OPEN); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getReportSections computed fields (totalDisplaySpend, nonReimbursableSpend, reimbursableSpend, isAllScanning)', () => { | ||
| const testReportID = 'spend-test-report'; | ||
| const testTxID1 = 'spend-tx-1'; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ CONSISTENCY-3 (docs)
isEligibleForTaskStatusis a near line-for-line duplicate of the existingisEligibleForStatus: both read theSTATUSfilter, returntruewhen empty, branch onstatus.isNegatediteratingObject.keys(<mapping>).some(...)with anisExcludedcheck, then fall through tostatus.value.some(...). The accompanyingtaskStatusActionMapping/isValidTaskStatusalso mirrorexpenseStatusActionMapping/isValidExpenseStatus. The only real differences are the mapping object and the validator.Consider extracting the shared control flow into one helper parameterized by the mapping and validator, e.g.:
so
isEligibleForStatusandisEligibleForTaskStatusboth delegate to it instead of copying the branching.Reviewed at: d9d0aad | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.