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

Adds Client-side Violations when creating money requests #32528

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
e4135d1
Possible solution to not passing down tags and categories
cdanwards Nov 21, 2023
8fe667d
Updated withPolicy with new properties and also passed through to IOU…
cdanwards Nov 28, 2023
1cd0e08
Added some logs for testing -- remove these
cdanwards Nov 28, 2023
22ee011
Fixed merge conflicts
cdanwards Nov 29, 2023
441788c
Merge branch 'trevor-coleman/violations/violation-utils' of github.co…
cdanwards Nov 30, 2023
0cf233c
Fixed merge conflicts
cdanwards Dec 5, 2023
6f84a87
remove unused export
cdanwards Dec 5, 2023
57f2aa8
Merge branch 'main' of github.com:infinitered/ExpensifyApp into cdanw…
cdanwards Dec 5, 2023
f6f9cac
Remove console.log statements and fix import path
cdanwards Dec 5, 2023
9a041cc
Remove unused function getPolicyTags
cdanwards Dec 5, 2023
150487e
Fixed withOnyx call
cdanwards Dec 6, 2023
5b8bcc5
Merge branch 'main' of github.com:infinitered/ExpensifyApp into cdanw…
cdanwards Dec 6, 2023
b2128e5
Merge branch 'main' of github.com:infinitered/ExpensifyApp into cdanw…
cdanwards Dec 8, 2023
6c30158
Update src/libs/actions/IOU.js
cdanwards Dec 11, 2023
705329c
Update src/pages/workspace/withPolicy.tsx
cdanwards Dec 11, 2023
c93a59d
Update src/pages/iou/steps/MoneyRequestConfirmPage.js
cdanwards Dec 11, 2023
b6e959f
Update src/pages/workspace/withPolicy.tsx
cdanwards Dec 11, 2023
f0c609b
Update src/pages/workspace/withPolicy.tsx
cdanwards Dec 11, 2023
dbf3e5a
Update src/pages/workspace/withPolicy.tsx
cdanwards Dec 11, 2023
9783c47
Update src/libs/actions/IOU.js
cdanwards Dec 11, 2023
66d2767
Merge branch 'main' of github.com:infinitered/ExpensifyApp into cdanw…
cdanwards Dec 13, 2023
3c0b52c
Updated proptypes and reimplemented the second withonyx
cdanwards Dec 13, 2023
1547445
Merge branch 'cdanwards/violations/money-request-violations' of githu…
cdanwards Dec 13, 2023
6c3a853
Merge branch 'main' into cdanwards/violations/money-request-violations
lindboe Dec 14, 2023
633199b
Fix withOnyx references
lindboe Dec 15, 2023
919f967
Add policy data to new verison of money request confirmation screen
lindboe Dec 18, 2023
9885e31
Merge remote-tracking branch 'upstream/main' into cdanwards/violation…
lindboe Dec 18, 2023
4b6f463
Merge remote-tracking branch 'upstream/main' into cdanwards/violation…
lindboe Dec 19, 2023
0f91f22
Fix hook dependencies
lindboe Dec 19, 2023
e5b607e
Fix transaction violation storage
lindboe Dec 19, 2023
a8741ab
Use SET instead
lindboe Dec 19, 2023
8d417a9
Check policy violations locally for distance requests
lindboe Dec 20, 2023
34c080e
Merge remote-tracking branch 'upstream/main' into cdanwards/violation…
lindboe Jan 2, 2024
ab06fbe
Use policy arg for fetching paid policy info
lindboe Jan 3, 2024
da3469e
Merge remote-tracking branch 'upstream/main' into cdanwards/violation…
lindboe Jan 3, 2024
9b2fb39
Merge remote-tracking branch 'upstream/main' into cdanwards/violation…
lindboe Jan 4, 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
87 changes: 83 additions & 4 deletions src/libs/actions/IOU.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import * as TransactionUtils from '@libs/TransactionUtils';
import * as UserUtils from '@libs/UserUtils';
import ViolationsUtils from '@libs/ViolationsUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
Expand Down Expand Up @@ -306,6 +307,27 @@ function getReceiptError(receipt, filename, isScanRequest = true) {
: ErrorUtils.getMicroSecondOnyxErrorObject({error: CONST.IOU.RECEIPT_ERROR, source: receipt.source, filename});
}

/**
* Builds the Onyx data for a money request.
*
* @param {Object} chatReport
* @param {Object} iouReport
* @param {Object} transaction
* @param {Object} chatCreatedAction
* @param {Object} iouCreatedAction
* @param {Object} iouAction
* @param {Object} optimisticPersonalDetailListAction
* @param {Object} reportPreviewAction
* @param {Array} optimisticPolicyRecentlyUsedCategories
* @param {Array} optimisticPolicyRecentlyUsedTags
* @param {boolean} isNewChatReport
* @param {boolean} isNewIOUReport
* @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} hasOutstandingChildRequest
* @returns {Array} - An array containing the optimistic data, success data, and failure data.
*/
function buildOnyxDataForMoneyRequest(
chatReport,
iouReport,
Expand All @@ -319,6 +341,9 @@ function buildOnyxDataForMoneyRequest(
optimisticPolicyRecentlyUsedTags,
isNewChatReport,
isNewIOUReport,
policy,
policyTags,
policyCategories,
hasOutstandingChildRequest = false,
) {
const isScanRequest = TransactionUtils.isScanRequest(transaction);
Expand Down Expand Up @@ -556,6 +581,22 @@ function buildOnyxDataForMoneyRequest(
},
];

// Policy won't be set for P2P cases for which we don't need to compute violations
if (!policy || !policy.id) {
cdanwards marked this conversation as resolved.
Show resolved Hide resolved
return [optimisticData, successData, failureData];
}

const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], policy.requiresTag, policyTags, policy.requiresCategory, policyCategories);
Copy link
Contributor

@eh2077 eh2077 Apr 9, 2024

Choose a reason for hiding this comment

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

This PR caused a regression #38131 as we don't want to show those violations when a transaction is a "partial transaction", see also #38131 (comment)

So, we need to add a check for partial transactions when adding violations data.


