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

[$250] Hold expense - RBR is not shown instantly when holding expense from main chat #47240

Closed
6 tasks done
izarutskaya opened this issue Aug 12, 2024 · 20 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Aug 12, 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: v9.0.19-0
Reproducible in staging?: Y
Reproducible in production?: N
Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Open DM with user that has no unsettled expense.
  3. Submit an expense.
  4. In the mian chat, right click on the expense preview.
  5. Select Hold.
  6. Enter a reason and save it.
  7. Note that the expense preview in the main chat does not show RBR.
  8. Go to the transaction thread and return to main chat.
  9. Note that the expense preview in the main chat now shows RBR.
  10. Submit another expense.
  11. Go to expense report.
  12. Right click on the expense preview > Hold.
  13. Enter reason and save it.
  14. Note that the expense preview in the expense report displays RBR and hold label without the need to visit the transaction thread.

Expected Result:

In Step 7, the expense preview in the main chat will show RBR after holding the expense from the main chat.

Actual Result:

In Step 7, the expense preview in the main chat does not show RBR after holding the expense from the main chat.
It only shows RBR after visiting the report and back to main chat in Step 9.

When holding the expense from expense report (at least two expenses), the RBR and Hold label are shown without visiting the transaction thread (Step 14).

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

Bug6569930_1723470967140.20240812_215033.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0162d2bf8d74991b81
  • Upwork Job ID: 1823077860579616696
  • Last Price Increase: 2024-08-12
  • Automatic offers:
    • Nodebrute | Contributor | 103588043
Issue OwnerCurrent Issue Owner: @mananjadhav
@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

Triggered auto assignment to @johnmlee101 (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link

melvin-bot bot commented Aug 12, 2024

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.

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Aug 12, 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.

@izarutskaya
Copy link
Author

We think this issue might be related to the #collect project.

@izarutskaya
Copy link
Author

Production

Screenshot (629)

@dylanexpensify
Copy link
Contributor

@johnmlee101 can repro, but don't think this is necessarily a deploy blocker? LMK what you think!

@dylanexpensify dylanexpensify added the External Added to denote the issue can be worked on by a contributor label Aug 12, 2024
@melvin-bot melvin-bot bot changed the title Hold expense - RBR is not shown instantly when holding expense from main chat [$250] Hold expense - RBR is not shown instantly when holding expense from main chat Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

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

@johnmlee101
Copy link
Contributor

Yeah doesn't seem like a blocker necessarily

@johnmlee101 johnmlee101 added Weekly KSv2 Daily KSv2 and removed DeployBlocker Indicates it should block deploying the API DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 Weekly KSv2 labels Aug 12, 2024
@Nodebrute
Copy link
Contributor

Nodebrute commented Aug 12, 2024

Edited by proposal-police: This proposal was edited at 2024-08-12 21:51:43 UTC.

Proposal

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

RBR is not shown instantly when holding expense from main chat

What is the root cause of that problem?

In the expense preview, we check for transaction violations to see if any are present. However, when we put an expense on hold through the preview, we don’t set the transaction violation optimistically. As a result, the transaction violation will be empty unless we access the child report. This is why we can’t see the RBR without visiting the report.

ReportUtils.hasViolations(iouReportID, transactionViolations) ||

but in MoneyRequestPreviewContent we also have another check isOnHold that's why we can see the RBR without visiting the child report.
const shouldShowRBR = hasNoticeTypeViolations || hasViolations || hasFieldErrors || (!isFullySettled && !isFullyApproved && isOnHold) || hasDuplicates;

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

We can optimistically add the violation data when placing the expense on hold. We can do something like this here

const newViolation = { name: 'hold', type: CONST.VIOLATION_TYPES.VIOLATION };
const transactionViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`] ?? [];
const updatedViolations = [...transactionViolations, newViolation];
  {
            onyxMethod: Onyx.METHOD.MERGE,
            key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`,
            value:updatedViolations,
        },

What alternative solutions did you explore? (Optional)

Use same logic by adding isOnHold that we are using in

const shouldShowRBR = hasNoticeTypeViolations || hasViolations || hasFieldErrors || (!isFullySettled && !isFullyApproved && isOnHold) || hasDuplicates;

Copy link

melvin-bot bot commented Aug 16, 2024

@mananjadhav, @johnmlee101, @dylanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Aug 16, 2024
@mananjadhav
Copy link
Collaborator

@Nodebrute's proposal would work.

🎀 👀 🎀

@melvin-bot melvin-bot bot removed the Overdue label Aug 16, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

Current assignee @johnmlee101 is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Overdue and removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 19, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

📣 @Nodebrute 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@johnmlee101
Copy link
Contributor

@Nodebrute just making sure you've seen this!

@Nodebrute
Copy link
Contributor

@johnmlee101 I'll raise the pull request in a few hours.

@Nodebrute
Copy link
Contributor

This was deployed to production in v9.0.26-6.
Payment is due 2024-09-06.

@mananjadhav
Copy link
Collaborator

@dylanexpensify this is ready for payout. I couldn't pinpoint the exact PR where we should've added the RBR changes. I also don't think we need a regression test for this one.

@dylanexpensify
Copy link
Contributor

Payment summary:

Contributor: @Nodebrute $250 via Upwork
Contributor+: @mananjadhav $250 via NewDot

Please apply/request!

@garrettmknight
Copy link
Contributor

$250 approved for @mananjadhav

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants