Skip to content
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

Bounced / failed not set correctly #14

Open
1 task
mzagmajster opened this issue Mar 11, 2022 · 23 comments
Open
1 task

Bounced / failed not set correctly #14

mzagmajster opened this issue Mar 11, 2022 · 23 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@mzagmajster
Copy link
Collaborator

mzagmajster commented Mar 11, 2022

If the status comes through webhook than it's a bounce (soft or hard) as indicated by webhook. If the message fails to send for example invalid credentials, incorrect sender/domain setting, etc. it should not be considered as a bounce as it was not sent in the first place and bounces result in recipient addresses being disabled

  • Check also the severity of bounce

SOURCE NOTES:

vendor\symfony\http-foundation\Response.php
vendor\swiftmailer\swiftmailer\lib\classes\Swift\Events\SendEvent.php
vendor\swiftmailer\swiftmailer\lib\classes\Swift\Mime\SimpleMessage.php
vendor\swiftmailer\swiftmailer\lib\classes\Swift\Mime\MimePart.php
vendor\swiftmailer\swiftmailer\lib\classes\Swift\Mime\SimpleMimeEntity.php
case Response::HTTP_INTERNAL_SERVER_ERROR:
case Response::HTTP_TOO_MANY_REQUESTS:


Discussion on forum: https://forum.mautic.org/t/failed-email-status-only-after-three-attempts/23522

@mzagmajster mzagmajster added the bug Something isn't working label Mar 11, 2022
@mzagmajster mzagmajster self-assigned this Mar 11, 2022
@mzagmajster
Copy link
Collaborator Author

image

I checked soft and hard bounce have an event failed, so even if I somehow adjust the logic to display message under failed graph column it would not make a difference since the vent is always the same.

@axelocz
Copy link
Contributor

axelocz commented Mar 13, 2022

You are confusing 2 different things. Mautic side failed and delivery side. When mautic fails to send the message, eg. It's not passed to server for sending it needs to be set as failed. That is sending failed.

A failed event coming from webhook is a Bounce event and there is also its further classification but thats a delivery failure so it means that email was actually passed from Mautic to OV

@axelocz
Copy link
Contributor

axelocz commented Mar 13, 2022

Here is a simple way to replicate. Set mailer as owner so emails are sent using an address outside the omnivery.dev domain.

The calls to send the message FAIL during http with status 401. For some reason it's retried 3 times, even though this is a fatal authentication error.

As nothing is passed to OV there could be no webhook callback.

This must be treated as send failure instead of bounce as bounce affects the contact.

@axelocz
Copy link
Contributor

axelocz commented Mar 13, 2022

image

I checked soft and hard bounce have an event failed, so even if I somehow adjust the logic to display message under failed graph column it would not make a difference since the vent is always the same.

yes, the event for soft/hard bounce is always "failed" but the severity attribute varies (temporary/permanent). temporary fails (soft) should not result in the recipient being deactivated immediatelly but rather after multiple (usually 3) consecutive sends that fail. permament fails (hard bounce) should deactivate the recipient as bounced straight away.

@mzagmajster
Copy link
Collaborator Author

I see you removed the webhooks that I manually added in the platform, I needed them to test things in development mode (from my local machine).

Anyway the next time I adjust code for this issue I will make total mapping of webhook events (as well as the events that occur just on Mautic side and should be tracked as failed), then I will kindly ask you to review the mapping and verify that everything is set the way its supposed to be.

@axelocz
Copy link
Contributor

axelocz commented Mar 13, 2022

oh, sorry. i wanted to make sure that the events are processed using the callback_url. You can pass the callback_url from your dev machine just as well as from the live server.

@axelocz
Copy link
Contributor

axelocz commented Mar 17, 2022

the swiftmailer/transports/omniverapitransport.php doesn't consider the http response code. only 429 should be retried later - https://documentation.mailgun.com/en/latest/api-intro.html#date-format-1 if any of these fail its email send failure not bounce

@axelocz axelocz added this to the 1.0 milestone Mar 17, 2022
@axelocz
Copy link
Contributor

axelocz commented Mar 17, 2022

the following statuses must be set depending on when and how does the message fail:

failed during send with error codes other than 429 or 500 -> no retries, set as emails failed

failed event from webhook (this is what bounce events are):
severity: permanent (hardbounce/permanent_fail)-> record as bounce, set contact as DoNotContact::BOUNCED
severity: temporary (softbounce/temporary_fail) -> records as bounce, no changes to contact

