Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.52k stars 2.88k forks source link

[$250] Dupe detect - Paid expense appears in Review duplicates flow but unable to complete the flow #46372

Closed lanitochka17 closed 3 weeks ago

lanitochka17 commented 3 months 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.13-3 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace chat
  3. Submit an expense
  4. Pay the expense
  5. Submit another two same expenses (same amount, date and merchant) as expense from Step 3
  6. Go to transaction thread of any of the submitted expense from Step 5
  7. Click Review duplicates
  8. Click Keep this one on the paid expense (from Step 4)
  9. Click Confirm
  10. Note that the other two expenses are not removed
  11. Go to transaction thread of any of the submitted expense from Step 5
  12. Click Review duplicates
  13. Click Keep this one on any of the unpaid expense (from Step 5)
  14. Click Confirm
  15. Note that the process fails and Review duplicates button reappears

Expected Result:

In Step 8, the paid expense should not appear in Review duplicates flow if we cannot keep only the paid one (Step 10)

Actual Result:

In Step 8, the paid expense appears in Review duplicates flow and we cannot keep only the paid one (Step 10) We also cannot keep only the unpaid one (Step 15) after attempting to keep only the paid expense

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/731d19e9-7c4a-4897-a442-db366f5e4dda

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01add0c19e8ce4b462
  • Upwork Job ID: 1817919014389694342
  • Last Price Increase: 2024-08-12
Issue OwnerCurrent Issue Owner: @zanyrenney
melvin-bot[bot] commented 3 months ago

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

lanitochka17 commented 3 months ago

We think that this bug might be related to #wave-collect - Release 2

lanitochka17 commented 3 months ago

@zanyrenney FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

nyomanjyotisa commented 3 months ago

Proposal

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

Paid expense appears in Review duplicates flow but unable to complete the flow

What is the root cause of that problem?

Paid duplicates transaction from other reports are included in the transactionViolations onyx data from the API response

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

We should filter the transactions to show in the Review duplicates page to only show transaction within the current report

Add the following code here

    const finalTransactions = useMemo(
        () => transactions?.filter((transaction) => report?.chatReportID === transaction?.reportID),
        [transactions, report?.chatReportID]
    );

    const finalTransactionIDs = useMemo(
        () => finalTransactions?.map((transaction) => transaction?.transactionID),
        [finalTransactions]
    );

use finalTransactionIDs here

and use finalTransactions here

RESULT

https://github.com/user-attachments/assets/fa997579-ce8e-4321-b508-4e5abf1f175b

What alternative solutions did you explore? (Optional)

daledah commented 3 months ago

Edited by proposal-police: This proposal was edited at 2024-08-09 07:00:14 UTC.

Proposal

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

In Step 8, the paid expense appears in Review duplicates flow and we cannot keep only the paid one (Step 10) We also cannot keep only the unpaid one (Step 15) after attempting to keep only the paid expense

What is the root cause of that problem?

In step 8, the list of transactions to display is from: https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/pages/TransactionDuplicate/Review.tsx#L36 where we do not filter out the paid transactions.

The similar logic appears in: https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L282 https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/libs/TransactionUtils.ts#L853 https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/pages/TransactionDuplicate/Review.tsx#L36 https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/libs/TransactionUtils.ts#L973

Then, when calling API Transaction_Merge, the transactionIDList includes paid expenses as well.

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

We should create a function to filter out the paid expense:

function removeSettledTransactions(transactionIDs: string[]) {
    return transactionIDs.filter((transactionID) => !isSettled(allTransactions?.[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`]?.reportID));
}

then use it in https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L282 https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/libs/TransactionUtils.ts#L853 https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/libs/TransactionUtils.ts#L973

Additional implementation based on new expected behavior in comment:

We should display a message under the Keep All button indicating that some duplicates have already been approved or reimbursed.

In there, add:

    const settledTransaction = transactions.find((transaction) => isSettled(transaction?.reportID));

to know whether we have the transactions that have already been approved or reimbursed.

Then use it to display the text "Some of these duplicates have been approved or paid already":

                    {!!settledTransaction && <View style={[styles.textNormal, styles.colorMuted]}>Some of these duplicates have been approved or paid already</View>}

We can't remove the Keep this one button completely as there might be more than one unapproved expense and the user might want to keep one of them.

We can update it: https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L443 to:

            {isReviewDuplicateTransactionPage && !isSettled (
zanyrenney commented 3 months ago

adding external and to project

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

zanyrenney commented 3 months ago

Hey there @parasharrajat please can you review the proposals above and let me know if we can move forward with either of these.

parasharrajat commented 3 months ago

Let me check.

parasharrajat commented 3 months ago

May be a backend issue. Let me get some clarifications.

parasharrajat commented 3 months ago

OK so the approved and paid expenses should not come in the duplicate violations. I think it is backend issue if backend is sending those. :ribbon: :eyes: :ribbon: C+ reviewed

melvin-bot[bot] commented 3 months ago

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

parasharrajat commented 3 months ago

@techievivek Could you please check if backend needs to be fixed here?

techievivek commented 3 months ago

Looks like we are returning transactions from other reports as well.

techievivek commented 3 months ago

@cead22 @pecanoro, Looking for some help here. Are transactions from other reports expected to show up on the review duplicates page?

techievivek commented 3 months ago

Not overdue, already commented above.

melvin-bot[bot] commented 3 months ago

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

JmillsExpensify commented 3 months ago

Looking for some help here. Are transactions from other reports expected to show up on the review duplicates page?

Yes, that's the core design of the feature, showing duplicates no matter whether they are on the same report or not. cc @trjExpensify

parasharrajat commented 3 months ago

But paid or approved expenses should not show up in duplicates.

trjExpensify commented 3 months ago

That's a good question. I think dupes do get flagged on approved and paid reports, but perhaps @pecanoro can confirm. I'm unsure how that's supposed to technically work today on OldDot when it comes to merging expenses and deleting (potentially) a dupe on an approved/paid report? 🤔

JmillsExpensify commented 3 months ago

@pecanoro can you confirm? I would be surprised if we are ignoring an expense that an admin has paid, which has the same date and amount that someone has just potentially re-submitting, though that scenario also creates problems in itself, since to @trjExpensify's point, you can't merge a paid/approved expense.

pecanoro commented 3 months ago

I need to test this on oldDot tomorrow. We don't compute violations on approved and paid reports, but I am not really sure if, when trying to find duplicates from an expense on an open report, we flag paid expenses as potential duplicates.

zanyrenney commented 3 months ago

Hey @pecanoro I'm just following up on this one if you managed to test it on olddot so we can move ahead here? Thanks!

pecanoro commented 3 months ago

In oldDot, it also gets flagged if there is an expense in a reimbursed report. But the one in the open report is the one that gets flagged:

image

image

pecanoro commented 3 months ago

But this is the difference, you can't merge them if one of them is in an approved or reimbursed report, you can only click ignore to dismiss the violation:

image

parasharrajat commented 3 months ago

So it means we need to handle this in NewDot as well on the client side. How UI would look on newDot for this case?

  1. Either we hide the paid or approved duplicated expense from the review duplicate page.
  2. Or we disable keep this one on the approved expense and the merge duplicate button. Thus user would only be able to click keep this one on the current report's expense which are not approved.
pecanoro commented 3 months ago

My opunion is that we should only leave the Keep All option and show a similar message to the one in oldDot to let them know they have to be in open or submitted reports. But @JmillsExpensify and @trjExpensify probably have a opinion that makes more sense form the design point of view

trjExpensify commented 3 months ago

My opunion is that we should only leave the Keep All option and show a similar message to the one in oldDot to let them know they have to be in open or submitted reports

I was thinking the same, so like:

image

parasharrajat commented 3 months ago

Ok. And pressing Keep all will resolve the unapproved expense only.

trjExpensify commented 3 months ago

Yeah, to Rocio's point:

But the one in the open report is the one that gets flagged:

The duplicate violations doesn't show on the expense on the approved / reimbursed report.

techievivek commented 3 months ago

Thank you for confirming the expected behavior. So we want to have

  1. We do not want to display duplicate violations for approved/reimbursed expenses.
  2. Instead, we should display a message under the Keep All button indicating that some duplicates have already been approved or reimbursed. And we want to get rid of the keep this one option completely from UI.

@nyomanjyotisa @daledah, can you please update the proposal accordingly? Thanks.

daledah commented 3 months ago

And we want to get rid of the keep this one option completely from UI.

@techievivek This will appear even if there is no approved/reimburse, right?

daledah commented 3 months ago

Proposal updated

parasharrajat commented 3 months ago

@techievivek I think it will be a little different.

  1. Agree with point 1.
  2. A minor improvement to point 2 would be that the keep this one will be hidden on paid or approved expenses only. Imagine there are 3 duplicates on the review screen and one of them is approved so that one will not have keep this one button.
parasharrajat commented 2 months ago

@daledah Do you mind updating your proposal based on https://github.com/Expensify/App/issues/46372#issuecomment-2277599713

parasharrajat commented 2 months ago

We can't remove the Keep this one button completely as there might be more than one unapproved expense and the user might want to keep one of them.

daledah commented 2 months ago

A minor improvement to point 2 would be that the keep this one will be hidden on paid or approved expenses only.

@parasharrajat You mean the paid or approved expenses should not be included in this page, right?

melvin-bot[bot] commented 2 months ago

@parasharrajat @techievivek @zanyrenney 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!

parasharrajat commented 2 months ago

No I mean the Keep this one button should be hidden for these.

daledah commented 2 months ago

@parasharrajat I updated proposal

techievivek commented 2 months ago

A minor improvement to point 2 would be that the keep this one will be hidden on paid or approved expenses only. Imagine there are 3 duplicates on the review screen and one of them is approved so that one will not have keep this one button.

@parasharrajat Make sense 👍

melvin-bot[bot] commented 2 months ago

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

parasharrajat commented 2 months ago

@daledah Why are we filtering paid expenses? What will happen when we will resolve duplicates?

daledah commented 2 months ago

We need to filtering paid expenses because if we sent it to BE, it will returns error that mentioned in OP:

  1. Note that the process fails and Review duplicates button reappears
JmillsExpensify commented 2 months ago

We can't filter paid expenses though, because this is the core feature (preventing expenses that have been paid already from being paid twice). If we are ignoring the paid expenses on Keep all and then not showing Keep this one on paid expenses, then we don't need a backend change, right?

parasharrajat commented 2 months ago

@daledah Could you please answer https://github.com/Expensify/App/issues/46372#issuecomment-2284614448?

daledah commented 2 months ago

If we are ignoring the paid expenses on Keep all and then not showing Keep this one on paid expenses, then we don't need a backend change, right

Yes

parasharrajat commented 2 months ago

But we want to show the paid expenses on the review page. Will your proposal filter them out on the screen?

daledah commented 2 months ago

But we want to show the paid expenses on the review page. Will your proposal filter them out on the screen?

No. My proposal does not filter the transactions in Review page: https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/pages/TransactionDuplicate/Review.tsx#L36

I just filter in: https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/components/ReportActionItem/MoneyRequestPreview/MoneyRequestPreviewContent.tsx#L282 https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/libs/TransactionUtils.ts#L853 https://github.com/Expensify/App/blob/08bb00b2dc515bb1da2e70f9b9eae9d522c3be50/src/libs/TransactionUtils.ts#L973