Handle the error with journal entry when tax is enabled by teneeto · Pull Request #41520 · Expensify/App

teneeto · 2024-05-02T17:11:13Z

Details

This PR allows to warn the user of the error that happens when Journal Entry is selected as the out-of-pocket expense export destination while tax is enabled.

Screen.Recording.2024-05-02.at.17.55.11.mov

Fixed Issues

$: 41042
PROPOSAL: issuecomment

Tests

Case When Journal is selected as Export entity

Set up QBO - UK QBO account.
select Export settings
select Export out-of-pocket expenses as
select Export as and choose Journal as the export entity type.
go back to qbo main menu and select Import settings.
Then select Taxes
Expectation: when Journal is selected as an export entity type, you should not be able to enable taxes, and there should be a hint text about why you can't enable taxes.

Case When Tax is enabled

From QBO settings menu, select Import settings
select Taxes and toggle on import.
go back to QBO setting menu and select Export settings
select Export out-of-pocket expenses as
select Export as

Expectation: Notice how you can't find Journal amongst the export options and there should be a hint text about why you don't have the Journal option.

Verify that no errors appear in the JS console

Offline tests

Case When Journal is selected as Export entity

Set up QBO - UK QBO account..
select Export settings
select Export out-of-pocket expenses as
select Export as and choose Journal as the export entity type.
go back to qbo main menu and select Import settings.
Then select Taxes
Expectation: when Journal is selected as an export entity type, you should not be able to enable taxes, and there should be a hint text about why you can't enable taxes.

Case When Tax is enabled

From QBO settings menu, select Import settings
select Taxes and toggle on import.
go back to QBO setting menu and select Export settings
select Export out-of-pocket expenses as
select Export as

Expectation: Notice how you can't find Journal amongst the export options and there should be a hint text about why you don't have the Journal option.

QA Steps

Case When Journal is selected as Export entity

Set up QBO - UK QBO account..
select Export settings
select Export out-of-pocket expenses as
select Export as and choose Journal as the export entity type.
go back to qbo main menu and select Import settings.
Then select Taxes
Expectation: when Journal is selected as an export entity type, you should not be able to enable taxes, and there should be a hint text about why you can't enable taxes.

Case When Tax is enabled

From QBO settings menu, select Import settings
select Taxes and toggle on import.
go back to QBO setting menu and select Export settings
select Export out-of-pocket expenses as
select Export as

Expectation: Notice how you can't find Journal amongst the export options and there should be a hint text about why you don't have the Journal option.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

MacOS: Chrome / Safari

Screen.Recording.2024-05-02.at.17.55.11.mov

iOS: Native

Android: Native

Android: mWeb Chrome

iOS: mWeb Safari

MacOS: Desktop

melvin-bot · 2024-05-02T17:11:43Z

@amyevans Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

teneeto · 2024-05-02T17:39:51Z

@pecanoro @Gonals a few translations here for your scrutiny 🧐.

amyevans · 2024-05-02T19:17:46Z

Assigning @rojiphil for review since that's who it looks like it should be based on the linked issue, lmk if not!

teneeto · 2024-05-02T19:21:59Z

@trjExpensify @hayata-suenaga mentioning you here too. PR is ready.

trjExpensify · 2024-05-02T20:26:23Z

Why is there an error here? 🤔

In the frame before, you showed Taxes as being not imported because Journal Entry is selected:

teneeto · 2024-05-02T22:30:32Z

@trjExpensify That error is there for all the fields that has error or brick road Indicator and that's because I'm not connected to QBO. I believe if you test on your end, you won't find those.

…ndle-the-error-with-journal-entry

trjExpensify · 2024-05-02T23:57:44Z

@trjExpensify That error is there for all the fields that has error or brick road Indicator and that's because I'm not connected to QBO. I believe if you test on your end, you won't find those.

I don't understand this comment, sorry. I'm going to create a build.

