Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.89k forks source link

[$250] IOU - In newly created workspace, with no members invited, split expense is displayed #46739

Open lanitochka17 opened 3 months ago

lanitochka17 commented 3 months 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.16 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch app
  2. Tap profile icon -- workspaces -- new workspace
  3. Navigate to newly created workspace in the list and using 3 dots on beside navigate to #admin room
  4. Tap plus icon near compose and select split expense.
  5. Enter an amount and tap next

Expected Result:

In newly created workspace, with no members invited, split expense option must not be displayed in #admin room. When no members invited in workspace, it must not show hidden member

Actual Result:

In newly created workspace, with no members invited, split expense option is displayed in #admin room. When trying to split expense, a hidden member is shown without invited

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

</details

https://github.com/user-attachments/assets/e01f812e-aab0-4f3a-bdb3-fa434f487b75

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0125de9b07488ab7a1
  • Upwork Job ID: 1820610996701136238
  • Last Price Increase: 2024-08-06
  • Automatic offers:
    • ahmedGaber93 | Reviewer | 103461530
Issue OwnerCurrent Issue Owner: @ahmedGaber93
melvin-bot[bot] commented 3 months ago

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.

lanitochka17 commented 3 months ago

@adelekennedy 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 3 months ago

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

rlmorrison74 commented 3 months ago

I really apologize for commenting here, but I don't know where else to go that someone from expensify might see it. I emailed contributors@expensify.com a week ago requesting access to slack, along with a follow up a few days ago, and I haven't heard back. Are we just not accepting new contributors at the moment, or is there some other step I need to follow to gain access? Can someone please advise? Again, sorry for hijacking an issue thread.

melvin-bot[bot] commented 3 months ago

πŸ“£ @rlmorrison74! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
abzokhattab commented 3 months ago

The issue is not reproducible on the latest main ... tried on IOS

kpadmanabhan commented 3 months ago

Proposal

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

Workspace with 0 members shows Split expense option.

What is the root cause of that problem?

We do not check count of members for displaying Split expense option in context menu.

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

Check for members count before showing the option in src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.tsx

What alternative solutions did you explore? (Optional)

daledah commented 3 months ago

Proposal

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

In newly created workspace, with no members invited, split expense option is displayed in #admin room. When trying to split expense, a hidden member is shown without invited

What is the root cause of that problem?

Here is the condition to display the split option in admin room: https://github.com/Expensify/App/blob/8abdcca86923a5a121734f34bbc53f2eeb5a45dd/src/libs/ReportUtils.ts#L6103 with the otherParticipants is: https://github.com/Expensify/App/blob/8abdcca86923a5a121734f34bbc53f2eeb5a45dd/src/libs/ReportUtils.ts#L6090 which is not filtered out the participant that belongs to Expensify team.

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

The otherParticipants should be:

    const otherParticipants = reportParticipants.filter((accountID) => {
        const detail = allPersonalDetails?.[accountID];
        return currentUserPersonalDetails?.accountID !== accountID && !PolicyUtils.isExpensifyTeam(detail?.login);
    });

What alternative solutions did you explore? (Optional)

Beside above changes, we can apply the same filter in: https://github.com/Expensify/App/blob/8abdcca86923a5a121734f34bbc53f2eeb5a45dd/src/pages/iou/request/step/IOURequestStepAmount.tsx#L256 and https://github.com/Expensify/App/blob/8abdcca86923a5a121734f34bbc53f2eeb5a45dd/src/components/MoneyRequestConfirmationList.tsx#L452 to make sure the specialist is not included when splitting.

hayes102 commented 3 months ago

I am not also able to reproduce the issue ... the split option doesn't appear in the admin room

melvin-bot[bot] commented 3 months ago

πŸ“£ @hayes102! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
melvin-bot[bot] commented 3 months ago

@adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

adelekennedy commented 3 months ago

reproduced - I'm also able to reproduce this on web, chrome v9.0.16-5, IMO this is a bug, we shouldn't show the split option until additional members are invited

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

adelekennedy commented 3 months ago

@ahmedGaber93 already some proposals to review!

adelekennedy commented 3 months ago

@rlmorrison74 Let me search for your email in the inbox - I'll reach out via email fro next steps

ahmedGaber93 commented 3 months ago

I'm not able to reproduce on web, do I missed something?

https://github.com/user-attachments/assets/5ae41726-b6dd-4c14-b319-83d4e3f4dc3b

daledah commented 3 months ago

@ahmedGaber93 You can try the new account, with the first workspace in that account.

ahmedGaber93 commented 3 months ago

You can try the new account, with the first workspace in that account.

@daledah Still not able to reproduce

https://github.com/user-attachments/assets/b5383772-1d31-4ec7-b0c4-6dd0d6b71c41

daledah commented 3 months ago

Maybe my video will help you reproduce. From my side, after create a 1st workspace, I need to clear cache and restart.

https://github.com/user-attachments/assets/6cb28d5e-2936-4edf-ba87-a60386258d99

ahmedGaber93 commented 3 months ago

I am able to reproduce now. Thanks @daledah.

ahmedGaber93 commented 3 months ago

@daledah's proposal LGTM!

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

πŸ“£ @ahmedGaber93 πŸŽ‰ 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[bot] commented 3 months ago

