Web - Taxes - RBR shown on expense preview without any errors in the transaction thread

IuliiaHerets commented 1 week 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: 9.0.64-0 Reproducible in staging?: Y Reproducible in production?: Y Issue was found when executing this PR: https://github.com/Expensify/App/pull/52309 Email or phone of affected tester (no customers): applausetester+nl797@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Navigate to staging.new.expensify.com
Create a workspace
Enable tax feature
Add a tax
Create an expense with the added tax on step 4

Expected Result:

No RBR on the expense

Actual Result:

RBR shown without any errors in the transaction thread

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/916d1666-0801-4d23-877e-c989a4d93482

View all open jobs on GitHub

melvin-bot[bot] commented 1 week ago

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

Krishna2323 commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-11-19 10:21:29 UTC.

Proposal 

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

Web - Taxes - RBR shown on expense preview without any errors in the transaction thread

What is the root cause of that problem?

In hasNoticeTypeViolation we aren't checking for ['taxAmountChanged', 'taxRateChanged'], these notice types shouldn't be used in the NewDot as mentioned here. https://github.com/Expensify/App/blob/f77087f02a42e17e7da7420528cc51b54003ca61/src/libs/TransactionUtils/index.ts#L874-L881
What changes do you think we should make in order to solve the problem? 
We should update hasNoticeTypeViolation to check if the violation name is taxAmountChanged' or 'taxRateChanged' and only proceed to the next check if it is not equal totaxAmountChanged' or 'taxRateChanged'.

function hasNoticeTypeViolation(transactionID: string, transactionViolations: OnyxCollection<TransactionViolation[]>, showInReview?: boolean): boolean {
    // We don't want to show these violations on NewDot
    const excludedViolationsName = ['taxAmountChanged', 'taxRateChanged'];
    return !!transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.some(
        (violation: TransactionViolation) =>
            !excludedViolationsName.includes(violation.name) &&
            violation.type === CONST.VIOLATION_TYPES.NOTICE &&
            (showInReview === undefined || showInReview === (violation.showInReview ?? false)),
    );

If needed, we can also add this check in other violation check util functions like hasWarningTypeViolation, hasViolation.
What alternative solutions did you explore? (Optional)

Result

melvin-bot[bot] commented 2 days ago

@JmillsExpensify Eep! 4 days overdue now. Issues have feelings too...

JmillsExpensify commented 19 hours ago

I actually forget if this is by design. @MonilBhavsar can you clarify?

Krishna2323 commented 18 hours ago

@JmillsExpensify, it was decided to not show taxRateChanged & taxAmountChanged.

Please see these comments: https://github.com/Expensify/App/issues/41401#issuecomment-2094301452 https://github.com/Expensify/App/pull/41649#issuecomment-2109228051

MonilBhavsar commented 11 hours ago

https://github.com/Expensify/App/issues/41401#issuecomment-2094301452 https://github.com/Expensify/App/pull/41649#issuecomment-2109228051

Reading through these comments.

Looks like we no longer display taxRateChanged and taxAmountChanged notice violation because we have system messages and reviewer/approver can know if it has changed. That sounds good!

If so, we need to stop sending these violations from server at all for newDot requests and not add any conditional logic on the client side. Also remove the code we have currently

Krishna2323 commented 5 hours ago

Also remove the code we have currently

@MonilBhavsar, I can work on removing the conditional logic from the frontend if needed.

JmillsExpensify commented 5 hours ago

Nice, thanks both for the refresher. Agree with those comments.

Expensify / App