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

Add violation logic for multi level tags #37461

Merged
merged 24 commits into from
Mar 8, 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
16 changes: 9 additions & 7 deletions src/components/ReportActionItem/MoneyRequestView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,10 @@ function MoneyRequestView({
const shouldShowBillable = isPolicyExpenseChat && (!!transactionBillable || !(policy?.disabledFields?.defaultBillable ?? true));

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

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

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

const getErrorForField = useCallback(
(field: ViolationField, data?: OnyxTypes.TransactionViolation['data'], shouldShowViolations = true) => {
(field: ViolationField, data?: OnyxTypes.TransactionViolation['data']) => {
// 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 @@ -223,9 +226,8 @@ function MoneyRequestView({
}

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

Expand Down Expand Up @@ -395,8 +397,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', {}, policyTagLists.length === 1) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
error={getErrorForField('tag', {}, policyTagLists.length === 1)}
brickRoadIndicator={getErrorForField('tag', {tagListIndex: index, tagListName: name}) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
error={getErrorForField('tag', {tagListIndex: index, tagListName: name})}
/>
</OfflineWithFeedback>
))}
Expand Down
29 changes: 28 additions & 1 deletion src/hooks/useViolations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,34 @@ function useViolations(violations: TransactionViolation[]) {
}
return violationGroups ?? new Map();
}, [violations]);
const getViolationsForField = useCallback((field: ViolationField) => violationsByField.get(field) ?? [], [violationsByField]);

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

// someTagLevelsRequired has special logic becase data.errorIndexes is a bit unique in how it denotes the tag list that has the violation
// tagListIndex can be 0 so we compare with undefined
if (currentViolations[0]?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(currentViolations[0]?.data?.errorIndexes)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the currentViolations always have a single element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because we only return 1 violation per field per request

return currentViolations
.filter((violation) => violation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1))
.map((violation) => ({
...violation,
data: {
...violation.data,
tagName: data?.tagListName,
},
}));
}

// tagOutOfPolicy has special logic because we have to account for multi-level tags and use tagName to find the right tag to put the violation on
if (currentViolations[0]?.name === 'tagOutOfPolicy' && data?.tagListName !== undefined && currentViolations[0]?.data?.tagName) {
return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName);
}

return currentViolations;
},
[violationsByField],
);

return {
getViolationsForField,
Expand Down
2 changes: 1 addition & 1 deletion src/languages/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2412,7 +2412,7 @@ export default {
return '';
},
smartscanFailed: 'Receipt scanning failed. Enter details manually.',
someTagLevelsRequired: 'Missing tag',
someTagLevelsRequired: ({tagName}: ViolationsTagOutOfPolicyParams) => `Missing ${tagName ?? 'Tag'}`,
tagOutOfPolicy: ({tagName}: ViolationsTagOutOfPolicyParams) => `${tagName ?? 'Tag'} no longer valid`,
taxAmountChanged: 'Tax amount was modified',
taxOutOfPolicy: ({taxName}: ViolationsTaxOutOfPolicyParams) => `${taxName ?? 'Tax'} no longer valid`,
Expand Down
2 changes: 1 addition & 1 deletion src/languages/es.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2904,7 +2904,7 @@ export default {
return '';
},
smartscanFailed: 'No se pudo escanear el recibo. Introduce los datos manualmente',
someTagLevelsRequired: 'Falta etiqueta',
someTagLevelsRequired: ({tagName}: ViolationsTagOutOfPolicyParams) => `Falta ${tagName ?? 'Tag'}`,
tagOutOfPolicy: ({tagName}: ViolationsTagOutOfPolicyParams) => `La etiqueta ${tagName ? `${tagName} ` : ''}ya no es válida`,
taxAmountChanged: 'El importe del impuesto fue modificado',
taxOutOfPolicy: ({taxName}: ViolationsTaxOutOfPolicyParams) => `${taxName ?? 'El impuesto'} ya no es válido`,
Expand Down
137 changes: 107 additions & 30 deletions src/libs/Violations/ViolationsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,106 @@ import type {TranslationPaths} from '@src/languages/types';
import ONYXKEYS from '@src/ONYXKEYS';
import type {PolicyCategories, PolicyTagList, Transaction, TransactionViolation} from '@src/types/onyx';

/**
* Calculates tag out of policy and missing tag violations for the given transaction
*/
function getTagViolationsForSingleLevelTags(
updatedTransaction: Transaction,
transactionViolations: TransactionViolation[],
policyRequiresTags: boolean,
policyTagList: PolicyTagList,
): TransactionViolation[] {
const policyTagKeys = Object.keys(policyTagList);
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;
let newTransactionViolations = [...transactionViolations];

// Add 'tagOutOfPolicy' violation if tag is not in policy
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 && 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});
}

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

