-
Notifications
You must be signed in to change notification settings - Fork 325
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
[WPB-8707] Remove phone functionality in the development client API version #4149
[WPB-8707] Remove phone functionality in the development client API version #4149
Conversation
9ca6d09
to
5fa63a1
Compare
82940a7
to
a244b39
Compare
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.
Looks good. If there is a good reason not to use the VersionedReqBody
wrapper, I'd like to know, and then it's good to go. Otherwise, it might be a sensible change?
:> CanThrow 'InvalidEmail | ||
:> CanThrow 'InvalidPhone | ||
:> "activate" | ||
:> ReqBody '[JSON] ActivateV5 |
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 we have a special type that can be used here as a wrapper: VersionedReqBody
:> CanThrow 'UserKeyExists | ||
:> CanThrow 'InvalidEmail | ||
:> CanThrow 'InvalidPhone | ||
:> CanThrow 'BlacklistedEmail | ||
:> CanThrow 'CustomerExtensionBlockedDomain | ||
:> "activate" | ||
:> "send" | ||
:> ReqBody '[JSON] SendActivationCodeV5 |
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.
Same as above
:> Summary "Authenticate a user to obtain a cookie and first access token" | ||
:> Description "Logins are throttled at the server's discretion" | ||
:> MakesFederatedCall 'Brig "send-connection-action" | ||
:> ReqBody '[JSON] LoginV5 |
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.
Same as above... and more below, will not comment on all
a244b39
to
efc32b9
Compare
For client API versions up to and including V5, the response is the same and the 'phone' field is always null. The field does not exist in versions V6 and above.
The response to `POST /register` no longer throws an error when the request contains a phone number. Instead, the "phone" and "phone_code" fields are not parsed and are therefore ignored.
The tests should have been dropped when removing phone number support in client API versions v0..v5 as they lost meaning at least then, if not even before that.
The test's name is suggesting one, but testing a different thing. The test does not make much sense in the current situation.
44a031f
to
e88c6e6
Compare
e88c6e6
to
1d1171a
Compare
The PR removes phone numbers and phone functionality from the development client API version, namely from v6.
A couple of up-for-debate choices have been made, and I'm open to discuss them:
phone
key was dropped from Brig'steam_invitation
DB table. Previous releases were still reading this column. As there is no Team Settings UI action to enter a phone number, so the column was never written to; hence not reading from the column will not miss to read actual phone numbers. Therefore, during deployment this will lead to benign 5xx errors. Related to this, the"phone"
field for endpoints that get one or more team invitations is silently ignored and the field has been removed from the request body of the v6 endpoint to send a team invitation."phone"
field of the request body toPOST /register
is now ignored instead of giving an error.Tracked by https://wearezeta.atlassian.net/browse/WPB-8707.
Checklist
POST /login/send
endpoint,PUT /self/phone
endpoint,DELETE /self/phone
endpoint,GET /self
endpoint to remove thephone
key from the response,GET /activate
endpoint to remove thephone
key from the response,POST /activate/send
endpoint to remove thephone
key from the request body, i.e., no ability to send an activation code via SMS,phone
field in the response to theGET /teams/invitations/info
endpoint,phone
field in the response to theGET /teams/{tid}/invitations
endpoint,phone
field in the response to theGET /teams/{tid}/invitations/{iid}
endpoint,phone
field from the request body ofPOST /teams/{tid}/invitations
endpoint,POST /register
endpoint to remove thephone
key both from the request and the response,POST /login
endpoint by removing phone-related keys, namelyphone
andcode
, from the request body,POST /activate
endpoint by removing thephone
key from the request body,changelog.d