[HOLD for payment 2024-12-05] [$125] Xero - Selected Xero bank account is not highlighted with a green checkmark

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.63-3
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: 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/5220818
Email or phone of affected tester (no customers): https://expensify.testrail.io/index.php?/tests/view/5220818
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace settings > Accounting.
3. Connect to Xero.
4. After connecting to Xero, click Export.
5. Click Xero bank account.

Expected Result:

The selected Xero bank account should be highlighted with a green checkmark.

Actual Result:

The selected Xero bank account is not highlighted with a green checkmark.
It is only highlighted after manually selecting it.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6668343_1731931675725.bandicam_2024-11-18_20-03-24-662.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021858554918265952310
• Upwork Job ID: 1858554918265952310
• Last Price Increase: 2024-11-18
• Automatic offers:
• ahmedGaber93 | Reviewer | 105021706
• twilight2294 | Contributor | 105021707

Issue Owner

Current Issue Owner: @sakluger

[melvin-bot]

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

[FitseTLT]

Edited by proposal-police: This proposal was edited at 2024-11-18 12:44:16 UTC.

Proposal

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

Xero - Selected Xero bank account is not highlighted with a green checkmark

What is the root cause of that problem?

We are setting the selected bank account to bankAccounts?.[0] as a backup when there is no bank account in policy?.connections?.xero?.data.bankAccounts that matches with exportConfiguration?.nonReimbursableAccount

App/src/pages/workspace/accounting/xero/export/XeroExportConfigurationPage.tsx

Line 26 in 63c8dd2

return selectedAccount?.name ?? bankAccounts?.[0]?.name ?? '';

but we didn't give the same backup in the account select page here

App/src/pages/workspace/accounting/xero/export/XeroBankAccountSelectPage.tsx

Line 31 in 63c8dd2

() => getXeroBankAccounts(policy ?? undefined, config?.export?.nonReimbursableAccount),

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

