Fix: Expense - Not here page shows up briefly when deleting the expense #52740
Conversation
…e..] briefly appeared
…x/45576-fix-not-found-decouple-2
…x/45576-fix-not-found-decouple-2
…ion during delete process
…x/45576-fix-not-found-decouple-2
…x/45576-fix-not-found-decouple-2
…t, to resolve not found page briefly appeared
…x/45576-fix-not-found-decouple-2
…x/45576-fix-not-found-decouple-2
…x/45576-fix-not-found-decouple-2
| value: { | ||
| reportID: null, | ||
| stateNum: CONST.REPORT.STATE_NUM.APPROVED, | ||
| statusNum: CONST.REPORT.STATUS_NUM.CLOSED, | ||
| participants: { | ||
| [userAccountID]: { | ||
| notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Why are we changing this? When the transaction is deleted we should delete the transaction thread, no?
There was a problem hiding this comment.
@luacmartins We switched from using SET to MERGE to prevent the appearance of the not found page due to platforms updating the value too quickly. This change has already been made for leaveRoom and leaveGroupChat to address a similar issue #49468.
There was a problem hiding this comment.
Thanks for clarifying. I think that makes sense, it just feels a bit odd to do that. Let's add a comment explaining why we're doing it so people don't try to revert this change
| if (shouldDeleteTransactionThread && transactionThread) { | ||
| successData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThreadID}`, | ||
| value: Object.keys(transactionThread).reduce<Record<string, null>>((acc, key) => { | ||
| acc[key] = null; | ||
| return acc; | ||
| }, {}), | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
@luacmartins Ensure that any remaining data is removed upon successful completion, even if the server sends a report removal response. This is done to prevent the removal update from lingering in the applyHTTPSOnyxUpdates function. #50846 (comment)
There was a problem hiding this comment.
Same here, let's add a comment about it. We should add a comment for all instances to make sure people don't revert this
| value: { | ||
| reportID: null, | ||
| stateNum: CONST.REPORT.STATE_NUM.APPROVED, | ||
| statusNum: CONST.REPORT.STATUS_NUM.CLOSED, | ||
| participants: { | ||
| [userAccountID]: { | ||
| notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN, | ||
| }, | ||
| }, | ||
| }, |
| if (shouldDeleteTransactionThread && transactionThread) { | ||
| successData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThreadID}`, | ||
| value: Object.keys(transactionThread).reduce<Record<string, null>>((acc, key) => { | ||
| acc[key] = null; | ||
| return acc; | ||
| }, {}), | ||
| }); | ||
| } | ||
|
|
Co-authored-by: Carlos Martins <[email protected]>
…ithub.com/wildan-m/App into wildan/fix/45576-fix-not-found-decouple-2
Co-authored-by: Carlos Martins <[email protected]>
Co-authored-by: Carlos Martins <[email protected]>
…ithub.com/wildan-m/App into wildan/fix/45576-fix-not-found-decouple-2
…x/45576-fix-not-found-decouple-2
| value: { | ||
| reportID: null, | ||
| stateNum: CONST.REPORT.STATE_NUM.APPROVED, | ||
| statusNum: CONST.REPORT.STATUS_NUM.CLOSED, | ||
| participants: { | ||
| [userAccountID]: { | ||
| notificationPreference: CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Thanks for clarifying. I think that makes sense, it just feels a bit odd to do that. Let's add a comment explaining why we're doing it so people don't try to revert this change
| if (shouldDeleteTransactionThread && transactionThread) { | ||
| successData.push({ | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: `${ONYXKEYS.COLLECTION.REPORT}${transactionThreadID}`, | ||
| value: Object.keys(transactionThread).reduce<Record<string, null>>((acc, key) => { | ||
| acc[key] = null; | ||
| return acc; | ||
| }, {}), | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same here, let's add a comment about it. We should add a comment for all instances to make sure people don't revert this
…x/45576-fix-not-found-decouple-2
There was a problem hiding this comment.
Thanks for addressing the change requests. LGTM. @alitoshmatov all yours
|
@wildan-m
I think this is too much, can't we just navigate to the final page instantly? Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-11-26.at.12.34.41.mp4Screen.Recording.2024-11-26.at.12.32.48.PM.mov |
…x/45576-fix-not-found-decouple-2
|
@alitoshmatov Thanks for your feedback I've got this loading error in dev and staging Seems BE? Kapture.2024-11-26.at.14.51.26.mp4I'll try to fix the issue after that BE issue resolved |
|
@wildan-m is that consistently reproducible? Do you have a requestID for a failing request? |
|
@luacmartins yes, it consistently reproduced, I think it's FE. Asking if anyone else has experienced the same. |
…x/45576-fix-not-found-decouple-2
|
@alitoshmatov can you retest with this new merged main? I can't reproduce your mentioned issue I can only reproduce the loading issue which is also occurred in staging. |
@alitoshmatov actually that behavior is current staging behavior, we can't see that clearly because it's deleting and navigation fast. If we throttle the CPU, we'll see it better IIRC the expected behavior if there is a comment when deleting expense is -- it should navigate to the existing expense page, not the parent report. @luacmartins do you know if the expected behavior changes? Kapture.2024-11-28.at.10.07.04.mp4Should we fix that here? edit: I've found the offending PR, this PR cause the change of behavior Also, I've noticed that isDeletedParentAction returned by
|
Explanation of Change
This PR will not here page briefly appears when deleting the expense by navigating to target url then perform actual delete when the report details unmounted.
Fixed Issues
$ #45576
PROPOSAL: #45576 (comment)
Tests
Offline tests
Same as test
QA Steps
Same as test
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
Kapture.2024-11-19.at.12.55.58.mp4
Android: mWeb Chrome
Kapture.2024-11-19.at.12.53.50.mp4
iOS: Native
Kapture.2024-11-19.at.11.07.05.mp4
iOS: mWeb Safari
Kapture.2024-11-19.at.11.13.30.mp4
MacOS: Chrome / Safari
Kapture.2024-11-19.at.11.10.45.mp4
MacOS: Desktop
Kapture.2024-11-19.at.12.39.18.mp4