|
Description
Julian Maurice
2016-09-05 11:51:55 UTC
Created attachment 55174 [details] [review] Bug 17247: Check if patron is debarred in CanItemBeReserved This way, calls to ILS-DI HoldTitle and HoldItem do this check too This patch produce software error on Staff Interfacce To reproduce: Do: http://dev.koha/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=127.0.0.1 This works as expected, but when you go to bib record #1 and click in Hold tab menu at left side of screen you get the error: <http://dev.koha:8080/cgi-bin/koha/reserve/request.pl?biblionumber=1> Software error: Can't call method "is_debarred" on an undefined value at /home/koha/kohaclone/C4/Reserves.pm line 476. Yet another error: if you run: $ prove t/db_dependent/Holds.t t/db_dependent/Holds.t .. 40/61 # Looks like you planned 61 tests but ran 62. t/db_dependent/Holds.t .. Dubious, test returned 255 (wstat 65280, 0xff00) All 61 subtests passed Test Summary Report ------------------- t/db_dependent/Holds.t (Wstat: 65280 Tests: 62 Failed: 1) Failed test: 62 Non-zero exit status: 255 Parse errors: Bad plan. You planned 61 tests but ran 62. Files=1, Tests=62, 4 wallclock secs ( 0.04 usr 0.01 sys + 1.81 cusr 0.04 csys = 1.90 CPU) Result: FAIL Created attachment 70684 [details] [review] Bug 17247: Check if patron is debarred in CanItemBeReserved This way, calls to ILS-DI HoldTitle and HoldItem do this check too Hi Héctor, I cannot reproduce the error in comment 2, and the tests pass for me. Can you try again ? Created attachment 72963 [details] [review] Bug 17247: Check if patron is debarred in CanItemBeReserved This way, calls to ILS-DI HoldTitle and HoldItem do this check too Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> QA: Looking here What about moving the debarred check to CanBookBeReserved instead of each time in Item ? Looking thru ILS-DI, it would not be a problem. Created attachment 75261 [details] [review] Bug 17247: Check if patron is debarred in CanItemBeReserved This way, calls to ILS-DI HoldTitle and HoldItem do this check too Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 75262 [details] [review] Bug 17247: (QA follow-up) Add check in CanBookBeReserved too If a patron is debarred, it is useless to do the same check over and over again in CanItemBeReserved. So adding an escape in CanBookBeReserved. Additionally, catching an invalid borrowernumber in Item. Test plan: Run t/db_dependent/Holds.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> if( !$patron || $patron->is_debarred ) {
return 'debarred';
}
we should not return "debarred" if the borrowernumber passed is wrong.
Created attachment 75270 [details] [review] Bug 17247: Do not return "debarred" if borrowernumber is invalid exception must be thrown instead This is not finished and it introduces changes that are not advertised. Placing holds for restricted patron is now impossible: before: https://screenshots.firefox.com/vYnI1v89SSv9DA8M/pro.kohadev.org after: https://screenshots.firefox.com/CLXPhG1xjMNn4yUS/pro.kohadev.org It's not only related to ILS-DI Moreover, the "debarred" string, close to the checkbox, is not translatable. (In reply to Jonathan Druart from comment #13) > before: > https://screenshots.firefox.com/vYnI1v89SSv9DA8M/pro.kohadev.org > after: > https://screenshots.firefox.com/CLXPhG1xjMNn4yUS/pro.kohadev.org The screenshots expired (default 14 days) but you can set a expiration to no limit. Or store them in bugzilla. Created attachment 89652 [details] [review] Bug 17247 : Check if patron is debarred in HoldItem and HoldTitle Test plan : Make sure a debarred patron cannot hold a title or item through ILS-DI. Response from the WebService should be "PatronDebarred". Created attachment 89653 [details] [review] Bug 17247 : Check if patron is debarred in HoldItem and HoldTitle Test plan : Make sure a debarred patron cannot hold a title or item through ILS-DI. Response from the WebService should be "PatronDebarred". Hi Jonathan, I moved the checks from C4/Reserve to C4/ILDSI. This way it doesn't affect other behaviours than the ILS-DI WebService. Also the WebService now sends explicit response code. Created attachment 93549 [details] [review] Bug 17247 : Check if patron is debarred in HoldItem and HoldTitle Test plan : Make sure a debarred patron cannot hold a title or item through ILS-DI. Response from the WebService should be "PatronDebarred". Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> This is a really small bug fix now, so I am inclined to passing it. It would be nice to have a regression test for this. Also, we should be using "Restricted" instead of Debarred. I'll add a follow-up suggesting the terminology change. Created attachment 94037 [details] [review] Bug 17247: Check if patron is restricted in HoldItem and HoldTitle Test plan: Make sure a restricted patron cannot hold a title or item through ILS-DI. Response from the WebService should be "PatronDebarred". Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 94038 [details] [review] Bug 17247: (follow-up) Terminology fix: Use Restricted instead of debarred Nice work! Pushed to master for 19.11.00 Has this been considered for the 19.05.x branch? I have seen problems caused by this there too. |
