Allow action trigger events to be repeated/reused

[Remington Steed]

Currently, there is no way to repeat a notification (action/trigger) when an event occurs more than once for the same target. This only matters for targets that are persistent (such as a library card) combined with events that can reoccur (such as expiration, if the expiration can be removed).

USE CASE
When a patron's library card expires, we send a notification email. They can have the expiration date extended on that card. We then need a way to send subsequent notification emails when that same card expires again.

[Remington Steed (rjs7)]

Here's my branch:
working/user/rsteed/lp1207902_trigger_event_repeatability_delay

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/rsteed/lp1207902_trigger_event_repeatability_delay

To test this new feature:
- apply the changes
- log in to staff client: Admin > Local Admin > Notifications / Action Triggers
- new "Event Repeatability Delay" field should display in grid view and edit view
- edit "3 Day Courtesy Notice"
- - add Repeatability Delay "1 minute"
- - add Granularity "Daily"
- - make sure Enabled is set to "True"
- check out an item to yourself, set the due date between 2 and 3 days in the future
- run: action_trigger_runner.pl --process-hooks --run-pending --granularity Daily
- check db table action_trigger.event for new rows [FIRST EVENT IS CREATED]
- BEFORE 1 MINUTE, run: action_trigger_runner.pl --process-hooks --run-pending --granularity Daily
- check db table action_trigger.event for new rows [NOTHING NEW]
- AFTER 1 MINUTE, run: action_trigger_runner.pl --process-hooks --run-pending --granularity Daily
- check db table action_trigger.event for new rows [REPEAT EVENT IS CREATED]

[Mike Rylander (mrylander)]

I really like this, and I want it to go in ASAP ... however, I have one change request (which I can do at commit time, really, but I want to open the request for comment and provide some knowledge sharing) and one question that might lead to a change:

 *) when adding fields to the fieldmapper XML, it's best to place non-virtual fields before all virtual fields, except in the case where it's reasonable to believe there might be objects of the type in question frozen as JSON outside the database -- say, bre objects. In the case of atevdef, that's not something we should worry about.

 *) should we use run_time instead of start_time when deciding on when to allow repeat events? That would be more stable and predictable, ISTM.

Thanks, Remington ... this looks great, and fits really well with all the existing (complicated) logic.

[Remington Steed (rjs7)]

Mike, per your request, I have moved the new field above the virtual fields in the fieldmapper and force pushed the change to the same branch. Thanks for the knowledge sharing.

Regarding run_time vs. start_time, it seems like run_time is when an event is intended to run, and start_time is when it actually starts running. In practice, these should normally be close together.

If my understanding is correct, there are two reasons I would still vote for using start_time. The first is the case where
some active events are run when they are created, so their run_time is set to an arbitrary time in the future and gets ignored by the code that runs those events. In those cases, start_time is the most meaningful value. The second reason is that run_time is set when the event is still pending, so you could potentially (and accidentally) create repeat events before the previous event has been started, so they would get run at the same time instead of having the intended delay. If, however, we use start_time, then the repeat event won't even be created until after the previous one has started (plus the delay time), which guarantees the desired delay behavior.

If my understanding is off, please correct me. I'm glad to hear why you think run_time is the better choice.

And to be clear, the logic of this feature was mostly the work of Dan Wells.

[Mike Rylander (mrylander)]

Remington,

I don't recall the first of your reasons being true, but I'm not looking at the code ATM. Your second is a solid reason on its own, though, so I'll leave start_time as the deciding time. Thanks!

[Mike Rylander (mrylander)]

And, it's in!