Skip to content

Commit

Permalink
Merge pull request #40741 from callstack-internal/pac-guerreiro/issue…
Browse files Browse the repository at this point in the history 
…/38095-missing-tag-violation-for-multi-level-dependencies
  • Loading branch information
@cead22
cead22 authored Jun 10, 2024
2 parents 60530f6 + df5e19f commit 19f9424
Show file tree
Hide file tree
Showing 7 changed files with 216 additions and 71 deletions.
72 changes: 38 additions & 34 deletions src/components/ReportActionItem/MoneyRequestView.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,8 @@ function MoneyRequestView({

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

Expand Down Expand Up @@ -238,7 +239,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'], policyHasDependentTags = false, tagValue?: string) => {
// Checks applied when creating a new expense
// 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 @@ -264,14 +265,14 @@ function MoneyRequestView({
}

// Return violations if there are any
if (canUseViolations && hasViolations(field, data)) {
const violations = getViolationsForField(field, data);
if (hasViolations(field, data, policyHasDependentTags, tagValue)) {
const violations = getViolationsForField(field, data, policyHasDependentTags, tagValue);
return ViolationsUtils.getViolationTranslation(violations[0], translate);
}

return '';
},
[transactionAmount, isSettled, isCancelled, isPolicyExpenseChat, isEmptyMerchant, transactionDate, hasErrors, canUseViolations, hasViolations, translate, getViolationsForField],
[transactionAmount, isSettled, isCancelled, isPolicyExpenseChat, isEmptyMerchant, transactionDate, hasErrors, hasViolations, translate, getViolationsForField],
);

const distanceRequestFields = canUseP2PDistanceRequests ? (
Expand Down Expand Up @@ -333,6 +334,37 @@ function MoneyRequestView({
...parentReportAction?.errors,
};

const tagList = policyTagLists.map(({name, orderWeight}, index) => {
const tagError = getErrorForField(
'tag',
{
tagListIndex: index,
tagListName: name,
},
PolicyUtils.hasDependentTags(policy, policyTagList),
TransactionUtils.getTagForDisplay(transaction, index),
);
return (
<OfflineWithFeedback
key={name}
pendingAction={getPendingFieldAction('tag')}
>
<MenuItemWithTopDescription
description={name ?? translate('common.tag')}
title={TransactionUtils.getTagForDisplay(transaction, index)}
interactive={canEdit}
shouldShowRightIcon={canEdit}
titleStyle={styles.flex1}
onPress={() =>
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_TAG.getRoute(CONST.IOU.ACTION.EDIT, iouType, orderWeight, transaction?.transactionID ?? '', report.reportID))
}
brickRoadIndicator={tagError ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined}
errorText={tagError}
/>
</OfflineWithFeedback>
);
});

return (
<View style={styles.pRelative}>
{shouldShowAnimatedBackground && <AnimatedEmptyStateBackground />}
Expand Down Expand Up @@ -468,35 +500,7 @@ function MoneyRequestView({
/>
</OfflineWithFeedback>
)}
{shouldShowTag &&
policyTagLists.map(({name, orderWeight}, index) => (
<OfflineWithFeedback
key={name}
pendingAction={getPendingFieldAction('tag')}
>
<MenuItemWithTopDescription
description={name ?? translate('common.tag')}
title={TransactionUtils.getTagForDisplay(transaction, index)}
interactive={canEdit}
shouldShowRightIcon={canEdit}
titleStyle={styles.flex1}
onPress={() =>
Navigation.navigate(
ROUTES.MONEY_REQUEST_STEP_TAG.getRoute(CONST.IOU.ACTION.EDIT, iouType, orderWeight, transaction?.transactionID ?? '', report.reportID),
)
}
brickRoadIndicator={
getErrorForField('tag', {
tagListIndex: index,
tagListName: name,
})
? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR
: undefined
}
errorText={getErrorForField('tag', {tagListIndex: index, tagListName: name})}
/>
</OfflineWithFeedback>
))}
{shouldShowTag && tagList}
{isCardTransaction && (
<OfflineWithFeedback pendingAction={getPendingFieldAction('cardID')}>
<MenuItemWithTopDescription
Expand Down
26 changes: 23 additions & 3 deletions src/hooks/useViolations.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,12 @@ function useViolations(violations: TransactionViolation[]) {
}, [violations]);

const getViolationsForField = useCallback(
(field: ViolationField, data?: TransactionViolation['data']) => {
(field: ViolationField, data?: TransactionViolation['data'], policyHasDependentTags = false, tagValue?: string) => {
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)) {
if (currentViolations[0]?.name === CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && data?.tagListIndex !== undefined && Array.isArray(currentViolations[0]?.data?.errorIndexes)) {
return currentViolations
.filter((violation) => violation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1))
.map((violation) => ({
Expand All @@ -79,8 +79,28 @@ function useViolations(violations: TransactionViolation[]) {
}));
}

// missingTag has special logic for policies with dependent tags, because only one violation is returned for all tags
// when no tags are present, so the tag name isn't set in the violation data. That's why we add it here
if (policyHasDependentTags && currentViolations[0]?.name === CONST.VIOLATIONS.MISSING_TAG && data?.tagListName) {
return [
{
...currentViolations[0],
data: {
...currentViolations[0].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) {
if (currentViolations[0]?.name === CONST.VIOLATIONS.TAG_OUT_OF_POLICY && data?.tagListName !== undefined && currentViolations[0]?.data?.tagName) {
return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName);
}

// allTagLevelsRequired has special logic because it is returned when one but not all the tags are set,
// so we need to return the violation for the tag fields without a tag set
if (currentViolations[0]?.name === CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED && tagValue) {
return currentViolations.filter((violation) => violation.data?.tagName === data?.tagListName);
}

Expand Down
8 changes: 8 additions & 0 deletions src/libs/PolicyUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,13 @@ function canSendInvoice(policies: OnyxCollection<Policy>): boolean {
return getActiveAdminWorkspaces(policies).length > 0;
}

function hasDependentTags(policy: OnyxEntry<Policy>, policyTagList: OnyxEntry<PolicyTagList>) {
if (!policy?.hasMultipleTagLists) {
return false;
}
return Object.values(policyTagList ?? {}).some((tagList) => Object.values(tagList.tags).some((tag) => !!tag.rules?.parentTagsFilter || !!tag.parentTagsFilter));
}

/** Get the Xero organizations connected to the policy */
function getXeroTenants(policy: Policy | undefined): Tenant[] {
// Due to the way optional chain is being handled in this useMemo we are forced to use this approach to properly handle undefined values
Expand Down Expand Up @@ -516,6 +523,7 @@ export {
shouldShowPolicy,
getActiveAdminWorkspaces,
canSendInvoice,
hasDependentTags,
getXeroTenants,
findCurrentXeroOrganization,
getCurrentXeroOrganizationName,
Expand Down
69 changes: 59 additions & 10 deletions src/libs/Violations/ViolationsUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as TransactionUtils from '@libs/TransactionUtils';
import CONST from '@src/CONST';
import type {TranslationPaths} from '@src/languages/types';
import ONYXKEYS from '@src/ONYXKEYS';
import type {PolicyCategories, PolicyTagList, Transaction, TransactionViolation} from '@src/types/onyx';
import type {PolicyCategories, PolicyTagList, Transaction, TransactionViolation, ViolationName} from '@src/types/onyx';

/**
* Calculates tag out of policy and missing tag violations for the given transaction
Expand Down Expand Up @@ -49,17 +49,41 @@ function getTagViolationsForSingleLevelTags(
}

/**
* Calculates some tag levels required and missing tag violations for the given transaction
* Calculates missing tag violations for policies with dependent tags
*/
function getTagViolationsForMultiLevelTags(
updatedTransaction: Transaction,
transactionViolations: TransactionViolation[],
policyRequiresTags: boolean,
policyTagList: PolicyTagList,
): TransactionViolation[] {
function getTagViolationsForDependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[], tagName: string) {
const tagViolations = [...transactionViolations];

if (!tagName) {
Object.values(policyTagList).forEach((tagList) =>
tagViolations.push({
name: CONST.VIOLATIONS.MISSING_TAG,
type: CONST.VIOLATION_TYPES.VIOLATION,
data: {tagName: tagList.name},
}),
);
} else {
const tags = TransactionUtils.getTagArrayFromName(tagName);
if (Object.keys(policyTagList).length !== tags.length || tags.includes('')) {
tagViolations.push({
name: CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED,
type: CONST.VIOLATION_TYPES.VIOLATION,
data: {},
});
}
}

return tagViolations;
}

/**
* Calculates missing tag violations for policies with independent tags
*/
function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[], transaction: Transaction) {
const policyTagKeys = getSortedTagKeys(policyTagList);
const selectedTags = updatedTransaction.tag?.split(CONST.COLON) ?? [];
const selectedTags = transaction.tag?.split(CONST.COLON) ?? [];
let newTransactionViolations = [...transactionViolations];

newTransactionViolations = newTransactionViolations.filter(
(violation) => violation.name !== CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && violation.name !== CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
);
Expand Down Expand Up @@ -109,6 +133,30 @@ function getTagViolationsForMultiLevelTags(
return newTransactionViolations;
}

/**
* Calculates tag violations for a transaction on a policy with multi level tags
*/
function getTagViolationsForMultiLevelTags(
updatedTransaction: Transaction,
transactionViolations: TransactionViolation[],
policyTagList: PolicyTagList,
hasDependentTags: boolean,
): TransactionViolation[] {
const tagViolations = [
CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED,
CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
CONST.VIOLATIONS.MISSING_TAG,
CONST.VIOLATIONS.ALL_TAG_LEVELS_REQUIRED,
] as ViolationName[];
const filteredTransactionViolations = transactionViolations.filter((violation) => !tagViolations.includes(violation.name));

if (hasDependentTags) {
return getTagViolationsForDependentTags(policyTagList, filteredTransactionViolations, updatedTransaction.tag ?? '');
}

return getTagViolationForIndependentTags(policyTagList, filteredTransactionViolations, updatedTransaction);
}

const ViolationsUtils = {
/**
* Checks a transaction for policy violations and returns an object with Onyx method, key and updated transaction
Expand All @@ -121,6 +169,7 @@ const ViolationsUtils = {
policyTagList: PolicyTagList,
policyRequiresCategories: boolean,
policyCategories: PolicyCategories,
hasDependentTags: boolean,
): OnyxUpdate {
const isPartialTransaction = TransactionUtils.isPartialMerchant(TransactionUtils.getMerchant(updatedTransaction)) && TransactionUtils.isAmountMissing(updatedTransaction);
if (isPartialTransaction) {
Expand Down Expand Up @@ -166,7 +215,7 @@ const ViolationsUtils = {
newTransactionViolations =
Object.keys(policyTagList).length === 1
? getTagViolationsForSingleLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList)
: getTagViolationsForMultiLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList);
: getTagViolationsForMultiLevelTags(updatedTransaction, newTransactionViolations, policyTagList, hasDependentTags);
}

return {
Expand Down
32 changes: 29 additions & 3 deletions src/libs/actions/IOU.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,15 @@ function buildOnyxDataForMoneyRequest(
return [optimisticData, successData, failureData];
}

const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {});
const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(
transaction,
[],
!!policy.requiresTag,
policyTagList ?? {},
!!policy.requiresCategory,
policyCategories ?? {},
PolicyUtils.hasDependentTags(policy, policyTagList ?? {}),
);

if (violationsOnyxData) {
optimisticData.push(violationsOnyxData);
Expand Down Expand Up @@ -1137,7 +1145,15 @@ function buildOnyxDataForInvoice(
return [optimisticData, successData, failureData];
}

const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {});
const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(
transaction,
[],
!!policy.requiresTag,
policyTagList ?? {},
!!policy.requiresCategory,
policyCategories ?? {},
PolicyUtils.hasDependentTags(policy, policyTagList ?? {}),
);

if (violationsOnyxData) {
optimisticData.push(violationsOnyxData);
Expand Down Expand Up @@ -1506,7 +1522,15 @@ function buildOnyxDataForTrackExpense(
return [optimisticData, successData, failureData];
}

const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(transaction, [], !!policy.requiresTag, policyTagList ?? {}, !!policy.requiresCategory, policyCategories ?? {});
const violationsOnyxData = ViolationsUtils.getViolationsOnyxData(
transaction,
[],
!!policy.requiresTag,
policyTagList ?? {},
!!policy.requiresCategory,
policyCategories ?? {},
PolicyUtils.hasDependentTags(policy, policyTagList ?? {}),
);

if (violationsOnyxData) {
optimisticData.push(violationsOnyxData);
Expand Down Expand Up @@ -2677,6 +2701,7 @@ function getUpdateMoneyRequestParams(
policyTagList ?? {},
!!policy.requiresCategory,
policyCategories ?? {},
PolicyUtils.hasDependentTags(policy, policyTagList ?? {}),
),
);
failureData.push({
Expand Down Expand Up @@ -5133,6 +5158,7 @@ function editRegularMoneyRequest(
policyTags,
!!policy.requiresCategory,
policyCategories,
PolicyUtils.hasDependentTags(policy, policyTags),
);
optimisticData.push(updatedViolationsOnyxData);
failureData.push({
Expand Down
14 changes: 14 additions & 0 deletions src/types/onyx/PolicyTag.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,20 @@ type PolicyTag = OnyxCommon.OnyxValueWithOfflineFeedback<{

/** A list of errors keyed by microtime */
errors?: OnyxCommon.Errors | null;

rules?: {

Check failure on line 21 in src/types/onyx/PolicyTag.ts

View workflow job for this annotation

GitHub Actions / lint / Run ESLint 

Missing JSDoc comment
/**
* String representation of regex to match against parent tag. Eg, if San Francisco is a child tag of California
* its parentTagsFilter will be ^California$
*/
parentTagsFilter?: string;
};

/**
* String representation of regex to match against parent tag. Eg, if San Francisco is a child tag of California
* its parentTagsFilter will be ^California$
*/
parentTagsFilter?: string;
}>;

/** Record of policy tags, indexed by their name */
Expand Down
Loading

0 comments on commit 19f9424

Please sign in to comment.