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

On Email Failed regular mark email as failed #11285

Open
wants to merge 3 commits into
base: 4.x
Choose a base branch
from

Conversation

kuzmany
Copy link
Member

@kuzmany kuzmany commented Jun 23, 2022

Q A
Bug fix? (use the a.b branch) [ ]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #...

Description:

Steps to test this PR:

On email failed we set contact to DNC.
But failed send email could have a lot of reasons (smtp timeout etc).
Then I suggest mark email as failed and do not set contact as DNC.

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Hard to tell how to reproduce. Need make failed send email
  3. Probably code review and unit tests are enoiught

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Jun 23, 2022
@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #11285 (68ebcd3) into 4.x (9092bbf) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                4.x   #11285      +/-   ##
============================================
+ Coverage     48.87%   48.92%   +0.05%     
+ Complexity    35395    35378      -17     
============================================
  Files          2144     2144              
  Lines        105510   105456      -54     
============================================
+ Hits          51563    51596      +33     
+ Misses        53947    53860      -87     
Impacted Files Coverage Δ
.../EmailBundle/Swiftmailer/Message/MauticMessage.php 71.42% <ø> (ø)
...dles/EmailBundle/EventListener/EmailSubscriber.php 95.45% <100.00%> (+17.67%) ⬆️
...es/CampaignBundle/EventListener/LeadSubscriber.php 58.69% <0.00%> (-20.52%) ⬇️
...p/bundles/LeadBundle/Entity/LeadListRepository.php 49.32% <0.00%> (-6.79%) ⬇️
...ndles/CampaignBundle/Entity/CampaignRepository.php 56.69% <0.00%> (-4.93%) ⬇️
app/bundles/EmailBundle/Entity/Copy.php 33.33% <0.00%> (-3.81%) ⬇️
app/bundles/UserBundle/Entity/User.php 80.26% <0.00%> (-0.67%) ⬇️
app/bundles/LeadBundle/Model/LeadModel.php 44.40% <0.00%> (-0.32%) ⬇️
app/bundles/IntegrationsBundle/Event/SyncEvent.php 0.00% <0.00%> (ø)
...MauticTagManagerBundle/Form/Type/TagEntityType.php 100.00% <0.00%> (ø)
... and 13 more

@escopecz escopecz added the WIP PR's that are not ready for review and are currently in progress label Jul 12, 2022
@kuzmany kuzmany added ready-to-test PR's that are ready to test enhancement Any improvement to an existing feature or functionality and removed WIP PR's that are not ready for review and are currently in progress labels Jul 14, 2022
@kuzmany kuzmany changed the title WIP: On Email Failed regular mark email as failed On Email Failed regular mark email as failed Jul 14, 2022
@mautibot
Copy link

This pull request has been mentioned on Mautic Community Forums. There might be relevant details there:

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

Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

I think it makes sense. The downside of this approach will be that if the email is failing for some reason like that the email inbox no longer exists then it will send the emails indefinitely. The upside is that this will work nicely with temporary failures.

Can you also write one more test to also test that the email stat was not failed if the stat is not failed? It's always good to test multiple scenarios.


public function testOnEmailFailed(): void
{
$this->mockSwiftMessage->leadIdHash = 'idhash';
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can we do something like this:

        $message               = new class() extends \Swift_Message {
            public $leadIdHash = 'some-hash';
        };

Instead of modifying the actual class to make a test work?

@escopecz escopecz added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-test PR's that are ready to test labels Jul 15, 2022
@RCheesley RCheesley added the code-review-needed PR's that require a code review before merging label Jul 15, 2022
@kuzmany
Copy link
Member Author

kuzmany commented Jul 15, 2022

The downside of this approach will be that if the email is failing for some reason like that the email inbox no longer exists then it will send the emails indefinitely.

This should cover bounce management of each transport.

@RCheesley RCheesley added the needs-rebase PR's that need to be rebased label Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The PR contributors have signed the contributors agreement code-review-needed PR's that require a code review before merging enhancement Any improvement to an existing feature or functionality needs-rebase PR's that need to be rebased pending-feedback PR's and issues that are awaiting feedback from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants