[Bug 13106] New: Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Bug ID: 13106 Summary: Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: olli-antti.kivilahti@jns.fi QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=11630 Assignee|koha-bugs@lists.koha-commun |olli-antti.kivilahti@jns.fi |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 --- Comment #1 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 32507 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32507&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |13113 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32507|0 |1 is obsolete| | --- Comment #2 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 32538 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32538&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch --- Comment #3 from Marc Véron <veron@veron.ch> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32538|0 |1 is obsolete| | --- Comment #4 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 32548 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32548&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 --- Comment #5 from Marc Véron <veron@veron.ch> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32548|0 |1 is obsolete| | --- Comment #6 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 32625 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32625&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32625|0 |1 is obsolete| | --- Comment #7 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 32660 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32660&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32660|0 |1 is obsolete| | --- Comment #8 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 32679 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32679&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 --- Comment #9 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 --- Comment #10 from Marc Véron <veron@veron.ch> --- A PDF with screenshots regarding usage and testing can be found here: http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=12547 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 --- Comment #11 from Marc Véron <veron@veron.ch> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32679|0 |1 is obsolete| | --- Comment #12 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Created attachment 32883 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32883&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 --- Comment #13 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- (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! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32883|0 |1 is obsolete| | --- Comment #14 from Marc Véron <veron@veron.ch> --- Created attachment 32887 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=32887&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 --- Comment #15 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- Thank you Marc for testing this one! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #32887|0 |1 is obsolete| | CC| |kyle@bywatersolutions.com --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 33069 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33069&action=edit 32887: Bug 13106 - Encapsulate Circulation::GetAgeRestriction() and modify it to check borrowers age as well. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33069|0 |1 is patch| | Attachment #33069|application/mbox |text/plain mime type| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patch pushed to master. Thanks Olli! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13106 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Summary|Encapsulate |Encapsulate |Circulation::GetAgeRestrict |Circulation::GetAgeRestrict |ion() and modify it to |ion() and modify it to |check borrowers age as |check borrowers age as well |well. | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org