Skip to content
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

FEAT: replace digit code with email signup link #683

Closed

Conversation

khoa165
Copy link
Contributor

@khoa165 khoa165 commented Jun 24, 2023

Description 📣

FEAT: replace digit code with email signup link
Issue 58

Type ✨

  • Bug fix
  • New feature
  • Breaking change
  • Documentation

Tests 🛠️

https://www.loom.com/share/3ed03c737c5f4757bd29565867a54670?sid=0e46e618-55b3-4519-9bd0-234a5060a4aa

@dangtony98
Copy link
Collaborator

Looks good to me from the backend; the functionality worked well!

I glossed over the frontend but @vmatsiiako can you give it a look? — Prolly more familiar with the signup sequence.

@dangtony98 dangtony98 requested a review from vmatsiiako June 25, 2023 12:24
@khoa165
Copy link
Contributor Author

khoa165 commented Jun 27, 2023

Hope the Loom video is useful to peek at frontend. Lemme know if you want me to make any changes @vmatsiiako

@maidul98
Copy link
Collaborator

Can you please verify that when SMTP related environments are not added to the backend, the step to confirm the users email is skipped?

@vmatsiiako
Copy link
Collaborator

This looks great to me! The only small thing is: in the email, should we give people both the button and the link? This way people can copy it easier

@dangtony98
Copy link
Collaborator

I think the button should be good (people can right-click it to get the link); I don't think having the link explicitly there is common-practice.

I'll do one last run through it and check for the edge-case @maidul98 highlighted today, then merge :)

@khoa165
Copy link
Contributor Author

khoa165 commented Jun 30, 2023

@dangtony98 Hi Tony, indeed I didn't skip step 2 when smtp is not configured. My latest commit should have that fix. Before, step 2 always made the request to backend regardless if there is smtp or not, and even without the code, the server still returns 200 (which does not make sense) hence step 2 is skipped to step 3 regardless. Anyway, I fixed that, hope all should be good to merge. My latest commit changed like 1 or 2 lines so should be easy to review

@dangtony98
Copy link
Collaborator

@maidul98 Can you take a look at this?

I've tried the flow without SMTP configured and it didn't work; it skipped the user email confirmation step but when completing the user's account, it errored out on this in v3/signupController.ts:

if (!user || (user && user?.publicKey)) {
	return res.status(403).send({
		error: "Failed to complete account for complete user",
	});
}

Is the flow typically supposed to work when SMTP is not configured?

@maidul98
Copy link
Collaborator

maidul98 commented Jul 14, 2023

@khoa165 The issue is you never called checkEmailVerificationCode. checkEmailVerificationCode needs to be called with a code even when smtp is disabled. This is because when this api call is made, it will create the user with a email which is needed for next step.

Because SMTP is disabled, the server will not do a code verification match as shown here

if (await getSmtpConfigured()) {

@maidul98
Copy link
Collaborator

Closing due to inactivity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants