Remove deprecated ReportUtils.getPolicy() method #39344
Conversation
|
@DylanDylann 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] |
|
@DylanDylann i just updated the code, please check again. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-04-02.at.15.47.15.movAndroid: mWeb ChromeScreen.Recording.2024-04-02.at.15.25.57.moviOS: NativeScreen.Recording.2024-04-02.at.15.46.32.moviOS: mWeb SafariScreen.Recording.2024-04-02.at.15.28.36.movMacOS: Chrome / SafariScreen.Recording.2024-04-02.at.15.24.06.movMacOS: DesktopScreen.Recording.2024-04-02.at.15.31.52.mov |
| @@ -30,7 +29,8 @@ function WorkspaceJoinUserPage({route, policies}: WorkspaceJoinUserPageProps) { | |||
| const styles = useThemeStyles(); | |||
| const policyID = route?.params?.policyID; | |||
| const inviterEmail = route?.params?.email; | |||
| const policy = ReportUtils.getPolicy(policyID); | |||
| const policy = useMemo(() => policies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`] ?? ({} as Policy), [policies, policyID]); | |||
There was a problem hiding this comment.
My first instinct was that this component doesn't need to connect to the entire policy collection. It looks like the only reason it connects to the entire collection is pass all policies to PolicyUtils.isPolicyMember(). All that method does is check that Onyx contains a policy with that ID (which doesn't necessarily mean the user is a member).
If that's all that isPolicyMember() is looking for, then it seems like isPolicyMember would be true as long as the policy object returned here is not an empty object.
Following that logic, then the full policy collection doesn't need to be subscribed to and isPolicyMember can be replaced with !isEmptyObject(policy).
There was a problem hiding this comment.
@tgolen Thanks your suggestions. I just updated code, please check again @DylanDylann
src/libs/actions/Policy.ts
Outdated
| @@ -665,7 +665,7 @@ function setWorkspaceApprovalMode(policyID: string, approver: string, approvalMo | |||
| } | |||
|
|
|||
| function setWorkspacePayer(policyID: string, reimburserEmail: string, reimburserAccountID: number) { | |||
| const policy = ReportUtils.getPolicy(policyID); | |||
| const policy = allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`] ?? ({} as Policy); | |||
There was a problem hiding this comment.
NAB: you could DRY all these up by creating a similar getPolicy() method and then also ensuring that getPolicy() isn't exported in the unit tests.
There was a problem hiding this comment.
@nkdengineer Also should do the same thing in src/libs/actions/IOU.ts
There was a problem hiding this comment.
Could you test again to make sure everything works well after these updates?
There was a problem hiding this comment.
@DylanDylann please check again, i updated code and tested well
There was a problem hiding this comment.
@nkdengineer Look good but getPolicy function is duplicated
@tgolen Should we create a utils function like this
function getPolicy(policyID: string | undefined, allPolicies): OnyxTypes.Policy | EmptyObject {
if (!allPolicies || !policyID) {
return {};
}
return allPolicies[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`] ?? {};
}
And export it to use in all files
There was a problem hiding this comment.
Heh, the balance of DRYness here is difficult to get right. As soon as you export that method, we're back in the same spot that this PR is attempting to clean up. So, I think the DRYness here is acceptable IMO.
There was a problem hiding this comment.
So, I think the DRYness here is acceptable IMO.
I realized right after I posted it that this might not be clear. I am suggesting that we leave the duplicated methods and that it is OK in this instance for it to not be DRY so that no one is tempted to use the exported method when they shouldn't.
|
@DylanDylann I'll give you a final 👍 👎 before I merge this since you had the question about DRYness. |
|
@tgolen It is not a problem. Let's merge this 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. |
|
Linking the Regression |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.60-13 🚀
|
Details
Fixed Issues
$ #38959
PROPOSAL: #38959 (comment)
Tests
submitworks normallyOffline tests
QA Steps
submitworks normallyPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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.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-resize.mp4
Android: mWeb Chrome
android-mweb-resize.mp4
iOS: Native
ios-resize.mp4
iOS: mWeb Safari
ios-mweb-resize.mp4
MacOS: Chrome / Safari
web-resize.mp4
MacOS: Desktop
desktop-resize.mp4