search

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.87k forks source link

[HOLD for payment 2024-10-14] [$250] [Dupe detection] Use new ResolveDuplicates when approver is resolving duplicates #48416

Closed pecanoro closed 2 weeks ago

pecanoro commented 2 months ago

Part of the Duplicate Detection project

Main issue: https://github.com/Expensify/Expensify/issues/411008 Project: Dupe detection

Feature Description

image

Right now, we always call MergeTransactions when resolving duplicates and click Confirm at the last page of resolving duplicates. However, we need to call a new command called ResolveDuplicates when an approver or admin (basically anyone but the submitter) is the person resolving the duplicates. The behaviour (so we can create the proper optimistic actions) should be as follows:

Main issue: https://github.com/Expensify/Expensify/issues/411008

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011781060ec2a6c428
  • Upwork Job ID: 1830626997874011636
  • Last Price Increase: 2024-09-02
  • Automatic offers:
    • nkdengineer | Contributor | 103792699
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @parasharrajat
pecanoro commented 2 months ago

@parasharrajat I am assigning you as the C+ in this issue since you reviewed the main ones.

parasharrajat commented 2 months ago

Sure

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @sonialiap (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] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

Current assignee @parasharrajat is eligible for the External assigner, not assigning anyone new.

nkdengineer commented 2 months ago

Proposal

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

Add a new command for admin/approver when resolving the duplicate transaction from submitter

What is the root cause of that problem?

We're always calling mergeDuplicates function without checking the current user is admin/approver or not

https://github.com/Expensify/App/blob/e60b8210a1f5a2471d8aa9d84d0e461774b791d6/src/pages/TransactionDuplicate/Confirmation.tsx#L45-L48

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

  1. To check the user is admin/approver we can use the same way we do here.

https://github.com/Expensify/App/blob/e60b8210a1f5a2471d8aa9d84d0e461774b791d6/src/libs/ReportUtils.ts#L3074-L3079

  1. Create a new function resolveDuplicates that will call ResolveDuplicates comman with the param as described here. It will be almost the same with mergeDuplicates, the only difference with mergeDuplicates is

It won't delete the rest of the transactions in transactionIDList but it will HOLD them by adding the hold NVP to it and will add the corresponding HOLD actions to each of them.

It will add a DISMISSEDVIOLATION action to the transactionIDToKeep

It will dismiss the violation by adding the corresponding transaction NVP for all transactions.

What alternative solutions did you explore? (Optional)

parasharrajat commented 2 months ago

The proposal sounds good @nkdengineer sounds good to me.

:ribbon: :eyes: :ribbon: C+ reviewed

melvin-bot[bot] commented 2 months ago

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

pecanoro commented 2 months ago

Assigning @nkdengineer!

melvin-bot[bot] commented 2 months ago

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

nkdengineer commented 2 months ago

Will raise the PR tomorrow.

nkdengineer commented 1 month ago

@pecanoro In the ResolveDuplicates API, what is the structure of reportActionIDList params?

pecanoro commented 1 month ago

In the ResolveDuplicates API, what is the structure of reportActionIDList params?

A comma-separated list of report actions

nkdengineer commented 1 month ago

A comma-separated list of report actions

@pecanoro Can you give an example? Do we need to add reportID in data?

pecanoro commented 1 month ago

@pecanoro Can you give an example?

Imagine we have 3 duplicates with transactionIDs 111, 222 and 333. We want to keep 111 and hold the other two. Then:

'transactionIDToKeep' => '111',
'transactionIDList' => '222,333',

Do we need to add reportID in data?

Nope, no reportID since they are not changing reports

nkdengineer commented 1 month ago

@pecanoro I mean I need an example for reportActionIDList param.

pecanoro commented 1 month ago

Ahh, the same, another comma-separated list, same as DismissViolations. The reportActionIDList contains the optimistic report actions for the HOLD actions. Then optimisticReportActionID should be for the dismissViolation action.

nkdengineer commented 1 month ago

Got it, So the reportActionIDList is the same order with the transactionIDList right (example the first reportActionID is the hold action of the first transaction in transactionIDList)?

actually for the normal hold reason flow it will have two report actions, one is the hold message, one is the reason. Should we do the same in this case or we will add only the first action or add both actions?

pecanoro commented 1 month ago

Got it, So the reportActionIDList is the same order with the transactionIDList right (example the first reportActionID is the hold action of the first transaction in transactionIDList)?

Yes!

actually for the normal hold reason flow it will have two report actions, one is the hold message, one is the reason. Should we do the same in this case or we will add only the first action or add both actions?

No need to add a reason for this one, it will only show that is was held.

dylanexpensify commented 1 month ago

@nkdengineer when can we expect the PR raised? ty!

nkdengineer commented 1 month ago

Will open this today

dylanexpensify commented 1 month ago

amazing, ty!

nkdengineer commented 1 month ago

@pecanoro When I call ResolveDuplicates, BE returns the error like this video. Here is the parameter that I called this API. Am I missing something?

{
    "amount": -200,
    "currency": "VND",
    "created": "2024-09-09",
    "transactionIDList": [
        "4002616464040896678"
    ],
    "billable": false,
    "reimbursable": true,
    "category": "",
    "tag": "",
    "merchant": "ssd",
    "comment": "",
    "transactionIDToKeep": "5332969134080823220",
    "reportActionIDList": [
        "637476526581947356"
    ],
    "optimisticReportActionID": "3766068727586466325"
}

https://github.com/user-attachments/assets/d9ca09c3-9441-41c9-9465-145f28816be9

pecanoro commented 1 month ago

Oh, it's a BE problem, I j just checked the logs. Let me create a PR to fix it

pecanoro commented 1 month ago

The PR is merged, we are waiting for a deploy and then you will be able to use the endpoint in staging again. I renamed transactionIDToKeep for just transactionID

dylanexpensify commented 1 month ago

@nkdengineer PR is deployed to prod now!

nkdengineer commented 1 month ago

@parasharrajat The PR is here.

parasharrajat commented 1 month ago

it will HOLD them by adding the hold NVP to it

What do you mean by NVP @pecanoro?

nkdengineer commented 1 month ago

@pecanoro While testing I found another BE bug, the dismiss violation action isn't stored from BE side.

pecanoro commented 1 month ago

What do you mean by NVP @pecanoro?

Ah sorry, when the transaction is put on HOLD, we add a value to the transaction namevaluepairs in the comment.

pecanoro commented 1 month ago

@pecanoro While testing I found another BE bug, the dismiss violation action isn't stored from BE side.

Are you sure? It is added for the transaction we want to keep, not for the others.

nkdengineer commented 1 month ago

Yes, I tested and after reset cache this action is removed since BE doesn't return this action. Also the resolve duplicate API doesn't return dismiss violation action data.

pecanoro commented 1 month ago

Let me double-check manually, but it's pretty odd since I even wrote automated tests for this and they are passing

pecanoro commented 1 month ago

@nkdengineer Can you paste here what the BE is returning for you?

