[$250] Review duplicates - Review duplicates button doesn’t disappear after deleting one expense #55138
Comments
IuliiaHerets
added
Daily
|
Triggered auto assignment to @anmurali ( |
|
BE issue.
so when the |
|
🚨 Edited by proposal-police: This proposal was edited at 2025-01-19 08:14:29 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Review duplicates button doesn’t disappear after deleting one expense What is the root cause of that problem?We are not deleting the transaction violations of the duplicate and only delete the transaction's violations. Lines 6117 to 6121 in c585eb9 Hence, the backend merges the valid transaction violations after What changes do you think we should make in order to solve the problem?Let's optimistically, delete the duplicate transaction violations over here. if (transactionViolations) {
TransactionUtils.removeSettledAndApprovedTransactions(
transactionViolations.find((violation) => violation?.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION)?.data?.duplicates ?? [],
).forEach((duplicateID) => {
const duplicateTransactionsViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${duplicateID}`];
if (!duplicateTransactionsViolations) {
return;
}
const dulipcateViolation = duplicateTransactionsViolations.find((violation: OnyxTypes.TransactionViolation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION);
if (!dulipcateViolation?.data?.duplicates) {
return;
}
const duplicateTransactionIDs = dulipcateViolation.data.duplicates.filter((duplicateTransactionID) => duplicateTransactionID !== transactionID);
optimisticData.push({
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${duplicateID}`,
value:
duplicateTransactionIDs.length === 0
? duplicateTransactionsViolations.filter((violation: OnyxTypes.TransactionViolation) => violation.name !== CONST.VIOLATIONS.DUPLICATED_TRANSACTION)
: [
...duplicateTransactionsViolations.filter((violation: OnyxTypes.TransactionViolation) => violation.name !== CONST.VIOLATIONS.DUPLICATED_TRANSACTION),
{
...dulipcateViolation,
data: {
...dulipcateViolation.data,
duplicates: duplicateTransactionIDs,
},
},
],
});
failureData.push({
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${duplicateID}`,
value: duplicateTransactionsViolations,
});
});
}What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?Currently, in the Hence, we should create the test transactions and then test the optimistic output of the |
|
@anmurali Whoops! This issue is 2 days overdue. Let's get this updated quick! |
anmurali
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~021880380172206780429 |
melvin-bot
bot
added
the
Help Wanted
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @suneox ( |
|
@Tony-MK Thank you for your proposal. However, we need to build the optimistic/failure data for the |
|
@suneox bump on Tony's revised proposal? |
@Tony-MK Your update still doesn't resolve the issue with the related duplicate transactions.
|
|
🚨 Edited by proposal-police: This proposal was edited at 2025-01-23 02:57:57 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Review duplicates button doesn’t disappear after deleting one expense What is the root cause of that problem?We only check for duplicates by the violation name. App/src/libs/TransactionUtils/index.ts Line 895 in d3735e1 And when we Describe the image above:
What changes do you think we should make in order to solve the problem?To resolve this issue, the condition for App/src/libs/TransactionUtils/index.ts Line 895 in d3735e1 Update to: const duplicates =
allTransactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`]?.find((violation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION)?.data
?.duplicates ?? [];
const hasDuplicatedViolation = !!allTransactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`]?.some(
(violation: TransactionViolation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION && !!duplicates.map((item) => getTransaction(item)).filter((item) => !!item).length,
);Or, if we want to return early, then: App/src/libs/TransactionUtils/index.ts Line 894 in d3735e1 const duplicates =
allTransactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`]?.find((violation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION)?.data
?.duplicates ?? [];
const transactions = duplicates.map((item) => getTransaction(item)).filter((item) => !!item);
if (!transactions.length) {
return false;
}What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?We need to update the isDuplicate test to return false if the transaction does not exist. 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. We can fix this issue by removing TRANSACTION_VIOLATIONS if it is the last duplicate, something like this: Line 6536 in 4d75438 add: const duplicates =
allTransactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`]?.find((violation) => violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION)?.data
?.duplicates ?? [];
const transactionViolationsDuplicates = duplicates.map((item) => {
const violations = getTransactionViolations(item, allTransactionViolations);
// Filter and update violations
const filteredViolations =
violations?.map((violation) => {
if (violation.name === CONST.VIOLATIONS.DUPLICATED_TRANSACTION) {
const updatedDuplicates = violation.data?.duplicates?.filter((id) => id !== transactionID);
if (!updatedDuplicates?.length) {
return null;
}
return {
...violation,
data: {
...violation.data,
duplicates: updatedDuplicates,
},
};
}
return violation;
}) ?? [];
return {transactionID: item, data: filteredViolations.filter((item) => !!item)};
});
transactionViolationsDuplicates.forEach((item) => {
optimisticData.push({
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${item.transactionID}`,
value: item.data.length ? item.data : null,
});
});We should also remove it in case of failure: Line 6738 in 4d75438 add: transactionViolationsDuplicates.forEach((item) => {
failureData.push({
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${item.transactionID}`,
value: item.data.length ? item.data : null,
});
});POCScreen.Recording.2025-01-23.at.09.56.11.mp4Test branch |
Proposal updated
@suneox Could you please review my proposal when you have time? Thank you |
|
Thank you for all the updates. The solutions from @huult and @Tony-MK still work when handling optimistic data in the case of removing expenses with multiple duplicated transaction violations. However, the alternative solution from @huult, which handles failureData the same as optimistic data, is incorrect. Therefore, we can proceed with @Tony-MK proposal, and the code lint from selected proposal can be performed during the code review. 🎀 👀 🎀 C+ reviewed |
|
Triggered auto assignment to @Julesssss, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
melvin-bot
bot
removed
the
Help Wanted
|
📣 @suneox 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
|
📣 @Tony-MK 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
|
@Julesssss @suneox @anmurali @Tony-MK 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
added
Reviewing
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.84-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
Issue reported by: Applause Internal Team
Device used: Mac 15.2
App Component: Chat Report View
Action Performed:
Expected Result:
The review duplicates button should disappear after deleting one of the expenses
Actual Result:
The review duplicates button doesn’t disappear after deleting one of the expenses
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6712538_1736704522689.Recording__400.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @suneoxThe text was updated successfully, but these errors were encountered: