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 2023-10-27] [$500] Chat - The three-dot menu fails to close after clicking on "Pin" #28606

Closed
3 of 6 tasks
kbecciv opened this issue Oct 2, 2023 · 54 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Oct 2, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Navigate to the "New room" section.
  2. Enter a name for the room and select the workspace.
  3. Set the visibility of the room to "Public"
  4. Click on the "Create room" button.
  5. Go to "Share code" > "Copy URL to clipboard"
  6. Open another browser where you are not signed in.
  7. Paste the copied URL.
  8. Click on three dots for menu and click on "Pin"

Expected Result:

Upon clicking "Pin" the three-dot menu should close

Actual Result:

The three-dot menu remains open after clicking on "Pin"

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.75.8
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
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2023-09-29.at.3.19.35.PM.1.mov
screen-recording-2023-09-29-at-25557-pm_IijAF1Pu.1.mp4
Recording.4814.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ayazhussain79
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695981995544409

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018cbad69c4c13b118
  • Upwork Job ID: 1708815358175035392
  • Last Price Increase: 2023-10-09
@kbecciv kbecciv 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 Oct 2, 2023
@melvin-bot melvin-bot bot changed the title Chat - The three-dot menu fails to close after clicking on "Pin" [$500] Chat - The three-dot menu fails to close after clicking on "Pin" Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

