-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Implement ViolationUtils lib #31656
Merged
Merged
Implement ViolationUtils lib #31656
Changes from 12 commits
Commits
Show all changes
78 commits
Select commit
Hold shift + click to select a range
f251978
feat(Violations): Add PolicyCategories type to match PolicyTags
trevor-coleman 51c2e7e
feat(Violations): Create stub for ViolationUtils
trevor-coleman f001066
feat(Violations): Add TRANSACTION_VIOLATION to ONYXKEYS.COLLECTIONS
trevor-coleman 2705280
feat(Violations): Add TransactionViolation and related types to src/t…
trevor-coleman e2aaa6a
feat(Violations): create possibleViolationsByField lookup
trevor-coleman 7f39f5a
feat(Violations): implement getViolationForField and getViolationsOny…
trevor-coleman ccb4206
prettier
trevor-coleman 6a67fb8
Merge pull request #73 from infinitered/trevorcoleman/violations/viol…
trevor-coleman 0a04347
feat(Violations): remove unnecessary callback
trevor-coleman 15f43ba
Merge remote-tracking branch 'origin/main' into trevorcoleman/violati…
trevor-coleman 575ea9b
update package-lock.json
trevor-coleman 17ef290
Merge remote-tracking branch 'upstream/main' into trevorcoleman/viola…
trevor-coleman cf65dbe
Merge remote-tracking branch 'upstream/main' into violation-utils
trevor-coleman 0353afa
restore package-lock.json
trevor-coleman 8c16f79
Merge remote-tracking branch 'upstream/main' into violation-utils
trevor-coleman 1e2e7c8
Update src/types/onyx/TransactionViolation.ts
trevor-coleman 0c8d067
Update src/libs/Violations/ViolationsUtils.ts
trevor-coleman 434fd9f
Update src/types/onyx/TransactionViolation.ts
trevor-coleman d251b9a
Update src/types/onyx/TransactionViolation.ts
trevor-coleman 19b11b1
Merge branch 'Expensify:main' into violation-utils
trevor-coleman 7f83963
feat(Violations): remove ViolationType, sort violation names
trevor-coleman e4a56b9
feat(Violations): add dummy translations for complete list
trevor-coleman 12a2aa3
feat(Violations): remove barrel file
trevor-coleman 12682bb
feat(Violations): extract getViolationsForField into useViolations hook
trevor-coleman d7ca133
Merge branch 'violation-utils' into trevorcoleman/violations/violatio…
trevor-coleman 58ea8d8
feat(Violations): export violationFields for testing
trevor-coleman 8f951e0
feat(Violations): add tests for useViolations
trevor-coleman 04c23f6
feat(Violations): remove comments on arguments and replace with JSDoc
trevor-coleman d1c8792
Merge remote-tracking branch 'origin/trevorcoleman/violations/violati…
trevor-coleman ddbe029
feat(Violations): lint
trevor-coleman 7054d05
package-lock
trevor-coleman 532c96a
prettier
trevor-coleman 6568133
feat(Violations): remove possibleViolationsByField
trevor-coleman 6aef0ba
Merge pull request #77 from infinitered/trevorcoleman/violations/viol…
trevor-coleman dfb444b
feat(Violations): update violationFields
trevor-coleman ae8acc5
feat(Violations): update translations for violations
trevor-coleman a796c2c
feat(Violations): update spanish translations for violations
trevor-coleman f2792e5
feat(Violations): create ViolationUtilsTest with stubs.
trevor-coleman 3710f82
feat(Violations): fix missing tags and categories logic
trevor-coleman 5158f20
feat(Violations): add tests for ViolationUtils
trevor-coleman eaa9c85
feat(Violations): remove test for case sensitivity
trevor-coleman 4fcc994
feat(Violations): add tests for ViolationUtils
trevor-coleman a66d253
feat(Violations): return dummy translations
trevor-coleman f504508
feat(Violations): remove unused imports
trevor-coleman ae1731b
feat(Violations): memoize return values for hasViolations, and return…
trevor-coleman de45660
feat(Violations): update tests for new return values.
trevor-coleman e239e41
Merge pull request #78 from infinitered/trevor-coleman/violations/vio…
trevor-coleman 2b27509
feat(Violations): remove index
trevor-coleman 47afbe9
feat(Violations): add quotes
trevor-coleman bc3b872
feat(Violations): Remove unnecessary casts
trevor-coleman 9fd9bf9
feat(Violations): capitalize
trevor-coleman e72bacc
feat(Violations): Add condition
trevor-coleman 2de9615
feat(Violations): Remove cast
trevor-coleman 478f934
feat(Violations): add condition
trevor-coleman ef5ebb3
feat(Violations): remove unused types
trevor-coleman 68c5485
feat(Violations): harmonize tag and category handling
trevor-coleman 75f003a
feat(Violations): reorder types
trevor-coleman 016b438
feat(Violations): Apply suggestions from code review
trevor-coleman 9c356f2
feat(Violations): remove hasViolationsMap
trevor-coleman 9b0c88a
feat(Violations): refactor useViolations tests
trevor-coleman e0d8d97
feat(Violations): refactor ViolationUtils tests
trevor-coleman 96544c9
Merge remote-tracking branch 'origin/violation-utils' into violation-…
trevor-coleman 06b7012
Merge branch 'main' into violation-utils
trevor-coleman ebb5751
feat(Violations): revert package.json
trevor-coleman 74b80df
prettier
trevor-coleman d3be1f6
feat(Violations): remove execute
trevor-coleman 4ab9931
feat(Violations): refactor tests
trevor-coleman 3737752
feat(Violations): refactor tests
trevor-coleman 1876a0b
feat(Violations): refactor tests
trevor-coleman 72667ec
feat(Violations): refactor tests to have simpler base case
trevor-coleman 27cfac1
Update tests/unit/ViolationUtilsTest.js
trevor-coleman e5ae614
feat(Violations): Suggested changes from PR Review
trevor-coleman 8498e80
feat(Violations): remove useViolationsTest
trevor-coleman e6e7612
feat(Violations): fixes from PR Comments
trevor-coleman 0aa36b8
Merge remote-tracking branch 'origin/violation-utils' into violation-…
trevor-coleman 391822c
prettier
trevor-coleman dd48fe1
Merge remote-tracking branch 'upstream/main' into violation-utils
trevor-coleman d2cfd33
feat(Violations): remove useViolationsTest
trevor-coleman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
import reject from 'lodash/reject'; | ||
import Onyx from 'react-native-onyx'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import {PolicyCategories, PolicyTags, Transaction, TransactionViolation} from '@src/types/onyx'; | ||
import possibleViolationsByField, {ViolationField} from './possibleViolationsByField'; | ||
|
||
const ViolationsUtils = { | ||
getViolationForField(transactionViolations: TransactionViolation[], field: ViolationField, translate: (key: string) => string): string[] { | ||
return transactionViolations.filter((violation) => possibleViolationsByField[field]?.includes(violation.name)).map((violation) => translate(violation.name)); | ||
}, | ||
|
||
getViolationsOnyxData( | ||
/** The transaction to check for policy violations. */ | ||
transaction: Transaction, | ||
/** An array of existing transaction violations. */ | ||
transactionViolations: TransactionViolation[], | ||
/** Indicates if the policy requires tags. */ | ||
policyRequiresTags: boolean, | ||
/** Collection of policy tags and their enabled states. */ | ||
policyTags: PolicyTags, | ||
/** Indicates if the policy requires categories. */ | ||
policyRequiresCategories: boolean, | ||
/** Collection of policy categories and their enabled states. */ | ||
policyCategories: PolicyCategories, | ||
): { | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
onyxMethod: string; | ||
key: string; | ||
value: TransactionViolation[]; | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} { | ||
let newTransactionViolations = [...transactionViolations]; | ||
|
||
if (policyRequiresCategories) { | ||
const categoryViolationExists = transactionViolations.some((violation) => violation.name === 'categoryOutOfPolicy'); | ||
const categoryIsInPolicy = policyCategories[transaction.category]?.enabled; | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Add 'categoryOutOfPolicy' violation if category is not in policy | ||
if (!categoryViolationExists && transaction.category && !categoryIsInPolicy) { | ||
newTransactionViolations.push({name: 'categoryOutOfPolicy', type: 'violation', userMessage: ''}); | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// Remove 'missingCategory' violation if category is valid according to policy | ||
if (categoryIsInPolicy) { | ||
newTransactionViolations = reject(newTransactionViolations, {name: 'missingCategory'}); | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
if (policyRequiresTags) { | ||
// Add 'tagOutOfPolicy' violation if tag is not in policy | ||
const tagViolationExists = transactionViolations.some((violation) => violation.name === 'tagOutOfPolicy'); | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const tagInPolicy = policyTags[transaction.tag]?.enabled; | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!tagViolationExists && transaction.tag && !tagInPolicy) { | ||
newTransactionViolations.push({name: 'tagOutOfPolicy', type: 'violation', userMessage: ''}); | ||
} | ||
|
||
// Remove 'missingTag' violation if tag is valid according to policy | ||
if (tagInPolicy) { | ||
newTransactionViolations = reject(newTransactionViolations, {name: 'missingTag'}); | ||
} | ||
} | ||
|
||
return { | ||
onyxMethod: Onyx.METHOD.SET, | ||
key: `${ONYXKEYS.COLLECTION.TRANSACTION_VIOLATIONS}${transaction.transactionID}`, | ||
value: newTransactionViolations, | ||
}; | ||
}, | ||
}; | ||
|
||
export default ViolationsUtils; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import * as ViolationsUtils from './ViolationsUtils'; | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export default ViolationsUtils; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import invertBy from 'lodash/invertBy'; | ||
import {ViolationName} from '@src/types/onyx'; | ||
|
||
/** | ||
* Map from Violation Names to the field where that violation can occur | ||
*/ | ||
const violationFields: Record<ViolationName, ViolationField> = { | ||
perDayLimit: 'amount', | ||
maxAge: 'date', | ||
overLimit: 'amount', | ||
overLimitAttendee: 'amount', | ||
overCategoryLimit: 'amount', | ||
receiptRequired: 'receipt', | ||
missingCategory: 'category', | ||
categoryOutOfPolicy: 'category', | ||
missingTag: 'tag', | ||
tagOutOfPolicy: 'tag', | ||
missingComment: 'comment', | ||
taxRequired: 'tax', | ||
taxOutOfPolicy: 'tax', | ||
billableExpense: 'billable', | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
/** | ||
* Names of Fields where violations can occur | ||
*/ | ||
type ViolationField = 'merchant' | 'amount' | 'category' | 'date' | 'tag' | 'comment' | 'billable' | 'receipt' | 'tax'; | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* Map from field name to array of violation types that can occur on that field. | ||
* @example | ||
* { | ||
* // ... | ||
* category: ['missingCategory', 'categoryOutOfPolicy'] | ||
* // ... | ||
* } | ||
*/ | ||
const possibleViolationsByField = invertBy(violationFields) as Record<ViolationField, ViolationName[]>; | ||
|
||
export default possibleViolationsByField; | ||
export type {ViolationField}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
/** | ||
* @module TransactionViolation | ||
* @description Transaction Violation | ||
*/ | ||
|
||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* Names of the various Transaction Violation types | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
type ViolationName = | ||
| 'perDayLimit' | ||
| 'maxAge' | ||
| 'overLimit' | ||
| 'overLimitAttendee' | ||
| 'overCategoryLimit' | ||
| 'receiptRequired' | ||
| 'missingCategory' | ||
| 'categoryOutOfPolicy' | ||
| 'missingTag' | ||
| 'tagOutOfPolicy' | ||
| 'missingComment' | ||
| 'taxRequired' | ||
| 'taxOutOfPolicy' | ||
| 'billableExpense'; | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
type ViolationType = string; | ||
trevor-coleman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
type TransactionViolation = { | ||
type: ViolationType; | ||
name: ViolationName; | ||
userMessage: string; | ||
data?: Record<string, string>; | ||
}; | ||
|
||
export type {TransactionViolation, ViolationName, ViolationType}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not a huge fan of the extra complexity added by
possibleViolationsByField
, and I suggest we get rid of it.In practice, it'll turn this line into something like this, which is just as simple
Let's also get rid of
src/libs/Violations/possibleViolationsByField.ts
and move that logic into this fileThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't like using lodash/invertBy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this was all a bit clunky, but as I used it in components I found a better solution. I was going to include it as part of my PR on the MoneyRequestView issue, but I will bring the changes into this branch so we can look at it here.