nkdengineer commented 1 month ago 
{
    "accountID": 18311825,
    "accounts": {
        "18311825": {
            "lastUpdateID": 1775760902,
            "previousUpdateID": 1775614249,
            "updateIDs": [
                1775760897,
                1775760898,
                1775760899,
                1775760900,
                1775760901,
                1775760902
            ]
        }
    },
    "authToken": "ED538D6882FC6B872FD1E38C8B75EC3ED4799DAF2809D40258D5A38DF3F448972D57954AF21C6EF04D9DBA533734CD6FC5B8908331F9904CF87AF73E8B286E9DDC523F8C2153DB499A0D6E433B2BD2E21CB71155B10A7F6D64826D4ACD1CA0443ECF354E9006107CD6DAC056448043E0FBDE3C32367A157108E87CBB53EDBFFD03E891BCEB5DA77E8EBEF24B5AF0D7E9FD9A757BDA391A6568E463678C8EF1BF1E55E96771BEEE6228EF37F1B8FF3BE280B9E5C6B2235E5FAD8E30F652EAA244C06EFE2E8B18BFB3CB8DA94286CC32C74D0B5F03C003CD78D2BB8A9D02A9478C89F64E9674B229779DDC5DEA5EED7B3DF5A513373D224C022CE70012AEAEA03F4BF4CF18ED5EBE8DF44F7A53F64C623731EF5689A91E9FB3E061CDF663B373ED10CECB4758B6D759E25715BB2EF0BE6C729E7A3928CF1F89005D8434D3BD4556A86E95D218CB4353E3B6E6A20E66F7775725A89AEB459453192F36B84EA2742A",
    "email": "nkdengineer+s102@outlook.com",
    "fetchableOnyxUpdates": [
        "1775760897",
        "1775760898",
        "1775760899",
        "1775760900",
        "1775760901",
        "1775760902"
    ],
    "onyxUpdates": {
        "1775760897": [
            {
                "key": "transactions_398480206897163307",
                "onyxMethod": "merge",
                "value": {
                    "comment": {
                        "dismissedViolations": {
                            "duplicatedTransaction": {
                                "nkdengineer+s102@outlook.com": 1726161878570510
                            }
                        }
                    }
                }
            }
        ],
        "1775760898": [
            {
                "key": "transactions_1367302367436179941",
                "onyxMethod": "merge",
                "value": {
                    "comment": {
                        "comment": "",
                        "dismissedViolations": {
                            "duplicatedTransaction": {
                                "nkdengineer+s102@outlook.com": 1726161878570610
                            }
                        }
                    }
                }
            }
        ],
        "1775760899": [
            {
                "key": "report_1113086820473870",
                "onyxMethod": "merge",
                "value": {
                    "participants": {
                        "18301280": {
                            "hidden": true,
                            "notificationPreference": "hidden"
                        },
                        "18308128": {
                            "hidden": true,
                            "notificationPreference": "hidden"
                        },
                        "18311825": {
                            "hidden": true,
                            "notificationPreference": "hidden"
                        }
                    }
                }
            }
        ],
        "1775760900": [
            {
                "key": "report_1113086820473870",
                "onyxMethod": "merge",
                "value": {
                    "chatType": "",
                    "description": "",
                    "lastActorAccountID": 18311825,
                    "lastMessageText": "held this expense with the comment: ",
                    "lastVisibleActionCreated": "2024-09-12 17:24:38.571",
                    "managerID": null,
                    "ownerAccountID": 0,
                    "parentReportActionID": "2951370456346266262",
                    "parentReportID": "8755324512976856",
                    "policyID": "2D62A4DBCE91AD7E",
                    "reportID": "1113086820473870",
                    "reportName": "Chat Report",
                    "state": "OPEN",
                    "stateNum": 0,
                    "statusNum": 0,
                    "type": "chat",
                    "visibility": null
                }
            },
            {
                "key": "reportActions_1113086820473870",
                "onyxMethod": "merge",
                "value": {
                    "2272851721205218606": {
                        "actionName": "HOLD",
                        "actorAccountID": 18311825,
                        "avatar": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_18.png",
                        "created": "2024-09-12 17:24:38.571",
                        "lastModified": "2024-09-12 17:24:38.571",
                        "message": [
                            {
                                "html": "",
                                "text": "",
                                "type": "COMMENT",
                                "whisperedTo": []
                            }
                        ],
                        "originalMessage": {
                            "accountID": 18311825,
                            "date": "2024-09-12 17:24:38",
                            "isDuplicate": "true",
                            "lastModified": "2024-09-12 17:24:38.571",
                            "message": ""
                        },
                        "person": [
                            {
                                "style": "strong",
                                "text": "nkd102",
                                "type": "TEXT"
                            }
                        ],
                        "reportActionID": "2272851721205218606",
                        "shouldShow": true
                    }
                }
            }
        ],
        "1775760901": [
            {
                "key": "personalDetailsList",
                "onyxMethod": "merge",
                "value": {
                    "18311825": {
                        "accountID": 18311825,
                        "avatar": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_18.png",
                        "displayName": "nkd102",
                        "firstName": "nkd102",
                        "lastName": "",
                        "login": "nkdengineer+s102@outlook.com",
                        "phoneNumber": "",
                        "pronouns": "",
                        "status": null,
                        "timezone": {
                            "automatic": true,
                            "selected": "Asia/Ho_Chi_Minh"
                        },
                        "validated": true
                    }
                }
            }
        ],
        "1775760902": [
            {
                "key": "transactions_1367302367436179941",
                "onyxMethod": "merge",
                "value": {
                    "comment": {
                        "hold": "2272851721205218606"
                    }
                }
            }
        ]
    },
    "httpCode": 200,
    "jsonCode": 200,
    "authResponseMessage": "200 OK",
    "requestID": "8c21979b5e5e1fbc-HKG",
    "onyxData": [
        {
            "key": "transactions_398480206897163307",
            "onyxMethod": "merge",
            "value": {
                "comment": {
                    "dismissedViolations": {
                        "duplicatedTransaction": {
                            "nkdengineer+s102@outlook.com": 1726161878570510
                        }
                    }
                }
            }
        },
        {
            "key": "transactions_1367302367436179941",
            "onyxMethod": "merge",
            "value": {
                "comment": {
                    "comment": "",
                    "dismissedViolations": {
                        "duplicatedTransaction": {
                            "nkdengineer+s102@outlook.com": 1726161878570610
                        }
                    }
                }
            }
        },
        {
            "key": "report_1113086820473870",
            "onyxMethod": "merge",
            "value": {
                "participants": {
                    "18301280": {
                        "hidden": true,
                        "notificationPreference": "hidden"
                    },
                    "18308128": {
                        "hidden": true,
                        "notificationPreference": "hidden"
                    },
                    "18311825": {
                        "hidden": true,
                        "notificationPreference": "hidden"
                    }
                }
            }
        },
        {
            "key": "report_1113086820473870",
            "onyxMethod": "merge",
            "value": {
                "chatType": "",
                "description": "",
                "lastActorAccountID": 18311825,
                "lastMessageText": "held this expense with the comment: ",
                "lastVisibleActionCreated": "2024-09-12 17:24:38.571",
                "managerID": null,
                "ownerAccountID": 0,
                "parentReportActionID": "2951370456346266262",
                "parentReportID": "8755324512976856",
                "policyID": "2D62A4DBCE91AD7E",
                "reportID": "1113086820473870",
                "reportName": "Chat Report",
                "state": "OPEN",
                "stateNum": 0,
                "statusNum": 0,
                "type": "chat",
                "visibility": null
            }
        },
        {
            "key": "reportActions_1113086820473870",
            "onyxMethod": "merge",
            "value": {
                "2272851721205218606": {
                    "actionName": "HOLD",
                    "actorAccountID": 18311825,
                    "avatar": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_18.png",
                    "created": "2024-09-12 17:24:38.571",
                    "lastModified": "2024-09-12 17:24:38.571",
                    "message": [
                        {
                            "html": "",
                            "text": "",
                            "type": "COMMENT",
                            "whisperedTo": []
                        }
                    ],
                    "originalMessage": {
                        "accountID": 18311825,
                        "date": "2024-09-12 17:24:38",
                        "isDuplicate": "true",
                        "lastModified": "2024-09-12 17:24:38.571",
                        "message": ""
                    },
                    "person": [
                        {
                            "style": "strong",
                            "text": "nkd102",
                            "type": "TEXT"
                        }
                    ],
                    "reportActionID": "2272851721205218606",
                    "shouldShow": true
                }
            }
        },
        {
            "key": "personalDetailsList",
            "onyxMethod": "merge",
            "value": {
                "18311825": {
                    "accountID": 18311825,
                    "avatar": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_18.png",
                    "displayName": "nkd102",
                    "firstName": "nkd102",
                    "lastName": "",
                    "login": "nkdengineer+s102@outlook.com",
                    "phoneNumber": "",
                    "pronouns": "",
                    "status": null,
                    "timezone": {
                        "automatic": true,
                        "selected": "Asia/Ho_Chi_Minh"
                    },
                    "validated": true
                }
            }
        },
        {
            "key": "transactions_1367302367436179941",
            "onyxMethod": "merge",
            "value": {
                "comment": {
                    "hold": "2272851721205218606"
                }
            }
        }
    ],
    "previousUpdateID": 1775614249,
    "lastUpdateID": 1775760902
}
nkdengineer commented 1 month ago

