App icon indicating copy to clipboard operation
App copied to clipboard

[HOLD for payment 2024-12-05] [$250] Dupe detection - RBR is not shown when creating dupe expense with a Gmail account

Open IuliiaHerets opened this issue 1 year ago β€’ 29 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: v9.0.59-0 Reproducible in staging?: Y Reproducible in production?: Y Issue was found when executing this PR: https://github.com/Expensify/App/pull/48958 Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

  1. Login to an account.
  2. Create a workspace
  3. Open the Workspace chat
  4. Submit an expense in the Workspace with a category,
  5. Submit another expense in the Workspace with a the same amount and merchant in the above but with no category.

Expected Result:

RBR(red dot) is shown in LHN and also in the report because of the dupe expense.

Actual Result:

RBR(red dot) is not shown in LHN and in the report. RBR is only shown inside the expense report

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/6067710d-aceb-409e-b71b-b12145014331

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021857074546459286027
  • Upwork Job ID: 1857074546459286027
  • Last Price Increase: 2024-11-21
  • Automatic offers:
    • ahmedGaber93 | Reviewer | 105026209
Issue OwnerCurrent Issue Owner: @muttmuure

IuliiaHerets avatar Nov 08 '24 10:11 IuliiaHerets

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

melvin-bot[bot] avatar Nov 08 '24 10:11 melvin-bot[bot]

Edited by proposal-police: This proposal was edited at 2024-11-21 13:28:28 UTC.

Proposal

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

RBR is not shown on the report preview when money request is dupe.

What is the root cause of that problem?

The RBR doesn't show on the report preview because it only shows if the dupe detection permission is enabled. IF the account doesn't have the permission, then RBR won't show. https://github.com/Expensify/App/blob/5e69bfc36ddfd82a113606257abb93f2fa78f7db/src/components/ReportActionItem/ReportPreview.tsx#L156-L160 https://github.com/Expensify/App/blob/5e69bfc36ddfd82a113606257abb93f2fa78f7db/src/libs/TransactionUtils/index.ts#L832-L842

However, on the individual expense preview, we just checks for the amount of the duplicates violation without checking whether the permission is enabled or not, which is different form hasWarningTypeViolations. https://github.com/Expensify/App/blob/5e69bfc36ddfd82a113606257abb93f2fa78f7db/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L147-L149

So, I think there are 2 issues here. In ReportPreview, we check for warning type violation. In MoneyRequestPreviewContent, we don't check for warning type violation, but we check for duplicates, ignoring the dupe detection permission.

For the RBR doesn't show for workspace chat in the LHN issue, we already have a logic to do that actually. https://github.com/Expensify/App/blob/5955bd797eaaf60c2b02db95a35c18663d47effe/src/libs/ReportUtils.ts#L6352-L6356

It checks all the related report and see if there are violations. However, hasWarningTypeViolations always returns false. It happens because the warningTypeViolations here is empty. Even though the dupe violation is a warning type, the showInReview is not null. https://github.com/Expensify/App/blob/5955bd797eaaf60c2b02db95a35c18663d47effe/src/libs/TransactionUtils/index.ts#L886-L891

It's because we pass nothing to showInReview. So, undefined !== null.

https://github.com/Expensify/App/blob/5955bd797eaaf60c2b02db95a35c18663d47effe/src/libs/ReportUtils.ts#L6355 https://github.com/Expensify/App/blob/5955bd797eaaf60c2b02db95a35c18663d47effe/src/libs/ReportUtils.ts#L6370-L6372

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

Let's use hasWarningTypeViolations in MoneyRequestPreviewContent too.

const hasWarningTypeViolations = TransactionUtils.hasWarningTypeViolation(transaction?.transactionID ?? '-1', transactionViolations);
...
const shouldShowRBR = hasNoticeTypeViolations || hasViolations || hasFieldErrors || (!isFullySettled && !isFullyApproved && isOnHold) || hasWarningTypeViolations;

(MoneyRequestPreviewContent also checks for hasNoticeTypeViolation but ReportPreview don't. We can add it to ReportPreview too if needed by adding it to ReportUtils, just like the others, but I don't know how to trigger notice type violation, so not sure.)

To fix the LHN RBR issue, we should first update the showInReview type by removing the null type. Then, check for undefined instead of null. https://github.com/Expensify/App/blob/5955bd797eaaf60c2b02db95a35c18663d47effe/src/libs/TransactionUtils/index.ts#L886-L891

(or we can just pass update showInReview to be a required param but keep it nullable)

bernhardoj avatar Nov 08 '24 11:11 bernhardoj

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

melvin-bot[bot] avatar Nov 11 '24 15:11 melvin-bot[bot]

@muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar Nov 12 '24 09:11 melvin-bot[bot]

@muttmuure Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] avatar Nov 14 '24 09:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 14 '24 14:11 melvin-bot[bot]

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

melvin-bot[bot] avatar Nov 14 '24 14:11 melvin-bot[bot]

