Bug 19945

Summary: ILSDI - Return the reason a reserve is impossible
Product: Koha Reporter: Alex Arnaud <alex.arnaud>
Component: Web servicesAssignee: Alex Arnaud <alex.arnaud>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: josef.moravec, martin.renvoize, mtompset, nick
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 19945 - ILSDI - Return the reason a reserve is impossible
Bug 19945 - ILSDI - Return the reason a reserve is impossible
Bug 19945: Follow up optimization and test correction
Bug 19945: ILSDI - Return the reason a reserve is impossible
Bug 19945: (follow-up) optimization and test correction
Bug 19945: ILSDI - Return the reason a reserve is impossible
Bug 19945: (follow-up) optimization and test correction
Bug 19945: Add a test proving that using CanBookBeReserved in ILSDI::HoldItem is wrong.
Bug 19945: Use CanItemBeReserved in ILSDI HoldItem instead of CanBookBeReserved.
Bug 21434: (follow-up) Switch two-column templates to Bootstrap grid: Tools part 2
Bug 21435: Update two-column templates with Bootstrap grid: Tools part 3
Bug 18591: Add missing schema file
Bug 18591: Fix pluralization of type
Bug 19945: ILSDI - Return the reason a reserve is impossible
Bug 19945: (follow-up) optimization and test correction
Bug 19945: Add a test proving that using CanBookBeReserved in ILSDI::HoldItem is wrong.
Bug 19945: Use CanItemBeReserved in ILSDI HoldItem instead of CanBookBeReserved.

Description Alex Arnaud 2018-01-10 09:59:57 UTC
Currently, the ILDSI services HoldTitle and HoldItem always return a "NotHoldable" code is the reserve is impossible. We need to know why. Patch is coming...
Comment 1 Alex Arnaud 2018-01-10 15:38:56 UTC
Created attachment 70409 [details] [review]
Bug 19945 - ILSDI - Return the reason a reserve is impossible

Currently, the ILDSI services HoldTitle and HoldItem always return a
"NotHoldable" code is the reserve is impossible. We need to know why

