Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for Payment 2024-08-27] [$250] Invoice - Paying invoice as a business results in console error #47187

Closed
1 of 6 tasks
izarutskaya opened this issue Aug 9, 2024 · 20 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Aug 9, 2024

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.18-7
Reproducible in staging?: Y
Reproducible in production?: N
Email or phone of affected tester (no customers): applausetester+nl405@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Create two accounts on different tabs/devices
  3. Create a workspace on each of the accounts
  4. Send an invoice from account A to account B
  5. On account B open the invoice and pay the invoice as a business

Expected Result:

No console errors appear

Actual Result:

Console error appears with the message "Cannot read properties of undefined (reading 'backButtonText"

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

Bug6566726_1723213373006.bandicam_2024-08-09_17-18-58-225.mp4

Bug6566726_1723213372999!staging.new.expensify.com-1723213166507.txt

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c50a9517130ce1f6
  • Upwork Job ID: 1823064487894700426
  • Last Price Increase: 2024-08-12
  • Automatic offers:
    • brunovjk | Reviewer | 103527458
Issue OwnerCurrent Issue Owner: @brunovjk
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Aug 9, 2024
Copy link

melvin-bot bot commented Aug 9, 2024

Triggered auto assignment to @francoisl (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Aug 9, 2024

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

Copy link
Contributor

github-actions bot commented Aug 9, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@izarutskaya
Copy link
Author

We think this issue might be related to the #vip-bills

@thienlnam thienlnam added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 DeployBlocker Indicates it should block deploying the API labels Aug 9, 2024
@thienlnam
Copy link
Contributor

Looks like an App issue - though if the payment goes through not sure if this needs to be a blocker

@Krishna2323
Copy link
Contributor

Proposal

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

Invoice - Paying invoice as a business results in console error

What is the root cause of that problem?

We are not using optional chaining operator when accessing backButtonText.

const hasBackButtonText = !!previouslySelectedItem.backButtonText;

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

Update to !!previouslySelectedItem?.backButtonText;.

What alternative solutions did you explore? (Optional)

@bernhardoj
Copy link
Contributor

Proposal

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

Paying invoice as business throws a console error.

What is the root cause of that problem?

The error comes from the back button rendering component.

const renderBackButtonItem = () => {
const previousMenuItems = getPreviousSubMenu();
const previouslySelectedItem = previousMenuItems[enteredSubMenuIndexes[enteredSubMenuIndexes.length - 1]];
const hasBackButtonText = !!previouslySelectedItem.backButtonText;

The back button will show if we are on a sub menu.

{enteredSubMenuIndexes.length > 0 && renderBackButtonItem()}

The way it works is, when we select an option that has a sub menu, the option index will be added to enteredSubMenuIndexes array. This 2 lines of code will get the sub-menu parent.

const previousMenuItems = getPreviousSubMenu();
const previouslySelectedItem = previousMenuItems[enteredSubMenuIndexes[enteredSubMenuIndexes.length - 1]];

But in our case, the parent is undefined. Why?

The invoice payment method initially has 2 options, individual and business. When we pay the invoice as business, we optimistically change the invoice receiver type of the report to BUSINESS.

App/src/libs/actions/IOU.ts

Lines 6594 to 6599 in e6546d6

if (ReportUtils.isIndividualInvoiceRoom(chatReport) && payAsBusiness && primaryPolicyID) {
optimisticChatReport.invoiceReceiver = {
type: CONST.REPORT.INVOICE_RECEIVER_TYPE.BUSINESS,
policyID: primaryPolicyID,
};
}

Because of that, the individual option is not added anymore as the payment method

if (ReportUtils.isIndividualInvoiceRoom(chatReport)) {
buttonOptions.push({
text: translate('iou.settlePersonal', {formattedAmount}),

leaving us the business as the only option. This triggers the popover menu re-render and when the back button component is trying to get the sub-menu parent, it fails and gets undefined.

[individual, business]
pay as business (parent index, 1)
pay elsewhere
[business]
re-rendered, trying to access index 1, undefined

But if we look at the popover component, we can see that there is already a logic to reset the sub-menu if the menu items/options are changed,

useEffect(() => {
if (menuItems.length === 0) {
return;
}
setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY);
setCurrentMenuItems(menuItems);
}, [menuItems]);

or when the modal closes.
onClose={() => {
setCurrentMenuItems(menuItems);
setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY);
onClose();
}}

But the menu items re-rendered happen earlier, so we still get the error.

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

Instead of using useEffect here, we can use useLayoutEffect which will be triggered before painting.

useEffect(() => {
if (menuItems.length === 0) {
return;
}
setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY);
setCurrentMenuItems(menuItems);
}, [menuItems]);

@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Aug 12, 2024
@melvin-bot melvin-bot bot changed the title Invoice - Paying invoice as a business results in console error [$250] Invoice - Paying invoice as a business results in console error Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

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

@wildan-m
Copy link
Contributor

wildan-m commented Aug 13, 2024

Proposal

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

Paying the invoice as a business leads to a console error.

What is the root cause of that problem?

previouslySelectedItem might be undefined

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

No need to render if previouslySelectedItem undefined

src/components/PopoverMenu.tsx

    const renderBackButtonItem = () => {
        const previousMenuItems = getPreviousSubMenu();
        
        const previouslySelectedItem = previousMenuItems[enteredSubMenuIndexes[enteredSubMenuIndexes.length - 1]];
        
        if (!previouslySelectedItem) {
            return null;
        }

What alternative solutions did you explore? (Optional)

@brunovjk
Copy link
Contributor

After testing, I think we can go with @bernhardoj's proposal. It effectively resolves the issue by ensuring the state is updated before rendering. I tested it and works fine in a quick test, but we can test it thoroughly in the PR.

Thank you for the proposals, @Krishna2323 and @wildan-m. While they addressed the symptom, they did not fully resolve the underlying cause.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Aug 13, 2024

Current assignee @francoisl is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 14, 2024
Copy link

melvin-bot bot commented Aug 14, 2024

📣 @brunovjk 🎉 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 melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 15, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @brunovjk

@brunovjk
Copy link
Contributor

brunovjk commented Aug 21, 2024

Note

The production deploy automation failed: This should be paid 8/27 according to #47639 prod deploy checklist, confirmed in #47488 (comment).

cc: @greg-schroeder

@brunovjk
Copy link
Contributor

brunovjk commented Aug 26, 2024

  • [@brunovjk] The PR that introduced the bug has been identified. Link to the PR: N/A
  • [@brunovjk] 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: N/A
  • [@brunovjk] A discussion in #expensify-bugs 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
  • [@brunovjk] Determine if we should create a regression test for this bug. Yes
  • [@brunovjk] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

  • User B send invoice to user A
  • User A open the invoice chat
  • User A pay the invoice as business
  • Verify there is no console error of "Cannot read properties of undefined (reading 'backButtonText..."
  • Do we agree 👍 or 👎

@greg-schroeder greg-schroeder changed the title [$250] Invoice - Paying invoice as a business results in console error [HOLD for Payment 2024-08-27] [$250] Invoice - Paying invoice as a business results in console error Aug 27, 2024
@greg-schroeder greg-schroeder added the Awaiting Payment Auto-added when associated PR is deployed to production label Aug 27, 2024
@greg-schroeder
Copy link
Contributor

Payment summary:

Contributor: @bernhardoj - $250 - You can make a manual request via ND
C+: @brunovjk - $250 - Paid via Upwork

@greg-schroeder
Copy link
Contributor

Regression test filed

@bernhardoj
Copy link
Contributor

Requested in ND.

@JmillsExpensify
Copy link

$250 approved for @bernhardoj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants