fix: do not reset txn if tab doesn't change

[dominictb]

Details

avoid txn in Onyx get reset after IOU confirmation page refresh

Fixed Issues

$ #40936
PROPOSAL: #40936 (comment)

Tests

1. Go to the app
2. Tap fab -- submit expense - scan
3. Upload an image using camera
4. Select a user
5. Enter description and save it
6. Refresh the page

Result: On refreshing scan submit expense confirmation page all details must not be disappeared

• Verify that no errors appear in the JS console

Offline tests

QA Steps

1. Go to the app
2. Tap fab -- submit expense - scan
3. Upload an image using camera
4. Select a user
5. Enter description and save it
6. Refresh the page

Result: On refreshing scan submit expense confirmation page all details must not be disappeared

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
• If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
  • I verified that all the inputs inside a form are aligned with each other.
  • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

compressed_android.webm.mp4

iOS: Native

iOS: mWeb Safari

compressed_simulator.mp4.mp4

MacOS: Chrome / Safari

compressed_web.mov.mp4

MacOS: Desktop

[melvin-bot]

@s77rt 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]

[s77rt]

@dominictb This fails if I refresh the page before processing to the submit expense flow

Screen.Recording.2024-06-14.at.12.15.26.AM.mov

[s77rt]

Now I can't get this to work at all 😅 Can you please investigate it

[dominictb]

@s77rt well, it works for me.

compressed_web.mov.mp4

[dominictb]

@s77rt if you can send me the Onyx details of the transactionDraft_1:

• before you reload in here: fix: do not reset txn if tab doesn't change #43503 (comment)
• after reload, the page go the scan page
• after you upload the image and click the Next button

that would be a bit help for me to investigate. Right now I'm stuck 'cause I couldn't reproduce what you just show

[s77rt]

@dominictb

1. Go to report A
2. Open FAB > Submit expense
3. Select a different user than the one on report A

After refresh this side effect will reset the money request

App/src/pages/iou/request/IOURequestStartPage.tsx

Lines 94 to 100 in bdb9789

	// Clear out the temporary expense if the reportID in the URL has changed from the transaction's reportID
	useEffect(() => {
	if (transaction?.reportID === reportID) {
	return;
	}
	IOU.initMoneyRequest(reportID, policy, isFromGlobalCreate, transactionRequestType.current);
	}, [transaction, policy, reportID, iouType, isFromGlobalCreate]);

Screen.Recording.2024-06-14.at.6.38.17.PM.mov

[dominictb]

@s77rt alright, give me some time. I'll get back to you in a short while about this.

[dominictb]

@s77rt below is my proposal to fix this issue. You can see the code change in the PR

Proposal

Issue

#43503 (comment)

RCA

If the transaction is created globally (via FAB for example. The full condition can be found here), the transaction.reportID can different from the route.params.reportID in the confirmation page, as we have to select the participants in the IOURequestStepParticipants, which determines the route.params.reportID when we move to confirmation step. We can see here that we don't set the transaction.reportID when we select participant.

Solutions:

