Skip to content

[$250] Expense - Review duplicates is shown for approver after employee resolved duplicates #73414

Closed
Closed
[$250] Expense - Review duplicates is shown for approver after employee resolved duplicates#73414
Assignees
sobitneupaneclarajones-expensify
Labels
BugSomething is broken. Auto assigns a BugZero manager.ExternalAdded to denote the issue can be worked on by a contributorHelp WantedApply this label when an issue is open to proposals by contributorsWeeklyKSv2

Description

@jponikarchuk
jponikarchuk
opened on Oct 24, 2025

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: V9.2.37-8
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from BrowserStack: https://test-management.browserstack.com/projects/2219752/test-runs/TR-1409/folder/13176704/41236826/965825487
Email or phone of affected tester (no customers): Slottwo1 fgg55@gmail.com
Issue reported by: Applause Internal Team
Device used: Redminote 10s android 13
App Component: Money Requests

Action Performed:

Pre-condition

  1. Launch app
  2. Login as admin
  3. Go to workspace and invite 2 members - employee and approver
  4. Go to workflow
  5. Change the default admin approver to user approver who is invited in step3
    Steps:
  6. Launch app
  7. As employee login
  8. Go to employee workspace chat
  9. Create 2 similar track manual distance expense to create duplicates
  10. Tap review duplicates
  11. Tap keep all
  12. Note duplicates resolved
  13. In mweb, log in as approver
  14. Open the workspace chat
  15. Note review duplicates is shown

Expected Result:

Review duplicates must not be shown for approver after employee resolved duplicates by tapping keep all.

Actual Result:

Review duplicates is shown for approver after employee resolved duplicates by tapping keep all.

Workaround:

Unknown

Platforms:

  • Android: App
  • Android: mWeb Chrome
  • iOS: App
  • iOS: mWeb Safari
  • iOS: mWeb Chrome
  • Windows: Chrome
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021981772328646379882
  • Upwork Job ID: 1981772328646379882
  • Last Price Increase: 2025-12-19
Issue OwnerCurrent Issue Owner: @sobitneupane

