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.

Sign up for GitHub

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

Handle the error with journal entry when tax is enabled #41520

Merged
merged 21 commits into from
May 15, 2024
Merged

Handle the error with journal entry when tax is enabled #41520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d5812cf
don not toggle switch on when export is set to jounal entry
teneeto May 2, 2024
d04a7a0
fix QBO undefined error for country check
teneeto May 2, 2024
0453720
add taxesJournalEntrySwitchNote for english and spanish translations
teneeto May 2, 2024
a2eae45
add reason why taxes can't toggle
teneeto May 2, 2024
1ae5a20
disable toggle when journal entry is selected
teneeto May 2, 2024
85a8447
translate first part taxesJournalEntrySwitchNote
teneeto May 2, 2024
0276320
add full translation for taxesJournalEntrySwitchNote
teneeto May 2, 2024
0f13f6c
Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…
teneeto May 2, 2024
f0cf4ef
fix tsc
teneeto May 2, 2024
1e17771
Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…
teneeto May 2, 2024
df6a3dd
update error field
teneeto May 3, 2024
cfa0aa1
Update src/languages/es.ts
teneeto May 3, 2024
09a2bb1
Update src/pages/workspace/accounting/qbo/import/QuickbooksTaxesPage.tsx
teneeto May 3, 2024
c1f4c3c
remove isTaxSwitchOn condition
teneeto May 3, 2024
38f7484
Journal entry should not show even when locations are enabled
teneeto May 3, 2024
54dbdf5
Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…
teneeto May 6, 2024
bbaf3b7
Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…
teneeto May 7, 2024
ccd10b0
Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…
teneeto May 9, 2024
0baa214
Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…
teneeto May 10, 2024
1861a11
Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…
teneeto May 13, 2024
b44b907
update check for disabled toggle
teneeto May 14, 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
2 changes: 2 additions & 0 deletions src/languages/en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1925,6 +1925,8 @@ export default {
customersDescription: 'Choose whether to import customers/projects and see where customers/projects are displayed.',
locationsDescription: 'Choose whether to import locations, and see where locations are displayed.',
taxesDescription: 'Choose whether to import tax rates and tax defaults from your accounting integration.',
taxesJournalEntrySwitchNote:
hayata-suenaga marked this conversation as resolved.
Show resolved Hide resolved
'Note: QuickBooks Online does not support a field for tax on Journal Entry exports. Change your export preference to Vendor Bill or Check to import taxes.',
locationsAdditionalDescription:
'Locations are imported as Tags. This limits exporting expense reports as Vendor Bills or Checks to QuickBooks Online. To unlock these export options, either disable Locations import or upgrade to the Control Plan to export Locations encoded as a Report Field.',
export: 'Export',
Expand Down
2 changes: 2 additions & 0 deletions src/languages/es.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1949,6 +1949,8 @@ export default {
customersDescription: 'Elige si queres importar clientes/proyectos y donde los clientes/proyectos son mostrados.',
locationsDescription: 'Elige si quieres importar lugares y donde los lugares son mostrados.',
taxesDescription: 'Elige si quires importar las tasas de impuestos y los impuestos por defecto de tu integración de contaduría.',
taxesJournalEntrySwitchNote:
'Nota: QuickBooks Online no admite un campo para impuestos al exportar entradas en el libro diario. Cambia tu preferencia de exportación a Factura de Proveedor o Cheque para importar impuestos.',
locationsAdditionalDescription:
'Los lugares son importados como Etiquegas. Esto limita a exportar los informes de gastos como Factura del Proveedor o Cheques a Quicbooks Online. Para desbloquear estas opciones de exportación desactiva la importación de Lugares o cambia al Plan Control para exportar Lugares como Campos de Informes.',
export: 'Exportar',
Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/accounting/qbo/export/QuickbooksOutOfPocketExpenseConfigurationPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ function QuickbooksOutOfPocketExpenseConfigurationPage({policy}: WithPolicyConne
const isTaxesEnabled = Boolean(syncTax);
const shouldShowTaxError = isTaxesEnabled && reimbursableExpensesExportDestination === CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.JOURNAL_ENTRY;
const shouldShowLocationError = isLocationEnabled && reimbursableExpensesExportDestination !== CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.JOURNAL_ENTRY;
const hasErrors = Boolean(errorFields?.exportEntity) || shouldShowTaxError || shouldShowLocationError;
const hasErrors = Boolean(errorFields?.reimbursableExpensesExportDestination) || shouldShowTaxError || shouldShowLocationError;

return (
<AccessOrNotFoundWrapper
Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/accounting/qbo/export/QuickbooksOutOfPocketExpenseEntitySelectPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function QuickbooksOutOfPocketExpenseEntitySelectPage({policy}: WithPolicyConnec
text: translate(`workspace.qbo.accounts.journal_entry`),
keyForList: CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.JOURNAL_ENTRY,
isSelected: reimbursableExpensesExportDestination === CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.JOURNAL_ENTRY,
isShown: !isTaxesEnabled || isLocationsEnabled,
isShown: !isTaxesEnabled,
},
{
value: CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.VENDOR_BILL,
Expand Down
6 changes: 5 additions & 1 deletion src/pages/workspace/accounting/qbo/import/QuickbooksTaxesPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ function QuickbooksTaxesPage({policy}: WithPolicyProps) {
const {translate} = useLocalize();
const styles = useThemeStyles();
const policyID = policy?.id ?? '';
const {syncTax, pendingFields} = policy?.connections?.quickbooksOnline?.config ?? {};
const {syncTax, pendingFields, reimbursableExpensesExportDestination} = policy?.connections?.quickbooksOnline?.config ?? {};
const isJournalExportEntity = reimbursableExpensesExportDestination === CONST.QUICKBOOKS_REIMBURSABLE_ACCOUNT_TYPE.JOURNAL_ENTRY;

return (
<AccessOrNotFoundWrapper
accessVariants={[CONST.POLICY.ACCESS_VARIANTS.ADMIN]}
Expand All @@ -44,10 +46,12 @@ function QuickbooksTaxesPage({policy}: WithPolicyProps) {
accessibilityLabel={translate('workspace.accounting.taxes')}
isOn={!!syncTax}
onToggle={() => Connections.updatePolicyConnectionConfig(policyID, CONST.POLICY.CONNECTIONS.NAME.QBO, CONST.QUICK_BOOKS_CONFIG.SYNC_TAX, !syncTax)}
disabled={!syncTax && isJournalExportEntity}
/>
</View>
</OfflineWithFeedback>
</View>
{isJournalExportEntity && <Text style={[styles.mutedNormalTextLabel, styles.pt2]}>{translate('workspace.qbo.taxesJournalEntrySwitchNote')}</Text>}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hint text does not seem right for the above-mentioned scenario i.e. when the syncTax is true
Journal Entry export is set).
A separate hint text may be better here for this specific scenario
What do you think?

Copy link
Contributor

@hayata-suenaga hayata-suenaga May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like !syncTax && isJournalExportEntity could solve this problem.

Yes I agree.

@teneeto, could you remind me if we have an error message on the frontend side for the rare case of Journal Entry is selected while the syncTax is on (when the configuration was changed in Expensify Classic, etc).

Copy link
Contributor

@hayata-suenaga hayata-suenaga May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@teneeto, could you remind me if we have an error message on the frontend side for the rare case of Journal Entry is selected while the syncTax is one (when the configuration was changed in Expensify Classic, etc).

Checked the code, and we display this error message when the above misconfiguration happens here. Should we do the same for the Tax toggle, too on the QuickbooksTaxesPage? (instead of displaying the hint text)?

Screenshot 2024-05-10 at 12 09 15 PM

Copy link
Contributor

@hayata-suenaga hayata-suenaga May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@teneeto, or do we already have an error message for the above case, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me confirm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no error fields here. should we add here?

Please see the comment below 😄

we will probably move the error handling to the backend. We don't have to do this on the front end. Please ignore this comment ⬇️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any change I'll need to make?

Thank you for reminding where the UI copy came from. I agree with @rojiphil on the following point:

The hint text does not seem right for the above-mentioned scenario i.e. when the syncTax is true
Journal Entry export is set).

I think we should have the same condition as what @rojiphil suggested -> !syncTax && isJournalExportEntity to display this hint message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@teneeto, can you modify the code according to my suggestion here?

What the user needs to do in this case (!syncTax && isJournalExportEntity) is to toggle off the Switch and the hint text can be misleading

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's rare case anyway, so I gonna address this change when I make a backend change to handle errors on the backend side 👍

</ScrollView>
</ScreenWrapper>
</AccessOrNotFoundWrapper>
Expand Down
Loading