-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
🪟 🤝 Free connectors program confirmation UI #21623
Changes from 6 commits
6f7b5b8
13670cc
83f1a27
f2a434b
86fd010
ef5fd68
2c3fa55
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 |
---|---|---|
@@ -1,4 +1,9 @@ | ||
import { useIntl } from "react-intl"; | ||
|
||
import { ToastType } from "components/ui/Toast"; | ||
|
||
import { useModalService } from "hooks/services/Modal"; | ||
import { useNotificationService } from "hooks/services/Notification"; | ||
import { useAuthService } from "packages/cloud/services/auth/AuthService"; | ||
import { useStripeCheckout } from "packages/cloud/services/stripe/StripeService"; | ||
import { useCurrentWorkspaceId } from "services/workspaces/WorkspacesService"; | ||
|
@@ -10,6 +15,19 @@ export const useShowEnrollmentModal = () => { | |
const { mutateAsync: createCheckout } = useStripeCheckout(); | ||
const workspaceId = useCurrentWorkspaceId(); | ||
const { emailVerified, sendEmailVerification } = useAuthService(); | ||
const { formatMessage } = useIntl(); | ||
const { registerNotification } = useNotificationService(); | ||
|
||
const verifyEmail = () => | ||
sendEmailVerification() | ||
.then(() => { | ||
registerNotification({ | ||
id: "fcp/verify-email", | ||
text: formatMessage({ id: "freeConnectorProgram.enrollmentModal.validationEmailConfirmation" }), | ||
type: ToastType.INFO, | ||
}); | ||
}) | ||
.catch(); // don't crash the page on error | ||
Comment on lines
+21
to
+30
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. If we're housing the email verification logic here rather than within the modal component itself, does it make sense to also move Edit: actually, it looks like we're doing more there with refs and redirects and that may not be as simple a change as it first looked. |
||
|
||
return { | ||
showEnrollmentModal: () => { | ||
|
@@ -21,7 +39,7 @@ export const useShowEnrollmentModal = () => { | |
createCheckout={createCheckout} | ||
closeModal={closeModal} | ||
emailVerified={emailVerified} | ||
sendEmailVerification={sendEmailVerification} | ||
sendEmailVerification={verifyEmail} | ||
/> | ||
), | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,47 @@ | ||
import { useState } from "react"; | ||
import { useIntl } from "react-intl"; | ||
import { useQuery } from "react-query"; | ||
import { useSearchParams } from "react-router-dom"; | ||
import { useEffectOnce } from "react-use"; | ||
|
||
import { ToastType } from "components/ui/Toast"; | ||
|
||
import { useExperiment } from "hooks/services/Experiment"; | ||
import { useNotificationService } from "hooks/services/Notification"; | ||
import { useConfig } from "packages/cloud/services/config"; | ||
import { useDefaultRequestMiddlewares } from "services/useDefaultRequestMiddlewares"; | ||
import { useCurrentWorkspaceId } from "services/workspaces/WorkspacesService"; | ||
|
||
import { webBackendGetFreeConnectorProgramInfoForWorkspace } from "../lib/api"; | ||
|
||
export const STRIPE_SUCCESS_QUERY = "fcpEnrollmentSuccess"; | ||
|
||
export const useFreeConnectorProgram = () => { | ||
const workspaceId = useCurrentWorkspaceId(); | ||
const { cloudApiUrl } = useConfig(); | ||
const config = { apiUrl: cloudApiUrl }; | ||
const middlewares = useDefaultRequestMiddlewares(); | ||
const requestOptions = { config, middlewares }; | ||
const freeConnectorProgramEnabled = useExperiment("workspace.freeConnectorsProgram.visible", false); | ||
const [searchParams, setSearchParams] = useSearchParams(); | ||
const [userDidEnroll, setUserDidEnroll] = useState(false); | ||
const { formatMessage } = useIntl(); | ||
const { registerNotification } = useNotificationService(); | ||
|
||
return useQuery(["freeConnectorProgramInfo", workspaceId], () => | ||
useEffectOnce(() => { | ||
if (searchParams.has(STRIPE_SUCCESS_QUERY)) { | ||
// Remove the stripe parameter from the URL | ||
setSearchParams({}, { replace: true }); | ||
setUserDidEnroll(true); | ||
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.
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. Almost right, but the situation is slightly worse than that. After a user successfully finishes the stripe checkout flow:
That means that even if we do a full re-query of the backend right away (which in fact we do, since we're reloading the whole webapp), there's a race condition: we cannot rely on the backend to have already processed the enrollment at the moment we ought to show the user a confirmation. |
||
registerNotification({ | ||
id: "fcp/enrolled", | ||
text: formatMessage({ id: "freeConnectorProgram.enroll.success" }), | ||
type: ToastType.SUCCESS, | ||
}); | ||
} | ||
}); | ||
|
||
const enrollmentStatusQuery = useQuery(["freeConnectorProgramInfo", workspaceId], () => | ||
webBackendGetFreeConnectorProgramInfoForWorkspace({ workspaceId }, requestOptions).then( | ||
({ hasEligibleConnector, hasPaymentAccountSaved }) => { | ||
const userIsEligibleToEnroll = !hasPaymentAccountSaved && hasEligibleConnector; | ||
|
@@ -27,4 +53,9 @@ export const useFreeConnectorProgram = () => { | |
} | ||
) | ||
); | ||
|
||
return { | ||
enrollmentStatusQuery, | ||
userDidEnroll, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -273,6 +273,7 @@ export const AuthenticationProvider: React.FC<React.PropsWithChildren<unknown>> | |
type: ToastType.ERROR, | ||
}); | ||
} | ||
throw error; | ||
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 find this error handling hard to grok, but it works 🙂 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 certainly did when I first pulled it out of The explanation, I guess for posterity:
As Joey asked on Slack, a reasonable follow-up question is whether the success handling should also be centralized. Maybe! We'd just have to verify that this behavior also makes sense in the context of 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. My confusion was more about throwing the error again on line 276, only to have empty 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. Ahh, that is a better question, thanks for clarifying. It was necessary (emphasis on "was") because I assumed the code I was extracting was structurally appropriate to the use case 🤦 (a reasonable default assumption, given that the internal API defined in |
||
} | ||
}, | ||
async verifyEmail(code: string): Promise<void> { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,7 +26,8 @@ export const ConnectionPageTitle: React.FC = () => { | |||||||||||
|
||||||||||||
const { connection } = useConnectionEditService(); | ||||||||||||
|
||||||||||||
const { data: freeConnectorProgramInfo } = useFreeConnectorProgram(); | ||||||||||||
const { enrollmentStatusQuery } = useFreeConnectorProgram(); | ||||||||||||
const { data: freeConnectorProgramInfo } = enrollmentStatusQuery; | ||||||||||||
const displayEnrollmentCallout = freeConnectorProgramInfo?.showEnrollmentUi; | ||||||||||||
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. can this be simplified to two lines instead of three by doing
Suggested change
or something of the like? Just seems redundant as is with the intermediate steps there. |
||||||||||||
|
||||||||||||
const steps = useMemo(() => { | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,11 @@ | ||
$tooltip: 9999 + 3; | ||
$tooltip: 9999 + 4; | ||
$notification: 9999 + 3; | ||
$datepicker: 9999 + 2; | ||
$modal: 9999 + 1; | ||
$sidebar: 9999; | ||
$panelSplitter: 0; | ||
$dropdownMenu: 2; | ||
$notification: 20; | ||
$schemaChangesBackdrop: 3; | ||
$schemaChangesBackdropContent: 4; | ||
$schemaChangesBackdrop: 3; | ||
$dropdownMenu: 2; | ||
$switchSliderBefore: 1; | ||
$tableScroll: 1; | ||
$panelSplitter: 0; | ||
Comment on lines
+1
to
+11
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. 🙏 |
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.
This variable name doesn't make much sense to me as we aren't using it anywhere for a menu, and I can't tell why a menu's width would be a relevant benchmark for this.
Is there a meaningful reason to use 2 * width-size-menu vs. the number itself?
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.
Agreed, it's a weird naming choice. Here, I used that variable purely because the modal does, to keep the two nicely-aligned vis a vis each other.
Zooming out a little, it's not used very many places:
I think it's a fine candidate to be pulled out or renamed or something, I just didn't want to have to verify the correctness of
components/connection/CatalogTree/next/StreamDetailsPanel
in order to ship the free connectors program 😬