|
Description
Olli-Antti Kivilahti
2014-10-17 14:28:16 UTC
Created attachment 32507 [details] [review] Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. 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. Created attachment 32538 [details] [review] Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. 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. 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 Created attachment 32548 [details] [review] Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. 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. 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 Created attachment 32625 [details] [review] Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. 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. Created attachment 32660 [details] [review] Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. 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. This feature adds a new function C4::Members::SetAge() to make testing ages a lot easier. A ton of Unit tests included. Created attachment 32679 [details] [review] Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. 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. This feature adds a new function C4::Members::SetAge() to make testing ages a lot easier. A ton of Unit tests included. C4::Circulate::CanBookBeIssued() fixed and issue with undefined $daysToAgeRestriction per Marc Véron's suggestion. 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. A PDF with screenshots regarding usage and testing can be found here: http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=12547 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 Created attachment 32883 [details] [review] Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. 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. This feature adds a new function C4::Members::SetAge() to make testing ages a lot easier. A ton of Unit tests included. C4::Circulate::CanBookBeIssued() fixed and issue with undefined $daysToAgeRestriction per Marc Véron's suggestion. (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! Created attachment 32887 [details] [review] Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. 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. This feature adds a new function C4::Members::SetAge() to make testing ages a lot easier. A ton of Unit tests included. C4::Circulate::CanBookBeIssued() fixed and issue with undefined $daysToAgeRestriction per Marc Véron's suggestion. Test plan: (See comment #10 for screenshots about using age restriction) 1) Without patch Configure Age Restricition (see Syspref AgeRestrictionMarker) and have a biblio record with e.g. PEGI 99 in age restriction field Try to check out to a patron with age < 99 Check out should be blocked Change entry in age restriction field to PEGI99 Check out schould now be blocked 2) With patch Try checkouts again, behaviour should be th same. Signed-off-by: Marc Véron <veron@veron.ch> Thank you Marc for testing this one! Created attachment 33069 [details] [review] 32887: Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. Patch pushed to master. Thanks Olli! |