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

Fix client side tag violations and Improve function signatures #36821

Merged
merged 27 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ function MoneyRequestPreviewContent({
const requestMerchant = truncate(merchant, {length: CONST.REQUEST_PREVIEW.MAX_LENGTH});
const hasReceipt = TransactionUtils.hasReceipt(transaction);
const isScanning = hasReceipt && TransactionUtils.isReceiptBeingScanned(transaction);
const hasViolations = TransactionUtils.hasViolation(transaction, transactionViolations);
const hasViolations = TransactionUtils.hasViolation(transaction?.transactionID ?? '', transactionViolations);
const hasFieldErrors = TransactionUtils.hasMissingSmartscanFields(transaction);
const shouldShowRBR = hasViolations || hasFieldErrors;
const isDistanceRequest = TransactionUtils.isDistanceRequest(transaction);
Expand Down Expand Up @@ -152,13 +152,13 @@ function MoneyRequestPreviewContent({

let message = translate('iou.cash');
if (hasViolations && transaction) {
const violations = TransactionUtils.getTransactionViolations(transaction, transactionViolations);
const violations = TransactionUtils.getTransactionViolations(transaction.transactionID, transactionViolations);
if (violations?.[0]) {
const violationMessage = ViolationsUtils.getViolationTranslation(violations[0], translate);
const isTooLong = violations.filter((v) => v.type === 'violation').length > 1 || violationMessage.length > 15;
message += ` • ${isTooLong ? translate('violations.reviewRequired') : violationMessage}`;
}
} else if (ReportUtils.isPaidGroupPolicyExpenseReport(iouReport) && ReportUtils.isReportApproved(iouReport) && !ReportUtils.isSettled(iouReport)) {
} else if (ReportUtils.isPaidGroupPolicyExpenseReport(iouReport) && ReportUtils.isReportApproved(iouReport) && !ReportUtils.isSettled(iouReport?.reportID)) {
message += ` • ${translate('iou.approved')}`;
} else if (iouReport?.isWaitingOnBankAccount) {
message += ` • ${translate('iou.pending')}`;
Expand Down
24 changes: 7 additions & 17 deletions src/components/ReportActionItem/MoneyRequestView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,7 @@ function MoneyRequestView({
const shouldShowBillable = isPolicyExpenseChat && (!!transactionBillable || !(policy?.disabledFields?.defaultBillable ?? true));

const {getViolationsForField} = useViolations(transactionViolations ?? []);
const hasViolations = useCallback(
(field: ViolationField, data?: OnyxTypes.TransactionViolation['data']): boolean => !!canUseViolations && getViolationsForField(field, data).length > 0,
[canUseViolations, getViolationsForField],
);
const hasViolations = useCallback((field: ViolationField): boolean => !!canUseViolations && getViolationsForField(field).length > 0, [canUseViolations, getViolationsForField]);

let amountDescription = `${translate('iou.amount')}`;

Expand Down Expand Up @@ -202,7 +199,7 @@ function MoneyRequestView({
const getPendingFieldAction = (fieldPath: TransactionPendingFieldsKey) => transaction?.pendingFields?.[fieldPath] ?? pendingAction;

const getErrorForField = useCallback(
(field: ViolationField, data?: OnyxTypes.TransactionViolation['data']) => {
(field: ViolationField, data?: OnyxTypes.TransactionViolation['data'], shouldShowViolations = true) => {
// Checks applied when creating a new money request
// NOTE: receipt field can return multiple violations, so we need to handle it separately
const fieldChecks: Partial<Record<ViolationField, {isError: boolean; translationPath: TranslationPaths}>> = {
Expand All @@ -228,8 +225,9 @@ function MoneyRequestView({
}

// Return violations if there are any
if (canUseViolations && hasViolations(field, data)) {
const violations = getViolationsForField(field, data);
// At the moment, we only return violations for tags for workspaces with single-level tags
if (canUseViolations && shouldShowViolations && hasViolations(field)) {
const violations = getViolationsForField(field);
return ViolationsUtils.getViolationTranslation(violations[0], translate);
}

Expand Down Expand Up @@ -400,16 +398,8 @@ function MoneyRequestView({
ROUTES.MONEY_REQUEST_STEP_TAG.getRoute(CONST.IOU.ACTION.EDIT, CONST.IOU.TYPE.REQUEST, index, transaction?.transactionID ?? '', report.reportID),
)
}
brickRoadIndicator={
getErrorForField('tag', {
tagName: name,
})
? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR
: undefined
}
error={getErrorForField('tag', {
tagName: name,
})}
brickRoadIndicator={getErrorForField('tag', {}, policyTagLists.length === 1) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
error={getErrorForField('tag', {}, policyTagLists.length === 1)}
/>
</OfflineWithFeedback>
))}
Expand Down
14 changes: 1 addition & 13 deletions src/hooks/useViolations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,19 +58,7 @@ function useViolations(violations: TransactionViolation[]) {
}
return violationGroups ?? new Map();
}, [violations]);

const getViolationsForField = useCallback(
(field: ViolationField, data?: TransactionViolation['data']) => {
const currentViolations = violationsByField.get(field) ?? [];

if (data?.tagName) {
return currentViolations.filter((violation) => violation.data?.tagName === data.tagName);
}
Comment on lines -66 to -68
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this filter no longer required? Looks it was added to support multi level tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created an issue to follow up to implement violations for multi level tags #37117


return currentViolations;
},
[violationsByField],
);
const getViolationsForField = useCallback((field: ViolationField) => violationsByField.get(field) ?? [], [violationsByField]);

return {
getViolationsForField,
Expand Down
6 changes: 2 additions & 4 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -722,12 +722,10 @@ function hasParticipantInArray(report: Report, policyMemberAccountIDs: number[])
/**
* Whether the Money Request report is settled
*/
function isSettled(reportOrID: Report | OnyxEntry<Report> | string | undefined): boolean {
if (!allReports || !reportOrID) {
function isSettled(reportID: string | undefined): boolean {
if (!allReports || !reportID) {
return false;
}
const reportID = typeof reportOrID === 'string' ? reportOrID : reportOrID?.reportID;

const report: Report | EmptyObject = allReports[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`] ?? {};
if (isEmptyObject(report) || report.isWaitingOnBankAccount) {
return false;
Expand Down
13 changes: 2 additions & 11 deletions src/libs/TransactionUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -587,20 +587,11 @@ function isOnHold(transaction: OnyxEntry<Transaction>): boolean {
/**
* Checks if any violations for the provided transaction are of type 'violation'
*/
function hasViolation(transactionOrID: Transaction | OnyxEntry<Transaction> | string, transactionViolations: OnyxCollection<TransactionViolation[]>): boolean {
if (!transactionOrID) {
return false;
}
const transactionID = typeof transactionOrID === 'string' ? transactionOrID : transactionOrID.transactionID;
function hasViolation(transactionID: string, transactionViolations: OnyxCollection<TransactionViolation[]>): boolean {
return Boolean(transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID]?.some((violation: TransactionViolation) => violation.type === 'violation'));
}

function getTransactionViolations(transactionOrID: OnyxEntry<Transaction> | string, transactionViolations: OnyxCollection<TransactionViolation[]>): TransactionViolation[] | null {
if (!transactionOrID) {
return null;
}
const transactionID = typeof transactionOrID === 'string' ? transactionOrID : transactionOrID.transactionID;

function getTransactionViolations(transactionID: string, transactionViolations: OnyxCollection<TransactionViolation[]>): TransactionViolation[] | null {
return transactionViolations?.[ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS + transactionID] ?? null;
}

Expand Down
67 changes: 17 additions & 50 deletions src/libs/Violations/ViolationsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import reject from 'lodash/reject';
import Onyx from 'react-native-onyx';
import type {OnyxUpdate} from 'react-native-onyx';
import type {Phrase, PhraseParameters} from '@libs/Localize';
import * as TransactionUtils from '@libs/TransactionUtils';
import CONST from '@src/CONST';
import type {TranslationPaths} from '@src/languages/types';
import ONYXKEYS from '@src/ONYXKEYS';
Expand Down Expand Up @@ -31,7 +30,7 @@ const ViolationsUtils = {

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

// Remove 'categoryOutOfPolicy' violation if category is in policy
Expand All @@ -46,72 +45,40 @@ const ViolationsUtils = {

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

if (policyRequiresTags) {
const selectedTags = TransactionUtils.getTagArrayFromName(updatedTransaction.tag ?? '') ?? [];
const policyTagKeys = Object.keys(policyTagList);

if (policyTagKeys.length === 0) {
newTransactionViolations.push({
name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
type: 'violation',
userMessage: '',
});
}

policyTagKeys.forEach((key, index) => {
const hasTagOutOfPolicyViolation = transactionViolations.some((violation) => violation.name === CONST.VIOLATIONS.TAG_OUT_OF_POLICY && violation.data?.tagName === key);
const hasMissingTagViolation = transactionViolations.some((violation) => violation.name === CONST.VIOLATIONS.MISSING_TAG && violation.data?.tagName === key);
const selectedTag = selectedTags[index];
const isTagInPolicy = Boolean(policyTagList[key]?.tags[selectedTag]?.enabled);
// At the moment, we only return violations for tags for workspaces with single-level tags
if (policyTagKeys.length === 1) {
const policyTagListName = policyTagKeys[0];
const policyTags = policyTagList[policyTagListName]?.tags;
const hasTagOutOfPolicyViolation = transactionViolations.some((violation) => violation.name === CONST.VIOLATIONS.TAG_OUT_OF_POLICY);
const hasMissingTagViolation = transactionViolations.some((violation) => violation.name === CONST.VIOLATIONS.MISSING_TAG);
const isTagInPolicy = policyTags ? !!policyTags[updatedTransaction.tag ?? '']?.enabled : false;

// Add 'tagOutOfPolicy' violation if tag is not in policy
if (!hasTagOutOfPolicyViolation && selectedTag && !isTagInPolicy) {
newTransactionViolations.push({
name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
type: 'violation',
userMessage: '',
data: {
tagName: key,
},
});
if (!hasTagOutOfPolicyViolation && updatedTransaction.tag && !isTagInPolicy) {
newTransactionViolations.push({name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY, type: 'violation'});
}

// Remove 'tagOutOfPolicy' violation if tag is in policy
if (hasTagOutOfPolicyViolation && selectedTag && isTagInPolicy) {
newTransactionViolations = reject(newTransactionViolations, {
name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
data: {
tagName: key,
},
});
if (hasTagOutOfPolicyViolation && updatedTransaction.tag && isTagInPolicy) {
newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY});
}

// Remove 'missingTag' violation if tag is valid according to policy
if (hasMissingTagViolation && isTagInPolicy) {
newTransactionViolations = reject(newTransactionViolations, {
name: CONST.VIOLATIONS.MISSING_TAG,
data: {
tagName: key,
},
});
newTransactionViolations = reject(newTransactionViolations, {name: CONST.VIOLATIONS.MISSING_TAG});
}

// Add 'missingTag violation' if tag is required and not set
if (!hasMissingTagViolation && !selectedTag && policyRequiresTags) {
newTransactionViolations.push({
name: CONST.VIOLATIONS.MISSING_TAG,
type: 'violation',
userMessage: '',
data: {
tagName: key,
},
});
if (!hasMissingTagViolation && !updatedTransaction.tag && policyRequiresTags) {
newTransactionViolations.push({name: CONST.VIOLATIONS.MISSING_TAG, type: 'violation'});
}
});
}
}

return {
Expand Down
1 change: 0 additions & 1 deletion src/types/onyx/TransactionViolation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ type ViolationName = (typeof CONST.VIOLATIONS)[keyof typeof CONST.VIOLATIONS];
type TransactionViolation = {
type: string;
name: ViolationName;
userMessage: string;
data?: {
rejectedBy?: string;
rejectReason?: string;
Expand Down
32 changes: 14 additions & 18 deletions tests/unit/ViolationUtilsTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,21 @@ import ONYXKEYS from '@src/ONYXKEYS';
const categoryOutOfPolicyViolation = {
name: 'categoryOutOfPolicy',
type: 'violation',
userMessage: '',
};

const missingCategoryViolation = {
name: 'missingCategory',
type: 'violation',
userMessage: '',
};

const tagOutOfPolicyViolation = {
name: 'tagOutOfPolicy',
type: 'violation',
userMessage: '',
};

const missingTagViolation = {
name: 'missingTag',
type: 'violation',
userMessage: '',
};

describe('getViolationsOnyxData', () => {
Expand Down Expand Up @@ -56,8 +52,8 @@ describe('getViolationsOnyxData', () => {

it('should handle multiple violations', () => {
transactionViolations = [
{name: 'duplicatedTransaction', type: 'violation', userMessage: ''},
{name: 'receiptRequired', type: 'violation', userMessage: ''},
{name: 'duplicatedTransaction', type: 'violation'},
{name: 'receiptRequired', type: 'violation'},
];
const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);
expect(result.value).toEqual(expect.arrayContaining(transactionViolations));
Expand Down Expand Up @@ -90,8 +86,8 @@ describe('getViolationsOnyxData', () => {
it('should add categoryOutOfPolicy violation to existing violations if they exist', () => {
transaction.category = 'Bananas';
transactionViolations = [
{name: 'duplicatedTransaction', type: 'violation', userMessage: ''},
{name: 'receiptRequired', type: 'violation', userMessage: ''},
{name: 'duplicatedTransaction', type: 'violation'},
{name: 'receiptRequired', type: 'violation'},
];

const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);
Expand All @@ -102,8 +98,8 @@ describe('getViolationsOnyxData', () => {
it('should add missingCategory violation to existing violations if they exist', () => {
transaction.category = undefined;
transactionViolations = [
{name: 'duplicatedTransaction', type: 'violation', userMessage: ''},
{name: 'receiptRequired', type: 'violation', userMessage: ''},
{name: 'duplicatedTransaction', type: 'violation'},
{name: 'receiptRequired', type: 'violation'},
];

const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);
Expand Down Expand Up @@ -157,39 +153,39 @@ describe('getViolationsOnyxData', () => {

const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);

expect(result.value).toEqual(expect.arrayContaining([{...missingTagViolation, data: {tagName: 'Meals'}}]));
expect(result.value).toEqual(expect.arrayContaining([{...missingTagViolation}]));
});

it('should add a tagOutOfPolicy violation when policy requires tags and tag is not in the policy', () => {
policyTags = {};

const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);

expect(result.value).toEqual(expect.arrayContaining([tagOutOfPolicyViolation]));
expect(result.value).toEqual([]);
});

it('should add tagOutOfPolicy violation to existing violations if transaction has tag that is not in the policy', () => {
transaction.tag = 'Bananas';
transactionViolations = [
{name: 'duplicatedTransaction', type: 'violation', userMessage: ''},
{name: 'receiptRequired', type: 'violation', userMessage: ''},
{name: 'duplicatedTransaction', type: 'violation'},
{name: 'receiptRequired', type: 'violation'},
];

const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);

expect(result.value).toEqual(expect.arrayContaining([{...tagOutOfPolicyViolation, data: {tagName: 'Meals'}}, ...transactionViolations]));
expect(result.value).toEqual(expect.arrayContaining([{...tagOutOfPolicyViolation}, ...transactionViolations]));
});

it('should add missingTag violation to existing violations if transaction does not have a tag', () => {
transaction.tag = undefined;
transactionViolations = [
{name: 'duplicatedTransaction', type: 'violation', userMessage: ''},
{name: 'receiptRequired', type: 'violation', userMessage: ''},
{name: 'duplicatedTransaction', type: 'violation'},
{name: 'receiptRequired', type: 'violation'},
];

const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);

expect(result.value).toEqual(expect.arrayContaining([{...missingTagViolation, data: {tagName: 'Meals'}}, ...transactionViolations]));
expect(result.value).toEqual(expect.arrayContaining([{...missingTagViolation}, ...transactionViolations]));
});
});

Expand Down
Loading