Show action badge in LHN for individual expense reports#88294
Show action badge in LHN for individual expense reports#88294AndrewGable merged 13 commits intomainfrom
Conversation
…reports Expense reports shown directly in the LHN were missing action badges because getIOUReportActionWithBadge only searched for REPORT_PREVIEW actions, which only exist in workspace chat reports. Added handling for expense reports to evaluate canIOUBePaid/canApproveIOU/canSubmitAndIsAwaitingForCurrentUser directly on the report itself. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
…ion filtering The 'should exclude deleted actions' test was passing an expense report directly to getIOUReportActionWithBadge, but the PR added a new early-return path for expense reports that returns reportAction: undefined. Updated the test to use a chat report with REPORT_PREVIEW actions pointing to a separate expense report, matching the pattern used in other tests. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Fixed the failing test The test was passing an expense report directly to |
| const iouReportID = getIOUReportIDFromReportActionPreview(iouReportActionToApproveOrPay); | ||
| const isExpenseReportOption = isExpenseReport(optionOrReport); | ||
| const iouReportID = isExpenseReportOption ? optionOrReport.reportID : getIOUReportIDFromReportActionPreview(iouReportActionToApproveOrPay); |
There was a problem hiding this comment.
@MelvinBot your solution is wrong (never reaches this line) because of early return here:
Lines 4302 to 4310 in ff9d4b7
As stated in isWaitingForAssigneeToCompleteAction, this issue happens only when you doesn't have access to workspace chat report (hasParentAccess = false).
Lines 4209 to 4215 in ff9d4b7
There was a problem hiding this comment.
You're right — when hasParentAccess === false, isWaitingForAssigneeToCompleteAction returns true for processing expense reports, so the badge computation was never reached.
Fixed by moving getIOUReportActionWithBadge above the isWaitingForAssigneeToCompleteAction check and including actionBadge in that early return path for expense reports.
…rly return When hasParentAccess is false, isWaitingForAssigneeToCompleteAction returns true for expense reports in processing state, causing an early return before the badge is computed. Move getIOUReportActionWithBadge call above this check and include the actionBadge in the early return path for expense reports. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
| // When the report is an expense report itself (not a workspace chat), check it directly | ||
| // since expense reports don't contain REPORT_PREVIEW actions | ||
| if (isExpenseReport(chatReport)) { | ||
| const parentChat = getReportOrDraftReport(chatReport?.chatReportID); | ||
| let expenseBadge: ValueOf<typeof CONST.REPORT.ACTION_BADGE> | undefined; | ||
| if ( | ||
| canIOUBePaid(chatReport, parentChat, policy, undefined, undefined, undefined, undefined, invoiceReceiverPolicy) || | ||
| canIOUBePaid(chatReport, parentChat, policy, undefined, undefined, true, undefined, invoiceReceiverPolicy) | ||
| ) { | ||
| expenseBadge = CONST.REPORT.ACTION_BADGE.PAY; | ||
| } else if (canApproveIOU(chatReport, policy, reportMetadata)) { | ||
| expenseBadge = CONST.REPORT.ACTION_BADGE.APPROVE; | ||
| } else { | ||
| const isWaitingSubmitFromCurrentUser = canSubmitAndIsAwaitingForCurrentUser( | ||
| chatReport, | ||
| parentChat, | ||
| policy, | ||
| getReportTransactions(chatReport?.reportID), | ||
| getAllTransactionViolations(), | ||
| getCurrentUserEmail(), | ||
| getUserAccountID(), | ||
| getAllReportActions(chatReport?.reportID), | ||
| ); | ||
| if (isWaitingSubmitFromCurrentUser) { | ||
| expenseBadge = CONST.REPORT.ACTION_BADGE.SUBMIT; | ||
| } | ||
| } | ||
| return {reportAction: undefined, actionBadge: expenseBadge}; | ||
| } |
There was a problem hiding this comment.
@MelvinBot this logic is duplicated. Extract this into getBadgeFromIOUReport function
There was a problem hiding this comment.
Extracted the shared Pay > Approve > Submit logic into getBadgeFromIOUReport. Both the expense report path and the REPORT_PREVIEW path now call it.
There was a problem hiding this comment.
Done — extracted into getBadgeFromIOUReport and deduplicated the logic in getIOUReportActionWithBadge.
The Pay > Approve > Submit badge computation was duplicated between the expense report path and the REPORT_PREVIEW path in getIOUReportActionWithBadge. Extract into a shared getBadgeFromIOUReport function. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
The Jest test failure ( |
| ); | ||
| } | ||
|
|
||
| function getBadgeFromIOUReport( |
There was a problem hiding this comment.
Added 3 unit tests for getBadgeFromIOUReport: APPROVE for submitted reports, PAY for approved reports, and undefined for settled reports.
There was a problem hiding this comment.
Added unit tests for getBadgeFromIOUReport: APPROVE for submitted reports, PAY for approved reports, and undefined for settled reports.
| return false; | ||
| } | ||
| const iouReport = getReportOrDraftReport(action.childReportID); | ||
| // Show to the actual payer, or to policy admins via the pay-elsewhere path for negative expenses |
There was a problem hiding this comment.
@MelvinBot restore this comment in new getBadgeFromIOUReport function
There was a problem hiding this comment.
Restored the comment: // Show to the actual payer, or to policy admins via the pay-elsewhere path for negative expenses
There was a problem hiding this comment.
Restored — the comment is at line 272 in the new getBadgeFromIOUReport function.
| if (isExpenseReport(chatReport)) { | ||
| const parentChat = getReportOrDraftReport(chatReport?.chatReportID); | ||
| return {reportAction: undefined, actionBadge: getBadgeFromIOUReport(chatReport, parentChat, policy, reportMetadata, invoiceReceiverPolicy)}; | ||
| } |
There was a problem hiding this comment.
@MelvinBot this is only when user doesn't have access to workspace chat.
Add this to if condition: && chatReport.hasParentAccess === false and pass undefined instead of parentChat as 2nd param of getBadgeFromIOUReport
There was a problem hiding this comment.
Done — added chatReport?.hasParentAccess === false to the condition and passing undefined instead of parentChat.
There was a problem hiding this comment.
Done — added chatReport?.hasParentAccess === false to the condition and passing undefined as the chatReport param to getBadgeFromIOUReport.
| ((optionOrReport.hasOutstandingChildRequest === true && !hasStaleChildRequest) || iouReportActionToApproveOrPay?.reportActionID) && | ||
| ((optionOrReport.hasOutstandingChildRequest === true && !hasStaleChildRequest) || iouReportActionToApproveOrPay?.reportActionID || (isExpenseReportOption && actionBadge)) && |
There was a problem hiding this comment.
@MelvinBot revert this line change as redundant as early returned already
There was a problem hiding this comment.
Reverted — removed (isExpenseReportOption && actionBadge) from the condition since the early return in isWaitingForAssigneeToCompleteAction now handles it.
There was a problem hiding this comment.
Already reverted in the previous commit — removed (isExpenseReportOption && actionBadge) from the condition since the expense report case is now handled by the isWaitingForAssigneeToCompleteAction early return.
…dd tests - Restore "Show to the actual payer" comment in getBadgeFromIOUReport - Scope expense report path to hasParentAccess === false and pass undefined for parentChat - Revert redundant (isExpenseReportOption && actionBadge) condition in getReasonAndReportActionThatRequiresAttention - Export getBadgeFromIOUReport and add unit tests for it Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
1 similar comment
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
| return { | ||
| reason: CONST.REQUIRES_ATTENTION_REASONS.IS_WAITING_FOR_ASSIGNEE_TO_COMPLETE_ACTION, | ||
| reportAction: Object.values(reportActions).find((action) => action.childType === CONST.REPORT.TYPE.TASK), | ||
| ...(isExpenseReportOption && actionBadge ? {actionBadge} : {}), |
There was a problem hiding this comment.
- define in CONST below this
ACTION_TYPES_FOR_ASSIGNEE_TO_COMPLETE: {
EXPENSE: 'expense',
TASK: 'task',
},
- refactor
isWaitingForAssigneeToCompleteActionfunction togetActionTypeForAssigneeToCompletewhich returns action type instead of boolean - Instead of
isExpenseReportOptioncondition, use this condition directly -actionTypeForAssigneeToComplete === CONST.REPORT.ACTION_TYPES_FOR_ASSIGNEE_TO_COMPLETE.EXPENSE
There was a problem hiding this comment.
Done. Renamed isWaitingForAssigneeToCompleteAction to getActionTypeForAssigneeToComplete which returns the action type (expense/task/undefined), added ACTION_TYPES_FOR_ASSIGNEE_TO_COMPLETE const, and replaced the isExpenseReportOption condition with actionTypeForAssigneeToComplete === CONST.REPORT.ACTION_TYPES_FOR_ASSIGNEE_TO_COMPLETE.EXPENSE.
…gneeToComplete Rename the function to return the specific action type (expense/task) instead of a boolean, and add ACTION_TYPES_FOR_ASSIGNEE_TO_COMPLETE const. Use the returned action type to conditionally include the badge in the early return path instead of relying on isExpenseReport. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
| * @param [parentReportAction] - The parent report action of the report (Used to check if the task has been canceled) | ||
| */ | ||
| function isWaitingForAssigneeToCompleteAction(report: OnyxEntry<Report>, parentReportAction: OnyxEntry<ReportAction>): boolean { | ||
| function getActionTypeForAssigneeToComplete( |
| @@ -4314,6 +4310,17 @@ function getReasonAndReportActionThatRequiresAttention( | |||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | |||
| const invoiceReceiverPolicy = invoiceReceiverPolicyID ? getPolicy(invoiceReceiverPolicyID) : undefined; | |||
| const {reportAction: iouReportActionToApproveOrPay, actionBadge} = getIOUReportActionWithBadge(optionOrReport, policy, optionReportMetadata, invoiceReceiverPolicy); | |||
There was a problem hiding this comment.
@MelvinBot For code optimization, move this line below if (actionTypeForAssigneeToComplete) condition.
And inside if (actionTypeForAssigneeToComplete) {} block, call this only when isAssigneeExpenseAction = true.
This way, we can avoid calling unnecessary getIOUReportActionWithBadge when assignee complete action type is 'task'.
There was a problem hiding this comment.
Done. Moved getIOUReportActionWithBadge below the actionTypeForAssigneeToComplete check. It's now only called inside the if block when isAssigneeExpenseAction is true, and otherwise called in the normal path below.
Move getIOUReportActionWithBadge below the actionTypeForAssigneeToComplete check and only call it when the action type is 'expense'. This avoids an unnecessary call when the assignee action is a task. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
…=false Verifies that getReasonAndReportActionThatRequiresAttention returns an actionBadge (APPROVE) when an expense report has hasParentAccess=false and is in processing state, ensuring the badge is included even when the early return path via getActionTypeForAssigneeToComplete is taken. Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com>
|
Added a unit test in |
|
…eForExpenseReports
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
Merged main and fixed prettier formatting. I wasn't able to mark the PR as ready for review due to permission restrictions — please mark it manually. |
|
@MelvinBot complete author checklist and mark as ready for review |
|
@MelvinBot add this to Tests / QA Steps. Precondition:
|
|
@MelvinBot check off missing items in author checklist |
|
🤖 Code Review The changes look correct for the stated goal. Summary: Correctness ✅
No regressions ✅
One pre-existing gap (not a regression from this PR):
Tests ✅
|
|
All items in the PR Author Checklist are already checked off — there are no unchecked items remaining. |
|
@MelvinBot these items are still missing in checklist |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Added the missing checklist items (non-English text / JaimeGPT and new assets verification). Both are checked since this PR doesn't add non-English text or new assets. |
|
Is this ready for review @aimane-chnaif? |
yes ready |
|
Conflicts |
|
@MelvinBot fix conflict |
…eForExpenseReports Co-authored-by: Aimane Chnaif <aimane-chnaif@users.noreply.github.com> # Conflicts: # src/libs/ReportUtils.ts
|
Merged
Typecheck passes. |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
| const expenseBadge = isAssigneeExpenseAction ? getBadgeFromIOUReport(optionOrReport, undefined, policy, optionReportMetadata, invoiceReceiverPolicy) : undefined; | ||
| return { | ||
| reason: CONST.REQUIRES_ATTENTION_REASONS.IS_WAITING_FOR_ASSIGNEE_TO_COMPLETE_ACTION, | ||
| reportAction: Object.values(reportActions).find((action) => action.childType === CONST.REPORT.TYPE.TASK), |
There was a problem hiding this comment.
@MelvinBot apply this change which came from #87166
There was a problem hiding this comment.
Applied the change from #87166 — the task reportAction lookup now filters by !isTaskCompleted(action) and childManagerAccountID === deprecatedCurrentUserAccountID, sorts by created, and picks the first match via .at(0).
Filter task report actions by completion status and current user,
and sort by creation date to pick the earliest incomplete task.
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
🤖 Code Review — PR #88294 Summary: The PR correctly adds action badge support for individual expense reports in the LHN by:
No issues found. The changes look correct and clean:
LGTM ✅ |
|
@AndrewGable done |
|
While I think the change is ok, it feels to me like an edge case, really, given that the user needs to "Open B's expense report from search router". I'll check with some other members of the product team before approving this one. |
|
FYI: this came from David's request |
|
Cool, thanks for the additional context here. I'll approve the PR. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @AndrewGable has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.3.62-0 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
The action badge (Approve/Pay/Submit) in the LHN was only showing for workspace chat reports because
getIOUReportActionWithBadgesearched exclusively forREPORT_PREVIEWactions, which only exist in workspace chats. When an individual expense report appeared directly in the LHN, the function found noREPORT_PREVIEWactions and returned no badge.This PR adds handling in
getIOUReportActionWithBadgeto detect when the report is an expense report itself (withhasParentAccess === false) and directly evaluatecanIOUBePaid/canApproveIOU/canSubmitAndIsAwaitingForCurrentUseron it, using the same priority order (Pay > Approve > Submit) as for workspace chats. The caller ingetReasonAndReportActionThatRequiresAttentionis also updated:isWaitingForAssigneeToCompleteActionis refactored intogetActionTypeForAssigneeToCompleteso the badge can be computed and included in the early return path for expense reports.Per the issue requirement, "Review duplicates" cases still get an Approve badge because
canApproveIOUreturns true regardless of duplicate state.Fixed Issues
$ #87712
PROPOSAL: #87712 (comment)
Tests
Precondition:
Offline tests
QA Steps
Precondition:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.npm run compress-svg)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
AI Tests
typecheck-tsgo): Passedlint-changed): Passed (no TS files changed per lint-changed; manual eslint run on changed files shows only pre-existing Onyx.connect deprecation warnings)ReportUtils,ReportWorkflow): Passed (877 + 42 = 919 tests)