[CORE-342] Submit Success Email#189
Conversation
There was a problem hiding this comment.
Problem
After a successful submit, the handler looked up the user's email addresses and sent thank-you messages synchronously before writing the HTTP response. Slow or hung SMTP relays could delay or block 201 even though the submission had already been persisted. Client retries in that window could also trigger duplicate emails.
Proposed change
- Return the HTTP response first — write 201 Created as soon as Submit succeeds.
- Dispatch email in the background — start a detached goroutine after the response is sent:
go email.SendSubmissionEmails(context.WithoutCancel(traceCtx), currentUser.ID, newResponse)
| for _, email := range emails { | ||
| err := h.mailer.SendSubmissionMail(traceCtx, email, newResponse) | ||
| if err != nil { | ||
| logger.Error( | ||
| "send submission email failed", | ||
| zap.Error(err), | ||
| zap.String("email", email), | ||
| zap.String("user_id", currentUser.ID.String()), | ||
| zap.String("response_id", responseID.String()), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| submitResponse := Response{ |
There was a problem hiding this comment.
Consider making thank-you email delivery asynchronous by running it in a detached goroutine after the response is sent. By doing so, the HTTP 201 responses are not blocked by SMTP
go email.SendSubmissionEmails(context.WithoutCancel(traceCtx), currentUser.ID, newResponse)
| } | ||
| } | ||
|
|
||
| func (s *SMTPSender) Send( |
There was a problem hiding this comment.
Consider adding a timeout to Email submission, so that it can be shut down if it cannot be sent successfully.
There was a problem hiding this comment.
Consider using a constant to make maintenance easier.
| ID: newResponse.ID.String(), | ||
| FormID: newResponse.FormID.String(), | ||
| CreatedAt: newResponse.CreatedAt.Time, | ||
| UpdatedAt: newResponse.UpdatedAt.Time, |
There was a problem hiding this comment.
Consider using the submission time instead of the mail-sent time so it matches the user's actual form submission time.
There was a problem hiding this comment.
change in internal/email/service.go
There was a problem hiding this comment.
Since the UpdateSubmitted function updated SubmittedAt only, we should use SubmittedAt, not UpdatedAt.
| } | ||
| } | ||
|
|
||
| func (s *SMTPSender) Send( |
There was a problem hiding this comment.
Consider using a constant to make maintenance easier.
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| default: | ||
| if err := ctx.Err(); err != nil { |
There was a problem hiding this comment.
Consider separating all this kind of code into two lines for the team convention.
| ID: newResponse.ID.String(), | ||
| FormID: newResponse.FormID.String(), | ||
| CreatedAt: newResponse.CreatedAt.Time, | ||
| UpdatedAt: newResponse.UpdatedAt.Time, |
There was a problem hiding this comment.
Since the UpdateSubmitted function updated SubmittedAt only, we should use SubmittedAt, not UpdatedAt.
| emails, err := h.userStore.GetEmails(ctx, userID) | ||
| if err != nil { | ||
| logger.Error( | ||
| "get user emails failed", | ||
| zap.Error(err), | ||
| zap.String("user_id", userID.String()), | ||
| zap.String("response_id", newResponse.ID.String()), | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| for _, email := range emails { | ||
| if err := h.mailer.SendSubmissionMail(ctx, email, newResponse); err != nil { | ||
| logger.Error( | ||
| "send submission email failed", | ||
| zap.Error(err), | ||
| zap.String("email", email), | ||
| zap.String("user_id", userID.String()), | ||
| zap.String("response_id", newResponse.ID.String()), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider moving them to the service layer since they are not related to the HTTP request, which should be placed in the handler.
2 participants
Type of changes
Purpose
Additional Information