Details
-
Type:
Bug
-
Status: Done/Fixed
-
Priority:
Trivial
-
Resolution: Cannot Reproduce
-
Affects Version/s: 4.6.6
-
Fix Version/s: 4.6.16
-
Component/s: None
-
Labels:
-
Versioning Impact:Patch (backwards-compatible bug fixes)
-
Documentation Required?:None
-
Funding Source:Needs Funding
Description
After upgrading from 4.4 to 4.6 the person configured as the manual recipient on a scheduled reminder received 500 reminder emails.The scheduled reminder had previously caused a total of 34 past emails to go out.
I see that this contact was added to the action log as requiring an email in the query
INSERT INTO civicrm_action_log (contact_id, entity_id, entity_table, action_schedule_id)
SELECT c.id as contact_id, c.id as entity_id, 'civicrm_contact' as entity_table, 1 as action_schedule_id
FROM (civicrm_contact c)
LEFT JOIN civicrm_action_log reminder ON reminder.contact_id = c.id AND
reminder.entity_id = c.id AND
reminder.entity_table = 'civicrm_contact' AND
reminder.action_schedule_id = 1
WHERE c.is_deleted = 0 AND c.is_deceased = 0
AND c.id IN (2)
AND c.id NOT IN (
SELECT rem.contact_id
FROM civicrm_action_log rem INNER JOIN civicrm_membership e ON rem.entity_id = e.id
WHERE rem.action_schedule_id = 1
AND rem.entity_table = 'civicrm_membership'
)
GROUP BY c.id
;
And was then selected for a reminder for each matching membership with this
SELECT reminder.id as reminderID, reminder.contact_id as contactID, reminder.entity_table as entityTable, reminder., e.id as entityID, e. , mt.minimum_fee as fee, e.id as id , e.join_date, e.start_date, e.end_date, ms.name as status, mt.name as type
FROM civicrm_action_log reminder
LEFT JOIN civicrm_membership e ON e.id = reminder.entity_id
LEFT JOIN civicrm_membership_type mt ON e.membership_type_id = mt.id
LEFT JOIN civicrm_membership_status ms ON e.status_id = ms.id
WHERE reminder.action_schedule_id = 1 AND reminder.action_date_time IS NULL
AND (e.id = reminder.entity_id OR reminder.entity_table = 'civicrm_contact')
;
I'm doing a little digging on this & will try to add a test but I'm not really sure the correct behaviour exactly...
Attachments
Issue Links
Activity
Hi Tim,
intended behaviour is clear as mud to me but current behaviour does not seem good. Am assigning to you as you probably know more than anyone about this code at the moment - the comments I have made are mostly within the test in the PR
Tim - the PR I submitted against this was a test only - I see it is now in QA - does that mean there is a fix elsewhere & the test might pass if we run it again now?
Nope. FWIW, when reading the PR, there is a BAO change – I guessed that was a fix. But I didn't really understand the issue well enough from skimming. Just to be clear – you're saying that the problem is reproduced by the test?
Q: The subject-line talks about 4.6 "upgrade". Is there some reason to think that the upgrade logic was somehow involved... or are you just saying that the behavior in 4.4 and 4.6 are different?
Also: I'm not too keen to do anything with the old sched-reminder code (pre https://github.com/civicrm/civicrm-core/pull/6297 ). The refactored version is broken down into smaller chunks.
Not sure if it's related, but there was another recent bug+patch about excessive scheduled-reminders (CRM-17028).
Right - the test reproduces the problem - I can't recall the point of the BAO change - but in theory the test could go against master & if it's fixed it will pass.
I don't think it was the upgrade that caused it - it was the behaviour change that was the issue
Not sure best way forwards with this. The functionality was pretty badly broken since masses of emails went out. Test supplied which demonstrates but doesn't fix the behaviour.
The BAO fix in the test changed the number of mails going out from hundreds to one-per-run from memory - but this is pretty stale now for me & we removed all config that used that functionality in response to Pete getting hundreds of emails a day (or was it a cron)
The partial fix & additional unit test are here
https://github.com/civicrm/civicrm-core/pull/6537
I've closed out the PR & deleted the branch to reduce 'noise' but the links from that PR still work.
nb - probably worth trying the test against the reworked code in master at some point
No idea if current civi version is affected - closing due to lack of interest
OK I've done a test to 'spell out' what is currently happening & I'm sure that is wrong but I'm not sure what is right. I won't comment so much here as I have put lots of comments in the tests.
I have also added a fix that changes the tested/ documented behaviour to reduce the number of emails sent to the manual recipient to one per cron run - which I think it is better & cannot see a down-side too. But, the setting & it's intent still seem confusing to me.
Also, I thought that a reminder scheduled to go out on the membership join date would only go out once on that day - even if the job ran more than once, but that is not what the test shows.
https://github.com/civicrm/civicrm-core/pull/6537/files