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 11 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
7 changes: 6 additions & 1 deletion src/components/ReportActionItem/MoneyRequestView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -467,11 +467,16 @@ function MoneyRequestView({
getErrorForField('tag', {
tagListIndex: index,
tagListName: name,
policyHasDependentTags: PolicyUtils.hasDependentTags(policy, policyTagList),
})
? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR
: undefined
}
error={getErrorForField('tag', {tagListIndex: index, tagListName: name})}
error={getErrorForField('tag', {
tagListIndex: index,
tagListName: name,
policyHasDependentTags: PolicyUtils.hasDependentTags(policy, policyTagList),
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
})}
/>
</OfflineWithFeedback>
))}
Expand Down
27 changes: 21 additions & 6 deletions src/hooks/useViolations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,24 +63,39 @@ function useViolations(violations: TransactionViolation[]) {
const getViolationsForField = useCallback(
(field: ViolationField, data?: TransactionViolation['data']) => {
const currentViolations = violationsByField.get(field) ?? [];
const violation = currentViolations[0];
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved

// 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 (violation?.name === 'someTagLevelsRequired' && data?.tagListIndex !== undefined && Array.isArray(violation?.data?.errorIndexes)) {
return currentViolations
.filter((violation) => violation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1))
.map((violation) => ({
.filter((currentViolation) => currentViolation.data?.errorIndexes?.includes(data?.tagListIndex ?? -1))
.map((currentViolation) => ({
...currentViolation,
data: {
...currentViolation.data,
tagName: data?.tagListName,
},
}));
}

// missingTag has special logic for policies with dependent tags, because only violation is returned for all tags
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
// when no tags are present, so the tag name isn't set in the violation data. That's why we add it here
if (data?.policyHasDependentTags && violation?.name === 'missingTag' && data?.tagListName) {
return [
{
...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);
if (violation?.name === 'tagOutOfPolicy' && data?.tagListName !== undefined && violation?.data?.tagName) {
return currentViolations.filter((currentViolation) => currentViolation.data?.tagName === data?.tagListName);
}

return currentViolations;
Expand Down
8 changes: 8 additions & 0 deletions src/libs/PolicyUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,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));
}

export {
getActivePolicies,
hasAccountingConnections,
Expand Down Expand Up @@ -435,6 +442,7 @@ export {
getPolicy,
getActiveAdminWorkspaces,
canSendInvoice,
hasDependentTags,
};

export type {MemberEmailsToAccountIDs};
62 changes: 49 additions & 13 deletions src/libs/Violations/ViolationsUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,31 +49,39 @@ function getTagViolationsForSingleLevelTags(
}

/**
* Calculates some tag levels required and missing tag violations for the given transaction
* Calculates missing tag violations for policies with dependent tags,
* by returning one per tag with its corresponding tagName in the data
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
*/
function getTagViolationsForMultiLevelTags(
updatedTransaction: Transaction,
transactionViolations: TransactionViolation[],
policyRequiresTags: boolean,
policyTagList: PolicyTagList,
): TransactionViolation[] {
const policyTagKeys = getSortedTagKeys(policyTagList);
const selectedTags = updatedTransaction.tag?.split(CONST.COLON) ?? [];
function getTagViolationsForDependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[]) {
return [
...transactionViolations,
...Object.values(policyTagList).map((tagList) => ({
name: CONST.VIOLATIONS.MISSING_TAG,
type: CONST.VIOLATION_TYPES.VIOLATION,
data: {tagName: tagList.name},
})),
];
}

/**
* Calculates missing tag violations for policies with independent tags,
* by returning one per tag with its corresponding tagName in the data
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
*/
function getTagViolationForIndependentTags(policyTagList: PolicyTagList, transactionViolations: TransactionViolation[], policyTagKeys: string[], selectedTags: string[]) {
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,
);

// 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 = [];

pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
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,
Expand All @@ -84,10 +92,12 @@ function getTagViolationsForMultiLevelTags(
});
} 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,
Expand All @@ -100,15 +110,40 @@ function getTagViolationsForMultiLevelTags(
break;
}
}

if (!hasInvalidTag) {
newTransactionViolations = reject(newTransactionViolations, {
name: CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
});
}
}

return newTransactionViolations;
}

/**
* Calculates some tag levels required and missing tag violations for the given transaction
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
*/
function getTagViolationsForMultiLevelTags(
updatedTransaction: Transaction,
transactionViolations: TransactionViolation[],
policyRequiresTags: boolean,
policyTagList: PolicyTagList,
hasDependentTags: boolean,
): TransactionViolation[] {
const policyTagKeys = getSortedTagKeys(policyTagList);
const selectedTags = updatedTransaction.tag?.split(CONST.COLON) ?? [];
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
const filteredTransactionViolations = transactionViolations.filter(
(violation) => violation.name !== CONST.VIOLATIONS.SOME_TAG_LEVELS_REQUIRED && violation.name !== CONST.VIOLATIONS.TAG_OUT_OF_POLICY,
pac-guerreiro marked this conversation as resolved.
Show resolved Hide resolved
);

if (hasDependentTags && !updatedTransaction.tag) {
return getTagViolationsForDependentTags(policyTagList, filteredTransactionViolations);
}

return getTagViolationForIndependentTags(policyTagList, filteredTransactionViolations, policyTagKeys, selectedTags);
}

const ViolationsUtils = {
/**
* Checks a transaction for policy violations and returns an object with Onyx method, key and updated transaction
Expand All @@ -121,6 +156,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 +202,7 @@ const ViolationsUtils = {
newTransactionViolations =
Object.keys(policyTagList).length === 1
? getTagViolationsForSingleLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList)
: getTagViolationsForMultiLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList);
: getTagViolationsForMultiLevelTags(updatedTransaction, newTransactionViolations, policyRequiresTags, policyTagList, hasDependentTags);
}

return {
Expand Down
22 changes: 20 additions & 2 deletions src/libs/actions/IOU.ts
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,15 @@
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 @@ -1074,7 +1082,7 @@
return [optimisticData, successData, failureData];
}

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

Check failure on line 1085 in src/libs/actions/IOU.ts

View workflow job for this annotation

GitHub Actions / typecheck

Expected 7 arguments, but got 6.

if (violationsOnyxData) {
optimisticData.push(violationsOnyxData);
Expand Down Expand Up @@ -1411,7 +1419,15 @@
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 @@ -2501,6 +2517,7 @@
policyTagList ?? {},
!!policy.requiresCategory,
policyCategories ?? {},
PolicyUtils.hasDependentTags(policy, policyTagList ?? {}),
),
);
failureData.push({
Expand Down Expand Up @@ -4878,6 +4895,7 @@
policyTags,
!!policy.requiresCategory,
policyCategories,
PolicyUtils.hasDependentTags(policy, policyTags),
);
optimisticData.push(updatedViolationsOnyxData);
failureData.push({
Expand Down
10 changes: 10 additions & 0 deletions src/types/onyx/PolicyTag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ 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
};

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
1 change: 1 addition & 0 deletions src/types/onyx/TransactionViolation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ type TransactionViolation = {
type: string;
name: ViolationName;
data?: {
policyHasDependentTags?: boolean;
rejectedBy?: string;
rejectReason?: string;
formattedLimit?: string;
Expand Down
Loading
Loading