[HOLD for payment 2024-07-24] [$250] Concierge - There is submit, split and pay options in Concierge chat #44985

lanitochka17 · 2024-07-08T16:15:34Z

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.5-4
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:

Go to staging.new.expensify.com
Go to Concierge chat
Click +

Expected Result:

There will be no submit, split and pay options in Concierge chat

Actual Result:

There is submit, split and pay options in Concierge chat

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6536003_1720451847402.20240708_231508.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01bee66e73dbacfc76
Upwork Job ID: 1810949265008721349
Last Price Increase: 2024-07-10
Automatic offers:

paultsimura | Reviewer | 103077735

Issue Owner

Current Issue Owner: @dylanexpensify

melvin-bot · 2024-07-08T16:15:44Z

Triggered auto assignment to @dylanexpensify (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 · 2024-07-08T16:15:58Z

@dylanexpensify 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 · 2024-07-08T16:16:28Z

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

Krishna2323 · 2024-07-08T16:24:33Z

Proposal

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

Concierge - There is submit, split and pay options in Concierge chat

What is the root cause of that problem?

We aren't checking for expensify only chats in getMoneyRequestOptions. We check for that before adding taskOption.

App/src/libs/ReportUtils.ts

Lines 5850 to 5854 in 547336d

    
           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)) { 
        
                   return []; 
        
               }

App/src/libs/ReportUtils.ts

Lines 1132 to 1142 in 547336d

    
           function canCreateTaskInReport(report: OnyxEntry<Report>): boolean { 
        
               const otherParticipants = Object.keys(report?.participants ?? {}) 
        
                   .map(Number) 
        
                   .filter((accountID) => accountID !== currentUserAccountID); 
        
               const areExpensifyAccountsOnlyOtherParticipants = otherParticipants.length >= 1 && otherParticipants.every((accountID) => CONST.EXPENSIFY_ACCOUNT_IDS.includes(accountID)); 
        
               if (areExpensifyAccountsOnlyOtherParticipants && isDM(report)) { 
        
                   return false; 
        
               } 
        
               return true; 
        
           }

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

Use || isExpensifyOnlyParticipantInReport(report) here so it returns empty array.

What alternative solutions did you explore? (Optional)

We can use isExpensifyOnlyParticipantInReport in AttachmentPickerWithMenuItems if we don't want to add that in getMoneyRequestOptions.

ShridharGoel · 2024-07-08T16:43:16Z

Proposal

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

There is submit, split and pay options in Concierge chat.

What is the root cause of that problem?

In the below code, there's no check to return empty list for concierge chat:

App/src/libs/ReportUtils.ts

Lines 5851 to 5854 in 547336d

    
           // In any thread or task report, we do not allow any new expenses yet 
        
           if (isChatThread(report) || isTaskReport(report) || isInvoiceReport(report) || isSystemChat(report)) { 
        
               return []; 
        
           }

Also, there's another bug. isConciergeChatReport logic doesn't filter out the logged in user:

App/src/libs/ReportUtils.ts

Lines 1046 to 1049 in 547336d

    
           function isConciergeChatReport(report: OnyxInputOrEntry<Report>): boolean { 
        
               const participantAccountIDs = Object.keys(report?.participants ?? {}); 
        
               return participantAccountIDs.length === 1 && Number(participantAccountIDs[0]) === CONST.ACCOUNT_ID.CONCIERGE && !isChatThread(report); 
        
           }

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

Update isConciergeChatReport method to filter out the logged in user:

function isConciergeChatReport(report: OnyxInputOrEntry<Report>): boolean {
    const participantAccountIDs = Object.keys(report?.participants ?? {})
        .map(Number)
        .filter((accountID) => accountID !== currentUserAccountID);
    return participantAccountIDs.length === 1 && Number(participantAccountIDs[0]) === CONST.ACCOUNT_ID.CONCIERGE && !isChatThread(report);
}

Then, add || isConciergeChatReport(report) check in getMoneyRequestOptions.

