Skip to content
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

fix(settings): Get email from account hook instead of location state #18157

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonalmeida
Copy link
Contributor

⚠️ I'm not really sure, what the right behaviour should be if the email is null.

Because

  • We want to handle the missing email case and not get this information from the location state.

This pull request

  • Switches to using useAccount.

Issue that this pull request solves

Closes: FXA-10876

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)

n/a

Other information (Optional)

Built on top of #18136. Will rebase when the prior PR lands.

@@ -44,11 +41,17 @@ const ResetPasswordWithRecoveryKeyVerifiedContainer = ({
useState<FinishOAuthFlowHandlerResult['error']>();

const account = useAccount();
const { uid, sessionToken } = currentAccount() || {};
// TODO: how do we properly handle the real non-null nature?
const { uid, sessionToken, email = '💩' } = currentAccount() || {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💩 🤣

currentAccount pulls from local storage right, which is what we want most of the time 👍 but just from a quick search, I only see us call storeAccountData in CompleteResetPassword so is this available from local storage at this point in the flow...? If not, then location state seems like maybe the correct place?

To hopefully answer the code comment question, before the return (<ResetPasswordWithRecoveryKeyVerified can we just check that these values are non-null and if so, redirect? See how we check for !email in the signin container component. (I don't think this is applicable here but if we ever have a case where we're passing non-null values into a component just to pass them back to a callback, we can curry them, see InlineRecoveryKeySetup and SetPassword containers)

FWIW related to your PR title, we shouldn't be using useAccount unless we want the entire GQL query to run, we've got a ticket to remove it in reset PW.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This page is reached after CompleteResetPassword so email should be stored in local storage at this stage - if it isn't, that means the user isn't signed in and should likely be redirected to /signin

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks Valerie, I can never keep these reset PW flows straight. So yeah, currentAccount is the right thing here and I think if the value is null we can just redirect to /signin as you said.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants