wishlist: Add Auto Renewal functionality

Bug #1779920 reported by Andrea Neiman
36
This bug affects 11 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
Wishlist
Unassigned

Bug Description

North Texas Library Consortium has contracted with Equinox to create a new feature for Evergreen to allow for automatic renewals of items out.

Auto renewals will be configurable by patron criteria, organizational unit, allowable number of auto renewals, and auto renewal notifications. This will be available via the existing Circulation Duration Rules interface and operates as part of the existing Action Trigger system.

Marking bug 1636923 as a duplicate since we are taking a different approach than what is described there.

A public branch will be forthcoming after partner testing is complete.

Full functional specifications are available here: https://yeti.esilibrary.com/dev/public/techspecs/auto-renewal.pdf

Revision history for this message
Terran McCanna (tmccanna) wrote :

+1

Revision history for this message
Jane Sandberg (sandbergja) wrote :

We're very excited for this!

Revision history for this message
Chelsea Jordan-Makely (cjordan-makely) wrote :

Hi there,
I'm at the Whistler Public Library here in British Columbia. We're interested in implementing this feature also! Please get in touch if there are opportunities to guinea pig or to learn more: <email address hidden>. Thanks!
Chelsea

Revision history for this message
Cesar V (cesardv) wrote :

As promised, here's a branch that implements autorenewals:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/cesardv/lp1779920-autorenewal-feature-squashed

Chelsea, I'd love to have have some test guinea pigs... err I mean community testers... thanks for volunteering ;)
Let me know if you have any questions on how to get it working.

tags: added: pullrequest
Revision history for this message
Andrea Neiman (aneiman) wrote :

Draft documentation is attached to assist with testing. This will be converted to asciidoc & submitted to the community documentation.

Changed in evergreen:
milestone: none → 3.2-beta
Revision history for this message
Bill Erickson (berick) wrote :

Thanks, Cesar. I made a pass through the code and have a few minor comments:

1. A number of INFO-level debug logs added to Trigger.pm should be removed or set to ->debug (or -internal). Ditto a new log in Validator.pm. At INFO, the Trigger log would be especially noisy.

2. In AutoRenew.pm a session connect call for open-ils.trigger is made without a corresponding disconnect. It looks like the purpose is to fire-and-forget the trigger autocreate call, so connecting serves no real purpose here. AppSession->create(...) instead of ->connect ensures the API calls will be spread among the existing trigger back-ends. Or just ->disconnect to cleanup. Or see AppUtils comment below.

3. In CircIsAutoRenewable, the trigger autocreate call is wrapped in a connect/disconnect which serves no purpose (but adds overhead) for single-call API requests.

Also note there's an AppUtils function for creating events: create_events_for_hook(...) -- with a fire-and-forget option.

4. Minor nit:

my $userObj = $AppUtils->simplereq('open-ils.cstore', 'open-ils.cstore.direct.actor.user.retrieve', $userid);

vs.

# Has better logging, easier to read.
my $userObj = new_editor()->retrieve_actor_user($userid);

Revision history for this message
Mike Rylander (mrylander) wrote :

Hey, Bill, thanks for reviewing! I have a comment for one of your comments. Re (2), the connect is intentional to avoid spamming the trigger app (imagine 50 circs autorenewing), but you're right that it needs a disconnect.

(Side note: maybe we should use https://perldoc.perl.org/Scalar/Util.html#weaken on the session pointer stored in the session cache so that the OpenSRF::AppSession::DESTROY method could be used to automatically disconnect abandoned sessions? That would help with other things like "pinned cstore" cases...)

Revision history for this message
Bill Erickson (berick) wrote :

Gotcha Mike, Thanks. Disconnect will fix it right up, then.

I have no practical experience with 'weaken', but I could imagine that being helpful.

Revision history for this message
Cesar V (cesardv) wrote :

Thanks for reviewing this Bill, I appreciate it!

I'll go thru and take care of any un-necessary info log calls and the other suggestions.

Revision history for this message
Cesar V (cesardv) wrote :
Bill Erickson (berick)
Changed in evergreen:
assignee: nobody → Bill Erickson (berick)
Revision history for this message
Bill Erickson (berick) wrote :

Thanks Cesar, testing the code now, but as a heads up we'll need release notes as well.

Changed in evergreen:
status: New → Confirmed
Revision history for this message
Bill Erickson (berick) wrote :

New branch pushed with sign-off to Caser's branch, plus an additional bug fix commit:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1779920-autorenew-additions

From the bug fix commit:

* Remove hard-coded granularity for auto-renewal notification event generation
* Moved schema update changes from 950 seed file into schema files.
* Added CircIsOpen validator to auto-renewal reactor on the off chance a circ is renewed by the patron in the middle of event processing.
* Notification template formatting and language updates, and removed debugging content.

So in addition to the release notes, we'll need a sign off on my additional commit.

tags: added: needsreleasenote
Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
Revision history for this message
Cesar V (cesardv) wrote :

Sounds good Bill, thanks! Right, so the release notes I figured I'd just cherry-pick in at the end (from that first branch) once the main main commit/feature is signed off. Not sure exactly what's the protocol for the release notes...

Revision history for this message
Bill Erickson (berick) wrote :

Cesar, a new commit for the release notes fine. I suggest creating a new branch based on my latest branch and adding your release notes commit to that. Testing and signing off on my commit would be a bonus, of course.

Revision history for this message
Cesar V (cesardv) wrote :

Cool, thanks Bill, I'll create a new signed off branch then.

Bill, I was wondering why the change to remove the granularity from the AutorenewNotify events?
We added in order to allow libraries to have better timing control of when their renewal emails were processed and sent out.

Revision history for this message
Bill Erickson (berick) wrote :

Thanks, Cesar.

Applying the granularity at event creation time only affects event creation. (As-was, the code failed to produce any notification events, because the A/T event def in the DB had no granularity applied). To control the email delivery schedule, the granularity filter should be a) applied to the A/T event definition in the DB and b) passed by action_trigger_runner.pl at event processing time. Typically, we leave this step up to the user so they can use whatever granularities they define locally, so it's OK the DB seed data has no granularity.

Revision history for this message
Cesar V (cesardv) wrote :

Ah, that makes sense... and true yeah libraries can always add any granularity to suit their needs.
Merci beaucoup monsieur!

Revision history for this message
Cesar V (cesardv) wrote :
Bill Erickson (berick)
Changed in evergreen:
assignee: nobody → Bill Erickson (berick)
Revision history for this message
Bill Erickson (berick) wrote :

Sign off to Cesar's release notes plus another commit pushed to:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1779920-autorenew-signoffs3-plus-updates

The extra commit adds the new columns to various circ-related tables:

    * action.all_circulation
    * action.all_circulation_slim
    * action.aged_circulation
    * action.all_circ_chain
    * action.summarize_all_circ_chain

(Noticed when installing the base schema from scratch).

Review and sign-off for my last commit required.

There's also a DB version stamp patch which I left in the branch in case we can use it. If the version number is already taken at merge time, I'll re-stamp.

Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
Galen Charlton (gmc)
Changed in evergreen:
assignee: nobody → Galen Charlton (gmc)
Revision history for this message
Galen Charlton (gmc) wrote :

Pushed to master for inclusion in 3.2-beta, along with a tweak of the release notes. Thanks, Cesar and Bill!

Changed in evergreen:
status: Confirmed → Fix Committed
assignee: Galen Charlton (gmc) → nobody
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.