feat(fxa-settings): setup signin_bounced in React #15128

millmason · 2023-04-04T20:16:44Z

Because:

We're moving the React components to their real routes and wiring them up to receive data

This commit:

Adds signin_bounced into the React app
Adds a check for the user's email (via localStorage) in the App root file

Closes #FXA-6489

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Other information (Optional)

Still needs metrics!!

millmason · 2023-04-04T20:18:43Z

packages/fxa-settings/src/models/Account.ts

    );
  }

+  async getAccountStatusByEmail(email: string): Promise<any> {


Originally in Backbone, we would check to make sure the account associated with the email existed. While I opted to simplify the page by omitting this per discussion with @vbudhram, we will need this method.

nit: Does this need to be any or can it be more specific?

Hmmmm I think it can be a boolean actually, checking!

pdehaan · 2023-04-04T22:06:07Z

packages/fxa-settings/src/components/App/index.tsx

+
+      if (currentUser) {
+        setEmail(currentUser.email);
+      } else {


pdehaan · 2023-04-04T22:11:16Z

packages/fxa-settings/src/models/Account.ts

    );
  }

+  async getAccountStatusByEmail(email: string): Promise<any> {


nit: Does this need to be any or can it be more specific?

pdehaan · 2023-04-04T22:14:40Z

packages/fxa-settings/src/pages/Signin/SigninBounced/index.stories.tsx

-      <SigninBounced email={props.email} canGoBack={props.canGoBack} />
-    </AppLayout>
+    <SigninBounced
+      emailLookupComplete={true}


~~nit: I think we can drop the {true} for boolean props.~~
Meh, it's a *.stories.* file, maybe we don't care too much. 🤷

I'll clean it up!

pdehaan · 2023-04-04T22:22:15Z

packages/fxa-settings/src/pages/Signin/SigninBounced/index.stories.tsx

+      canGoBack={true}
+      emailLookupComplete={true}


nit: same here re: {true} boolean props (although this was already using {true} before), per https://arcticicestudio.github.io/styleguide-javascript/rules/react/props.html#boolean-attributes-notation

pdehaan · 2023-04-04T22:31:45Z

packages/fxa-settings/src/pages/Signin/SigninBounced/index.test.tsx

+    renderWithRouter(
+      <SigninBounced
+        email={MOCK_ACCOUNT.primaryEmail.email}
+        canGoBack={true}


nit: can omit {true}, although it is a *.test.* file. 🤷

vpomerleau

LGTM! 🎉 I've checked out the branch and tested that I do see the React SigninBounced page when navigating to http://localhost:3030/signin_bounced?showReactApp=true after initiating signin, and get redirected to /signup when trying to hit this route after clearing local storage.

I am right in assuming that the actual check of whether an email has bounced will be happening somewhere else? And if so, is there an existing ticket to implement that? I recall that checking for bounced email was marked as a to-do for some pages.

vpomerleau · 2023-04-06T18:38:10Z

packages/fxa-settings/src/pages/Signin/SigninBounced/index.tsx

-    // clear the session
-    // clear the form prefill
-    navigate('/signup');
+    logViewEvent(viewName, 'link.create-account', REACT_ENTRYPOINT);


I think this is a separate issue (because I've spotted this in other pages while working on FXA-7100), but the emitted metrics event doesn't have a payload.

millmason · 2023-04-06T19:13:23Z

LGTM! 🎉 I've checked out the branch and tested that I do see the React SigninBounced page when navigating to http://localhost:3030/signin_bounced?showReactApp=true after initiating signin, and get redirected to /signup when trying to hit this route after clearing local storage.

I am right in assuming that the actual check of whether an email has bounced will be happening somewhere else? And if so, is there an existing ticket to implement that? I recall that checking for bounced email was marked as a to-do for some pages.

What I've understood from going through the code is that the user is redirected to the signin_bounced page by the session-verification-poll-mixin, which is used by choose_what_to_sync, confirm_signup_code, confirm, and sign_in_token_code. I know that at least in our existing React version of confirm_signup_code, this is an existing TODO. The session-verification-poll-mixin basically just polls for errors and then determines what to do next based on the incoming errors.
A tentative search of our existing tickets did not indicate to me that this work has been ticketed -- I'll make a ticket 👍
New ticket 🎟️ : https://mozilla-hub.atlassian.net/browse/FXA-7199

bcolsson · 2023-04-06T20:48:22Z

packages/fxa-settings/src/pages/Signin/SigninBounced/en.ftl

-# linkExternal is a link to a mozilla support
-signin-bounced-help = If this is a valid email address, <linkExternal>let us know</linkExternal> and we can help unlock your account.
+# supportButton is button which logs the user's action and navigates them to mozilla support
+signin-bounced-help = If this is a valid email address, <supportButton>let us know</supportButton> and we can help unlock your account.


Is it necessary to change <linkExternal> here? If so, we'll need to update the ID for signin-bounced-help to trigger updates for the tag change.

ohhh I see -- the HTML component which is being used had to be changed, so I changed the name to be reflective of that, but I can leave it as it was (it does still function as if it were an external link, it just also logs some metrics before redirecting the user.) Does changing the HTML component need to trigger changes (if we don't change the variable?)

The short answer is, if its purely a change on the backend and on the there doesn't appear to be any change to the end user, then we wouldn't need to update the string.

If the context as it appears to the end-user doesn't change - as in <linkExternal>/<supportButton> essentially look/function the same on the surface, then we wouldn't need to trigger changes. However, if for example the change here makes the text look like a button instead of a link, then perhaps it would be wise to update the variable and ID.

Variable name changes for existing strings end up requiring a new translation (but text is essentially the same as before), though likely a fairly simple update it still ends up requiring a translator to look at. For some locales, it could ultimately end up in fallback text appearing for some time until a team has the bandwidth to look at it.

I'll just set the var back to what it was in that case -- there will be no change to the appearance of the component on the page

Thank you @bcolsson !

Because: * We're moving the React components to their real routes and wiring them up to receive data This commit: * Adds signin_bounced into the React app Closes #FXA-6489

millmason commented Apr 4, 2023

View reviewed changes

pdehaan reviewed Apr 4, 2023

View reviewed changes

millmason force-pushed the FXA-6489-signin_bounced--setup branch from 123e5b5 to d3599be Compare April 4, 2023 23:31

millmason marked this pull request as ready for review April 4, 2023 23:47

millmason requested a review from a team as a code owner April 4, 2023 23:47

millmason force-pushed the FXA-6489-signin_bounced--setup branch from d3599be to 44f05dd Compare April 5, 2023 22:25

millmason requested a review from a team as a code owner April 5, 2023 22:25

millmason force-pushed the FXA-6489-signin_bounced--setup branch 2 times, most recently from c322f77 to 36d0d38 Compare April 6, 2023 16:10

vpomerleau approved these changes Apr 6, 2023

View reviewed changes

bcolsson reviewed Apr 6, 2023

View reviewed changes

feat(fxa-settings): setup signin_bounced in React

9ef8616

Because: * We're moving the React components to their real routes and wiring them up to receive data This commit: * Adds signin_bounced into the React app Closes #FXA-6489

millmason force-pushed the FXA-6489-signin_bounced--setup branch from 36d0d38 to 9ef8616 Compare April 7, 2023 21:55

bcolsson approved these changes Apr 10, 2023

View reviewed changes

millmason merged commit 90c1c14 into main Apr 10, 2023

millmason deleted the FXA-6489-signin_bounced--setup branch April 10, 2023 20:07

feat(fxa-settings): setup signin_bounced in React #15128

feat(fxa-settings): setup signin_bounced in React #15128

Uh oh!

Conversation

millmason commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Because:

This commit:

Checklist

Screenshots (Optional)

Other information (Optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpomerleau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millmason commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

millmason commented Apr 4, 2023 •

edited

Loading

millmason commented Apr 6, 2023 •

edited

Loading