@bernhardoj Thanks for the proposal

Your proposal fix the issue on report preview but not on LHN. Can you also fix it?

https://github.com/user-attachments/assets/7a611cc4-d524-4546-9194-1bf974d19dae

ahmedGaber93 avatar Nov 16 '24 07:11 ahmedGaber93

I'm not sure if we should show the RBR on the workspace chat. In this PR, they mention that the RBR should show on the workspace chat, but this comment says otherwise.

NOTE: the previous logic before the PR also doesn't show expense report violations RBR on workspace chat.

bernhardoj avatar Nov 16 '24 14:11 bernhardoj

they mention that the RBR should show on the workspace chat, but this https://github.com/Expensify/App/pull/51893#issuecomment-2468956992 says otherwise.

I think they said it should show on some cases only

Screenshot 2024-11-16 at 4 00 45β€―PM

@iwiznia Just for confirmation, in the duplication violations case, should we show RBR on the workspace chat on LHN?

ahmedGaber93 avatar Nov 17 '24 00:11 ahmedGaber93

Yes, RBRs in the LHN should only show in the workspace chat

iwiznia avatar Nov 18 '24 14:11 iwiznia

Thanks for confirmation

ahmedGaber93 avatar Nov 18 '24 14:11 ahmedGaber93

@bernhardoj Bump for this change https://github.com/Expensify/App/issues/52243#issuecomment-2483225270

ahmedGaber93 avatar Nov 19 '24 11:11 ahmedGaber93

Ok, I got it now. It's supposed to work but there is a small bug. Updated my proposal to fix that.

bernhardoj avatar Nov 21 '24 13:11 bernhardoj

πŸ“£ 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] avatar Nov 21 '24 16:11 melvin-bot[bot]

Updated my proposal to fix that.

Great, I will review it tomorrow

ahmedGaber93 avatar Nov 22 '24 00:11 ahmedGaber93

@ahmedGaber93 @muttmuure this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar Nov 22 '24 09:11 melvin-bot[bot]

@bernhardoj's proposal LGTM!

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

ahmedGaber93 avatar Nov 22 '24 12:11 ahmedGaber93

Triggered auto assignment to @AndrewGable, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] avatar Nov 22 '24 12:11 melvin-bot[bot]

πŸ“£ @ahmedGaber93 πŸŽ‰ 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] avatar Nov 22 '24 17:11 melvin-bot[bot]

PR is ready

cc: @ahmedGaber93

bernhardoj avatar Nov 23 '24 07:11 bernhardoj

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] avatar Nov 28 '24 14:11 melvin-bot[bot]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.67-9 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

  • https://github.com/Expensify/App/pull/52994

If no regressions arise, payment will be issued on 2024-12-05. :confetti_ball:

For reference, here are some details about the assignees on this issue:

  • @ahmedGaber93 requires payment automatic offer (Reviewer)
  • @bernhardoj requires payment through NewDot Manual Requests

melvin-bot[bot] avatar Nov 28 '24 14:11 melvin-bot[bot]

@ahmedGaber93 @muttmuure @ahmedGaber93 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot[bot] avatar Nov 28 '24 14:11 melvin-bot[bot]

BugZero Checklist:

  • [x] [Contributor] Classify the bug:
Bug classification

Source of bug:

  • [x] 1a. Result of the original design (eg. a case wasn't considered)
  • [ ] 1b. Mistake during implementation
  • [ ] 1c. Backend bug
  • [ ] 1z. Other:

Where bug was reported:

  • [x] 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
  • [ ] 2b. Reported on staging (eg. found during regression or PR testing)
  • [ ] 2d. Reported on a PR
  • [ ] 2z. Other:

Who reported the bug:

  • [ ] 3a. Expensify user
  • [ ] 3b. Expensify employee
  • [ ] 3c. Contributor
  • [ ] 3d. QA
  • [x] 3z. Other: Applause Internal Team
  • [x] [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

    Link to comment: https://github.com/Expensify/App/pull/47056/files#r1876926777

  • [x] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

    Link to discussion:N/A.

  • [x] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

  • [ ] [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

There is no need for regression test because this issue related to dupe detection beta and it recently removed here https://github.com/Expensify/App/pull/53167

ahmedGaber93 avatar Dec 09 '24 23:12 ahmedGaber93

@muttmuure Could you please add the payment summary here? Also, please ignore the Upwork offer for me (I will request it on ND).

ahmedGaber93 avatar Dec 09 '24 23:12 ahmedGaber93

Requested in ND.

bernhardoj avatar Dec 10 '24 02:12 bernhardoj

@AndrewGable, @ahmedGaber93, @muttmuure, @bernhardoj Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] avatar Dec 10 '24 09:12 melvin-bot[bot]

Just waiting on payment to close

AndrewGable avatar Dec 10 '24 23:12 AndrewGable

I need a payment summary to approve payment

JmillsExpensify avatar Dec 11 '24 13:12 JmillsExpensify