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

CRM-20789 Fix unsubscribing when link comes from an AB Test #10583

Merged
merged 2 commits into from
Jul 2, 2017

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Jun 29, 2017

CRM-20789 Fix retrieving entity when in AB test context
@seamuslee001
Copy link
Contributor Author

Just commenting this has been tested by AUG works in both AB and non ab testing modes

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton @totten

@seamuslee001
Copy link
Contributor Author

This was a bug introduced i believe in 4.7.20 hence why i have put it against the RC

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton does this look ok to you?

@seamuslee001
Copy link
Contributor Author

@jitendrapurohit Jitendra as you last did work in this area does this make sense to you?

@jitendrapurohit
Copy link
Contributor

sure, will test this later today.

@jitendrapurohit
Copy link
Contributor

jitendrapurohit commented Jun 30, 2017

ok - I wasn't able to replicate this on my local. Steps I performed was -

  1. Create an AB test mailing with Test entirely different emails option.
  2. Select existing group in recipients.
  3. Include unsubscribe token in body and Submit the mailing.
  4. Check the mail log -> paste the unsub url in the browser and try to unsubscribe - It worked correctly for me.

Also tried with including/excluding previous mailing in step 2 - same result. Please let me know if you think I'm missing anything.


// If $entity is null and $mailing_Type is either winner or experiment then we are deailing with an AB test
$abtest_types = array('experiment', 'winner');
if (empty($entity) && in_array($mailing_type, $abtest_types)) {
Copy link
Contributor

@jitendrapurohit jitendrapurohit Jun 30, 2017

Choose a reason for hiding this comment

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

I understand this aims to fix the query when $entity (entity_table column from civicrm_mailing_group) is NULL, but it always gets set to some value for me whether I choose recipients to be existing group(sets to civicrm_group) or previous mailing(civicrm_mailing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jitendra was $mailing_id resolving to the id of the A Mailing or the B Mailing or the Final. We were finding issues when $mailing_id = the b or final and there was nothing in the mailing_group table for the b or final.

Copy link
Contributor

@jitendrapurohit jitendrapurohit Jul 1, 2017

Choose a reason for hiding this comment

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

For me - mailing_group api called here always ensured entity_table to set to either group or mailing. And if it doesn't in your case, not sure but shouldn't the fix be more likely to set this column instead of fixing the query if it is empty ?

Update - the above user tag just notified another jitendra on github, for me it is @jitendrapurohit :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jitendrapurohit the issue for me is that in civicrm_mailing_group the only records there are for the A mailing so when someone clicks a link that is linked to either the mailing id of B or Final then $entity ends up being entity as there are no records for those mailings in civicrm_mailing_group

Copy link
Contributor

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

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

Okay - based on the last comment by seamus, this makes sense when a unsub url from a mailing not present in civicrm_mailing_group is triggered in the url(though I wasn't able to replicate). And its seems to be pretty safe and modifies the code only for the ab test type mailings with limited scope.

@eileenmcnaughton eileenmcnaughton merged commit 63de7b4 into civicrm:4.7.21-rc Jul 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants