ILS-DI HoldTitle and HoldItem don't check patron category setting BlockExpiredPatronOpacActions.
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
Created attachment 55060 [details] [review] Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
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
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
Created attachment 62037 [details] [review] Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
Patches rebased on master
(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
Created attachment 67324 [details] screenshot with the number saved
Created attachment 67325 [details] screenshot with the number removed
There is a small conflict in Reserves.pm. Can you please rebase?
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
Created attachment 70683 [details] [review] Bug 17229: Add unit test for BlockExpiredPatronOpacActions check
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>
Created attachment 72971 [details] [review] Bug 17229: Add unit test for BlockExpiredPatronOpacActions check Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Hi Julian, can you please add a test plan for this to your commit message? Thx!
Changed from minor to normal as bypassing expiration check (important circulation rule) is bad
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.
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>
Not sure what I did is right but rebased this patch so that it applies to branch master.
Created attachment 85065 [details] [review] Bug 17229: Add unit test for BlockExpiredPatronOpacActions check Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 85066 [details] [review] Bug 17229: Add unit test for BlockExpiredPatronOpacActions check Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 85070 [details] [review] Bug 17229: Add unit test for BlockExpiredPatronOpacActions check Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Created attachment 85071 [details] [review] Bug 17229: Add unit test for BlockExpiredPatronOpacActions check Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
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>
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>
What about reserve/request.tt?
Same question from me - as this changes a core routine not only used for ILS-DI, what are the consequences in the GUI?
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
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.
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>
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.
You need to reconsider the unit test patch. Move to t/db_dependent/ILSDI_Services.t maybe.
Created attachment 89760 [details] [review] Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t This reverts commit c968aabc865e9fb9397547f5c12f0017337f7b77.
Moved unit tests :) Didn't amend previous patch for tests this time ;)
This can not really be tested before this bug is fixed: Bug 23710 - Holds broken, displays a JSON page with an error
(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
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
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
Created attachment 99701 [details] [review] Bug 17229: Add unit test for BlockExpiredPatronOpacActions check Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
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>
Created attachment 99703 [details] [review] Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t
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
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>
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>
Created attachment 99819 [details] [review] Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t Signed-off-by: David Nind <david@davidnind.com>
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
This is blocked by bug 24841. Both implementations must be identical.
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?
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>
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>
Created attachment 113743 [details] [review] Bug 17229: moved UnitTest to t/db_dependant/ILSDI_Service.t Signed-off-by: David Nind <david@davidnind.com>
(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.
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>
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>
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>
Works as expected, change of functionality but not a regression. Passing QA
I don't think my concerns have been taken into account. QA please have a look at bug 24841 comment 4.
(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
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.
(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
Will push as it, but I don't think we are going in the right direction.
Pushed to master for 21.05, thanks to everybody involved!
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.
Last patch pushed to master for 21.05.00, thanks Frido!
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)
Last patch pushed to master for 21.05.00.
Pushed to 20.11.x for 20.11.03
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