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

dev/membership#2 - Add membership start date and registration start/end date to schedule reminder #12114

Merged
merged 3 commits into from Jun 4, 2018

Conversation

Projects
None yet
6 participants
@jitendrapurohit
Copy link
Contributor

commented May 11, 2018

Overview

Add membership start date and event registration start/end date to schedule reminder.

Before

No membership start date OR event registration start/end date available on the form

Membership -
image

Event -
image

After

Available and working.

Membership -
image

Event -
image


Gitlab Issue - https://lab.civicrm.org/dev/membership/issues/2

@monishdeb

This comment has been minimized.

Copy link
Member

commented May 11, 2018

Jenkins test this please

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

@michaelmcandrew is this one you could review?

@mattwire

This comment has been minimized.

Copy link
Contributor

commented May 17, 2018

@jitendrapurohit Could you update the PR subject per https://docs.civicrm.org/dev/en/latest/tools/git/#pr-subject:
"dev/membership#2 - Add membership start date and registration start/end date to schedule reminder"

@jitendrapurohit jitendrapurohit changed the title membership/issues/2 - Add membership start date and registration star… dev/membership#2 - Add membership start date and registration stardev/membership#2 - Add membership start date and registration start/end date to schedule reminder May 18, 2018

@jitendrapurohit jitendrapurohit changed the title dev/membership#2 - Add membership start date and registration stardev/membership#2 - Add membership start date and registration start/end date to schedule reminder dev/membership#2 - Add membership start date and registration start/end date to schedule reminder May 18, 2018

@michaelmcandrew

This comment has been minimized.

Copy link
Contributor

commented May 21, 2018

@jitendrapurohit I did some manual testing of this functionality. It is fairly hard to test this stuff but I feel like some things are not working correctly.

I created two conferences:

cv api event.create title="reg closes may 1 2018" event_type_id=Conference start_date=2018-06-06T0900 registration_end_date=2018-05-01T0000
cv api event.create title="reg closes may 1 2019" event_type_id=Conference start_date=2019-06-06T0900 registration_end_date=2019-05-01T0000

I then registered the same contact at each with status registered

I then set a scheduled reminder as shown in the following screen shot:
scheduled-reminder

but running cv api job.send_reminder did not cause any scheduled reminders to be sent.

I would have expected the contact to receive one scheduled reminder.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented May 21, 2018

I think we would need a test on it

@michaelmcandrew

This comment has been minimized.

Copy link
Contributor

commented May 21, 2018

@eileenmcnaughton - scheduled reminders itself seems quite under tested. From what I can see, there is a single web test for the admin form and nothing testing that the reminders are behaving as they should.

I'm guessing this is at least in part because there is a lot of set up: creating events and participants (or memberships and members), running cron jobs, etc. and that the output (email) is a bit hard to test.

Also, the scheduled reminders api is also a little under developed (read non existent).

Not sure if there have been attempts in the past to improve testing in this area of CiviCRM. I'm guessing some of the CiviMail testing infra could be re purposed here.

🤔

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor

commented May 21, 2018

@michaelmcandrew I think @totten HAS created some tests - so they might just be somewhere unexpected...

@jitendrapurohit jitendrapurohit force-pushed the jitendrapurohit:membership-2 branch from 9286b64 to 7b78c1c May 22, 2018

@jitendrapurohit

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2018

test this please

@jitendrapurohit jitendrapurohit force-pushed the jitendrapurohit:membership-2 branch from 7b78c1c to 76ea51c May 22, 2018

@jitendrapurohit

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2018

@michaelmcandrew seems the event in your testing has registration_end_date set as 1 may 2019 and the reminder is set to execute 6 months before the date which might not be true unless the date is - 1 may minus 6 months.

I've written a unit test which should explain the test procedure. My initial testing was something like -

  • Create event with registration end date set to tomorrow.
  • Create sched reminder with offset = 1 day before registration end date.
  • Register a participant to the event.
  • Execute SR job and verify if the mail was sent.
@michaelmcandrew

This comment has been minimized.

Copy link
Contributor

commented May 22, 2018

seems the event in your testing has registration_end_date set as 1 may 2019 and the reminder is set to execute 6 months before the date which might not be true unless the date is - 1 may minus 6 months.

@jitendrapurohit - I created two events.

cv api event.create title="reg closes may 1 2018" event_type_id=Conference start_date=2018-06-06T0900 registration_end_date=2018-05-01T0000

^ for this one, I thought the email would be sent because the we are less than 6 months before the registration date but no email was sent

cv api event.create title="reg closes may 1 2019" event_type_id=Conference start_date=2019-06-06T0900 registration_end_date=2019-05-01T0000

^ For this one, I wanted to ensure that no email was sent.

@michaelmcandrew

This comment has been minimized.

Copy link
Contributor

commented May 22, 2018

Also, the scheduled reminders api is also a little under developed (read non existent).

Ah, so the scheduled reminder api is actually called ActionSchedule. Sorry - I didn't realise that.

OK so it does have an API and it does have some tests.

And now it has another one :)

I did a bit more manual testing and came to a realisation.

First I created a scheduled reminder as follows

cv api ActionSchedule.create \
  title="Email 2 weeks before conferences"\
  mapping_id=2\
  entity_value=1\
  entity_status=1\
  start_action_date=registration_end_date\
  start_action_offset=6\
  start_action_unit=month\
  start_action_condition=before\
  from_email=admin@example.com\
  body_html=Testing\
  subject=Testing

I then created an event and participant as follows

cv api event.create title="reg closes may 1 2018" event_type_id=Conference start_date=2018-06-06T0900 registration_end_date=2018-05-01T0000 # registration closes before today
cv api participant.create contact_id=104 status_id=registered event_id=7

I did not get a reminder.

I then created an event and participant as follows

cv api event.create title="reg closes may 1 2018" event_type_id=Conference start_date=2018-06-06T0900 registration_end_date=2018-05-30T0000 # registration closes after today
cv api participant.create contact_id=104 status_id=registered event_id=7

I did get a reminder.

So it seems that a reminder is only sent if the start_action_date is in the future. Which means it won't send emails that were meant to be sent before a start_action_date after the event. Which makes sense.

A test that created an event 2 weeks in the future, and a scheduled reminder to be sent 1 week before the event and ensured that it wasn't sent 'today' would be nice to have but is not necessary.

Unless you wanted to add that, I think it is good to merge.

@jitendrapurohit

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2018

Thanks for the review @michaelmcandrew. Seems like the review is positive with the below test requirement?

A test that created an event 2 weeks in the future, and a scheduled reminder to be sent 1 week before the event and ensured that it wasn't sent 'today' would be nice to have but is not necessary.

I've added this to the PR now. @eileenmcnaughton @michaelmcandrew Can we have this merged into core?

@monishdeb monishdeb added the has-test label Jun 4, 2018

@monishdeb

This comment has been minimized.

Copy link
Member

commented Jun 4, 2018

Great work @jitendrapurohit and thank you all for your feedback. I have tested the PR and the schedule reminder is behaving correctly on those 3 new dates added for membership and event + UT.

Hence marking with merge-on-pass

@monishdeb monishdeb merged commit 6a8366e into civicrm:master Jun 4, 2018

1 check passed

default Build finished.
Details
@michaelmcandrew

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2018

looks great @jitendrapurohit :)

@jitendrapurohit jitendrapurohit deleted the jitendrapurohit:membership-2 branch Aug 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.