Repository navigation
Feat: Adding email_login (legacy) and login to passwordless - #267
4x4notfound wants to merge 2 commits into
Conversation
4x4notfound
changed the title
|
this has been open for 2 days |
fix: new newline at end of test file docs: changelong and version bump fix: typo fix: more typos feat: deprecate legacy passwordless login in favor of new grant type login docs: updated changlog and docs wip: code -> otp fix: test
lbalmaceda
left a comment
lbalmaceda
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I left some comments. If you are going to resolve them, make sure to update the docs and tests to match what has changed. Let me know if you have questions!
| } | ||
| ) | ||
|
|
||
| def sms_login_legacy(self, client_id, phone_number, code, scope='openid'): |
There was a problem hiding this comment.
The passwordless login functionality is already covered by sms_login. What's this method for?
There was a problem hiding this comment.
The API uses just login with a type parameter which I've added
There was a problem hiding this comment.
I went through this PR again. Let's remove this method sms_login_legacy and keep sms_login for compatibility purposes.
From now on, the documented way to consume the passwordless endpoint should be through the login method that you added.
| data=data | ||
| ) | ||
|
|
||
| def login(self, |
There was a problem hiding this comment.
You copy-pasted the "login for database" method signature. For passwordless there are parameters you don't require. The signature should take: client_id, email, code, scope where scope would have a default value of "openid".
Please, rename this method to email_login to match the existing SMS variant.
There was a problem hiding this comment.
The API has a single endpoint for login. In this case setting the realm and the username (email or phone) will work. This method is a direct mapping to that.
The existing sms_login methods uses a different endpoint which is why I named and aliased it with legacy and didn't add an email_login
lbalmaceda
added
the
waiting for customer
lbalmaceda
left a comment
lbalmaceda
left a comment
There was a problem hiding this comment.
Thanks for the changes and apologies for the delay. I think this PR is adding a bit of complexity to the existing API design, which I will grant you it's really old (+4 years).
I believe that the way the passwordless module of this library is today, exposing two separate methods for the SMS and email "start" step of the passwordless flow, should be followed by two separate methods for the SMS and email "completion" step of the passwordless flow. Adding a "generic" login method could be a bit confusing to our devs.
That said, it's been a long time since this PR and feature request has been opened. I'll accept the generic login method but not additional "legacy" methods. I left a comment to remove sms_login_legacy and to update the way we signal deprecation. You will have to update the tests as well.
Hopefully soon we can start looking into how the next major version of this SDK would look like. Once again, thanks for the contribution.
| } | ||
| ) | ||
|
|
||
| def sms_login_legacy(self, client_id, phone_number, code, scope='openid'): |
There was a problem hiding this comment.
I went through this PR again. Let's remove this method sms_login_legacy and keep sms_login for compatibility purposes.
From now on, the documented way to consume the passwordless endpoint should be through the login method that you added.
| def sms_login(self, client_id, phone_number, code, scope='openid'): | ||
| """Login using phone number/verification code. | ||
|
|
||
| Note: this method should be depcrecated in the future. |
There was a problem hiding this comment.
Let's print a warning instead (watch for the import)
See reference
|
Closing in favor of #279. Thanks for your contribution. |
Changes
Please describe both what is changing and why this is important. Include:
email_login_legacytoPasswordlesslogintoPasswordlessReferences
Please include relevant links supporting this change such as a:
Testing
Checklist