Skip to content

fix: no RBR when deleting distance rates #57767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aldo-expensify merged 4 commits into from
Mar 5, 2025
Merged

fix: no RBR when deleting distance rates #57767

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b070148
fix: no RBR when deleting distance rates
daledah Mar 4, 2025
27de0ff
Merge branch 'main' into fix/57579
daledah Mar 5, 2025
4529623
fix: show #admin room when invite user through actionable whisper
daledah Mar 5, 2025
e792ff9
Revert "fix: show #admin room when invite user through actionable whi…
daledah Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion src/libs/actions/Policy/DistanceRate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {getDistanceRateCustomUnit, goBackWhenEnableFeature, removePendingFieldsF
import * as ReportUtils from '@libs/ReportUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Policy, Report} from '@src/types/onyx';
import type {Policy, Report, Transaction, TransactionViolation} from '@src/types/onyx';
import type {ErrorFields} from '@src/types/onyx/OnyxCommon';
import type {Attributes, CustomUnit, Rate} from '@src/types/onyx/Policy';
import type {OnyxData} from '@src/types/onyx/Request';
Expand Down Expand Up @@ -62,6 +62,29 @@ Onyx.connect({
},
});

let allTransactions: NonNullable<OnyxCollection<Transaction>> = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.TRANSACTION,
waitForCollectionCallback: true,
callback: (value) => {
if (!value) {
allTransactions = {};
return;
}

allTransactions = value;
},
});

let transactionViolations: OnyxCollection<TransactionViolation[]>;
Onyx.connect({
key: ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS,
waitForCollectionCallback: true,
callback: (value) => {
transactionViolations = value;
},
});

/**
* Takes array of customUnitRates and removes pendingFields and errorFields from each rate - we don't want to send those via API
*/
Expand Down Expand Up @@ -550,6 +573,36 @@ function deletePolicyDistanceRates(policyID: string, customUnit: CustomUnit, rat
},
];

const transactions = Object.values(allTransactions ?? {}).filter((transaction) => transaction?.comment?.customUnit?.customUnitID === customUnit.customUnitID);
Copy link
Contributor

@ikevin127 ikevin127 Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line logic is setting violations for all transactions with the specific customUnitID instead of checking for the customUnitRateID which led to this issue:

which we fixed by filtering for transactions with customUnitRateID as the deleted one.

const optimisticTransactionsViolations: OnyxUpdate[] = [];
Comment on lines +576 to +577
Copy link
Contributor

@aldo-expensify aldo-expensify Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that these transaction actually belong to reports that are associated to the policy with policyID

Copy link
Contributor

@aldo-expensify aldo-expensify Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should check for the report state/status too. I don't think we want to add violations we can't resolve to reports have already been payed for example.

Copy link
Contributor Author

@daledah daledah Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that these transaction actually belong to reports that are associated to the policy with policyID

We filter transactions using customUnit.customUnitID so it will filter out other policy transactions as well.

We probably should check for the report state/status too. I don't think we want to add violations we can't resolve to reports have already been payed for example.

Violation also appear on paid expense

Screenshot 2025-03-05 at 13 17 14

Copy link
Contributor

@paultsimura paultsimura Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm: we add a violation, but do not show the RBR in LHN for paid requests:

image image

Copy link
Contributor

@aldo-expensify aldo-expensify Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that these transaction actually belong to reports that are associated to the policy with policyID

We filter transactions using customUnit.customUnitID so it will filter out other policy transactions as well.

customUnitID are not necessarily unique across workspaces, but I see now that we generate them randomly with a shape like 52F16411C502C so it is VERY unlikely to have collisions. Considering that, I think we can ignore checking the policyID.

We probably should check for the report state/status too. I don't think we want to add violations we can't resolve to reports have already been payed for example.

Violation also appear on paid expense

Ahh nice, I didn't know :)

I can confirm: we add a violation, but do not show the RBR in LHN for paid requests:

Thanks for confirming @paultsimura !


transactions.forEach((transaction) => {
const currentTransactionViolations = transactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction?.transactionID}`] ?? [];
if (currentTransactionViolations.some((violation) => violation.name === CONST.VIOLATIONS.CUSTOM_UNIT_OUT_OF_POLICY)) {
return;
}
optimisticTransactionsViolations.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction?.transactionID}`,
value: [
...currentTransactionViolations,
{
type: CONST.VIOLATION_TYPES.VIOLATION,
name: CONST.VIOLATIONS.CUSTOM_UNIT_OUT_OF_POLICY,
showInReview: true,
},
],
});
});

const failureTransactionsViolations: OnyxUpdate[] = transactions.map((transaction) => {
const currentTransactionViolations = transactionViolations?.[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction?.transactionID}`];
return {onyxMethod: Onyx.METHOD.MERGE, key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction?.transactionID}`, value: currentTransactionViolations};
});

optimisticData.push(...optimisticTransactionsViolations);
failureData.push(...failureTransactionsViolations);

const params: DeletePolicyDistanceRatesParams = {
policyID,
customUnitID: customUnit.customUnitID,
Expand Down