Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

Android - BA - One moment loader appears after every step while entering Personal info #41847

Closed lanitochka17 closed 5 days ago

lanitochka17 commented 1 week ago

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.71-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+ck0205@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Precondition: Workspace created and enables Workflows in More features

  1. Launch ND app on Android device
  2. Go to Workspace settings >> Workflows >> select Connect Bank account
  3. Select Connect manually
  4. Enter Routing & Account number
  5. Enter first & last name as "First" & "Last" name & tap Next (Note: One moment loader appears before DOB page)
  6. Enter DOB ((Note: One moment loader appears before SSN page)
  7. Enter SSN ((Note: One moment loader appears before address page)
  8. Enter address (Note: One moment loader appears before confirm page)

Expected Result:

One moment loader should not appear after every step while entering personal info

Actual Result:

One moment loader appears after every step while entering Personal info DOB page appears briefly after providing FN/LN, then immediately one moment loader is seen & this happens after DOB, SSN & address steps

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/1056aba6-8797-47f7-bf8d-9c201dcad5b6

View all open jobs on GitHub

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 1 week ago

@MitchExpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 1 week ago

We think that this bug might be related to #wave-collect - Release 1

bernhardoj commented 1 week ago

Proposal

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

The one moment loader appears every time completing a sub-step of personal info. This happens in company/business info too.

What is the root cause of that problem?

This happens after this PR where we save each sub-step data. Every time we save a substep, we set the loading data to true. https://github.com/Expensify/App/blob/e0b6a06d2f34f35cda96ccc4d05252ede111c732/src/libs/actions/BankAccounts.ts#L137-L139

The loading data will then show the one moment loading indicator as shown below. https://github.com/Expensify/App/blob/e0b6a06d2f34f35cda96ccc4d05252ede111c732/src/pages/ReimbursementAccount/ReimbursementAccountPage.tsx#L391-L396

The reason this only happens in Android is because shouldReopenOnfido is true for Android but false for other platforms. The comment above explains why. https://github.com/Expensify/App/blob/e0b6a06d2f34f35cda96ccc4d05252ede111c732/src/libs/shouldReopenOnfido/index.android.ts#L3

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

If we are saving a sub-step data, don't set the loading data to true. We can pass the isConfirmPage from updatePersonalInformationForBankAccount to getVBBADataForOnyx. https://github.com/Expensify/App/blob/e0b6a06d2f34f35cda96ccc4d05252ede111c732/src/libs/actions/BankAccounts.ts#L286-L297

Then, set the loading state to true if isConfirmPage is true. https://github.com/Expensify/App/blob/e0b6a06d2f34f35cda96ccc4d05252ede111c732/src/libs/actions/BankAccounts.ts#L139

function getVBBADataForOnyx(currentStep?: BankAccountStep, shouldSetLoading = true): OnyxData {
    return {
        optimisticData: [
            {
                onyxMethod: Onyx.METHOD.MERGE,
                key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
                value: {
                    isLoading: shouldSetLoading,
                    errors: null,
                },
            },
        ],
MitchExpensify commented 1 week ago

We should probably wait for the performance issues to die down before resting this but I agree the copy is a bit weird regardless of the loader showing after every step. Even then this doesn't seem buggy to me, its just a loading pattern when we are loading/saving data

bernhardoj commented 1 week ago

@MitchExpensify hi, I believe this is a buggy behavior.

  1. This only happens in Android
  2. The loading appears after the next sub-step is shown which gives a bad UX. (as pointed out in the 2nd actual result)

I think the loading should only show when moving between steps 1 -> 2 just like it was before https://github.com/Expensify/App/pull/37680

MitchExpensify commented 1 week ago

Reopening to investigate further

bernhardoj commented 1 week ago

@MitchExpensify looks like it gets closed again unintentionally.

MitchExpensify commented 5 days ago

I reproduced this but I just don't feel like its worth fixing right now