Bug 17229

Summary: ILS-DI HoldTitle and HoldItem should check if patron is expired
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: Web servicesAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: andrewfh, arthur.suzuki, caroline.cyr-la-rose, david, dominic.pichette, fridolin.somers, hector.hecaxmmx, jonathan.druart, katrin.fischer, magnus, martin.renvoize, nick, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17247
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11999
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26463
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.03
Bug Depends on: 23710    
Bug Blocks:    
Attachments: Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
screenshot with the number saved
screenshot with the number removed
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Bug 17229: Check if patron is expired in CanItemBeReserved
Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t
Bug 17229: Fix unit test warning
Bug 17229: Fix perlcritic on ILSDI_Services.t

Description Julian Maurice 2016-08-31 13:33:44 UTC
ILS-DI HoldTitle and HoldItem don't check patron category setting BlockExpiredPatronOpacActions.
Comment 1 Julian Maurice 2016-08-31 13:36:01 UTC
Created attachment 55059 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too
Comment 2 Julian Maurice 2016-08-31 13:52:26 UTC
Created attachment 55060 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Comment 3 Héctor Eduardo Castro Avalos 2016-10-15 16:06:50 UTC
Hi Julian

Sorry but patch does not apply anymore

Applying: Bug 17229: Check if patron is expired in CanItemBeReserved
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 17229: Check if patron is expired in CanItemBeReserved
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-17229-Check-if-patron-is-expired-in-CanItemBeR-oomixj.patch


You should do this one dependent of bug 16246
Comment 4 Julian Maurice 2017-04-11 07:44:48 UTC
Created attachment 62036 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too
Comment 5 Julian Maurice 2017-04-11 07:44:51 UTC
Created attachment 62037 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Comment 6 Julian Maurice 2017-04-11 07:45:08 UTC
Patches rebased on master
Comment 7 Julian Maurice 2017-04-11 07:47:44 UTC
(In reply to Héctor Eduardo Castro Avalos from comment #3)
> You should do this one dependent of bug 16246

Why ? They seem to be independent
Comment 8 Dominic Pichette 2017-09-22 18:34:46 UTC
Created attachment 67324 [details]
screenshot with the number saved
Comment 9 Dominic Pichette 2017-09-22 18:35:04 UTC
Created attachment 67325 [details]
screenshot with the number removed
Comment 10 Katrin Fischer 2017-10-15 09:08:38 UTC
There is a small conflict in Reserves.pm. Can you please rebase?
Comment 11 Julian Maurice 2018-01-18 11:16:57 UTC
Created attachment 70682 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too
Comment 12 Julian Maurice 2018-01-18 11:17:01 UTC
Created attachment 70683 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Comment 13 Brendan Gallagher 2018-03-15 15:12:38 UTC
Created attachment 72970 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 14 Brendan Gallagher 2018-03-15 15:12:49 UTC
Created attachment 72971 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 15 Katrin Fischer 2018-03-20 21:41:28 UTC
Hi Julian,
can you please add a test plan for this to your commit message? Thx!
Comment 16 Victor Grousset/tuxayo 2018-06-13 10:36:15 UTC
Changed from minor to normal as bypassing expiration check (important  circulation rule) is bad
Comment 17 Arthur Suzuki 2019-02-12 10:58:23 UTC
How to test :
-Set Syspref  BlockExpiredPatronOpacActions to "ON",
-Set a patron's category variable "Block expired patrons" to "Follow SysPref" or -"Block" (ideally test both).
-Get the id of a patron from this category (ie : 1234).
-Set this patron's expiration date to a date earlier than today.
-Get a biblionumber which can be reserved (ie : 5678).

Put the following string in your webbrowser (replacing OpacBaseUrl, 1234 and 5678 by your own values) : 
http://[OpacBaseUrl]/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1234&bib_id=5678&request_location='127.0.0.1'

Should not create a new hold for the patron and report an error.
Comment 18 Arthur Suzuki 2019-02-13 18:22:51 UTC
Created attachment 85064 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 19 Arthur Suzuki 2019-02-13 18:26:57 UTC
Not sure what I did is right but rebased this patch so that it applies to branch master.
Comment 20 Arthur Suzuki 2019-02-13 18:37:53 UTC
Created attachment 85065 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 21 Arthur Suzuki 2019-02-13 18:48:11 UTC
Created attachment 85066 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 22 Arthur Suzuki 2019-02-14 01:20:00 UTC
Created attachment 85070 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 23 Arthur Suzuki 2019-02-14 01:20:32 UTC
Created attachment 85071 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 24 Arthur Suzuki 2019-02-14 01:21:18 UTC
Created attachment 85072 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 25 Arthur Suzuki 2019-02-14 14:16:43 UTC
Created attachment 85118 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too

Added test plan to commit message :
-Set Syspref  BlockExpiredPatronOpacActions to "ON",
-Set a patron's category variable "Block expired patrons" to "Follow SysPref" or -"Block" (ideally test both).
-Get the id of a patron from this category (ie : 1234).
-Set this patron's expiration date to a date earlier than today.
-Get a biblionumber which can be reserved (ie : 5678).

Put the following string in your webbrowser (replacing OpacBaseUrl, 1234 and 5678 by your own values) :
http://[OpacBaseUrl]/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1234&bib_id=5678&request_location='127.0.0.1'

Should not create a new hold for the patron and report an error.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 26 Jonathan Druart 2019-02-16 20:07:47 UTC
What about reserve/request.tt?
Comment 27 Katrin Fischer 2019-02-17 11:31:30 UTC
Same question from me - as this changes a core routine not only used for ILS-DI, what are the consequences in the GUI?
Comment 28 Katrin Fischer 2019-02-17 11:33:12 UTC
Tiny issue:

 FAIL	t/db_dependent/Holds.t
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 142)
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid
Comment 29 Arthur Suzuki 2019-02-22 10:59:51 UTC
Patch also impacts holds submitted through Intranet, need reworks...
Maybe doing the check in ILSDI/Services.pm instead of Reserves.pm would do the job.
Comment 30 Arthur Suzuki 2019-02-28 18:52:45 UTC
Created attachment 85883 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too

Added test plan to commit message :
-Set Syspref  BlockExpiredPatronOpacActions to "ON",
-Set a patron's category variable "Block expired patrons" to "Follow SysPref" or -"Block" (ideally test both).
-Get the id of a patron from this category (ie : 1234).
-Set this patron's expiration date to a date earlier than today.
-Get a biblionumber which can be reserved (ie : 5678).

Put the following string in your webbrowser (replacing OpacBaseUrl, 1234 and 5678 by your own values) :
http://[OpacBaseUrl]/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1234&bib_id=5678&request_location='127.0.0.1'

Should not create a new hold for the patron and report an error.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 31 Victor Grousset/tuxayo 2019-03-01 10:16:23 UTC
Hi :)
To preserve the validity of the sign off (and other reasons which I don't remember ^^"), it's usual to do followup patches instead of amending the signed one.
Comment 32 Fridolin Somers 2019-03-07 08:46:32 UTC
You need to reconsider the unit test patch.
Move to t/db_dependent/ILSDI_Services.t maybe.
Comment 33 Arthur Suzuki 2019-05-14 14:59:09 UTC
Created attachment 89760 [details] [review]
Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t

This reverts commit c968aabc865e9fb9397547f5c12f0017337f7b77.
Comment 34 Arthur Suzuki 2019-05-14 15:00:12 UTC
Moved unit tests :)
Didn't amend previous patch for tests this time ;)
Comment 35 Magnus Enger 2019-10-03 13:54:53 UTC
This can not really be tested before this bug is fixed:
Bug 23710 - Holds broken, displays a JSON page with an error
Comment 36 Arthur Suzuki 2019-11-22 11:33:55 UTC
(In reply to Magnus Enger from comment #35)
> This can not really be tested before this bug is fixed:
> Bug 23710 - Holds broken, displays a JSON page with an error

this bug has been resolved fixed :)
Also this could have been tested using curl and trying to put a hold through ILS-DI
Comment 37 David Nind 2020-02-23 09:32:35 UTC
Patch no longer applies

Applying: Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
.git/rebase-apply/patch:41: space before tab in indent.
    	dateexpiry => '2000-01-01',
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	t/db_dependent/Holds.t
.git/rebase-apply/patch:41: space before tab in indent.
    	dateexpiry => '2000-01-01',
.git/rebase-apply/patch:56: new blank line at EOF.
+
warning: 1 line applied after fixing whitespace errors.
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
error: Failed to merge in the changes.
Patch failed at 0001 Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Comment 38 David Nind 2020-02-23 09:56:16 UTC
You get an error on the patron's page when following the test plan, but before trying to apply the patch - the URL is entered:

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.

