lanitochka17 commented 1 month 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.50-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A 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+kh081006@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
[User A] Send an invoice to User B
[User B] Go to invoice chat
[User B] Click on the pay button dropdown in the preview in the main chat (not the thread)
[User B] Select any option

Expected Result:

User will be able to select payment option from the dropdown

Actual Result:

User is unable to select payment option from the dropdown. App does not proceed after selecting any option from the dropdown

Workaround:

Unknown

Platforms:

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

[ ] Android: Standalone
[ ] Android: HybridApp
[x] Android: mWeb Chrome
[ ] iOS: Standalone
[ ] iOS: HybridApp
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/888e47a3-5b9b-4ed7-a052-629ce9305f98

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021848792881281684450
Upwork Job ID: 1848792881281684450
Last Price Increase: 2024-10-22
Automatic offers:

truph01 | Contributor | 104576764

Issue Owner

Current Issue Owner: @OfstadC

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @OfstadC (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 month ago

@OfstadC 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 month ago

We think that this bug might be related to #vip-bills

abzokhattab commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-10-17 15:38:53 UTC.

Proposal

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

Invoices - Unable to select payment option in the dropdown in expense preview in the main chat

What is the root cause of that problem?

In the ReportPreview component, when users click on options in the SettlementButton that have submenus, the submenu fails to display properly. Instead, the popover menu blinks

The issue occurs because the menuItems prop passed to the PopoverMenu component changes on every render due to unstable function references. Specifically:

In SettlementButton, the onOptionSelected function changes on every render because it's recreated each time without memoization.
In ButtonWithDropdownMenu, the menuItems array is recreated on every render because it depends on onOptionSelected and options, which are changing references. This causes the PopoverMenu to reset its internal state on each render, preventing the submenu from displaying correctly.

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

Memoize menuItems in ButtonWithDropdownMenu:
- Use useMemo to memoize the menuItems array, ensuring it only changes when options or onOptionSelected change.
- This prevents unnecessary re-renders and state resets in PopoverMenu.

// In ButtonWithDropdownMenu.js
const menuItems = useMemo(
    () =>
        options.map((item, index) => ({
            ...item,
            onSelected: item.onSelected
                ? () => item.onSelected?.()
                : () => {
                      onOptionSelected?.(item);
                      setSelectedItemIndex(index);
                  },
            shouldCallAfterModalHide: true,
        })),
    [options, onOptionSelected],
);

Memoize onOptionSelected in SettlementButton:
- Use useCallback to memoize the onOptionSelected function in SettlementButton, ensuring it has a stable reference.
- This prevents onOptionSelected from changing on every render, which stabilizes the menuItems prop.

// In SettlementButton.js
const onOptionSelected = useCallback(
    (option) => {
        if (policyID === '-1') {
            return;
        }
        savePreferredPaymentMethod(policyID, option.value);
    },
    [policyID],
);

By making these changes, we stabilize the menuItems prop passed to PopoverMenu, which prevents it from resetting its state and allows submenus to display correctly.

POC

https://github.com/user-attachments/assets/4e8970dc-424c-43b7-9e85-d1efc4b342f0

What alternative solutions did you explore? (Optional)

truph01 commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-10-19 06:03:19 UTC.

Proposal

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

User is unable to select payment option from the dropdown. App does not proceed after selecting any option from the dropdown

What is the root cause of that problem?

users open payment popover, hovered is true
they hover to payment option -> hovered is false
they select any option, we will render the sublist

https://github.com/Expensify/App/blob/66cf824036a51183d73dab2cc621901e43fe3196/src/components/PopoverMenu.tsx#L179

The hover event will be detected at the moment when we remove the main list then show the sublist

-> The menuItems will be re-calculated

If the menuItems get changed, we will reset the current menu items

https://github.com/Expensify/App/blob/66cf824036a51183d73dab2cc621901e43fe3196/src/components/PopoverMenu.tsx#L261-L267

The RCA of this issue is: hovered value change while showing the sublist (The RCA is not memorize problem since this issue doesn't happen on MoneyReportHeader)

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

We have the same issue here, and the PR author introduce shouldFreezeCapture ro freeze capture hover event when the popover is visible

so we should apply the same in here

<Hoverable
                shouldHandleScroll
                isDisabled={draftMessage !== undefined}
                shouldFreezeCapture={isPaymentMethodPopoverActive}
            >

We also need to prevent resetting hover in

https://github.com/Expensify/App/blob/66cf824036a51183d73dab2cc621901e43fe3196/src/components/Hoverable/ActiveHoverable.tsx#L129

if shouldFreezeCapture is true

In case user press outside the action, we need to reset hover to false by triggering unsetHoveredIfOutside at the begining

we just update this line to

document.addEventListener('mouseover', unsetHoveredIfOutside, true);

What alternative solutions did you explore? (Optional)

OfstadC commented 3 weeks ago

Added to BillPay - just waiting on confirmation here

melvin-bot[bot] commented 3 weeks ago

@OfstadC Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

jayeshmangwani commented 3 weeks ago

Thanks for the proposals. I have tested both, and both will fix the issue. However, I agree with @truph01 that we should return early in the hoverable component to prevent flickering of the hover state value and freeze the hover, This approach will resolve our issue

jayeshmangwani commented 3 weeks ago

@truph01 's Proposal looks good to me. We can use shouldFreezeCapture when the popover is visible, so that the menuItems don't recalculate when pressing the SettlementButton.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

📣 @truph01 🎉 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 📖

jayeshmangwani commented 3 weeks ago

@truph01 , could you let us know when we can expect the PR?

jayeshmangwani commented 2 weeks ago

friendly bump @truph01

truph01 commented 2 weeks ago

I will open PR in a few hours

jayeshmangwani commented 2 weeks ago

Thanks for the update

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

https://github.com/Expensify/App/pull/51645

If no regressions arise, payment will be issued on 2024-11-11. :confetti_ball:

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

@jayeshmangwani requires payment through NewDot Manual Requests
@truph01 requires payment automatic offer (Contributor)

melvin-bot[bot] commented 2 weeks ago

@jayeshmangwani @OfstadC 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]

OfstadC commented 1 week ago

@jayeshmangwani Please complete the BZ checklist before the payment date. Thanks 😃

OfstadC commented 1 week ago

@jayeshmangwani Please complete the BZ checklist so I can issue payment

OfstadC commented 1 week ago

Payment Summary

@jayeshmangwani due $250 via manual New Expensify request
@truph01 paid $250 via Upwork

jayeshmangwani commented 1 week ago

Really sorry, I missed the BZ checklist , Let me complete it now!

jayeshmangwani commented 1 week ago

BugZero Checklist:

[x] [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 - [x] 1z. Other: We changed a memoization condition, which resulted in this bug. Where bug was reported: - [x] 2a. Reported on production - [ ] 2b. Reported on staging (deploy blocker) - [ ] 2c. Reported on a PR - [ ] 2z. Other: Who reported the bug: - [ ] 3a. Expensify user - [ ] 3b. Expensify employee - [ ] 3c. Contributor - [x] 3d. QA - [ ] 3z. Other:

[x] [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/49051#discussion_r1830739940
[x] [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
[x] [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.

Regression Test Proposal

Open the app on two devices as User A and User B.
[User A]: Send an invoice to User B.
[User B]: Open the invoice chat.
[User B]: In the main chat preview (not the thread), click on the Pay button dropdown.
[User B]: Select any payment option from the dropdown.
Verify that User B can select a payment option from the dropdown successfully.

Do we agree 👍 or 👎

OfstadC commented 1 week ago

https://github.com/Expensify/Expensify/issues/443345

OfstadC commented 1 week ago

Thanks @jayeshmangwani !

Payment Summary updated

Expensify / App

[HOLD for payment 2024-11-11] [$250] Invoices - Unable to select payment option in the dropdown in expense preview in the main chat #51016

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Proposal

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

What is the root cause of that problem?

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

POC

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Payment Summary

BugZero Checklist:

Regression Test Proposal