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

Prioritize using user.username whenever provided #8050

Merged
merged 6 commits into from
May 12, 2021
Merged

Conversation

wlee221
Copy link
Contributor

@wlee221 wlee221 commented Apr 8, 2021

Description of changes

This properly handles username input on confirmSignUp whenever usernameAlias is set to phone_number.

Problem
Before confirmPassword is submitted, we stringify phone_number using composePhoneNumberInput(this.phoneNumber). The problem occurs when a valid username has already been passed through the user prop. In that case, we disable the form for username and just use the provided user.username. But we still try to compose phone number from this.phoneNumber, which is undefined, so we get an error message for that.

This PR fixes it by prioritizing username to be user.username and only look at form values whenever it's not the case.

Issue #, if available

Fixes #7267

Description of how you validated changes

Since username validations has been problematic especially with custom usernameAlias, I'll do testing across multiple auth components.

  • Case 1: usernameAlias = 'phone_number': testing with gist
  • Case 2: usernameAlias = 'email': testing with gist

Checklist

  • PR description included
  • yarn test passes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@wlee221 wlee221 marked this pull request as draft April 8, 2021 00:01
@@ -0,0 +1,5 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Is this file meant to be included / are the prettier changes meant to be included in this pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this isn't supposed to be here. I have added that to suppress prettier while I'm working on this, but I created #8136 to better address this.

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2021

Codecov Report

Merging #8050 (967f303) into main (3881d53) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8050   +/-   ##
=======================================
  Coverage   77.66%   77.66%           
=======================================
  Files         236      236           
  Lines       16505    16505           
  Branches     3535     3535           
=======================================
  Hits        12819    12819           
  Misses       3575     3575           
  Partials      111      111           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3881d53...967f303. Read the comment docs.

@wlee221 wlee221 marked this pull request as ready for review May 11, 2021 18:59
@wlee221 wlee221 requested a review from hkjpotato May 11, 2021 23:00
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2022
@jimblanc jimblanc deleted the get-username-fix branch November 23, 2022 15:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AmplifyConfirmSignUp usernameAlias="phone_number" not working
4 participants