Repository navigation
Update Remaining functions of ReportWorkflow to remove getAllReportNameValuePairs() - #103475
parasharrajat wants to merge 8 commits into
Conversation
parasharrajat
changed the title
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
| // When the caller threads the archived state down | ||
| // Then no badge is returned because the archived chat blocks paying the invoice | ||
| expect(getBadgeFromIOUReport(iouReport, chatReport, policy, {}, invoiceReceiverPolicy, RORY_EMAIL, RORY_ACCOUNT_ID, undefined)).toBeUndefined(); | ||
| expect(getBadgeFromIOUReport(iouReport, chatReport, policy, {}, invoiceReceiverPolicy, RORY_EMAIL, RORY_ACCOUNT_ID, undefined, true)).toBeUndefined(); |
There was a problem hiding this comment.
🟡 Only getBadgeFromIOUReport gets tested with true. All 15 getIOUReportActionWithBadge calls in this file pass false, so nothing checks that the outer function actually passes isChatReportArchived on to the inner one. If someone changes L452 in ReportWorkflow.ts to a hardcoded false, typecheck still passes and no test fails.
Could we add one case in the getIOUReportActionWithBadge describe, reusing the invoice setup from this test ?
it('should not return a PAY badge when the invoice chat is archived', async () => {
// Given an invoice chat with a payable invoice preview
// ...same setup as the invoice test above, plus the REPORT_PREVIEW action in chatReportActions
// When the caller threads the archived state down
const result = getIOUReportActionWithBadge(chatReport, policy, {}, invoiceReceiverPolicy, RORY_EMAIL, RORY_ACCOUNT_ID, chatReportActions, true);
// Then the archived chat blocks paying, so no candidate action or badge is returned
expect(result.reportAction).toBeUndefined();
expect(result.actionBadge).toBeUndefined();
});
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafariScreen.Recording.2026-10-08.at.15.25.21.mov |
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM - No blockers: the refactor doesn't change behavior. The 2 code-comments I dropped are non-blockers.
|
@ikevin127 Looks like the functions have been moved to a different module but I have already worked on it. I will update those ones in the other module. |
|
This requires recheck @ikevin127 as functions were moved to a different file and I reapplied changes. |
|
@parasharrajat Looks like 3 CIs are 🔴, please address them then tag me when ready for another pass. |
| currentUserAccountID: RORY_ACCOUNT_ID, | ||
| chatReportActions, | ||
| allViolations: undefined, | ||
| isChatReportArchived: true, |
There was a problem hiding this comment.
🟡 This only checks that true gives no badge, so it would also pass if the setup never produced a badge at all. For example, if createRandomReport(2) comes back with pendingFields.preview: delete, or the preview gets filtered out, the test still goes green.
Could we assert that the same setup gives PAY with false before flipping to true ? Then the test actually proves the param reaches the inner function:
// When the caller says the chat is not archived
// Then the invoice preview is payable, which proves the setup yields a badge
const notArchivedResult = getIOUReportActionWithBadge({...params, isChatReportArchived: false});
expect(notArchivedResult.actionBadge).toBe(CONST.REPORT.ACTION_BADGE.PAY);
// When the caller threads the archived state down
const result = getIOUReportActionWithBadge({...params, isChatReportArchived: true});
| initialKeyStates: { | ||
| [ONYXKEYS.SESSION]: {accountID: RORY_ACCOUNT_ID, email: RORY_EMAIL}, | ||
| [ONYXKEYS.PERSONAL_DETAILS_LIST]: {[RORY_ACCOUNT_ID]: {accountID: RORY_ACCOUNT_ID, login: RORY_EMAIL}}, | ||
| [ONYXKEYS.PERSONAL_DETAILS_LIST]: { |
There was a problem hiding this comment.
🟡 About half of this file's diff is formatting-only changes to tests this PR doesn't touch. Objects like {accountID: RORY_ACCOUNT_ID, login: RORY_EMAIL} and transactionParams: {amount, currency, reportID} were split onto multiple lines.
Our .oxfmtrc.json uses printWidth: 190, so npm run fmt didn't do this. It looks like an IDE formatter with a narrower width. It makes blame noisy for everyone who later touches these tests and hides the real change in the diff.
Could we revert the hunks that only reformat and keep the call-signature updates plus the new test ?
ikevin127
left a comment
ikevin127
left a comment
There was a problem hiding this comment.
🟢 LGTM
My previous comments
ReportUtils.ts(old L4699), "add a short comment so nobody reorders the early return": Addressed. The comment is at L5122, aboveisChatReportArchived: isReportArchived. The thread is resolved.ReportWorkflowTest.ts(old L6335), "add onegetIOUReportActionWithBadgecase withtrue": Partially addressed. The test was added at L6478, but it has no positive control (see below). The thread is still unresolved on GitHub, and outdated.- PR description, "
Explanation of Changeis empty": Not addressed. It's still blank.
Only had 2 comments regarding testing and formatting - non blockers.
This branch has not been deployed
3 participants
Explanation of Change
Fixed Issues
$ #66518
PROPOSAL:
Tests
Offline tests
NA
QA Steps
Same as tests.
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))npm run compress-svg)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.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
08.10.2026_19.31.57_REC.mp4