Other notes for testing:

- Enable the ILS-DI system preference
- The BlockExpiredPatronOpacActions system preference options are now 'Block' and 'Don't block', rather then 'ON'
- For the URL, enter the bibliographic record id, not an individual item id
Comment 39 Arthur Suzuki 2020-02-27 14:41:33 UTC
Created attachment 99701 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 40 Arthur Suzuki 2020-02-27 14:41:49 UTC
Created attachment 99702 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too

Added test plan to commit message :
-Set Syspref  BlockExpiredPatronOpacActions to "ON",
-Set a patron's category variable "Block expired patrons" to "Follow SysPref" or -"Block" (ideally test both).
-Get the id of a patron from this category (ie : 1234).
-Set this patron's expiration date to a date earlier than today.
-Get a biblionumber which can be reserved (ie : 5678).

Put the following string in your webbrowser (replacing OpacBaseUrl, 1234 and 5678 by your own values) :
http://[OpacBaseUrl]/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1234&bib_id=5678&request_location='127.0.0.1'

Should not create a new hold for the patron and report an error.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 41 Arthur Suzuki 2020-02-27 14:42:17 UTC
Created attachment 99703 [details] [review]
Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t
Comment 42 Arthur Suzuki 2020-02-27 14:48:03 UTC
Hi David,
I've rebased the patch on master, should now applies fine (and I don't have the error on Holds tab in patroninfo page anymore.
Kind regards,
Arthur
Comment 43 David Nind 2020-02-29 19:40:22 UTC
Created attachment 99817 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 44 David Nind 2020-02-29 19:40:26 UTC
Created attachment 99818 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too

Added test plan to commit message :
-Set Syspref  BlockExpiredPatronOpacActions to "ON",
-Set a patron's category variable "Block expired patrons" to "Follow SysPref" or -"Block" (ideally test both).
-Get the id of a patron from this category (ie : 1234).
-Set this patron's expiration date to a date earlier than today.
-Get a biblionumber which can be reserved (ie : 5678).

Put the following string in your webbrowser (replacing OpacBaseUrl, 1234 and 5678 by your own values) :
http://[OpacBaseUrl]/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1234&bib_id=5678&request_location='127.0.0.1'

Should not create a new hold for the patron and report an error.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 45 David Nind 2020-02-29 19:40:30 UTC
Created attachment 99819 [details] [review]
Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 46 David Nind 2020-02-29 19:56:14 UTC
Hi Arthur.

Just confirming this works.

I have created bug 24769 for the DataTables error on the patron's hold page:
- this does not occur in 19.11.x; and
- this error occurs before applying your patch.

David
Comment 47 Jonathan Druart 2020-03-18 09:02:50 UTC
This is blocked by bug 24841. Both implementations must be identical.
Comment 48 Arthur Suzuki 2020-11-17 15:47:43 UTC
Implementation here is the same as in BZ 17247 which is refered to by 24841...
I've rebased the patch on current master, hopping it may pass.
If not, can you be more precise on the changes needed for this bz to pass?
Comment 49 Arthur Suzuki 2020-11-17 16:40:50 UTC
Created attachment 113741 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 50 Arthur Suzuki 2020-11-17 16:41:04 UTC
Created attachment 113742 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too

Added test plan to commit message :
-Set Syspref  BlockExpiredPatronOpacActions to "ON",
-Set a patron's category variable "Block expired patrons" to "Follow SysPref" or -"Block" (ideally test both).
-Get the id of a patron from this category (ie : 1234).
-Set this patron's expiration date to a date earlier than today.
-Get a biblionumber which can be reserved (ie : 5678).

Put the following string in your webbrowser (replacing OpacBaseUrl, 1234 and 5678 by your own values) :
http://[OpacBaseUrl]/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1234&bib_id=5678&request_location='127.0.0.1'

Should not create a new hold for the patron and report an error.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 51 Arthur Suzuki 2020-11-17 16:41:08 UTC
Created attachment 113743 [details] [review]
Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 52 Jonathan Druart 2020-11-18 11:12:45 UTC
(In reply to Arthur Suzuki from comment #48)
> Implementation here is the same as in BZ 17247 which is refered to by
> 24841...
> I've rebased the patch on current master, hopping it may pass.
> If not, can you be more precise on the changes needed for this bz to pass?

I've tried to explain on bug 24841 comment 4.
Comment 53 Martin Renvoize 2021-01-12 12:22:32 UTC
Created attachment 115081 [details] [review]
Bug 17229: Add unit test for BlockExpiredPatronOpacActions check

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 54 Martin Renvoize 2021-01-12 12:22:37 UTC
Created attachment 115082 [details] [review]
Bug 17229: Check if patron is expired in CanItemBeReserved

This way, calls to ILS-DI HoldTitle and HoldItem do this check too

Added test plan to commit message :
-Set Syspref  BlockExpiredPatronOpacActions to "ON",
-Set a patron's category variable "Block expired patrons" to "Follow SysPref" or -"Block" (ideally test both).
-Get the id of a patron from this category (ie : 1234).
-Set this patron's expiration date to a date earlier than today.
-Get a biblionumber which can be reserved (ie : 5678).

Put the following string in your webbrowser (replacing OpacBaseUrl, 1234 and 5678 by your own values) :
http://[OpacBaseUrl]/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1234&bib_id=5678&request_location='127.0.0.1'

Should not create a new hold for the patron and report an error.

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2021-01-12 12:22:41 UTC
Created attachment 115083 [details] [review]
Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2021-01-12 12:23:15 UTC
Works as expected, change of functionality but not a regression.

Passing QA
Comment 57 Jonathan Druart 2021-01-12 15:02:04 UTC
I don't think my concerns have been taken into account. QA please have a look at bug 24841 comment 4.
Comment 58 Nick Clemens 2021-01-13 13:31:57 UTC
(In reply to Jonathan Druart from comment #57)
> I don't think my concerns have been taken into account. QA please have a
> look at bug 24841 comment 4.

I think this fix is okay. Current code for this check exists in the opac scripts and not the modules. Possibly these checks should be moved to the modules, but that is a larger refactoring.

ILS-DI behaviour vs REST API behaviour I think are expected to be different. ILS-DI is unprivileged but meant to act as the OPAC itself (these functions live in Level 3: Elementary OPAC alternative) and presumes the caller has sorted out the rights etc.

The REST API is called by a user - that user's privileges decide whether the action can be completed or not.

Moving the check into CanItemBeReserved means we need to make that function interface aware and is a bigger change

TL/DR: BlockExpiredOpacActions should affect the opac only, ILS-DI is the OPAC, REST API is not
Comment 59 Jonathan Druart 2021-01-14 13:53:08 UTC
Here the behaviours are expected to be the same (between ILSDI and REST API), the existence of bug 24841 is proving it.

If we go with this patch we will have to fix bug 24841 anyway, and what are we going to do? Add the checks to the controller?
So we will end up with the same checks at 3 different places, with eventually other places where the checks are missing.

I know it's more work to have things done properly, but it's not a good pro argument :D

I don't think I am convinced yet.
Comment 60 Nick Clemens 2021-01-14 14:00:22 UTC
(In reply to Jonathan Druart from comment #59)
> Here the behaviours are expected to be the same (between ILSDI and REST
> API), the existence of bug 24841 is proving it.

Unless that bug is incorrect?

The holds route is privileged, not public - it behaves as staff, not as OPAC - so it doesn't check if patron restricted/debarred, I will add this comment to that bug

> 
> If we go with this patch we will have to fix bug 24841 anyway, and what are
> we going to do? Add the checks to the controller?

We have to decide what we want to do yes, but we shouldn't enforce that conversation here

> I know it's more work to have things done properly, but it's not a good pro
> argument :D

I don't see it as doing things properly, but fixing a confirmed bug here and making architecture decisions later.

We should work towards deprecating ILSDI, but until then we should allow for fixing these outstanding issues
Comment 61 Jonathan Druart 2021-01-19 14:38:31 UTC
Will push as it, but I don't think we are going in the right direction.
Comment 62 Jonathan Druart 2021-01-19 15:23:57 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 63 Fridolin Somers 2021-01-20 08:52:09 UTC
Created attachment 115411 [details] [review]
Bug 17229: Fix unit test warning

Remove warning :
t/db_dependent/ILSDI_Services.t .. 3/10 Null value for biblionumber in Item not allowed at /home/koha/src/t/lib/TestBuilder.pm line 387

$biblio_with_no_item is a Koha::Biblio object.
Comment 64 Jonathan Druart 2021-01-20 12:51:31 UTC
Last patch pushed to master for 21.05.00, thanks Frido!
Comment 65 Jonathan Druart 2021-01-20 14:21:20 UTC
Created attachment 115435 [details] [review]
Bug 17229: Fix perlcritic on ILSDI_Services.t

Subroutine "new" called using indirect syntax at line 456, column 14.  See page 349 of PBP.  (Severity: 4)
Comment 66 Jonathan Druart 2021-01-21 13:40:43 UTC
Last patch pushed to master for 21.05.00.
Comment 67 Fridolin Somers 2021-01-25 13:56:06 UTC
Pushed to 20.11.x for 20.11.03
Comment 68 Andrew Fuerste-Henry 2021-02-01 20:16:36 UTC
I'm getting test failures when trying to apply this to 20.05. Please address for backport. Here's what I see:

$ prove t/db_dependent/ILSDI_Services.t
t/db_dependent/ILSDI_Services.t .. 1/10 Use of bare << to mean <<"" is deprecated at t/db_dependent/ILSDI_Services.t line 452.
Use of bare << to mean <<"" is deprecated at t/db_dependent/ILSDI_Services.t line 452.
Bareword found where operator expected at t/db_dependent/ILSDI_Services.t line 466, near "HEAD
    is"
  (Might be a runaway multi-line << string starting on line 459)
	(Do you need to predeclare HEAD?)
Global symbol "$module" requires explicit package name (did you forget to declare "my $module"?) at t/db_dependent/ILSDI_Services.t line 454.
Global symbol "$module" requires explicit package name (did you forget to declare "my $module"?) at t/db_dependent/ILSDI_Services.t line 455.
Global symbol "$issue" requires explicit package name (did you forget to declare "my $issue"?) at t/db_dependent/ILSDI_Services.t line 456.
Global symbol "$origin_branch" requires explicit package name (did you forget to declare "my $origin_branch"?) at t/db_dependent/ILSDI_Services.t line 463.
syntax error at t/db_dependent/ILSDI_Services.t line 466, near "HEAD
    is"
syntax error at t/db_dependent/ILSDI_Services.t line 466, near ""Patron has issued same book" )"
Global symbol "$reply" requires explicit package name (did you forget to declare "my $reply"?) at t/db_dependent/ILSDI_Services.t line 467.
Global symbol "$biblio_with_no_item" requires explicit package name (did you forget to declare "my $biblio_with_no_item"?) at t/db_dependent/ILSDI_Services.t line 490.
Global symbol "$query" requires explicit package name (did you forget to declare "my $query"?) at t/db_dependent/ILSDI_Services.t line 495.
Global symbol "$query" requires explicit package name (did you forget to declare "my $query"?) at t/db_dependent/ILSDI_Services.t line 496.
Global symbol "$query" requires explicit package name (did you forget to declare "my $query"?) at t/db_dependent/ILSDI_Services.t line 497.
Global symbol "$biblio_with_no_item" requires explicit package name (did you forget to declare "my $biblio_with_no_item"?) at t/db_dependent/ILSDI_Services.t line 497.
Global symbol "$query" requires explicit package name (did you forget to declare "my $query"?) at t/db_dependent/ILSDI_Services.t line 498.
Global symbol "$reply" requires explicit package name (did you forget to declare "my $reply"?) at t/db_dependent/ILSDI_Services.t line 500.
Global symbol "$query" requires explicit package name (did you forget to declare "my $query"?) at t/db_dependent/ILSDI_Services.t line 500.
Global symbol "$reply" requires explicit package name (did you forget to declare "my $reply"?) at t/db_dependent/ILSDI_Services.t line 501.
syntax error at t/db_dependent/ILSDI_Services.t line 503, near ">>"
t/db_dependent/ILSDI_Services.t has too many errors.
# Looks like your test exited with 255 just after 1.
t/db_dependent/ILSDI_Services.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 9/10 subtests 

Test Summary Report
-------------------
t/db_dependent/ILSDI_Services.t (Wstat: 65280 Tests: 1 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 10 tests but ran 1.
Files=1, Tests=1,  1 wallclock secs ( 0.02 usr  0.01 sys +  0.67 cusr  0.14 csys =  0.84 CPU)
Result: FAIL