🪟 🤝 Free connectors program confirmation UI #21623

ambirdsall · 2023-01-19T21:39:41Z

What

Suppress enrollment banners and show confirmation Toast when user returns to the successUrl from an enrollment stripe checkout session; dismiss modal and show confirmation Toast when a user requests a verification email resend from the enrollment modal. Closes #21616.

screen recordings

Enrollment success. In retrospect, I should have started recording the gif before opening the modal, to make the difference more apparent when the enrollment banner is hidden. Ah well, c'est la vie. Gifs are hidden behind <details> tags because they are large and distracting:

![fcp-enrollment-success](https://user-images.githubusercontent.com/7516653/213574254-30a04245-9128-464e-beb3-06f3724f031e.gif)

Email validation resend requested

![fcp-email-validation-confirmation](https://user-images.githubusercontent.com/7516653/213574240-cf6c21ff-a791-4631-93aa-829fda766cfe.gif)

How

Pretty straightforward implementation. One key but nonobvious detail: this changes the query param appended to the stripe successUrl, in order to distinguish between a Free Connector Program enrollment and a one-off transaction on the credits page.

Open question about a future UX improvement

Should the email verification popup mention that the user has to refresh the page after confirming their email? Simply retriggering the modal won't reflect any changes that may have happened if the banner that triggers it hasn't rerendered, because the current modal implementation cannot directly use the required hooks (it renders outside of the necessary context provider components).

Options:

tell users to refresh the page in the popup
start polling email validation status from AuthService in the banner components as soon as the user requests email validation
other??

Also updates the query string itself so it's no longer identical to the pre-existing checkout success query: this means that a one-off credit transaction triggered from the credits page won't be mistaken for enrollment.

ambirdsall · 2023-01-19T22:59:01Z

...pp/src/packages/cloud/components/experiments/FreeConnectorProgram/LargeEnrollmentCallout.tsx


-  return (
+  return userDidEnroll ? (
+    // eslint-disable-next-line react/jsx-no-useless-fragment


If the whole UI subtree should be rendered or not based on some condition, I think keeping the conditional logic outside the UI it toggles, like this:

return someCond ? ( <></> ) : ( <TheActualComponent> <And> <ItsChildren /> </And> </TheActualComponent> );

can be much nicer to read than sticking it inside the nominal UI it's trying to toggle:

return ( <> {someCond ? null : ( <TheActualComponent> <And> <ItsChildren /> </And> </TheActualComponent> )} </> )

No strong opinion here, they are functionally the same and I don't see a big difference in readability. Usually I would argue to keep conditions like this outside the component (e.g. don't render the component at all), but in this case it makes sense inside.

I would suggest ditching the empty fragment (<></>) that is not necessary here (just return null if you don't want react to render anything to the DOM.

josephkmh

Just a couple small comments. I think we should find a way to re-check whether a user's email has been verified when the window regains focus. That would make the UX a lot smoother. But I think that will require a refactor of the AuthService, so we can make that a separate PR.

...cloud/components/experiments/FreeConnectorProgram/EnrollmentModal/useShowEnrollmentModal.tsx

josephkmh · 2023-01-20T10:22:46Z

...cloud/components/experiments/FreeConnectorProgram/EnrollmentModal/useShowEnrollmentModal.tsx

+        text: formatMessage({ id: "freeConnectorProgram.enrollmentModal.validationEmailConfirmation" }),
+        type: ToastType.INFO,
+      });
+      closeModal();


I think closing the modal is kind of weird UX. But since we don't have an easy way to revalidate whether the user's email has been verified, it might be the easiest short-term solution.

Btw I think we can safely move ModalServiceProvider inside the AuthenticationProvider in App.tsx, that way we can call most of our normal hooks (including useAuthService()) from within modals. @timroes any reason why ModalServiceProvider is so far outside the provider tree in cloud's App.tsx?

I think closing the modal is kind of weird UX

Agreed. Unfortunately, because the modal is at a higher z-index than the toast, leaving it open means the only way to confirm the action is rendering something inline in the modal, which also seemed weird.

I think we can switch those z-indexes around. In general I'd assume an incoming notification should be higher even than an open modal?

And for posterity (talked offline with Joey about that): We should be fine reordering the providers, but we might need to check which orders work since there are "some" dependencies between them.

👍 to both. To keep this particular PR's diff focused, though, I'll leave the providers reastructuring and clean up associated cleanup for a follow-up PR.

josephkmh · 2023-01-20T10:47:40Z

...pp/src/packages/cloud/components/experiments/FreeConnectorProgram/LargeEnrollmentCallout.tsx


-  return (
+  return userDidEnroll ? (
+    // eslint-disable-next-line react/jsx-no-useless-fragment


No strong opinion here, they are functionally the same and I don't see a big difference in readability. Usually I would argue to keep conditions like this outside the component (e.g. don't render the component at all), but in this case it makes sense inside.

I would suggest ditching the empty fragment (<></>) that is not necessary here (just return null if you don't want react to render anything to the DOM.

...p/src/packages/cloud/components/experiments/FreeConnectorProgram/InlineEnrollmentCallout.tsx

ambirdsall · 2023-01-20T20:52:02Z

Updates since the last round of reviews:

while keeping the shared error handling in sendEmailVerification, also reraise caught errors. Client code (all two files of it lol) now need to use .catch() if they don't want to risk crashing the page, but it means a chained .then callback will only be run on success instead of being a de facto .finally
updated the toast z-index to be higher than the modal's. Also, for readability, sorted the z-index variables in order of, well, z-index.
centered the toast popup vis a vis the modal.

Before tweaking the toast alignment:

and after:

teallarson

I'll defer to Joey on a review here! I mostly just had questions.

teallarson · 2023-01-20T21:13:06Z

airbyte-webapp/src/components/ui/Toast/Toast.module.scss

@@ -43,6 +43,7 @@ $toast-bottom-margin: 27px;
  position: fixed;
  box-sizing: border-box;
  bottom: $toast-bottom-margin;
+  margin-left: calc(vars.$width-size-menu / 2);


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?

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 😬

teallarson · 2023-01-20T21:16:14Z

...cloud/components/experiments/FreeConnectorProgram/EnrollmentModal/useShowEnrollmentModal.tsx

+  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


If we're housing the email verification logic here rather than within the modal component itself, does it make sense to also move startStripeCheckout() here from within the modal? They feel parallel-ish to me.

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.

teallarson · 2023-01-20T21:20:46Z

.../packages/cloud/components/experiments/FreeConnectorProgram/hooks/useFreeConnectorProgram.ts

+    if (searchParams.has(STRIPE_SUCCESS_QUERY)) {
+      // Remove the stripe parameter from the URL
+      setSearchParams({}, { replace: true });
+      setUserDidEnroll(true);


userDidEnroll is essentially a temporary place for us to know that a user has enrolled, but we haven't done a full re-query of the backend yet so we can show the Toast and remove the enrollment materials, right?

Almost right, but the situation is slightly worse than that. After a user successfully finishes the stripe checkout flow:

Stripe redirects the user directly to the successUrl their checkout session was configured with (in this case, the airbyte URL the user came from with STRIPE_SUCCESS_QUERY appended)

fires off a webhook to update our backend

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.

teallarson · 2023-01-20T21:22:56Z

airbyte-webapp/src/pages/connections/ConnectionPage/ConnectionPageTitle.tsx

+  const { enrollmentStatusQuery } = useFreeConnectorProgram();
+  const { data: freeConnectorProgramInfo } = enrollmentStatusQuery;
  const displayEnrollmentCallout = freeConnectorProgramInfo?.showEnrollmentUi;


can this be simplified to two lines instead of three by doing

Suggested change

const { enrollmentStatusQuery } = useFreeConnectorProgram();

const { data: freeConnectorProgramInfo } = enrollmentStatusQuery;

const displayEnrollmentCallout = freeConnectorProgramInfo?.showEnrollmentUi;

const { enrollmentStatusQuery } = useFreeConnectorProgram();

const displayEnrollmentCallout = enrollmentStatusQuery?.data?.freeConnectorProgramInfo?.showEnrollmentUi;

or something of the like? Just seems redundant as is with the intermediate steps there.

teallarson · 2023-01-20T21:23:23Z

airbyte-webapp/src/scss/_z-indices.scss

+$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;


josephkmh

I think the error handling here is kind of hard to reason about (throwing & catching without handling seems... unintuitive?) but it works for now, and we've got a deadline. So happy to merge it the way it is!

josephkmh · 2023-01-20T22:49:14Z

airbyte-webapp/src/packages/cloud/services/auth/AuthService.tsx

@@ -273,6 +273,7 @@ export const AuthenticationProvider: React.FC<React.PropsWithChildren<unknown>>
                type: ToastType.ERROR,
              });
          }
+          throw error;


I find this error handling hard to grok, but it works 🙂

I certainly did when I first pulled it out of EmailVerificationHint.tsx (it's basically unchanged otherwise), but at least it's structurally simple.

The explanation, I guess for posterity:

wrap the firebase call in a try/catch

match against a firebase-defined code (if you want to get fancy, it effectively makes the error type a tagged union) with switch(error.code)

for each case in that switch statement, match the error code against the AuthErrorCodes object imported from "firebase/auth" and give the contextually-appropriate notification

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 EmailValidationHint first, but I didn't want to hold up the free connectors work by going off on that sort of tangent.

My confusion was more about throwing the error again on line 276, only to have empty catch() statements in the consumer. I don't quite get why that's necessary?

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 authService had an identical type signature to the firebase/auth function it was wrapping, but incorrect). It's quite a simple fix, though.

Extract STRIPE_SUCCESS_QUERY to hook for reuse

6f7b5b8

Also updates the query string itself so it's no longer identical to the pre-existing checkout success query: this means that a one-off credit transaction triggered from the credits page won't be mistaken for enrollment.

octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 19, 2023

ambirdsall marked this pull request as ready for review January 19, 2023 22:31

ambirdsall requested review from teallarson and josephkmh January 19, 2023 22:32

ambirdsall changed the title ~~Alex/free connectors program confirmation UI~~ 🪟 🤝 Free connectors program confirmation UI Jan 19, 2023

ambirdsall commented Jan 19, 2023

View reviewed changes

ambirdsall added 2 commits January 19, 2023 15:40

Hide banners, show pop-up after successful enrollment

13670cc

Show confirmation when user requests verification email

83f1a27

ambirdsall force-pushed the alex/free-connectors-program-confirmation-UI branch from 5cbde96 to 83f1a27 Compare January 19, 2023 23:40

josephkmh requested changes Jan 20, 2023

View reviewed changes

ambirdsall added 3 commits January 20, 2023 12:38

Clean up conditional UI: s/<></>/null

f2a434b

Don't fully swallow sendEmailVerification errors

86fd010

Make Toast and Modal play together more nicely

ef5fd68

ambirdsall requested review from josephkmh and timroes January 20, 2023 20:52

teallarson reviewed Jan 20, 2023

View reviewed changes

Remove intermediate vars when querying showEnrollmentUi

2c3fa55

josephkmh approved these changes Jan 20, 2023

View reviewed changes

ambirdsall merged commit 680f5da into master Jan 20, 2023

ambirdsall deleted the alex/free-connectors-program-confirmation-UI branch January 20, 2023 23:39

octavia-squidington-iii mentioned this pull request Jan 24, 2023

Bump Airbyte version from 0.40.28 to 0.40.29 #21767

Merged

ambirdsall mentioned this pull request Jan 24, 2023

🪟 🔧 Cleaner error handling for authService's sendVerificationEmail #21763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🤝 Free connectors program confirmation UI #21623

🪟 🤝 Free connectors program confirmation UI #21623

ambirdsall commented Jan 19, 2023 •

edited

Loading

ambirdsall Jan 19, 2023 •

edited

Loading

josephkmh Jan 20, 2023

josephkmh left a comment

josephkmh Jan 20, 2023 •

edited

Loading

ambirdsall Jan 20, 2023

timroes Jan 20, 2023

timroes Jan 20, 2023

ambirdsall Jan 20, 2023

josephkmh Jan 20, 2023

ambirdsall commented Jan 20, 2023

teallarson left a comment

teallarson Jan 20, 2023

ambirdsall Jan 20, 2023

teallarson Jan 20, 2023

teallarson Jan 20, 2023

ambirdsall Jan 20, 2023 •

edited

Loading

teallarson Jan 20, 2023

teallarson Jan 20, 2023

josephkmh left a comment

josephkmh Jan 20, 2023

ambirdsall Jan 20, 2023

josephkmh Jan 21, 2023

ambirdsall Jan 24, 2023

🪟 🤝 Free connectors program confirmation UI #21623

🪟 🤝 Free connectors program confirmation UI #21623

Conversation

ambirdsall commented Jan 19, 2023 • edited Loading

What

screen recordings

How

Open question about a future UX improvement

ambirdsall Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephkmh left a comment

Choose a reason for hiding this comment

josephkmh Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambirdsall commented Jan 20, 2023

teallarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambirdsall Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephkmh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambirdsall commented Jan 19, 2023 •

edited

Loading

ambirdsall Jan 19, 2023 •

edited

Loading

josephkmh Jan 20, 2023 •

edited

Loading

ambirdsall Jan 20, 2023 •

edited

Loading