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

[AWAITING CHECKLIST] [HOLD for payment 2025-02-06] [$125] Split expense should not be the first option in the create menu from a Workspace chat #55205

Open
1 of 8 tasks
m-natarajan opened this issue Jan 14, 2025 · 45 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

@m-natarajan
Copy link

m-natarajan commented Jan 14, 2025

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: 9.0.84-6
Reproducible in staging?: y
Reproducible in production?: y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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: @shawnborton
Slack conversation (hyperlinked to channel name): convert

Action Performed:

  1. Open a workspace chat
  2. Click on +

Expected Result:

First option should be "Submit expense"

Actual Result:

First option is "Split expense"

Workaround:

unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021879250726854737335
  • Upwork Job ID: 1879250726854737335
  • Last Price Increase: 2025-02-07
  • Automatic offers:
    • jaydamani | Contributor | 105808729
Issue OwnerCurrent Issue Owner: @garrettmknight
@m-natarajan m-natarajan added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jan 14, 2025
Copy link

melvin-bot bot commented Jan 14, 2025

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

@thelullabyy
Copy link
Contributor

thelullabyy commented Jan 14, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-01-20 17:00:20 UTC.

Proposal

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

Split expense should not be the first option in the create menu from a Workspace chat

What is the root cause of that problem?

App/src/libs/ReportUtils.ts

Lines 7285 to 7300 in 328ca8a

