Bug 17247 - ILS-DI HoldTitle and HoldItem should check if patron is restricted
Summary: ILS-DI HoldTitle and HoldItem should check if patron is restricted
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low normal with 1 vote (vote)
Assignee: Julian Maurice
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 24841
  Show dependency treegraph
 
Reported: 2016-09-05 11:51 UTC by Julian Maurice
Modified: 2021-06-14 21:33 UTC (History)
8 users (show)

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


Attachments
Bug 17247: Check if patron is debarred in CanItemBeReserved (2.50 KB, patch)
2016-09-05 11:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17247: Check if patron is debarred in CanItemBeReserved (2.20 KB, patch)
2018-01-18 11:30 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17247: Check if patron is debarred in CanItemBeReserved (2.25 KB, patch)
2018-03-15 14:08 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 17247: Check if patron is debarred in CanItemBeReserved (2.35 KB, patch)
2018-05-11 08:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17247: (QA follow-up) Add check in CanBookBeReserved too (2.48 KB, patch)
2018-05-11 08:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17247: Do not return "debarred" if borrowernumber is invalid (1.02 KB, patch)
2018-05-11 14:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17247 : Check if patron is debarred in HoldItem and HoldTitle (1.43 KB, patch)
2019-05-13 11:10 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17247 : Check if patron is debarred in HoldItem and HoldTitle (1.43 KB, patch)
2019-05-13 11:14 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 17247 : Check if patron is debarred in HoldItem and HoldTitle (1.49 KB, patch)
2019-10-03 10:00 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 17247: Check if patron is restricted in HoldItem and HoldTitle (1.55 KB, patch)
2019-10-12 11:24 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17247: (follow-up) Terminology fix: Use Restricted instead of debarred (1.44 KB, patch)
2019-10-12 11:24 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.