trjExpensify · 2024-05-03T00:03:23Z

Here are the two screens we're expecting to see.

Case: When Journal Entry is selected for exporting out of pocket expenses

Set up QBO.
select Export settings
select Export out-of-pocket expenses as
select Export as and choose Journal Entry as the export entity type.
go back to qbo main menu and select Import settings.
Then select Taxes

Expectation: when Journal Entry is selected as an export entity type, you should not be able to import taxes, and there should be a hint text about why you can't import taxes.

Case: When Taxes are imported, you can't export as Journal Entry

From QBO settings menu, select Import settings
select Taxes and toggle on import.
go back to QBO setting menu and select Export settings
select Export out-of-pocket expenses as
select Export as
Expectation: Notice how you can't find Journal Entry amongst the export options and there should be a hint text about why you don't have the Journal Entry option.

teneeto · 2024-05-03T00:09:56Z

@trjExpensify That error is there for all the fields that has error or brick road Indicator and that's because I'm not connected to QBO. I believe if you test on your end, you won't find those.

I don't understand this comment, sorry. I'm going to create a build.

O Apologies for getting you lost there. what i meant was, that error was showing because I did not setup QBO on my local. I bypassed to make that fix. But if tested on real data or live it should work as expected without the error.

teneeto · 2024-05-03T00:10:27Z

Here are the two screens we're expecting to see.

Case: When Journal Entry is selected for exporting out of pocket expenses

Set up QBO.

select Export settings

select Export out-of-pocket expenses as

select Export as and choose Journal Entry as the export entity type.

go back to qbo main menu and select Import settings.

Then select Taxes

Expectation: when Journal Entry is selected as an export entity type, you should not be able to import taxes, and there should be a hint text about why you can't import taxes.
**Case: When Taxes are imported, you can't export as Journal Entry**

From QBO settings menu, select Import settings

select Taxes and toggle on import.

go back to QBO setting menu and select Export settings

select Export out-of-pocket expenses as

select Export as

Expectation: Notice how you can't find Journal Entry amongst the export options and there should be a hint text about why you don't have the Journal Entry option.

And yes! it should work this way.

github-actions · 2024-05-03T00:30:05Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/41520/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/41520/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/41520/NewExpensify.dmg	https://41520.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

hayata-suenaga · 2024-05-06T22:10:31Z

and in parallel, we can try to do something in the backend so it doesn't happen.

If enabled, why don’t we disable the syncTax when we update the policy connection configuration to set Journal entry ?

Turning off the tax import on the backend when the Journal Entry is selected introduces a new pattern that we have not yet considered. This might confuse users, as they do not anticipate these configuration options changing simultaneously.

There is an existing error message that has not been used yet. Could it be the one that @teneeto used in the earlier commits of this PR? The message was added in this PR. I suggest we use it now in the event of a misconfiguration.

cc: @aldo-expensify and @trjExpensify

PS: @trjExpensify, we're discussing this very rare case when the misconfiguration does happen.

…ndle-the-error-with-journal-entry

trjExpensify · 2024-05-07T10:21:26Z

I suggest we use it now in the event of a misconfiguration.

I think that sounds reasonable for this edge case. Let's ensure that we apply the "red brick road" to lead to this error though. I.e

Red dot on the settings avatar in the bottom tabs
Red dot on the Workspaces row in the Settings LHN menu
Red dot on the workspace in the list that contains an error
Red dot on the Accounting row in the workspace settings LHN menu
Red dot on the Export row in the QBO config card
Red dot on the Export out of pocket expenses as row
Red dot and error message in-line on the Export as page with the incorrect export method selected

pecanoro · 2024-05-09T08:46:00Z

I got a bit lost on the next steps here. Do we need to update the PR to handle the previous cases that were discussed?

…ndle-the-error-with-journal-entry

hayata-suenaga · 2024-05-09T22:47:25Z

by "previous case", do you mean this one?

hayata-suenaga · 2024-05-09T22:47:35Z

I'll do a more closer code review today

hayata-suenaga · 2024-05-10T01:21:57Z

I tested this PR on my local machine and it tested well.

The Tax toggle is locked and the hint text is displayed

The Journal Entry doesn't show up in the list of options when the Tax is enabled

src/languages/en.ts

rojiphil · 2024-05-10T05:03:10Z

If enabled, why don’t we disable the syncTax when we update the policy connection configuration to set Journal entry ?

Turning off the tax import on the backend when the Journal Entry is selected introduces a new pattern that we have not yet considered. This might confuse users, as they do not anticipate these configuration options changing simultaneously.

Got it. Thanks @hayata-suenaga

rojiphil · 2024-05-10T05:05:23Z

Usually, this isn't problematic as users are unable to select Journal Entry when the tax import is enabled. However, issues may occur when the device is offline or when users alter specific options on Expensify Classic and others on New Expensify.

I haven’t reviewed the code from this perspective yet. I will do this today and provide an update.

…ndle-the-error-with-journal-entry

rojiphil

Thanks @teneeto. The recent changes work well.
But I have left some comments for your consideration based on the review from the perspective mentioned here,

src/pages/workspace/accounting/qbo/import/QuickbooksTaxesPage.tsx

rojiphil · 2024-05-10T16:32:06Z

src/pages/workspace/accounting/qbo/import/QuickbooksTaxesPage.tsx

                            </View>
                        </OfflineWithFeedback>
                    </View>
+                    {isJournalExportEntity && <Text style={[styles.mutedNormalTextLabel, styles.pt2]}>{translate('workspace.qbo.taxesJournalEntrySwitchNote')}</Text>}


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?

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).

@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)?

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

Let me confirm

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 ⬇️

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

@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

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

hayata-suenaga · 2024-05-10T18:48:16Z

@teneeto, could you update the testing steps in the OP to mention that the tester needs to use a UK QBO account?

…ndle-the-error-with-journal-entry

hayata-suenaga · 2024-05-13T23:28:53Z

I'm not sure if we should handle these errors in the front end. If we're going to display errors, we have to create the complete RBR flow (Tom explained this here).

For now, I'd say we don't account for the edges cases that can happen because the user changes some configurations on Expensify Classics). Let's rely on the front end safeguard for now.

We can follow up with the backend change (return an error if the invalid configuration combination is attempted to made). And we can display the error returned from the backend following the RBR pattern that Tom described here)

I created a follow-up backend issue here (internal link)

rojiphil · 2024-05-15T07:21:14Z

Working on the checklist now

rojiphil

LGTM. And tests well too.

melvin-bot · 2024-05-15T10:57:19Z

🎯 @rojiphil, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #42187.

OSBotify · 2024-05-23T00:39:27Z

🚀 Deployed to staging by https://github.com/hayata-suenaga in version: 1.4.75-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-05-27T14:02:35Z

🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.75-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

teneeto added 5 commits May 2, 2024 17:12

don not toggle switch on when export is set to jounal entry

d5812cf

fix QBO undefined error for country check

d04a7a0

add taxesJournalEntrySwitchNote for english and spanish translations

0453720

add reason why taxes can't toggle

a2eae45

disable toggle when journal entry is selected

1ae5a20

teneeto requested a review from a team as a code owner May 2, 2024 17:11

teneeto changed the title ~~Handle the error with journal entry when tax is enable~~ Handle the error with journal entry when tax is enabled May 2, 2024

melvin-bot bot requested review from amyevans and removed request for a team May 2, 2024 17:11

teneeto added 2 commits May 2, 2024 18:34

translate first part taxesJournalEntrySwitchNote

85a8447

add full translation for taxesJournalEntrySwitchNote

0276320

amyevans requested review from rojiphil and removed request for amyevans May 2, 2024 19:17

