Bug 17247

Summary: ILS-DI HoldTitle and HoldItem should check if patron is restricted
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: Web servicesAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: normal    
Priority: P5 - low CC: arthur.suzuki, hector.hecaxmmx, jonathan.druart, katrin.fischer, m.de.rooy, magnus, veron, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17229
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26463
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00
Bug Depends on:    
Bug Blocks: 24841    
Attachments: Bug 17247: Check if patron is debarred in CanItemBeReserved
Bug 17247: Check if patron is debarred in CanItemBeReserved
Bug 17247: Check if patron is debarred in CanItemBeReserved
Bug 17247: Check if patron is debarred in CanItemBeReserved
Bug 17247: (QA follow-up) Add check in CanBookBeReserved too
Bug 17247: Do not return "debarred" if borrowernumber is invalid
Bug 17247 : Check if patron is debarred in HoldItem and HoldTitle
Bug 17247 : Check if patron is debarred in HoldItem and HoldTitle
Bug 17247 : Check if patron is debarred in HoldItem and HoldTitle
Bug 17247: Check if patron is restricted in HoldItem and HoldTitle
Bug 17247: (follow-up) Terminology fix: Use Restricted instead of debarred

Description Julian Maurice 2016-09-05 11:51:55 UTC

    
Comment 1 Julian Maurice 2016-09-05 11:52:39 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
Comment 2 Héctor Eduardo Castro Avalos 2016-10-15 15:53:57 UTC
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.
Comment 3 Héctor Eduardo Castro Avalos 2016-10-15 15:58:21 UTC
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
Comment 4 Julian Maurice 2018-01-18 11:30:09 UTC
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
Comment 5 Julian Maurice 2018-01-18 11:32:26 UTC
Hi Héctor,

I cannot reproduce the error in comment 2, and the tests pass for me. Can you try again ?
Comment 6 Brendan Gallagher 2018-03-15 14:08:34 UTC
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>
Comment 7 Marcel de Rooy 2018-05-11 07:29:23 UTC
QA: Looking here
Comment 8 Marcel de Rooy 2018-05-11 07:43:38 UTC
What about moving the debarred check to CanBookBeReserved instead of each time in Item ?
Looking thru ILS-DI, it would not be a problem.
Comment 9 Marcel de Rooy 2018-05-11 08:42:50 UTC
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>
Comment 10 Marcel de Rooy 2018-05-11 08:42:55 UTC
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>
Comment 11 Jonathan Druart 2018-05-11 14:21:50 UTC
    if( !$patron || $patron->is_debarred ) {
        return 'debarred';
    }


we should not return "debarred" if the borrowernumber passed is wrong.
Comment 12 Jonathan Druart 2018-05-11 14:32:26 UTC
Created attachment 75270 [details] [review]
Bug 17247: Do not return "debarred" if borrowernumber is invalid

exception must be thrown instead
Comment 13 Jonathan Druart 2018-05-11 14:36:11 UTC
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.
Comment 14 Victor Grousset/tuxayo 2018-06-13 12:33:42 UTC
(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.
Comment 15 Jonathan Druart 2019-02-16 20:01:52 UTC
https://snag.gy/v5QUZg.jpg

https://snag.gy/GQqzoW.jpg
Comment 16 Arthur Suzuki 2019-05-13 11:10:51 UTC
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".
Comment 17 Arthur Suzuki 2019-05-13 11:14:27 UTC
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".
Comment 18 Arthur Suzuki 2019-05-13 11:34:19 UTC
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.
Comment 19 Brendan Gallagher 2019-10-03 10:00:07 UTC
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>
Comment 20 Katrin Fischer 2019-10-12 11:20:29 UTC
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.
Comment 21 Katrin Fischer 2019-10-12 11:24:21 UTC
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>
Comment 22 Katrin Fischer 2019-10-12 11:24:25 UTC
Created attachment 94038 [details] [review]
Bug 17247: (follow-up) Terminology fix: Use Restricted instead of debarred
Comment 23 Martin Renvoize 2019-10-15 13:16:26 UTC
Nice work!

Pushed to master for 19.11.00
Comment 24 Magnus Enger 2020-03-10 12:35:22 UTC
Has this been considered for the 19.05.x branch? I have seen problems caused by this there too.