Triggered auto assignment to @dylanexpensify (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 Oct 2, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 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

@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

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

@ishpaul777
Copy link
Contributor

ishpaul777 commented Oct 2, 2023

Proposal

Problem

Chat - The three-dot menu fails to close after clicking on "Pin"

Root cause

We are using Session.checkIfActionIsAllowed to execute certain actions here when user is not logged the callback for onItemSelected never gets executed.

Changes

We can use the Modal.close action to hide any modal visible if the user is anonymous, we'll pass a new argument hideModal (false by default) and use the method if true

Screen.Recording.2023-10-02.at.7.12.25.PM.mov

@dukenv0307
Copy link
Contributor

dukenv0307 commented Oct 2, 2023

Proposal

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

The three-dot menu remains open after clicking on "Pin"

What is the root cause of that problem?

When the user is anonymous and clicking Pin, the check here will lead the user to sign in. So the onPress of the menu was never called to close the popover menu.

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

For popover menu, we have our own checks here for anonymous user, so we don't have to check inside MenuItem

We can add a flag shouldCheckActionAllowedOnPress to the MenuItem, defaults to true. Only if it's true, we wrap the onPress callback with the check here. If it's false we rely on the parent component's checks and do not check in MenuItem

Then we pass shouldCheckActionAllowedOnPress={false} in the popover menu.

What alternative solutions did you explore? (Optional)

We can fix the same for any parent component which uses MenuItem, that has the same issue.

An alternative is to call Modal.close, but this is noticeably slow, the menu will stay there for a while instead of being closed immediately. Also I prefer not touching global state if possible since it causes a lot of rerendering.

@yh-0218
Copy link
Contributor

yh-0218 commented Oct 2, 2023

Proposal

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

The three-dot menu remains open after clicking on "Pin"

What is the root cause of that problem?

When the user is clicking Pin, we show Login Modal and don't hide Popover menu.

onPress={Session.checkIfActionIsAllowed((e) => {

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

we need to show Login Modal and must close Popover menu.
For this.

  1. We don't check isAnonymousUser() on this for only Popover Menu
    onPress={Session.checkIfActionIsAllowed((e) => {

    we can add isAnonymousAction={item.isAnonymousAction} to here
  2. We need to check isAnonymousUser() and add isAnonymousAction=true on onSelected function of threeDotMenuItems on HeaderView.js.
    For pin, we have already this check.
    onSelected: Session.checkIfActionIsAllowed(() => Report.togglePinnedState(props.report.reportID, props.report.isPinned)),

    Add
isAnonymousAction: true

threeDotMenuItems.push({

We need to this check and add for other options (zoom, googlemeet)

What alternative solutions did you explore? (Optional)

@dylanexpensify
Copy link
Contributor

@situchan let's get some reviews! 🙇‍♂️

@melvin-bot melvin-bot bot added the Overdue label Oct 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

@dylanexpensify, @situchan Eep! 4 days overdue now. Issues have feelings too...

@dylanexpensify
Copy link
Contributor

@situchan can we please get some reviews in today? 🙏

@melvin-bot melvin-bot bot removed the Overdue label Oct 9, 2023
@situchan
Copy link
Contributor

situchan commented Oct 9, 2023

reviewing today

@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

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

@situchan
Copy link
Contributor

situchan commented Oct 9, 2023

@dukenv0307's proposal looks good to me.
🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Oct 9, 2023

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

@dylanexpensify
Copy link
Contributor

Nice! @marcochavezf let us know what you think!

@melvin-bot melvin-bot bot added the Overdue label Oct 13, 2023
@marcochavezf
Copy link
Contributor

Sounds good, thanks @situchan for the review. Assigning @dukenv0307 🚀

@melvin-bot melvin-bot bot removed the Overdue label Oct 13, 2023
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 13, 2023
@dukenv0307
Copy link
Contributor

@dylanexpensify I applied, thank you!

@melvin-bot melvin-bot bot added the Overdue label Nov 20, 2023
Copy link

melvin-bot bot commented Nov 21, 2023

@marcochavezf, @dylanexpensify, @situchan, @dukenv0307 Still overdue 6 days?! Let's take care of this!

@dylanexpensify
Copy link
Contributor

nice! Payment now!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Nov 22, 2023
Copy link

melvin-bot bot commented Nov 27, 2023

@marcochavezf, @dylanexpensify, @situchan, @dukenv0307 Huh... This is 4 days overdue. Who can take care of this?

@marcochavezf
Copy link
Contributor

Hi @dylanexpensify, I think we can close this out, correct? I will go ahead and close it, but feel free to re-open it if something is still pending :)

@situchan
Copy link
Contributor

This is still waiting for payment

@mountiny mountiny reopened this Nov 29, 2023
@mountiny mountiny added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Nov 29, 2023
Copy link

melvin-bot bot commented Nov 29, 2023

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

Copy link

melvin-bot bot commented Nov 29, 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

@mountiny
Copy link
Contributor

@lschurr could you help with the payment here please @situchan what is pending?

@situchan
Copy link
Contributor

Here's payment summary: #28606 (comment)

@situchan
Copy link
Contributor

situchan commented Nov 29, 2023

Automatic offer failed on this issue.
I applied job as requested by @dylanexpensify
Job link: #28606 (comment)

@lschurr
Copy link
Contributor

lschurr commented Nov 29, 2023

Looks like @dylanexpensify is the BZ on this one. Could you take a look at this one?

@lschurr lschurr removed their assignment Nov 29, 2023
@situchan
Copy link
Contributor

@lschurr @dylanexpensify is OOO. You're assigned based on https://expensify.slack.com/archives/C01GTK53T8Q/p1701271832347369?thread_ts=1701262010.017819&cid=C01GTK53T8Q.
Would be much appreciated if you can help.

@lschurr lschurr self-assigned this Nov 29, 2023
@lschurr
Copy link
Contributor

lschurr commented Nov 29, 2023

Got it, thanks. Just reviewing the job now and seeing what happened with payments: https://www.upwork.com/ab/applicants/1721476234243555328/applicants

@lschurr
Copy link
Contributor

lschurr commented Nov 29, 2023

Alright, looks like none of the payments were made, so I've paid @ayazhussain79 and have sent offers to @dukenv0307 and @situchan - could you accept those so that we can pay and close?

Just repeating the payment summary here:

@lschurr
Copy link
Contributor

lschurr commented Nov 30, 2023

All payments done. Closing!

@lschurr lschurr closed this as completed Nov 30, 2023
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. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

10 participants