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-12-11] [$250] Held expense - Empty message with reply appears after paying the unheld amount for second time #52696

Closed
5 of 8 tasks
IuliiaHerets opened this issue Nov 18, 2024 · 21 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

@IuliiaHerets
Copy link

IuliiaHerets commented Nov 18, 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.63-3
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+kh051115@applause.expensifail.com
Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. [User A] Submit two expenses to User B.
  3. [User B] Hold the first expense.
  4. [User B] Click Pay button and pay the unheld amount.
  5. [User A] Submit another expense to User B.
  6. [User B] Go to the expense report.
  7. [User B] Click Pay button and pay the unheld amount.
  8. [User B] Note that the held expense is marked as paid when it should be moved to another report.
  9. [User B] Refresh the expense report.
  10. [User B] Note that there is an empty message with reply which opens the held expense when clicked.

Expected Result:

In Step 8, the held expense should move to another report when only the pending amount is paid.
In Step 10, after refreshing the page, there should not be an empty message in the expense report.

Actual Result:

In Step 8, the held expense does not move to another report when only the pending amount is paid. The expense preview is marked as paid when only the unheld amount is paid.

In Step 10, after refreshing the page, there is an empty message in the expense report, which opens the held expense.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6668237_1731920941849.bandicam_2024-11-18_17-00-07-546.mp4

Bug6668237_1731920941852!bandicam_2024-11-18_17-01-57-378

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021859730399211188082
  • Upwork Job ID: 1859730399211188082
  • Last Price Increase: 2024-11-21
  • Automatic offers:
    • DylanDylann | Reviewer | 105075616
Issue OwnerCurrent Issue Owner: @johncschuster
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 18, 2024
Copy link

melvin-bot bot commented Nov 18, 2024

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

@bernhardoj
Copy link
Contributor

Proposal

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

The hold report action isn't removed from the paid expense chat and becomes empty when revisiting the report.

What is the root cause of that problem?

When we pay the expense report with held expense, it will optimistically remove the held expense and move it to the new optimistic expense report.

App/src/libs/actions/IOU.ts

Lines 6309 to 6331 in f77087f

