[HOLD PR#34498] [$500] Bank account - Form is not saved with new changes after coming back to it

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.36
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4279485
Email or phone of affected tester (no customers): vdargentotest+020224@gmail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

Pre-requisite: user must be logged in and have created a workspace

1. Go to Workspaces > Select Workspace > Bank account
2. Start and follow the add BA flow with testing credentials until you reach de Beneficial owners step
3. Tap on the back button to go back to Personal information page
4. Change the first and last name
5. Tap on the back button again to go back to Company information page
6. Tap on Save & continue button

Expected Result:

The Personal information page should be edited with the last data entered

Actual Result:

The Personal information page is not edited with the last data entered

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6366536_1707025254927.Ntxz6054_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0114287c47132d2711
• Upwork Job ID: 1754201133193392128
• Last Price Increase: 2024-02-04

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0114287c47132d2711

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @s77rt (External)

[melvin-bot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[Krishna2323]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Bank account - Form is not saved with new changes after coming back to it

What is the root cause of that problem?

When we move to next step from RequestorStep, we call BankAccounts.updatePersonalInformationForBankAccount(payload) with draft values which save the values in the database. And in the code below we update the draft value using BankAccounts.updateReimbursementAccountDraft(getBankAccountFields(getFieldsForStep(draftStep))). The problem here is that we don't use useEffect to update draft value and due to this the draft values updates unnecessarily, can sets the draft values same as the backend data which was saved when we moved to next step from RequestorStep.

App/src/pages/ReimbursementAccount/RequestorStep.js

Line 111 in 0eb6d02

BankAccounts.updatePersonalInformationForBankAccount(payload);

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 213 to 217 in 0eb6d02

	// Update the data that is returned from back-end to draft value
	const draftStep = reimbursementAccount.draftStep;
	if (draftStep) {
	BankAccounts.updateReimbursementAccountDraft(getBankAccountFields(getFieldsForStep(draftStep)));
	}

What changes do you think we should make in order to solve the problem?

We need to use useEffect to update the draft values correctly and only when reimbursementAccount.draftStep changes.

Result

form_save.mp4

[abzokhattab]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Bank account - Form is not saved with new changes after coming back to it

What is the root cause of that problem?

currently when saving a step we set the draftStep here then inside the ReimbursementAccountPage we set the draft value to be the same as in the backend here

so when going back to a previous step and then clicking continue, the Oynx data gets updated to the backend data which is not needed in this case

What changes do you think we should make in order to solve the problem?

we should sync the draft step with the data returned from the backend only on goBack to a previous step

in this case we dont need the draftStep from oynx we can remove it including all of its uses. we just need to get the prev step and update their draft values before navigating back

so we need to remove this block

and here we should remove this line

and inside the goBack function; before navigating we should add:

      const currentStepIndex = _.findIndex(_.keys(CONST.BANK_ACCOUNT.STEP), (key) => CONST.BANK_ACCOUNT.STEP[key] === currentStep);
        if (currentStepIndex > 0) {
            const prevStep = CONST.BANK_ACCOUNT.STEP[_.keys(CONST.BANK_ACCOUNT.STEP)[currentStepIndex - 1]];
            BankAccounts.updateReimbursementAccountDraft(getBankAccountFields(getFieldsForStep(prevStep)));
        }

in the prev snippet i used the object keys array to get the prev step because we have the steps in order here

for better readability, we can create a function getPreviousStep that would decide the previous step given the current step.

alternatively

instead of using the CONST.BANK_ACCOUNT.STEP object,
we can use the usePrevious hook on the currentStep variable so everytime the var changes it will save the prev step

[s77rt]

@Krishna2323 Thanks for the proposal. Your RCA is not correct. The input id has to be unique only per form and not per app since each form has its own onyx key (object for drafts).

[s77rt]

@abzokhattab Thanks for the proposal. Your RCA is not correct. Upon testing I found out that changes to the input after pressing the back button does not even make it to the draft values i.e. new changes are not saved as draft in the first place.

[Krishna2323]

@s77rt, proposal updated, also the screenshot is recorded after applying the new changes.

[s77rt]

@Krishna2323 Thanks for the update. Your RCA is about right. However the solution is not fixing the root cause. I think it would be better to update reimbursementAccount.draftStep when going back so that if you are editing the requestor info the draft step should be reset to the previous step i.e. CompanyStep. Can you please look into that?

[jeremy-croff]

@s77rt

Proposal

Please re-state the problem that we are trying to solve in this issue.

After navigating back in the bank account setup, the changed fields are not persisted unless you hit save and continue. If you press back again, your changes are lost.

What is the root cause of that problem?

The current draft value logic is wonky!
We are actually overwriting the draft each time, with the current reimbursement account data:

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Line 178 in 7f4cdce

..._.pick(lodashGet(reimbursementAccount, 'achData'), ...fieldNames),

When we update the draft with the logic here.

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Line 214 in 0eb6d02

const draftStep = reimbursementAccount.draftStep;

However, at the moment reimbursementAccount.draftStep is always 1 screen behind. If you are on the Requestor page, reimbursementAccount.draftStep says Company page. This is because it doesn't update until we hit 'Save and continue' and the service returns us back the previous step to merge into fromOnyx.

The way this allows the draft data to be saved in the happy paths, is that it's not overriding the draft in

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Line 216 in 0eb6d02

BankAccounts.updateReimbursementAccountDraft(getBankAccountFields(getFieldsForStep(draftStep)));

because when it calls getFieldsForStep it gets the fields for the previous screen, which doesn't override the current draft screen values. It looks like it works.

However, when you hit the back button, now the draftStep is actually also the current route, and getFieldsForStep gets the correct fields for the current screen. And it will keep overwriting the onyx draft data you enter with the latest saved onyx data. You can get out of this step by going back one screen, and then hitting save and continue again to let the service update reimbursementAccount.draftStep to the previous screen.

What changes do you think we should make in order to solve the problem?

I believe the best logic is to not maintain a reimbursementAccount.draftStep, and instead derive the step from the reimbursementAccount.achData.currentStep to see if we want to update the current data as a draft. Then update the logic to actually just merge in reimbursementAccountDraft into the reimbursementAccount as we persist it on reimbursementAccountDraft in Onyx - our data and state variables are clean and aligned.

function getBankAccountFields(fieldNames) {
+      const mergedDraft = {...reimbursementAccount.achData, ...reimbursementAccountDraft};
        return {
+          ..._.pick(lodashGet(mergedDraft), ...fieldNames),
        };
 }

What alternative solutions did you explore? (Optional)

If we want to keep the reimbursementAccount.draftStep pattern being behind 1 screen, we can ensure the back button updates the reimbursementAccount.draftStep to the previous screen. We will need to created an ordered list in order to know what the previous screen is.

[Krishna2323]

@s77rt, this code works very well, can you pls try once again, I'm bit afraid to do major changes here, as it can cause regression.

-    // Update the data that is returned from back-end to draft value
-   // const draftStep = reimbursementAccount.draftStep;
-    // if (draftStep) {
-    //     BankAccounts.updateReimbursementAccountDraft(getBankAccountFields(getFieldsForStep(draftStep)));
-    // }

+    useEffect(() => {
+        const draftStep = reimbursementAccount.draftStep;
+        if (draftStep) {
+           BankAccounts.updateReimbursementAccountDraft(getBankAccountFields(getFieldsForStep(draftStep)));
+        }
+        // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [reimbursementAccount.draftStep]);

Update here:

App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js

Lines 213 to 217 in 0eb6d02

	// Update the data that is returned from back-end to draft value
	const draftStep = reimbursementAccount.draftStep;
	if (draftStep) {
	BankAccounts.updateReimbursementAccountDraft(getBankAccountFields(getFieldsForStep(draftStep)));
	}

[s77rt]

@jeremy-croff Thanks for the proposal. Your RCA is correct. Regarding the solution why would we need to merge the reimbursementAccountDraft with reimbursementAccount? That defeats the whole point of using back-end data as draft.

• New draft should be coming from reimbursementAccount only
• New draft coming from reimbursementAccount and reimbursementAccountDraft is same as just using reimbursementAccountDraft

PS: Please do not post code diffs

[s77rt]

@Krishna2323 I didn't mean that the solution is not working. I meant that it's not fixing the root cause i.e. a workaround.

PS: Please do not post code diffs

[jeremy-croff]

reimbursementAccountDraft

I chose to merge them onto eachother so that on initial render, draft gets created from reimbursementAccount. Then any user entered details get merged on top of it from the modified draft state in onyx.

This way draft is a complete object we can save and promote to reimbursement account on save and continue.

[s77rt]

@jeremy-croff

This way draft is a complete object

I don't see the need for this. The draft merge process does not require you to supply all the fields. Can you clarify why this is needed?

[jeremy-croff]

@jeremy-croff

This way draft is a complete object

I don't see the need for this. The draft merge process does not require you to supply all the fields. Can you clarify why this is needed?

I think you're eluding to a good point. And maybe my approach doesn't make sense after all.

I don't really understand why draft originally came from reimbursementAccount, and my solution followed that pattern with the changes merged on top of it to solve the bug.

However if theory crafting, If we only maintain what changed in draft, and not all what we see on screen (as we get from reimbursement account), there will always be possibility for inconsistency between the screen we intend to save and what's in the database. If we passed all the data, we would ensure what we intended to save is the latest saved.

My approach can still stand without merging onto eachother, and just maintaining draft instead like you pointed out. But then I want to dive deeper into understanding what the original intend was of merging the reimbursementAccount into draft.

[s77rt]

@jeremy-croff That logic was introduced by #28305 to fix #25996. As long as the new approach still fixes the linked issue then we should be fine

[Krishna2323]

@s77rt, why do you feel that my proposal doesn't fix the root cause, as far as I understood, the problem is unnecessarily updating the draft in onyx, we update draft even on every key stroke, and the draft should only be updated when reimbursementAccount.draftStep is updated, If thats correct I think my proposal still solves it correctly.

[abzokhattab]

Proposal is updated to sync the draft data only on going back

[jeremy-croff]

@jeremy-croff That logic was introduced by #28305 to fix #25996. As long as the new approach still fixes the linked issue then we should be fine

This is very helpful.
So the purpose of putting ReimbursementAccount in draft is so that service side formatting gets applied to the draft data as that it what we see on the screen. Making the state accurate on back button. I think my fix does not solve this use case, as it will overwrite the formatted data with the draft.

New thoughts:
We'd only want to override ReimbursementAccountDraft with ReimbursementAccount from the previous step (identified as draftStep), since that's what just got saved and can be return differently from service..
However we run into an issue where draftStep is an outdated variable, breaking this logic.

I observe two things:
Each keystroke is running this code to update ReimbursementAccountDraft(prevStep) with ReimbursementAccount. And it only should be ran if ReimbursementAccount changes. That's 1 optimization for reducing unecessary onyx merges.

Then the second thing:
the draftStep is outdated. How can we fix this elegantly? We need a way to return to previous draftStep on the goBack method. We'd use the existing BANK_ACCOUNT.STEP object keys like @abzokhattab pointed out.

[jeremy-croff]

Proposal

Please re-state the problem that we are trying to solve in this issue.

draft changes not saved on back button flows that don't have a Save and Continue step to fix the state.

What is the root cause of that problem?

ReimbursementAccount.draftStep becomes outdated on back buttons for screens without a Save step to fix the state.

What changes do you think we should make in order to solve the problem?

Updated ReimbursementAccount.draftStep in onyx, can invoke it from the screens goBack method.
Using the BANK_ACCOUNT.STEP object keys to determine draftStep.

What alternative solutions did you explore? (Optional)

Optimize updateReimbursementAccountDraft to only apply ReimbursementAccount to draft on ReimbursementAccount changes rather than being tied to the render cycle of the entire component.

[s77rt]

@Krishna2323

the problem is unnecessarily updating the draft in onyx, we update draft even on every key stroke, and the draft should only be updated when reimbursementAccount.draftStep is updated

This is a problem indeed but it's not the root problem. Updating the draft on every key stroke on its own is not a problem. It's a performance related issue but it's not our issue here.

The root cause here is that draftStep is stale (out-of-sync). If we get draftStep to have its correct value then even updating the drafts on every key stroke won't cause the bug.

[s77rt]

@abzokhattab Thanks for the update. Your RCA is correct. The solution looks good to me.

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

Triggered auto assignment to @mountiny, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[s77rt]

@jeremy-croff Thanks for the follow up. You and @abzokhattab have very similar solutions. I think its fair to go with @abzokhattab for having the first complete (working) proposal.

[jeremy-croff]

@jeremy-croff Thanks for the follow up. You and @abzokhattab have very similar solutions. I think its fair to go with @abzokhattab for having the first complete (working) proposal.

My first proposal had this as an alternate solution first, to update draftStep on goback. While @abzokhattab did edit it from a wrong proposal to the correct one after my proposal, instead of him posting a new one. However he seemed to be the first one to identify to be able to use BANK_ACCOUNT.STEP keys for order, or the usePrevious hook. Which I quite like that solution and it makes sense for him to be assigned on it just based on that.

[mountiny]

guys, I am sorry for not having a better process for this, but we will most likely tomorrow merge this big refactor for bank accounts #34498

I will hold this issue for that and then we can see if its still repro there

[melvin-bot]

@slafortune, @s77rt, @mountiny Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[s77rt]

Still on hold

[mountiny]

Still will come off hold soon

[s77rt]

#34498 is merged. Seems that the bug is still reproducible

Screen.Recording.2024-02-16.at.6.42.19.PM.mov

[mountiny]

I will seem more advice, I am not sure if this is actually bug 😢 the flow clearly sets expectation to save the data in the backend with the Save button.

This feels like convoluted flow to me and we are fine with keeping the behaviour as is

[trjExpensify]

Yeah, from the doc that introduced these changes, it's only expected to save at each of the confirm steps:

User entered data is updated to the database at each of the “confirm” steps so leaving the flow before reaching the confirmation page of that specific step will cause the user to lose the data they entered thus far in that step

@kevinksullivan @joekaufmanexpensify @anmurali - as these issues are being created by Applause, I suspect some updated regression test scripts need to be passed over to them to update/add/remove based on the new refactored flow. Who's taking care of that for the project?

[joekaufmanexpensify]

Agreed. I think the behavior here is a bit odd. But the order of steps taken to identify this behavior doesn't feel to me like something many/any customers will do in this flow. I don't expect many people will get to the first confirmation page, then go back and edit their name, and then not proceed with saving on the form. This feels like one where there'd be benefit to seeing if anyone actually experiences this.

[joekaufmanexpensify]

as these issues are being created by Applause, I suspect some updated regression test scripts need to be passed over to them to update/add/remove based on the new refactored flow. Who's taking care of that for the project?

I haven't been involved much in this project since helping with the initial design. My understanding is @anmurali has been leading it. Though, latest update in the issue makes it seem like the implementation is still in progress.

[mountiny]

I will go ahead and close this since I think this is expected and if we want to change this its more of a feature request.

I am sorry it was not caught earlier.

If anyone internal disagrees feel free to reopen

[trjExpensify]

Cool, thanks @joekaufmanexpensify. I'll comment on the tracker.