[HOLD for payment 2024-08-01] [$250] Invoice - Invoice thread opens when starting DM with the user after sending invoice to them

lanitochka17 commented 1 month 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.6-0 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 FAB > Send invoice
Enter amount and select any receiver
Go to FAB > Start chat > Chat
Scroll down and click on the same user in Step 3 (invoice receiver)

Expected Result:

DM with the user will open

Actual Result:

Invoice thread with the user opens

Workaround:

Unknown

Platforms:

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

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/f0529ee6-dbab-4f63-9714-956f7a1f8240

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0165c9231ff254ae89
Upwork Job ID: 1811427918558506425
Last Price Increase: 2024-07-11
Automatic offers:

rayane-djouah | Reviewer | 103140621

Issue Owner

Current Issue Owner: @trjExpensify

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @trjExpensify (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 1 month ago

@trjExpensify 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

bernhardoj commented 1 month ago

Proposal

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

An invoice thread opens when we select the user from new chat page.

What is the root cause of that problem?

When we select a user, it will try to find the user chat by participants using getChatByParticipants. https://github.com/Expensify/App/blob/a19ef9889c85991414943da1515abe811c05e31d/src/libs/ReportUtils.ts#L5561-L5584

The participants of the invoice report is the same as the DM participants, that is [currentUser, the other user]. Because they contains the same participant, getChatByParticipants has a chance to wrongly returns the wrong report, that is the invoice report instead of the DM.

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

Skip invoice report/room when comparing the participant. https://github.com/Expensify/App/blob/a19ef9889c85991414943da1515abe811c05e31d/src/libs/ReportUtils.ts#L5567-L5577

|| isInvoiceReport(report) || isInvoiceRoom(report)

(and any other report type if needed)

What alternative solutions did you explore? (Optional)

I want to propose using !isOneOnOneChat, but getChatByParticipants is also being used to find group chat in split bill. https://github.com/Expensify/App/blob/a19ef9889c85991414943da1515abe811c05e31d/src/libs/actions/IOU.ts#L3806-L3809

So, the solution is if we aren't searching for group, then we use !isOneOnOneChat(report) condition.

|| (!shouldIncludeGroupChats && !isOneOnOneChat(report))

Then, we can remove the other checks (thread, group, task, etc.)

trjExpensify commented 1 month ago

@cristipaval @davidcardoza for eyes on this.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

nkdengineer commented 1 month ago

Proposal

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

Invoice - Invoice thread opens when starting DM with the user after sending invoice to them

What is the root cause of that problem?

When we start DM with the user, we use getChatByParticipants function to find the report of the DM chat. In this function, we will exclude some types of reports and then compare the participants. But we don't exclude the invoice report and this report has the same participant with the DM chat. So sometimes this function can return wrongly if we find the invoice report first.

https://github.com/Expensify/App/blob/77d95c54b3b4d9d9eb7db560f150137de433f657/src/libs/ReportUtils.ts#L5544-L5549

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

To avoid this bug happening in the future, we can prioritize the one one chat and group chat. With this change, we can make sure that this function will always return the DM or group first if the participant is matched.

return lodashOrderBy(Object.values(reports ?? {}), (report) => {
    if (isOneOnOneChat(report) || (isGroupChat(report) && shouldIncludeGroupChats)) {
        return 0;
    }
    return 1;
}).find((report) => {

https://github.com/Expensify/App/blob/77d95c54b3b4d9d9eb7db560f150137de433f657/src/libs/ReportUtils.ts#L5540

What alternative solutions did you explore? (Optional)

rayane-djouah commented 1 month ago

Reviewing proposals

rayane-djouah commented 1 month ago

@bernhardoj's proposal looks good to me.

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

📣 @rayane-djouah 🎉 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 commented 1 month ago

PR is ready

cc: @rayane-djouah

trjExpensify commented 1 month ago

Deployed to staging 4 hours ago.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

https://github.com/Expensify/App/pull/45500

If no regressions arise, payment will be issued on 2024-08-01. :confetti_ball:

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

@bernhardoj requires payment through NewDot Manual Requests
@rayane-djouah requires payment automatic offer (Reviewer)

melvin-bot[bot] commented 1 month ago

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:

[ ] [@rayane-djouah] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@rayane-djouah] 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:
[ ] [@rayane-djouah] 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:
[ ] [@rayane-djouah] Determine if we should create a regression test for this bug.
[ ] [@rayane-djouah] 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.
[ ] [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

trjExpensify commented 3 weeks ago

Checklist time!

bernhardoj commented 3 weeks ago

Requested in ND

rayane-djouah commented 3 weeks ago

[x] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/40303
[x] 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/40303#issuecomment-2265775140
[x] 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
[x] Determine if we should create a regression test for this bug. There's no need; we've already included automated tests.
[x] 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. N/A

cc @trjExpensify

trjExpensify commented 3 weeks ago

Determine if we should create a regression test for this bug. There's no need; we've already included automated tests.

Hm, I don't think we forgo manual regression testing of features in lieu of automated tests. @cristipaval @davidcardoza are manual regression tests being handed over and added centrally for this project?

trjExpensify commented 3 weeks ago

Payment summary as follows:

$250 to @bernhardoj for the fix
$250 to @rayane-djouah for the C+ review

trjExpensify commented 3 weeks ago

^^ going to hold on the Upwork payment for C+ until we're aligned on this Q.

rayane-djouah commented 3 weeks ago

@trjExpensify - Below are the manual regression test steps if we decide to include them:

Regression Test Proposal

Send Invoice flow:
1. Create a New Account
2. Click on the Floating Action Button (FAB).
3. Select "Send Invoice".
4. Choose a recipient with whom you have no prior interactions.
5. Complete the invoice details and send it.
6. Click on FAB again.
7. Choose "Start Chat".
8. Select the same recipient as in Step 4.
9. Verify that the chat that opens should be a direct message (DM) chat, not the invoice report chat.
Submit expense flow:
1. Create a New Account
2. Click on the Floating Action Button (FAB).
3. Select "Submit expense".
4. Choose a recipient with whom you have no prior interactions.
5. Complete the expense details and send it.
6. Click on FAB again.
7. Choose "Start Chat".
8. Select the same recipient as in Step 4.
9. Verify that the chat that opens should be a direct message (DM) chat, not the expense report chat.

Do we agree 👍 or 👎

trjExpensify commented 3 weeks ago

Cool, thanks! Settled up. 👍

JmillsExpensify commented 3 weeks ago

$250 approved for @bernhardoj

Expensify / App