Bug 11630 - AgeRestrictionMarker doesn't handle marker immediately followed by age
Summary: AgeRestrictionMarker doesn't handle marker immediately followed by age
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: paxed
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-29 07:40 UTC by paxed
Modified: 2015-06-04 23:33 UTC (History)
6 users (show)

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


Attachments
Make AgeRestrictionMarker consider "K16" (2.07 KB, patch)
2014-01-29 07:40 UTC, paxed
Details | Diff | Splinter Review
[SIGNED-OFF]Improve AgeRestrictionMarker to consider the case where the marker is immediately followed by the age, eg. "K16" in Finland. (2.62 KB, patch)
2014-04-11 08:47 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 11630 [QA Followup] - Move code to subroutine, add unit tests (6.10 KB, patch)
2014-04-14 12:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
[Signed-off] Bug 11630 [QA Followup] - Move code to subroutine, add unit tests (6.27 KB, patch)
2014-04-16 07:16 UTC, Marc Véron
Details | Diff | Splinter Review
Improve AgeRestrictionMarker to consider the case where the marker is immediately followed by the age, eg. "K16" in Finland. (2.67 KB, patch)
2014-04-16 10:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11630 [QA Followup] - Move code to subroutine, add unit tests (6.32 KB, patch)
2014-04-16 10:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11630: Unit tests can be independent of DB (1.65 KB, patch)
2014-04-16 10:56 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description paxed 2014-01-29 07:40:47 UTC Comment hidden (obsolete)
Comment 1 Marc Véron 2014-04-11 08:47:07 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-04-11 12:25:12 UTC
Would it be too onerous a request to move the code for parsing the age restriction marker to a separate subroutine and add unit tests?
Comment 3 Marc Véron 2014-04-14 10:56:47 UTC
Kyle,

I think it is a good idea to have a subroutine for the age restriction. Until now the code is used only at this place, but later we could expand the age restrictions e.g. to suppress titles in the catalogue if a kid is logged in etc. 

However, this is a small patch that makes an existing feature usable for Finland as well, and I can imagine that they need it immediately.

That's why I kindly ask you to QA paxed's patch.

In a next step we could open a bug to clean up the whole sub CanBookBeIssued in Circulation.pm (there is more functionality that could be moved in separate sbroutines).

What do you think about?

Thanks
Marc
Comment 4 Kyle M Hall 2014-04-14 12:04:21 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-04-15 11:34:06 UTC
Paxed, could you test and add your signoff to the Kyle's patch please?
Comment 6 Marc Véron 2014-04-16 07:16:08 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-04-16 10:56:51 UTC
Created attachment 27163 [details] [review]
Improve AgeRestrictionMarker to consider the case where the marker is immediately followed by the age, eg. "K16" in Finland.

How I tested:
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
Checkout should be possible
Apply patch
Checkout schould now be blocked

Patch behaves as expected

http://bugs.koha-community.org/show_bug.cgi?id=11630
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 8 Jonathan Druart 2014-04-16 10:56:54 UTC
Created attachment 27164 [details] [review]
Bug 11630 [QA Followup] - Move code to subroutine, add unit tests

Patch behaves as expected.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 9 Jonathan Druart 2014-04-16 10:56:57 UTC
Created attachment 27165 [details] [review]
Bug 11630: Unit tests can be independent of DB

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 10 Galen Charlton 2014-04-16 15:36:54 UTC
Pushed to master.  Thanks, Pasi!

Thanks also to Kyle and Jonathan for working on the test cases.  Nowadays, changes to routines in the C4 and Koha namespaces really ought to come with unit and/or regression tests.
Comment 11 Olli-Antti Kivilahti 2014-07-31 15:58:39 UTC
Just rebasing our production branch to 3.16.x and wanted to thank you guys for your great work in getting this through!