feat(messaging): Adds Mailjet web hook event callbacks #33407
Conversation
There was a problem hiding this comment.
PR Summary
Implements Mailjet webhook integration for email event tracking, adding secure event handling and metric collection for email-related activities (sent, opened, clicked, etc.).
- Added
plugin-server/src/cdp/mailjet-metrics.tsfor tracking email event metrics with Prometheus counters - Implemented HMAC SHA-256 signature validation for webhook security in
/public/messaging/mailjet_webhookendpoint - Modified app metrics serializers to support Mailjet webhook events while removing 'composeWebhook' from request choices
- Consider aligning TypeScript interface with serializer choices to maintain consistency
3 files reviewed, 5 comments
Edit PR Review Bot Settings | Greptile
plugin-server/src/cdp/cdp-api.ts
Outdated
| } | ||
|
|
||
| const payload = `${timestamp}.${req.rawBody.toString()}` | ||
| const hmac = crypto.createHmac('sha256', this.hub.MAILJET_SECRET_KEY).update(payload).digest() |
There was a problem hiding this comment.
logic: MAILJET_SECRET_KEY might be undefined here. Add validation or provide a default value to prevent runtime errors.
| @@ -79,6 +81,7 @@ export class CdpApi { | |||
| router.get('/api/projects/:team_id/hog_functions/:id/status', asyncHandler(this.getFunctionStatus())) | |||
| router.patch('/api/projects/:team_id/hog_functions/:id/status', asyncHandler(this.patchFunctionStatus())) | |||
| router.get('/api/hog_function_templates', this.getHogFunctionTemplates) | |||
| router.post('/public/messaging/mailjet_webhook', asyncHandler(this.postMailjetWebhook())) | |||
There was a problem hiding this comment.
Have you given any thought as to how this would work for the different regions? EU / US? Do we have different mailjet accounts or something?
There was a problem hiding this comment.
Yeah we have 3 subaccounts, one for each environment:
| } | ||
|
|
||
| // Track Mailjet webhook metrics | ||
| // TODO: Zod validation |
There was a problem hiding this comment.
We should arguably validate the payload rather than a cast here. We can sort of trust it due to the signature validation but nonetheless
| // Track Mailjet webhook metrics | ||
| // TODO: Zod validation | ||
| const event = req.body as MailjetEvent | ||
| const category = EVENT_TYPE_TO_CATEGORY[event.event as keyof typeof EVENT_TYPE_TO_CATEGORY] |
There was a problem hiding this comment.
the "as" cast isn't great. This could lead to an undefined category (the typing is incorrect)
There was a problem hiding this comment.
typed the event field in the web hook body and added a type guard
| const event = req.body as MailjetEvent | ||
| const category = EVENT_TYPE_TO_CATEGORY[event.event as keyof typeof EVENT_TYPE_TO_CATEGORY] | ||
|
|
||
| if (event) { |
There was a problem hiding this comment.
General comment - tests 😅 tests tests tests
There was a problem hiding this comment.
Added some unit testing to mailjet-manager.service.test.ts, not sure if you want them in a separate file or if they are better as part of the CDP API tests in cdp-api.test.ts
There was a problem hiding this comment.
No here makes most sense. an e2e test at some point will be good but the bulk of testing should be localised to the service itself for sure
| import { parseJSON } from '../../../utils/json-parse' | ||
| import { MessagingMailjetManagerService } from './mailjet-manager.service' | ||
|
|
||
| describe('MessagingMailjetManagerService', () => { |
There was a problem hiding this comment.
I'm picky about tests because tests are important but also a source of pain over time if they aren't well maintained. A few things that could be done way better here commented where they are
plugin-server/src/cdp/services/messaging/mailjet-manager.service.test.ts
Outdated
Show resolved
Hide resolved
plugin-server/src/cdp/services/messaging/mailjet-manager.service.test.ts
Outdated
Show resolved
Hide resolved
| const event = req.body as MailjetEvent | ||
| const category = EVENT_TYPE_TO_CATEGORY[event.event as keyof typeof EVENT_TYPE_TO_CATEGORY] | ||
|
|
||
| if (event) { |
There was a problem hiding this comment.
No here makes most sense. an e2e test at some point will be good but the bulk of testing should be localised to the service itself for sure
Co-authored-by: Ben White <ben@posthog.com>
Problem
Part of #32436 (we shouldnt close it until its fully implemented tbh)
Changes
Adds a Mailjet CDP source to inject Mailjet web hook events
Did you write or update any docs for this change?
How did you test this code?
-unit testing for callbacks