Refactor: Deprecate getPolicy (part 5)#76881
Conversation
|
@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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Reviewer Checklist
Screenshots/VideosScreen.Recording.2025-12-06.at.17.45.45.mov |
|
|
||
| const policyIDsWithVBBA = useMemo(() => { | ||
| return Object.values(policies ?? {}) | ||
| .filter((policy): policy is Policy => !!policy?.achAccount?.bankAccountID) |
There was a problem hiding this comment.
🟡 Potentially Inconsistent VBBA Check Pattern
Concern: This check only verifies bankAccountID exists but doesn't verify the bank account's state. Looking at SearchUIUtils.ts:
// SearchUIUtils.ts line 585 - More complete check
const hasVBBA = !!policy.achAccount?.bankAccountID && policy.achAccount.state === CONST.BANK_ACCOUNT.STATE.OPEN;Bug Impact: A policy with a bank account in PENDING or VERIFYING state would incorrectly be considered as having a VBBA, potentially allowing payment options that shouldn't be available.
| .filter((policy): policy is Policy => !!policy?.achAccount?.bankAccountID) | |
| .filter((policy): policy is Policy => !!policy?.achAccount?.bankAccountID && policy?.achAccount?.state === CONST.BANK_ACCOUNT.STATE.OPEN) |
There was a problem hiding this comment.
@ikevin127 Is this a blocker for merging the PR?
There was a problem hiding this comment.
@tgolen Not a blocker since the old / removed logic didn't check for account state open either, but thought I should mention it as a potential due to existing logic having that check.
tgolen
left a comment
There was a problem hiding this comment.
Can you please add a unit test for the parameter change in getTagApproverRule()?
|
Product review not required. Removing myself and unsubscribing |
|
@tgolen Done |
tgolen
left a comment
There was a problem hiding this comment.
Can you please see if merging main will get the tests to pass?
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.2.79-0 🚀
|
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.2.81-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.81-5 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.81-5 🚀
|
Explanation of Change
Fixed Issues
$ #66397
Tests
Auto-pay approved reportstoggle is locked.Auto-pay approved reportstoggle is not locked.Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Screen.Recording.2025-12-06.at.10.53.38.AM.mov