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

Fixes several issues with email sends #1266

Merged
merged 8 commits into from Dec 23, 2015

Conversation

alanhartless
Copy link
Contributor

Description

This PR fixes three main bugs discovered.

  1. Fixes Possibile BUG e-mail link tracking #1246 where links in emails send through a campaign were converted to trackables only for the first email. Subsequent emails did not have trackable links. (List sends were not affected).
  2. Emails sent through lead lists (batched) all had the same bounce/return path email rendering bounces in-effective for batch sends.
  3. If a lead was part of more than one list assigned to an email, the ajax send process would never finish due to the count including duplicate leads but the actual lead processing did not.

(Also accidentally committed a small fix to forms prepopulating values for hidden fields https://github.com/mautic/mautic/pull/1266/files#diff-068791cef1a33e1b839aff9add5a4ba4R494)

Testing

Trackable Links

  1. Create a lead list with multiple leads
  2. Create a template email with multiple links (try to test at least one {assetlink=ID}, {pagelink=ID} and any external link).
  3. Create a campaign that simply sends the above email
  4. Trigger the email and review the source of the emails received. The very first email will have links converted to trackables. Subsequent emails will not. After the PR, all links in all emails should be trackable.

Duplicate Bounce/Return Path and Ajax Batch Issue

  1. Setup bounce monitoring in Configuration
  2. Create a second list that includes at least one of the leads that is in the first lead list.
  3. Clone the above email and convert it to a List Email. Assign both lead lists created above.
  4. Send the email. Note the count of pending. When you send, notice that the total number of leads do not match the pending count (it'll have a difference of the leads belonging to both lists). Also note that the process will never finish since the number process never matches the total number to process. Once it's noticed that the process won't finish, refresh the page. Now there will be 0 leads pending.
  5. Review the emails sent. They will all have the same return path email (something+bounce_STRING@something.com).
  6. Apply the PR and either truncate the email_stats table, or simply clone the email and try again. This time, notice the process should finish. Also notice that the return paths should now be unique per email sent.

@alanhartless alanhartless added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels Dec 21, 2015
@alanhartless alanhartless added this to the 1.2.3 milestone Dec 21, 2015
@dbhurley dbhurley added the T3 Hard difficulty to fix (issue) or test (PR) label Dec 22, 2015
@@ -90,6 +90,9 @@
'mautic.emailbuilder.subscriber' => array(
'class' => 'Mautic\EmailBundle\EventListener\BuilderSubscriber'
),
'mautic.emailtoken_subscriber' => array(
Copy link
Member

Choose a reason for hiding this comment

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

I know this is minor but wouldn't this be more uniform as mautic.emailtoken.subscriber?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-) A true OCD comment. Yes, indeed and it would have bothered me too if I had noticed it. Fixed.

@dbhurley
Copy link
Member

👍 This worked for me and fixed issues.

@dbhurley dbhurley added the pending-test-confirmation PR's that require one test before they can be merged label Dec 22, 2015
@escopecz
Copy link
Sponsor Member

While testing the trackable links issue on the latest staging, cache cleared, migrations applied, latest DB schema (all double checked), I got this error while triggering campaigns:

Triggering events for campaign 46
Triggering first level events
45 total events(s) to be processed in batches of 100
  4/45 [==>-------------------------]   8%                                                                                                              
  [Doctrine\ORM\ORMInvalidArgumentException]                                                                      
  A new entity was found through the relationship 'Mautic\PointBundle\Entity\LeadPointLog#lead' that was not con  
  figured to cascade persist operations for entity: Mautic\LeadBundle\Entity\Lead with ID #98. To solve this iss  
  ue: Either explicitly call EntityManager#persist() on this unknown entity or configure cascade persist  this a  
  ssociation in the mapping for example @ManyToOne(..,cascade={"persist"}). 

@alanhartless
Copy link
Contributor Author

Try to disable any point actions you have just to test this PR.

As for the error, it looks very similar to that referred to in #1221. What's the setup of your campaign and what point actions do you have?

@escopecz
Copy link
Sponsor Member

My campaign is simple as you've suggested. A list > send email. The active point actions are:

  • email sends > add 1 point
  • submits form > add 4 points
  • visits specific URL with a wildcard > add 13 points
  • email sends > add 33 points
  • email sends > add 33 points

When disabled, no error appears when the campaigns are triggered. Test continues...

@escopecz
Copy link
Sponsor Member

I was able to replicate the tracking issue and this PR fixes it. 👍

However, I'm not able to configure the bounce monitoring with my email provider so I probably cannot test the second issue.

alanhartless added a commit that referenced this pull request Dec 23, 2015
@alanhartless alanhartless merged commit 1422309 into mautic:staging Dec 23, 2015
@alanhartless alanhartless deleted the campaign-email-link-fix branch December 23, 2015 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs pending-test-confirmation PR's that require one test before they can be merged ready-to-test PR's that are ready to test T3 Hard difficulty to fix (issue) or test (PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibile BUG e-mail link tracking
3 participants