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-22][$250] Send invoice - Members are selectable but no option after selecting the members #46652

Closed
4 of 6 tasks
izarutskaya opened this issue Aug 1, 2024 · 28 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 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@izarutskaya
Copy link

izarutskaya commented Aug 1, 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.15-4
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Launch New Expensify app.
  2. Go to FAB > Send invoice.
  3. Enter amount, select participant and send the invoice.
  4. In invoice chat, tap on the header.
  5. Tap Members.
  6. Long press on the invoice participant.
  7. Tap Select.

Expected Result:

App should not display option when long pressing on the member in Members page.

Actual Result:

App displays Select option when long pressing on the member in Members page but there is no action available after selecting the member.

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

Bug6558939_1722496858020.ScreenRecording_08-01-2024_15-14-40_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0189024d7c4bff0b08
  • Upwork Job ID: 1819113726462379309
  • Last Price Increase: 2024-08-01
  • Automatic offers:
    • ikevin127 | Contributor | 103414509
Issue OwnerCurrent Issue Owner: @mollfpr
@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 1, 2024
Copy link

melvin-bot bot commented Aug 1, 2024

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

melvin-bot bot commented Aug 1, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Aug 1, 2024
Copy link
Contributor

github-actions bot commented Aug 1, 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.

@neonbhai
Copy link
Contributor

neonbhai commented Aug 1, 2024

Proposal

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

Send invoice - Members are selectable but no option after selecting the members

What is the root cause of that problem?

We allow the member selection to be started for invoice Report Participants. Since these participants cannot be edited, we should not allow the selection to be started.

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

We will update the turnOnSelectionModeOnLongPress prop here to account for Invoice Reports

turnOnSelectionModeOnLongPress={isCurrentUserAdmin}

We will use ReportUtils.isInvoiceReport for the check

@Krishna2323
Copy link
Contributor

Krishna2323 commented Aug 1, 2024

Proposal

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

Send invoice - Members are selectable but no option after selecting the members

What is the root cause of that problem?

turnOnSelectionModeOnLongPress is true even when canSelectMultiple is false.

canSelectMultiple={canSelectMultiple}
turnOnSelectionModeOnLongPress={isCurrentUserAdmin}

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

Update the turnOnSelectionModeOnLongPress prop to turnOnSelectionModeOnLongPress={isCurrentUserAdmin && canSelectMultiple}. We can also remove isCurrentUserAdmin check because canSelectMultiple already uses that.

We should check other components using SelectionListWithModal with turnOnSelectionModeOnLongPress prop.

What alternative solutions did you explore? (Optional)

We can update the code below:

