Fix - Don't show RBR on the LHN for transaction threads with violations if the report has already been reimbursed/settled #37767

FitseTLT · 2024-03-05T15:33:51Z

Details

Fixed Issues

$ #34548
PROPOSAL: #34548 (comment)

Tests

Precondition:
Make sure you're on the violations beta
On the collect workspace set category as required

Request money without adding a category from the workspace (it should have a missing category violation)
Submit the request if you need to
pay (reimburse) the request as a workspace admin so that the request is set as settled/payed
Open the transaction thread as the requester. You may need to navigate to another chat, and then back to the transaction thread
Verify that RBR (Red Dot) no more shows on LHN for the transaction thread report

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android.mp4

Android: mWeb Chrome

android.web.mp4

iOS: Native

ios.mp4

iOS: mWeb Safari

ios.web.mp4

MacOS: Chrome / Safari

2024-03-20.23-36-57.mp4

MacOS: Desktop

2024-03-20.23-40-52.mp4

melvin-bot · 2024-03-05T15:33:57Z

@situchan Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

src/libs/SidebarUtils.ts

cead22 · 2024-03-05T22:45:41Z

Code looks good. I'll wait for @situchan to review before approving

quinthar · 2024-03-08T02:02:44Z

Hi, what's the ETA on this? Eager to get it done!

situchan · 2024-03-08T09:24:25Z

completing today

situchan · 2024-03-08T13:23:19Z

src/libs/SidebarUtils.ts

