[Simplified Actions] Implement DeleteAppReport #58020
[Simplified Actions] Implement DeleteAppReport #58020luacmartins merged 45 commits intoExpensify:mainfrom
Conversation
|
@allgandalf how's this PR going? This is starting to block other PRs. Let's please prioritize this or if you're unable to, let's find someone to continue the work. |
|
@allgandalf refactoring the code to just move the reportAction proved to be a big lift since there's a lot of code in App that relies on App marking the action as deleted and creating a new one, so we'll need to pass the optimistic reportActionIDs to the API to handle the creation of those actions. I'll report back on this on Monday |
|
@allgandalf I have a couple of PRs in review that add a This way, we can optimistically create the IOU reportActions when we move the transactions to the selfDM and the server will use the optimistic IDs to avoid the duplicate actions in App. |
|
@luacmartins thanks, will do!, i will update it for now, let me know once they are deployed, will test in real time |
|
@allgandalf I believe it should be deployed to production tomorrow. I'll keep you updated |
|
Was it??? |
|
One of the PRs is in staging, but the rest is available in production |
|
@allgandalf PRs are in production |
melvin-bot
bot
requested review from
DylanDylann
and removed request for
a team
|
@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] |
luacmartins
left a comment
luacmartins
left a comment
There was a problem hiding this comment.
@allgandalf let me know when this PR is ready for review. There are still a few places with commented code and hardcoded values
|
@luacmartins I'm getting the following response with the new api: {
"jsonCode": 400,
"message": "400 Unique Constraints Violation",
"source": "auth-via-api",
"onyxData": [],
"requestID": "930925447c4d4844-SJC"
}Can you check that request ID and let me know what's wrong? |
|
I'm gonna make this a draft while @allgandalf works on the necessary changes |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.1.51-0 🚀
|
MonilBhavsar
left a comment
MonilBhavsar
left a comment
There was a problem hiding this comment.
This PR probably caused this #62763
I think we need to update the system message copy if report get's deleted
|
Similar to Monil's comment, we are also having issues with the copy here when modifying those deleted expenses: #62722 |
|
I think this PR caused this issue - #62695 |
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.1.51-6 🚀
|
|
This PR caused this issue - #62797 |
|
We CP-d some code to fix recent issues but a couple more blockers had come up so we decided to place this feature behind a beta. We CP-d the code to put this behind a beta. The beta PR is this - #62828. So now are related issues should be non deploy blockers. Since it shouldn't affect production users. |
|
Thanks @chiragsalian. I'll look into the issues |
| onConfirm={() => { | ||
| setIsDeleteReportModalVisible(false); | ||
|
|
||
| deleteAppReport(moneyRequestReport?.reportID); |
There was a problem hiding this comment.
this line caused a bug - #63094 (comment)
we should have waiting for deleteAppReport to complete before navigating
| const transactionIDToReportActionAndThreadData: Record<string, TransactionThreadInfo> = {}; | ||
|
|
||
| Object.values(reportActionsForReport ?? {}).forEach((reportAction) => { | ||
| if (!ReportActionsUtils.isMoneyRequestAction(reportAction)) { |
There was a problem hiding this comment.
money request action also includes pay action.
--> causing this issue #64027
| // 4. Add UNREPORTEDTRANSACTION report action | ||
| const unreportedAction = buildOptimisticUnreportedTransactionAction(childReportID, reportID); | ||
|
|
||
| optimisticData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${childReportID}`, | ||
| value: {[unreportedAction.reportActionID]: unreportedAction}, | ||
| }); |
There was a problem hiding this comment.
This is missing successData and failureData to clear the pending action field or the action itself.
Coming from #65413
|
Coming from this #63358 , We didn't handle a case where when a held expense is moved to tracked expenses on deletion of report, the held status is not removed. |
| deletedTransaction: ({amount, merchant}: DeleteTransactionParams) => `deleted an expense on this report, ${merchant} - ${amount}`, | ||
| movedTransaction: ({reportUrl, reportName}: MovedTransactionParams) => `moved this expense to <a href="${reportUrl}">${reportName}</a>`, | ||
| unreportedTransaction: ({reportUrl, reportName}: UnreportedTransactionParams) => `removed this expense from <a href="${reportUrl}">${reportName}</a>`, | ||
| unreportedTransaction: 'moved this expense to your personal space', |
There was a problem hiding this comment.
This was missing a hyperlink so we updated it in #68586
There was a problem hiding this comment.
hey i think we didn't want a hyperlink at the time of the PR, so i don't this this PR would be the offending one, please update the C+ checklist accordingly 😄 c.c. @luacmartins
| }, | ||
| ); | ||
|
|
||
| // 4. Add UNREPORTEDTRANSACTION report action |
There was a problem hiding this comment.
We should avoid the deleted report action which caused an issue #68628
@parasharrajat we didn't have the functionality to delete report from the preview when this API was implemented , can you please try to find the correct offending PR :) thanks |
| // 8. Delete chat report preview | ||
| const reportActionID = report?.parentReportActionID; | ||
| const reportAction = allReportActions?.[`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`]; | ||
| const parentReportID = report?.parentReportID; | ||
|
|
||
| if (reportActionID) { | ||
| optimisticData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReportID}`, | ||
| value: { | ||
| [reportActionID]: null, | ||
| }, | ||
| }); | ||
|
|
||
| failureData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${parentReportID}`, | ||
| value: { | ||
| [reportActionID]: reportAction, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
@allgandalf Here the preview should have offline delete pattern.
There was a problem hiding this comment.
No i meant the delete action from the report preview (workspace chat view) wasn't implemented during this PR, so there was no way to test the reported bug :)
There was a problem hiding this comment.
@allgandalf Here the preview should have offline delete pattern.
We used to get redirected to self-DM (even in offline case) at the time of the PR AFAIR, so i highly doubt if this was the case
There was a problem hiding this comment.
We didn't handle the Optimistic Delete pattern for Report preview which caused #72666
There was a problem hiding this comment.
In the PR, we updated the code which was added in this PR.
There was a problem hiding this comment.
No i meant the delete action from the report preview (workspace chat view) wasn't implemented during this PR, so there was no way to test the reported bug :)
But the issue happens bcz we didn't handled offline case in the logic added in this PR.
13 participants
Explanation of Change
This PR introduces the functionality to delete the report and move all the unreported expenses to self-dm
Fixed Issues
$ #57466
PROPOSAL:
Tests
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop