[OldDot Rules Migration] Individual expense rules#47409
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
| RULES_REQUIRED_RECEIPT_AMOUNT_FORM: 'RulesRequiredReceiptAmountForm', | ||
| RULES_MAX_EXPENSE_AMOUNT_FORM: 'RulesMaxExpenseAmountForm', | ||
| RULES_MAX_EXPENSE_AGE_FORM: 'RulesMaxExpenseAgeForm', |
There was a problem hiding this comment.
I've been using _MODAL_FORM in the naming - I'd suggest unifying it, but there are many cases where a standard _FORM has been applied, so it can go either way 🤷
| label?: string; | ||
|
|
||
| displayAsTextInput?: boolean; |
There was a problem hiding this comment.
I'll add descriptions for both in my PR
| renderSubtitle={() => ( | ||
| <Text style={[styles.flexRow, styles.alignItemsCenter, styles.w100, styles.mt2]}> | ||
| <Text style={[styles.textNormal, styles.colorMuted]}>{translate('workspace.rules.individualExpenseRules.subtitle')}</Text>{' '} | ||
| <TextLink | ||
| style={styles.link} | ||
| onPress={handleOnPressCategoriesLink} | ||
| > | ||
| {translate('workspace.common.categories').toLowerCase()} | ||
| </TextLink>{' '} | ||
| <Text style={[styles.textNormal, styles.colorMuted]}>{translate('common.and')}</Text>{' '} | ||
| <TextLink | ||
| style={styles.link} | ||
| onPress={handleOnPressTagsLink} | ||
| > | ||
| {translate('workspace.common.tags').toLowerCase()} | ||
| </TextLink> | ||
| . | ||
| </Text> | ||
| )} |
There was a problem hiding this comment.
I'd move this to a separate function or component
| <MenuItemWithTopDescription | ||
| key={translate('workspace.rules.individualExpenseRules.receiptRequiredAmount')} | ||
| shouldShowRightIcon | ||
| title={maxExpenseAmountNoReceiptText} | ||
| description={translate('workspace.rules.individualExpenseRules.receiptRequiredAmount')} | ||
| onPress={() => { | ||
| Navigation.navigate(ROUTES.RULES_RECEIPT_REQUIRED_AMOUNT.getRoute(policyID)); | ||
| }} | ||
| wrapperStyle={[styles.sectionMenuItemTopDescription]} | ||
| numberOfLinesTitle={2} | ||
| /> |
There was a problem hiding this comment.
In my PR moving item content to the array was more necessary, because I had a lot of nested items, but here most of the items have very similar configurations and this would benefit readability a lot because we'd avoid repeating the components and this would be easier to expand in the future
|
|
||
| return ( | ||
| <AccessOrNotFoundWrapper | ||
| policyID={route.params.policyID ?? '-1'} |
There was a problem hiding this comment.
is '-1' required? I think it was accepting undefined value
| const {inputCallbackRef} = useAutoFocusInput(); | ||
| const policy = usePolicy(route.params.policyID); | ||
|
|
||
| const defaultValue = policy?.maxExpenseAge === CONST.DISABLED_MAX_EXPENSE_VALUE || !policy?.maxExpenseAge ? '' : `${policy?.maxExpenseAge}`; |
There was a problem hiding this comment.
maybe maxExpenseAgeDefaultValue?
| let age; | ||
| if (!maxExpenseAge) { | ||
| age = CONST.DISABLED_MAX_EXPENSE_VALUE; | ||
| } | ||
| age = parseInt(maxExpenseAge, 10); |
|
|
||
| type RulesReceiptRequiredAmountPageProps = StackScreenProps<SettingsNavigatorParamList, typeof SCREENS.WORKSPACE.RULES_RECEIPT_REQUIRED_AMOUNT>; | ||
|
|
||
| function RulesReceiptRequiredAmountPage({route}: RulesReceiptRequiredAmountPageProps) { |
There was a problem hiding this comment.
you're using route.params.policyID in many places, can't we destructure it on the props level?
|
I have one question. When we upgrade the policy, default max expense amounts are set: Is this code still valid or should we set all these values to 10000000000? |
| receiptRequiredAmount: 'Monto requerido para recibo', | ||
| receiptRequiredAmountDescription: 'Requiere recibos cuando el gasto excede este monto, a menos que una regla de categoría lo anule.', |
There was a problem hiding this comment.
| receiptRequiredAmount: 'Monto requerido para recibo', | |
| receiptRequiredAmountDescription: 'Requiere recibos cuando el gasto excede este monto, a menos que una regla de categoría lo anule.', | |
| receiptRequiredAmount: 'Cantidad requerida para los recibos', | |
| receiptRequiredAmountDescription: 'Exige recibos cuando los gastos superen este importe, a menos que lo anule una regla de categoría.', |
| maxExpenseAmount: 'Monto máximo de gasto', | ||
| maxExpenseAmountDescription: 'Marca el gasto que excede este monto, a menos que una regla de categoría lo anule.', |
There was a problem hiding this comment.
| maxExpenseAmount: 'Monto máximo de gasto', | |
| maxExpenseAmountDescription: 'Marca el gasto que excede este monto, a menos que una regla de categoría lo anule.', | |
| maxExpenseAmount: 'Importe máximo del gasto', | |
| maxExpenseAmountDescription: 'Marca los gastos que superen este importe, a menos que una regla de categoría lo anule.', |
| maxAge: 'Edad máxima', | ||
| maxExpenseAge: 'Antigüedad máxima de gasto', |
There was a problem hiding this comment.
| maxAge: 'Edad máxima', | |
| maxExpenseAge: 'Antigüedad máxima de gasto', | |
| maxAge: 'Antigüedad máxima', | |
| maxExpenseAge: 'Antigüedad máxima de los gastos', |
| maxExpenseAmountDescription: 'Marca el gasto que excede este monto, a menos que una regla de categoría lo anule.', | ||
| maxAge: 'Edad máxima', | ||
| maxExpenseAge: 'Antigüedad máxima de gasto', | ||
| maxExpenseAgeDescription: 'Marca los gastos con más de un número específico de días.', |
There was a problem hiding this comment.
| maxExpenseAgeDescription: 'Marca los gastos con más de un número específico de días.', | |
| maxExpenseAgeDescription: 'Marca los gastos de más de un número determinado de días.', |
| maxExpenseAgeDays: (age: number) => `${age} ${Str.pluralize('día', 'días', age)}`, | ||
| billableDefault: 'Valor predeterminado facturable', | ||
| billableDefaultDescription: | ||
| 'Elige si los gastos en efectivo y con tarjeta de crédito deben ser facturables por defecto. Los gastos facturables se habilitan o deshabilitan en etiquetas', |
There was a problem hiding this comment.
| 'Elige si los gastos en efectivo y con tarjeta de crédito deben ser facturables por defecto. Los gastos facturables se habilitan o deshabilitan en etiquetas', | |
| 'Elige si los gastos en efectivo y con tarjeta de crédito deben ser facturables por defecto. Los gastos facturables se activan o desactivan en', |
| billableDefaultDescription: | ||
| 'Elige si los gastos en efectivo y con tarjeta de crédito deben ser facturables por defecto. Los gastos facturables se habilitan o deshabilitan en etiquetas', | ||
| billable: 'Facturable', | ||
| billableDescription: 'Los gastos se vuelven a facturar a los clientes con mayor frecuencia', |
There was a problem hiding this comment.
| billableDescription: 'Los gastos se vuelven a facturar a los clientes con mayor frecuencia', | |
| billableDescription: 'Los gastos se vuelven a facturar a los clientes en la mayoría de los casos', |
| billable: 'Facturable', | ||
| billableDescription: 'Los gastos se vuelven a facturar a los clientes con mayor frecuencia', | ||
| nonBillable: 'No facturable', | ||
| nonBillableDescription: 'Los gastos ocasionalmente se vuelven a facturar a los clientes', |
There was a problem hiding this comment.
| nonBillableDescription: 'Los gastos ocasionalmente se vuelven a facturar a los clientes', | |
| nonBillableDescription: 'Los gastos se vuelven a facturar a los clientes en ocasiones', |
| eReceiptsHint: 'El Recibos electrónicos se crean automáticamente para', | ||
| eReceiptsHintLink: 'la mayoría de las transacciones con crédito en USD', |
There was a problem hiding this comment.
| eReceiptsHint: 'El Recibos electrónicos se crean automáticamente para', | |
| eReceiptsHintLink: 'la mayoría de las transacciones con crédito en USD', | |
| eReceiptsHint: 'Los recibos electrónicos se crean automáticamente', | |
| eReceiptsHintLink: 'para la mayoría de las transacciones en USD', |
|
Screenshots look pretty good to me! |
@WojtekBoman What you have there appears to be correct. When the API is called to upgrade to corporate those are the default values we set. |
| SET_POLICY_EXPENSE_MAX_AMOUNT: 'SetPolicyExpenseMaxAmount', | ||
| SET_POLICY_EXPENSE_MAX_AGE: ' SetPolicyExpenseMaxAge', | ||
| SET_POLICY_BILLABLE_MODE: ' SetPolicyBillableMode', | ||
| SET_WORKSPACE_ERECEIPTS_ENABLED: 'SetWorkspaceEReceiptsEnabled', |
There was a problem hiding this comment.
Note to myself that I will create some issue to fix this as polish.
|
Looks great so far! |
This comment was marked as outdated.
This comment was marked as outdated.
I mentioned it here. When we upgrade our policy, the default values for rules are set |
Sorry, I missed that. I'm testing on all platforms and will fill out the checklist. |
|
Excuse @WojtekBoman, what should happen when we save a 'max expense age' equal to the maximum limit? Thanks. Screen.Recording.2024-08-26.at.10.29.55.mov |
I asked about it here @marcaaron Could you take a look at it? |
Reviewer Checklist
Screenshots/VideosAndroid: Native47409_android_native.movAndroid: mWeb Chrome47409_android_web.moviOS: Native47409_ios_native.moviOS: mWeb Safari47409_ios_web.movMacOS: Chrome / Safari47409_browser.movMacOS: Desktop47409_desktop.mov |
|
I can reproduce this issue in the 'receipt required amount' and 'max expense amount' fields (in native only). 47409_dupe_issue_native.mov |
brunovjk
left a comment
There was a problem hiding this comment.
Miding those comments: #comment_1 and #comment_2 everything else LGTM.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.0.26-1 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.0.26-6 🚀
|
| defaultValue={defaultValue} | ||
| markdownStyle={markdownStyle} | ||
| /> | ||
| {!!suffixCharacter && ( |
| const policyCurrency = policy?.outputCurrency ?? CONST.CURRENCY.USD; | ||
|
|
||
| const maxExpenseAmountNoReceiptText = useMemo(() => { | ||
| if (policy?.maxExpenseAmountNoReceipt === CONST.DISABLED_MAX_EXPENSE_VALUE || !policy?.maxExpenseAmountNoReceipt) { |
There was a problem hiding this comment.
We shouldn't had returned empty string if the amount was 0, this caused #54290

Details
This PR adds a new section to the Rules Page:
Individual expense rules. This section allows the user to:Implementation details on how the actions listed above work can be found here
Fixed Issues
$ #47013
PROPOSAL:
Tests
Precondition: Make sure you have access to the workspaceRules beta. If you run the app locally you can modify the
canUseWorkspaceRulesorcanUseAllBetasfunction inPermissions.tsto always returntrue.Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2024-08-23.at.12.38.25.mp4
Android: mWeb Chrome
Screen.Recording.2024-08-23.at.12.43.07.mp4
iOS: Native
Screen.Recording.2024-08-23.at.11.50.35.mp4
iOS: mWeb Safari
Screen.Recording.2024-08-23.at.12.34.35.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-08-23.at.14.42.29.mov
MacOS: Desktop
Screen.Recording.2024-08-23.at.11.45.01.mov