Description
Kyle M Hall (khall)
2017-02-17 15:06:29 UTC
Created attachment 60427 [details] [review] Bug 18139 - Prep unit tests Created attachment 60428 [details] [review] Bug 18139 - 'Too many checked out' can confuse librarians The too many checked out message can be very confusing for librarians when the limit specified is more or less than the number of checkouts the patron already has. We should make it clear to the librarian that his is only counting certain types of checkouts by giving them the issuing rule criteria used. Test Plan: 1) Apply this patch 2) Try to check out more items than the circ rules allow for regular and on-site checkouts Created attachment 60429 [details] [review] Bug 18139 - Add additional unit tests Hi Kyle, I like this feature very much. However, I get an Internal Server Error while systematically testing. It occurs with following settings: - Delete all rules in 'Circulation rules and fines'. - Set a Default limit by patron category, e.g. Student 2 current checkouts , 2 on-site-chekouts Checkout to a student until the limit is reached. Result: Internal server error Plack error log says: Template process failed: undef error - Can't call method "description" on an undefined value at (...)/Koha/Template/Plugin/Categories.pm line 32. Without patch, it works fine, the message is triggered. BTW: I can always check out one item more than defined by the rules. E.g. I allow 2 checkouts (by any rule), the message triggers with the 4th checkout. I would expect it to trigger with the 3rd checkout. This happens with and without patch. Patch tested with a sandbox, by Eric Gosselin <eric.gosselin@inlibro.com> Created attachment 61489 [details] [review] Bug 18139 - Prep unit tests Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Created attachment 61490 [details] [review] Bug 18139 - 'Too many checked out' can confuse librarians The too many checked out message can be very confusing for librarians when the limit specified is more or less than the number of checkouts the patron already has. We should make it clear to the librarian that his is only counting certain types of checkouts by giving them the issuing rule criteria used. Test Plan: 1) Apply this patch 2) Try to check out more items than the circ rules allow for regular and on-site checkouts Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Created attachment 61491 [details] [review] Bug 18139 - Add additional unit tests Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> (In reply to Marc Véron from comment #4) Mark, I do not recreate. Could you double check please? (In reply to Jonathan Druart from comment #9) > (In reply to Marc Véron from comment #4) > > Mark, I do not recreate. Could you double check please? Hi Jonathan I can no longer recreate the first part of comment #4 (internal server error) However I can recreate the "BTW": I can always check out one item more than defined by the rules. E.g. I allow 2 checkouts (by any rule), the message triggers with the 4th checkout. I would expect it to trigger with the 3rd checkout. This happens with and without patch. With the 4th checkout, message says: "Too many checked out. 3 checked out, only 2 are allowed." Should that go to a separate Bug? Marc Created attachment 61541 [details] [review] Bug 18139 - Prep unit tests Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 61542 [details] [review] Bug 18139 - 'Too many checked out' can confuse librarians The too many checked out message can be very confusing for librarians when the limit specified is more or less than the number of checkouts the patron already has. We should make it clear to the librarian that his is only counting certain types of checkouts by giving them the issuing rule criteria used. Test Plan: 1) Apply this patch 2) Try to check out more items than the circ rules allow for regular and on-site checkouts Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 61543 [details] [review] Bug 18139 - Add additional unit tests Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Tested al patches together, works OK, can no longer reprocuce internal server error from comment 4. Regarding "Too meny count" (see BTW in comment #4) I will file a separate bug. Signed-off-by: Marc Véron <veron@veron.ch> (In reply to Marc Véron from comment #10) > However I can recreate the "BTW": > I can always check out one item more than defined by the rules. E.g. I allow > 2 checkouts (by any rule), the message triggers with the 4th checkout. I > would expect it to trigger with the 3rd checkout. This happens with and > without patch. > > With the 4th checkout, message says: > "Too many checked out. 3 checked out, only 2 are allowed." > > Should that go to a separate Bug? Yes please, with more information because I did not recreate either. (In reply to Jonathan Druart from comment #14) > (In reply to Marc Véron from comment #10) > > However I can recreate the "BTW": > > I can always check out one item more than defined by the rules. E.g. I allow > > 2 checkouts (by any rule), the message triggers with the 4th checkout. I > > would expect it to trigger with the 3rd checkout. This happens with and > > without patch. > > > > With the 4th checkout, message says: > > "Too many checked out. 3 checked out, only 2 are allowed." > > > > Should that go to a separate Bug? > > Yes please, with more information because I did not recreate either. See Bug 18321 :-) Created attachment 61663 [details] [review] Bug 18139: Adapt tests Kyle, I think the way you changed the tests is wrong. We need to deeply check if the issuing rule is correct instead of only check if the key exists. Take a look at these tests, they almost all passed, unless if an issuing rule does not exist but one exists at patron category level (BranchBorrowerCircRule). I think it highlights an issue with your changes. Created attachment 159307 [details] [review] Bug 18139: Display the rule critera used to determin the checkout limits to the template The too many checked out message can be very confusing for librarians when the limit specified is more or less than the number of checkouts the patron already has. We should make it clear to the librarian that his is only counting certain types of checkouts by giving them the issuing rule criteria used. Test Plan: 1) Apply this patch 2) Try to check out more items than the circ rules allow for regular and on-site checkouts Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 159308 [details] [review] Bug 18139: Add/update unit tests Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 159522 [details] [review] Bug 18139: Display the rule critera used to determin the checkout limits to the template The too many checked out message can be very confusing for librarians when the limit specified is more or less than the number of checkouts the patron already has. We should make it clear to the librarian that his is only counting certain types of checkouts by giving them the issuing rule criteria used. Test Plan: 1) Apply this patch 2) Try to check out more items than the circ rules allow for regular and on-site checkouts Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 159523 [details] [review] Bug 18139: Add/update unit tests Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> All working here.. minor perltidy issues highlighted, but on inspection, I can't work out how to resolve them. Passing QA 1) Please tidy! WARN C4/Circulation.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 1093, now: 1098) OK koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt WARN t/db_dependent/Circulation/TooMany.t WARN tidiness The file is less tidy than before (bad/messy lines before: 172, now: 178) 2) We have "All libraries" in the include, but "All" for categories and itemtypes. Can we make this consistent? Sorry, running out of time today for adjusting myself! Created attachment 159728 [details] [review] Bug 18139: Change 'All libraries' to 'All' for consistency Created attachment 159729 [details] [review] Bug 18139: Tidy code (In reply to Kyle M Hall from comment #24) > Created attachment 159728 [details] [review] [review] > Bug 18139: Change 'All libraries' to 'All' for consistency Argh, I am sorry, I was not clear. The "All" is causing a bit of a translation headache, I meant to make them all specific. See bug 31735 for context. "In French, those different things have different genders and the "All" must be in that gender ("tous" for "male-gendered" things and "toutes" for "female-gendered" things)." Created attachment 159740 [details] [review] Bug 18139: Specify 'all item types' and 'all patron categories' in template (In reply to Katrin Fischer from comment #26) > (In reply to Kyle M Hall from comment #24) > > Created attachment 159728 [details] [review] [review] [review] > > Bug 18139: Change 'All libraries' to 'All' for consistency > > Argh, I am sorry, I was not clear. The "All" is causing a bit of a > translation headache, I meant to make them all specific. > > See bug 31735 for context. > > "In French, those different things have different genders and the "All" must > be in that gender ("tous" for "male-gendered" things and "toutes" for > "female-gendered" things)." Np! New patch fixes that! Created attachment 159904 [details] [review] Bug 18139: Display the rule critera used to determin the checkout limits to the template The too many checked out message can be very confusing for librarians when the limit specified is more or less than the number of checkouts the patron already has. We should make it clear to the librarian that his is only counting certain types of checkouts by giving them the issuing rule criteria used. Test Plan: 1) Apply this patch 2) Try to check out more items than the circ rules allow for regular and on-site checkouts Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 159905 [details] [review] Bug 18139: Add/update unit tests Signed-off-by: Eric Gosselin <eric.gosselin@inlibro.com> Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 159906 [details] [review] Bug 18139: Tidy code Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 159907 [details] [review] Bug 18139: Specify 'all item types' and 'all patron categories' in template Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.02 Doesn't apply cleanly to 23.05.x, will not backport. |