We should give the same fallback value in.XeroBankAccountSelectPage.tsx, i.e.

    const {bankAccounts} = policy?.connections?.xero?.data ?? {};

    const xeroSelectorOptions = useMemo<SelectorType[]>(
        () => getXeroBankAccounts(policy ?? undefined, config?.export?.nonReimbursableAccount ?? bankAccounts?.[0]?.id),
        [config?.export?.nonReimbursableAccount, policy, bankAccounts],

What alternative solutions did you explore? (Optional)

[sakluger]

This one feels like a super easy change so I'm going to set the price to $125. If there are additional fields where this is happening, or if it is actually more complex than I realize, let me know and we can consider increasing the price.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Upwork job price has been updated to $125

[twilight2294]

Proposal

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

Default selected bank account isn't highlighted with a green checkmark

What is the root cause of that problem?

In getXeroBankAccounts, for the first time, config?.export?.nonReimbursableAccount will be a empty string.

App/src/pages/workspace/accounting/xero/export/XeroBankAccountSelectPage.tsx

Line 31 in 63c8dd2

() => getXeroBankAccounts(policy ?? undefined, config?.export?.nonReimbursableAccount),

Thus there is no green checkmark

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

We should use a || bankAccounts?.[0]?.id as a fallback route here to display the first default bank account in the list.

    const {bankAccounts} = policy?.connections?.xero?.data ?? {};

    const xeroSelectorOptions = useMemo<SelectorType[]>(
        () => getXeroBankAccounts(policy ?? undefined, config?.export?.nonReimbursableAccount || bankAccounts?.[0]?.id),
        [config?.export?.nonReimbursableAccount, policy],

Note that we cannot use ?? in this case, as empty string is a valid string in javascript and by using ?? we will never fallback to bankAccounts?.[0]?.id and this bug is still reproducible if we do so, so correct fix is to use || bankAccounts?.[0]?.id.

What alternative solutions did you explore? (Optional)

[ahmedGaber93]

reviewing today

[ahmedGaber93]

@twilight2294's proposal LGTM!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[FitseTLT]

@ahmedGaber93 Can u please take a look again? The problem doesn't have anything related with using ?? or || we are already using ?? to give fallback in the Configuration page to show the default bank account

App/src/pages/workspace/accounting/xero/export/XeroExportConfigurationPage.tsx

Line 26 in 63c8dd2

return selectedAccount?.name ?? bankAccounts?.[0]?.name ?? '';

This is because the selectedAccount either doesn't exist or it is non-empty string and we should also be consistent with it. You are being mislead by the contributor into applying unnecessary safety. The bug is only related to not giving the first bank account as a fallback here

App/src/pages/workspace/accounting/xero/export/XeroBankAccountSelectPage.tsx

Line 31 in 63c8dd2

() => getXeroBankAccounts(policy ?? undefined, config?.export?.nonReimbursableAccount),

The default bank account is visible in the menu item because we give the first bank account as a fallback here (note: using ??)

App/src/pages/workspace/accounting/xero/export/XeroExportConfigurationPage.tsx

Line 26 in 63c8dd2

return selectedAccount?.name ?? bankAccounts?.[0]?.name ?? '';

But when the user opens the page we haven't given the same first bank account as fallback in XeroBankAccountSelectPage so it is not selected.
So we should give the same fallback in both cases in the same way. The selectedAccount?.name cannot just be an empty string we know we set it to some non-empty string value or it is not set in which case we give a fallback.
The selected proposal even has a wrong RCA

In getXeroBankAccounts, for the first time, config?.export?.nonReimbursableAccount will be a empty string.

If the case is having an empty string then in the configuration page we will give an empty string to the menu here

App/src/pages/workspace/accounting/xero/export/XeroExportConfigurationPage.tsx

Line 26 in 63c8dd2

return selectedAccount?.name ?? bankAccounts?.[0]?.name ?? '';

and we wouldn't see the first bank account displayed in the menu item as shown in the OP

Please re-review. Thx!
cc @justinpersaud

[ahmedGaber93]

@FitseTLT Thanks for the feedback, I tested your proposal well, and it doesn't fix the issue.

see the test video here

20241122131049586.mp4

there is a different between type of config?.export?.nonReimbursableAccount and type of selectedAccount
config?.export?.nonReimbursableAccount is come from BE as string, and in this issue case it comes with empty string "" and nullish check ?? only checks for null and undefined
selectedAccount type should be object | undefined (see line 25)

App/src/pages/workspace/accounting/xero/export/XeroExportConfigurationPage.tsx

Lines 25 to 26 in 63c8dd2

	const selectedAccount = (bankAccounts ?? []).find((bank) => bank.id === exportConfiguration?.nonReimbursableAccount);
	return selectedAccount?.name ?? bankAccounts?.[0]?.name ?? '';

Another point I was thinking about it before choose the second proposal, is this small different between two proposal is worth to choose the second proposal over the first proposal that have the first root cause?

I think here it is a small different, but it is important, and it should be worth If we take into consideration the issue is a straight forward with simple RCA, and the Actual Result in OP line 2 can easily lead to what the fix should be.

[FitseTLT]

@ahmedGaber93 Got it! Thx 👍

[melvin-bot]

📣 @ahmedGaber93 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @twilight2294 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[twilight2294]

I will raise the PR today

[twilight2294]

PR ready for review @ahmedGaber93

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.67-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Selected Xero bank account is not highlighted with a green checkmark  #52974

If no regressions arise, payment will be issued on 2024-12-05. 🎊

For reference, here are some details about the assignees on this issue:

• @ahmedGaber93 requires payment automatic offer (Reviewer)
• @twilight2294 requires payment automatic offer (Contributor)

[melvin-bot]

@ahmedGaber93 @sakluger @ahmedGaber93 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[sakluger]

@ahmedGaber93 please complete the BZ checklist so I can complete payment.

[sakluger]

Summarizing payment on this issue:

Contributor: @twilight2294 $125, paid via Upwork
Contributor+: @ahmedGaber93 $125, please request via NewDot.

[ahmedGaber93]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other: Applause Internal Team

• [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: https://github.com/Expensify/App/pull/46898/files#r1871855560

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion: N/A.

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

This is a minor change, and I think no regression test required.

[ahmedGaber93]

@sakluger Checklist have been completed, and I will get paid via NewDot.

[sakluger]

@ahmedGaber93 thanks! Also, thanks for clarifying that you're paid now via NewDot. I withdrew the Upwork offer and updated the payment summary above.

FYI, you should no longer get automated Upwork offers sent to you - if that continues to happen, please let us know so we can make sure you're set up correctly in our system.

[ahmedGaber93]

Requested in ND

[JmillsExpensify]

$125 approved for @ahmedGaber93