• One option is that we set the transaction.reportID when we select participant. This leads to a problem that the IOURequestStartPage is not unmounted yet (stay in un-focused mode), i.e, in here, route.params.reportID will be the old reportID (not the newest one from the participant selection step) while transaction.reportID is a latest reportID value -> Causing the IOU.initMoneyRequest to run and reset the whole transaction again
• A complete solution is that we will cache the route.params.reportID in confirmation step in the transactionDraft Onyx collection (let's say the field is reportIDFromConfirmationStep). In here, if the transaction.reportIDFromConfirmationStep === route.params.reportID, we just need to update the transaction.reportID to route.params.reportID (and set the reportIDFromConfirmationStep to null)

Result:

compressed_web.mov.mp4

[s77rt]

@dominictb

This leads to a problem that the IOURequestStartPage is not unmounted yet (stay in un-focused mode), i.e, in here, route.params.reportID will be the old reportID (not the newest one from the participant selection step) while transaction.reportID is a latest reportID value -> Causing the IOU.initMoneyRequest to run and reset the whole transaction again

How will the side effect run if we won't re-render the screen?

[dominictb]

@s77rt, in that part I'm trying to explain why the first solution option didn't work (or if we can make it work, we will be exposed to high risk of regression since we have to change lots of code without knowing any side effects caused by the change). Just to be clear, it's not root cause explanation, in case you somehow think so 😄

So, if we are trying to update the transaction.reportID in the participant selection step before moving to confirmation step (which is one way to fix the root cause directly), this effect will run. That is because the IOURequestStartPage is not completely unmounted in the DOM tree, but it stays in blur mode, but the route props of that IOURequestStartPage is not populated with the latest reportID params as in the current action confirmation page, but with the old reportID. So, transaction.reportID !== reportID holds true in this case, and the draft transaction will be re-init.

We can continue this path further by checking the whether the IOURequestStartPage is focused, i.e: If not focused then don't re-init the transaction draft even transaction.reportID !== reportID holds true. But I realized that that will make things complicated and could easily cause regression

[s77rt]

@dominictb I noticed that the report id does not always change when a participant is selected. Can you investigate this and see if we can keep the same report id? (only the ones in the "Recent" section causes the change)

Screen.Recording.2024-06-18.at.7.05.06.PM.mov

[dominictb]

@s77rt I'll get back to you on this one shortly.

[dominictb]

@s77rt you can see here the all items in the section Contacts doesn't have reportID assoicated with that, so in here the firstParticipantReportID will be empty -> we'll use the reportID params instead. Hence is what you see.

There's no easy solution that directly address this. There are 2 options we can explore:

• When selecting the recent participant, update the transaction.reportID. Again, as explained above, it would cause the draft transaction to be unexpectedly reset
• When move from the participant selection step to confirmation step, we should keep the reportID param intact in the route. However, we'll have to: 1. persist the correct reportID param in the Onyx storage and 2. change the way we fetch the report in the withWritableReportOrNotFound component, since the correct reportID is not in the route props but somewhere in Onyx storage. Actually, this solution resemble somewhat my initial solution, but I personally like my initial solution better, as it doesn't touch the majority of current implementation.

[s77rt]

@dominictb The current implemented solution is adding more complexity to the flow. I think we should either:

1. Push forward with the changes that come after setting transaction.reportID (e.g. replace useEffect with useFocusEffect, etc.)
2. Get rid the second useEffect that's causing the reinit, see what problem it's fixing and fix it in another way

[dominictb]

@s77rt alright, after checking around a bit I think I will go forward with the setting transaction.reportID solution

[dominictb]

@s77rt done and test well. Please re-review, thanks!

[s77rt]

Bug: After selecting a participant and going back the money request is reset

Screen.Recording.2024-06-20.at.4.29.10.PM.mov

[dominictb]

Bug: After selecting a participant and going back the money request is reset

Alright, let me check this and let you know. Thanks!

[dominictb]

@s77rt ok I have figured out the solution:

• we will still replace useEffect with useFocusEffect as we are following fix: do not reset txn if tab doesn't change #43503 (comment).
• However, we won't set the transaction.reportID in the participant selection step like my previous suggestion as it will break stuff when user navigate back and forth between IOURequestStartPage, IOUParticipantSelectionPage, IOUConfirmationPage.
• I realize that when we reload the page, fail to read the receipt file from disk and navigating back to the start step, in case the transaction.reportID is different from the route.params.reportID, we are moving to the flow where the transaction is created inside the report instead of globally created like before. So, one idea is that: if we failed to read the receipt file from the disk and the transaction.reportID !== route.params.reportID , we will need to set the transaction.reportID to the route.params.reportID values before navigating.

Already updated the PR with the change.

[s77rt]

@dominictb Setting the transaction report id on participant selection is the right approach. Setting the report id only after refresh is a workaround. When going back can we use the transaction report id instead of the previous uri param report id?

[dominictb]

@s77rt after a closer look, probably going with setting transaction.reportID in the participant or in the confirmation step is a bad idea (apology from my side), because it will basically lose the optimistically generated reportID generated when initializing the global transaction creation flow, and it will seriously affect the participant selection logic.

To illustrate what I mean above, we are having a bug in here.

Bug report

1. Reproduction step (on staging)

• Open FAB, select Submit expense
• Select Manual tab, enter amount and navigate to the participant selection page
• Select any participant in the Recent section -> go to confirmation step (*)
• Reload the page
• Click on back button in the RHP header
• Select any participant in the Recent section -> go to confirmation step (**)
• Fill in the rest of the details and then submit

Expected: the expense should be submitted in the report that is selected in (**)
Actual: the expense submitted in the report in (*)

bug-8MB.mp4

1. RCA:

After reloading, the Navigate.goBack cannot find the previous route in the stack, so it will use the fallback route. However, the reportID value now is the reportID of the existing report TB workspace main, so if we select any participant in the Recent section, we'll still come back to the confirmation page with the same reportID, and even though in the To section of the confirmation page, it shows newly selected account, but during expense submission, the expense will be submitted in the existing report TB workspace main.

2. Solution: The fallback route which point to the participant selection step should not use the reportID params, instead, it should use transaction.reportID.

The point here, in the IOURequestStepParticipant, the route.params.reportID must be an optimistically generated reportID, not the existing reportID, or else we will encounter this bug. By changing transactionDraft.reportID value, we delete the optimistically generated reportID, so we will always get into the bug mentioned above, or the user will never be able to reselect the participant again if they somehow reload the confirmation page.

So, I believe the obvious option right now is the second option #43503 (comment) here. I'll update the code and explanation shortly so you can review

[dominictb]

Proposed solution:

In here, if the transaction is globally created from the start, we should not re-init the transaction.

In each IOURequestStepAmount, IOURequestStepDistance, IOURequestStepScan, in case the transaction is globally created, we should:

• Set the participants of the money request from the report, but do not set participantAutoAssigned = true for the IOU request
• always navigate to the confirmation step

[dominictb]

When going back can we use the transaction report id instead of the previous uri param report id

FYI, yes we can, but that means after re-selecting the receipt file we'll have to go through the participant selection step. Is that behavior acceptable or we should skip the participant selection step in this case?

[s77rt]

Bug: Stuck at participant selection page (have to press go back multiple times)

Screen.Recording.2024-06-24.at.1.01.56.PM.mov

[s77rt]

I'm afraid this is getting more complex than initially evaluated. Do you think we should close the PR and get back to the issue to construct a more solid proposal that covers the new found case

[dominictb]

I'm afraid this is getting more complex than initially evaluated. Do you think we should close the PR and get back to the issue to construct a more solid proposal that covers the new found case

@s77rt let's not close this PR. I think I'm on the right track to fix this issue. Just that I agreed that the scope has been doubled or even trippled. We should re-define the scope to see which are the buggy behaviors that we need to fix

#43503 (comment) probably this is my bad when trying to enforce the fallback navigation in IOURequestStepConfirmation. We'll need to confirm the exact scope so I could proceed with the proposal. Thanks!

[s77rt]

@dominictb The money request should not reset on going back to the start page. All the rest behaviour should be kept as is

[dominictb]

@s77rt alright, give me some time. I'll update and let you know!

[dominictb]

@s77rt closing this one out as it is growing lots of unnecessary commits. Let's review on my latest PR #44363