if (violationsOnyxData) {
optimisticData.push(violationsOnyxData);
failureData.push({
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`,
value: [],
});
}

return [optimisticData, successData, failureData];
}

Expand All @@ -577,6 +618,9 @@ function buildOnyxDataForMoneyRequest(
* @param {String} [category]
* @param {String} [tag]
* @param {Boolean} [billable]
* @param {Object} [policy]
* @param {Object} [policyTags]
* @param {Object} [policyCategories]
* @returns {Object} data
* @returns {String} data.payerEmail
* @returns {Object} data.iouReport
Expand Down Expand Up @@ -606,6 +650,9 @@ function getMoneyRequestInformation(
category = undefined,
tag = undefined,
billable = undefined,
policy = undefined,
policyTags = undefined,
policyCategories = undefined,
) {
const payerEmail = OptionsListUtils.addSMSDomainIfPhoneNumber(participant.login);
const payerAccountID = Number(participant.accountID);
Expand Down Expand Up @@ -639,7 +686,6 @@ function getMoneyRequestInformation(
let needsToBeManuallySubmitted = false;
let isFromPaidPolicy = false;
if (isPolicyExpenseChat) {
const policy = ReportUtils.getPolicy(chatReport.policyID);
isFromPaidPolicy = PolicyUtils.isPaidGroupPolicy(policy);

// If the scheduled submit is turned off on the policy, user needs to manually submit the report which is indicated by GBR in LHN
Expand Down Expand Up @@ -774,6 +820,9 @@ function getMoneyRequestInformation(
optimisticPolicyRecentlyUsedTags,
isNewChatReport,
isNewIOUReport,
policy,
policyTags,
policyCategories,
hasOutstandingChildRequest,
);

Expand Down Expand Up @@ -808,9 +857,12 @@ function getMoneyRequestInformation(
* @param {String} currency
* @param {String} merchant
* @param {Boolean} [billable]
* @param {Obejct} validWaypoints
* @param {Object} validWaypoints
* @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 createDistanceRequest(report, participant, comment, created, category, tag, amount, currency, merchant, billable, validWaypoints) {
function createDistanceRequest(report, participant, comment, created, category, tag, amount, currency, merchant, billable, validWaypoints, policy, policyTags, policyCategories) {
// If the report is an iou or expense report, we should get the linked chat report to be passed to the getMoneyRequestInformation function
const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report);
const currentChatReport = isMoneyRequestReport ? ReportUtils.getReport(report.chatReportID) : report;
Expand All @@ -834,6 +886,9 @@ function createDistanceRequest(report, participant, comment, created, category,
category,
tag,
billable,
policy,
policyTags,
policyCategories,
);
API.write(
'CreateDistanceRequest',
Expand Down Expand Up @@ -1089,6 +1144,9 @@ function updateDistanceRequest(transactionID, transactionThreadReportID, transac
* @param {String} [taxCode]
* @param {Number} [taxAmount]
* @param {Boolean} [billable]
* @param {Object} [policy]
* @param {Object} [policyTags]
* @param {Object} [policyCategories]
*/
function requestMoney(
report,
Expand All @@ -1106,12 +1164,33 @@ function requestMoney(
taxCode = '',
taxAmount = 0,
billable = undefined,
policy = undefined,
policyTags = undefined,
policyCategories = undefined,
) {
// If the report is iou or expense report, we should get the linked chat report to be passed to the getMoneyRequestInformation function
const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report);
const currentChatReport = isMoneyRequestReport ? ReportUtils.getReport(report.chatReportID) : report;
const {payerAccountID, payerEmail, iouReport, chatReport, transaction, iouAction, createdChatReportActionID, createdIOUReportActionID, reportPreviewAction, onyxData} =
getMoneyRequestInformation(currentChatReport, participant, comment, amount, currency, created, merchant, payeeAccountID, payeeEmail, receipt, undefined, category, tag, billable);
getMoneyRequestInformation(
currentChatReport,
participant,
comment,
amount,
currency,
created,
merchant,
payeeAccountID,
payeeEmail,
receipt,
undefined,
category,
tag,
billable,
policy,
policyTags,
policyCategories,
);
const activeReportID = isMoneyRequestReport ? report.reportID : chatReport.reportID;

API.write(
Expand Down
31 changes: 28 additions & 3 deletions src/pages/iou/request/step/IOURequestStepConfirmation.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import React, {useCallback, useEffect, useMemo, useState} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import categoryPropTypes from '@components/categoryPropTypes';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import * as Expensicons from '@components/Icon/Expensicons';
import MoneyRequestConfirmationList from '@components/MoneyTemporaryForRefactorRequestConfirmationList';
import ScreenWrapper from '@components/ScreenWrapper';
import tagPropTypes from '@components/tagPropTypes';
import transactionPropTypes from '@components/transactionPropTypes';
import withCurrentUserPersonalDetails, {withCurrentUserPersonalDetailsDefaultProps, withCurrentUserPersonalDetailsPropTypes} from '@components/withCurrentUserPersonalDetails';
import useLocalize from '@hooks/useLocalize';
Expand Down Expand Up @@ -46,6 +49,12 @@ const propTypes = {
/** The policy of the report */
...policyPropTypes,

/** The tag configuration of the report's policy */
policyTags: tagPropTypes,

/** The category configuration of the report's policy */
policyCategories: PropTypes.objectOf(categoryPropTypes),

/** The full IOU report */
report: reportPropTypes,

Expand All @@ -55,6 +64,8 @@ const propTypes = {
const defaultProps = {
personalDetails: {},
policy: {},
policyCategories: {},
policyTags: {},
report: {},
transaction: {},
...withCurrentUserPersonalDetailsDefaultProps,
Expand All @@ -63,6 +74,8 @@ function IOURequestStepConfirmation({
currentUserPersonalDetails,
personalDetails,
policy,
policyTags,
policyCategories,
report,
route: {
params: {iouType, reportID, transactionID},
Expand Down Expand Up @@ -164,9 +177,12 @@ function IOURequestStepConfirmation({
transactionTaxCode,
transactionTaxAmount,
transaction.billable,
policy,
policyTags,
policyCategories,
);
},
[report, transaction, transactionTaxCode, transactionTaxAmount, currentUserPersonalDetails.login, currentUserPersonalDetails.accountID],
[report, transaction, transactionTaxCode, transactionTaxAmount, currentUserPersonalDetails.login, currentUserPersonalDetails.accountID, policy, policyTags, policyCategories],
);

/**
Expand All @@ -187,9 +203,12 @@ function IOURequestStepConfirmation({
transaction.merchant,
transaction.billable,
TransactionUtils.getValidWaypoints(transaction.comment.waypoints, true),
policy,
policyTags,
policyCategories,
);
},
[report, transaction],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change from a single transaction, to several transaction.property?

Copy link
Contributor

@lindboe lindboe Dec 21, 2023

Choose a reason for hiding this comment

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

That was an auto-fix from prettier/the linter, I didn't intentionally make this change. I'm not sure why it applied to one hook and not the other, I'll take a look.

That said, this is probably more accurate as to whether the hook should re-run, as it's going to be looking at values more directly instead of the whole object (which can pretty easily change identity even if all the values are equivalent, depending on the data source's implementation).

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like when I used the auto-fix function to add policy, etc, to the dependency array, the auto-fix also updated the array to match everything it expected, including being more specific about the args. Like I said before, using transaction here isn't technically wrong, it's just probably less good.

That said I just pushed an update to remove the unnecessary change.

[policy, policyCategories, policyTags, report, transaction],
);

const createTransaction = useCallback(
Expand Down Expand Up @@ -375,7 +394,13 @@ export default compose(
// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
withOnyx({
policy: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${lodashGet(report, 'policyID', '0')}`,
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report ? report.policyID : '0'}`,
},
policyCategories: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${report ? report.policyID : '0'}`,
},
policyTags: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY_TAGS}${report ? report.policyID : '0'}`,
},
}),
)(IOURequestStepConfirmation);
44 changes: 42 additions & 2 deletions src/pages/iou/steps/MoneyRequestConfirmPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import categoryPropTypes from '@components/categoryPropTypes';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import * as Expensicons from '@components/Icon/Expensicons';
import MoneyRequestConfirmationList from '@components/MoneyRequestConfirmationList';
import {usePersonalDetails} from '@components/OnyxProvider';
import ScreenWrapper from '@components/ScreenWrapper';
import tagPropTypes from '@components/tagPropTypes';
import withCurrentUserPersonalDetails, {withCurrentUserPersonalDetailsDefaultProps, withCurrentUserPersonalDetailsPropTypes} from '@components/withCurrentUserPersonalDetails';
import withLocalize from '@components/withLocalize';
import useInitialValue from '@hooks/useInitialValue';
Expand All @@ -23,6 +25,7 @@ import * as OptionsListUtils from '@libs/OptionsListUtils';
import * as ReportUtils from '@libs/ReportUtils';
import {iouDefaultProps, iouPropTypes} from '@pages/iou/propTypes';
import reportPropTypes from '@pages/reportPropTypes';
import {policyDefaultProps, policyPropTypes} from '@pages/workspace/withPolicy';
import * as IOU from '@userActions/IOU';
import * as Policy from '@userActions/Policy';
import CONST from '@src/CONST';
Expand All @@ -47,12 +50,22 @@ const propTypes = {
/** Holds data related to Money Request view state, rather than the underlying Money Request data. */
iou: iouPropTypes,

/** The policy of the current request */
policy: policyPropTypes,

policyTags: tagPropTypes,

policyCategories: PropTypes.objectOf(categoryPropTypes),

...withCurrentUserPersonalDetailsPropTypes,
};

const defaultProps = {
report: {},
policyCategories: {},
policyTags: {},
iou: iouDefaultProps,
policy: policyDefaultProps,
...withCurrentUserPersonalDetailsDefaultProps,
};

Expand Down Expand Up @@ -163,6 +176,9 @@ function MoneyRequestConfirmPage(props) {
props.iou.category,
props.iou.tag,
props.iou.billable,
props.policy,
props.policyTags,
props.policyCategories,
);
},
[
Expand All @@ -176,6 +192,9 @@ function MoneyRequestConfirmPage(props) {
props.iou.category,
props.iou.tag,
props.iou.billable,
props.policy,
props.policyTags,
props.policyCategories,
],
);

Expand All @@ -197,9 +216,25 @@ function MoneyRequestConfirmPage(props) {
props.iou.currency,
props.iou.merchant,
props.iou.billable,
props.policy,
props.policyTags,
props.policyCategories,
);
},
[props.report, props.iou.created, props.iou.transactionID, props.iou.category, props.iou.tag, props.iou.amount, props.iou.currency, props.iou.merchant, props.iou.billable],
[
props.report,
props.iou.created,
props.iou.transactionID,
props.iou.category,
props.iou.tag,
props.iou.amount,
props.iou.currency,
props.iou.merchant,
props.iou.billable,
props.policy,
props.policyTags,
props.policyCategories,
],
);

const createTransaction = useCallback(
Expand Down Expand Up @@ -424,10 +459,15 @@ export default compose(
key: `${ONYXKEYS.COLLECTION.SELECTED_TAB}${CONST.TAB.RECEIPT_TAB_ID}`,
},
}),
// eslint-disable-next-line rulesdir/no-multiple-onyx-in-file
withOnyx({
policy: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY}${report ? report.policyID : '0'}`,
},
policyCategories: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${report ? report.policyID : '0'}`,
},
policyTags: {
key: ({report}) => `${ONYXKEYS.COLLECTION.POLICY_TAGS}${report ? report.policyID : '0'}`,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I got an error

Screenshot 2023-12-16 at 03 33 24

I think the policy data is not yet found. Either we go with the multiple onyx or use the report.policyID instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated to use report.policyID.

}),
)(MoneyRequestConfirmPage);
12 changes: 12 additions & 0 deletions src/pages/workspace/withPolicy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,18 @@ const policyPropTypes = {
* }
*/
errorFields: PropTypes.objectOf(PropTypes.objectOf(PropTypes.string)),

/** Whether or not the policy requires tags */
requiresTag: PropTypes.bool,

/** Whether or not the policy requires categories */
requiresCategory: PropTypes.bool,

/** Whether or not the policy has multiple tag lists */
hasMultipleTagLists: PropTypes.bool,

/** Whether or not the policy has tax tracking enabled */
isTaxTrackingEnabled: PropTypes.bool,
}),

/** The employee list of this policy */
Expand Down
Loading