-
Notifications
You must be signed in to change notification settings - Fork 556
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
Always remove spaces from email and username #1280
Conversation
src/__tests__/field/email.test.js
Outdated
}); | ||
describe('setEmail()', () => { | ||
it(`removes spaces`, () => { | ||
email.setEmail(Immutable.fromJS({}), ' email@te st.com '); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree leading and trailing whitespaces should be trimmed, i think having whitespaces on the middle of an email address or username is an "invalid value" rather.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why show "invalid field" if we can remove them at once and move on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was just typing this out, actually. Fully agree (if spaces are actually invalid characters ... are they excluded by the platform?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's a user's typing error and we should point them that it's an invalid value and they should fix it. If we don't, they might think they registered using "this email @address.com" and when they try to log in on other platform they won't be allowed.
Also, A0 username regex: ^[a-zA-Z0-9_]+$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/field/email.js
Outdated
@@ -15,8 +15,8 @@ export function isEmail(str) { | |||
return !!result && result[0] !== null; | |||
} | |||
|
|||
export function setEmail(m, str) { | |||
return setField(m, 'email', str, str => { | |||
export function setEmail(m, str = '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're adding a default value here (if str is not present). Isn't this breaking further validation? I know you do this so you can call trim without checking the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to be sure you would not get null/undefined, but the code is already not throwing here, so we're fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine!
fix #1180