Refactor: Deprecate getPolicy (part 13)#81798
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!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e517e80707
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-02-09.at.18.35.48.mov |
| const reviewDuplicatesReport = allReports?.[`${ONYXKEYS.COLLECTION.REPORT}${reviewDuplicates?.reportID}`]; | ||
| const [policyCategories] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${getNonEmptyStringOnyxID(reviewDuplicatesReport?.policyID)}`, {canBeMissing: true}); | ||
| const compareResult = TransactionUtils.compareDuplicateTransactionFields(transaction, allDuplicates, reviewDuplicatesReport, undefined, policyCategories); | ||
| const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`, {canBeMissing: true}); |
There was a problem hiding this comment.
🔴 Policy ID Source Inconsistency
The policy is fetched using report?.policyID, while policyCategories uses reviewDuplicatesReport?.policyID. These could be different reports with different policy IDs, causing a mismatch between the policy and policy categories used in the comparison.
Caution
All other Review* pages (ReviewBillable, ReviewCategory, etc.) correctly use reviewDuplicatesReport?.policyID for the policy fetch. Only Confirmation.tsx uses report?.policyID, creating an inconsistency.
Bug scenario: If report?.policyID !== reviewDuplicatesReport?.policyID, the function would compare categories from one policy against settings from another. This would cause:
- Categories being incorrectly filtered (shown/hidden) based on the wrong policy's areCategoriesEnabled flag
- Tax validation against the wrong policy's tax rates
- Tags being evaluated against the wrong policy's areTagsEnabled flag
Suggested fix: Use the same policy ID source reviewDuplicatesReport?.policyID for both, matching what the other Review* pages do:
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${getNonEmptyStringOnyxID(reviewDuplicatesReport?.policyID)}`, {canBeMissing: true});There was a problem hiding this comment.
@ikevin127 Can you tell me when this case will occur so I can write comment for this?
There was a problem hiding this comment.
@shubham1206agra I cannot tell, I did not reproduce a bug specifically - I just noticed the inconsistency during code review:
- all other edited components within
src/pages/TransactionDuplicate/*dir usereviewDuplicatesReport?.policyIDto getpolicy:
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${getNonEmptyStringOnyxID(reviewDuplicatesReport?.policyID)}`, {canBeMissing: true});- only
src/pages/TransactionDuplicate/Confirmation.tsxusesreport?.policyIDto getpolicy:
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`, {canBeMissing: true});The AI reviewer caught this as well, but if you think this is not an issue / won't cause a bug then it's ok to ignore ✅
There was a problem hiding this comment.
All other review pages use const [policy] = useOnyx(${ONYXKEYS.COLLECTION.POLICY}${getNonEmptyStringOnyxID(reviewDuplicatesReport?.policyID)}, {canBeMissing: true});
So I think this may be valid.
@tgolen could you confirm?
There was a problem hiding this comment.
@justinpersaud I am not seeing why it should be different in the first place, as all the duplicates come from the same report, which would have the same policyID, no matter which transaction.
There was a problem hiding this comment.
I think it would be best to always use report.policyID when possible. However, there are a few things to note:
- The subscriptions aren't changing in this PR, so this PR isn't going to cause any new regression (the existing subscription was just moved to a different place)
- I'd be a fan of trying to standardize this component to use
report.policyIDfor getting the categories, but this should probably be done in a separate PR
In short, I think this PR is fine as-is.
This comment was marked as resolved.
This comment was marked as resolved.
|
@ikevin127 I am not able to repro the bug you mentioned. Can you please check again? |
Checked, doesn't happen anymore after merge w/ main - must've come from older main which was fixed in the meantime ✅ |
|
✋ 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/justinpersaud in version: 9.3.17-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.17-9 🚀
|
Explanation of Change
Fixed Issues
$ #66397
Tests
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.2026-02-08.at.11.40.46.AM.mov