+    let shouldShowViolations = false;
+    if (hasViolations && parentReportAction?.actionName === CONST.REPORT.ACTIONS.TYPE.IOU) {
+        const {IOUReportID} = parentReportAction?.originalMessage ?? {};
+        shouldShowViolations = !ReportUtils.isSettled(IOUReportID);


I am still seeing RBR after approve request

@cead22 should we show RBR when approved but not reimbursed?

ReportUtils has isReportApproved (statusNum = APPROVED) and isSettled (statusNum = REIMBURSED) methods.

Yes

On the LHN only to the user that made the request

On the money request view, to all users that have access

On the LHN only to the user that made the request

🟢

On the money request view, to all users that have access

I am not seeing violation on payer side

I am not seeing violation on payer side

That's probably because RequestMoney doesn't return violations, and doesn't push them to the relevant users either. There's another issue that OpenReport only returns violations to the user that made the money request.

I submitted a backend fix for the second issue, and I'm working on a backend solution for the first as well

situchan · 2024-03-08T17:40:34Z

@FitseTLT please update Tests / QA step as per #37767 (comment).
Currently it's wrong.
We should still show RBR for approved requests (but not reimbursed).
Also, please update videos accordingly.

FitseTLT · 2024-03-08T18:11:24Z

@FitseTLT please update Tests / QA step as per #37767 (comment). Currently it's wrong. We should still show RBR for approved requests (but not reimbursed). Also, please update videos accordingly.

By the way, I know and the reason I said approved or payed is to cover the condition of auto-reimbursement but I have now updated it to avoid confusion. But the snapshots are correct.

situchan · 2024-03-08T18:50:16Z

@FitseTLT though RBR doesn't show on LHN, it looks as if pinned. It should be below pinned items

situchan · 2024-03-11T14:08:58Z

@FitseTLT bump on above.
I suggest you to give priority to pending PRs over submitting proposals on new issues.

FitseTLT · 2024-03-11T16:04:39Z

Fixed 👍

FitseTLT · 2024-03-13T14:48:51Z

@situchan can u please give it a priority as it is considered a highly requested issue

situchan · 2024-03-13T14:55:37Z

@FitseTLT can you please update screen recording after applying that fix?

@FitseTLT though RBR doesn't show on LHN, it looks as if pinned. It should be below pinned items

For this bug, one platform re-testing is fine

quinthar · 2024-03-14T01:07:39Z

Yes, can't wait for this one.

cead22 · 2024-03-14T03:12:16Z

src/libs/ReportUtils.ts

@@ -4048,7 +4048,8 @@ function doesTransactionThreadHaveViolations(report: OnyxEntry<Report>, transact
    if (report?.stateNum !== CONST.REPORT.STATE_NUM.OPEN && report?.stateNum !== CONST.REPORT.STATE_NUM.SUBMITTED) {
        return false;
    }
-    return TransactionUtils.hasViolation(IOUTransactionID, transactionViolations);
+
+    return TransactionUtils.hasViolation(IOUTransactionID, transactionViolations) && !isSettled(IOUReportID);


Can we move this call to isSettled outside of this function like in the proposal? I don't think it belongs in this function

FitseTLT · 2024-03-14T21:31:34Z

revert pr #38081 has made the last change unnecessary. I will revert it.

FitseTLT · 2024-03-15T13:28:06Z

@cead22 I have a question on

Can we move this call to isSettled outside of this function like in the proposal? I don't think it belongs in this function

Except the one we are dealing with here (showing RBR of LHN) there are two instances we use doesTransactionThreadHaveViolations

App/src/libs/OptionsListUtils.ts

Lines 1449 to 1450 in 0e9bd7f

    
           const doesReportHaveViolations = 
        
               (betas?.includes(CONST.BETAS.VIOLATIONS) && ReportUtils.doesTransactionThreadHaveViolations(report, transactionViolations, parentReportAction)) ?? false;

App/src/libs/SidebarUtils.ts

Lines 83 to 84 in 0e9bd7f

    
           const doesReportHaveViolations = 
        
               betas.includes(CONST.BETAS.VIOLATIONS) && !!parentReportAction && ReportUtils.doesTransactionThreadHaveViolations(report, transactionViolations, parentReportAction);

and in both case we pass it to shouldReportBeInOptionList but in shouldReportBeInOptionList violations will only affect the result when the transaction(report) is not settled we alread have isSettled Check here

App/src/libs/ReportUtils.ts

Lines 4225 to 4228 in e9c112a

    
           const reportIsSettled = report.statusNum === CONST.REPORT.STATUS_NUM.REIMBURSED; 
        
           // Always show IOU reports with violations unless they are reimbursed 
        
           if (isExpenseRequest(report) && doesReportHaveViolations && !reportIsSettled) {

And in fact the appearance of settled report along with the pinned ones bug (the one @situchan ) reported was caused by the lack of isSettled check on here (but now it is reverted)

App/src/libs/SidebarUtils.ts

Lines 95 to 97 in 3f7a361

    
           if (doesReportHaveViolations) { 
        
               reportIDsWithViolations.add(report.reportID); 
        
           }

Questions:

So I fear that not centralizing the isSettled check here will cause problems than putting the check inside it. may be let's update the name of the function by adding sth like should display or sth.
The isSettled check we have here

App/src/libs/ReportUtils.ts

Lines 4225 to 4226 in e9c112a

const reportIsSettled = report.statusNum === CONST.REPORT.STATUS_NUM.REIMBURSED;

check if it is reimbursed but our isSettled function also assumes it is settled if it is approved and we are waiting for the employee bank account to be set. So which one is the path we should follow (I prefer the ReportUtils.isSettled one)

App/src/libs/ReportUtils.ts

Lines 749 to 752 in 0e9bd7f

    
           // In case the payment is scheduled and we are waiting for the payee to set up their wallet, 
        
           // consider the report as paid as well. 
        
           if (report.isWaitingOnBankAccount && report.statusNum === CONST.REPORT.STATUS_NUM.APPROVED) { 
        
               return true;

cead22 · 2024-03-15T17:48:02Z

Discussing this with @JmillsExpensify, I think we want to do something slightly different, and I'll update the issue to reflect this. Let me make sure I got the requirements right and I'll update here

cead22 · 2024-03-19T16:01:17Z

@FitseTLT sorry for the delay

So I fear that not centralizing the isSettled check here will cause problems than putting the check inside it. may be let's update the name of the function by adding sth like should display or sth.

I'm good with adding a function like you suggest shouldDisplayViolationsRBRInLHN = (...) => ReportUtils.doesTransactionThreadHaveViolations(...) && ! isSettled(...)

2. check if it is reimbursed but our isSettled function also assumes it is settled if it is approved and we are waiting for the employee bank account to be set. So which one is the path we should follow (I prefer the ReportUtils.isSettled one)

ReportUtils.isSettled sounds good 👍

FitseTLT · 2024-03-20T20:42:58Z

@situchan ready for your review!

src/components/LHNOptionsList/OptionRowLHNData.tsx

src/libs/ReportUtils.ts

situchan · 2024-03-21T01:36:52Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

android.mov

Android: mWeb Chrome

mchrome.mov

iOS: Native

ios.mov

iOS: mWeb Safari

msafari.mov

MacOS: Chrome / Safari

MacOS: Desktop

situchan

Let's just address Carlos's feedback

FitseTLT · 2024-03-21T09:11:11Z

@cead22 All comments addressed and changes test well 👍

melvin-bot · 2024-03-21T16:38:29Z

@cead22 looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

cead22 · 2024-03-21T16:39:12Z

Tests pass, but performance tests are running, but they take a long time and will alert if it fails. This wasn't an emergency change

OSBotify · 2024-03-21T16:44:21Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-03-22T16:20:00Z

🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.56-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

sakluger · 2024-03-22T18:13:36Z

Hey guys, unfortunately, this doesn't appear to have fixed the issue for me. I signed out and back into staging.new.expensify.com, and when everything finished loading, all the individual report chats were still in my LHN, without the RBR dot now. When I clicked on any of them, the RBR dot returned.

Here's what it looks like after I have clicked on two of the report chats:

situchan · 2024-03-22T18:17:00Z

Hey guys, unfortunately, this doesn't appear to have fixed the issue for me. I signed out and back into staging.new.expensify.com, and when everything finished loading, all the individual report chats were still in my LHN, without the RBR dot now. When I clicked on any of them, the RBR dot returned.

Here's what it looks like after I have clicked on two of the report chats:

Do you mind sharing full video of that request (expense report, transaction detail)?

cead22 · 2024-03-26T03:13:27Z

Can you also share the transaction thread reportIDs (if you're looking at the money request page, where you can edit category/tag/etc, it's the ID in the URL)

FitseTLT · 2024-03-26T07:47:21Z

@cead22 From the explanation #37767 (comment) It looks like the issue is that the iouReport haven't been loaded to onyx but unfortunately for now when iou Report is approved/settled we are not setting the transaction thread reports statusNum so to determine whether it is settled we need to depend on the status of the iou report which is another report so whenever it is not in onyx this cases can occur.

cead22 · 2024-03-26T15:29:51Z

Thanks @FitseTLT, let me know if you find a way to reproduce, since I haven't been able to

I disabled a category that's set on requests that are settled
Signed out and back in to try to get the transaction threads to load without their parent report
I tried focus and most recent mode

cead22 · 2024-03-26T15:43:06Z

cead22 · 2024-03-26T17:02:22Z

Here are some log requestIDs Sasha pulled from the Network tab from when I clicked on one of the report chats:

OpenReport: 86a8a5f2191ffa8a-SJC
Ping: 86a8a5f42b1afa8a-SJC
AuthenticatePusher: 86a8a5f43b2efa8a-SJC
OpenReport: 86a8a5f54c40fa8a-SJC
SendPerformanceTiming: 86a8a5f65d84fa8a-SJC
Log: 86a8a5fbbab5fa8a-SJC

cead22 · 2024-03-26T17:16:22Z

I tried disabling tags as well, and still couldn't repro

OSBotify · 2024-03-27T04:49:09Z

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.56-8 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

situchan · 2024-08-12T18:12:47Z

src/libs/ReportUtils.ts

+    parentReportAction: OnyxEntry<ReportAction>,
+): boolean {
+    const {IOUReportID} = (parentReportAction?.originalMessage as IOUMessage) ?? {};
+    if (isSettled(IOUReportID)) {


Later, we added more condition (approved reports, not only reimbursed/settled) to hide RBR on the LHN for transaction threads.
#45230

hide RBR for settled requests

20a754b

FitseTLT requested a review from a team as a code owner March 5, 2024 15:33

melvin-bot bot requested review from situchan and removed request for a team March 5, 2024 15:33

cead22 requested changes Mar 5, 2024

View reviewed changes

src/libs/SidebarUtils.ts Outdated Show resolved Hide resolved

src/libs/SidebarUtils.ts Outdated Show resolved Hide resolved

updated based on comments

ca74d99

FitseTLT requested a review from cead22 March 5, 2024 16:41

situchan reviewed Mar 8, 2024

View reviewed changes

FitseTLT added 2 commits March 11, 2024 17:45

Merge branch 'main' into fix-showing-RBR-for-settled-transaction

9a0386b

took the condition into doesTransactionThreadHaveViolations

3f7a361

FitseTLT requested a review from situchan March 11, 2024 16:04

cead22 requested changes Mar 14, 2024

View reviewed changes

Merge branch 'main' into fix-showing-RBR-for-settled-transaction

0e9bd7f

Merge branch 'main' into fix-showing-RBR-for-settled-transaction

dc222a1

cead22 requested changes Mar 20, 2024

View reviewed changes

src/components/LHNOptionsList/OptionRowLHNData.tsx Outdated Show resolved Hide resolved

src/libs/ReportUtils.ts Outdated Show resolved Hide resolved

src/libs/ReportUtils.ts Outdated Show resolved Hide resolved

src/libs/ReportUtils.ts Outdated Show resolved Hide resolved

situchan approved these changes Mar 21, 2024

View reviewed changes

melvin-bot bot requested a review from cead22 March 21, 2024 01:37

fix based on comments

384d183

Merge branch 'main' into fix-showing-RBR-for-settled-transaction

f9f8bb2

cead22 approved these changes Mar 21, 2024

View reviewed changes

cead22 merged commit 715714c into Expensify:main Mar 21, 2024
14 of 15 checks passed

melvin-bot bot added the Emergency label Mar 21, 2024

cead22 removed the Emergency label Mar 21, 2024

github-actions bot mentioned this pull request Mar 22, 2024

Deploy Checklist: New Expensify 2024-03-22 #38811

Closed

sakluger mentioned this pull request Mar 22, 2024

[ON HOLD Payment 4/3/24] [Violations] [$500] Don't show RBR on the LHN for transaction threads with violations if the report has already been reimbursed/settled #34548

Closed

6 tasks

marcaaron mentioned this pull request Apr 2, 2024

HIGH: [Reliability] Transaction threads with no RBR are persisting in the LHN on sign in #39468

Closed

situchan reviewed Aug 12, 2024

View reviewed changes

situchan mentioned this pull request Aug 12, 2024

[HOLD for payment 2024-08-09] [$250] Approved expenses are showing pinned expenses with RBR #45230

Closed

6 tasks

Fix - Don't show RBR on the LHN for transaction threads with violations if the report has already been reimbursed/settled #37767

Fix - Don't show RBR on the LHN for transaction threads with violations if the report has already been reimbursed/settled #37767

Conversation

FitseTLT commented Mar 5, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Mar 5, 2024

cead22 commented Mar 5, 2024

quinthar commented Mar 8, 2024 • edited Loading

situchan commented Mar 8, 2024

situchan Mar 8, 2024

Choose a reason for hiding this comment

situchan Mar 8, 2024

Choose a reason for hiding this comment

cead22 Mar 8, 2024

Choose a reason for hiding this comment

situchan Mar 8, 2024

Choose a reason for hiding this comment

cead22 Mar 9, 2024

Choose a reason for hiding this comment

situchan commented Mar 8, 2024

FitseTLT commented Mar 8, 2024

situchan commented Mar 8, 2024

situchan commented Mar 11, 2024

FitseTLT commented Mar 11, 2024

FitseTLT commented Mar 13, 2024

situchan commented Mar 13, 2024

quinthar commented Mar 14, 2024

cead22 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

FitseTLT commented Mar 14, 2024

FitseTLT commented Mar 15, 2024

cead22 commented Mar 15, 2024

cead22 commented Mar 19, 2024

FitseTLT commented Mar 20, 2024

situchan commented Mar 21, 2024

Reviewer Checklist

Screenshots/Videos

situchan left a comment

Choose a reason for hiding this comment

FitseTLT commented Mar 21, 2024

melvin-bot bot commented Mar 21, 2024

cead22 commented Mar 21, 2024

OSBotify commented Mar 21, 2024

OSBotify commented Mar 22, 2024

sakluger commented Mar 22, 2024

situchan commented Mar 22, 2024

cead22 commented Mar 26, 2024

FitseTLT commented Mar 26, 2024

cead22 commented Mar 26, 2024

cead22 commented Mar 26, 2024

cead22 commented Mar 26, 2024

cead22 commented Mar 26, 2024

OSBotify commented Mar 27, 2024

situchan Aug 12, 2024

Choose a reason for hiding this comment

FitseTLT commented Mar 5, 2024 •

edited

Loading

quinthar commented Mar 8, 2024 •

edited

Loading

cead22 Mar 14, 2024 •

edited

Loading