Skip to content

[Webhook] Add Mailchimp webhook #50324

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

Merged
merged 1 commit into from
Oct 7, 2024
Merged

[Webhook] Add Mailchimp webhook #50324

merged 1 commit into from
Oct 7, 2024

Conversation

JohJohan
Copy link
Contributor

@JohJohan JohJohan commented May 15, 2023

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #50285
License MIT
Doc PR symfony/symfony-docs#20286

This will add Mailchimp webhook see https://mailchimp.com/developer/transactional/docs/webhooks/

@carsonbot carsonbot added this to the 6.3 milestone May 15, 2023
@JohJohan JohJohan marked this pull request as draft May 15, 2023 10:27
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@OskarStark
Copy link
Contributor

Any plans to finish this PR @JohJohan ?

@JohJohan
Copy link
Contributor Author

JohJohan commented Jan 4, 2024

Any plans to finish this PR @JohJohan ?

Yes will wrap up today. For #50324 (comment) i will add the values in the test.json files and unset the values.

@OskarStark OskarStark changed the title [Webhook] Add Mailchimp webhook (fixes #50285) [Webhook] Add Mailchimp webhook Jan 4, 2024
@ghost
Copy link

ghost commented Jan 4, 2024

I see now MailChimp will send multiple events at once see https://mailchimp.com/developer/transactional/guides/track-respond-activity-webhooks/#handling-the-webhook-response-in-your-app

I'm thinking to make a class MailerDeliveriesEvent that contains Symfony\Component\RemoteEvent\Event\Mailer\MailerDeliveryEvent and will add email to MailerDeliveryEvent so you can handle each event. The concume should then look like:

    public function consume(RemoteEvent|MailerDeliveriesEvent $event): void
    {

        foreach ($event as $deliveryEvent) {
           // Handle the event based on email
           $deliveryEvent->getEmail()
        }
    }

Maybe you also have other suggestion? I dont like it that consume function will have the naming for 1 or multiple events and that is probably not a good idea.

Comment on lines 24 to 26
if (!\in_array($payload['event'], ['send', 'deferral', 'hard_bounce', 'soft_bounce', 'delivered', 'open', 'click', 'spam', 'unsub', 'reject'], true)) {
throw new ParseException(sprintf('Unsupported event "%s".', $payload['event']));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can avoid this redundant condition

See MailgunPayloadConverter

default => throw new ParseException(sprintf('Unsupported event "%s".', $payload['event'])),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done added the same default case

Comment on lines 30 to 31
'deferral', 'soft_bounce' => MailerDeliveryEvent::DEFERRED,
'hard_bounce' => MailerDeliveryEvent::BOUNCE,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In BrevoPayloadConverter, soft_bounce and hard_bounce match MailerDeliveryEvent::BOUNCE, should we do same here ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say yes. A soft bounce is not deffered

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added soft_bounce to MailerDeliveryEvent::BOUNCE

Comment on lines 68 to 75
if (null !== $payload['msg']['smtp_events'] && null !== $payload['msg']['smtp_events']['diag']) {
return $payload['msg']['smtp_events']['diag'];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your right an array of JSON objects, each of which is an SMTP response received for the message. Each item in the array will contain the following keys:

I will have to update to read every diag and concat them together

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now i go over the events and add them all to the reason log with type and diag

@alamirault
Copy link
Contributor

I see now MailChimp will send multiple events at once see https://mailchimp.com/developer/transactional/guides/track-respond-activity-webhooks/#handling-the-webhook-response-in-your-app

I'm thinking to make a class MailerDeliveriesEvent that contains Symfony\Component\RemoteEvent\Event\Mailer\MailerDeliveryEvent and will add email to MailerDeliveryEvent so you can handle each event. The concume should then look like:

    public function consume(RemoteEvent|MailerDeliveriesEvent $event): void
    {

        foreach ($event as $deliveryEvent) {
           // Handle the event based on email
           $deliveryEvent->getEmail()
        }
    }

Maybe you also have other suggestion? I dont like it that consume function will have the naming for 1 or multiple events and that is probably not a good idea.

Indeed, bulk event in payload is complicated with this signature

/**
* Parses an HTTP Request and converts it into a RemoteEvent.
*
* @return ?RemoteEvent Returns null if the webhook must be ignored
*
* @throws RejectWebhookException When the payload is rejected (signature issue, parse issue, ...)
*/
public function parse(Request $request, #[\SensitiveParameter] string $secret): ?RemoteEvent;

(Fore this case, SendgridRequestParser consume only the first event and ignore others 😨)

I'm not sure if it's the best way, but changing signature (without BC break) to return an array could be cleaner

    /**
     * Parses an HTTP Request and converts it into a RemoteEvent.
     *
     * @return array<RemoteEvent> Returns empty if the webhook must be ignored
     *
     * @throws RejectWebhookException When the payload is rejected (signature issue, parse issue, ...)
     */
    public function parse(Request $request, #[\SensitiveParameter] string $secret): array;

@JohJohan
Copy link
Contributor Author

JohJohan commented Apr 3, 2024

I see now MailChimp will send multiple events at once see https://mailchimp.com/developer/transactional/guides/track-respond-activity-webhooks/#handling-the-webhook-response-in-your-app
I'm thinking to make a class MailerDeliveriesEvent that contains Symfony\Component\RemoteEvent\Event\Mailer\MailerDeliveryEvent and will add email to MailerDeliveryEvent so you can handle each event. The concume should then look like:

    public function consume(RemoteEvent|MailerDeliveriesEvent $event): void
    {

        foreach ($event as $deliveryEvent) {
           // Handle the event based on email
           $deliveryEvent->getEmail()
        }
    }

Maybe you also have other suggestion? I dont like it that consume function will have the naming for 1 or multiple events and that is probably not a good idea.

Indeed, bulk event in payload is complicated with this signature

/**
* Parses an HTTP Request and converts it into a RemoteEvent.
*
* @return ?RemoteEvent Returns null if the webhook must be ignored
*
* @throws RejectWebhookException When the payload is rejected (signature issue, parse issue, ...)
*/
public function parse(Request $request, #[\SensitiveParameter] string $secret): ?RemoteEvent;

(Fore this case, SendgridRequestParser consume only the first event and ignore others 😨)

I'm not sure if it's the best way, but changing signature (without BC break) to return an array could be cleaner

    /**
     * Parses an HTTP Request and converts it into a RemoteEvent.
     *
     * @return array<RemoteEvent> Returns empty if the webhook must be ignored
     *
     * @throws RejectWebhookException When the payload is rejected (signature issue, parse issue, ...)
     */
    public function parse(Request $request, #[\SensitiveParameter] string $secret): array;

Yes i think only parsing the first message is not an option as you will miss the rest and in our cases allot is bulked together so it would not be worth implementing that i think.

Indeed updating parse to be able to return an array would be a solution but that will for sure bring complications and will be breaking right. @fabpot maybe you have some insights on what to do next?

TLDR; The problem is that we get multiple message in one webhook request and the current interface allows for only parsing one message in a webhook request.

Copy link
Contributor Author

@JohJohan JohJohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for #58248 to be merged then i can update my code for implementation of multiple messages.

Comment on lines 24 to 26
if (!\in_array($payload['event'], ['send', 'deferral', 'hard_bounce', 'soft_bounce', 'delivered', 'open', 'click', 'spam', 'unsub', 'reject'], true)) {
throw new ParseException(sprintf('Unsupported event "%s".', $payload['event']));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done added the same default case

Comment on lines 30 to 31
'deferral', 'soft_bounce' => MailerDeliveryEvent::DEFERRED,
'hard_bounce' => MailerDeliveryEvent::BOUNCE,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added soft_bounce to MailerDeliveryEvent::BOUNCE

Comment on lines 68 to 75
if (null !== $payload['msg']['smtp_events'] && null !== $payload['msg']['smtp_events']['diag']) {
return $payload['msg']['smtp_events']['diag'];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now i go over the events and add them all to the reason log with type and diag

fabpot added a commit that referenced this pull request Sep 27, 2024
…moteEvent`'s (kbond)

This PR was merged into the 7.2 branch.

Discussion
----------

[Webhook] Allow request parsers to return multiple `RemoteEvent`'s

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Unblocks #50324
| License       | MIT

I've run into services that send their webhook events in bulk - a single request containing multiple events.

This is one idea to allow this. I'd like to get some input before going further.

TODO:
- [x] Update changelog
- [x] Update/add tests

Commits
-------

a1fcfae [Webhook] Allow request parsers to return multiple `RemoteEvent`'s
@fabpot
Copy link
Member

fabpot commented Sep 27, 2024

@JohJohan #58248 has been merged now.

@kbond
Copy link
Member

kbond commented Sep 27, 2024

@JohJohan check out #58403 to see how I implemented for Mailtrap. I suggest adding a batch.json/batch.php test that contains multiple events.

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @JohJohan! Could you add an entry in the bridge's CHANGELOG.md?

@JohJohan
Copy link
Contributor Author

JohJohan commented Oct 2, 2024

Looks good @JohJohan! Could you add an entry in the bridge's CHANGELOG.md?

Thanks for the feedback i updated src/Symfony/Component/Mailer/Bridge/Mailchimp/CHANGELOG.md and added:

7.2
---

* Add support for webhook

@kbond kbond added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 3, 2024
@JohJohan
Copy link
Contributor Author

JohJohan commented Oct 7, 2024

Its ready for review, i dont know why https://ci.appveyor.com/project/fabpot/symfony/builds/50744376 fails:

Testing C:\projects\symfony\src\Symfony\Component\Lock
SSSSSSSSSSSSSSSS..............SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS.  63 / 393 ( 16%)
.S.........SSSSSSSSSSSSSSSSSSSSSSSSSSF..........SSSSSSSSSSS.... 126 / 393 ( 32%)
..............SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS
Time: 00:50.021, Memory: 8.00 MB
There was 1 failure:
1) Symfony\Component\Lock\Tests\Store\RedisArrayStoreTest::testBackwardCompatibility
Failed asserting that false is true.
C:\projects\symfony\src\Symfony\Component\Lock\Tests\Store\AbstractRedisStoreTestCase.php:49
FAILURES!
Tests: 178, Assertions: 201, Failures: 1, Skipped: 124.
KO src\Symfony/Component/Lock
PHPUnit 9.6.21 by Sebastian Bergmann and contributors.

@fabpot
Copy link
Member

fabpot commented Oct 7, 2024

Thank you @JohJohan.

@fabpot fabpot merged commit 8b2ffdd into symfony:7.2 Oct 7, 2024
9 of 10 checks passed
@JohJohan JohJohan deleted the 50285 branch October 7, 2024 09:31
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Oct 7, 2024
…are)

This PR was merged into the 7.2 branch.

Discussion
----------

[Mailer][Webhook] add Mandrill webhook docs

Code PR: symfony/symfony#50324

Commits
-------

e8084a2 [Mailer][Webhook] Mandrill Webhook support
@fabpot fabpot mentioned this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed Webhook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Webhook] Add webhook implementation for Mailchimp
9 participants