Bug 19444 - Automatic renewal script should not auto-renew if a patron's record has expired
Summary: Automatic renewal script should not auto-renew if a patron's record has expired
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Julian Maurice
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-11 09:12 UTC by Janet McGowan
Modified: 2018-06-04 20:15 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19444: Do not auto renew if patron is expired and BlockExpiredPatronOpacActions is set (7.82 KB, patch)
2017-10-26 16:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19444: Display error message for auto_account_expired (5.49 KB, patch)
2017-12-05 13:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19444: Do not auto renew if patron is expired and BlockExpiredPatronOpacActions is set (7.23 KB, patch)
2017-12-05 13:49 UTC, Claire Gravely
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 19444: Display error message for auto_account_expired (5.59 KB, patch)
2017-12-05 13:49 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 19444: Do not auto renew if patron is expired and BlockExpiredPatronOpacActions is set (7.22 KB, patch)
2017-12-15 09:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19444: Display error message for auto_account_expired (5.60 KB, patch)
2017-12-15 09:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19444: (QA follow-up) Fix tests for UNIMARC (5.89 KB, patch)
2017-12-15 09:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 19444: (QA follow-up) Remove repeating condition (1.17 KB, patch)
2017-12-15 09:11 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Janet McGowan 2017-10-11 09:12:14 UTC
Currently with the automatic_renewals.pl script, materials are still auto-renewed even if the patron's account has expired.
The automatic renewals script has error conditions which prevent automatic renewals  in these circumstances:
	$error eq 'too_many'
        or $error eq 'on_reserve'
        or $error eq 'restriction'
        or $error eq 'overdue'
        or $error eq 'auto_too_late'
        or $error eq 'auto_too_much_oweing'
        or $error eq 'auto_too_soon

The error conditions should take into account the expiry date of borrower and not auto renew if the borrower's account has expired.
For example adding a new error code such as 'borrower_expired' - or whatever convention achieves this.
Comment 1 Jonathan Druart 2017-10-11 17:04:36 UTC
Do we need a new syspref here or must it be the default behaviour?

Or based on BlockExpiredPatronOpacActions?
Comment 2 Jonathan Field 2017-10-18 09:12:59 UTC
We believe it should be the default behaviour rather than requiring some sort of system preference. If the user has expired, they shouldn't be making any new transactions.
Comment 3 Katrin Fischer 2017-10-18 09:17:19 UTC
Hi Jonathan, there is has been a lot of dicussion about this and libraries have different ideas about this, for some a hold that can be placed is an incentive for the user to come back to the library and renew their membership etc.
Have you checked the BlockExpiredPatronOpacActions system prefernece?

[Block|don't block] expired patrons from OPAC actions such as placing a hold or renewing. Note that the setting for a patron category takes priority over this system preference.  

I think it would make sense to treat auto-renewals according to this setting.
Comment 4 Jonathan Field 2017-10-18 10:56:55 UTC
Hi Katrin, very happy to do that if there is a use case for other libraries. We just need to make sure there is a mechanism in place for expired users and auto-renewals. happy to go with your suggestion though!
Comment 5 Jonathan Druart 2017-10-23 18:00:25 UTC
It is not clear to me if we want another pref "ExpiredBlockRenewing" (like OverduesBlockRenewing and RestrictionBlockRenewing) to block renewals globally or if we want to fix this behaviour only for auto renewals.
Comment 6 Katrin Fischer 2017-10-25 19:19:15 UTC
It's a good question. I think as ExpiredPatronBlockOpacActions already covers normal renewals it makes sense to include AutoRenewals there. If we want to have both separate, I'd make the new pref specific, so that it doesn't interfere with the existing pref: ExpiredBlockAutoRenewing.
Comment 7 Jonathan Field 2017-10-26 07:22:13 UTC
I agree with Katrin, would be very happy with that.
Comment 8 Jonathan Druart 2017-10-26 16:20:04 UTC
Created attachment 68633 [details] [review]
Bug 19444: Do not auto renew if patron is expired and BlockExpiredPatronOpacActions is set

If the patron's account has expired and BlockExpiredPatronOpacActions is set,
we expect auto renewal to be rejected.

Test plan:
Use the automatic_renewals.pl cronjob script to auto renew a checkout

Before this patch, if the patron's account has expired the auto renew was done.
With this patch, it will only be auto renewed if BlockExpiredPatronOpacActions is not set.
Comment 9 M. Tompsett 2017-10-31 02:38:00 UTC
Comment on attachment 68633 [details] [review]
Bug 19444: Do not auto renew if patron is expired and BlockExpiredPatronOpacActions is set

Review of attachment 68633 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/updatedatabase.pl
@@ +14854,4 @@
>      });
>  
>      SetVersion( $DBversion );
> +    print "Upgrade to $DBversion done (Bug 12768 - Add 'Processing Fee' to the account_offset_types table if missing)\n";

This isn't related to this bug.
Comment 10 Dominic Pichette 2017-11-03 19:45:25 UTC Comment hidden (obsolete)
Comment 11 Dominic Pichette 2017-11-03 19:56:17 UTC
I tried it, I started by setting BlockExpiredPatronOpacActions to set, checked out an item for a particular patron, change the expiration date of that patron so he is now expired and then executed automatic_renewals.pl. The patch didn't seem to make a difference, it passed in both cases..
Comment 12 Jonathan Druart 2017-11-06 14:34:31 UTC
(In reply to Dominic Pichette from comment #11)
> I tried it, I started by setting BlockExpiredPatronOpacActions to set,
> checked out an item for a particular patron, change the expiration date of
> that patron so he is now expired and then executed automatic_renewals.pl.
> The patch didn't seem to make a difference, it passed in both cases..

Dominic, you need to know how automatic renewals work if you want to test this patch. You should enable it in the issuing rule and then tick the "Automatic renewal" on the checkout screen. Is it what you tried?
Comment 13 Claire Gravely 2017-12-05 10:08:50 UTC
Tested patch, seems to work as expected :)

One comment: when I turned the syspref on to "Block" the renew column in the list of checkouts (circulation.pl) it shows 

auto_account_expired 
(5 of 5 renewals remaining)

do we want to show that wording? Would just 'Account expired' be more user friendly?

Otherwise happy to sign off.
Comment 14 Jonathan Druart 2017-12-05 13:24:05 UTC
Created attachment 69502 [details] [review]
Bug 19444: Display error message for auto_account_expired
Comment 15 Jonathan Druart 2017-12-05 13:25:38 UTC
(In reply to Claire Gravely from comment #13)
> do we want to show that wording? Would just 'Account expired' be more user
> friendly?

Thanks for testing Claire!
Yes you are right, the last patch display a human friendly message for this error code.
Comment 16 Claire Gravely 2017-12-05 13:49:50 UTC
Created attachment 69503 [details] [review]
[SIGNED-OFF] Bug 19444: Do not auto renew if patron is expired and BlockExpiredPatronOpacActions is set

If the patron's account has expired and BlockExpiredPatronOpacActions is set,
we expect auto renewal to be rejected.

Test plan:
Use the automatic_renewals.pl cronjob script to auto renew a checkout

Before this patch, if the patron's account has expired the auto renew was done.
With this patch, it will only be auto renewed if BlockExpiredPatronOpacActions is not set.

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 17 Claire Gravely 2017-12-05 13:49:53 UTC
Created attachment 69504 [details] [review]
[SIGNED-OFF] Bug 19444: Display error message for auto_account_expired

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 18 Julian Maurice 2017-12-15 09:10:48 UTC
Created attachment 69810 [details] [review]
Bug 19444: Do not auto renew if patron is expired and BlockExpiredPatronOpacActions is set

If the patron's account has expired and BlockExpiredPatronOpacActions is set,
we expect auto renewal to be rejected.

Test plan:
Use the automatic_renewals.pl cronjob script to auto renew a checkout

Before this patch, if the patron's account has expired the auto renew was done.
With this patch, it will only be auto renewed if BlockExpiredPatronOpacActions is not set.

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 19 Julian Maurice 2017-12-15 09:10:52 UTC
Created attachment 69811 [details] [review]
Bug 19444: Display error message for auto_account_expired

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 20 Julian Maurice 2017-12-15 09:10:56 UTC
Created attachment 69812 [details] [review]
Bug 19444: (QA follow-up) Fix tests for UNIMARC

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 21 Julian Maurice 2017-12-15 09:11:01 UTC
Created attachment 69813 [details] [review]
Bug 19444: (QA follow-up) Remove repeating condition

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 22 Jonathan Druart 2017-12-18 15:18:56 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 23 Nick Clemens 2018-01-15 13:18:35 UTC
Awesome work all! Pushed to stable for 17.11.02
Comment 24 Fridolin SOMERS 2018-01-15 15:39:34 UTC
Pushed to 17.05.x for v17.05.08