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

[PAID] [LOW] [Splits] [$500] IOU - The user's name is displayed twice on the LHN after receiving payment #34525

Closed
6 tasks done
lanitochka17 opened this issue Jan 15, 2024 · 46 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 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 15, 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: 1.4.25-1
Reproducible in staging?: Y
Reproducible in production?: Y
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: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Log in with User A and open User B's chat
  2. Go to request money and enter the amount and completing the flow
  3. Click on created IOU
  4. Log in with User B from another browser and open User A's chat.
  5. Click on IOU and then click on "Pay elsewhere"
  6. After paying send a message in IOU and delete it
  7. Go back to User A's account
  8. Notice in LHN the user's name is displayed twice after deleting the last message

Expected Result:

The user's name should be displayed once in the LHN

Actual Result:

The user's name appears twice on the LHN after receiving payment

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

Bug6343327_1705343327092.2024-01-15_22-07-45.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a34a9fdde701015a
  • Upwork Job ID: 1746968111315644416
  • Last Price Increase: 2024-02-07
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 15, 2024
Copy link

melvin-bot bot commented Jan 15, 2024

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

Copy link

melvin-bot bot commented Jan 15, 2024

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

@melvin-bot melvin-bot bot changed the title IOU - The user's name is displayed twice on the LHN after receiving payment [$500] IOU - The user's name is displayed twice on the LHN after receiving payment Jan 15, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 15, 2024
Copy link

melvin-bot bot commented Jan 15, 2024

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

Copy link

melvin-bot bot commented Jan 15, 2024

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Jan 15, 2024

Proposal

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

The user's name is displayed twice on the LHN after receiving payment

What is the root cause of that problem?

When we delete a comment it is not deleted from the report action list of the report but only the message.0.html is reset to empty therefore so the last report action here will be wrongly set to this deleted message which hasn't been removed from the list

