-
Notifications
You must be signed in to change notification settings - Fork 324
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
chore: migrate EmailVerification to pg #9492
base: master
Are you sure you want to change the base?
Conversation
Sasha fell asleep early, and I felt itchy |
@@ -55,9 +57,9 @@ const signUpVerified = async (email: string) => { | |||
expect(verifyEmail).toMatchObject({ | |||
data: { | |||
verifyEmail: { | |||
authToken: expect.toBeString(), |
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 was getting TS failures here
@@ -153,7 +155,7 @@ test.skip('autoJoin on multiple teams does not create duplicate `OrganizationUse | |||
const newEmail = `${faker.internet.userName()}@${domain}`.toLowerCase() | |||
const {user: newUser} = await signUpVerified(newEmail) | |||
|
|||
expect(newUser.tms).toIncludeSameMembers(teamIds) | |||
expect(newUser.tms).toEqual(expect.arrayContaining(teamIds)) |
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.
and here...
@@ -12,16 +10,14 @@ interface Input { | |||
} | |||
|
|||
export default class EmailVerification { | |||
id: string |
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.
Once again, id is now managed by PG
d67de35
to
98755f9
Compare
.selectAll() | ||
.where('email', '=', email) | ||
.where('expiration', '>', new Date()) | ||
.executeTakeFirst()) || null |
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.
+1 no need to coerce to null since kysely will return undefined
which works just as well in this case
.selectFrom('EmailVerification') | ||
.selectAll() | ||
.where('token', '=', verificationToken) | ||
.executeTakeFirst()) as EmailVerification) || null |
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.
+1 no need to coerce to EmailVerification || null since kysely knows the return type
Description
Resolves #9491
Testing scenarios
Migrations
EmailVerification
on pgCan verify email address with token
Final checklist