feat: [Auth Violations] Support isDismissed on newDot to hide violations. #54455

Krishna2323 · 2024-12-23T04:04:21Z

Explanation of Change

Fixed Issues

$ #53398
PROPOSAL: #53398 (comment)

Tests

Scan a receipt in which the smartscan will fail (illegible or not a receipt)
Go to old Expensify, find the report and dismiss the violation.
Go back to new Expensify and make sure the violation is not showing anymore.
Note: You might need to refresh the page, I am not sure if the BE is queueing these updates, let me know if not.
Create 2 manual expenses with same amount and merchant > Open the first one
Go to old Expensify, find the report and dismiss the duplicate violation.
Go back to new Expensify and make sure the duplicate violation is not showing anymore.

Verify that no errors appear in the JS console

Offline tests

Scan a receipt in which the smartscan will fail (illegible or not a receipt)
Go to old Expensify, find the report and dismiss the violation.
Go back to new Expensify and make sure the violation is not showing anymore.
Note: You might need to refresh the page, I am not sure if the BE is queueing these updates, let me know if not.
Create 2 manual expenses with same amount and merchant > Open the first one
Go to old Expensify, find the report and dismiss the duplicate violation.
Go back to new Expensify and make sure the duplicate violation is not showing anymore.

QA Steps

Scan a receipt in which the smartscan will fail (illegible or not a receipt)
Go to old Expensify, find the report and dismiss the violation.
Go back to new Expensify and make sure the violation is not showing anymore.
Note: You might need to refresh the page, I am not sure if the BE is queueing these updates, let me know if not.
Create 2 manual expenses with same amount and merchant > Open the first one
Go to old Expensify, find the report and dismiss the duplicate violation.
Go back to new Expensify and make sure the duplicate violation is not showing anymore.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android_native.mp4

Android: mWeb Chrome

android_chrome.mp4

iOS: Native

ios_native.mp4

iOS: mWeb Safari

ios_safari.mp4

MacOS: Chrome / Safari

web_chrome.mp4

MacOS: Desktop

desktop_app.mp4

…ons. Signed-off-by: krishna2323 <[email protected]>

Signed-off-by: krishna2323 <[email protected]>

melvin-bot · 2025-01-05T12:30:58Z

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

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 · 2025-01-05T13:21:53Z

Hmm, not sure why lint warnings increases every time I commit changes.

Krishna2323 · 2025-01-06T12:47:14Z

src/components/MoneyRequestHeader.tsx

                        report={report}
                        policy={policy}
                    />
                ),
            };
        }
