Bug 13106

Summary: Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well
Product: Koha Reporter: Olli-Antti Kivilahti <olli-antti.kivilahti>
Component: CirculationAssignee: Olli-Antti Kivilahti <olli-antti.kivilahti>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, katrin.fischer, kyle.m.hall, kyle, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11630
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 13113    
Attachments: Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.
32887: Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.

Description Olli-Antti Kivilahti 2014-10-17 14:28:16 UTC
This patch moves the logic of deciding whether or not a borrower is old enough to access this material to its own function GetAgeRestriction.

This makes it easier to use AgeRestriction elsewhere, like with placing holds.

Unit tests included.
Comment 1 Olli-Antti Kivilahti 2014-10-17 14:29:50 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2014-10-20 14:51:11 UTC Comment hidden (obsolete)
Comment 3 Marc Véron 2014-10-20 15:00:16 UTC
Hi Olli,

Great idea to encapsulate the age restriction.

I did not yet test, but I have the following remark:

In Circulation.pm, line 988 reads:
if ( $daysToAgeRestriction ) 
(...)

Negative days mean the borrower has gone past the age restriction age. But a negative value would evaluate to true and trigger the age restriction.
(see: http://www.foo.be/docs/tpj/issues/vol4_2/tpj0402-0002.html , rule 7)


I think the line should be something like 
if ( $daysToAgeRestriction > 0 ) 
or
if ( $daysToAgeRestriction >= 0 ) 

Oh, it seems that $daysToAgeRestriction is not always returned as a defined value in sub GetAgeRestriction. (See Line 3807). Then maybe the comparision should be something like:

if ( $daysToAgeRestriction && $daysToAgeRestriction > 0 ) 
or
if ( $daysToAgeRestriction && $daysToAgeRestriction >= 0 ) 

Marc
Comment 4 Olli-Antti Kivilahti 2014-10-21 11:47:42 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2014-10-21 12:11:19 UTC
Hi Olli-Antti,

It seems that the line did not change in the newest patch, it still reads
if ( $daysToAgeRestriction )  (...)

The patch seems to be unchanged.

Something similar happend to me several times when I forgot to do
git add [file]  :-)

Marc
Comment 6 Olli-Antti Kivilahti 2014-10-23 14:45:53 UTC Comment hidden (obsolete)
Comment 7 Olli-Antti Kivilahti 2014-10-24 09:38:14 UTC Comment hidden (obsolete)
Comment 8 Olli-Antti Kivilahti 2014-10-24 15:00:09 UTC Comment hidden (obsolete)
Comment 9 Olli-Antti Kivilahti 2014-10-24 15:02:33 UTC
Thank you Mr. Véron for taking a look at this patch!
I fixed the issue you encountered.

Just pushed this feature set to our production yesterday, getting solid feedback :)

I didn't get your comment until now, so I must have been fixing some other issues.
Comment 10 Marc Véron 2014-10-28 19:05:59 UTC
A PDF with screenshots regarding usage and testing can be found here:

http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=12547
Comment 11 Marc Véron 2014-10-28 19:47:39 UTC
Wanted to test. Patch did not apply. Fixed conflict in /t/db_dependent/Members.t

The age restriction works as before, no regression found.

However /t/db_dependent/Members.t exits on line 21 (compilation aborted)
Maybe I did a mistake by fixing the conflict.

Marc
Comment 12 Olli-Antti Kivilahti 2014-10-29 08:12:07 UTC Comment hidden (obsolete)
Comment 13 Olli-Antti Kivilahti 2014-10-29 08:14:55 UTC
(In reply to Marc Véron from comment #11)
> Wanted to test. Patch did not apply. Fixed conflict in
> /t/db_dependent/Members.t
> 
> The age restriction works as before, no regression found.
> 
Happy to hear that!

> However /t/db_dependent/Members.t exits on line 21 (compilation aborted)
> Maybe I did a mistake by fixing the conflict.
> 
> Marc

I don't know about mistakes, but I rebased this feature for today's master and fixed a small regression issue with my test cases. Thanks for keeping me on my toes Marc!
Comment 14 Marc Véron 2014-10-29 10:33:49 UTC Comment hidden (obsolete)
Comment 15 Olli-Antti Kivilahti 2014-10-29 12:21:55 UTC
Thank you Marc for testing this one!
Comment 16 Kyle M Hall 2014-10-31 13:40:06 UTC
Created attachment 33069 [details] [review]
32887: Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.
Comment 17 Tomás Cohen Arazi 2014-11-11 12:56:38 UTC
Patch pushed to master.

Thanks Olli!