Activity

  1. jponikarchuk
    added
    DailyKSv2
    BugSomething is broken. Auto assigns a BugZero manager.
    on Oct 24, 2025
  2. melvin-bot
    assigned
    clarajones-expensify
    on Oct 24, 2025

  3. melvin-bot commented on Oct 24, 2025

    @melvin-bot
    melvin-botBot
    on Oct 24, 2025 – with Melvin Bot

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

  4. clarajones-expensify
    added
    ExternalAdded to denote the issue can be worked on by a contributor
    on Oct 24, 2025
  5. melvin-bot
    changed the title [-]Expense - Review duplicates is shown for approver after employee resolved duplicates[/-] [+][$250] Expense - Review duplicates is shown for approver after employee resolved duplicates[/+] on Oct 24, 2025

  6. melvin-bot commented on Oct 24, 2025

    @melvin-bot
    melvin-botBot
    on Oct 24, 2025 – with Melvin Bot

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

  7. melvin-bot
    added
    Help WantedApply this label when an issue is open to proposals by contributors
    on Oct 24, 2025
  8. melvin-bot
    assigned
    sobitneupane
    on Oct 24, 2025

  9. melvin-bot commented on Oct 24, 2025

    @melvin-bot
    melvin-botBot
    on Oct 24, 2025 – with Melvin Bot

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

  10. paulnjs commented on Oct 24, 2025

    @paulnjs
    paulnjs
    on Oct 24, 2025
    Contributor

    🚨 Edited by proposal-police: This proposal was edited at 2025-11-05 16:27:28 UTC.

    Proposal

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

    Review duplicates is shown for approver after employee resolved duplicates by tapping keep all.

    What is the root cause of that problem?

    isViolationDismissed is checking the the violation is dismissed by current user. Since the violation is dismissed by employee, it's still displayed for approver

    App/src/libs/TransactionUtils/index.ts

    Lines 1323 to 1328 in c85cfc4

    function isViolationDismissed(transaction: OnyxEntry<Transaction>, violation: TransactionViolation | undefined, currentUserEmail?: string): boolean {
    if (!transaction || !violation) {
    return false;
    }
    return !!transaction?.comment?.dismissedViolations?.[violation.name]?.[currentUserEmail ?? deprecatedCurrentUserEmail];
    }

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

    We can remove currentUserEmail and check whether transaction?.comment?.dismissedViolations?.[violation.name] has some values instead

    function isViolationDismissed(transaction: OnyxEntry<Transaction>, violation: TransactionViolation | undefined): boolean {
    if (!transaction || !violation) {
        return false;
    }
    return !!Object.values(transaction?.comment?.dismissedViolations?.[violation.name] ?? {}).length;
}

    App/src/libs/TransactionUtils/index.ts

    Lines 1323 to 1328 in c85cfc4

    function isViolationDismissed(transaction: OnyxEntry<Transaction>, violation: TransactionViolation | undefined, currentUserEmail?: string): boolean {
    if (!transaction || !violation) {
    return false;
    }
    return !!transaction?.comment?.dismissedViolations?.[violation.name]?.[currentUserEmail ?? deprecatedCurrentUserEmail];
    }

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

    Test isViolationDismissed function to verify that it returns true if the violation is dismissed by another user who is not current user

    What alternative solutions did you explore? (Optional)

    We can only do this check !!Object.values(transaction?.comment?.dismissedViolations?.[violation.name] ?? {}).length if the violation is duplicate or define a new param to check whether we should check the violation is dismissed by anyone.

    function isViolationDismissed(transaction: OnyxEntry<Transaction>, violation: TransactionViolation | undefined, currentUserEmail?: string): boolean {
    if (!transaction || !violation) {
        return false;
    }

    if (violation?.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION) {
        return Object.values(transaction?.comment?.dismissedViolations?.[violation.name]).length > 0;
    }

    return !!transaction?.comment?.dismissedViolations?.[violation.name]?.[currentUserEmail ?? deprecatedCurrentUserEmail];
}

    App/src/libs/TransactionUtils/index.ts

    Lines 1323 to 1328 in c85cfc4

    function isViolationDismissed(transaction: OnyxEntry<Transaction>, violation: TransactionViolation | undefined, currentUserEmail?: string): boolean {
    if (!transaction || !violation) {
    return false;
    }
    return !!transaction?.comment?.dismissedViolations?.[violation.name]?.[currentUserEmail ?? deprecatedCurrentUserEmail];
    }

    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.

  11. lorretheboy commented on Oct 25, 2025

    @lorretheboy
    lorretheboy
    on Oct 25, 2025
    Contributor

    Proposal

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

    Expense - Review duplicates is shown for approver after employee resolved duplicates

    What is the root cause of that problem?

    When an employee dismissed duplicate violations by tapping "Keep all", the approver would still see the "Review duplicates" button. This happened because: The code was storing the dismissal in TWO places:

    1. In TRANSACTION_VIOLATIONS collection (violations list) - correctly filtered out the duplicate violation

      App/src/libs/actions/Transaction.ts

      Line 94 in 7ae48f3

      key: ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS,

    2. In transaction.comment.dismissedViolations - stored per-user email

      App/src/libs/actions/Transaction.ts

      Lines 460 to 464 in 7ae48f3

      dismissedViolations: {
      duplicatedTransaction: {
      [dismissedPersonalDetails.login ?? '']: getUnixTime(new Date()),
      },
      },

    But the isViolationDismissed function only checked if the violation was dismissed for the current user

    App/src/libs/TransactionUtils/index.ts

    Lines 1309 to 1314 in 092472e

    function isViolationDismissed(transaction: OnyxEntry<Transaction>, violation: TransactionViolation | undefined): boolean {
    if (!transaction || !violation) {
    return false;
    }
    return !!transaction?.comment?.dismissedViolations?.[violation.name]?.[currentUserEmail];
    }

    Which then caused the mismatch:

    • Employee dismissed: dismissedViolations.duplicatedTransaction[employee@email] = timestamp
    • Approver checked: isViolationDismissed(..., approver@email) → looked for dismissedViolations.duplicatedTransaction[approver@email] → NOT FOUND → returned false

    Therefore: Approver still saw the "Review duplicates" button

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

    We should remove the redundant dismissal storage in dismissedViolations for duplicate violations. The violation is already being removed from the violations list, which is the source of truth

    App/src/libs/actions/Transaction.ts

    Lines 453 to 469 in 7ae48f3

    const optimisticDataTransactions: OnyxUpdate[] = currentTransactions.map((transaction) => ({
    onyxMethod: Onyx.METHOD.MERGE,
    key: `${ONYXKEYS.COLLECTION.TRANSACTION}${transaction.transactionID}`,
    value: {
    ...transaction,
    comment: {
    ...transaction.comment,
    dismissedViolations: {
    duplicatedTransaction: {
    [dismissedPersonalDetails.login ?? '']: getUnixTime(new Date()),
    },
    },
    },
    },
    }));
    optimisticData.push(...optimisticDataTransactions);

    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.

  12. melvin-bot commented on Oct 27, 2025

    @melvin-bot
    melvin-botBot
    on Oct 27, 2025 – with Melvin Bot

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

  13. melvin-bot
    added
    Overdue
    on Oct 27, 2025

  14. sobitneupane commented on Oct 29, 2025

    @sobitneupane
    sobitneupane
    on Oct 29, 2025
    Contributor

    Thanks for the proposal, @paulnjs and @lorretheboy

    isViolationDismissed was added in #54455 PR as a part of #53398 issue to make the behavior in newDot consistent with oldDot. I think it’d be helpful to review both the issue and PR for more context.

    @paulnjs Instead of removing currentUserEmail entirely, could we simply avoid passing it for this specific case?

  15. 27 remaining items

  16. clarajones-expensify commented on Dec 4, 2025

    @clarajones-expensify
    clarajones-expensify
    on Dec 4, 2025
    Contributor

    @pecanoro - is this comment correct? if so i will close this out. TY!

  17. melvin-bot commented on Dec 5, 2025

    @melvin-bot
    melvin-botBot
    on Dec 5, 2025 – with Melvin Bot

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

  18. melvin-bot
    added
    WeeklyKSv2
    and removed
    DailyKSv2
    on Dec 9, 2025

  19. melvin-bot commented on Dec 9, 2025

    @melvin-bot
    melvin-botBot
    on Dec 9, 2025 – with Melvin Bot

    This issue has not been updated in over 14 days. @sobitneupane, @clarajones-expensify eroding to Weekly issue.

  20. melvin-bot
    removed
    Overdue
    on Dec 9, 2025

  21. melvin-bot commented on Dec 12, 2025

    @melvin-bot
    melvin-botBot
    on Dec 12, 2025 – with Melvin Bot

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

  22. mvtglobally commented on Dec 13, 2025

    @mvtglobally
    mvtglobally
    on Dec 13, 2025

    Issue not reproducible during KI retests. (First week)

  23. melvin-bot commented on Dec 19, 2025

    @melvin-bot
    melvin-botBot
    on Dec 19, 2025 – with Melvin Bot

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

  24. melvin-bot
    added
    Overdue
    on Dec 19, 2025

  25. sobitneupane commented on Dec 22, 2025

    @sobitneupane
    sobitneupane
    on Dec 22, 2025
    Contributor

    Issue not reproducible.

  26. melvin-bot
    removed
    Overdue
    on Dec 22, 2025

  27. pecanoro commented on Dec 23, 2025

    @pecanoro
    pecanoro
    on Dec 23, 2025
    Contributor

    Let's close this since it's not reproducible

  28. pecanoro
    closed this as completedon Dec 23, 2025
  29. github-project-automation
    moved this to Done in [#whatsnext] #qualityon Dec 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

BugSomething is broken. Auto assigns a BugZero manager.ExternalAdded to denote the issue can be worked on by a contributorHelp WantedApply this label when an issue is open to proposals by contributorsWeeklyKSv2

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions