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

Tags - Violation for unselected dependent tags displays tag name briefly then changes to "tag" #40741

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
43e030b
fix(money request): missing tags in violations translations
pac-guerreiro Apr 23, 2024
b9a8d90
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro Apr 23, 2024
b9d8b24
refactor: remove unexpected return
pac-guerreiro Apr 23, 2024
fcdc252
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro Apr 24, 2024
0a0cac1
chore: apply PR suggestions
pac-guerreiro Apr 24, 2024
7a3db00
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro Apr 25, 2024
a174c60
fix: tag name missing in error UI
pac-guerreiro Apr 25, 2024
47bf6f2
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro Apr 29, 2024
72f8575
refactor: apply pull request suggestions
pac-guerreiro Apr 29, 2024
5c7e16c
chore(typescript): add missing argument
pac-guerreiro Apr 29, 2024
3d6478f
chore(tests): add violation case for missing dependent tags
pac-guerreiro Apr 29, 2024
0ab716d
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro May 3, 2024
e297686
refactor: apply pull request suggestions
pac-guerreiro May 3, 2024
a8ef1c3
chore(typescript): add missing argument
pac-guerreiro May 3, 2024
5e36ed7
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro May 6, 2024
94c657c
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro May 7, 2024
b873788
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro May 13, 2024
2e4f7f8
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro May 24, 2024
d958c37
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro May 27, 2024
8558f65
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro May 29, 2024
5e20695
fix: allTagLevels required violation showing on tags that are already…
pac-guerreiro May 29, 2024
525be58
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro Jun 3, 2024
d67febb
feat: calculate all tag required violations locally
pac-guerreiro Jun 3, 2024
30189ca
refactor: apply pull request suggestions
pac-guerreiro Jun 5, 2024
4cd7802
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro Jun 7, 2024
492a672
refactor: apply suggestions
pac-guerreiro Jun 7, 2024
df3326a
refactor: remove unused argument
pac-guerreiro Jun 7, 2024
e54d678
Merge branch 'main' into pac-guerreiro/issue/38095-missing-tag-violat…
pac-guerreiro Jun 7, 2024
e7f175f
refactor: apply suggestions
pac-guerreiro Jun 7, 2024
df5e19f
refactor: apply suggestions
pac-guerreiro Jun 7, 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
72 changes: 38 additions & 34 deletions src/components/ReportActionItem/MoneyRequestView.tsx
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
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)) {
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion: currentViolations[0] is used several times so we can create a variable for this like firstViolation.

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
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
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(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed? I think we should keep it so we're not adding SOME_TAG_LEVELS_REQUIRED to an array of violations that already has it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cead22 I removed it because we're already filtering those in the function that calls this one 😅

(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
Original file line number Diff line number Diff line change
Expand Up @@ -822,7 +822,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 @@ -1136,7 +1144,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 @@ -1505,7 +1521,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 @@ -2675,6 +2699,7 @@ function getUpdateMoneyRequestParams(
policyTagList ?? {},
!!policy.requiresCategory,
policyCategories ?? {},
PolicyUtils.hasDependentTags(policy, policyTagList ?? {}),
),
);
failureData.push({
Expand Down Expand Up @@ -5130,6 +5155,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
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,20 @@ type PolicyTag = OnyxCommon.OnyxValueWithOfflineFeedback<{

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

rules?: {
/**
* 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;
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
};

/**
* 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;
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
}>;

type PolicyTags = Record<string, PolicyTag>;
Expand Down
Loading
Loading