rejected event from webhook:
set email as failed, no change of contact

complained event -> set contact as DoNotContact::UNSUBSCRIBED

unsubscribed event -> set contact as DoNotContact::UNSUBSCRIBED

delivered event -> ignore event as this is not supported by current version of Mautic

@mzagmajster
Copy link
Collaborator Author

mzagmajster commented Mar 18, 2022

This will require more investigation. I have to investigate methods in this class:

TransportCallback

When actual contact needs to be modified we should probably call addFailureByContactId

Instead of canUseChannelId a should probably use var like updateLead or something and when true setting this statuses with leadId and when its false without leadId.

I rewrote the logic in an effort to make everything more readable. Can I ask you you provide similar description for events: unsubscribed, complained, bounce and permanent_fail (maybe you can just edit the comment, where you have description for failed & rejected).

Also what do we do when severity is temporary?

If you see that an event is missing also let me know.

@axelocz
Copy link
Contributor

axelocz commented Mar 18, 2022

updated comment with list of events and statuses

@axelocz
Copy link
Contributor

axelocz commented Mar 18, 2022

I believe the failure should be returned in failedRecipients array.

@mzagmajster
Copy link
Collaborator Author

Here response codes are still not handled. If I understand this correctly you do not like that Mautic tries to send message again when it fails.

@axelocz
Copy link
Contributor

axelocz commented Mar 23, 2022

a) there is nothing to retry
b) the 3 retries then lead to the contact being marked as donotcontact

@mzagmajster
Copy link
Collaborator Author

Yeah the chosen tactic did not work for the response codes. Later today I will try dispatching mautic events from

EmailEvents.php:

  • EMAIL_FAILED
  • EMAIL_RESEND

I fixed the label for sending domain

@mzagmajster
Copy link
Collaborator Author

I looked at this again and again, there is no way for me to prevent calling endpoint again since its determined by onEmailResend from here: app\bundles\EmailBundle\EventListener\EmailSubscriber.php, which is the subsriber for events triggered here: app\bundles\EmailBundle\Command\ProcessEmailQueueCommand.php.

Without modifying the core I do not see how I could prevent this. What do you want to do next about the status codes. I have not really tested yet but the statuses should be correct on the graph now.

@mzagmajster
Copy link
Collaborator Author

mzagmajster commented Apr 2, 2022

Also I checked amazon transport implementation and I could not found any such logic about the status codes as you described above.

@axelocz
Copy link
Contributor

axelocz commented Apr 2, 2022

ok, i'll do some more testing. my concern with this is that in case someone misconfigures the sending (eg. sets an invalid from address) the contacts will be set as failed during sending which is less than optimal. I wonder how does sendgird transport handles this situation. There should be a way to indicate that sending failed rather than having it as bounced. Have you reached out to the Mautic team on the forums?

@mzagmajster
Copy link
Collaborator Author

Just checked for Sendgrid, they are checking 2xx, everything else is an error.

image

No additional manipulation on message as far as I can see. I will reach out to the community as well.

@axelocz
Copy link
Contributor

axelocz commented Apr 2, 2022

But what's in the exception matters. Check the SengridBadRequestException and SendGridBadLoginexception

@mzagmajster
Copy link
Collaborator Author

mzagmajster commented Apr 3, 2022

Exception is just an empty class, those exceptions are then caught by: app\bundles\EmailBundle\Swiftmailer\SendGrid\SendGridApiFacade.php

the class above is called by: app\bundles\EmailBundle\Swiftmailer\Transport\SendgridApiTransport.php (method send) -> and method from this class is called by mautic cronjob.

See the initial description for ticket on the forum.

@axelocz
Copy link
Contributor

axelocz commented Apr 3, 2022

i see it throws an exception "throw new \Swift_TransportException($e->getMessage());"

i wonder how is that used going forward.

@axelocz
Copy link
Contributor

axelocz commented Apr 3, 2022

also I have pinged Ruth Cheesley about the forum post

@axelocz axelocz added the enhancement New feature or request label Apr 27, 2022
@mzagmajster mzagmajster removed this from the 1.0 milestone Jun 3, 2022
@mzagmajster
Copy link
Collaborator Author

Do we have any next steps here, we can take? What do you suggest we do? I am curious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants