Skip to content
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.

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

Client-side violations for money request updates #34402

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
a551927
Add optimistic violations to money request edits
lindboe Jan 10, 2024
5ea3000
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Jan 10, 2024
56dd74d
Lint fix
lindboe Jan 10, 2024
fe4b178
Fix bugs
lindboe Jan 10, 2024
a189970
Fix data passing
lindboe Jan 10, 2024
69ad695
Temp fix for tag issue
lindboe Jan 11, 2024
41e87ca
Use full policyTags data as ViolationsUtils input
lindboe Jan 12, 2024
a09e4ac
getViolationsOnyxData requires new transaction data but previous tran…
lindboe Jan 12, 2024
72cfa45
Improve types
lindboe Jan 12, 2024
8ff7c69
Fix case of passing empty object to policyTags
lindboe Jan 12, 2024
f2942ea
We should only run getViolationsOnyxData if there's a policy
lindboe Jan 12, 2024
37bbacc
Feedback: Don't use Maybe type
lindboe Jan 15, 2024
0a3e818
Ensure an array is always passed to getViolationsOnyxData
lindboe Jan 15, 2024
d211324
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Jan 15, 2024
ede3bbe
More prettier changes
lindboe Jan 17, 2024
87d2f07
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Jan 19, 2024
04fcb21
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Jan 23, 2024
d87ded8
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Jan 24, 2024
371e166
Support client-side violations in new commands
lindboe Jan 25, 2024
38ffff6
Pass policy args to all commands
lindboe Jan 26, 2024
210cfea
Make currentTransactionViolations always an empty array if falsy
lindboe Jan 31, 2024
269b7df
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Jan 31, 2024
699dc34
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Feb 5, 2024
938c6bc
Manage MoneyRequestView type error
lindboe Feb 5, 2024
84267ea
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Feb 5, 2024
b8248ef
Remove obsolete ts-expect-error
lindboe Feb 5, 2024
32d6617
First stab at fixing typing
lindboe Feb 6, 2024
28a1f95
WIP update
lindboe Feb 6, 2024
3493040
editMoneyRequest is no longer used, delete
lindboe Feb 6, 2024
86ea25f
Finish typing IOU
lindboe Feb 6, 2024
4197a85
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Feb 6, 2024
352f5b1
Expect error in MoneyRequestView
lindboe Feb 6, 2024
934bab4
Random lint change
lindboe Feb 6, 2024
fbbcdae
Revert "editMoneyRequest is no longer used, delete"
lindboe Feb 6, 2024
1eebb39
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Feb 7, 2024
30a2f24
Update command in new description component
lindboe Feb 7, 2024
9cbb446
Merge remote-tracking branch 'upstream/main' into lindboe/violations/…
lindboe Feb 9, 2024
d528580
Apply suggestions from code review
lindboe Feb 9, 2024
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
26 changes: 14 additions & 12 deletions src/libs/ViolationsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import reject from 'lodash/reject';
import Onyx from 'react-native-onyx';
import type {TranslationPaths} from '@src/languages/types';
import ONYXKEYS from '@src/ONYXKEYS';
import type {PolicyCategories, PolicyTags, Transaction, TransactionViolation} from '@src/types/onyx';
import type {MaybePolicyTagList, PolicyCategories, Transaction, TransactionViolation} from '@src/types/onyx';
import type {Phrase, PhraseParameters} from './Localize';

