Bug 19945 - ILSDI - Return the reason a reserve is impossible
Summary: ILSDI - Return the reason a reserve is impossible
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Arnaud
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-10 09:59 UTC by Alex Arnaud
Modified: 2020-11-12 15:07 UTC (History)
4 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:


Attachments
Bug 19945 - ILSDI - Return the reason a reserve is impossible (5.13 KB, patch)
2018-01-10 15:38 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 19945 - ILSDI - Return the reason a reserve is impossible (4.97 KB, patch)
2018-03-27 19:35 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 19945: Follow up optimization and test correction (1.79 KB, patch)
2018-03-27 19:35 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 19945: ILSDI - Return the reason a reserve is impossible (5.06 KB, patch)
2018-05-30 19:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19945: (follow-up) optimization and test correction (1.88 KB, patch)
2018-05-30 19:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19945: ILSDI - Return the reason a reserve is impossible (5.38 KB, patch)
2018-09-18 13:07 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 19945: (follow-up) optimization and test correction (1.89 KB, patch)
2018-09-18 13:07 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 19945: Add a test proving that using CanBookBeReserved in ILSDI::HoldItem is wrong. (2.20 KB, patch)
2018-09-18 13:07 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 19945: Use CanItemBeReserved in ILSDI HoldItem instead of CanBookBeReserved. (1.10 KB, patch)
2018-09-18 13:07 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 21434: (follow-up) Switch two-column templates to Bootstrap grid: Tools part 2 (1.65 KB, patch)
2018-10-27 20:51 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21435: Update two-column templates with Bootstrap grid: Tools part 3 (13.47 KB, patch)
2018-10-27 20:51 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 18591: Add missing schema file (3.20 KB, patch)
2018-10-27 20:51 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 18591: Fix pluralization of type (748 bytes, patch)
2018-10-27 20:51 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 19945: ILSDI - Return the reason a reserve is impossible (6.01 KB, patch)
2019-01-09 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19945: (follow-up) optimization and test correction (1.55 KB, patch)
2019-01-09 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19945: Add a test proving that using CanBookBeReserved in ILSDI::HoldItem is wrong. (2.27 KB, patch)
2019-01-09 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19945: Use CanItemBeReserved in ILSDI HoldItem instead of CanBookBeReserved. (1.17 KB, patch)
2019-01-09 14:55 UTC, Kyle M Hall
Details | Diff | Splinter Review

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