hayata-suenaga self-requested a review May 2, 2024 20:23

teneeto added 3 commits May 2, 2024 23:40

Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…

0f13f6c

…ndle-the-error-with-journal-entry

fix tsc

f0cf4ef

Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…

1e17771

…ndle-the-error-with-journal-entry

trjExpensify added the Ready To Build label May 2, 2024

update error field

df6a3dd

Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…

bbaf3b7

…ndle-the-error-with-journal-entry

Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…

ccd10b0

…ndle-the-error-with-journal-entry

hayata-suenaga reviewed May 10, 2024

View reviewed changes

src/languages/en.ts Show resolved Hide resolved

Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…

0baa214

…ndle-the-error-with-journal-entry

rojiphil reviewed May 10, 2024

View reviewed changes

Merge branch 'main' of github.com:teneeto/Expensify into fix/41042/ha…

1861a11

…ndle-the-error-with-journal-entry

update check for disabled toggle

b44b907

teneeto dismissed pecanoro’s stale review via b44b907 May 14, 2024 07:31

hayata-suenaga requested a review from rojiphil May 15, 2024 06:50

rojiphil approved these changes May 15, 2024

View reviewed changes

melvin-bot bot mentioned this pull request May 15, 2024

Payment issue for E/App PR#41520 #42187

Closed

pecanoro approved these changes May 15, 2024

View reviewed changes

hayata-suenaga approved these changes May 15, 2024

View reviewed changes

hayata-suenaga merged commit b839bdf into Expensify:main May 15, 2024

github-actions bot mentioned this pull request May 22, 2024

Deploy Checklist: New Expensify 2024-05-22 #42496

Closed

45 tasks

rojiphil mentioned this pull request Jun 3, 2024

[Hold for payment 2024-06-03] [$250] [Wave Collect][QBO] Prevent the simultaneous enabling of JE export and Tax import #41042

Closed

Conversation

teneeto commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented May 2, 2024

Uh oh!

teneeto commented May 2, 2024

Uh oh!

amyevans commented May 2, 2024

Uh oh!

teneeto commented May 2, 2024

Uh oh!

trjExpensify commented May 2, 2024

Uh oh!

teneeto commented May 2, 2024

Uh oh!

trjExpensify commented May 2, 2024

Uh oh!

trjExpensify commented May 3, 2024

Uh oh!

teneeto commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teneeto commented May 3, 2024

Uh oh!

github-actions bot commented May 3, 2024

Uh oh!

hayata-suenaga commented May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trjExpensify commented May 7, 2024

Uh oh!

pecanoro commented May 9, 2024

Uh oh!

hayata-suenaga commented May 9, 2024

Uh oh!

hayata-suenaga commented May 9, 2024

Uh oh!

hayata-suenaga commented May 10, 2024

Uh oh!

Uh oh!

rojiphil commented May 10, 2024

Uh oh!

rojiphil commented May 10, 2024

Uh oh!

rojiphil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rojiphil May 10, 2024

Choose a reason for hiding this comment

Uh oh!

hayata-suenaga May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hayata-suenaga May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hayata-suenaga May 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teneeto May 13, 2024

Choose a reason for hiding this comment

Uh oh!

hayata-suenaga May 14, 2024

Choose a reason for hiding this comment

Uh oh!

hayata-suenaga May 14, 2024

Choose a reason for hiding this comment

Uh oh!

teneeto commented May 2, 2024 •

edited

Loading

teneeto commented May 3, 2024 •

edited

Loading

hayata-suenaga commented May 6, 2024 •

edited

Loading

hayata-suenaga May 10, 2024 •

edited

Loading

hayata-suenaga May 10, 2024 •

edited

Loading

hayata-suenaga May 10, 2024 •

edited

Loading

hayata-suenaga commented May 10, 2024 •

edited

Loading

hayata-suenaga commented May 13, 2024 •

edited

Loading