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
3.48k stars 2.83k forks source link

[$250] Card reconciliation - Nothing happens after clicking on Reconciliation account #50498

Open IuliiaHerets opened 1 week ago

IuliiaHerets 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: 9.0.46-1 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/5057262 Email or phone of affected tester (no customers): applausetester+pso@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Precondition:

  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Accounting.
  3. Click Card reconciliation.
  4. Toggle on Continuous Reconciliation.
  5. Select an account.
  6. Click Reconciliation account.

Expected Result:

Reconciliation account should redirect to the relevant page.

Actual Result:

Nothing happens after clicking on Reconciliation account.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/1fb33c0c-6ea7-4b60-927c-d6672867dcb6

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021844844894523335302
  • Upwork Job ID: 1844844894523335302
  • Last Price Increase: 2024-10-11
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @johncschuster (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.

IuliiaHerets commented 1 week ago

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

IuliiaHerets commented 1 week ago

@johncschuster 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

abzokhattab commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-10-10 07:14:09 UTC.

Proposal

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

Card Reconciliation - Nothing happens after clicking on the Reconciliation Account.

What is the root cause of that problem?

In the reconciliation menu item, we have shouldShowRightIcon enabled, but there is no onPress action defined. https://github.com/Expensify/App/blob/f08c970b8738f445f201d0600d6e3e7cc5258d20/src/pages/workspace/accounting/reconciliation/CardReconciliationPage.tsx#L110-L116

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

We should add an onPress action that navigates to the account reconciliation page when clicked.

onPress={() => Navigation.navigate(ROUTES.WORKSPACE_ACCOUNTING_RECONCILIATION_ACCOUNT_SETTINGS.getRoute(policyID, connection))}

Optional: If we want to ensure the back navigation works correctly after a page refresh, we can extend ROUTES.WORKSPACE_ACCOUNTING_RECONCILIATION_ACCOUNT_SETTINGS to include a backTo parameter, as we've done on other pages. When navigating, we should pass the activeRoute to the backTo argument. Then, inside the Reconciliation Account Settings page, we can parse route.params to retrieve backTo and modify this line to goBack(backTo). This approach ensures that the user is always directed back to the correct original route.

image

What alternative solutions did you explore? (Optional)

Alternatively, we could remove shouldShowRightIcon if we want it to be non-interactive and use interactive={false}.

allgandalf commented 1 week ago

Proposal

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

RHP doesn't open when we click on Reconciliation Account

What is the root cause of that problem?

MenuItemWithTopDescription does not have onPress prop passed hence it doesn't get any functionality https://github.com/Expensify/App/blob/f08c970b8738f445f201d0600d6e3e7cc5258d20/src/pages/workspace/accounting/reconciliation/CardReconciliationPage.tsx#L110

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

We need to pass onPress prop to MenuItemWithTopDescription:

onPress={() => Navigation.navigate(ROUTES.WORKSPACE_ACCOUNTING_RECONCILIATION_ACCOUNT_SETTINGS.getRoute(policyID, connection))}

What alternative solutions did you explore? (Optional)

twilight2294 commented 1 week ago

Proposal

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

Card reconciliation - Nothing happens after clicking on Reconciliation account

What is the root cause of that problem?

We don't pass onPress in reconciliation menu, hence there is not navigation https://github.com/Expensify/App/blob/f08c970b8738f445f201d0600d6e3e7cc5258d20/src/pages/workspace/accounting/reconciliation/CardReconciliationPage.tsx#L110-L116

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

Pass onPress to to menuitem:

<MenuItemWithTopDescription
    onPress={() => Navigation.navigate(ROUTES.WORKSPACE_ACCOUNTING_RECONCILIATION_ACCOUNT_SETTINGS.getRoute(policyID, connection))}
/>

But this is not enough, in the same flow on the settings page, if we refresh the page and then press the back button, then the RHP will get dismissed instead of going back to the settings page, which will cause a regression,

To fix that, In ReconciliationAccountSettingsPage here, we should pass onBackButtonPress prop as well

        <ConnectionLayout
            onBackButtonPress={() => Navigation.goBack(ROUTES.WORKSPACE_ACCOUNTING_CARD_RECONCILIATION.getRoute(policyID,connection))}
        >

Also the bug will still happen if we refresh and click on any of the bank accounts as when we call selectBankAccount when we select a bank account:

https://github.com/Expensify/App/blob/3777223374152bb83333674e06b3497aaea27b81/src/pages/workspace/accounting/reconciliation/ReconciliationAccountSettingsPage.tsx#L58-L61

where we call goBack this will again call regression that the RHP will get dismissed instead of going to the settings page so we need to update it too:

    const selectBankAccount = (newBankAccountID?: number) => {
        Card.updateSettlementAccount(workspaceAccountID, policyID, newBankAccountID, paymentBankAccountID);
        Navigation.goBack(ROUTES.WORKSPACE_ACCOUNTING_CARD_RECONCILIATION.getRoute(policyID,connection));
    };

What alternative solutions did you explore? (Optional)

abzokhattab commented 1 week ago

Proposal updated

Covered the back to navigation in case we want to cover this point as well.

melvin-bot[bot] commented 6 days ago

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

melvin-bot[bot] commented 6 days ago

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

johncschuster commented 6 days ago

Not overdue. Just triaged

allgandalf commented 6 days ago

I guess this one is only for the C+ team, @getusha please refer to our docs, we need credentials to test integrations. I have my proposal here

abzokhattab commented 6 days ago

I guess this one is only for the C+ team, @getusha please refer to our docs, we need credentials to test integrations. I have my proposal https://github.com/Expensify/App/issues/50498#issuecomment-2402131721

Isn't the external label mean it's open for all external contributors as per the contribution guidelines? I am a bit confused about this

melvin-bot[bot] commented 2 days ago

@johncschuster, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!

johncschuster commented 2 days ago

Yeah, I'm not quite following either. @allgandalf, what made you say this is only for C+ team? Can you help me understand the thought process you followed to get to that conclusion?

allgandalf commented 2 days ago

what made you say this is only for C+ team?

Yeah, To fix the ^ bug, we need to have Accounting connection for Quick Books Online, this particular issue is related to accounting, and to access the app screen for the mentioned bug, we need to be connected to QBO, the credentials for QBO are only shared with the C+ team via 1Password and hence i said that way, you can read more about it here

johncschuster commented 1 day ago

Got it! Thank you for spelling that out!

abzokhattab commented 1 day ago

I see but I can still create a QBO account using their free trial + if you check other QBO issues you can find that normal contributors are hired in some of them. so i don't think this would be a problem