-
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
[OldDot Rules Migration] Individual expense rules #47409
[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! |
src/ONYXKEYS.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe maxExpenseAgeDefaultValue
?
let age; | ||
if (!maxExpenseAge) { | ||
age = CONST.DISABLED_MAX_EXPENSE_VALUE; | ||
} | ||
age = parseInt(maxExpenseAge, 10); |
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.
maybe ternary?
|
||
type RulesReceiptRequiredAmountPageProps = StackScreenProps<SettingsNavigatorParamList, typeof SCREENS.WORKSPACE.RULES_RECEIPT_REQUIRED_AMOUNT>; | ||
|
||
function RulesReceiptRequiredAmountPage({route}: RulesReceiptRequiredAmountPageProps) { |
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.
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? |
src/languages/es.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.', |
src/languages/es.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.', |
src/languages/es.ts
Outdated
maxAge: 'Edad máxima', | ||
maxExpenseAge: 'Antigüedad máxima de gasto', |
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.
maxAge: 'Edad máxima', | |
maxExpenseAge: 'Antigüedad máxima de gasto', | |
maxAge: 'Antigüedad máxima', | |
maxExpenseAge: 'Antigüedad máxima de los gastos', |
src/languages/es.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.', |
src/languages/es.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'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', |
src/languages/es.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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', |
src/languages/es.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nonBillableDescription: 'Los gastos ocasionalmente se vuelven a facturar a los clientes', | |
nonBillableDescription: 'Los gastos se vuelven a facturar a los clientes en ocasiones', |
src/languages/es.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Miding those comments: #comment_1 and #comment_2 everything else LGTM.
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.
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 🚀
|
@@ -402,6 +407,17 @@ function BaseTextInput( | |||
defaultValue={defaultValue} | |||
markdownStyle={markdownStyle} | |||
/> | |||
{!!suffixCharacter && ( |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
canUseWorkspaceRules
orcanUseAllBetas
function inPermissions.ts
to always returntrue
.Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.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