(dev/core#285) Fixed second membership reminder #13487

Dawnthorn · 2019-01-20T06:01:06Z

Overview

With this change if you setup a reminder based on membership end date, the reminder will go out again if the membership is renewed and has a new end date.

Before

If you setup a scheduled reminder for memberships based on end date. It would send the first reminder, but if the user renewed their membership it wouldn't send out another reminder when the condition became true for the new end date.

After

Now it sends out a second reminder when the condition becomes true for the renewed membership.

Technical Details

I made the civicrm_activity_log.reference_date field a date time instead of just a date. This lets us always store the datetime that was used for the condition check. That way if the datetime changes (like the membership end date), we now know the condition needs to be checked again.

https://lab.civicrm.org/dev/core/issues/285

civibot · 2019-01-20T06:01:08Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

magnolia61 · 2019-01-20T07:28:34Z

Hi there, nice work for the membership renewal scheduled reminders. I am not yet convinced this solution would be wise for event (and other) scheduled reminders.

If I would change the start time of an event the day before. Would that trigger all of our 8 reminders relative to the start of the event again?

I am all for better scheduled reminders functionality but also for better ways to configure in such a way we can prevent massive unintentional mails being sent out.

So if this would be a checkbox in the UI I would endorse it right away. Checkbox: "re-evaluate when trigger date changes". I wonder how other people think about this.

Dawnthorn · 2019-01-20T08:34:49Z

Yeah. I think I was mislead by the simplicity. I've just pushed a version that works the same as the old way except it fixes the bugs. It should be pretty easy to add the UI checkbox later if someone wants to.

colemanw · 2019-01-23T22:01:44Z

@civicrm-builder retest this please

seamuslee001 · 2019-01-26T19:57:43Z

CRM/Upgrade/Incremental/sql/5.11.alpha1.mysql.tpl

@@ -1 +1,2 @@
 {* file to handle db changes in 5.11.alpha1 during upgrade *}
+ALTER TABLE civicrm_action_log CHANGE COLUMN reference_date reference_date datetime;


@Dawnthorn what is the reasoning behind this?

It's so that this methodology of storing the reference date will work with all the kinds of entities, not just memberships. For example, for activities, the reference date is activity_date_time which is a datetime field. Making this field date time makes it work for both dates and datetimes.

Dawnthorn · 2019-01-28T19:59:28Z

I'm not sure where to put this, but the work for fixing this bug was sponsored by the EFF.

mfb · 2019-03-22T21:01:46Z

The database migration will need to be updated to run on a later CiviCRM version. (Hopefully we can get this in soon without constantly revising that :)

eileenmcnaughton · 2019-03-28T21:48:59Z

CRM/Event/ActionMapping.php

@@ -158,7 +158,7 @@ public function createQuery($schedule, $phase, $defaultParams) {
    $query['casContactTableAlias'] = NULL;
    $query['casDateField'] = str_replace('event_', 'r.', $schedule->start_action_date);
    if (empty($query['casDateField']) && $schedule->absolute_date) {
-      $query['casDateField'] = $schedule->absolute_date;
+      $query['casDateField'] = "'{$schedule->absolute_date}'";


this feels wrong - why is it needed?

Yeah. This is wrong. Not because of the quoting I added though. We need to quote it because usually casDateField is a column name and gets added to the query raw. What's wrong with this is we need to escape it. I've pushed up a fix where I escape it.

I should also note that without the single quotes it causes a crash if you try and run an event scheduled reminder with an absolute date.

eileenmcnaughton · 2019-03-28T21:58:11Z

looking at this I think it needs pretty thorough engagement by someone else with skin in this issue (possibly @agh1 or @petednz or @JoeMurray or @seamuslee001 have? - although I may just be not letting good deeds go unpunished & suggesting the reward for having done work it be asked to do more)

I will email the dev list.

It looks like a valuable & much wanted change but it definitely needs a significant time commitment to review

eileenmcnaughton · 2019-03-28T22:34:58Z

I emailed the dev-list to try to get reviewers for this

jaapjansma · 2019-03-29T08:29:04Z

Do I understand it correctly that this PR introduces the functionality of sending a second reminder, even if the membership is renewed? If so I do think this should be optional, as with a few clients with reminders configured in such a way that we are happy with the second reminder not going out because people already have paid for another year.

agh1 · 2019-03-29T13:45:17Z

@jaapjansma there's currently a bug in the following scenario:

Alice joins in May 2017 for one year
In April 2018, Alice gets sent a reminder, adding a record to the activity log
She renews the next day, extending her membership for a year
In April 2019, you'd expect she'd get a reminder, but there's already a record in civicrm_activity_log for that reminder and membership ID combination, so she's never reminded.

jaapjansma · 2019-03-29T13:53:37Z

@agh thanks for the explanation. I had a client who had problems with membership renewals. My first thought was that the batch was too large to be processed in one go. But it might be that it was this bug.

nganivet · 2019-03-29T17:29:55Z

@yashodha Can you please have yourself or @sushantpaste review this PR? Thanks.

eileenmcnaughton · 2019-03-29T17:35:26Z

@Dawnthorn are you able to rebase this ?

yashodha · 2019-04-09T11:32:41Z

@Dawnthorn

Can you please re-base this?

mfb · 2019-04-12T21:11:20Z

In case it counts as a "review" of this PR, I would note that we are running this in production.

eileenmcnaughton · 2019-06-24T22:38:19Z

On the 3rd question I feel like perhaps @mfb has done code review on this (I'm guess technically we should have an outside person as this is consultant-client) but I'm also aware that @mfb has great tech chops and is a careful reviewer - @mfb how carefully have you looked at the code?

jackrabbithanna · 2019-06-24T23:16:56Z

@eileenmcnaughton the patch I posted was before this PR even existed, several weeks ago, for 5.7
We found that if a membership end dates were updated, that scheduled reminders would not go out during a cron job's execution to send them, well, only one would go out for the set that day, because only one would get their dates updated. May not be the best place to have posted that patch, but I was skimming PRs and this one seems at least tangentially relevant to what we found. When @josephlacey said "no previous reminders when out for the contact", that triggered me to remember this patch we had to do. I include for ya'll only as additional information. I hope it didn't distract from the main problem being discussed here, and it may not be related to this at all. Just food for thought. We hadn't gotten around to creating an issue yet for that patch we posted ... we'll do that soon.

Honestly, I'm surprised nobody noticed this bug before ... we have some sites where clients reported that membership reminders didn't go out, and it took some time to figure out why

eileenmcnaughton · 2019-06-24T23:31:29Z

@jackrabbithanna I think there has been ongoing issues in this area that have never quite been addressed :-( I love the idea of getting a bank of tests into place (like this PR has) & tightening the noose on all the issues - I just haven't managed to get a ringing endorsement to merge yet

jackrabbithanna · 2019-06-24T23:41:34Z

I see now this patch does away with the code we patched in Civi/ActionSchedule/RecipientBuilder.php
buildRelFirstPass() .. so maybe this will fix our issue too. The scheduled reminders that weren't going out were always memberships that had been renewed at least once, so the first scheduled reminder before the first renewal was working..... I'm reading this task more carefully, and the description is very similar to how we had described it internally. Let me see if I can get this PR tested by our team

jackrabbithanna · 2019-06-24T23:53:04Z

When I was reviewing the tests in 5.7 for this, I found that only one membership reminder was tested. Having a test where two memberships were created, end dates changed, and checking for 2 reminders go out would have caught the problem ... because the code in buildRelFirstPass() only updated the action log record dates for the first contact to get a reminder sent for the day, not the others. AFAIK

totten · 2019-06-25T00:33:06Z

(eileen) On the 3rd question I feel like perhaps @mfb has done code review on this (I'm guess technically we should have an outside person as this is consultant-client) but I'm also aware that @mfb has great tech chops and is a careful reviewer - @mfb how carefully have you looked at the code?

WRT review-thresholds and potential conflicts, I think we're OK there. The main concern is that someone goes "easy" on a review because of a conflict - but I think @Dawnthorn and @mfb have been pretty rigorous. Additionally, we have had feedback from folks with a other perspectives (like @magnolia61's points about events) that were assimilated, and there's r-run from folks at other shops. Moreover, the scheduled-reminder functionality has generally abided TDD practices for years, and this patch both passes and extends the relevant test-suite, so it avoids a lot of subjectivity (and is generally awesome wrt to r-maint).

I haven't gotten my head deep enough into this PR to form an opinion on the other merits or on meta-review, but I thought it might be useful to clarify that bit about the formalities.

eileenmcnaughton · 2019-06-25T02:52:05Z

@jackrabbithanna I think this PR adds those tests...

eileenmcnaughton · 2019-06-25T02:53:03Z

@totten didn't say this above but on chat I pinged him to lightly skim the code for any red flags & at a high level he thought the code looked like it was on track

eileenmcnaughton · 2019-06-25T02:53:38Z

@jackrabbithanna "Let me see if I can get this PR tested by our team" -> thank you

nganivet · 2019-06-25T06:53:47Z

@ all on this thread: THANK YOU!

JoeMurray · 2019-06-25T16:15:56Z

CRM/Upgrade/Incremental/sql/5.14.alpha1.mysql.tpl

@@ -1 +1,2 @@
 {* file to handle db changes in 5.14.alpha1 during upgrade *}
+ALTER TABLE civicrm_action_log CHANGE COLUMN reference_date reference_date datetime;


Does this need to go into a different file as 5.14 is released?

yes - we can fix after merge I think - since it's already changed version a few times

eileenmcnaughton · 2019-06-28T02:37:19Z

@ALL so here is where I think we are at

there is agreement that this is a bug
we agree it is a serious bug
there is also at least one tangental bug causing confusion
we have 2 people @mfb & @sushantpaste who have confirmed this works for them
@mfb is running in production & has been for months
@josephlacey's testing is so far inconclusive
the PR add a lot of tests & 'protects' the change
@totten has indicated that that the coding approach looks good & correct for a high level
it's unclear if anyone has done a real code review and understood the exact changes being made - but @mfb may have?
I've put out multiple requests via dev list & other channels to get people to engage with this & the people on this gitlab thread are the ones who have responded.
the PR is still missing a definitive review that would say ' I person x have gone through this code carefully and have high degree of confidence it won't make unintended changes and that the logic being applied is correct at the business logic level'

After 6 months I'm not quite sure how to get 11 resolved. I'm also at this stage open to merging if the people on this thread together agree that I should merge it by commenting to that effect (which also implicitly means you are co-mergers on it & are morally obliged to jump in to help with any regressions).

eileenmcnaughton · 2019-06-28T02:38:07Z

I might be jumping the gun with @jackrabbithanna's comment only being a few days ago - I'm just thinking about the rc being cut next week and a desire to hit that release

jaapjansma · 2019-06-28T13:28:18Z

Regarding 11, I would say please merge. However I have not and will not do any full review of the code. However there is an reason to merge this without a full review and loosing quality. The reason is that we favor as a community any contribution someone does and because we favor it we applaud and merge it (although there is a quality risk but we take that as a community).

JoeMurray · 2019-06-28T14:21:20Z

This is big and risky because of area. It adds tests and has been tested in production and focussed manual testing. I support merging and commit that JMA will assist with dealing with any regressions that result.

eileenmcnaughton · 2019-06-28T21:41:36Z

Thanks for weighing in. I'll leave a few days for more input.

Re " there is a quality risk but we take that as a community" - it's probably fairer to say there is a regression risk since the suite of tests in this improve our quality and robustness - which is a big plus on the 'merge' side to me. The issue is not our code quality standard but our review quality standards are at risk of not being hit here

eileenmcnaughton · 2019-07-01T22:07:11Z

@mfb do you want to weigh in on what code review you have done / your in-production experience to give us more comfort with this change?

Also anyone else want to weigh in on merging this - we have 2 * yay

jackrabbithanna · 2019-07-01T23:33:37Z

We haven't had a chance to test this one. My two cents: I do feel like this is a bug that needs killing. I like that this test was added: testMultipleMembershipEndDateMatch() ... how we experienced the bug was that one membership end date change would work for the sending of the reminder, but two, the second wouldn't .. and if this test can catch that .. that's great. I kind of feel like even if there are a couple of new bugs/regressions that come out of this, it's a step in the right direction, and kill those bugs (with more tests!) if they pop up. So add a yay.

mfb · 2019-07-10T19:37:49Z

EFF folks reviewed this PR - but we don't have subject matter expertise re: CiviMember, that's why we hired Giant Rabbit to develop it :)

eileenmcnaughton · 2019-07-10T19:45:34Z

@mfb what level of review did you do? ie did you understand the changes in the code?

mfb · 2019-07-10T20:07:54Z

I read thru the code and have a general understanding of the concept of the changes being made, but our main contribution here is just funding the development and running it in production.

eileenmcnaughton · 2019-07-11T23:44:33Z

@kcristiano @agh1 @Stoob @totten @MegaphoneJon and I discussed this on a hangout & agreed to merge this based on a verbal review - note after merge . upgrade will need fixing

Fix upgrade version for #13487

civibot bot added the master label Jan 20, 2019

Dawnthorn force-pushed the 17880-after-first-reminder-for-membership-fix branch from 21b87b3 to bf87fff Compare January 20, 2019 06:30

Dawnthorn force-pushed the 17880-after-first-reminder-for-membership-fix branch 2 times, most recently from bf378f3 to 8df0128 Compare January 20, 2019 08:32

Dawnthorn force-pushed the 17880-after-first-reminder-for-membership-fix branch 3 times, most recently from 0cc086f to fb96edd Compare January 20, 2019 22:46

seamuslee001 reviewed Jan 26, 2019

View reviewed changes

Dawnthorn force-pushed the 17880-after-first-reminder-for-membership-fix branch 3 times, most recently from 064b1c2 to 4d1388a Compare January 27, 2019 21:44

eileenmcnaughton added the has-test label Feb 4, 2019

eileenmcnaughton reviewed Mar 28, 2019

View reviewed changes

JoeMurray reviewed Jun 25, 2019

View reviewed changes

eileenmcnaughton merged commit 110cb51 into civicrm:master Jul 11, 2019

eileenmcnaughton mentioned this pull request Jul 12, 2019

Fix upgrade for membership second reminder #14810

Merged

mfb added a commit to mfb/civicrm-core that referenced this pull request Aug 13, 2019

Fix upgrade version for civicrm#13487

773b5a1

mfb mentioned this pull request Aug 13, 2019

Fix upgrade version for #13487 #15031

Merged

eileenmcnaughton added a commit that referenced this pull request Aug 13, 2019

Merge pull request #15031 from mfb/upgrade-fix

25958ba

Fix upgrade version for #13487

magnolia61 pushed a commit to magnolia61/civicrm-core that referenced this pull request Oct 13, 2019

Fix upgrade version for civicrm#13487

04cf354

		@@ -1 +1,2 @@
		{* file to handle db changes in 5.11.alpha1 during upgrade *}
		ALTER TABLE civicrm_action_log CHANGE COLUMN reference_date reference_date datetime;

		@@ -1 +1,2 @@
		{* file to handle db changes in 5.14.alpha1 during upgrade *}
		ALTER TABLE civicrm_action_log CHANGE COLUMN reference_date reference_date datetime;

(dev/core#285) Fixed second membership reminder #13487

(dev/core#285) Fixed second membership reminder #13487

Conversation

Dawnthorn commented Jan 20, 2019 • edited by eileenmcnaughton Loading

Overview

Before

After

Technical Details

civibot bot commented Jan 20, 2019

magnolia61 commented Jan 20, 2019

Dawnthorn commented Jan 20, 2019

colemanw commented Jan 23, 2019

seamuslee001 Jan 26, 2019

Choose a reason for hiding this comment

Dawnthorn Jan 27, 2019

Choose a reason for hiding this comment

Dawnthorn commented Jan 28, 2019

mfb commented Mar 22, 2019

eileenmcnaughton Mar 28, 2019

Choose a reason for hiding this comment

Dawnthorn Apr 20, 2019

Choose a reason for hiding this comment

Dawnthorn Apr 20, 2019

Choose a reason for hiding this comment

eileenmcnaughton commented Mar 28, 2019

eileenmcnaughton commented Mar 28, 2019

jaapjansma commented Mar 29, 2019

agh1 commented Mar 29, 2019

jaapjansma commented Mar 29, 2019

nganivet commented Mar 29, 2019

eileenmcnaughton commented Mar 29, 2019

yashodha commented Apr 9, 2019

mfb commented Apr 12, 2019

eileenmcnaughton commented Jun 24, 2019

jackrabbithanna commented Jun 24, 2019 • edited Loading

eileenmcnaughton commented Jun 24, 2019

jackrabbithanna commented Jun 24, 2019

jackrabbithanna commented Jun 24, 2019

totten commented Jun 25, 2019

eileenmcnaughton commented Jun 25, 2019

eileenmcnaughton commented Jun 25, 2019

eileenmcnaughton commented Jun 25, 2019

nganivet commented Jun 25, 2019

JoeMurray Jun 25, 2019

Choose a reason for hiding this comment

eileenmcnaughton Jun 25, 2019

Choose a reason for hiding this comment

eileenmcnaughton commented Jun 28, 2019

eileenmcnaughton commented Jun 28, 2019

jaapjansma commented Jun 28, 2019

JoeMurray commented Jun 28, 2019

eileenmcnaughton commented Jun 28, 2019 • edited Loading

eileenmcnaughton commented Jul 1, 2019

jackrabbithanna commented Jul 1, 2019

mfb commented Jul 10, 2019

eileenmcnaughton commented Jul 10, 2019

mfb commented Jul 10, 2019

eileenmcnaughton commented Jul 11, 2019

Dawnthorn commented Jan 20, 2019 •

edited by eileenmcnaughton

Loading

jackrabbithanna commented Jun 24, 2019 •

edited

Loading

eileenmcnaughton commented Jun 28, 2019 •

edited

Loading