-
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
Trim auth params before sending to the API (not while typing) #1546
Conversation
@luisrudge can you clarify your description please, you trim on submission of the field or it's only trimmed when an API request is made? |
@cocojoe I only trim the value being sent to the API. The input value is left untouched. |
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 think I found the MFA case is missing. That aside, could you edit the tests to show that spaces in between are not getting removed as well?
e.g. change " [email protected] "
to " email @me .com "
which should always end up in "email @me .com"
src/core/web_api/helper.js
Outdated
|
||
export function trimAuthParams(params = {}) { | ||
const { ...p } = params; | ||
['username', 'email', 'phoneNumber'].forEach(k => { |
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.
what about MFA code?
...this.authOpt, | ||
...authParams | ||
}); | ||
const loginOptions = trimAuthParams( |
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.
The function wrap feels ... wrong. Can trimAuthParams
be a part of normalizeAuthParams
? What if we need to do another transform here? Maybe there should be a prepareParams
or similar to handle everything?
} | ||
|
||
resetPassword(options, cb) { | ||
this.client.changePassword(options, cb); | ||
this.client.changePassword(trimAuthParams(options), cb); |
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.
Similar feedback here ... there might be another transform in the future, in which case this starts to get really hard to read.
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.
Have a few comments on clarity but functionality is a pass.
fix #1537
Changes
Instead of trimming email/username/phoneNumber while the user is typing, we're now trimming them when we send them to the server.
References
#1537
Testing