holdReportActions.forEach((holdReportAction) => {
const originalMessage = ReportActionsUtils.getOriginalMessage(holdReportAction);
deleteHoldReportActions[holdReportAction.reportActionID] = {
message: [
{
deleted: DateUtils.getDBTime(),
type: CONST.REPORT.MESSAGE.TYPE.TEXT,
text: '',
},
],
};
const reportActionID = rand64();
addHoldReportActions[reportActionID] = {
...holdReportAction,
reportActionID,
originalMessage: {
...originalMessage,
IOUReportID: optimisticExpenseReport.reportID,
},
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
};

But the issue started to happen when we did it again the second time. It's because when we optimistically add the held expense to a new expense report, we set the pending action to add (as shown above). But we don't clear it when the request is successful. So, the pending action is always true and when we optimistically delete it, it will still be shown because a pending action is always shown.

const isDeleted = isDeletedAction(reportAction);
const isPending = !!reportAction.pendingAction;
return !isDeleted || isPending || isDeletedParentAction(reportAction) || isReversedTransaction(reportAction);

It becomes an empty message when we reopen it because the BE response changes the action to ADDCOMMENT instead of IOU.

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

We need to clear the pending action when the request is successful.

const successAddHoldReportActions = {};

holdReportActions.forEach((holdReportAction) => {

const reportActionID = rand64();
addHoldReportActions[reportActionID] = {
    ...
    pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
};

successAddHoldReportActions[reportActionID] = {
    pendingAction: null,
};

We might need to do it for the optimistic report preview too because it also adds a pending action when created optimistically.

App/src/libs/actions/IOU.ts

Lines 6295 to 6302 in f77087f

const optimisticExpenseReportPreview = ReportUtils.buildOptimisticReportPreview(
chatReport,
optimisticExpenseReport,
'',
firstHoldTransaction,
optimisticExpenseReport.reportID,
newParentReportActionID,
);

We can do the same with other optimistic data that I missed.

@melvin-bot melvin-bot bot added the Overdue label Nov 21, 2024
@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Nov 21, 2024
Copy link

melvin-bot bot commented Nov 21, 2024

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

@melvin-bot melvin-bot bot changed the title Held expense - Empty message with reply appears after paying the unheld amount for second time [$250] Held expense - Empty message with reply appears after paying the unheld amount for second time Nov 21, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 21, 2024
Copy link

melvin-bot bot commented Nov 21, 2024

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

Copy link

melvin-bot bot commented Nov 25, 2024

@johncschuster, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Nov 25, 2024
@johncschuster
Copy link
Contributor

@DylanDylann please review the proposal above!

@melvin-bot melvin-bot bot removed the Overdue label Nov 25, 2024
@DylanDylann
Copy link
Contributor

@bernhardoj's proposal looks good to me

Note for the PR phase: we need to find out all pendingAction fields and make sure it is reset in successData

🎀 👀 🎀 C+ Reviewed

Copy link

melvin-bot bot commented Nov 26, 2024

Triggered auto assignment to @Julesssss, 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 Nov 26, 2024
Copy link

melvin-bot bot commented Nov 26, 2024

📣 @DylanDylann 🎉 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
Copy link
Contributor

There is currently a bug where the pay button doesn't show if there are held expenses. It's reported on the issue that causes it here

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 26, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @DylanDylann

@garrettmknight garrettmknight moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Nov 26, 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 Dec 4, 2024
@melvin-bot melvin-bot bot changed the title [$250] Held expense - Empty message with reply appears after paying the unheld amount for second time [HOLD for payment 2024-12-11] [$250] Held expense - Empty message with reply appears after paying the unheld amount for second time Dec 4, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Dec 4, 2024
Copy link

melvin-bot bot commented Dec 4, 2024

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

Copy link

melvin-bot bot commented Dec 4, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.70-9 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-12-11. 🎊

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

Copy link

melvin-bot bot commented Dec 4, 2024

@DylanDylann @johncschuster @DylanDylann 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]

@johncschuster
Copy link
Contributor

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] 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:

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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:

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal Template
  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

Test:

Do we agree 👍 or 👎

@johncschuster
Copy link
Contributor

Payment Summary:

Contributor: @bernhardoj due $250 via NewDot
Contributor+: @DylanDylann paid $250 via Upwork

Upwork job here! Please apply

@bernhardoj
Copy link
Contributor

Requested in ND.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 10, 2024
@garrettmknight garrettmknight moved this from Bugs and Follow Up Issues to Hold for Payment in [#whatsnext] #expense Dec 10, 2024
@JmillsExpensify
Copy link

$250 approved for @bernhardoj

@johncschuster
Copy link
Contributor

Payment has been issued through Upwork. Thanks, everyone, for your contributions!

@github-project-automation github-project-automation bot moved this from Hold for Payment to Done in [#whatsnext] #expense Dec 11, 2024
@johncschuster johncschuster reopened this Dec 11, 2024
@johncschuster
Copy link
Contributor

Oh wait, I jumped the gun. @DylanDylann, can you please post the regression test steps if applicable?

@DylanDylann
Copy link
Contributor

DylanDylann commented Dec 11, 2024

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other:

Where bug was reported:

  • 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • 2b. Reported on staging (eg. found during regression or PR testing)
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] 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: Create optimistic report when approving/paying for report with hold expenses #42573 (comment)

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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:

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Test:

  1. Open a workspace chat
  2. Submit 2 money requests
  3. Open the expense report
  4. Hold one of the expenses
  5. Pay the report partially
  6. Verify the held expense is removed
  7. Go back to workspace chat
  8. Open the new expense report
  9. Submit another money request
  10. Pay the report partially
  11. Verify the held expense is removed

Do we agree 👍 or 👎

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: Done
Development

No branches or pull requests

6 participants