Skip to content
This repository has been archived by the owner on Jul 6, 2020. It is now read-only.

fix: Fix #123 Password Validation Error on Signup #124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mtahafarooq
Copy link
Contributor

@mtahafarooq mtahafarooq commented Apr 8, 2019

Fixes #123

Changes proposed in this pull request:

  • Changed label to 'password1'
  • Changed input label from 'password' to 'password1' before passing to formValueForLabel.

image

@codecov-io
Copy link

codecov-io commented Apr 8, 2019

Codecov Report

Merging #124 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #124   +/-   ##
=======================================
  Coverage   53.39%   53.39%           
=======================================
  Files          55       55           
  Lines        2358     2358           
  Branches      253      253           
=======================================
  Hits         1259     1259           
  Misses       1025     1025           
  Partials       74       74
Impacted Files Coverage Δ
src/app/components/auth/signup/signup.component.ts 74.19% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/app/components/auth/signup/signup.component.ts 74.19% <ø> (ø) ⬆️

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 7e7ce24...aab961b. Read the comment docs.

@mtahafarooq mtahafarooq changed the title fix: Fix Password Validation Error on Signup fix: Fix #123 Password Validation Error on Signup Apr 8, 2019
@@ -1,7 +1,7 @@
<div class="signup-container">
<app-input [isRequired]="true" [label]="'Username'" [type]="'text'" [theme]="'dark'" [icon]="'assets/images/username_dark.png'" #formsignup></app-input>
<app-input [isRequired]="true" [label]="'Email'" [type]="'email'" [theme]="'dark'" [icon]="'assets/images/envelope_dark.png'" #formsignup></app-input>
<app-input [isRequired]="true" [label]="'Password'" [type]="'password'" [theme]="'dark'" [icon]="'assets/images/password_dark.png'" #formsignup></app-input>
<app-input [isRequired]="true" [label]="'Password1'" [type]="'password'" [theme]="'dark'" [icon]="'assets/images/password_dark.png'" #formsignup></app-input>
Copy link
Member

Choose a reason for hiding this comment

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

Please elaborate a bit about these changes. Also let us know other scenarios like alphabet only, Capital letter, special character etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked other scenarios and it's breaking on entirely numeric password for me.

So, when the password is entirely numeric the server returns the error with the key "password1", now the problem is that there's a function called formValueForLabel whose job is to fetch the form field against the given key, since the submission has failed and we're in the error handling part, when given "password1" as the key the function fails to fetch the field since the label of the field is "password" and not "password1". That's why the error was not showing for entirely numeric password.

Copy link
Member

@RishabhJain2018 RishabhJain2018 left a comment

Choose a reason for hiding this comment

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

Screenshot 2019-05-09 at 2 58 18 AM

It gives the below error @mtahafarooq. Can you please fix this?

@lunayach
Copy link
Member

Hi @mtahafarooq, are you working on this or @RishabhJain2018 we could close this?

@Shekharrajak
Copy link
Member

Also, @Sanji515 @sanketbansal share your thoughts about this PR changes. Are you aware of this issue?

@Sanji515
Copy link
Member

Sanji515 commented Jun 24, 2019

I think this PR can be closed as @sanketbansal has build the authentication pages a/c to the current version of EvalAI so I think he have taken care of all these cases and if not then these all can be done in that PR

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

Successfully merging this pull request may close these issues.

Signup Form label mismatch for error handling
6 participants