if (isChatThread(report) || isTaskReport(report) || isInvoiceReport(report) || isSystemChat(report) || isConciergeChatReport(report)) {
    return [];
}

Else, we can also use isExpensifyOnlyParticipantInReport instead of just concierge check if we want to exclude all accounts in CONST.EXPENSIFY_ACCOUNT_IDS.

bernhardoj · 2024-07-09T05:39:04Z

Proposal

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

There is money request option shown in Concierge chat.

What is the root cause of that problem?

Currently, we already have a condition to not show money request options for expensify chat including Concierge.

App/src/libs/ReportUtils.ts

Lines 5862 to 5867 in dd96852

    
           // 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 isPolicyOwnedByExpensifyAccounts = report?.policyID ? CONST.EXPENSIFY_ACCOUNT_IDS.includes(getPolicy(report?.policyID ?? '-1')?.ownerAccountID ?? -1) : false; 
        
           if (doParticipantsIncludeExpensifyAccounts && !isPolicyOwnedByExpensifyAccounts) { 
        
               return []; 
        
           }

However, isPolicyOwnedByExpensifyAccounts is always true. For Concierge, the policyID is __FAKE__ because it's not tied to a policy. Because it's not tied to a policy, the ownerAccountID defaults to -1 and when we check CONST.EXPENSIFY_ACCOUNT_IDS.includes(-1), it returns true.

(This happens after the default to -1 PR. Previously, the default ownerAccountID is 0)

That's because in the list, there are some account IDs that default to -1.

App/src/CONST.ts

Line 1570 in dd96852

ADMIN: Number(Config?.EXPENSIFY_ACCOUNT_ID_ADMIN ?? -1),

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

Filter out account IDs of -1.

App/src/CONST.ts

Lines 2275 to 2294 in dd96852

    
           get EXPENSIFY_ACCOUNT_IDS() { 
        
               return [ 
        
                   this.ACCOUNT_ID.ACCOUNTING, 
        
                   this.ACCOUNT_ID.ADMIN, 
        
                   this.ACCOUNT_ID.BILLS, 
        
                   this.ACCOUNT_ID.CHRONOS, 
        
                   this.ACCOUNT_ID.CONCIERGE, 
        
                   this.ACCOUNT_ID.CONTRIBUTORS, 
        
                   this.ACCOUNT_ID.FIRST_RESPONDER, 
        
                   this.ACCOUNT_ID.HELP, 
        
                   this.ACCOUNT_ID.INTEGRATION_TESTING_CREDS, 
        
                   this.ACCOUNT_ID.PAYROLL, 
        
                   this.ACCOUNT_ID.QA, 
        
                   this.ACCOUNT_ID.QA_TRAVIS, 
        
                   this.ACCOUNT_ID.RECEIPTS, 
        
                   this.ACCOUNT_ID.REWARDS, 
        
                   this.ACCOUNT_ID.STUDENT_AMBASSADOR, 
        
                   this.ACCOUNT_ID.SVFG, 
        
               ]; 
        
           },

OR

Only call includes when the ownerAccountID is > 0, otherwise return false.

App/src/libs/ReportUtils.ts

Line 5864 in dd96852

    
           const isPolicyOwnedByExpensifyAccounts = report?.policyID ? CONST.EXPENSIFY_ACCOUNT_IDS.includes(getPolicy(report?.policyID ?? '-1')?.ownerAccountID ?? -1) : false;

melvin-bot · 2024-07-10T08:08:46Z

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

dylanexpensify · 2024-07-10T08:08:47Z

moving

melvin-bot · 2024-07-10T08:08:51Z

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

paultsimura · 2024-07-10T08:31:33Z

Reviewing today 👀

paultsimura · 2024-07-10T13:05:41Z

@bernhardoj I like the idea of keeping only non- -1 IDs in EXPENSIFY_ACCOUNT_IDS.
If we go this way, do you think we should use some kind of memoization to prevent the .filter function from being called every time EXPENSIFY_ACCOUNT_IDS is used?

bernhardoj · 2024-07-10T13:20:14Z