-        if (TransactionUtils.hasPendingRTERViolation(TransactionUtils.getTransactionViolations(transaction?.transactionID ?? '-1', transactionViolations))) {
+        if (TransactionUtils.hasPendingRTERViolation(TransactionUtils.getTransactionViolations(transaction?.transactionID))) {


@rojiphil, could you please check the ESLint failures below? I have already resolved dozens of those but it keeps increasing everytime I push a commit. I want your help especially in resolving the warning just below this comment and when we use ?? while getting a route, I have no idea how to deal with that, please let me know if you are familiar with that. Thanks 🙏🏻

In the case of calling route, can we not do an early return as shown below? It does not seem correct to call navigate with an invalid report id anyway.

The guidelines for the fix of the ESlint failures is already mentioned here. Please look for similar handling for ESlint failures elsewhere in code as that can help. But let me know if you get stuck otherwise.

if(!reportID) { return; }

@rojiphil thanks for the help 🙇🏻.

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 · 2025-01-08T12:40:46Z

I will fix ESLint issues again today.

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 · 2025-01-09T15:55:05Z

Finally🥴, all the lint issues are fixed, I'm on my way to record the videos... @rojiphil do you know how to replicate RTER violation?

pecanoro · 2025-01-13T16:50:18Z

@Krishna2323 Is it ready for @rojiphil's review?

Krishna2323 · 2025-01-13T23:40:34Z

@pecanoro, the code changes are done (new ESLint check issues😬). I was trying to record videos for native platforms, and it was failing on iOS a few days back. I will try again and upload them today. By the way, do you know how to replicate an RTER violation?

pecanoro · 2025-01-14T09:37:39Z

@Krishna2323 This issue is for a different bug but it explains how the RTER violation is triggered: #47511

Krishna2323 · 2025-01-15T00:09:40Z

@pecanoro, I think a company card is required to trigger an RTER violation, or is there alternative way?

rojiphil · 2025-01-15T08:10:56Z

I think a company card is required to trigger an RTER violation, or is there alternative way?

@Krishna2323 You can record for the remaining scenarios and during PR review stage we can see how we can ensure validating the fix for RTER violation. Does this help?

pecanoro · 2025-01-15T13:19:36Z

Yeah, we can do that, I will try to find an account that we can use to check if it works for RTER

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 · 2025-01-15T15:06:57Z

You can record for the remaining scenarios and during PR review stage we can see how we can ensure validating the fix for RTER violation. Does this help?

Yeah, sure. I will record the videos today. I'm currently busy with a super high-priority PR and have spent many hours resolving the ESLint issues on this PR, but the warnings increase every time I push a commit🤧.

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 · 2025-01-15T21:59:02Z

@rojiphil, you can start reviewing this, recordings have been added.

pecanoro · 2025-01-16T12:38:05Z

@rojiphil Don't forget to get to this when you have some time!

rojiphil · 2025-01-16T20:01:30Z

I have started the review here and will update tomorrow my day after the first round of review.
Meanwhile, @Krishna2323 we have conflicts to be addressed. Please resolve them. Thanks.

Signed-off-by: krishna2323 <[email protected]>

rojiphil

Thanks @Krishna2323 for the PR. The changes work well but I have left some NAB comments. Also, there are more conflicts to be resolved. Please have a look.

And regarding rter violations, we can update onyx and test like this:

Create a manual expense and ensure that the transaction thread report is open in central pane for the created expense.
Find out the transaction id for the expense report actions as shown below:
Replace <transaction_id> with the identified transaction id in the following and run it in console.
Onyx.merge("transactions_<transaction_id>”, {"cardID": 1});
Onyx.merge("transactionViolations_<transaction_id>", [{"type": "test", "name": "rter", "data": { "pendingPattern": true}}]);
Verify that the rter violation is displayed. (Note: You may have to do a dummy click on LHN to re-render the transaction report)
Click on Mark as Cash
Verify that the rter violation is dismissed

54455-web-safari-001.mp4

rojiphil · 2025-01-17T12:09:39Z

src/components/MoneyRequestHeader.tsx

@@ -60,20 +60,19 @@ function MoneyRequestHeader({report, parentReportAction, policy, onBackButtonPre
    // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth
    const {shouldUseNarrowLayout, isSmallScreenWidth} = useResponsiveLayout();
    const route = useRoute();
-    const [parentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID}`);
+    const [parentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${report?.parentReportID ?? CONST.DEFAULT_NUMBER_ID}`);


Let us use || instead of ?? as that will help address empty strings as well.

rojiphil · 2025-01-17T12:31:43Z

src/libs/TransactionUtils/index.ts

+        (violation: TransactionViolation) =>
+            violation.type === CONST.VIOLATION_TYPES.NOTICE &&
+            (showInReview === undefined || showInReview === (violation.showInReview ?? false)) &&
+            !isViolationDismissed(transactionID, violation),


Inside hasNoticeTypeViolation, let us early return for invalid transactionID just like we did for hasViolation

rojiphil · 2025-01-17T12:32:28Z

src/libs/TransactionUtils/index.ts

+            (violation: TransactionViolation) =>
+                violation.type === CONST.VIOLATION_TYPES.WARNING &&
+                (showInReview === undefined || showInReview === (violation.showInReview ?? false)) &&
+                !isViolationDismissed(transactionID, violation),


Same as above. Inside hasWarningTypeViolation, let us early return for invalid transactionID just like we did for hasViolation

rojiphil · 2025-01-17T13:50:59Z

src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx

-            transactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction?.transactionID}`]?.find(
-                (violation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION,
-            )?.data?.duplicates ?? [],
-        [transaction?.transactionID, transactionViolations],


Don't we have to keep transactionID as dependency so that allDuplicates can be updated when it becomes available? Also, I think we can remove transactionViolations and use getTransactionViolations directly.

rojiphil · 2025-01-17T13:58:21Z

src/components/ReportActionItem/MoneyRequestView.tsx

-    const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? '-1'];
-    const originalMessage = parentReportAction && ReportActionsUtils.isMoneyRequestAction(parentReportAction) ? ReportActionsUtils.getOriginalMessage(parentReportAction) : undefined;
-    return originalMessage?.IOUTransactionID ?? -1;
+    const parentReportAction = parentReportActions?.[report?.parentReportActionID ?? CONST.DEFAULT_NUMBER_ID];


I think conditional operator is neater here i.e. report?.parentReportActionID ? parentReportActions?.[report?.parentReportActionID] : undefined;

rojiphil · 2025-01-17T14:03:56Z

src/components/ReportActionItem/MoneyRequestView.tsx

-    const [parentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${parentReportID}`);
+    const parentReportID = report?.parentReportID;
+    const policyID = report?.policyID;
+    const [parentReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${parentReportID ?? CONST.DEFAULT_NUMBER_ID}`);


Same as mentioned earlier. Let us use || instead of ??. Please change this at all other similar places in this file where we default to CONST.DEFAULT_NUMBER_ID

rojiphil · 2025-01-17T14:16:37Z

src/libs/ReportActionsUtils.ts

-function getIOUActionForReportID(reportID: string, transactionID: string): OnyxEntry<ReportAction> {
+function getIOUActionForReportID(reportID?: string, transactionID?: string): OnyxEntry<ReportAction> {
+    if (!reportID || !transactionID) {
+        return;


Let's return undefined here.

rojiphil · 2025-01-17T14:22:25Z

src/libs/TransactionUtils/index.ts

+    if (!transactionID) {
+        return null;
+    }
+    return allTransactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.filter((v) => !isViolationDismissed(transactionID, v)) ?? null;


Let us use the variable name violation instead of v

rojiphil · 2025-01-17T15:16:25Z

Note: You might need to refresh the page, I am not sure if the BE is queueing these updates, let me know if not.

@pecanoro Yeah. We have to refresh the page as pusher updates are not sent. I think we need BE changes to send the pusher updates as well.

Krishna2323 added 3 commits December 23, 2024 09:33

feat: [Auth Violations] Support isDismissed on newDot to hide violati…

6463ecf

…ons. Signed-off-by: krishna2323 <[email protected]>

Merge branch 'main' into krishna2323/issue/53398

6166bb7

fix eslint.

2a179a2

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 marked this pull request as ready for review January 5, 2025 12:30

Krishna2323 requested a review from a team as a code owner January 5, 2025 12:30

melvin-bot bot requested review from rojiphil and removed request for a team January 5, 2025 12:30

Krishna2323 added 2 commits January 5, 2025 18:18

fix eslint.

cd59a56

Signed-off-by: krishna2323 <[email protected]>

fix eslint.

5682ae0

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 commented Jan 6, 2025

View reviewed changes

Krishna2323 added 3 commits January 7, 2025 03:01

fix eslint.

1016065

Signed-off-by: krishna2323 <[email protected]>

fix ESLint.

a2d0da6

Signed-off-by: krishna2323 <[email protected]>

fix ESLint.

ee414e3

Signed-off-by: krishna2323 <[email protected]>

pecanoro self-requested a review January 9, 2025 12:14

Krishna2323 added 3 commits January 9, 2025 20:54

Merge branch 'Expensify:main' into krishna2323/issue/53398

82877a6

fix ESlint issues.

18f9c6f

Signed-off-by: krishna2323 <[email protected]>

fix ESlint issues.

40c5559

Signed-off-by: krishna2323 <[email protected]>

Merge branch 'main' into krishna2323/issue/53398

3f4dd02

Merge branch 'Expensify:main' into krishna2323/issue/53398

7eb5639

fix ESlint.

593f497

Signed-off-by: krishna2323 <[email protected]>

fix ESlint.

825c4be

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 added 5 commits January 16, 2025 00:19

Merge branch 'main' into krishna2323/issue/53398

969623b

revert unnecessary changes.

486acb5

Signed-off-by: krishna2323 <[email protected]>

fix ESLint.

0a0cbc3

Signed-off-by: krishna2323 <[email protected]>

fix ESLint.

55f73fa

Signed-off-by: krishna2323 <[email protected]>

minor update.

a5194c8

Signed-off-by: krishna2323 <[email protected]>

Krishna2323 added 6 commits January 17, 2025 04:45

fix merge conflicts.

332efdf

Signed-off-by: krishna2323 <[email protected]>

minor import fix.

1ef4be5

Signed-off-by: krishna2323 <[email protected]>

Merge branch 'main' into krishna2323/issue/53398

afe362c

remove duplicate imports.

6abedea

Signed-off-by: krishna2323 <[email protected]>

minor fix.

05747b2

Signed-off-by: krishna2323 <[email protected]>

fix typescript issue.

d870956

Signed-off-by: krishna2323 <[email protected]>

rojiphil reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [Auth Violations] Support isDismissed on newDot to hide violations. #54455

feat: [Auth Violations] Support isDismissed on newDot to hide violations. #54455

Krishna2323 commented Dec 23, 2024 •

edited

Loading

melvin-bot bot commented Jan 5, 2025

Krishna2323 commented Jan 5, 2025

Krishna2323 Jan 6, 2025

rojiphil Jan 6, 2025

Krishna2323 Jan 7, 2025

Krishna2323 commented Jan 8, 2025

Krishna2323 commented Jan 9, 2025

pecanoro commented Jan 13, 2025

Krishna2323 commented Jan 13, 2025

pecanoro commented Jan 14, 2025

Krishna2323 commented Jan 15, 2025

rojiphil commented Jan 15, 2025

pecanoro commented Jan 15, 2025

Krishna2323 commented Jan 15, 2025 •

edited

Loading

Krishna2323 commented Jan 15, 2025

pecanoro commented Jan 16, 2025

rojiphil commented Jan 16, 2025

rojiphil left a comment

rojiphil Jan 17, 2025

rojiphil Jan 17, 2025

rojiphil Jan 17, 2025

rojiphil Jan 17, 2025

rojiphil Jan 17, 2025

rojiphil Jan 17, 2025

rojiphil Jan 17, 2025

rojiphil Jan 17, 2025

rojiphil commented Jan 17, 2025

feat: [Auth Violations] Support isDismissed on newDot to hide violations. #54455

Are you sure you want to change the base?

feat: [Auth Violations] Support isDismissed on newDot to hide violations. #54455

Conversation

Krishna2323 commented Dec 23, 2024 • edited Loading

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 5, 2025

Krishna2323 commented Jan 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krishna2323 commented Jan 8, 2025

Krishna2323 commented Jan 9, 2025

pecanoro commented Jan 13, 2025

Krishna2323 commented Jan 13, 2025

pecanoro commented Jan 14, 2025

Krishna2323 commented Jan 15, 2025

rojiphil commented Jan 15, 2025

pecanoro commented Jan 15, 2025

Krishna2323 commented Jan 15, 2025 • edited Loading

Krishna2323 commented Jan 15, 2025

pecanoro commented Jan 16, 2025

rojiphil commented Jan 16, 2025

rojiphil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rojiphil commented Jan 17, 2025

Krishna2323 commented Dec 23, 2024 •

edited

Loading

Krishna2323 commented Jan 15, 2025 •

edited

Loading