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-07-24] Pay someone - System message thread does not show up on LHN #44586

Closed
6 tasks done
lanitochka17 opened this issue Jun 28, 2024 · 28 comments
Closed
6 tasks done
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering

Comments

@lanitochka17
Copy link

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

Issue found when executing PR #43742

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to any chat
  3. Go to + > Pay someone
  4. Enter amount and pay them
  5. Go to transaction thread
  6. Right click on paid system message > Reply in thread
  7. Send a reply

Expected Result:

System message thread will show up on LHN

Actual Result:

System message thread is missing on LHN

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

Add any screenshot/video evidence

Bug6526860_1719530886499.pay_some.mp4

View all open jobs on GitHub

@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 DeployBlocker Indicates it should block deploying the API labels Jun 28, 2024
Copy link

melvin-bot bot commented Jun 28, 2024

Triggered auto assignment to @hayata-suenaga (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 Jun 28, 2024
Copy link
Contributor

👋 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.

@lanitochka17
Copy link
Author

@hayata-suenaga 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

@dominictb
Copy link
Contributor

Proposal

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

System message thread is missing on LHN

What is the root cause of that problem?

When we pay someone, the paid system message is the send money action and the child of this action is transaction thread report.

If the report is the one transaction thread we don't show it in LHN here) but we still can access to this report . This bug also happens if we access to the transaction thread report of the combine report.

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

I don't think it makes sense to hide the transaction thread report in LHN if it's the one transaction thread because we still can access this report via deeplink or go back from the child thread of this report. I think we can remove this check.

App/src/libs/ReportUtils.ts

Lines 5434 to 5436 in 1d607ac

if (isOneTransactionThread(report.reportID, report.parentReportID ?? '-1')) {
return false;
}

What alternative solutions did you explore? (Optional)

Display not found page if it doesn't show in LHN.

@bernhardoj
Copy link
Contributor

Proposal

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

The paid system message thread doesn't show in LHN.

What is the root cause of that problem?

The paid system message thread is a one-transaction thread and we don't show a one-transaction thread in LHN.

App/src/libs/ReportUtils.ts

Lines 5433 to 5436 in 1d607ac

// If this is a transaction thread associated with a report that only has one transaction, omit it
if (isOneTransactionThread(report.reportID, report.parentReportID ?? '-1')) {
return false;
}

This issue is reproducible after #43742. Previously, the paid system message was combined into the one-transaction report so it's not possible for the user to open the paid system message thread manually, but now we show it so the users know the paid method.

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

Because we make an exception to the combined report actions for the paid system message, we should do the same for shouldReportBeInOptionList by ignoring it if it's a paid action.

if (isOneTransactionThread(report.reportID, report.parentReportID ?? '-1') && !ReportActionsUtils.isSentMoneyReportAction(parentReportAction)) {
    return false;
}

@hayata-suenaga
Copy link
Contributor

@bernhardoj's solution seems to address the cause. Assining @bernhardoj. @bernhardoj, is it possible to open a PR as soon as possible?

@bernhardoj
Copy link
Contributor

Here is the PR

@dominictb
Copy link
Contributor

dominictb commented Jun 28, 2024

@hayata-suenaga I believe we should remove this check, here is another case this bug still happen. It always happens when we open the transaction thread report of a combine report. In this case when we open the thread of change amount system message and go to the parent report, it's the transaction thread report and it doesn't show in LHN.

Screen.Recording.2024-06-28.at.15.36.11.mov

@bernhardoj
Copy link
Contributor

I believe that's because it's an empty report/thread. The paid system message doesn't show at all in LHN, while the one in your video shows but disappears when navigating away.

@dominictb
Copy link
Contributor

It's the same case.

  1. The child (the thread) of paid system message in the send money flow is the transaction thread report.

  2. And in the case below, the system message is the action of the transaction thread report. So when we go back to parent report of this thread it's also the transaction thread report.

@mountiny
Copy link
Contributor

@bernhardoj @hayata-suenaga Lets please pause on implementing this change, I think we might have to discuss a bit more, we would like to hide the transaction thread from the LHN in general.

I think we might want to show it for the Send Money flow as its a specific case, but if you consider the single expense Request money flow, then its a weird experience because the user cannot really go to the transaction thread from the expense report and the comments are intertwined.

cc @NikkiWines @trjExpensify

I will demote this from being a blocker, its very specific edge case and nobody is really using send money now

@mountiny mountiny removed DeployBlockerCash This issue or pull request should block deployment DeployBlocker Indicates it should block deploying the API labels Jun 28, 2024
@dominictb
Copy link
Contributor

@mountiny I think we can only show it in LHN if this report is focused.

@dominictb
Copy link
Contributor

I suggest we can move this block before the check one transaction thread here. So this report is only shown in LHN if it's focused and everything is fine.

App/src/libs/ReportUtils.ts

Lines 5441 to 5442 in 1d607ac

if (report.reportID === currentReportId) {
return true;

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Jul 17, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 17, 2024
Copy link

melvin-bot bot commented Jul 17, 2024

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

Copy link

melvin-bot bot commented Jul 17, 2024

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:

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

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

  • @bernhardoj requires payment through NewDot Manual Requests

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 23, 2024
@bernhardoj
Copy link
Contributor

Payment is due today. Requested in ND.

Copy link

melvin-bot bot commented Jul 24, 2024

Issue is ready for payment but no BZ is assigned. @muttmuure you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

Copy link

melvin-bot bot commented Jul 24, 2024

Payment Summary

Upwork Job

BugZero Checklist (@muttmuure)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@eVoloshchak
Copy link
Contributor

I'm eligible for payment for reviewing #44594, will finish the checklist tomorrow (I wonder why Melvin didn't post the BugZero Checklist)

@melvin-bot melvin-bot bot added the Overdue label Jul 26, 2024
@muttmuure
Copy link
Contributor

$250 for @bernhardoj - C
$250 for @eVoloshchak - C+

@melvin-bot melvin-bot bot removed the Overdue label Jul 29, 2024
@muttmuure
Copy link
Contributor

Please request in NewDot

@JmillsExpensify
Copy link

$250 approved for @bernhardoj

@eVoloshchak
Copy link
Contributor

Regression Test Proposal

  1. Go to staging.new.expensify.com
  2. Go to any chat
  3. Go to + > Pay someone
  4. Enter an amount and pay them
  5. Go to transaction thread
  6. Right click on paid system message > Reply in thread
  7. Send a reply
  8. Verify that system message thread is shown in LHN

@JmillsExpensify
Copy link

$250 approved for @eVoloshchak. Re-opening for regression test.

@melvin-bot melvin-bot bot added the Overdue label Aug 7, 2024
@muttmuure
Copy link
Contributor

Will handle in the morning

Copy link

melvin-bot bot commented Aug 13, 2024

@muttmuure, @bernhardoj Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Aug 15, 2024

@muttmuure, @bernhardoj Still overdue 6 days?! Let's take care of this!

@muttmuure
Copy link
Contributor

Regression test created

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
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 Daily KSv2 Engineering
Projects
None yet
Development

No branches or pull requests

8 participants