-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
mailing#60: Fix regression where multiple bulk recipient option isn't respected #16223
Conversation
(Standard links)
|
test fails look related here @MegaphoneJon |
Jenkins re test this please |
'email' => 'mail1@example.org', | ||
'is_bulkmail' => 1, | ||
]); | ||
$this->callAPISuccess('email', 'create', [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->callAPISuccess('email', 'create', [ | |
$email2 = $this->callAPISuccess('email', 'create', [ |
'civimail_multiple_bulk_emails' => 1, | ||
]); | ||
$contactID = $this->individualCreate(['first_name' => 'test recipient']); | ||
$this->callAPISuccess('email', 'create', [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->callAPISuccess('email', 'create', [ | |
$email1 = $this->callAPISuccess('email', 'create', [ |
$this->assertEquals(2, $this->callAPISuccess('MailingRecipients', 'get', ['mailing_id' => $mailing['id']])['count']); | ||
$this->callAPISuccess('job', 'process_mailing', []); | ||
$this->_mut->assertRecipients([['mail1@example.org'], ['mail2@example.org']]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->callAPISuccess('Email', 'delete', ['id' => $email1['id']]); |
$this->callAPISuccess('job', 'process_mailing', []); | ||
$this->_mut->assertRecipients([['mail1@example.org'], ['mail2@example.org']]); | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
$this->callAPISuccess('Email', 'delete', ['id' => $email2['id']]); | |
} |
@MegaphoneJon this looks good to me the test failures are coming from your new test (data lying around) have added suggestions to make it happen and can you rebase this against 5.22 please as it is a regression it is better to get fixed into the RC |
Closing in favor of #16239 (rebase on 5.22) |
Overview
A regression in Civi 5.19+ causes "Enable bulk email recipients" setting in CiviMail to not be respected. In fact, the "bulk email' value isn't respected at all.
Before
Only a primary email address can receive a CiviMail email.
After
Original behavior restored.
Technical Details
The PR introducing the regression filters out emails from a mailing where e.g. a contact was marked as deceased after the mailing was queued but before it was sent. It also filters out folks who were marked "do not mail" or "on hold" between scheduling and sending. However, it erroneously includes a requirement that the email address be primary.