const ViolationsUtils = {
Expand All @@ -11,10 +11,10 @@ const ViolationsUtils = {
* violations.
*/
getViolationsOnyxData(
transaction: Transaction,
updatedTransaction: Transaction,
transactionViolations: TransactionViolation[],
policyRequiresTags: boolean,
policyTags: PolicyTags,
policyTagList: MaybePolicyTagList,
lindboe marked this conversation as resolved.
Show resolved Hide resolved
policyRequiresCategories: boolean,
policyCategories: PolicyCategories,
): {
Expand All @@ -27,15 +27,15 @@ const ViolationsUtils = {
if (policyRequiresCategories) {
const hasCategoryOutOfPolicyViolation = transactionViolations.some((violation) => violation.name === 'categoryOutOfPolicy');
const hasMissingCategoryViolation = transactionViolations.some((violation) => violation.name === 'missingCategory');
const isCategoryInPolicy = Boolean(policyCategories[transaction.category]?.enabled);
const isCategoryInPolicy = Boolean(policyCategories[updatedTransaction.category]?.enabled);

// Add 'categoryOutOfPolicy' violation if category is not in policy
if (!hasCategoryOutOfPolicyViolation && transaction.category && !isCategoryInPolicy) {
if (!hasCategoryOutOfPolicyViolation && updatedTransaction.category && !isCategoryInPolicy) {
newTransactionViolations.push({name: 'categoryOutOfPolicy', type: 'violation', userMessage: ''});
}

// Remove 'categoryOutOfPolicy' violation if category is in policy
if (hasCategoryOutOfPolicyViolation && transaction.category && isCategoryInPolicy) {
if (hasCategoryOutOfPolicyViolation && updatedTransaction.category && isCategoryInPolicy) {
newTransactionViolations = reject(newTransactionViolations, {name: 'categoryOutOfPolicy'});
}

Expand All @@ -45,23 +45,25 @@ const ViolationsUtils = {
}

// Add 'missingCategory' violation if category is required and not set
if (!hasMissingCategoryViolation && policyRequiresCategories && !transaction.category) {
if (!hasMissingCategoryViolation && policyRequiresCategories && !updatedTransaction.category) {
newTransactionViolations.push({name: 'missingCategory', type: 'violation', userMessage: ''});
}
}

if (policyRequiresTags) {
const policyTagListName = Object.keys(policyTagList)[0];
const policyTags = policyTagList[policyTagListName]?.tags;
const hasTagOutOfPolicyViolation = transactionViolations.some((violation) => violation.name === 'tagOutOfPolicy');
const hasMissingTagViolation = transactionViolations.some((violation) => violation.name === 'missingTag');
const isTagInPolicy = Boolean(policyTags[transaction.tag]?.enabled);
const isTagInPolicy = Boolean(policyTags?.[updatedTransaction.tag]?.enabled);

// Add 'tagOutOfPolicy' violation if tag is not in policy
if (!hasTagOutOfPolicyViolation && transaction.tag && !isTagInPolicy) {
if (!hasTagOutOfPolicyViolation && updatedTransaction.tag && !isTagInPolicy) {
newTransactionViolations.push({name: 'tagOutOfPolicy', type: 'violation', userMessage: ''});
}

// Remove 'tagOutOfPolicy' violation if tag is in policy
if (hasTagOutOfPolicyViolation && transaction.tag && isTagInPolicy) {
if (hasTagOutOfPolicyViolation && updatedTransaction.tag && isTagInPolicy) {
newTransactionViolations = reject(newTransactionViolations, {name: 'tagOutOfPolicy'});
}

Expand All @@ -71,14 +73,14 @@ const ViolationsUtils = {
}

// Add 'missingTag violation' if tag is required and not set
if (!hasMissingTagViolation && !transaction.tag && policyRequiresTags) {
if (!hasMissingTagViolation && !updatedTransaction.tag && policyRequiresTags) {
newTransactionViolations.push({name: 'missingTag', type: 'violation', userMessage: ''});
}
}

return {
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${updatedTransaction.transactionID}`,
value: newTransactionViolations,
};
},
Expand Down
93 changes: 76 additions & 17 deletions src/libs/actions/IOU.js
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ function buildOnyxDataForMoneyRequest(
optimisticPolicyRecentlyUsedTags,
isNewChatReport,
isNewIOUReport,
policy,
policy = {},
policyTags,
policyCategories,
hasOutstandingChildRequest = false,
Expand Down Expand Up @@ -918,13 +918,16 @@ function createDistanceRequest(report, participant, comment, created, category,
* @param {String} transactionThreadReportID
* @param {Object} transactionChanges
* @param {String} [transactionChanges.created] Present when updated the date field
* @param {Object} policy - May be undefined, an empty object, or an object matching the Policy type (src/types/onyx/Policy.ts)
* @param {Array} policyTags
* @param {Array} policyCategories
* @param {Boolean} onlyIncludeChangedFields
* When 'true', then the returned params will only include the transaction details for the fields that were changed.
* When `false`, then the returned params will include all the transaction details, regardless of which fields were changed.
* This setting is necessary while the UpdateDistanceRequest API is refactored to be fully 1:1:1 in https://github.com/Expensify/App/issues/28358
* @returns {object}
*/
function getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, onlyIncludeChangedFields) {
function getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, policy = {}, policyTags, policyCategories, onlyIncludeChangedFields) {
Copy link
Member

Choose a reason for hiding this comment

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

We should update passed params wherever this function is called. Currently a few places it is not updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I did do this originally, but the refactor added some I didn't know about. Will fix.

const optimisticData = [];
const successData = [];
const failureData = [];
Expand Down Expand Up @@ -1050,6 +1053,14 @@ function getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, t
}
}

// Add optimistic transaction violations if there is a policy
const currentTransactionViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`];
if (policy && policy.id) {
optimisticData.push(
ViolationsUtils.getViolationsOnyxData(updatedTransaction, currentTransactionViolations, policy.requiresTag, policyTags, policy.requiresCategory, policyCategories),
);
}

// Clear out the error fields and loading states on success
successData.push({
onyxMethod: Onyx.METHOD.MERGE,
Expand Down Expand Up @@ -1088,6 +1099,15 @@ function getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, t
value: iouReport,
});

// If there is a policy, restore transaction violations to their original state
if (policy && policy.id) {
failureData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`,
value: currentTransactionViolations,
});
}

return {
params,
onyxData: {optimisticData, successData, failureData},
Expand All @@ -1100,12 +1120,15 @@ function getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, t
* @param {String} transactionID
* @param {String} transactionThreadReportID
* @param {String} val
* @param {Object} policy - May be undefined, an empty object, or an object matching the Policy type (src/types/onyx/Policy.ts)
* @param {Array} policyTags
* @param {Array} policyCategories
*/
function updateMoneyRequestDate(transactionID, transactionThreadReportID, val) {
function updateMoneyRequestDate(transactionID, transactionThreadReportID, val, policy, policyTags, policyCategories) {
const transactionChanges = {
created: val,
};
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, true);
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, policy, policyTags, policyCategories, true);
API.write('UpdateMoneyRequestDate', params, onyxData);
}

Expand All @@ -1130,12 +1153,15 @@ function updateMoneyRequestBillable(transactionID, transactionThreadReportID, va
* @param {String} transactionID
* @param {Number} transactionThreadReportID
* @param {String} val
* @param {Object} policy - May be undefined, an empty object, or an object matching the Policy type (src/types/onyx/Policy.ts)
* @param {Array} policyTags
* @param {Array} policyCategories
*/
function updateMoneyRequestMerchant(transactionID, transactionThreadReportID, val) {
function updateMoneyRequestMerchant(transactionID, transactionThreadReportID, val, policy, policyTags, policyCategories) {
const transactionChanges = {
merchant: val,
};
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, true);
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, policy, policyTags, policyCategories, true);
API.write('UpdateMoneyRequestMerchant', params, onyxData);
}

Expand All @@ -1145,12 +1171,15 @@ function updateMoneyRequestMerchant(transactionID, transactionThreadReportID, va
* @param {String} transactionID
* @param {Number} transactionThreadReportID
* @param {String} tag
* @param {Object} policy - May be undefined, an empty object, or an object matching the Policy type (src/types/onyx/Policy.ts)
* @param {Array} policyTags
* @param {Array} policyCategories
*/
function updateMoneyRequestTag(transactionID, transactionThreadReportID, tag) {
function updateMoneyRequestTag(transactionID, transactionThreadReportID, tag, policy, policyTags, policyCategories) {
Copy link
Member

Choose a reason for hiding this comment

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

I think updateMoneyRequestCategory should also be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agh, yup, you're right, that was added in the refactor that just got merged in and so it was missed.

const transactionChanges = {
tag,
};
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, true);
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, policy, policyTags, policyCategories, true);
API.write('UpdateMoneyRequestTag', params, onyxData);
}

Expand All @@ -1164,10 +1193,12 @@ function updateMoneyRequestTag(transactionID, transactionThreadReportID, tag) {
* @param {Number} [transactionChanges.amount]
* @param {Object} [transactionChanges.comment]
* @param {Object} [transactionChanges.waypoints]
*
* @param {Object} policy - May be undefined, an empty object, or an object matching the Policy type (src/types/onyx/Policy.ts)
* @param {Array} policyTags
* @param {Array} policyCategories
*/
function updateDistanceRequest(transactionID, transactionThreadReportID, transactionChanges) {
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, false);
function updateDistanceRequest(transactionID, transactionThreadReportID, transactionChanges, policy, policyTags, policyCategories) {
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, policy, policyTags, policyCategories, false);
API.write('UpdateDistanceRequest', params, onyxData);
}

Expand Down Expand Up @@ -2185,8 +2216,11 @@ function setDraftSplitTransaction(transactionID, transactionChanges = {}) {
* @param {String} transactionID
* @param {Number} transactionThreadReportID
* @param {Object} transactionChanges
* @param {Object} policy - May be undefined, an empty object, or an object matching the Policy type (src/types/onyx/Policy.ts)
* @param {Array} policyTags
* @param {Array} policyCategories
*/
function editRegularMoneyRequest(transactionID, transactionThreadReportID, transactionChanges) {
function editRegularMoneyRequest(transactionID, transactionThreadReportID, transactionChanges, policy = {}, policyTags, policyCategories) {
// STEP 1: Get all collections we're updating
const transactionThread = allReports[`${ONYXKEYS.COLLECTION.REPORT}${transactionThreadReportID}`];
const transaction = allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`];
Expand Down Expand Up @@ -2396,6 +2430,25 @@ function editRegularMoneyRequest(transactionID, transactionThreadReportID, trans
},
];

// Add transaction violations if there is a policy
if (policy && policy.id) {
const currentTransactionViolations = allTransactionViolations[`${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`];
const updatedViolationsOnyxData = ViolationsUtils.getViolationsOnyxData(
updatedTransaction,
currentTransactionViolations,
policy.requiresTag,
policyTags,
policy.requiresCategory,
policyCategories,
);
optimisticData.push(updatedViolationsOnyxData);
failureData.push({
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transactionID}`,
value: currentTransactionViolations,
});
}

// STEP 6: Call the API endpoint
const {created, amount, currency, comment, merchant, category, billable, tag} = ReportUtils.getTransactionDetails(updatedTransaction);
API.write(
Expand All @@ -2420,12 +2473,15 @@ function editRegularMoneyRequest(transactionID, transactionThreadReportID, trans
* @param {object} transaction
* @param {String} transactionThreadReportID
* @param {Object} transactionChanges
* @param {Object} policy - May be undefined, an empty object, or an object matching the Policy type (src/types/onyx/Policy.ts)
* @param {Array} policyTags
* @param {Array} policyCategories
*/
function editMoneyRequest(transaction, transactionThreadReportID, transactionChanges) {
function editMoneyRequest(transaction, transactionThreadReportID, transactionChanges, policy, policyTags, policyCategories) {
if (TransactionUtils.isDistanceRequest(transaction)) {
updateDistanceRequest(transaction.transactionID, transactionThreadReportID, transactionChanges);
updateDistanceRequest(transaction.transactionID, transactionThreadReportID, transactionChanges, policy, policyTags, policyCategories);
} else {
editRegularMoneyRequest(transaction.transactionID, transactionThreadReportID, transactionChanges);
editRegularMoneyRequest(transaction.transactionID, transactionThreadReportID, transactionChanges, policy, policyTags, policyCategories);
}
}

Expand All @@ -2436,13 +2492,16 @@ function editMoneyRequest(transaction, transactionThreadReportID, transactionCha
* @param {String} transactionThreadReportID
* @param {String} currency
* @param {Number} amount
* @param {Object} policy - May be undefined, an empty object, or an object matching the Policy type (src/types/onyx/Policy.ts)
* @param {Array} policyTags
* @param {Array} policyCategories
*/
function updateMoneyRequestAmountAndCurrency(transactionID, transactionThreadReportID, currency, amount) {
function updateMoneyRequestAmountAndCurrency(transactionID, transactionThreadReportID, currency, amount, policy, policyTags, policyCategories) {
const transactionChanges = {
amount,
currency,
};
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, true);
const {params, onyxData} = getUpdateMoneyRequestParams(transactionID, transactionThreadReportID, transactionChanges, policy, policyTags, policyCategories, true);
API.write('UpdateMoneyRequestAmountAndCurrency', params, onyxData);
}

Expand Down
Loading
Loading