Remove Onyx.connect() usage for ONYXKEYS.COLLECTION.POLICY_TAGS in requestMoney function from src/libs/actions/IOU/TrackExpense.ts#89084
Conversation
|
@parasharrajat 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] |
|
There are no changes except tests here. @Guccio163 is that correct |
|
I forgot to push yesterday 😳, already fixed 👍 |
|
Failing tests... |
| const isMoneyRequestReport = isMoneyRequestReportReportUtils(requestMoneyParams.report); | ||
| const moneyRequestReportID = isMoneyRequestReport ? requestMoneyParams.report?.reportID : undefined; | ||
| const currentChatReport = isMoneyRequestReport ? getReportOrDraftReport(requestMoneyParams.report?.chatReportID) : requestMoneyParams.report; | ||
| const isMovingTransactionFromTrackExpense = isMovingTransactionFromTrackExpenseIOUUtils(requestMoneyParams.action); | ||
| const parentChatReport = isMovingTransactionFromTrackExpense ? undefined : currentChatReport; | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| const policyTagList = getMoneyRequestPolicyTags({ | ||
| existingIOUReport: requestMoneyParams.existingIOUReport, | ||
| moneyRequestReportID, | ||
| parentChatReport, | ||
| participant: requestMoneyParams.participantParams.participant, | ||
| }); | ||
| requestMoneyParams.policyParams = {...(requestMoneyParams.policyParams ?? {}), policyTagList}; |
There was a problem hiding this comment.
Why are we passing these values here? You should set the params in retryParams on respective functions. Those values are passed here internally.
There was a problem hiding this comment.
Good idea, I thought we'll be migrating level-by-level with handleRetryPress and DotIndicatorMessage by the way, but I moved it right now; Skipping straight up to component can be tricky sometimes, so please take a look if the code looks good for you too 👀
There was a problem hiding this comment.
Actually React Complier check is failing, I'll let you know once it's fixed
…-fork into Guccio163/onyx-connect/policy_tags/requestMoney
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
| if (receiptError.action === CONST.IOU.ACTION_PARAMS.MONEY_REQUEST) { | ||
| const requestMoneyParams = receiptError.retryParams as IOU.RequestMoneyInformation; | ||
| requestMoneyParams.policyParams = {...(requestMoneyParams.policyParams ?? {}), policyTagList}; | ||
| } |
There was a problem hiding this comment.
I am still not getting this. Why do we need to set the retryparams here?
| const retryParams: IOU.RequestMoneyInformation | undefined = receiptError | ||
| ? typeof receiptError.retryParams === 'string' | ||
| ? (JSON.parse(receiptError.retryParams) as IOU.RequestMoneyInformation) | ||
| : (receiptError.retryParams as IOU.RequestMoneyInformation) | ||
| : undefined; |
There was a problem hiding this comment.
Why are you using retryParams here?
Retry params are supposed to use the same params that were sent to the original request, with the values passed during the original call.
So if you pass the params correctly to the `moneyRequest call, those should be available to you to assign to the retryparams in the function.
There was a problem hiding this comment.
You should set policTags on line 1624 in retryParams.
There was a problem hiding this comment.
Yeah you're right, I didn't address it yesterday as I was already finishing work 🏃♂️ I guess that every requestMoney usage (beside handleFileRetry) sets policyTagList in requestMoneyInformation so it will be automatically passed to retryParams and already available on handleFileRetry level, I'll remove this logic from there 🧹 Good catch @parasharrajat, I didn't notice that logic.
Please let me know if I conclude correctly 🤔
…-fork into Guccio163/onyx-connect/policy_tags/requestMoney
…-fork into Guccio163/onyx-connect/policy_tags/requestMoney
|
Kindly bump @parasharrajat |
|
Also @parasharrajat kindly bump here 👀 |
…-fork into Guccio163/onyx-connect/policy_tags/requestMoney
…-fork into Guccio163/onyx-connect/policy_tags/requestMoney
| const policyTagsForRequestMoney = useMoneyRequestPolicyTags({ | ||
| moneyRequestReportID: isIouReport ? report?.reportID : undefined, | ||
| parentChatReportPolicyID: isMovingTransactionFromTrackExpense ? undefined : report?.policyID, | ||
| participantReportID: participants?.at(0)?.reportID, |
There was a problem hiding this comment.
Are we sure if this is the correct participant? In the RequestMoney function below, we are passing the participant as a parameter.
There was a problem hiding this comment.
Looks like this is a similar case as in #90353, so I'll wait with this for that one
…-fork into Guccio163/onyx-connect/policy_tags/requestMoney
Explanation of Change
This PR is part of a refactor to remove Onyx.connect for the keys: ONYXKEYS.COLLECTION.POLICY_TAGS from the src/libs/actions/IOU/TrackExpense.ts file and replace it with useOnyx.
It isolates the
requestMoneyfunction from the ONYXKEYS.COLLECTION.POLICY_TAGS Onyx.connect key .Fixed Issues
$ #72721
PROPOSAL:
Tests
Test Steps
Setup: Sign in to a workspace that has at least one tag list configured (e.g.
Department: Engineering, Design) with tags enabled.Scenario 1 — Submit expense via confirmation page
Code path:
useExpenseSubmission.ts→requestMoneyTag1,Tag2).Scenario 2 — Submit expense directly from amount screen (skip confirmation)
Code path:
IOURequestStepAmount.tsx→requestMoney(triggered wheniouTypeisSUBMITorREQUESTand the confirmation screen is skipped)Scenario 3 — Convert tracked expense to submitted expense
Code path:
useExpenseSubmission.ts→requestMoneywithisMovingTransactionFromTrackExpense = trueTag1,Tag2).Scenario 4 — Duplicate an expense
Code path:
Duplicate.ts→createExpenseByType(default case) →requestMoneyScenario 5 — Retry a failed receipt upload
Code path:
handleFileRetry.ts→requestMoney(MONEY_REQUEST action)Scenario 6 — Submit expense via native Share extension, f.ex. iOS
Code path:
SubmitDetailsPage.tsx→requestMoneyTag1,Tag2).Offline tests
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
Scenario 1
scenario1.mov
Scenario 2a
scenario2a.mov
Scenario 2b
scenario2b.mov
Scenario 3
scenario3.mov
Scenario 4
scenario4.mov
Scenario 5
scenario5.mov
Scenario 6
scenario6.mov