-
Notifications
You must be signed in to change notification settings - Fork 26
Merge wied03/ENG-2158/change-password #171
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
Conversation
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.
self review
], | ||
"type": "urlParameter", | ||
"parameterName": "username", | ||
"parameterName": "loginId", |
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.
We didn't have anything using this particular API endpoint so this was not noticed but the parameter name was wrong. The action does not have a username
field, only a loginId
field.
The difference is noticeable in the java client https://github.com/FusionAuth/fusionauth-java-client/pull/144/files#diff-907b0157ed4d05913f38430e58f3513c6262bf0f51eb14b81ad41a190d8af575R547
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.
So this client method wouldn't have worked before?
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.
That's right. It would not have. We don't consume it ourselves so that's 1 reason we didn't notice.
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 want to make sure the username
parameter meant that it was broken before and now fixed with this change.
It seems odd that the loginIdTypes
comment is not capitalized, but I guess it ends up as a doc comment. It's probably fine either way.
], | ||
"type": "urlParameter", | ||
"parameterName": "username", | ||
"parameterName": "loginId", |
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.
So this client method wouldn't have worked before?
{ | ||
"name": "loginIdTypes", | ||
"comments": [ | ||
"the identity types that FusionAuth will compare the loginId to." |
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.
There are a couple other APIs that have the t
for this parameter. Can we fix those too?
"the identity types that FusionAuth will compare the loginId to." | |
"The identity types that FusionAuth will compare the loginId to." |
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.
Pushed in 0ddacb5 - everything will need re-approval as a result (app especially)
Related Client PRs