if (!turnOnSelectionModeOnLongPress || !isSmallScreenWidth) {

To:

        if (!turnOnSelectionModeOnLongPress || !isSmallScreenWidth || !rest.canSelectMultiple) {
           return;
       }

@ShridharGoel
Copy link
Contributor

Proposal

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

Members are selectable but no option after selecting the members.

What is the root cause of that problem?

We allow changes in the member list only for group chats. But turnOnSelectionModeOnLongPress is using just the isCurrentUserAdmin check.

turnOnSelectionModeOnLongPress={isCurrentUserAdmin}

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

Update the condition to:

turnOnSelectionModeOnLongPress={isGroupChat && isCurrentUserAdmin}

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Aug 1, 2024

CC: @pecanoro

#46096

@NikkiWines
Copy link
Contributor

Ah good call out @ShridharGoel - let's see if we end up reverting #46096 and if not we can evaluate proposals to fix this from there

@ikevin127
Copy link
Contributor

@filip-solecki We can probably fix this in a follow-up since we're still within the regression period. Wdyt ?

cc @luacmartins

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Aug 1, 2024

I think the original author mentioned somewhere that they're not available as of now, so maybe it can be made external.

@roryabraham roryabraham removed the DeployBlocker Indicates it should block deploying the API label Aug 1, 2024
@roryabraham
Copy link
Contributor

requested retest: https://expensify.slack.com/archives/C9YU7BX5M/p1722544655820829

as discussed in slack, we'll see if this is fixed. but if not, we'll demote it and ship the deploy anyways

@isagoico
Copy link

isagoico commented Aug 1, 2024

Still able to reproduce the issue on 9.0.15-9 on iOS

@roryabraham roryabraham added Daily KSv2 External Added to denote the issue can be worked on by a contributor and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Aug 1, 2024
Copy link

melvin-bot bot commented Aug 1, 2024

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

Copy link

melvin-bot bot commented Aug 1, 2024

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

@nkdengineer
Copy link
Contributor

Proposal

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

App displays Select option when long pressing on the member in Members page but there is no action available after selecting the member.

What is the root cause of that problem?

The turnOnSelectionModeOnLongPress doesn't match with the check canSelectMultiple. It missed isGroupChat check.

const canSelectMultiple = isGroupChat && isCurrentUserAdmin && (isSmallScreenWidth ? selectionMode?.isEnabled : true);

turnOnSelectionModeOnLongPress={isCurrentUserAdmin}

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

turnOnSelectionModeOnLongPress and canSelectMultiple have a pattern like this:

canSelectMultiple = canSelectCondition && (isSmallScreenWidth ? selectionMode?.isEnabled : true)
turnOnSelectionModeOnLongPress = canSelectCondition

So we can improve the prop in SelectionListWithModal by replacing turnOnSelectionModeOnLongPress and canSelectMultiple with a prop like canSelect that is canSelectCondition in the pattern above and then we will replace turnOnSelectionModeOnLongPress with canSelect prop

if (!turnOnSelectionModeOnLongPress || !isSmallScreenWidth) {

and pass canSelectMultiple={canSelect && (isSmallScreenWidth ? selectionMode?.isEnabled : true)} here

With this change, we can ensure that the selection mode can only be opened if we can select the option and prevent this same bug in the feature.

What alternative solutions did you explore? (Optional)

I think we should move the check group chat into isGroupChatAdmin because this function checks whether a user is an admin of a group chat or not but it doesn't check whether the report is the group chat or not.

const isCurrentUserAdmin = ReportUtils.isGroupChatAdmin(report, currentUserAccountID);

And then we can remove the check isGroupChat here

const canSelectMultiple = isGroupChat && isCurrentUserAdmin && (isSmallScreenWidth ? selectionMode?.isEnabled : true);

Copy link
Contributor

github-actions bot commented Aug 2, 2024

true

@filip-solecki
Copy link
Contributor

Hi! I am Filip from SWM and I'd like to take this as it comes from my PR as @ikevin127 mentioned above

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 2, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 2, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Aug 2, 2024
@ShridharGoel
Copy link
Contributor

@NikkiWines @roryabraham Am I eligible for compensation here since this was marked as "Help Wanted" and the implemented fix is same as my solution?

@nkdengineer
Copy link
Contributor

@filip-solecki What do you think about my idea in the main solution here? That can prevent the same bug can happening further.

@NikkiWines
Copy link
Contributor

@ShridharGoel yes, in this case you're eligible for compensation (cc: @adelekennedy for when we issue payments)

@mollfpr
Copy link
Contributor

mollfpr commented Aug 6, 2024

@adelekennedy Please assign @ikevin127, they've reviewed the PR.

@mollfpr mollfpr removed their assignment Aug 6, 2024
Copy link

melvin-bot bot commented Aug 6, 2024

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

@ikevin127
Copy link
Contributor

⚠️ Production deploy automation failed here -> this should be on [HOLD for Payment 2024-08-22] according to yesterday’s production deploy confirmed in #46698 (comment) and deploy checklist.

cc @adelekennedy

@NikkiWines NikkiWines changed the title [$250] Send invoice - Members are selectable but no option after selecting the members [HOLD for Payment 2024-08-22][$250] Send invoice - Members are selectable but no option after selecting the members Aug 19, 2024
@NikkiWines NikkiWines added the Awaiting Payment Auto-added when associated PR is deployed to production label Aug 19, 2024
@ikevin127
Copy link
Contributor

cc @adelekennedy

@adelekennedy
Copy link

adelekennedy commented Aug 23, 2024

Payment due date

Payouts due:

Upwork job is here.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Aug 25, 2024
@adelekennedy
Copy link

@ShridharGoel bump on accepting the job!

@adelekennedy
Copy link

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 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests