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

[Payment due 03-04][$500] Menu - Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu #33541

Closed
2 of 6 tasks
izarutskaya opened this issue Dec 23, 2023 · 41 comments
Assignees
Labels
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 Dec 23, 2023

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: 1.4.16-3
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @

Action Performed:

  1. Navigate to New dot app as Employee
  2. Go to workspace chat> create Scan request
  3. On detail page tap receipt and 3-Dot menu in a quick manner

Expected Result:

The receipt should be open and no 3-Dot menu should be open

Actual Result:

3-Dot menu (Pin & Delete request) from detail page appear on Receipt image when quickly tap on receipt and 3-Dot menu

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

Bug6324744_1703317651951.az_recorder_20231222_204456.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ea7a6ca457ab8814
  • Upwork Job ID: 1738519644108128256
  • Last Price Increase: 2024-01-06
  • Automatic offers:
    • c3024 | Contributor | 28106449
@izarutskaya izarutskaya added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 23, 2023
@melvin-bot melvin-bot bot changed the title Menu - Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu [$500] Menu - Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu Dec 23, 2023
Copy link

melvin-bot bot commented Dec 23, 2023

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

Copy link

melvin-bot bot commented Dec 23, 2023

Triggered auto assignment to @Christinadobrzyn (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 23, 2023
Copy link

melvin-bot bot commented Dec 23, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Copy link

melvin-bot bot commented Dec 23, 2023

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

@shahinyan11
Copy link

shahinyan11 commented Dec 23, 2023

Proposal

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

Menu - Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu

What is the root cause of that problem?

The root cause is that the Receipt image view and 3-dot menu are modals. The modals always open on top of screen in react native and the last opened modal will be on top

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

I think would be better if we use one component for displaying any modals in root of project . But in this case, we can fix the problem with the following steps.

  1. Add call willAlertModalBecomeVisible(true) in this function
  2. Add this call willAlertModalBecomeVisible(false); in this function
  3. Use getModalState function or wrap ThreeDotsMenu component in withOnyx to access to modal state in component
export default withOnyx({
    modal: {
        key: ONYXKEYS.MODAL,
    },
})(ThreeDotsMenu);
  1. Change this function like below
const showPopoverMenu = () => {
        if (!modal.willAlertModalBecomeVisible) {
            setPopupMenuVisible(true);
        }
};

What alternative solutions did you explore? (Optional)

We can add another key in Onyx.modal like willAlertModalBecomeVisible and use it to avoid using the key which seems to be for modal alert only

@bernhardoj

This comment was marked as outdated.

@tienifr
Copy link
Contributor

tienifr commented Dec 23, 2023

Proposal

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

3-Dot menu (Pin & Delete request) from detail page appear on Receipt image when quickly tap on receipt and 3-Dot menu

What is the root cause of that problem?

If we click receipt image and 3 dot menu quickly, the receipt image will be opened and then 3 dot menu will be opened on top of it, and currently we don't have any logic to prevent 3 dot menu that are behind modal from being opened on top of the modal.

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

We should use similar approach as this PR that fixes a similar issue. Basically if the three dots menu is behind a modal, its popover should not be visible.

  1. Connect to ONYXKEYS.MODAL here
export default withOnyx({
    modal: {
        key: ONYXKEYS.MODAL,
    },
})(ThreeDotsMenu);
  1. If the three dots menu is behind a modal, hide the popover if it's currently opened
    (We need to check !shouldOverlay below because if shouldOverlay is true, that means the current three dots itself is inside a modal: AttachmentModal)
const isBehindModal = modal.willAlertModalBecomeVisible && !shouldOverlay;

useEffect(() => {
    if (isBehindModal && isPopupMenuVisible) {
        hidePopoverMenu();
    }
}, [isBehindModal, isPopupMenuVisible]);
  1. Add isBehindModal to the visible condition as well
isVisible={isPopupMenuVisible && !isBehindModal}
  1. We can optionally add || isBehindModal to the disabled condition here

This is basically the same approach already used in this PR for another component, just that in here we relies on the modal visibility rather than screen focus. This is a global fix that makes sure no such issue like this will occur again for the three dots menu, and rely on existing values/lifecycle of willAlertModalBecomeVisible rather than manipulating its lifecycle to workaround the race conditions as suggested in the above proposal, which unfortunately still doesn't work if pressing the receipt image and 3 dot very quickly.

What alternative solutions did you explore? (Optional)

The shouldOverlay name is a bit vague and can be considered renaming to isInModal for clarity.

We can consider relying on modal.isVisible (or both willAlertModalBecomeVisible and isVisible)

@c3024
Copy link
Contributor

c3024 commented Dec 25, 2023

I find this extremely difficult to reproduce on my device and emulator. I am not able to quickly press the three dot menu after pressing on the receipt. Any tips to consistently reproduce this?

@tienifr
Copy link
Contributor

tienifr commented Dec 25, 2023

I find this extremely difficult to reproduce on my device and emulator. I am not able to quickly press the three dot menu after pressing on the receipt. Any tips to consistently reproduce this?

@c3024 I run on my physical Android device and press the receipt with 1 finger and immediately press the three dot using another finger and can reproduce it every time, hope that helps 👍

@suneox
Copy link
Contributor

suneox commented Dec 25, 2023

Proposal

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

Menu - Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu

What is the root cause of that problem?

When the user clicks on 3-Dot menu, the menu will be open while the menu is rendering if a user quickly clicks on an attachment the Modal for attachment is also triggered to open, and this issue always happens when device performance is slow.
We can perform by using scripting or putting a device to low-performance mode

  let menu = document.querySelectorAll('button[aria-label="More"]')[0];
  let attachment = document.querySelectorAll('[aria-label="View attachment"]')[0];
  menu?.click();
  attachment?.click()
Reproduce
Screen.Recording.2023-12-25.at.21.23.14.mov

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

Due to this issue can happen anywhere in the app so we need to handle it generally. We will create ThreeDotsMenuManager look like ReportActionComposeFocusManager has function isMenuOpen, openMenu, closeMenu
When showModal we will call ThreeDotsMenuManager.closeMenu() if we have any ThreeDotsMenu has opened state

    const showModal = () => {
+       ThreeDotsMenuManager.closeMenu()
        ...
        onModalShow?.();
    };

What alternative solutions did you explore? (Optional)

Instead of create ThreeDotsMenuManager we can create PopoverManager

@melvin-bot melvin-bot bot added the Overdue label Dec 27, 2023
@c3024
Copy link
Contributor

c3024 commented Dec 28, 2023

Reviewing

@melvin-bot melvin-bot bot removed the Overdue label Dec 28, 2023
Copy link

melvin-bot bot commented Dec 30, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Dec 30, 2023
@c3024
Copy link
Contributor

c3024 commented Dec 31, 2023

Is there a similar case anywhere else where an undesired three dot menu modal appears over another modal so that a global fix is warranted?

@melvin-bot melvin-bot bot removed the Overdue label Dec 31, 2023
@shahinyan11
Copy link

@c3024 I think a global fix should be made around a new feature, not around bug. Because AttachmentModal is not associated with other modals and works separately .

@melvin-bot melvin-bot bot added the Overdue label Jan 2, 2024
@Christinadobrzyn
Copy link
Contributor

looks like we're reviewing this, @c3024 let me know if you need any help from me!

@melvin-bot melvin-bot bot removed the Overdue label Jan 3, 2024
@tienifr
Copy link
Contributor

tienifr commented Jan 5, 2024

Is there a similar case anywhere else where an undesired three dot menu modal appears over another modal so that a global fix is warranted?

@c3024 it happens on the normal chat report as well, when pressing three dots and sent attachment at the same time. I believe it happens basically everywhere an AttachmentModal and three dot menu appears together. So yeah I think we need a global fix

@melvin-bot melvin-bot bot added the Overdue label Jan 5, 2024
@tienifr
Copy link
Contributor

tienifr commented Jan 15, 2024

Did you try opening only the 3-dot menu ? I faced this problem when I used @tienifr's approach

@shahinyan11 There'll be a isPopover field accompanying the willAlertModalBecomeVisible, so the isBehindModal won't be true if the modal that's "becoming visible" is the PopoverMenu itself.

I'll handle that during the PR.

Copy link

melvin-bot bot commented Jan 16, 2024

@puneetlath, @Christinadobrzyn, @c3024 Eep! 4 days overdue now. Issues have feelings too...

@puneetlath
Copy link
Contributor

Ok, plan sounds good to me.

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

melvin-bot bot commented Jan 17, 2024

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

@Christinadobrzyn
Copy link
Contributor

working on PR - #34720

@Christinadobrzyn
Copy link
Contributor

Ooh boy! #34720 Hit production today!

I'll add the payment timeframe in the Title.

@Christinadobrzyn Christinadobrzyn changed the title [$500] Menu - Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu [Payment due 03-04][$500] Menu - Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu Feb 27, 2024
@tienifr

This comment was marked as outdated.

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Mar 7, 2024

Payouts due:

Contributor: $500 @tienifr (paid in Upwork - https://www.upwork.com/nx/wm/offer/101256857)
Contributor+: $500 @c3024 (paid in Upwork - https://www.upwork.com/nx/wm/workroom/36050068/overview)

Upwork job is here (it's closed).

@tienifr are you paid in upwork or newDot? If yes, can you accept this offer - https://www.upwork.com/nx/wm/offer/101256857

@c3024 do we need a regression test for this?

@tienifr
Copy link
Contributor

tienifr commented Mar 7, 2024

@Christinadobrzyn I accepted, thanks!

@c3024
Copy link
Contributor

c3024 commented Mar 8, 2024

Regression Test Proposal

  1. Login ND as an employee of a Workspace
  2. Navigate to the Workspace Chat > Create Scan Request and complete the request
  3. Navigate to this money request details page
  4. On this details page press on the receipt and then on the 3-Dot menu of this details page quickly
  5. Verify that only the receipt is opened with no 3-Dot menu of the details page opened

👍 or 👎

@Christinadobrzyn
Copy link
Contributor

Awesome! Thank you @c3024 and @tienifr!

Both of you are paid based on this payment summary #33541 (comment)

I'll submit the regression test on Monday

@Christinadobrzyn
Copy link
Contributor

submitted regression test - https://github.com/Expensify/Expensify/issues/378295

closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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