I think we can move the const definition outside the CONST object.

App/src/CONST.ts

Lines 66 to 77 in 97b3831

    
           const onboardingChoices = { 
        
               PERSONAL_SPEND: 'newDotPersonalSpend', 
        
               MANAGE_TEAM: 'newDotManageTeam', 
        
               EMPLOYER: 'newDotEmployer', 
        
               CHAT_SPLIT: 'newDotSplitChat', 
        
               LOOKING_AROUND: 'newDotLookingAround', 
        
           }; 
        
           type OnboardingPurposeType = ValueOf<typeof onboardingChoices>; 
        
           const CONST = { 
        
               RECENT_WAYPOINTS_NUMBER: 20,

const ACCOUNT_ID = {
    ACCOUNTING: Number(Config?.EXPENSIFY_ACCOUNT_ID_ACCOUNTING ?? 9645353),
    ...,
}
const EXPENSIFY_ACCOUNT_IDS = Object.values(ACCOUNT_ID).filter(accountID => accountID !== -1)

const CONST = {
    ...,
    ACCOUNT_ID,
    ...,
    EXPENSIFY_ACCOUNT_IDS,
}

paultsimura · 2024-07-10T15:45:47Z

Thanks for your proposals, everybody.
I'd go with the proposal by @bernhardoj since he's found the lowest level root cause (defaulting to -1) that broke down the otherwise functioning logic, while the other proposals offer an easier fix with no deep RCA.

I like the idea of returning only non- -1 account IDs from EXPENSIFY_ACCOUNT_IDS. As for the optimization – let's just use the .filter(accountID => accountID !== -1) approach without moving it outside of the CONST object – I've checked some metrics and the total execution time does not change within 0.001 ms, so I think we're good to go this way. But maybe the assigned Engineer will object to this 🤔

🎀👀🎀 C+ reviewed

melvin-bot · 2024-07-10T15:45:59Z

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

melvin-bot · 2024-07-11T12:43:16Z

📣 @paultsimura 🎉 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

bernhardoj · 2024-07-11T14:59:16Z

PR is ready

cc: @paultsimura

melvin-bot · 2024-07-17T00:21:04Z

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

melvin-bot · 2024-07-17T00:21:09Z

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

Fix money request options show in Concierge chat #45294

If no regressions arise, payment will be issued on 2024-07-24. 🎊

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

@paultsimura requires payment automatic offer (Reviewer)
@bernhardoj requires payment through NewDot Manual Requests

melvin-bot · 2024-07-17T00:21:10Z

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[@paultsimura] The PR that introduced the bug has been identified. Link to the PR:
[@paultsimura] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[@paultsimura] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[@paultsimura] Determine if we should create a regression test for this bug.
[@paultsimura] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[@dylanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

dylanexpensify · 2024-07-22T12:38:55Z

Payment coming up!

paultsimura · 2024-07-22T13:49:04Z

The PR that introduced the bug has been identified. Link to the PR: [TS Migration] Define a concise way to access or default to an inexistent record #42634
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/42634/files#r1686586602
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
Determine if we should create a regression test for this bug: Yes
If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

Log in as any account
Open the Concierge chat
Click "+"
Verify there are no payment-related options in the menu

Do we agree 👍 or 👎

dylanexpensify · 2024-07-24T08:18:29Z

Payment summary:

Contributor: @paultsimura $250
Contributor+: @bernhardoj $250

Please apply/request!

dylanexpensify · 2024-07-24T08:32:56Z

Done

JmillsExpensify · 2024-07-24T13:06:05Z

$250 approved for @bernhardoj

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 8, 2024

melvin-bot bot assigned dylanexpensify Jul 8, 2024

dylanexpensify added the External Added to denote the issue can be worked on by a contributor label Jul 10, 2024

melvin-bot bot changed the title ~~Concierge - There is submit, split and pay options in Concierge chat~~ [$250] Concierge - There is submit, split and pay options in Concierge chat Jul 10, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 10, 2024

melvin-bot bot assigned paultsimura Jul 10, 2024

melvin-bot bot assigned hayata-suenaga Jul 10, 2024

hayata-suenaga assigned bernhardoj Jul 11, 2024

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 11, 2024

bernhardoj mentioned this issue Jul 11, 2024

Fix money request options show in Concierge chat #45294

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jul 11, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jul 17, 2024

melvin-bot bot changed the title ~~[$250] Concierge - There is submit, split and pay options in Concierge chat~~ [HOLD for payment 2024-07-24] [$250] Concierge - There is submit, split and pay options in Concierge chat Jul 17, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Jul 17, 2024

paultsimura mentioned this issue Jul 22, 2024

[TS Migration] Define a concise way to access or default to an inexistent record #42634

Merged

50 tasks

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 23, 2024

dylanexpensify closed this as completed Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2024-07-24] [$250] Concierge - There is submit, split and pay options in Concierge chat #44985

[HOLD for payment 2024-07-24] [$250] Concierge - There is submit, split and pay options in Concierge chat #44985

lanitochka17 commented Jul 8, 2024 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Jul 8, 2024

lanitochka17 commented Jul 8, 2024

lanitochka17 commented Jul 8, 2024

Krishna2323 commented Jul 8, 2024 •

edited

Loading

ShridharGoel commented Jul 8, 2024 •

edited

Loading

bernhardoj commented Jul 9, 2024

melvin-bot bot commented Jul 10, 2024

dylanexpensify commented Jul 10, 2024

melvin-bot bot commented Jul 10, 2024

paultsimura commented Jul 10, 2024

paultsimura commented Jul 10, 2024

bernhardoj commented Jul 10, 2024

paultsimura commented Jul 10, 2024

melvin-bot bot commented Jul 10, 2024

melvin-bot bot commented Jul 11, 2024

bernhardoj commented Jul 11, 2024

melvin-bot bot commented Jul 17, 2024

melvin-bot bot commented Jul 17, 2024

melvin-bot bot commented Jul 17, 2024

dylanexpensify commented Jul 22, 2024

paultsimura commented Jul 22, 2024 •

edited

Loading

dylanexpensify commented Jul 24, 2024

dylanexpensify commented Jul 24, 2024

JmillsExpensify commented Jul 24, 2024

[HOLD for payment 2024-07-24] [$250] Concierge - There is submit, split and pay options in Concierge chat #44985

[HOLD for payment 2024-07-24] [$250] Concierge - There is submit, split and pay options in Concierge chat #44985

Comments

lanitochka17 commented Jul 8, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Jul 8, 2024

lanitochka17 commented Jul 8, 2024

lanitochka17 commented Jul 8, 2024

Krishna2323 commented Jul 8, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

ShridharGoel commented Jul 8, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

bernhardoj commented Jul 9, 2024

Proposal

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

What is the root cause of that problem?

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

melvin-bot bot commented Jul 10, 2024

dylanexpensify commented Jul 10, 2024

melvin-bot bot commented Jul 10, 2024

paultsimura commented Jul 10, 2024

paultsimura commented Jul 10, 2024

bernhardoj commented Jul 10, 2024

paultsimura commented Jul 10, 2024

melvin-bot bot commented Jul 10, 2024

melvin-bot bot commented Jul 11, 2024

bernhardoj commented Jul 11, 2024

melvin-bot bot commented Jul 17, 2024

melvin-bot bot commented Jul 17, 2024

melvin-bot bot commented Jul 17, 2024

dylanexpensify commented Jul 22, 2024

paultsimura commented Jul 22, 2024 • edited Loading

Regression Test Proposal

dylanexpensify commented Jul 24, 2024

dylanexpensify commented Jul 24, 2024

JmillsExpensify commented Jul 24, 2024

lanitochka17 commented Jul 8, 2024 •

edited by melvin-bot bot

Loading

Krishna2323 commented Jul 8, 2024 •

edited

Loading

ShridharGoel commented Jul 8, 2024 •

edited

Loading

paultsimura commented Jul 22, 2024 •

edited

Loading