-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🪟 🔧 ☁️ Check email verification status for free connector enrollment #21514
🪟 🔧 ☁️ Check email verification status for free connector enrollment #21514
Conversation
3088df7
to
97a8387
Compare
[note from Alex] ignore this bot: it's talking about an unrelated file that got pulled in from latest
|
File | Coverage [45.16%] | ❌ |
---|---|---|
EnvConfigs.java | 45.16% | ❌ |
Total Project Coverage | 26.63% | 🍏 |
---|
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.
Could use a rebase to remove changes from the inline banner.
Other than that, one comment and one question but looks pretty much good to go.
...ckages/cloud/components/experiments/FreeConnectorProgram/EnrollmentModal/EnrollmentModal.tsx
Outdated
Show resolved
Hide resolved
.../packages/cloud/components/experiments/FreeConnectorProgram/hooks/useFreeConnectorProgram.ts
Outdated
Show resolved
Hide resolved
bfae290
to
b6a4950
Compare
@@ -745,12 +745,5 @@ | |||
|
|||
"jobs.noAttemptsFailure": "Failed to start job.", | |||
|
|||
"cloudApi.loginCallbackUrlError": "There was an error connecting to the developer portal. Please try again.", | |||
|
|||
"freeConnectorProgram.enrollmentModal.title": "Free connector program", |
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.
these are not being deleted, just moved to the cloud-specific locale file
b6a4950
to
6797b89
Compare
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.
Nice!
Some thoughts I had trying it out:
- Could use a success message on send ("verification email sent!" or something along the lines)
- I have to refresh the page after verifying to remove the warning
@tealjulia there's a slack discussion on the confirmation feedback; definitely going to be added, though I might hold that for a follow-up for easier reviewing. The other issue, "have to refresh the page after verifying to remove the warning", would have to be solved by polling |
Reasonable! I wouldn't say it's blocking. Possibly a related solution to https://github.com/airbytehq/airbyte-cloud/issues/3992 (redirecting when a user logs out or in in a separate tab) |
6815c14
to
abb7abd
Compare
- rename the function to match its filename - check the experiment within the hook, not at every call site
abb7abd
to
f766423
Compare
…21514) * check email verification status for real * Extract email verification error handling to hook * Move enrollmentModal's text copy to cloud locale * Make FCP enrollment modal aware of email verification status * Clean up useFreeConnectorProgram - rename the function to match its filename - check the experiment within the hook, not at every call site * Update FCP modal for users with unverified emails
What
A few improvements to the frontend for free connector enrollment:
emailVerified
andsendEmailVerification
intoEnrollmentModalContent
as props (viauseAuthService()
, which can't be called within the modal content component itself but can be from theuseShowEnrollmentModal
hook)useFreeConnectorProgram
hookScreenshot of the modal for a user with unvalidated email:
Okay, but why is
CreditsPage/components/EmailVerificationHint.tsx
in this PRRight, that. That component was the only place in the app that used the
sendEmailVerification
function fromuseAuthService
. It also added a bunch of error handling that we'll want every time we use this API, so before I usedsendEmailVerification
in the FCP enrollment modal, I extracted the error handling intouseAuthService
itself, so we would get it "for free". As of now, the error handling strategy is hardcoded in the hook; if the need for context-dependent error handling arises, we will have to rewritesendEmailVerification
to return some well-typed error object instead of handling errors right inside the function. But I'm not sure we're gonna need it.Recommended reading order
There are three pairs of files whose changes are linked; you can read them in either order, but should probably consider them together.
useShowEnrollmentModal.tsx
/EnrollmentModal.tsx
useFreeConnectorProgram.ts
/ConnectionPageTitle.tsx
EmailVerificationHint.tsx
/AuthService.tsx