@pecanoro The response here.

nkdengineer commented 1 month ago

The payload here.

Screenshot 2024-09-13 at 00 26 11
pecanoro commented 1 month ago

There is something off, also because both hold actions are returned for the same report

pecanoro commented 1 month ago

Ah nvm, forget my previous comment, you were resolving duplicates with only two transactions. I am going to check the database

pecanoro commented 1 month ago

Ah I found the problem, another parameter name that got cleared because of sanitization. I am going to rename it to something better either way. I will create a PR in the back-end

pecanoro commented 1 month ago

@nkdengineer BE fix is on staging, you can test it in the PR now! New parameter is called dismissedViolationReportActionID instead of optimisticReportActionID

parasharrajat commented 1 month ago

@pecanoro Does this change affect keep All functionality as well? Do we need to change that too?

pecanoro commented 1 month ago

@parasharrajat No, it should not change it

parasharrajat commented 1 month ago

Got it. Thanks.

parasharrajat commented 1 month ago

Note for future: I noticed that when admin resolves a duplicate, the transaction is no more duplicate for admin but same transaction still shown duplicate for the original user who requested. I confirmed that this is expected https://github.com/Expensify/App/pull/48522#issuecomment-2364201769

nkdengineer commented 1 month ago

@parasharrajat The original PR was reverted so I created a new one here to fix another bug https://github.com/Expensify/App/issues/49900

melvin-bot[bot] commented 3 weeks ago

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

melvin-bot[bot] commented 3 weeks ago

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

If no regressions arise, payment will be issued on 2024-10-14. :confetti_ball:

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

melvin-bot[bot] commented 3 weeks ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

sonialiap commented 2 weeks ago

Payment summary: