[IMP] mail_tracking{,_mailgun}: process sent events and fix wrong failure event mappings... and refactor #787
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Who is calling to that method? |
pedrobaeza
reviewed
Oct 11, 2021
yajo
force-pushed
the
12.0-mail_trackin-process_sent
branch
from
f84fda4 to
0e28d09
Compare
This: |
yajo
changed the title
yajo
force-pushed
the
12.0-mail_trackin-process_sent
branch
from
0e28d09 to
aa8910b
Compare
|
Finally, ready to review. |
|
Travis is red. |
chienandalu
reviewed
Oct 13, 2021
yajo
force-pushed
the
12.0-mail_trackin-process_sent
branch
from
aa8910b to
2ba4f73
Compare
yajo
changed the title
Up until now, the `sent` event type was never processed, probably because it "made no sense", as an unsent email would never trigger an event. However, it makes sense to process it because you may have a local relay that transmits mails over to the mail provider. In those circumstances, you should have 2 "sent" events (one from the relay and another one from the provider). Also marked some useless parts of code for removal. @Tecnativa TT32365
yajo
force-pushed
the
12.0-mail_trackin-process_sent
branch
from
2ba4f73 to
ae3d727
Compare
yajo
changed the title
|
😓 hard one... ready to review. |
yajo
force-pushed
the
12.0-mail_trackin-process_sent
branch
2 times, most recently
from
2bedef6 to
2f399e7
Compare
|
Travis is green finally. |
pedrobaeza
reviewed
Oct 20, 2021
yajo
force-pushed
the
12.0-mail_trackin-process_sent
branch
from
2f399e7 to
cb9ccdf
Compare
yajo
force-pushed
the
12.0-mail_trackin-process_sent
branch
2 times, most recently
from
c1daa36 to
496461b
Compare
chienandalu
approved these changes
Oct 22, 2021
Before this patch, the module was designed after the [deprecated Mailgun webhooks][3]. However Mailgun had the [events API][2] which was quite different. Modern Mailgun has deprecated those webhooks and instead uses new ones that include the same payload as the events API, so you can reuse code. However, this was incorrectly reusing the code inversely: trying to process the events API through the same code prepared for the deprecated webhooks. Besides, both `failed` and `rejected` mailgun events were mapped to `error` state, but that was also wrong because [`mail_tracking` doesn't have an `error` state][1]. So the logic of the whole module is changed, adapting it to process the events API payload, both through controllers (prepared for the new webhooks) and manual updates that directly call the events API. Also, `rejected` is now translated into `reject`, and `failed` is translated into `hard_bounce` or `soft_bounce` depending on the severity, as specified by [mailgun docs][2]. Also, `bounced` and `dropped` mailgun states are removed because they don't exist, and instead `failed` and `rejected` properly get their metadata. Of course, to know the severity, now the method to obtain that info must change, it' can't be a simple dict anymore. Added more parameters because for example modern Mailgun uses different keys for signing payload than for accessing the API. As there are so many parameters, configuration is now possible through `res.config.settings`. Go there to autoregister webhooks too. Since the new webhooks are completely incompatible with the old supposedly-abstract webhooks controllers (that were never really that abstract), support for old webhooks is removed, and it will be removed in the future from `mail_tracking` directly. There is a migration script that attempts to unregister old webhooks and register new ones automatically. [1]: https://github.com/OCA/social/blob/f73de421e28a43d018176f61725a3a59665f715d/mail_tracking/models/mail_tracking_event.py#L31-L42 [2]: https://documentation.mailgun.com/en/latest/api-events.html#event-types [3]: https://documentation.mailgun.com/en/latest/api-webhooks-deprecated.html
yajo
force-pushed
the
12.0-mail_trackin-process_sent
branch
from
496461b to
0f37487
Compare
|
Please forward-port it to v13 and v14: /ocabot merge major |
|
This PR looks fantastic, let's merge it! |
|
Congratulations, your PR was merged at 153b4fb. Thanks a lot for contributing to OCA. ❤️ |
SiesslPhillip
pushed a commit
to grueneerde/OCA-social
that referenced
this pull request
Nov 20, 2024
Syncing from upstream OCA/social (13.0)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[FIX] mail_tracking: process sent events
Up until now, the
sentevent type was never processed, probably because it "made no sense", as an unsent email would never trigger an event.However, it makes sense to process it because you may have a local relay that transmits mails over to the mail provider. In those circumstances, you should have 2 "sent" events (one from the relay and another one from the provider).
Also marked some useless parts of code for removal.
@Tecnativa TT32365
[IMP] mail_tracking_mailgun: refactor to support modern webhooks
Before this patch, the module was designed after the deprecated Mailgun webhooks. However Mailgun had the events API which was quite different. Modern Mailgun has deprecated those webhooks and instead uses new ones that include the same payload as the events API, so you can reuse code.
However, this was incorrectly reusing the code inversely: trying to process the events API through the same code prepared for the deprecated webhooks.
Besides, both
failedandrejectedmailgun events were mapped toerrorstate, but that was also wrong becausemail_trackingdoesn't have anerrorstate.So the logic of the whole module is changed, adapting it to process the events API payload, both through controllers (prepared for the new webhooks) and manual updates that directly call the events API.
Also,
rejectedis now translated intoreject, andfailedis translated intohard_bounceorsoft_bouncedepending on the severity, as specified by mailgun docs. Also,bouncedanddroppedmailgun states are removed because they don't exist, and insteadfailedandrejectedproperly get their metadata.Of course, to know the severity, now the method to obtain that info must change, it' can't be a simple dict anymore.
Added more parameters because for example modern Mailgun uses different keys for signing payload than for accessing the API. As there are so many parameters, configuration is now possible through
res.config.settings. Go there to autoregister webhooks too.Since the new webhooks are completely incompatible with the old supposedly-abstract webhooks controllers (that were never really that abstract), support for old webhooks is removed, and it will be removed in the future from
mail_trackingdirectly. There is a migration script that attempts to unregister old webhooks and register new ones automatically.