[$250] [Auth Violations] Support isDismissed on newDot to hide violations

[pecanoro]

Part of the Auth Violations project

Main issue: https://github.com/Expensify/Expensify/issues/422533 Project: https://github.com/Expensify/Expensify/issues/393838

Feature Description

Currently we have four violations that can be dismissed in old Expensify:

• DUPLICATED_TRANSACTION
• SMARTSCAN_FAILED
• RTER
• AUTOREPORTED_REJECTED_EXPENSE (Not implemented in newDot yet)

If the violation has been dismissed, we don't show the violation to the user. In PHP, we currently have a function like this:

    /**
     * Checks whether a violation has been dismissed, optionally by a particular email address.
     *
     * @param string $violationName check Policy_Violation::NAME_*
     */
    public function isViolationDismissed(string $violationName, string $email = ''): bool
    {
        // If we don't provide an email, we just check if the violation has been dismissed by anyone
        if (!$email) {
            return count($this->getNVP('dismissedViolations')[$violationName] ?? []) > 0;
        }

        $timestamp = $this->getNVP('dismissedViolations')[$violationName][$email] ?? '';
        return strlen(strval($timestamp)) > 0;
    }

Note: The NVP is stored in the transaction's "comment" along with the rest of the name-value pairs.

Ideally, we should add this logic in newDot similar to what we did for duplicate transactions here. My best guess is that we need to make that logic work in these functions here so we stop showing the violation in the LHN and the transaction itself. However, I might be missing other spots.

Take into account that checking for dismissed violations should be optional in case there are flows in which we want to show it regardless or block submission because of that.

Manual Test Steps

1. Scan a receipt in which the smartscan will fail (illegible or not a receipt)
2. Go to old Expensify, find the report and dismiss the violation.
3. 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.

cc/ @cead22 @iwiznia

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021864348688314983900
• Upwork Job ID: 1864348688314983900
• Last Price Increase: 2024-12-04

Issue Owner

Current Issue Owner: @rojiphil

[melvin-bot[bot]]

Triggered auto assignment to @johncschuster (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[melvin-bot[bot]]

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

[melvin-bot[bot]]

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[rojiphil]

@pecanoro, While testing the scenarios for resolving DUPLICATED_TRANSACTION and SMARTSCAN_FAILED from OD, it was noticed that the pusher updates had not been sent in ND. Refreshing the page though makes it work. So, we will need these to be pushed from BE. I have not yet checked RTER which I will do tomorrow my day and update.

Meanwhile, where can we find the details related to AUTOREPORTED_REJECTED_EXPENSE? If this is within a design document, please grant me access. Thanks.

[pecanoro]

Meanwhile, where can we find the details related to AUTOREPORTED_REJECTED_EXPENSE? If this is within a design document, please grant me access. Thanks.

I don't think we have talked about implementing this yet but we should still implement this in a way that will work with all types of violations regardless.

[pecanoro]

What I mean is that even though we cannot test AUTOREPORTED_REJECTED_EXPENSE, the solution should cover any possible dismissed violation.

[dubielzyk-expensify]

Any visual changes as part of this change you want me to review from a design side?

[pecanoro]

@dubielzyk-expensify No, no visual changes, feel free to unassign yourself!

[melvin-bot[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[bot]]

Upwork job price has been updated to $400

[melvin-bot[bot]]

Upwork job price has been updated to $500

[nkdengineer]

Proposal

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

Support isDismissed on newDot to hide violations

What is the root cause of that problem?

This is a new feature

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

1. We can define a util to check if a violation is dismissed. This function will have two params violationName, transactionID. It will return true if the violation is dismissed. Otherwise, return false.

function isViolationDismissed(transactionID: string, violationName: ViolationName): boolean {
    return allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]?.comment?.dismissedViolations?.[violationName]?.[currentUserEmail] === `${currentUserAccountID}`;
}

2. After that we can reuse this function in these places to only return true if we have some violation that isn't dismissed

For example for hasViolation function, we can reuse like this

function hasViolation(transactionID: string, transactionViolations: OnyxCollection<TransactionViolations>, showInReview?: boolean): boolean {
    return !!transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.some(
        (violation: TransactionViolation) =>
            violation.type === CONST.VIOLATION_TYPES.VIOLATION &&
            (showInReview === undefined || (showInReview === (violation.showInReview ?? false) && !isViolationDismissed(transactionID, violation.name))),
    );
}

https://github.com/Expensify/App/blob/3f375387b1881fe524e8b80cc3d1af4a456466de/src/libs/TransactionUtils/index.ts#L858

Maybe we need to update some other places like hasBrokenConnectionViolation function (RTER violation) or MoneyRequestView here (SMARTSCAN_FAILED) to filter out the dismissed violation before we get the result. We need to test carefully in the PR to ensure we update all necessary places.

Optional: We can add a new param checkDismissed in hasViolation and other functions to only exclude the dismissed violation if checkDismissed is true.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Create a test for isViolationDismissed function to verify that it works as expected for all requirement cases

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[melvin-bot[bot]]

@rojiphil, @johncschuster, @pecanoro Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Krishna2323]

Proposal

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

[Auth Violations] Support isDismissed on newDot to hide violations

What is the root cause of that problem?

New implementation

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

• We will first create a util function isViolationDismissed, the function will look like:

function isViolationDismissed(transactionID: string, violation: TransactionViolation) {
    const dismissedViolations = Object.keys(allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]?.comment?.dismissedViolations ?? {});
    return !dismissedViolations.includes(violation.name);
}

• Then We will modify the functions below to add an additional check in the functions below to filter out the dismissed violations using isViolationDissmissed.

Code details

/**
 * Checks if any violations for the provided transaction are of type 'violation'
 */
function hasViolation(transactionID: string, transactionViolations: OnyxCollection<TransactionViolations>, showInReview?: boolean): boolean {
    return !!transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.some(
        (violation: TransactionViolation) =>
            violation.type === CONST.VIOLATION_TYPES.VIOLATION &&
            !isViolationDismissed(transactionID, violation) &&
            (showInReview === undefined || showInReview === (violation.showInReview ?? false)),
    );
}

/**
 * Checks if any violations for the provided transaction are of type 'notice'
 */
function hasNoticeTypeViolation(transactionID: string, transactionViolations: OnyxCollection<TransactionViolation[]>, showInReview?: boolean): boolean {
    return !!transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.some(
        (violation: TransactionViolation) =>
            violation.type === CONST.VIOLATION_TYPES.NOTICE &&
            !isViolationDismissed(transactionID, violation) &&
            (showInReview === undefined || showInReview === (violation.showInReview ?? false)),
    );
}

/**
 * Checks if any violations for the provided transaction are of type 'warning'
 */
function hasWarningTypeViolation(transactionID: string, transactionViolations: OnyxCollection<TransactionViolation[]>, showInReview?: boolean): boolean {
    const violations = transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID];
    const warningTypeViolations =
        violations?.filter(
            (violation: TransactionViolation) =>
                violation.type === CONST.VIOLATION_TYPES.WARNING &&
                !isViolationDismissed(transactionID, violation) &&
                (showInReview === undefined || showInReview === (violation.showInReview ?? false)),
        ) ?? [];

    const hasOnlyDupeDetectionViolation = warningTypeViolations?.every((violation: TransactionViolation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION);
    if (hasOnlyDupeDetectionViolation) {
        return false;
    }

    return warningTypeViolations.length > 0;
}

/**
 * Get all transaction violations of the transaction with given tranactionID.
 */
function getTransactionViolations(transactionID: string, transactionViolations: OnyxCollection<TransactionViolations> | null): TransactionViolations | null {
    return transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.filter((v) => !isViolationDismissed(transactionID, v)) ?? null;
}

https://github.com/Expensify/App/blob/50c1db81004285968c6388bc941860f554d411e6/src/libs/TransactionUtils/index.ts#L872-L906 https://github.com/Expensify/App/blob/50c1db81004285968c6388bc941860f554d411e6/src/libs/TransactionUtils/index.ts#L694-L699

• We also need to AUTOREPORTED_REJECTED_EXPENSE violation in the VIOLATIONS object.
  https://github.com/Expensify/App/blob/50c1db81004285968c6388bc941860f554d411e6/src/CONST.ts#L4774
• Additionally, I thing it would be the best to use getTransactionViolations when getting violations for a specific transaction. This way we can fix all instances without adding the filter everywhere. https://github.com/Expensify/App/blob/50c1db81004285968c6388bc941860f554d411e6/src/components/ReportActionItem/MoneyRequestView.tsx#L101 https://github.com/Expensify/App/blob/50c1db81004285968c6388bc941860f554d411e6/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L135-L142 https://github.com/Expensify/App/blob/50c1db81004285968c6388bc941860f554d411e6/src/pages/Debug/TransactionViolation/DebugTransactionViolationCreatePage.tsx#L65 https://github.com/Expensify/App/blob/50c1db81004285968c6388bc941860f554d411e6/src/pages/TransactionDuplicate/Review.tsx#L33

NOTE: I haven't mentioned all the places where we should use getTransactionViolations, we can do that in the PR phase. We can also update isViolationDismissed to check for currentUserEmail every violation if needed, like it is done in isDuplicate. https://github.com/Expensify/App/blob/3f375387b1881fe524e8b80cc3d1af4a456466de/src/libs/TransactionUtils/index.ts#L828-L830

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?


• We can write a test where we can mock few transactions and transactionViolations in ONYX and test out the util functions we are updating.

What alternative solutions did you explore? (Optional)

[johncschuster]

@rojiphil can you please review the proposals above?

[rojiphil]

can you please review the proposals above?

@johncschuster Yeah... I was working on this. Will send my comments in a while.

[rojiphil]

Thanks all for your proposals.

I thing it would be the best to use getTransactionViolations when getting violations for a specific transaction. This way we can fix all instances without adding the filter everywhere.

I like the idea of filtering the dismissedViolations in getTransactionViolations which can help us to have a generic approach without having to worry about individual cases @Krishna2323 Please note that we do not have to add AUTOREPORTED_REJECTED_EXPENSE to violations object as we would anyway have a generic solution.

@pecanoro We can go with @Krishna2323 proposal which looks LGTM in general. More specifics about handling on the individual cases can be handled at the PR stage. 🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@rojiphil, @johncschuster, @pecanoro Whoops! This issue is 2 days overdue. Let's get this updated quick!

[pecanoro]

Sounds good! Assigning @Krishna2323 to the issue

[melvin-bot[bot]]

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

Offer link Upwork job

[melvin-bot[bot]]

📣 @Krishna2323 🎉 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 📖

[Krishna2323]

PR will be up within 24 hours.

[johncschuster]

Heads up, I will be OOO from Monday, December 23, and returning Monday, January 6. I am not reassigning my assigned issues. If you feel this issue needs to be actioned by a Bug Zero team member, please reach out for help from any available team members in #expensify-open-source.

If not, I'll action this as soon as I'm back in office!

[Krishna2323]

@pecanoro after we dismiss the smartscan fail violation from OldDot and enter the details manually, a new warning type violation is received receiptNotSmartScanned. Is this expected?

https://github.com/user-attachments/assets/32056ef0-0c3c-4b0e-b900-78aa5e70947c

[pecanoro]

@pecanoro after we dismiss the smartscan fail violation from OldDot and enter the details manually, a new warning type violation is received receiptNotSmartScanned. Is this expected?

Yeah, I think it only happens with that one, it's a weird one. They are different types of violations.

[Krishna2323]

@pecanoro @rojiphil , do I need to record all the listed violations, or is recording only SMARTSCAN_FAILED sufficient?

1. DUPLICATED_TRANSACTION
2. SMARTSCAN_FAILED
3. RTER

[melvin-bot[bot]]

@rojiphil, @johncschuster, @pecanoro, @Krishna2323 Eep! 4 days overdue now. Issues have feelings too...