Description
Julian Maurice
2014-03-26 15:37:12 UTC
Created attachment 26589 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) This patch makes CanBookBeReserved and CanItemBeReserved to return a another variable, a hashref that describes why it cannot be reserved. It changes all calls to these functions and removes code in Perl scripts that is not needed anymore. The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Increasing severity from enh to normal, as the patch fixes a bug in ILS-DI Comment on attachment 26589 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved Review of attachment 26589 [details] [review]: ----------------------------------------------------------------- ::: C4/ILSDI/Services.pm @@ +609,4 @@ > my $title = $$biblio{title}; > > # Check if the biblio can be reserved > + my ($canbookbereserved) = CanBookBeReserved($borrowernumber, $biblionumber); The change in interface that passes back reasons is lost, when you don't include a variable for it. Also, it makes this look like a pointless variable. I suppose it's just hard to read and not really incorrect. @@ +686,5 @@ > return { code => 'RecordNotFound' } if $$item{biblionumber} ne $$biblio{biblionumber}; > > # Check for item disponibility > + my ($canitembereserved) = C4::Reserves::CanItemBeReserved( $borrowernumber, $itemnumber ); > + my ($canbookbereserved) = C4::Reserves::CanBookBeReserved( $borrowernumber, $biblionumber ); Why is there a C4::Reserves::CanBookBeReserved call here, but a CanBookBeReserved in the earlier code? > The change in interface that passes back reasons is lost, when you don't > include a variable for it. Also, it makes this look like a pointless > variable. I suppose it's just hard to read and not really incorrect. I don't understand. CanBookBeReserved now returns a list so the return value is affected to a list. Why does the variable look pointless ? I just don't need the $reasons here, so I don't retrieve it. > Why is there a C4::Reserves::CanBookBeReserved call here, but a > CanBookBeReserved in the earlier code? I don't know, but it was like that before the patch, so I didn't change this. Created attachment 28986 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved Rebased on master Pardon if I am confused, but CanBooksBeReserved can return three different reasons [alreadyreserved, maxreseres, noholdableitems] But the check in request.pl checks for alreadyreserved and assumes maxreserves otherwise my $alreadyreserved = 0; 190 $maxreserves = 1; 191 my ($can_book_be_reserved, $reasons) = 192 CanBookBeReserved( $borrowerinfo->{borrowernumber}, $biblionumber ); 193 194 unless ($can_book_be_reserved) { 195 if ($reasons->{alreadyreserved}) { 196 $alreadyreserved = 1; 197 $biblioloopiter{warn} = 1; 198 $biblioloopiter{alreadyres} = 1; 199 } else { 200 $maxreserves = 1; 201 } This seems to be the same issue as Bug 10912 To my mind, maxreserves shouldn't be flagged unless the patron is over their number of reserves. Created attachment 31524 [details]
Payment Thanks value entered into accountlines.description
Here are sequential screenshots of my steps
Comment on attachment 31524 [details]
Payment Thanks value entered into accountlines.description
Wrong bug# sorry
Hi! It looks like this duplicates Bug 13116. Could you take a look at Comment 11 of Bug 13116 and give me some ideas? Also, could you change your title to something more descriptive? Like "Propagating error messages out from CanBookBeReserved"? Hi there! Apparently Bug 13116 is already signed off. Can you refactor your feature to supplement mine? CONFLICT (content): Merge conflict in t/db_dependent/Holds.t Auto-merging reserve/request.pl CONFLICT (content): Merge conflict in reserve/request.pl Auto-merging opac/opac-reserve.pl CONFLICT (content): Merge conflict in opac/opac-reserve.pl Auto-merging C4/Reserves.pm CONFLICT (content): Merge conflict in C4/Reserves.pm Auto-merging C4/ILSDI/Services.pm CONFLICT (content): Merge conflict in C4/ILSDI/Services.pm Created attachment 35697 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Patch rebased on master (and consequently on top of bug 13116) There is one unit test that fails in t/db_dependent/Holds.t (line 331), but it fails on master too. Should I open a separate bug for this ? (In reply to Julian Maurice from comment #13) > Patch rebased on master (and consequently on top of bug 13116) > > There is one unit test that fails in t/db_dependent/Holds.t (line 331), but > it fails on master too. > Should I open a separate bug for this ? Current master e5b834a1c474f761c9f9c8c01dd6abeb99ee5eac does not fail this test with me. (In reply to M. de Rooy from comment #14) > Current master e5b834a1c474f761c9f9c8c01dd6abeb99ee5eac does not fail this > test with me. Maybe this is the kind of test that depends on some configuration in database that you have and I have not. Could you try to apply the patch and see if the test pass ? Hi Julian, I've just tried to sign this one off using a sandbox, but there was a problem applying the patch. I think it may need to be rebased again? I'll try again when you've had a look. Cheers, Alex Created attachment 36358 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved Rebased on master Created attachment 36404 [details] [review] Signed patch Alex, to change the patch's author, you can use git commit --amend --author "Author Name <authoremail@example.org>" (In reply to Jonathan Druart from comment #19) > Alex, to change the patch's author, you can use > git commit --amend --author "Author Name <authoremail@example.org>" Sorry Jonathan, I'm afraid I'm confused: do you need me to change the name of the contributor on the patch? (In reply to Alex Sassmannshausen from comment #20) > (In reply to Jonathan Druart from comment #19) > > Alex, to change the patch's author, you can use > > git commit --amend --author "Author Name <authoremail@example.org>" > > Sorry Jonathan, I'm afraid I'm confused: do you need me to change the name > of the contributor on the patch? Actually it could be done by the QAer, just wanted to share with you the trick if you didn't know. Created attachment 36630 [details] [review] Revised patch with correct author I understand now. Thank you. Attached the revised patch. Alex This seems to break one of the existing tests, please check: not ok 52 - Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails # Failed test 'Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails' # at t/db_dependent/Reserves.t line 498. # got: 'none_available' # expected: 'ageRestricted' ok 53 - Reserving a 'PEGI 16' Biblio by a 30 year old borrower succeeds # Looks like you failed 1 test of 53. Please also include a short explanation in your new tests: ok 35 - cannot request item if policy that matches on bib-level item type forbids it (bug 9532) ok 36 ok 37 ok 38 ok 39 - Waiting reserve beyond max pickup delay *not* canceled on holiday Created attachment 38397 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved + improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation This patch should make the "age restriction" test pass (In reply to Julian Maurice from comment #25) > This patch should make the "age restriction" test pass This may totally be tangential and unrelated, but I vaguely recall hitting an age restriction problem in some test file, and it turned out to be a timezone mismatch problem somewhere. Created attachment 41951 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved Rebased on master Created attachment 41952 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved Rebased on master (In reply to Katrin Fischer from comment #23) > This seems to break one of the existing tests, please check: > > not ok 52 - Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails > # Failed test 'Reserving a 'PEGI 16' Biblio by a 15 year old borrower > fails' > # at t/db_dependent/Reserves.t line 498. > # got: 'none_available' > # expected: 'ageRestricted' > ok 53 - Reserving a 'PEGI 16' Biblio by a 30 year old borrower succeeds > # Looks like you failed 1 test of 53. > > Please also include a short explanation in your new tests: > > ok 35 - cannot request item if policy that matches on bib-level item type > forbids it (bug 9532) > ok 36 > ok 37 > ok 38 > ok 39 - Waiting reserve beyond max pickup delay *not* canceled on holiday You ran it at the magical failure hour for your timezone. See bug 14362. Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Holds.t CONFLICT (content): Merge conflict in t/db_dependent/Holds.t Auto-merging reserve/request.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt Created attachment 44631 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved Rebased on master Created attachment 44632 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved Rebased on master Created attachment 46262 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 46263 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved + improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> *** Bug 14155 has been marked as a duplicate of this bug. *** Can we get tests for the age restriction check please? Hello Jonathan, Why should we add tests for the age restriction ? Code about checking age restriction has not chenged and sub C4::Circulation::GetAgeRestriction() is already tested int/Circulation/AgeRestrictionMarkers.t. Also, i can see: t/db_dependent/Reserves.t:is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'OK', "Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds" ); t/db_dependent/Reserves.t:is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , 'ageRestricted', "Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails"); (In reply to Alex Arnaud from comment #37) > Hello Jonathan, > > Why should we add tests for the age restriction ? Code about checking age > restriction has not chenged and sub C4::Circulation::GetAgeRestriction() is > already tested int/Circulation/AgeRestrictionMarkers.t. The changes about the age restriction in CanBookBeReserved is not covered by tests. Created attachment 50224 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 50225 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved + improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> I forgot to change the bug status last time. Back to Signed off Patch does not apply and needs adjustments in view of the pushed "multiple holds per record"-functionality. Created attachment 58741 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 58742 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved + improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 58743 [details] [review] Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved Since bug 14695 it is possible to reserve more than one item per biblio Also fix bad use of $item WAnted to test, but does not apply: Bug 11999: Check for age restriction in CanBookBeReserved fatal: sha1 information is lacking or useless (C4/Reserves.pm). Bug 13113 added check for age restriction on CanItemBeReserved Created attachment 63593 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 63594 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved + improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 63595 [details] [review] Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved Since bug 14695 it is possible to reserve more than one item per biblio Also fix bad use of $item Test t/db_dependent/Holds.t is failing: # Failed test 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' # at t/db_dependent/Holds.t line 472. # got: 'alreadyReserved' # expected: 'tooManyReserves' Created attachment 66267 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 66268 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved + improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 66269 [details] [review] Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved Since bug 14695 it is possible to reserve more than one item per biblio Also fix bad use of $item Created attachment 66270 [details] [review] Bug 11999: Remove the "biblio already reserved" check in CanItemBeReserved All patches rebased on master. The last patch should fix comment 52 Some comments from QA test tools, sorry FAIL C4/Reserves.pm FAIL valid "my" variable $holds masks earlier declaration in same scope FAIL reserve/request.pl FAIL valid "my" variable $holds masks earlier declaration in same scope FAIL t/db_dependent/Holds.t FAIL valid "my" variable $holds masks earlier declaration in same scope FAIL t/db_dependent/Reserves.t FAIL valid "my" variable $holds masks earlier declaration in same scope Looks like CanBookBeReserved may be called with undef biblionumber : when at intranet clicking on holds tab of a record. This generates error : Can't call method "holds" on an undefined value (In reply to Fridolin SOMERS from comment #59) > Looks like CanBookBeReserved may be called with undef biblionumber : when at > intranet clicking on holds tab of a record. > This generates error : > Can't call method "holds" on an undefined value Ok in master, CanBookBeReserved is not called if no borrower. Sorry Created attachment 88300 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 88301 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved + improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 88302 [details] [review] Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved Since bug 14695 it is possible to reserve more than one item per biblio Also fix bad use of $item Created attachment 88303 [details] [review] Bug 11999: Remove the "biblio already reserved" check in CanItemBeReserved rebased on master, plz sign me ^^ attachment 88300 [details] [review] has merge markers (<<<<) in it. Maybe the rebase was not complete? Created attachment 90142 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 90143 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same biblio (or item). - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same biblio/item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same biblio/item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 90144 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved + improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 90146 [details] [review] Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved Since bug 14695 it is possible to reserve more than one item per biblio Also fix bad use of $item Created attachment 90147 [details] [review] Bug 11999: Remove the "biblio already reserved" check in CanItemBeReserved (In reply to Magnus Enger from comment #66) > attachment 88300 [details] [review] [review] has merge markers (<<<<) in it. Maybe > the rebase was not complete? Thanks for pointing that out :) did a re-rebase and re-upload, everything applies fine now (still asking for QA though) "Pushed for QA" still doesn't mean what you think ;) Run QA tools please, and fix Created attachment 91046 [details] [review] Bug 11999: QA follow-up Arg there are conflict signes in patches : +<<<<<<< HEAD CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status}, 'tooManyReserves', +======= + CanItemBeReserved( $borrowernumbers[0], $itemnumber2), 'tooManyReserves', +>>>>>>> Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved Created attachment 91402 [details] [review] Bug 11999: QA follow-up Created attachment 91403 [details] [review] Bug 11999: QA follow-up [18.11] I see test "alreadyReserved" on same biblio still exists inCanBookBeReserved. It should be removed says "Remove 'alreadyReserved' check in CanBookBeReserved". Created attachment 91479 [details] [review] Bug 11999: Remove 'alreadyReserved' check in CanBookBeReserved Since bug 14695 it is possible to reserve more than one item per biblio Also fix bad use of $item Created attachment 91481 [details] [review] Bug 11999: QA follow-up I think you need to obsolete some of these, not sure why one followup is 18.11? Created attachment 91494 [details] [review] Bug 11999: QA follow-up (In reply to Nick Clemens from comment #82) > I think you need to obsolete some of these, not sure why one followup is > 18.11? Indeed... that one was to prepare backporting to 18.11. I tried cleaning these up, but hit some snags, a few comments: 1 - Please squash these - removing the merge markers at the end is messy 2 - We now have a routine to create helper bibs 'build_sample_biblio' - please use that 3 - you change some tests to expect 'tooManyHoldsForThisRecord' but are allowing 99 so these are failing - it would be nice to have those cases covered, but I think this change may have been accidental? Created attachment 91580 [details] [review] Bug 11999: Add two checks in CanBookBeReserved and CanItemBeReserved - Check if borrower already has reserved the same item. - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower 3/ Try to hold the same item with the same borrower through ILS-DI and note that you can Apply the patch 4/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 5/ Try to hold the same item with the same borrower through ILS-DI. You shouldn't be able to do that. 6/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 91581 [details] [review] Bug 11999: Check for age restriction in CanBookBeReserved Created attachment 91582 [details] [review] Bug 11999: Test improvements improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Created attachment 91590 [details] [review] Bug 11999: Test improvements improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Created attachment 91591 [details] [review] Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower Apply the patch 3/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 4/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 91592 [details] [review] Bug 11999: Test improvements improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Created attachment 91593 [details] [review] Bug 11999: Test improvements improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Patch no longer applies: root@e83d8895749e:koha(bz11999)$ git bz apply 11999 Bug 11999 - Add two checks in CanBookBeReserved and CanItemBeReserved 91581 - Bug 11999: Check for age restriction in CanBookBeReserved 91591 - Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved 91593 - Bug 11999: Test improvements Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11999: Check for age restriction in CanBookBeReserved error: sha1 information is lacking or useless (C4/Reserves.pm). error: could not build fake ancestor Patch failed at 0001 Bug 11999: Check for age restriction in CanBookBeReserved Similar to Bug 17229 you get an error on the patron's page when following the test plan (step 2 before patch), but before trying to apply the patch: 1) For the patron, the holds tab heading shows that there is a hold (1). 2) When you click on the patron's holds tab a message pops up with "DataTables warning: table id=holds-table - Ajax error. For more information about this error, please see http://datatables.net/tn/7" 3) The bibliographic record shows that there is a hold. I'm assuming that is because the DataTables feature for displaying results is a more recent addition to Koha. (Test URL I used for ILS_DI was http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=19&bib_id=15&request_location='127.0.0.1' using koha-testing-docker) Just realized Age Restriction issue has been fixed in BZ13113. Will re-post patches and change tests accordingly. Created attachment 99927 [details] [review] Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower Apply the patch 3/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 4/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 99928 [details] [review] Bug 11999: Test improvements improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation (In reply to David Nind from comment #94) > Similar to Bug 17229 you get an error on the patron's page when following > the test plan (step 2 before patch), but before trying to apply the patch: > > 1) For the patron, the holds tab heading shows that there is a hold (1). > > 2) When you click on the patron's holds tab a message pops up with > "DataTables warning: table id=holds-table - Ajax error. For more information > about this error, please see http://datatables.net/tn/7" > > 3) The bibliographic record shows that there is a hold. > > I'm assuming that is because the DataTables feature for displaying results > is a more recent addition to Koha. > > (Test URL I used for ILS_DI was > http://127.0.0.1:8080/cgi-bin/koha/ilsdi. > pl?service=HoldTitle&patron_id=19&bib_id=15&request_location='127.0.0.1' > using koha-testing-docker) Hi David, Thanks again for testing, I've posted new patches with conflicts fixed + tests green + qa tools green. I cannot reproduce the issue you've seen with DataTable. Kr, Arthur Hi Arthur. Have tested again, even though it is already signed off now. Everything works fine, except for the DataTables/Ajax warning. Bug 24769 created for that. QAing Sorry, but I cannot pass QA on this patch set: You add changes to Reserves.pm but you only change a test script for updating or inserting age restriction in marc subfields. We need solid test cases for the changes you make. But looking further, do we really want this change here? We already check for maxreserves in opac-reserve.pl and reserve/request.pl. So if you would need it for ILS-DI, I would expect you add it there? If we want the check in the module, we should also consider the scripts mentioned. And what about performance? If you come from CanBook, why would you test it again and again in CanItem? This check is only needed for the first item? + { status => alreadyReserved }, if the borrower has already reserved this item. Where do you find that one ? Failed QA I agree about the tests, but with all the API work - Doesn't it makes sense to centralize all the checks needed instead of repeating (and forgetting them) all over the place? The general idea makes sense to me. (In reply to Katrin Fischer from comment #102) > I agree about the tests, but with all the API work - Doesn't it makes sense > to centralize all the checks needed instead of repeating (and forgetting > them) all over the place? The general idea makes sense to me. Thanks for the +1 Katrin, that was definitely the idea behind this patch. While working with library frontend software we (@BibLibre) have stumbled across several checks and behaviors which are different from OPAC to ILS-DI. Centralizing those checks and behaviors makes a lot of sense to me as well with also the Rest API coming if we want to have a bit of coherence across the different entry points of Koha. Also makes the whole thing much easier to maintain I guess. Still I do have a bit of work ahead for the tests. I will try to solve this before the NoHackFest / CanceledHackFest Created attachment 106481 [details] [review] Bug 11999: Test improvements + check maxreserves improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Daag Marcel, from your comment : "And what about performance? If you come from CanBook, why would you test it again and again in CanItem? This check is only needed for the first item?" No performance issues involved. There is a test in CanBook before the item loop. Then if test is false it won't enter the loop. The check in CanItem is there to have the syspref checked even when using ItemLevelHold (like ILSDI-HoldItem for example). Mvg, Arthur (In reply to Arthur Suzuki from comment #105) > Daag Marcel, Goedemorgen Arthur :) > There is a test in CanBook before the item loop. > Then if test is false it won't enter the loop. > The check in CanItem is there to have the syspref checked even when using > ItemLevelHold (like ILSDI-HoldItem for example). I do not see the change? You test in CanBook and if it's green, you will test it again for every item? Maybe you need to tell CanItem to test once? Created attachment 106494 [details] [review] Bug 11999: QA follow-up > You test in CanBook and if it's green, you will
> test it again for every item?
> Maybe you need to tell CanItem to test once?
I told CanItem to test only if caller is different than CanBook, should solve.
I think $patron->holds do not actually execute SQL,
only if queried with $holds->count right?
then if first condition is false, it is not queried
eet smakelijk een goeie namiddag :)
Hi Arthur, I thought I would test again but the patch no longer applies 8-( : root@kohadevbox:koha(bz11999)$ git bz apply 11999 Bug 11999 - Add two checks in CanBookBeReserved and CanItemBeReserved 99927 - Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved 106481 - Bug 11999: Test improvements + check maxreserves 106494 - Bug 11999: QA follow-up Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved Using index info to reconstruct a base tree... M C4/Reserves.pm M reserve/request.pl Falling back to patching base and 3-way merge... Auto-merging reserve/request.pl Auto-merging C4/Reserves.pm CONFLICT (content): Merge conflict in C4/Reserves.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-11999-Add-maxreserves-check-in-CanBookBeReserv-WtX_N_.patch Created attachment 111687 [details] [review] Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower Apply the patch 3/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 4/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 111688 [details] [review] Bug 11999: Test improvements + check maxreserves improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Created attachment 111689 [details] [review] Bug 11999: (QA follow-up) Attached new patches rebased on current master, just in time for KohaCon ;) Everything seems to work as expected. However, prove t/db_dependent/Holds.t fails: t/db_dependent/Holds.t (Wstat: 3072 Tests: 67 Failed: 12) Failed tests: 37-39, 42, 47, 53, 55-58, 62, 64 Non-zero exit status: 12 Files=1, Tests=67, 7 wallclock secs ( 0.04 usr 0.01 sys + 5.38 cusr 0.41 csys = 5.84 CPU) Result: FAIL Let me know if you want a verbose log. Huuum... Hi David, Sorry, can't reproduce, this is what I got. Can you post verbose log? (in the meantime i'll try to clear those warnings) root@kohadevbox:koha(bz11999)$ prove t/db_dependent/Holds.t t/db_dependent/Holds.t .. 1/67 Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. Use of uninitialized value $caller in index at /kohadevbox/koha/C4/Reserves.pm line 429. t/db_dependent/Holds.t .. ok All tests successful. Files=1, Tests=67, 5 wallclock secs ( 0.03 usr 0.01 sys + 3.68 cusr 0.38 csys = 4.10 CPU) Result: PASS Patches no longer apply 8-( Created attachment 112871 [details] [review] Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower Apply the patch 3/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 4/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 112872 [details] [review] Bug 11999: Test improvements + check maxreserves improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Created attachment 112873 [details] [review] Bug 11999: (QA follow-up) Thanks david :) Patch did no longer apply because one of my other patch actually got into master, hurray! \o/ Re-posted all patches. Created attachment 112874 [details] [review] Bug 11999: (QA follow-up) The attached patch contains git merge conflict resolution hints. Also, if I understand correctly maxreserves is already checked in some other places in some other way, it would be really good idea to have a patch then to convert those calls to just use CanBookBeReserved(). I didn't realize the follow-ups here removed those git merge conflict resolution hints. It would still probably be better if the conflicts were solved in the patches they happen. If I review the patch "Test improvements + check maxreserves" it contains merge resolution and test improvements, so two things, git commits should do just one thing. Also "Bug 11999: (QA follow-up)" doesn't say what it does. Created attachment 120776 [details] [review] Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower Apply the patch 3/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 4/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 120777 [details] [review] Bug 11999: Test improvements + check maxreserves improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Created attachment 120778 [details] [review] Bug 11999: Test improvements + check maxreserves improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Created attachment 120779 [details] [review] Bug 11999: Add maxreserves check in CanBookBeReserved and CanItemBeReserved - Check if borrower has reached the maximum number of holds allowed (syspref maxreserves) The goal of this patch is to do these checks also when using ILS-DI services HoldTitle and HoldItem Test plan: ---------- Before patch: 1/ Set syspref maxreserves to 1 2/ Place some holds through ILS-DI and note that you can place more than 1 hold for a borrower Apply the patch 3/ Place some holds through ILS-DI, you shouldn't be able to place more than 1 hold for a borrower 4/ Try to place holds on staff interface and OPAC. The behaviour must be identical than before the patch. Maxreserves and alreadyreserved works on ILSDI Maxreserves works also on staff and opac Already reserves works also on staff and opac tests on t/db_dependent/Reserves.t and t/db_dependent/Holds.t passe (using koha_ut db) Signed-off-by: Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 120780 [details] [review] Bug 11999: Test improvements + check maxreserves improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation (In reply to Joonas Kylmälä from comment #123) > I didn't realize the follow-ups here removed those git merge conflict > resolution hints. It would still probably be better if the conflicts were > solved in the patches they happen. If I review the patch "Test improvements > + check maxreserves" it contains merge resolution and test improvements, so > two things, git commits should do just one thing. Also "Bug 11999: (QA > follow-up)" doesn't say what it does. I've squashed fixes from QA follow-up inside patches. And rebase master. I set back to Needs Signoff Test failed - if I change the number of tests to 68, everything works fine: t/db_dependent/Reserves.t (Wstat: 65280 Tests: 68 Failed: 1) Failed test: 68 Non-zero exit status: 255 Parse errors: Bad plan. You planned 67 tests but ran 68. Also, while not covered in the test plan, prove t/db_dependent/Holds.t fails (it passed before the patch was applied): t/db_dependent/Holds.t (Wstat: 4352 Tests: 71 Failed: 17) Failed tests: 37-39, 42, 47, 53-60, 63, 65, 67, 71 Non-zero exit status: 17 Created attachment 121531 [details] [review] Bug 11999: Test improvements + check maxreserves improve t/db_dependent/Reserves.t to make tests pass on UNIMARC installation Hi David, I've not reproduced the issue with t/db_dependent/Holds.t number of tests in Reserves.t has been updated. Kr, Created attachment 121612 [details]
Test fails for holds - 2021-06-04
I've attached the test results for prove -v t/db_dependent/Holds.t after the patch is applied - it passed before the patch was applied.
I'm testing on a MARC21 installation, does this need to be a UNIMARC setup?
Checking hold limits has been driving me crazy lately. It seems to me that these functions should check all the different hold limits and report back specifically why the book or item can't be reserved. For example, exceeded max reserves, exceeded total holds for the day, exceeded total holds for record, etc. Needs rebase (In reply to David Cook from comment #134) > Checking hold limits has been driving me crazy lately. > > It seems to me that these functions should check all the different hold > limits and report back specifically why the book or item can't be reserved. > For example, exceeded max reserves, exceeded total holds for the day, > exceeded total holds for record, etc. This seems to be BZ 17712. I've been trying to rebase that one with not much success so far. Git branch available here : https://git.biblibre.com/biblibre/kohac/src/branch/bz17712 Lots of tests are failing and needs rework. FYI here is our patch for 22.11 https://git.biblibre.com/biblibre/kohac/commit/2fd63a2a5916e3e19ba9b0aad90caca0225a1212 |