πŸ“£ @daledah You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing πŸ“–

melvin-bot[bot] commented 3 months ago

@Gonals, @ahmedGaber93, @adelekennedy, @daledah Whoops! This issue is 2 days overdue. Let's get this updated quick!

ahmedGaber93 commented 3 months ago

Not overdue, PR in progress.

daledah commented 3 months ago

@ahmedGaber93 This PR is ready for review.

trjExpensify commented 2 months ago

This PR caused a pretty big regression where none of us with expensify.com emails can use the create options in DM: https://github.com/Expensify/App/issues/48282

ahmedGaber93 commented 2 months ago

@trjExpensify Yes, unfortunately this is a big regression, but the good news is the PR is not merged into production yet. I will create a revert PR now.

trjExpensify commented 2 months ago

Oh hm, but it's on a checklist that's closed? πŸ€” CC: @puneetlath is something broken?

image
puneetlath commented 2 months ago

We didn't do a production deploy for that checklist, so it's still on staging.

daledah commented 2 months ago

@ahmedGaber93 Do you think we need to revert the PR? I think we just need to create the follow up PR, which will update: https://github.com/Expensify/App/blob/fa8fa1a62d5346b3317c8c6a46088d6c584c1868/src/libs/ReportUtils.ts#L6398

        (accountID) => currentUserPersonalDetails?.accountID !== accountID && !PolicyUtils.isGuide(allPersonalDetails?.[accountID]?.login)

with isGuide is:

function isGuide(email: string | undefined): boolean {
    const emailDomain = Str.extractEmailDomain(email ?? '');
    return emailDomain === CONST.EMAIL.GUIDES_DOMAIN;
}
ahmedGaber93 commented 2 months ago

@daledah I think this not urgent issue and there is no need to rush, let's revert it before deploying to production then we can discuss the solution. May be emails with GUIDES_DOMAIN also need to use this feature on other scopes.

ahmedGaber93 commented 2 months ago

We didn't do a production deploy for that checklist, so it's still on staging.

@puneetlath is that's mean we have the chance to revert it before deploying to production?

@Gonals I DM you to revert PR https://github.com/Expensify/App/pull/47214, but it seems you are offline.

puneetlath commented 2 months ago

I merged and am CPing the revert.

puneetlath commented 2 months ago

Confirmed the options appear after reverting

image
ahmedGaber93 commented 2 months ago

@adelekennedy the expect behavior here is to hide the split option in admin room if the only other user in the room is guide team member and there is no other members.

The question here: is there is any case the guide member with email team.expensify.com can be normal user of admin room and not a guide member? For example, if we have a workspace for guide team, the guide team members here will be normal users on admin room not guide member

CC @trjExpensify @Gonals

adelekennedy commented 2 months ago

I'm not sure but my instinct is that they'd always be guide members but @trjExpensify or @Gonals for thoughts? I'll take this to Slack to confirm if we don't come to a conclusion here

trjExpensify commented 2 months ago

is there is any case the guide member with email team.expensify.com can be normal user of admin room and not a guide member? For example, if we have a workspace for guide team, the guide team members here will be normal users on admin room not guide member

What do you mean by normal user? They can have their own workspaces for testing and demos on that domain, if that's the question.

trjExpensify commented 2 months ago

Does anyone have the latest on what we do with Guides/AMs now? Do we just add them as a member of the #admins room, versus adding them to the workspace when assigned to a lead? CC: @puneetlath

If that's the case, then I think it's probably fine to just show the guide/AM as a member of the #admins room and thus as a participant here:

image
puneetlath commented 2 months ago

Does anyone have the latest on what we do with Guides/AMs now? Do we just add them as a member of the #admins room, versus adding them to the workspace when assigned to a lead? CC: @puneetlath

Yes, that's correct. We add them to the admins room, but not the workspace.

trjExpensify commented 2 months ago

Okay cool, then I think it's fine to show the guide/AM as a participant of the #admins room and not hide that. Meaning, they can be selected for a Split in the room, but I highly doubt anyone will do that in reality.

So what we should try to fix here is why they're showing as hidden. Agreed?

puneetlath commented 2 months ago

Makes sense to me. Hiding seems to cause more problems than benefits.

ahmedGaber93 commented 2 months ago

Thanks all for clearing the context.

ahmedGaber93 commented 2 months ago

@adelekennedy @daledah The "hidden" participant not reproduced with me in this case, it always displays the Guide member. Is there anyone able to reproduce it?

https://github.com/user-attachments/assets/1550c22a-162e-4792-8e2a-cc515b83dd3f

ahmedGaber93 commented 1 month ago

@daledah the issue is not reproduced on latest main with me. Can you confirm?

daledah commented 1 month ago

@ahmedGaber93 For me, on the lastest main, the Split expense option is not displayed:

https://github.com/user-attachments/assets/b3c02869-6def-4d3a-8af4-612171df3f3c

ahmedGaber93 commented 1 month ago

@daledah can you try to clear the cache like you described here https://github.com/Expensify/App/issues/46739#issuecomment-2273382189 then retest.

ahmedGaber93 commented 1 month ago

@daledah bump here https://github.com/Expensify/App/issues/46739#issuecomment-2354587449