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

Fix of campaign reschedule loop #7413

Merged
merged 1 commit into from May 12, 2019

Conversation

Projects
5 participants
@escopecz
Copy link
Member

commented Apr 12, 2019

Please be sure you are submitting this against the staging branch.

Q A
Bug fix? Y
New feature? N
Automated tests included? N
Related user documentation PR URL /
Related developer documentation PR URL /
Issues addressed (#s or URLs) #6971, #6625, #7226
BC breaks? N
Deprecations? N

Description:

We found a never ending loop that was creating massive amount of user notifications. When a contact is added to a campaign with the Send (marketing) Email event and the marketing email was sent to this contact previously then it the event will fail with error: Last execution error: John Doe has already received this marketing email.. The problem is that the event was rescheduled for another 30 minutes. And so this generated new notifications for the same contact and the same campaign event every 30 minutes.

The solution is to just send the error once without rescheduling.

Steps to reproduce the bug:

  1. Create a campaign that has 2 events to send the same marketing email. The second event will have delay of 3 minutes (for example)
  2. Trigger the campaign for 1 contact. It will send the email properly.
  3. Wait 3 minutes and trigger the campaign again.
  4. You should see the error message in the contact detail page in his history and the email should be scheduled for another 30 minutes.

You will see the notification created for every 30 minutes if you will test that long.

Steps to test this PR:

  1. Checkout this branch.
  2. Wait another 30 minutes or clone the campaign and test from the beginning.
  3. The scheduled email should disappear.

Only 1 notification should be created.

If a campaign send mail event fails because the contact already recei…
…ved it then do not fail the job for rescheduling. Just pass the error to show in the UI
@Carlos-mb

This comment has been minimized.

Copy link

commented Apr 29, 2019

I have modified the file CampaignSubscriber.php and it doesn't crash.. but it neither create the notification that the email was not sent because it was already sent. Is it a correct behaviour?

@escopecz

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

If it generates it only once and not every 30 minutes than that's how it should work after this patch is applied.

@Carlos-mb

This comment has been minimized.

Copy link

commented Apr 29, 2019

If it generates it only once and not every 30 minutes than that's how it should work after this patch is applied.

It generates none. Table maujs_notifications is empty

I have activated the old campaigns and it doesn't create any notification. I have a lot of marketing emails already sent because I have created alternative campaigns to workaround the issue. So, after activate the old campaigns, there should be about 500 notifications of marketing emails already sent.

@Carlos-mb

This comment has been minimized.

Copy link

commented Apr 29, 2019

This is what I have changed

image

@escopecz

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

@Carlos-mb I can't tell from your comments if it works for you or not.

@Carlos-mb

This comment has been minimized.

Copy link

commented Apr 29, 2019

I don't know if it works.

  1. It's not crashing (that's good)
  2. It's not creating thousand of notifications (it's good)
  3. It's not creating any notification (it's not good... It should create one per email already sent)
  4. I haven't info enough o know it it's sending rest of emails...

That's what I know right now.

@AlbanL74fr

This comment has been minimized.

Copy link

commented May 12, 2019

@escopecz @kuzmany I tried, and it's working as expected. Just a notification and no rescheduling message. Thanks

@kuzmany kuzmany merged commit 515ca6d into mautic:staging May 12, 2019

2 checks passed

Scrutinizer Analysis: 1 new issues – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Mautic 2 automation moved this from Ready to Test (first time) to Merged May 12, 2019

@escopecz escopecz deleted the mautic-inc:campaign-event-reschedule-cases branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.