Migrate gRPC VerificationService #19143
Conversation
|
I'm not sure how to test. We need to ask for help to clarify. cc @geropl |
jeanp413
force-pushed
the
hw/papi-auth
branch
from
5e46f90 to
faf58f4
Compare
| return user; | ||
| } | ||
|
|
||
| public async verifyOrgMembers(organizationId: string): Promise<void> { |
There was a problem hiding this comment.
is it fine that we are changing the logic here, are there DB tests for this service? If we are just moving functions without changing semantic is fine with me without adding tests, but if we start changing code we should rather start adding tests.
| }, | ||
| ); | ||
| const channel = phoneVerificationByCall ? "call" : "sms"; | ||
| const { verificationId } = await this.verificationService.sendVerificationToken( |
There was a problem hiding this comment.
we are missing analytics enent here which is present in JSON-RPC
I think much of code from JSON-RPC should have been moved to VerificationService and not copied in API layer, here we shoudl not have logic but rather only translations
| if (!req.token) { | ||
| throw new ApplicationError(ErrorCodes.BAD_REQUEST, "token is required"); | ||
| } | ||
| const phoneNumber = formatPhoneNumber(req.phoneNumber); |
There was a problem hiding this comment.
it should be part of verificaiton services, not copied pasted
svenefftinge
left a comment
svenefftinge
left a comment
There was a problem hiding this comment.
Code changes look good. I didn't test, though
Description
Summary generated by Copilot
🤖[deprecated] Generated by Copilot at 8ba3e63
No summary available (Limit exceeded: required to process 106320 tokens, but only 50000 are allowed per call)
Related Issue(s)
Fixes EXP-846
How to test
Test 1
Create a fresh GitHub account and sign in (use admin to unblock).
You should need to do phone number verification before you can start your first workspace.
Once the number is verified you should be able to work as usual.
Test 2
Use a GitHub account older than 30 days.
You should be able to start a workspace without verification.
Test 1 ++
delete the lastVerificationTimestamp in your user's additional Data.
if the account had started a workspace before, you should not need to verify
if the account doesn't have any workspaces, you should be asked to verify
Documentation
Preview status
Gitpod was successfully deployed to your preview environment.
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh. If enabled,with-previewandwith-large-vmwill be enabled./hold