-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix/74127 Invoicify related bugs #79950
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
4ff430c
f099305
01399c5
80726e5
eefc094
43d4a87
eddf817
b277643
3ee76c9
654081d
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 | ||
|---|---|---|---|---|
|
|
@@ -4,10 +4,12 @@ import WorkspaceConfirmationForm from '@components/WorkspaceConfirmationForm'; | |||
| import type {WorkspaceConfirmationSubmitFunctionParams} from '@components/WorkspaceConfirmationForm'; | ||||
| import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; | ||||
| import useOnyx from '@hooks/useOnyx'; | ||||
| import usePrivateSubscription from '@hooks/usePrivateSubscription'; | ||||
| import useResponsiveLayout from '@hooks/useResponsiveLayout'; | ||||
| import {createWorkspaceWithPolicyDraftAndNavigateToIt} from '@libs/actions/App'; | ||||
| import {generatePolicyID} from '@libs/actions/Policy/Policy'; | ||||
| import getCurrentUrl from '@libs/Navigation/currentUrl'; | ||||
| import {isSubscriptionTypeOfInvoicing} from '@libs/SubscriptionUtils'; | ||||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||||
| import ROUTES from '@src/ROUTES'; | ||||
| import type {LastPaymentMethodType} from '@src/types/onyx'; | ||||
|
|
@@ -21,6 +23,7 @@ function WorkspaceConfirmationPage() { | |||
| const [activePolicyID] = useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID, {canBeMissing: true}); | ||||
| const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED, {canBeMissing: true}); | ||||
| const currentUserPersonalDetails = useCurrentUserPersonalDetails(); | ||||
| const privateSubscription = usePrivateSubscription(); | ||||
| const onSubmit = (params: WorkspaceConfirmationSubmitFunctionParams) => { | ||||
| const policyID = params.policyID || generatePolicyID(); | ||||
| const routeToNavigate = isSmallScreenWidth ? ROUTES.WORKSPACE_INITIAL.getRoute(policyID) : ROUTES.WORKSPACE_OVERVIEW.getRoute(policyID); | ||||
|
|
@@ -39,6 +42,7 @@ function WorkspaceConfirmationPage() { | |||
| activePolicyID, | ||||
| currentUserAccountIDParam: currentUserPersonalDetails.accountID, | ||||
| currentUserEmailParam: currentUserPersonalDetails.email ?? '', | ||||
| shouldCreateControlPolicy: isSubscriptionTypeOfInvoicing(privateSubscription?.type), | ||||
|
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. Don't we need to set Line 590 in 87e6d66
Contributor
Author
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. I'm not even entirely sure how to test this functionality, as it seems that this logic is rarely used, and I think that Invoicy users will almost never use it, especially in offline mode, and in the online backend will return the correct data. In addition, it seems that this useEffect is called once when the page is mounted after the transition, at a time when privateSettings will still be undefined. I'm afraid that if we add any dependencies to this useEffect, we will cause many more bugs than we will gain benefits, because the chance of such a scenario occurring is very small and only applies to offline mode, which, in my opinion, does not justify the risks. What do you think about this?
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. Yeah. Digged in a little deeper to figure out the usage and it’s here. |
||||
| }); | ||||
| }; | ||||
| const currentUrl = getCurrentUrl(); | ||||
|
|
||||
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.
usePrivateSubscriptionreturnsundefinedwhile Onyx is still loading, andisSubscriptionTypeOfInvoicing(undefined)is false. That means an Invoicify user who opens this page beforeNVP_PRIVATE_SUBSCRIPTIONis ready (e.g., fresh login or cache miss/offline) will still create a TEAM/Collect workspace, recreating the optimistic-type bug you’re trying to fix. Consider blocking submission until the subscription is loaded or handling the “loading/unknown” state explicitly.Useful? React with 👍 / 👎.
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 don't think this applies in this case, because this function is only executed when the user clicks the button, not when the page loads, so this data will already be available at the moment of clicking.