-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Add receipt requirement columns to category spreadsheet import #88962
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
Changes from all commits
d101b53
5be506e
ed9ea6f
0023995
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,33 @@ import SCREENS from '@src/SCREENS'; | |
| import type {Errors} from '@src/types/onyx/OnyxCommon'; | ||
| import isLoadingOnyxValue from '@src/types/utils/isLoadingOnyxValue'; | ||
|
|
||
| /** | ||
| * Parses a CSV cell value for receipt requirement columns. | ||
| * Mirrors the OD import logic: "required"/"always_required" → 0, | ||
| * "not_required" → DISABLED_MAX_EXPENSE_VALUE, numeric string → number. | ||
| * Returns undefined for unmapped columns, empty/default values, or invalid input. | ||
| */ | ||
| function parseCsvReceiptValue(raw: string | undefined): number | undefined { | ||
| if (raw === undefined) { | ||
| return undefined; | ||
| } | ||
| const trimmed = raw.trim().toLowerCase(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-2 (docs)The string literals Suggested fix: Define named constants and reference them: // In CONST
CATEGORY_RECEIPT_CSV_VALUES: {
DEFAULT: 'default',
REQUIRED: 'required',
ALWAYS_REQUIRED: 'always_required',
NOT_REQUIRED: 'not_required',
},Then use Reviewed at: 5be506e | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| if (!trimmed || trimmed === 'default') { | ||
| return undefined; | ||
| } | ||
| if (trimmed === 'required' || trimmed === 'always_required') { | ||
| return 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❌ CONSISTENCY-2 (docs)The magic number Suggested fix: // In CONST
ALWAYS_REQUIRE_RECEIPTS_VALUE: 0,Then use Reviewed at: 5be506e | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| } | ||
| if (trimmed === 'not_required') { | ||
| return CONST.DISABLED_MAX_EXPENSE_VALUE; | ||
| } | ||
| const num = Number(trimmed); | ||
| if (Number.isFinite(num) && num >= 0) { | ||
| return num; | ||
| } | ||
|
Comment on lines
+46
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this value should never be a number; I think if it is we should probably show an error? Or did we discuss this before haha.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do we do for other values if not in the expected format? Do we handle that on the backend, or first in App?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could maybe just treat anything but 0 as required?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, our BE still supports submitting values greater than 0, but if it’s not the MAX_AMOUNT_NO_RECEIPT value, it still returns the default value. If we don't have a feature to update the default value I think we can return 0 in this case CleanShot.2026-05-07.at.08.32.46.1.mp4
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh okay if we do that on the backend, it's not a huge deal. I think this is fine for now, we can always update later if it ends up being unexpected. |
||
| return undefined; | ||
| } | ||
|
|
||
| type ImportedCategoriesPageProps = { | ||
| route: RouteProp<SettingsNavigatorParamList, typeof SCREENS.WORKSPACE.DYNAMIC_CATEGORIES_IMPORTED | typeof SCREENS.SETTINGS_CATEGORIES.SETTINGS_CATEGORIES_IMPORTED>; | ||
| }; | ||
|
|
@@ -52,7 +79,11 @@ function ImportedCategoriesPage({route}: ImportedCategoriesPageProps) { | |
| ); | ||
|
|
||
| if (isControlPolicy(policy)) { | ||
| roles.push({text: translate('workspace.categories.glCode'), value: CONST.CSV_IMPORT_COLUMNS.GL_CODE}); | ||
| roles.push( | ||
| {text: translate('workspace.categories.glCode'), value: CONST.CSV_IMPORT_COLUMNS.GL_CODE}, | ||
| {text: translate('workspace.rules.categoryRules.requireReceiptsOver'), value: CONST.CSV_IMPORT_COLUMNS.MAX_AMOUNT_NO_RECEIPT}, | ||
| {text: translate('workspace.rules.categoryRules.requireItemizedReceiptsOver'), value: CONST.CSV_IMPORT_COLUMNS.MAX_AMOUNT_NO_ITEMIZED_RECEIPT}, | ||
| ); | ||
| } | ||
|
|
||
| return roles; | ||
|
|
@@ -99,17 +130,29 @@ function ImportedCategoriesPage({route}: ImportedCategoriesPageProps) { | |
| const categoriesNamesColumn = columns.findIndex((column) => column === CONST.CSV_IMPORT_COLUMNS.NAME); | ||
| const categoriesGLCodeColumn = columns.findIndex((column) => column === CONST.CSV_IMPORT_COLUMNS.GL_CODE); | ||
| const categoriesEnabledColumn = columns.findIndex((column) => column === CONST.CSV_IMPORT_COLUMNS.ENABLED); | ||
| const categoriesMaxAmountNoReceiptColumn = columns.findIndex((column) => column === CONST.CSV_IMPORT_COLUMNS.MAX_AMOUNT_NO_RECEIPT); | ||
| const categoriesMaxAmountNoItemizedReceiptColumn = columns.findIndex((column) => column === CONST.CSV_IMPORT_COLUMNS.MAX_AMOUNT_NO_ITEMIZED_RECEIPT); | ||
| const categoriesNames = spreadsheet?.data[categoriesNamesColumn].map((name) => name); | ||
| const categoriesEnabled = categoriesEnabledColumn !== -1 ? spreadsheet?.data[categoriesEnabledColumn].map((enabled) => enabled) : []; | ||
| const categoriesGLCode = categoriesGLCodeColumn !== -1 ? spreadsheet?.data[categoriesGLCodeColumn].map((glCode) => glCode) : []; | ||
| const categoriesMaxAmountNoReceipt = categoriesMaxAmountNoReceiptColumn !== -1 ? spreadsheet?.data[categoriesMaxAmountNoReceiptColumn] : []; | ||
| const categoriesMaxAmountNoItemizedReceipt = categoriesMaxAmountNoItemizedReceiptColumn !== -1 ? spreadsheet?.data[categoriesMaxAmountNoItemizedReceiptColumn] : []; | ||
| const categories = categoriesNames?.slice(containsHeader ? 1 : 0).map((name, index) => { | ||
| const categoryAlreadyExists = policyCategories?.[name]; | ||
| const existingGLCodeOrDefault = categoryAlreadyExists?.['GL Code'] ?? ''; | ||
| const dataIndex = containsHeader ? index + 1 : index; | ||
|
|
||
| const parsedMaxAmountNoReceipt = categoriesMaxAmountNoReceiptColumn !== -1 ? parseCsvReceiptValue(categoriesMaxAmountNoReceipt?.[dataIndex]?.toString()) : undefined; | ||
| const parsedMaxAmountNoItemizedReceipt = | ||
| categoriesMaxAmountNoItemizedReceiptColumn !== -1 ? parseCsvReceiptValue(categoriesMaxAmountNoItemizedReceipt?.[dataIndex]?.toString()) : undefined; | ||
|
|
||
| return { | ||
| name, | ||
| enabled: categoriesEnabledColumn !== -1 ? ['true', 'yes'].includes(categoriesEnabled?.[containsHeader ? index + 1 : index]?.toString().trim().toLowerCase() ?? '') : true, | ||
| enabled: categoriesEnabledColumn !== -1 ? ['true', 'yes'].includes(categoriesEnabled?.[dataIndex]?.toString().trim().toLowerCase() ?? '') : true, | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| 'GL Code': categoriesGLCodeColumn !== -1 ? (categoriesGLCode?.[containsHeader ? index + 1 : index] ?? '') : existingGLCodeOrDefault, | ||
| 'GL Code': categoriesGLCodeColumn !== -1 ? (categoriesGLCode?.[dataIndex] ?? '') : existingGLCodeOrDefault, | ||
| ...(parsedMaxAmountNoReceipt !== undefined && {maxAmountNoReceipt: parsedMaxAmountNoReceipt}), | ||
| ...(parsedMaxAmountNoItemizedReceipt !== undefined && {maxAmountNoItemizedReceipt: parsedMaxAmountNoItemizedReceipt}), | ||
| }; | ||
| }); | ||
|
|
||
|
|
||
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.
@MelvinBot Let’s fix formattedHeader in the findColumnName function, since Str.removeSpaces doesn’t replace all spaces. The current runtime value is
itemisedreceipt requirement.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.
Fixed — replaced
Str.removeSpaceswith.replaceAll(' ' , ' ' )and removed the unusedexpensify-commonimport.