/**
* Calculates some tag levels required and missing tag violations for the given transaction
*/
function getTagViolationsForMultiLevelTags(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi team, come from #38095, this function was missing to handle multilevel dependent tags cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This wasn't really missing, as much as it just hadn't been implemented yet. When we added support for multi level tags, we broke the violations for tags, and this PR was meant to fix that.

We then added support for dependent tags, and then implemented getTagViolationsForDependentTags and getTagViolationForIndependentTags in https://github.com/Expensify/App/pull/40741/files

updatedTransaction: Transaction,
transactionViolations: TransactionViolation[],
policyRequiresTags: boolean,
policyTagList: PolicyTagList,
): TransactionViolation[] {
const policyTagKeys = Object.keys(policyTagList);
const selectedTags = updatedTransaction.tag?.split(CONST.COLON) ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have split the tags using getTagArrayFromName method instead. The split function fails if a tag contains more than one :. This caused #50105

let newTransactionViolations = [...transactionViolations];
newTransactionViolations = newTransactionViolations.filter(
(violation) => violation.name !== CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && violation.name !== CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
);

// We first get the errorIndexes for someTagLevelsRequired. If it's not empty, we puth SOME_TAG_LEVELS_REQUIRED in Onyx.
// Otherwise, we put TAG_OUT_OF_POLICY in Onyx (when applicable)
const errorIndexes = [];
for (let i = 0; i < policyTagKeys.length; i++) {
const isTagRequired = policyTagList[policyTagKeys[i]].required ?? true;
const isTagSelected = Boolean(selectedTags[i]);
if (isTagRequired && (!isTagSelected || (selectedTags.length === 1 && selectedTags[0] === ''))) {
errorIndexes.push(i);
}
}
if (errorIndexes.length !== 0) {
newTransactionViolations.push({
name: CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED,
type: 'violation',
data: {
errorIndexes,
},
});
} else {
let hasInvalidTag = false;
for (let i = 0; i < policyTagKeys.length; i++) {
const selectedTag = selectedTags[i];
const tags = policyTagList[policyTagKeys[i]].tags;
const isTagInPolicy = Object.values(tags).some((tag) => tag.name === selectedTag && Boolean(tag.enabled));
if (!isTagInPolicy) {
newTransactionViolations.push({
name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
type: 'violation',
data: {
tagName: policyTagKeys[i],
},
});
hasInvalidTag = true;
break;
}
}
if (!hasInvalidTag) {
newTransactionViolations = reject(newTransactionViolations, {
name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
});
}
}
return newTransactionViolations;
}

