Merge pull request specklesystems#3382 from specklesystems/fabians/em…

…ail-verification-twice-fix fix(server): on sign up send out only 1 verification email
KatKatKateryna · Oct 23, 2024 · 5624f15 · 5624f15
2 parents def74d1 + f4b3b8a
commit 5624f15
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 61 deletions.
diff --git a/packages/server/modules/emails/index.ts b/packages/server/modules/emails/index.ts
@@ -1,59 +1,24 @@
 /* istanbul ignore file */
-import { db } from '@/db/knex'
 import { moduleLogger } from '@/logging/logging'
-import { UsersEmitter } from '@/modules/core/events/usersEmitter'
-import { getServerInfoFactory } from '@/modules/core/repositories/server'
-import { findPrimaryEmailForUserFactory } from '@/modules/core/repositories/userEmails'
-import { getUserFactory } from '@/modules/core/repositories/users'
-import { deleteOldAndInsertNewVerificationFactory } from '@/modules/emails/repositories'
-import { renderEmail } from '@/modules/emails/services/emailRendering'
 import * as SendingService from '@/modules/emails/services/sending'
-import {
-  initializeVerificationOnRegistrationFactory,
-  requestEmailVerificationFactory
-} from '@/modules/emails/services/verification/request'
 import { initializeTransporter } from '@/modules/emails/utils/transporter'
-import { Optional, SpeckleModule } from '@/modules/shared/helpers/typeHelper'
-
-let quitVerificationListeners: Optional<() => void> = undefined
+import { SpeckleModule } from '@/modules/shared/helpers/typeHelper'
 
 const emailsModule: SpeckleModule = {
-  init: async (app, isInitial) => {
+  init: async (app) => {
     moduleLogger.info('📧 Init emails module')
 
     // init transporter
     await initializeTransporter()
 
     // init rest api
     ;(await import('./rest')).default(app)
-
-    // init event listeners
-    if (isInitial) {
-      const getUser = getUserFactory({ db })
-      const initializeVerificationOnRegistration =
-        initializeVerificationOnRegistrationFactory({
-          userEmitterListener: UsersEmitter.listen,
-          requestEmailVerification: requestEmailVerificationFactory({
-            getUser,
-            getServerInfo: getServerInfoFactory({ db }),
-            deleteOldAndInsertNewVerification: deleteOldAndInsertNewVerificationFactory(
-              { db }
-            ),
-            findPrimaryEmailForUser: findPrimaryEmailForUserFactory({ db }),
-            sendEmail: SendingService.sendEmail,
-            renderEmail
-          })
-        })
-
-      quitVerificationListeners = initializeVerificationOnRegistration()
-    }
-  },
-
-  shutdown() {
-    quitVerificationListeners?.()
   }
 }
 
+/**
+ * @deprecated Use `sendEmail` from `@/modules/emails/services/sending` instead
+ */
 async function sendEmail({
   from,
   to,

diff --git a/packages/server/modules/emails/services/sending.ts b/packages/server/modules/emails/services/sending.ts
@@ -1,6 +1,7 @@
 import { logger } from '@/logging/logging'
 import { getTransporter } from '@/modules/emails/utils/transporter'
 import { getEmailFromAddress } from '@/modules/shared/helpers/envHelper'
+import { resolveMixpanelUserId } from '@speckle/shared'
 
 export type SendEmailParams = {
   from?: string
@@ -22,7 +23,7 @@ export async function sendEmail({
 }: SendEmailParams): Promise<boolean> {
   const transporter = getTransporter()
   if (!transporter) {
-    logger.warn('No email transport present. Cannot send emails.')
+    logger.warn('No email transport present. Cannot send emails. Skipping send...')
     return false
   }
   try {
@@ -34,6 +35,13 @@ export async function sendEmail({
       text,
       html
     })
+    logger.info(
+      {
+        subject,
+        distinctId: resolveMixpanelUserId(to || '')
+      },
+      'Email "{subject}" sent out to distinctId {distinctId}'
+    )
     return true
   } catch (error) {
     logger.error(error)

diff --git a/packages/server/modules/emails/services/verification/request.ts b/packages/server/modules/emails/services/verification/request.ts
@@ -3,7 +3,6 @@ import {
   FindPrimaryEmailForUser
 } from '@/modules/core/domain/userEmails/operations'
 import { UserEmail } from '@/modules/core/domain/userEmails/types'
-import { UsersEmitter, UsersEvents } from '@/modules/core/events/usersEmitter'
 import { getEmailVerificationFinalizationRoute } from '@/modules/core/helpers/routeHelper'
 import { ServerInfo, UserRecord } from '@/modules/core/helpers/types'
 import { EmailVerificationRequestError } from '@/modules/emails/errors'
@@ -184,25 +183,11 @@ export const requestEmailVerificationFactory =
     await sendVerificationEmailFactory(deps)(newVerificationState)
   }
 
-/**
- * Listen for user:created events and trigger email verification initialization
- */
-export const initializeVerificationOnRegistrationFactory =
-  (deps: {
-    userEmitterListener: typeof UsersEmitter.listen
-    requestEmailVerification: RequestEmailVerification
-  }) =>
-  () => {
-    return deps.userEmitterListener(UsersEvents.Created, async ({ user }) => {
-      // user might already be verified because of registration through an external identity provider
-      if (user.verified) return
-
-      await deps.requestEmailVerification(user.id)
-    })
-  }
-
 type RequestNewEmailVerificationDeps = CreateNewEmailVerificationFactoryDeps
 
+/**
+ * Request email verification for email with specified ID
+ */
 export const requestNewEmailVerificationFactory =
   (
     deps: RequestNewEmailVerificationDeps & SendVerificationEmailDeps

diff --git a/packages/server/modules/emails/tests/verifications.spec.ts b/packages/server/modules/emails/tests/verifications.spec.ts
@@ -70,10 +70,15 @@ describe('Email verifications @emails', () => {
     await cleanup()
   })
 
-  it('sends out verification email immediatelly after new account creation', async () => {
+  afterEach(async () => {
+    mailerMock.resetMockedFunctions()
+  })
+
+  it('sends out 1 verification email immediately after new account creation', async () => {
     const sendEmailInvocations = mailerMock.hijackFunction(
       'sendEmail',
-      async () => true
+      async () => true,
+      { times: 2 }
     )
 
     const newGuy: BasicTestUser = {
@@ -91,6 +96,9 @@ describe('Email verifications @emails', () => {
 
     const verification = await getPendingToken({ email: newGuy.email })
     expect(verification).to.be.ok
+
+    // There should be only 1 email!
+    expect(sendEmailInvocations.args.length).to.eq(1)
   })
 
   describe('when authenticated', () => {

diff --git a/packages/shared/src/core/helpers/tracking.ts b/packages/shared/src/core/helpers/tracking.ts
@@ -7,3 +7,5 @@ export function resolveMixpanelUserId(email: string): string {
 export function resolveMixpanelServerId(serverHostname: string): string {
   return md5(serverHostname.toLowerCase()).toUpperCase()
 }
+
+export const resolveDistinctId = resolveMixpanelUserId