const reportAction = lastReportActions?.[report.reportID];
if (result.isArchivedRoom) {

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

We need to filter deleted comment report actions here so that we will not wrongly set a deleted comment report action as the last report action of the report

lastReportActions[reportID] = actionsArray[actionsArray.length - 1];

        lastReportActions[reportID] = actionsArray.filter((action) => !(action?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && action.message?.[0].html === ''))[

or if we don't want to change the logic of lastReportAction we can update how isThreadMessage is calculated here

const isThreadMessage =
ReportUtils.isThread(report) && reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && reportAction?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;

const isThreadMessage =
        ReportUtils.isThread(report) && reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && reportAction?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE && reportAction?.message?.[0].html;

We can also use this logic to filter out other type of deleted report actions too

action.message?.[0].type === 'COMMENT' && action.message?.[0].html === ''

What alternative solutions did you explore? (Optional)

We can update ReportActionsUtils.getSortedReportActions and other functions to not consider deleted comment actions in their logic to prevent other bugs.

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jan 16, 2024

Proposal

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

The user's name appears twice on the LHN after receiving payment

What is the root cause of that problem?

reportAction is the last action from the list reportActions (it contains deleted action) of the report

const reportAction = lastReportActions?.[report.reportID];

After we add a message and delete it, reportAction is the add comment action that is deleted. That makes isThreadMessage here is true

const isThreadMessage =
ReportUtils.isThread(report) && reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && reportAction?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;

Then the last message from LHN is dupe the user's name in this case because lastMessageText already contain the user's name.

} else if (lastAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.REPORTPREVIEW && lastActorDisplayName && lastMessageTextFromReport) {
result.alternateText = `${lastActorDisplayName}: ${lastMessageText}`;

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

Since we are using lastAction from the visible action list to return the last message text in LHN for this case

if ((result.isChatRoom || result.isPolicyExpenseChat || result.isThread || result.isTaskReport || isThreadMessage) && !result.isArchivedRoom) {
const lastAction = visibleReportActionItems[report.reportID];

we should also use this to isThreadMessage. To do that we can bring the declaration of lastAction to the outside and use this instead of reportAction.

const isThreadMessage =
ReportUtils.isThread(report) && reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && reportAction?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;

*Note: We shouldn't change reportAction to the last visible action because we use it here to get the archived reason.

const archiveReason = (reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.CLOSED && reportAction?.originalMessage?.reason) || CONST.REPORT.ARCHIVE_REASON.DEFAULT;

What alternative solutions did you explore? (Optional)

const isThreadMessage =
ReportUtils.isThread(report) && reportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.ADDCOMMENT && reportAction?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE;

We can replace this check reportAction?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE
with ReportActionUtils.isDeletedParentAction(reportAction) which will return true if the action is deleted and it has child visible action

@melvin-bot melvin-bot bot added the Overdue label Jan 18, 2024
@strepanier03
Copy link
Contributor

Working on repro'ing and tying to a wave/VIP. Will update when able.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 18, 2024
Copy link

melvin-bot bot commented Jan 22, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@strepanier03
Copy link
Contributor

User A: sheena@expensifail.com
User B: employee.sheena@expensicorp.com

I've tested this a few different times now and cannot recreate the behavior.

Anyone else able to repro this still?

@melvin-bot melvin-bot bot removed the Overdue label Jan 26, 2024
@strepanier03 strepanier03 added the Needs Reproduction Reproducible steps needed label Jan 26, 2024
@strepanier03
Copy link
Contributor

I'm going to close this out for now and if we can repro this we can reopen it.

@FitseTLT
Copy link
Contributor

Yes I did repoduce it on latest main now @strepanier03

2024-01-26.23-54-35.mp4

@FitseTLT
Copy link
Contributor

@strepanier03 This is still reproducible, only need to take a close note to the steps. ^

@strepanier03
Copy link
Contributor

@FitseTLT - Thanks for confirming, I did follow the repro steps closely and still cannot repro this so I'll ask another team member to test as well.

@strepanier03 strepanier03 reopened this Jan 30, 2024
Copy link

melvin-bot bot commented Jan 31, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@FitseTLT
Copy link
Contributor

FitseTLT commented Feb 1, 2024

@parasharrajat Can U please check and confirm that this reproducible in latest main? 🙏

@parasharrajat
Copy link
Member

Yes, I can reproduce this. Going to review the proposals in 2 hours. Currently away for some work.

Copy link

melvin-bot bot commented Feb 26, 2024

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

@dukenv0307
Copy link
Contributor

@situchan The PR is ready for review.

@arielgreen arielgreen changed the title [VIP-Split] [$500] IOU - The user's name is displayed twice on the LHN after receiving payment [LOW] [Splits] [$500] IOU - The user's name is displayed twice on the LHN after receiving payment Mar 4, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 6, 2024
@melvin-bot melvin-bot bot changed the title [LOW] [Splits] [$500] IOU - The user's name is displayed twice on the LHN after receiving payment [HOLD for payment 2024-03-13] [LOW] [Splits] [$500] IOU - The user's name is displayed twice on the LHN after receiving payment Mar 6, 2024
Copy link

melvin-bot bot commented Mar 6, 2024

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 6, 2024
Copy link

melvin-bot bot commented Mar 6, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.47-10 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-03-13. 🎊

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

  • @situchan requires payment (Needs manual offer from BZ)
  • @dukenv0307 requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Mar 6, 2024

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:

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 13, 2024
Copy link

melvin-bot bot commented Mar 13, 2024

Payment Summary

Upwork Job

BugZero Checklist (@strepanier03)

  • 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/1746968111315644416/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@zboutchyard
Copy link

does this issue still exist? Looks like there was a PR merged, but upwork just posted a job for this a few minutes ago.

Copy link

melvin-bot bot commented Mar 13, 2024

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

@strepanier03
Copy link
Contributor

@situchan and @dukenv0307 - I have hired you both to the job in Upwork, please accept and I'll pay out today or more likely tomorrow morning.

@situchan - Please complete the checklist and I'll finish up your payment once I have that and can do the reg test (if we need one).

@dukenv0307
Copy link
Contributor

@situchan and @dukenv0307 - I have hired you both to the job in Upwork, please accept and I'll pay out today or more likely tomorrow morning.

@strepanier03 I've accepted, thank you!

@strepanier03
Copy link
Contributor

@dukenv0307 - Paid out!

I'll keep an eye open for the checklist @situchan.

@situchan
Copy link
Contributor

Not able to find the exact offending PR as there were several refactors on LHN last message logic.
We can skip regression test as this was caught by QA team.

@strepanier03
Copy link
Contributor

Thanks @situchan, I'll update the checklist and finish this up.

@strepanier03 strepanier03 changed the title [HOLD for payment 2024-03-13] [LOW] [Splits] [$500] IOU - The user's name is displayed twice on the LHN after receiving payment [PAID] [LOW] [Splits] [$500] IOU - The user's name is displayed twice on the LHN after receiving payment Mar 15, 2024
@strepanier03
Copy link
Contributor

All set, contract closed. Thanks everyone!

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 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

9 participants