[No QA] Implement MoveIOUReportToPolicyAndInviteSubmitter#58337
Conversation
…7-moveIOUReportToPolicyAndInviteSubmitter
|
@dukenv0307 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] |
|
It's still on hold |
|
@dukenv0307 I think we can do a preliminary review of the code here, while we wait for the backend |
|
@ikevin127 Do we need to fill the test sections and screenshots? |
|
@dukenv0307 I asked a similar question regarding testing on a similar PR (#58078) where I'm reviewing:
The answer from CME was:
So I guess for now you'd only be doing:
|
src/libs/actions/Report.ts
Outdated
| cleanUpSetQueries[`${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${reportID}`] = null; | ||
| cleanUpSetQueries[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${reportID}`] = null; | ||
| }); | ||
| Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT, cleanUpMergeQueries); |
There was a problem hiding this comment.
Do we need cleanUpMergeQueries here since it's empty?
There was a problem hiding this comment.
Makes sense, I will delete the Onyx.mergeCollection and remove the unused cleanUpMergeQueries.
|
@ikevin127 what's the latest on this one? |
|
@luacmartins Looks like there's 1 conflict and I also have to apply this change requested by reviewer. But then what's next when it comes to advancing this PR ? I remember I asked you in the other issue where I'm reviewing, how do we test these PRs in order to verify that the implementation is correct and you said:
Is the BE PR available for ♻️ I will use the answer for this to review #58078 as well since you just tagged me there to prioritize the review. |
|
@ikevin127 yes, the API command is available. I think the test steps are something like this: Pre-requisite: a user (not member of any workspace), a workspace with an admin
|
…7-moveIOUReportToPolicyAndInviteSubmitter
|
@dukenv0307 I resolved the conflict and removed the unused Q:
A:
If you need any other clarifications feel free to tag @luacmartins and ask 👍 |
|
I'll pause review on this PR until #58078 is merged, since this PR depends on that |
|
✅ Done with the review on the other PR, @dukenv0307 feel free to use the same review method I used in the other PR to review this one 👍 |
src/libs/actions/Report.ts
Outdated
| * @param reportID - The ID of the IOU report to move | ||
| * @param policyID - The ID of the policy to move the report to | ||
| */ | ||
| function moveIOUReportToPolicyAndInviteSubmitter( |
There was a problem hiding this comment.
Please update/add the param names in the description
I'm stuck on this step, |
|
I think I'll pass the other params from within the function since we should already have them there. The initial (current) implementation is pretty much inspired from the other PR so I'll have to align it after the most recent updates 👍 |
…7-moveIOUReportToPolicyAndInviteSubmitter
…7-moveIOUReportToPolicyAndInviteSubmitter
|
@luacmartins 🟢 Merged the PR with latest changes after the other PR was merged and I also updated this PRs ♻️ Using your testing steps from #58337 (comment), when attempting the move by clicking on a workspace in the change workspace selector, UI wise it looks like it does what it should but the {
"jsonCode": 400,
"message": "400 Unique Constraints Violation",
"source": "auth-via-api",
"onyxData": [],
"requestID": "926252ad0fc6fb3c-SJC"
}and upon refreshing the UI (on WS Owner / Admin side) the changes are reverted - while on the User side nothing changes since the API call failed.
Possible reasons coming from the FE part of this PR are: // -> here I'm not sure we're supposed to pass the IOU's ownerAccountID when creating the Expense
const ownerAccountID = iouReport.ownerAccountID;
const expenseChatReportID = getPolicyExpenseChat(ownerAccountID, policyID)?.reportID; const parameters: MoveIOUReportToPolicyAndInviteSubmitterParams = {
iouReportID,
policyID,
policyExpenseChatReportID: expenseChatReportID,
// -> here not sure I'm passing the right policyExpenseCreatedReportActionID
policyExpenseCreatedReportActionID: expenseReport.parentReportActionID ?? String(CONST.DEFAULT_NUMBER_ID),
changePolicyReportActionID: changePolicyReportAction.reportActionID,
}; |
src/libs/actions/Report.ts
Outdated
| const ownerAccountID = iouReport.ownerAccountID; | ||
|
|
||
| // Create an optimistic policy expense chat for the submitter who's not a policy member | ||
| let optimisticExpenseChatReportID: string | undefined; |
There was a problem hiding this comment.
| let optimisticExpenseChatReportID: string | undefined; | |
| let optimisticPolicyExpenseChatReportID: string | undefined; |
src/libs/actions/Report.ts
Outdated
| const changePolicyReportAction = buildOptimisticChangePolicyReportAction(iouReport.policyID, policyID, true); | ||
| optimisticData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${optimisticExpenseChatReportID}`, |
This is a BE bug. I have a PR in review to fix it.
Yea, this is known and we're discussing the solution for it. I don't think we need to block this PR on it though |
…7-moveIOUReportToPolicyAndInviteSubmitter
|
@luacmartins Thanks for the clarifications and suggestions, applied them and here's the latest look: web.mov🟢 I think this is ready for review, please let me know! |
luacmartins
left a comment
There was a problem hiding this comment.
We shouldn't commit the changes to Mobile-Expensify
|
@ikevin127 try this diff, I've tested and things work well. I moved some things around since it made more sense to start by inviting the user to the policy, creating the workspace chat and then moving the report over. |
…7-moveIOUReportToPolicyAndInviteSubmitter
|
@luacmartins Thanks for the refactor diff, I applied it and things look good, couldn't find any issues 👍 |
luacmartins
left a comment
There was a problem hiding this comment.
@ikevin127 please remove the diff for Mobile-Expensify. You can do that by running git submodule update and committing the changes
🟢 Done! |
luacmartins
left a comment
There was a problem hiding this comment.
LGTM! All yours @dukenv0307
|
@ZhenjaHorbach I reviewed this PR before. Can I continue? |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-03-31.at.23.42.05.movAndroid: mWeb ChromeScreen.Recording.2025-03-31.at.23.27.50.moviOS: NativeScreen.Recording.2025-03-31.at.23.39.20.moviOS: mWeb SafariScreen.Recording.2025-03-31.at.23.31.34.movMacOS: Chrome / SafariScreen.Recording.2025-03-31.at.23.23.15.movMacOS: DesktopScreen.Recording.2025-03-31.at.23.47.41.mov |
|
✋ 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/luacmartins in version: 9.1.22-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.1.22-10 🚀
|

Explanation of Change
Implements this section of the #migrate: Simplified actions framework for all reports design doc.
Fixed Issues
$ #57472
PROPOSAL:
Tests
Pre-requisite: a user (not member of any workspace), a workspace with an admin.
/change-workspaceto the URL in order to open the workspace selector).If any other clarifications are needed, feel free to tag @luacmartins and ask 👍
Offline tests
QA Steps
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))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
web.mov
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop