Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well
Summary: Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 13113
  Show dependency treegraph
 
Reported: 2014-10-17 14:28 UTC by Olli-Antti Kivilahti
Modified: 2015-06-04 23:33 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. (6.00 KB, patch)
2014-10-17 14:29 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. (6.23 KB, patch)
2014-10-20 14:51 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. (6.23 KB, patch)
2014-10-21 11:47 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. (6.23 KB, patch)
2014-10-23 14:45 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. (12.68 KB, patch)
2014-10-24 09:38 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. (12.90 KB, patch)
2014-10-24 15:00 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. (12.99 KB, patch)
2014-10-29 08:12 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. (13.46 KB, patch)
2014-10-29 10:33 UTC, Marc Véron
Details | Diff | Splinter Review
32887: Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. (13.53 KB, patch)
2014-10-31 13:40 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!