if (canRequestMoney(report, policy, otherParticipants)) {
options = [...options, CONST.IOU.TYPE.SUBMIT];
if (!filterDeprecatedTypes) {
options = [...options, CONST.IOU.TYPE.REQUEST];
}
// If the user can request money from the workspace report, they can also track expenses
if (isPolicyExpenseChat(report) || isExpenseReport(report)) {
options = [...options, CONST.IOU.TYPE.TRACK];
}
}
// Pay someone option should be visible only in 1:1 DMs
if (isDM(report) && hasSingleParticipantInReport) {
options = [...options, CONST.IOU.TYPE.PAY];
if (!filterDeprecatedTypes) {

In getMoneyRequestOptions, we are putting the Split expense option above the Create expense option

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

In getMoneyRequestOptions we need to move Split expense to below the Create expense option

if (canRequestMoney(report, policy, otherParticipants)) {

    if (canRequestMoney(report, policy, otherParticipants)) {
        options = [CONST.IOU.TYPE.SUBMIT];
        if (!filterDeprecatedTypes) {
            options = [...options, CONST.IOU.TYPE.REQUEST];
        }

        // If the user can request money from the workspace report, they can also track expenses
        if (isPolicyExpenseChat(report) || isExpenseReport(report)) {
            options = [...options, CONST.IOU.TYPE.TRACK];
        }
    }

    // User created policy rooms and default rooms like #admins or #announce will always have the Split Expense option
    // unless there are no other participants at all (e.g. #admins room for a policy with only 1 admin)
    // DM chats will have the Split Expense option.
    // Your own workspace chats will have the split expense option.
    if (
        (isChatRoom(report) && !isAnnounceRoom(report) && otherParticipants.length > 0) ||
        (isDM(report) && otherParticipants.length > 0) ||
        (isGroupChat(report) && otherParticipants.length > 0) ||
        (isPolicyExpenseChat(report) && report?.isOwnPolicyExpenseChat)
    ) {
        options = [...options, CONST.IOU.TYPE.SPLIT];
    }

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We should move this block to an util function to get moneyRequestOptions

const moneyRequestOptions = useMemo(() => {

And then, need to write an unit test to make sure that the order of options is correct

What alternative solutions did you explore? (Optional)

@shawnborton
Copy link
Contributor

@garrettmknight I think we should consider halving or quartering the price on this one, since it should be a very simple change.

@jaydamani
Copy link
Contributor

jaydamani commented Jan 14, 2025

Proposal

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

Split expense is shown above Submit expense

What is the root cause of that problem?

getMoneyRequestOptions returns the list of money request options and split expense is above submit expense in this.

App/src/libs/ReportUtils.ts

Lines 7149 to 7178 in 89a18c6

if (
(isChatRoom(report) && !isAnnounceRoom(report) && otherParticipants.length > 0) ||
(isDM(report) && otherParticipants.length > 0) ||
(isGroupChat(report) && otherParticipants.length > 0) ||
(isPolicyExpenseChat(report) && report?.isOwnPolicyExpenseChat)
) {
options = [CONST.IOU.TYPE.SPLIT];
}
if (canRequestMoney(report, policy, otherParticipants)) {
options = [...options, CONST.IOU.TYPE.SUBMIT];
if (!filterDeprecatedTypes) {
options = [...options, CONST.IOU.TYPE.REQUEST];
}
// If the user can request money from the workspace report, they can also track expenses
if (isPolicyExpenseChat(report) || isExpenseReport(report)) {
options = [...options, CONST.IOU.TYPE.TRACK];
}
}
// Pay someone option should be visible only in 1:1 DMs
if (isDM(report) && hasSingleParticipantInReport) {
options = [...options, CONST.IOU.TYPE.PAY];
if (!filterDeprecatedTypes) {
options = [...options, CONST.IOU.TYPE.SEND];
}
}
return options;

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

Update getMoneyRequestOptions to return submit exepnse first.
Updated code:

function getMoneyRequestOptions(){
    ...

    if (canRequestMoney(report, policy, otherParticipants)) {
        options = [...options, CONST.IOU.TYPE.SUBMIT];
        if (!filterDeprecatedTypes) {
            options = [...options, CONST.IOU.TYPE.REQUEST];
        }

        // If the user can request money from the workspace report, they can also track expenses
        if (isPolicyExpenseChat(report) || isExpenseReport(report)) {
            options = [...options, CONST.IOU.TYPE.TRACK];
        }
    }


    if (
        (isChatRoom(report) && !isAnnounceRoom(report) && otherParticipants.length > 0) ||
        (isDM(report) && otherParticipants.length > 0) ||
        (isGroupChat(report) && otherParticipants.length > 0) ||
        (isPolicyExpenseChat(report) && report?.isOwnPolicyExpenseChat)
    ) {
        options = [...options, CONST.IOU.TYPE.SPLIT];
    }

    // Pay someone option should be visible only in 1:1 DMs
    if (isDM(report) && hasSingleParticipantInReport) {
        options = [...options, CONST.IOU.TYPE.PAY];
        if (!filterDeprecatedTypes) {
            options = [...options, CONST.IOU.TYPE.SEND];
        }
    }

    return options;

I think we should also replace [...options, SPLIT] with options.push(SPLIT) for code readability

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Add unit test to check submit expense is first when an applicable workspace chat is passed to getMoneyRequestOptions or temporary_getMoneyRequestOptions

What alternative solutions did you explore? (Optional)

Add sortOrder property here then sort using that property here

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@jaydamani
Copy link
Contributor

@shawnborton just wanted to confirm if it is fine to have split expense below track expense because the logic for submit and track is kinda tied together for workspaces.
so, the updated order will be

  1. Submit Expense
  2. Track Expense
  3. Split Expense

@misamikasa
Copy link

misamikasa commented Jan 14, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-01-16 17:21:00 UTC.

Proposal

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

Split expense should not be the first option in the create menu from a Workspace chat

What is the root cause of that problem?

Split Expense is above Submit Expense here:

App/src/libs/ReportUtils.ts

Lines 7272 to 7283 in 0e7af01

// User created policy rooms and default rooms like #admins or #announce will always have the Split Expense option
// unless there are no other participants at all (e.g. #admins room for a policy with only 1 admin)
// DM chats will have the Split Expense option.
// Your own workspace chats will have the split expense option.
if (
(isChatRoom(report) && !isAnnounceRoom(report) && otherParticipants.length > 0) ||
(isDM(report) && otherParticipants.length > 0) ||
(isGroupChat(report) && otherParticipants.length > 0) ||
(isPolicyExpenseChat(report) && report?.isOwnPolicyExpenseChat)
) {
options = [CONST.IOU.TYPE.SPLIT];
}

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

If we only want to move the Submit Expense to the first option we can do the following:

  • Remove this code
  • And add the following code above Split Expense
    if (canRequestMoney(report, policy, otherParticipants)) {
        options = [...options, CONST.IOU.TYPE.SUBMIT];
    }
  • And change this code to options = [...options, CONST.IOU.TYPE.SPLIT];

If we want to move Submit Expense, Request Expense, & Track Expense above Split Expense we can do the following:

  • move this block of code above Split Expense
  • And change this code to options = [...options, CONST.IOU.TYPE.SPLIT];

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Create new test to check if the Submit Expense is on the first option if the user can submit an expense

It should be a unit test under this to check temporary_getMoneyRequestOptions function return a data with first value in the options data is submit expense if it is user's own policy expense chat

or we can simply add expect(moneyRequestOptions[0]).toBe(CONST.IOU.TYPE.SUBMIT); here

What alternative solutions did you explore? (Optional)

Copy link

melvin-bot bot commented Jan 14, 2025

📣 @misamikasa! 📣
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>

@misamikasa

This comment has been minimized.

Copy link

melvin-bot bot commented Jan 14, 2025

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@huult
Copy link
Contributor

huult commented Jan 14, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-01-14 14:18:06 UTC.

🚨 Edited by proposal-police: This proposal was edited at 2025-01-14 14:18:06 UTC.

Proposal

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

Split expense should not be the first option in the create menu from a Workspace chat

What is the root cause of that problem?

Split expense alway on above submit/create/track expense by this code

options = [CONST.IOU.TYPE.SPLIT];

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

To update this issue we will update return split expense option bellow submit/create/track expense

function getMoneyRequestOptions(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>, reportParticipants: number[], filterDeprecatedTypes = false): IOUType[] {

update to

function getMoneyRequestOptions(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>, reportParticipants: number[], filterDeprecatedTypes = false): IOUType[] {
    // In any thread or task report, we do not allow any new expenses yet
    if (isChatThread(report) || isTaskReport(report) || isInvoiceReport(report) || isSystemChat(report) || isArchivedReport(report)) {
        return [];
    }

    if (isInvoiceRoom(report)) {
        if (canSendInvoiceFromWorkspace(policy?.id) && isPolicyAdmin(report?.policyID, allPolicies)) {
            return [CONST.IOU.TYPE.INVOICE];
        }
        return [];
    }

    // We don't allow IOU actions if an Expensify account is a participant of the report, unless the policy that the report is on is owned by an Expensify account
    const doParticipantsIncludeExpensifyAccounts = lodashIntersection(reportParticipants, CONST.EXPENSIFY_ACCOUNT_IDS).length > 0;
    const policyOwnerAccountID = getPolicy(report?.policyID)?.ownerAccountID;
    const isPolicyOwnedByExpensifyAccounts = policyOwnerAccountID ? CONST.EXPENSIFY_ACCOUNT_IDS.includes(policyOwnerAccountID) : false;
    if (doParticipantsIncludeExpensifyAccounts && !isPolicyOwnedByExpensifyAccounts) {
        return [];
    }

    const otherParticipants = reportParticipants.filter((accountID) => currentUserPersonalDetails?.accountID !== accountID);
    const hasSingleParticipantInReport = otherParticipants.length === 1;
    let options: IOUType[] = [];

    if (isSelfDM(report)) {
        options = [CONST.IOU.TYPE.TRACK];
    }

    // Determine if the report supports SPLIT expense or should reset options
    const hasOtherParticipants = otherParticipants.length > 0;
    const isEligibleForSplitExpense =
        (isChatRoom(report) && !isAnnounceRoom(report) && hasOtherParticipants) ||
        (isDM(report) && hasOtherParticipants) ||
        (isGroupChat(report) && hasOtherParticipants) ||
        (isPolicyExpenseChat(report) && report?.isOwnPolicyExpenseChat);

    if (isEligibleForSplitExpense) {
        options = [];
    }

    if (canRequestMoney(report, policy, otherParticipants)) {
        options.push(CONST.IOU.TYPE.SUBMIT);

        if (!filterDeprecatedTypes) {
            options.push(CONST.IOU.TYPE.REQUEST);
        }

        // Add TRACK option for workspace or expense reports
        if (isPolicyExpenseChat(report) || isExpenseReport(report)) {
            options.push(CONST.IOU.TYPE.TRACK);
        }
    }

    // Add SPLIT option for eligible reports
    if (isEligibleForSplitExpense) {
        options.push(CONST.IOU.TYPE.SPLIT);
    }

    // Pay someone option should be visible only in 1:1 DMs
    if (isDM(report) && hasSingleParticipantInReport) {
        options = [...options, CONST.IOU.TYPE.PAY];
        if (!filterDeprecatedTypes) {
            options = [...options, CONST.IOU.TYPE.SEND];
        }
    }

    return options;
}

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

we create test to check return options data of getMoneyRequestOptions

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@jaydamani
Copy link
Contributor

jaydamani commented Jan 14, 2025

@huult Please check your "Restate the problem" section. Not sure how this damages expensify cards 😅

@shawnborton
Copy link
Contributor

just wanted to confirm if it is fine to have split expense below track expense because the logic for submit and track is kinda tied together for workspaces.

Yes I think that's totally fine.

@garrettmknight garrettmknight added the External Added to denote the issue can be worked on by a contributor label Jan 14, 2025
@melvin-bot melvin-bot bot changed the title Split expense should not be the first option in the create menu from a Workspace chat [$250] Split expense should not be the first option in the create menu from a Workspace chat Jan 14, 2025
Copy link

melvin-bot bot commented Jan 14, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 14, 2025
@garrettmknight garrettmknight moved this to In Progress in [#whatsnext] #convert Jan 14, 2025
Copy link

melvin-bot bot commented Jan 14, 2025

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

@hoangzinh
Copy link
Contributor

Thank you for making proposals everyone.

@hoangzinh
Copy link
Contributor

In getMoneyRequestOptions we need to move Split expense to above the Create expense option

@thelullabyy it seems your solution is incorrect. Could you check it again?

@hoangzinh
Copy link
Contributor

I think we should also replace [...options, SPLIT] with options.split(SPLIT)

@jaydamani can you elaborate on this propose? Why do we need to split an array? Thank you.

@hoangzinh
Copy link
Contributor

Create new test to check if the Submit Expense is on the first option if the user can submit an expense

@misamikasa your proposal looks good as a new contributor. Can you elaborate it is a unit test for a function or UI tests?

@hoangzinh
Copy link
Contributor

// Determine if the report supports SPLIT expense or should reset options

@huult Please check your "re-state the problem" section, it looks incorrect. Also, can you elaborate why we need to reset options in this case?

@huult
Copy link
Contributor

huult commented Jan 16, 2025

@hoangzinh

Please check your "re-state the problem" section, it looks incorrect.

it's done

Also, can you elaborate why we need to reset options in this case?

old

App/src/libs/ReportUtils.ts

Lines 7268 to 7275 in 6f446da

if (
(isChatRoom(report) && !isAnnounceRoom(report) && otherParticipants.length > 0) ||
(isDM(report) && otherParticipants.length > 0) ||
(isGroupChat(report) && otherParticipants.length > 0) ||
(isPolicyExpenseChat(report) && report?.isOwnPolicyExpenseChat)
) {
options = [CONST.IOU.TYPE.SPLIT];
}

Resetting options with options = [] when isEligibleForSplitExpense ensures that previously added options are cleared. This avoids conflicts or incorrect options being displayed in reports eligible for SPLIT expenses.

@jaydamani
Copy link
Contributor

I think we should also replace [...options, SPLIT] with options.split(SPLIT)

@jaydamani can you elaborate on this propose? Why do we need to split an array? Thank you.

It was meant to be push, I have updated the proposal. This is mostly for code readability as [...options, SPLIT] is a bit weird so, this is an optional change

@hoangzinh
Copy link
Contributor

@thelullabyy your proposal has correct RCA. But your solution has an issue, it should be options = [...options, CONST.IOU.TYPE.SUBMIT]; to avoid resetting previous options. Also, your automated test suggestion doesn't look good to me. We can just need to write unit tests for either getMoneyRequestOptions or temporary_getMoneyRequestOptions

@hoangzinh
Copy link
Contributor

@misamikasa your proposal has correct RCA and solution for me. But it seems it's almost the same approach as @jaydamani's proposal

@hoangzinh
Copy link
Contributor

@huult your proposal has the correct RCA and solution for me as well. But I don't really like the solution as it will add complexity to the codebase, which I don't think it's necessary except if you can give me any testing steps to prove that we really need those conditions.

@hoangzinh
Copy link
Contributor

@jaydamani proposal looks good to me. He is first one who provided correct root cause, solution and automated tests.

Link to proposal #55205 (comment)

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Jan 22, 2025

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

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

melvin-bot bot commented Jan 22, 2025

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

@jaydamani
Copy link
Contributor

Working on this, PR should be up in few hours

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 22, 2025
@jaydamani
Copy link
Contributor

created PR #55634

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 30, 2025
@melvin-bot melvin-bot bot changed the title [$250] Split expense should not be the first option in the create menu from a Workspace chat [HOLD for payment 2025-02-06] [$250] Split expense should not be the first option in the create menu from a Workspace chat Jan 30, 2025
Copy link

melvin-bot bot commented Jan 30, 2025

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 30, 2025
Copy link

melvin-bot bot commented Jan 30, 2025

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

If no regressions arise, payment will be issued on 2025-02-06. 🎊

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

Copy link

melvin-bot bot commented Jan 30, 2025

@hoangzinh @garrettmknight @hoangzinh 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]

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 5, 2025
@garrettmknight garrettmknight changed the title [HOLD for payment 2025-02-06] [$250] Split expense should not be the first option in the create menu from a Workspace chat [HOLD for payment 2025-02-06] [$125] Split expense should not be the first option in the create menu from a Workspace chat Feb 7, 2025
Copy link

melvin-bot bot commented Feb 7, 2025

Upwork job price has been updated to $125

@garrettmknight
Copy link
Contributor

@jaydamani can you fill out the checklist when you get a chance?

@garrettmknight garrettmknight changed the title [HOLD for payment 2025-02-06] [$125] Split expense should not be the first option in the create menu from a Workspace chat [AWAITING CHECKLIST] [HOLD for payment 2025-02-06] [$125] Split expense should not be the first option in the create menu from a Workspace chat Feb 7, 2025
@jaydamani
Copy link
Contributor

jaydamani commented Feb 7, 2025

@garrettmknight @hoangzinh is the C+ for this. I am the contributor. Is there a checklist for contributors? :D

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
Status: In Progress
Development

No branches or pull requests

9 participants