[$250] LHN - RBR stays for paid report

izarutskaya commented 2 months 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: v9.0.27-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: https://expensify.testrail.io/index.php?/cases/view/3266257 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Precondition: control ws exist with enabled categories, Tags and Taxes. Enable toggle 'Members must categorize/tag all expenses'.

Employee: submit an expense with violation (without category and tag) in ws chat.
Admin: approve or pay the report with violation.

Expected Result:

Employee: report doesn't have a RBR in the LHN and isn't pinned.

Actual Result:

Employee: report still have a RBR in the LHN and isn't pinned.

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

https://github.com/user-attachments/assets/4ed69491-f663-4e83-b488-26731680c9d5

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021832065922256660213
Upwork Job ID: 1832065922256660213
Last Price Increase: 2024-09-27

Issue Owner

Current Issue Owner: @mkhutornyi

melvin-bot[bot] commented 2 months ago

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

izarutskaya commented 2 months ago

We think this issue might be related to the #wave-control

FitseTLT commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-09-02 20:14:01 UTC.

Proposal

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

LHN - RBR stays for paid report

What is the root cause of that problem?

In all MoneyRequestPreview, MoneyReportView and LHN RBR we are showing RBR whenever there is a report or transaction violations without checking if the report is settled one https://github.com/Expensify/App/blob/e60b8210a1f5a2471d8aa9d84d0e461774b791d6/src/components/ReportActionItem/ReportPreview.tsx#L168-L170 https://github.com/Expensify/App/blob/e60b8210a1f5a2471d8aa9d84d0e461774b791d6/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L112 https://github.com/Expensify/App/blob/e60b8210a1f5a2471d8aa9d84d0e461774b791d6/src/components/LHNOptionsList/OptionRowLHNData.tsx#L40

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

We should only display RBR when violations exist if the iouReport is not settled (or not approved if we choose to do so). Therefore, in all the cases listed above we need to add a condition of !ReportUtils.isSettled( for both transaction and report violations. We can centralize the code by creating new functions to add these condition or we can update all TransactionUtils.hasViolation, ReportUtils.hasReportViolations and ReportUtils.hasViolations to check for !isSettled of the linked iou report.

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

@kevinksullivan, @mkhutornyi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

trjExpensify commented 2 months ago

Hm, I'm curious for where this regressed as I'm sure it has been discussed before. CC: @JmillsExpensify @cead22

Moving to #wave-control project anyway, as a violations bug.

mkhutornyi commented 2 months ago

@FitseTLT thanks for the proposal. Are you able to find offending PR as I am pretty sure this used to work before.

FitseTLT commented 2 months ago

@FitseTLT thanks for the proposal. Are you able to find offending PR as I am pretty sure this used to work before.

I didn't get in FE but of course in some cases when you pay a report the backend removes the violation in which case the RBR disappears so my thought was to implement a shield in FE code (in case the BE doesn't remove) same like we do in shouldDisplayTransactionThreadViolations here https://github.com/Expensify/App/blob/fc33ac1ac35248ecf888bb8d4e8e04b5e72cc56e/src/libs/ReportUtils.ts#L5897-L5899

kevinksullivan commented 1 month ago

Looping in another BZ member as I am going OOO

melvin-bot[bot] commented 1 month ago

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

mkhutornyi commented 1 month ago

We fixed similar issues in the past: https://github.com/Expensify/App/issues/34548 https://github.com/Expensify/App/issues/45230 They were in transaction threads.

This time, the issue is happening on single-transaction expense report

mkhutornyi commented 1 month ago

@FitseTLT can you please share test branch?

melvin-bot[bot] commented 1 month ago

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

FitseTLT commented 1 month ago

Here is a test branch @mkhutornyi

mkhutornyi commented 1 month ago

@FitseTLT's proposal looks good to me. 🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

cead22 commented 1 month ago

@FitseTLT from what I understand, the functions you mentioned in the proposal have never had code to check if a report is settled, so that check has to exist somewhere ele, and I think it works for some cases. Can you share where that check is, and why it works for some cases and not others?

FitseTLT commented 1 month ago

Can you share where that check is, and why it works for some cases and not others?

@cead22 the only FE check we are including is for transaction thread violations here https://github.com/Expensify/App/blob/fc33ac1ac35248ecf888bb8d4e8e04b5e72cc56e/src/libs/ReportUtils.ts#L5897-L5899 But the inconsistency for other cases comes from the BE: sometimes the BE clears all the violations (transactionViolations and reportViolations) on payment and sometimes it doesn't.

cead22 commented 1 month ago

If the backend doesn't clear the violations, and they show when the report is settled, then we're going through a flow that doesn't go through the code you linked. What flow is that?

Can you share the flows for which the backend isn't updating violations correctly?

melvin-bot[bot] commented 1 month ago

@marcochavezf @OfstadC @kevinksullivan @mkhutornyi this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

FitseTLT commented 1 month ago

If the backend doesn't clear the violations, and they show when the report is settled, then we're going through a flow that doesn't go through the code you linked. What flow is that?

Can you share the flows for which the backend isn't updating violations correctly?

Ok @cead22 the flow is here in report preview https://github.com/Expensify/App/blob/e60b8210a1f5a2471d8aa9d84d0e461774b791d6/src/components/ReportActionItem/ReportPreview.tsx#L168-L170 and here in money request preview https://github.com/Expensify/App/blob/e60b8210a1f5a2471d8aa9d84d0e461774b791d6/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L112 Here is a demo vid: after paying the RBR remains and when clearing cache the RBR will disappear from report preview but after opening the transactions the RBR reappears because the transactionViolations are fetched by then.

https://github.com/user-attachments/assets/c27fcca0-1295-4d87-b7cf-1f10c6afd88f

melvin-bot[bot] commented 1 month ago

@marcochavezf, @OfstadC, @kevinksullivan, @mkhutornyi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

cead22 commented 1 month ago

From @nkdengineer in https://github.com/Expensify/App/issues/48397#issuecomment-2362825011

@cead22 I think we should clear the hold violation in optimistic data if we paid full instead of adding !isSettled condition.

melvin-bot[bot] commented 1 month ago

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

cead22 commented 1 month ago

@FitseTLT thanks for that

The red circle on showing on the report preview is working as intended, because the report has violations and we show that no matter what the state of the report is.
The green circle on the LHN showing for that report might be working as intended, assuming we decided the green circle that is shown on reports with pending actions -- in this case paying, should take precedence over the red circle in the LHN that we show on reports with violations. Can you confirm @JmillsExpensify?
After paying, the red circle should remain in the report preview, for the same reason as in the first bullet
After clearing the cache, the report preview isn't showing the red circle, and after clicking on the preview, the transaction previews aren't showing the red circle. These are bugs because they should be showing for the same reason as in the first bullet

@JmillsExpensify did I get this right or has this changed from what I remember?

melvin-bot[bot] commented 1 month ago

@marcochavezf, @OfstadC, @kevinksullivan, @mkhutornyi Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 1 month ago

@marcochavezf, @OfstadC, @kevinksullivan, @mkhutornyi Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@marcochavezf @OfstadC @mkhutornyi this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 1 month ago

@marcochavezf, @OfstadC, @mkhutornyi Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 month ago

@marcochavezf, @OfstadC, @mkhutornyi Huh... This is 4 days overdue. Who can take care of this?

marcochavezf commented 1 month ago

From this comment, seems part of the behavior is expected. Closing it out for now, but we can re-open if it's reported again as an issue

Expensify / App