Test plan:

 - Apply this patch
 - Place a hold on a non reservable title using ILS-DI web service
   (http://koha-opac.example.org/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=)
 - you should get the reason instead of NotHoldable,
 - Place a hold on a non reservable item using ILS-DI web service
   (http://koha-opac.example.org/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
 - you should get the reason instead of NotHoldable,
Comment 2 Mark Tompsett 2018-03-27 19:35:28 UTC
Created attachment 73351 [details] [review]
Bug 19945 - ILSDI - Return the reason a reserve is impossible

Currently, the ILDSI services HoldTitle and HoldItem always return a
"NotHoldable" code is the reserve is impossible. We need to know why

Test plan:

 - Apply this patch
 - Place a hold on a non reservable title using ILS-DI web service
   (http://koha-opac.example.org/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=)
 - you should get the reason instead of NotHoldable,
 - Place a hold on a non reservable item using ILS-DI web service
   (http://koha-opac.example.org/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
 - you should get the reason instead of NotHoldable,

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 3 Mark Tompsett 2018-03-27 19:35:31 UTC
Created attachment 73352 [details] [review]
Bug 19945: Follow up optimization and test correction

The double call to CanItemBeReserved was pointless in
the C4/Reserves.pm HoldItem, as CanBookBeReserved calls
CanItemBeReserved and even returns that code.

Additionally, the tests failed, because there were 5
tests not 4.

TEST PLAN
----------
prove t/db_dependent/ILSDI_Services.t
run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 4 Jonathan Druart 2018-05-30 19:06:16 UTC
Created attachment 75688 [details] [review]
Bug 19945: ILSDI - Return the reason a reserve is impossible

Currently, the ILDSI services HoldTitle and HoldItem always return a
"NotHoldable" code is the reserve is impossible. We need to know why

Test plan:

 - Apply this patch
 - Place a hold on a non reservable title using ILS-DI web service
   (http://koha-opac.example.org/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=)
 - you should get the reason instead of NotHoldable,
 - Place a hold on a non reservable item using ILS-DI web service
   (http://koha-opac.example.org/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
 - you should get the reason instead of NotHoldable,

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 5 Jonathan Druart 2018-05-30 19:06:19 UTC
Created attachment 75689 [details] [review]
Bug 19945: (follow-up) optimization and test correction

The double call to CanItemBeReserved was pointless in
the C4/Reserves.pm HoldItem, as CanBookBeReserved calls
CanItemBeReserved and even returns that code.

Additionally, the tests failed, because there were 5
tests not 4.

TEST PLAN
----------
prove t/db_dependent/ILSDI_Services.t
run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Nick Clemens 2018-06-22 21:07:09 UTC
I don't think the second patch is quite right - CanBookBeReserved calls CanItemBeReserved, but it might not be correct for a specific item

Set a rule for 'No Holds Allowed' for an itemtype
Find a bib with no items or create on
Add an item with the disallowed item type
Attempt hold on item via ILSDI - Not allowed
Add a second item with a holdable itemtype
Attempt hold on the original unholdable item via ILSDI - Allowed?

I think the call in 'Hold Item' to CanBookBeReserved is the extraneous one unless I missed something

I also note that neither of the routines mentioned check for 'On shelf holds allowed' policy - filed as bug 20985
Comment 7 Alex Arnaud 2018-06-26 08:30:15 UTC
(In reply to Nick Clemens from comment #6)
> I don't think the second patch is quite right - CanBookBeReserved calls
> CanItemBeReserved, but it might not be correct for a specific item
> 
> Set a rule for 'No Holds Allowed' for an itemtype
> Find a bib with no items or create on
> Add an item with the disallowed item type
> Attempt hold on item via ILSDI - Not allowed
> Add a second item with a holdable itemtype
> Attempt hold on the original unholdable item via ILSDI - Allowed?
> 
> I think the call in 'Hold Item' to CanBookBeReserved is the extraneous one
> unless I missed something
> 
> I also note that neither of the routines mentioned check for 'On shelf holds
> allowed' policy - filed as bug 20985

I agree. For further, it seems to me that HoldItem should call CanItemBeReserved only, even though it may not be the point of this bug.
Comment 8 Alex Arnaud 2018-09-18 13:07:14 UTC
Created attachment 79046 [details] [review]
Bug 19945: ILSDI - Return the reason a reserve is impossible

Currently, the ILDSI services HoldTitle and HoldItem always return a
"NotHoldable" code is the reserve is impossible. We need to know why

Test plan:

 - Apply this patch
 - Place a hold on a non reservable title using ILS-DI web service
   (http://koha-opac.example.org/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=)
 - you should get the reason instead of NotHoldable,
 - Place a hold on a non reservable item using ILS-DI web service
   (http://koha-opac.example.org/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
 - you should get the reason instead of NotHoldable,

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Alex Arnaud 2018-09-18 13:07:24 UTC
Created attachment 79047 [details] [review]
Bug 19945: (follow-up) optimization and test correction

The double call to CanItemBeReserved was pointless in
the C4/Reserves.pm HoldItem, as CanBookBeReserved calls
CanItemBeReserved and even returns that code.

Additionally, the tests failed, because there were 5
tests not 4.

TEST PLAN
----------
prove t/db_dependent/ILSDI_Services.t
run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Alex Arnaud 2018-09-18 13:07:35 UTC
Created attachment 79048 [details] [review]
Bug 19945: Add a test proving that using CanBookBeReserved in ILSDI::HoldItem is wrong.
Comment 11 Alex Arnaud 2018-09-18 13:07:41 UTC
Created attachment 79049 [details] [review]
Bug 19945: Use CanItemBeReserved in ILSDI HoldItem instead of CanBookBeReserved.
Comment 12 Alex Arnaud 2018-09-18 13:10:52 UTC
Hello,

I've rebased the first two patches. Also i provide a third one that prove what Nick said is right and a fourth one that fix it.
Comment 13 Michal Denar 2018-10-27 20:51:20 UTC Comment hidden (obsolete)
Comment 14 Michal Denar 2018-10-27 20:51:24 UTC Comment hidden (obsolete)
Comment 15 Michal Denar 2018-10-27 20:51:28 UTC Comment hidden (obsolete)
Comment 16 Michal Denar 2018-10-27 20:51:32 UTC Comment hidden (obsolete)
Comment 17 Josef Moravec 2018-10-29 22:54:38 UTC
When I try to add title level hold on biblio without items, I get this error:

Can't use an undefined value as a HASH reference at /home/vagrant/kohaclone/C4/ILSDI/Services.pm line 670
Comment 18 Alex Arnaud 2018-10-31 10:01:45 UTC
(In reply to Josef Moravec from comment #17)
> When I try to add title level hold on biblio without items, I get this error:
> 
> Can't use an undefined value as a HASH reference at
> /home/vagrant/kohaclone/C4/ILSDI/Services.pm line 670

Got the same error on master. This should be fixed in an other bug imo.
Comment 19 Alex Arnaud 2018-10-31 13:22:52 UTC
I've created Bug 21738. Turn this back to Signed off
Comment 20 Kyle M Hall 2019-01-09 14:55:17 UTC
Created attachment 83745 [details] [review]
Bug 19945: ILSDI - Return the reason a reserve is impossible

Currently, the ILDSI services HoldTitle and HoldItem always return a
"NotHoldable" code is the reserve is impossible. We need to know why

Test plan:

 - Apply this patch
 - Place a hold on a non reservable title using ILS-DI web service
   (http://koha-opac.example.org/cgi-bin/koha/ilsdi.pl?service=HoldTitle&patron_id=1&bib_id=1&request_location=)
 - you should get the reason instead of NotHoldable,
 - Place a hold on a non reservable item using ILS-DI web service
   (http://koha-opac.example.org/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=1&bib_id=1&item_id=1)
 - you should get the reason instead of NotHoldable,

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2019-01-09 14:55:33 UTC
Created attachment 83746 [details] [review]
Bug 19945: (follow-up) optimization and test correction

The double call to CanItemBeReserved was pointless in
the C4/Reserves.pm HoldItem, as CanBookBeReserved calls
CanItemBeReserved and even returns that code.

Additionally, the tests failed, because there were 5
tests not 4.

TEST PLAN
----------
prove t/db_dependent/ILSDI_Services.t
run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2019-01-09 14:55:36 UTC
Created attachment 83747 [details] [review]
Bug 19945: Add a test proving that using CanBookBeReserved in ILSDI::HoldItem is wrong.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2019-01-09 14:55:40 UTC
Created attachment 83748 [details] [review]
Bug 19945: Use CanItemBeReserved in ILSDI HoldItem instead of CanBookBeReserved.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Nick Clemens 2019-01-28 14:11:51 UTC
Awesome work all!

Pushed to master for 19.05
Comment 25 Martin Renvoize 2019-02-01 12:36:39 UTC
As this is an old and very stable API and this could be perceived as a breaking change I am not going to backport this one to 18.11.x series at this time.. please speak up if you disagree and would like it as I may be persuadable under my caveat of backporting API improvements.