[$250] LHN - RBR stays for paid report

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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'.

1. Employee: submit an expense with violation (without category and tag) in ws chat.
2. 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
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6590320_1725270477874.RBR_violation.mp4

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]

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]

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

[FitseTLT]

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

App/src/components/ReportActionItem/ReportPreview.tsx

Lines 168 to 170 in e60b821

	ReportUtils.hasViolations(iouReportID, transactionViolations) ||
	ReportUtils.hasWarningTypeViolations(iouReportID, transactionViolations) ||
	(ReportUtils.isReportOwner(iouReport) && ReportUtils.hasReportViolations(iouReportID)) ||

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Line 112 in e60b821

const hasViolations = TransactionUtils.hasViolation(transaction?.transactionID ?? '-1', transactionViolations);

App/src/components/LHNOptionsList/OptionRowLHNData.tsx

Line 40 in e60b821

const shouldDisplayReportViolations = ReportUtils.isReportOwner(fullReport) && ReportUtils.hasReportViolations(reportID);

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]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[trjExpensify]

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]

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

[FitseTLT]

@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

App/src/libs/ReportUtils.ts

Lines 5897 to 5899 in fc33ac1

	if (isSettled(IOUReportID) || isReportApproved(IOUReportID?.toString())) {
	return false;
	}

[kevinksullivan]

Looping in another BZ member as I am going OOO

[melvin-bot]

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.

[FitseTLT]

Here is a test branch @mkhutornyi

[mkhutornyi]

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

[melvin-bot]

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

[cead22]

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

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

App/src/libs/ReportUtils.ts

Lines 5897 to 5899 in fc33ac1

	if (isSettled(IOUReportID) || isReportApproved(IOUReportID?.toString())) {
	return false;
	}

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]

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]

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

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

App/src/components/ReportActionItem/ReportPreview.tsx

Lines 168 to 170 in e60b821

	ReportUtils.hasViolations(iouReportID, transactionViolations) ||
	ReportUtils.hasWarningTypeViolations(iouReportID, transactionViolations) ||
	(ReportUtils.isReportOwner(iouReport) && ReportUtils.hasReportViolations(iouReportID)) ||

and here in money request preview

App/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

Line 112 in e60b821

const hasViolations = TransactionUtils.hasViolation(transaction?.transactionID ?? '-1', transactionViolations);

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.

2024-09-17.19-11-14.mp4

[melvin-bot]

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

[cead22]

From @nkdengineer in #48397 (comment)

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

[melvin-bot]

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

[cead22]

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

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

[melvin-bot]

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

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

[melvin-bot]

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

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

[melvin-bot]

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

[marcochavezf]

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