const ViolationsUtils = {
/**
* Checks a transaction for policy violations and returns an object with Onyx method, key and updated transaction
Expand All @@ -22,6 +122,7 @@ const ViolationsUtils = {
): OnyxUpdate {
let newTransactionViolations = [...transactionViolations];

// Calculate client-side category violations
if (policyRequiresCategories) {
const hasCategoryOutOfPolicyViolation = transactionViolations.some((violation) => violation.name === 'categoryOutOfPolicy');
const hasMissingCategoryViolation = transactionViolations.some((violation) => violation.name === 'missingCategory');
Expand Down Expand Up @@ -49,36 +150,12 @@ const ViolationsUtils = {
}
}

// Calculate client-side tag violations
if (policyRequiresTags) {
const policyTagKeys = Object.keys(policyTagList);

// 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 && updatedTransaction.tag && !isTagInPolicy) {
newTransactionViolations.push({name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY, type: 'violation'});
}

// Remove 'tagOutOfPolicy' violation if tag is in policy
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});
}
// Add 'missingTag violation' if tag is required and not set
if (!hasMissingTagViolation && !updatedTransaction.tag && policyRequiresTags) {
newTransactionViolations.push({name: CONST.VIOLATIONS.MISSING_TAG, type: 'violation'});
}
}
newTransactionViolations =
Object.keys(policyTagList).length === 1
? getTagViolationsForSingleLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList)
: getTagViolationsForMultiLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList);
}

return {
Expand Down Expand Up @@ -181,7 +258,7 @@ const ViolationsUtils = {
case 'smartscanFailed':
return translate('violations.smartscanFailed');
case 'someTagLevelsRequired':
return translate('violations.someTagLevelsRequired');
return translate('violations.someTagLevelsRequired', {tagName});
case 'tagOutOfPolicy':
return translate('violations.tagOutOfPolicy', {tagName});
case 'taxAmountChanged':
Expand Down
3 changes: 3 additions & 0 deletions src/types/onyx/TransactionViolation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ type TransactionViolation = {
isTransactionOlderThan7Days?: boolean;
member?: string;
taxName?: string;
tagListIndex?: number;
tagListName?: string;
errorIndexes?: number[];
};
};

Expand Down
78 changes: 73 additions & 5 deletions tests/unit/ViolationUtilsTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,6 @@ describe('getViolationsOnyxData', () => {
Lunch: {name: 'Lunch', enabled: true},
Dinner: {name: 'Dinner', enabled: true},
},
Tag: {
name: 'Tag',
required: true,
tags: {Lunch: {enabled: true}, Dinner: {enabled: true}},
},
},
};
transaction.tag = 'Lunch';
Expand Down Expand Up @@ -201,4 +196,77 @@ describe('getViolationsOnyxData', () => {
expect(result.value).not.toContainEqual([missingTagViolation]);
});
});
describe('policy has multi level tags', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: can we add a line break in between describe statements?

beforeEach(() => {
policyRequiresTags = true;
policyTags = {
Department: {
name: 'Department',
tags: {
Accounting: {
name: 'Accounting',
enabled: true,
},
},
required: true,
},
Region: {
name: 'Region',
tags: {
Africa: {
name: 'Africa',
enabled: true,
},
},
},
Project: {
name: 'Project',
tags: {
Project1: {
name: 'Project1',
enabled: true,
},
},
required: true,
},
};
});
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: can we add a line break in between tests?

it('should return someTagLevelsRequired when a required tag is missing', () => {
const someTagLevelsRequiredViolation = {
name: 'someTagLevelsRequired',
type: 'violation',
data: {
errorIndexes: [0, 1, 2],
},
};

// Test case where transaction has no tags
let result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);
expect(result.value).toEqual([someTagLevelsRequiredViolation]);

// Test case where transaction has 1 tag
transaction.tag = 'Accounting';
someTagLevelsRequiredViolation.data = {errorIndexes: [1, 2]};
result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);
expect(result.value).toEqual([someTagLevelsRequiredViolation]);

// Test case where transaction has 2 tags
transaction.tag = 'Accounting::Project1';
someTagLevelsRequiredViolation.data = {errorIndexes: [1]};
result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);
expect(result.value).toEqual([someTagLevelsRequiredViolation]);

// Test case where transaction has all tags
transaction.tag = 'Accounting:Africa:Project1';
result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);
expect(result.value).toEqual([]);
});
it('should return tagOutOfPolicy when a tag is not enabled in the policy but is set in the transaction', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: can we add a line break in between tests?

policyTags.Department.tags.Accounting.enabled = false;
transaction.tag = 'Accounting:Africa:Project1';
const result = ViolationsUtils.getViolationsOnyxData(transaction, transactionViolations, policyRequiresTags, policyTags, policyRequiresCategories, policyCategories);
const violation = {...tagOutOfPolicyViolation, data: {tagName: 'Department'}};
expect(result.value).toEqual([violation]);
});
});
});
Loading