Bug 21284 - ILS-DI: Allow GetPatronInfo to tell if a checked out item is on hold for someone else
Summary: ILS-DI: Allow GetPatronInfo to tell if a checked out item is on hold for some...
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matthias Meusburger
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-29 13:34 UTC by Matthias Meusburger
Modified: 2023-11-14 15:18 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds two new entries in the loans section of a GetPatronInfo response: - item_on_hold: number of holds on this specific item - record_on_hold: number of holds on the record This allows an ILS-DI client to know if a loaned item is already on hold by someone else, and how many holds there are.
Version(s) released in:
23.11.00,23.05.06


Attachments
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else. (2.13 KB, patch)
2018-08-29 14:09 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 21284: Unit tests (5.55 KB, patch)
2018-08-29 14:10 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else. (1.87 KB, patch)
2019-06-05 12:15 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 21284: Unit tests (5.58 KB, patch)
2020-09-01 14:18 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else. (1.88 KB, patch)
2020-09-01 14:18 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else. (9.04 KB, patch)
2023-10-23 13:18 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else. (7.87 KB, patch)
2023-10-24 08:41 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else. (7.92 KB, patch)
2023-10-24 09:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else. (7.98 KB, patch)
2023-11-03 16:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21284: (QA follow-up) Rename itemonhold and recordonhold (5.21 KB, patch)
2023-11-03 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21284: (QA follow-up) Fix QA script issues (8.76 KB, patch)
2023-11-03 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21284: [23.05] (follow-up) Fix unit test with unblessed patron (1.07 KB, patch)
2023-11-13 19:04 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Meusburger 2018-08-29 13:34:31 UTC
This patch adds two entries in the loans section of GetPatronInfo's response:
 
 - itemonhold: number of holds on this specific item.
 - recordonhold: number of holds on the record.


It allows an ILS-DI client to know if a loaned item is already on hold by someone else, and how much holds there are.
Comment 1 Matthias Meusburger 2018-08-29 14:09:54 UTC
Created attachment 78268 [details] [review]
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

This patch adds two entries in the loans section of GetPatronInfo's response:

 - itemonhold: number of holds on this specific item.
 - recordonhold: number of holds on the record.

Test plan:

 - Call GetPatronInfo on a user with a loan that has no holds
 - Check that itemonhold and recordonhold entries are equal to zero

 - Add holds on biblio and/or item level
 - Check that itemonhold and recordonhold are incremented accordingly

Please note that a hold on item level counts as a hold on record level, but not vice-versa.
Comment 2 Matthias Meusburger 2018-08-29 14:10:22 UTC
Created attachment 78269 [details] [review]
Bug 21284: Unit tests
Comment 3 Katrin Fischer 2018-12-20 21:29:46 UTC
Hi Matts, please don't forget to assign the bug to yourself when providing a patch!
Comment 4 Fridolin Somers 2019-04-25 14:11:53 UTC
 use C4::Reserves qw(AddReserve CanBookBeReserved CanItemBeReserved IsAvailableForItemLevelRequest);
 use C4::Context;
 use C4::AuthoritiesMarc;
+use C4::Reserves;
 use XML::Simple;

I think this is a mistake, use C4::Reserves already exists
Comment 5 Matthias Meusburger 2019-06-05 12:15:41 UTC
Created attachment 90326 [details] [review]
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

This patch adds two entries in the loans section of GetPatronInfo's response:

 - itemonhold: number of holds on this specific item.
 - recordonhold: number of holds on the record.

Test plan:

 - Call GetPatronInfo on a user with a loan that has no holds
 - Check that itemonhold and recordonhold entries are equal to zero

 - Add holds on biblio and/or item level
 - Check that itemonhold and recordonhold are incremented accordingly

Please note that a hold on item level counts as a hold on record level, but not vice-versa.
Comment 6 Matthias Meusburger 2019-06-05 12:16:37 UTC
Patch rebased and use C4::Reserves removed.
Comment 7 Fridolin Somers 2019-07-18 08:48:27 UTC
Shouldn't we use Koha::Holds->search() ?
Comment 8 David Nind 2020-04-01 08:14:01 UTC
Unit tests patch no longer applies - the rest of the test plan works as expected:

root@007e9492a1aa:koha(bz21284)$ git bz apply 21284
Bug 21284 - ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

78269 - Bug 21284: Unit tests
90326 - Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 21284: Unit tests
Using index info to reconstruct a base tree...
M	t/db_dependent/ILSDI_Services.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/ILSDI_Services.t
CONFLICT (content): Merge conflict in t/db_dependent/ILSDI_Services.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 21284: Unit tests
Comment 9 Fridolin Somers 2020-09-01 14:18:03 UTC
Created attachment 109438 [details] [review]
Bug 21284: Unit tests
Comment 10 Fridolin Somers 2020-09-01 14:18:16 UTC
Created attachment 109439 [details] [review]
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

This patch adds two entries in the loans section of GetPatronInfo's response:

 - itemonhold: number of holds on this specific item.
 - recordonhold: number of holds on the record.

Test plan:

 - Call GetPatronInfo on a user with a loan that has no holds
 - Check that itemonhold and recordonhold entries are equal to zero

 - Add holds on biblio and/or item level
 - Check that itemonhold and recordonhold are incremented accordingly

Please note that a hold on item level counts as a hold on record level, but not vice-versa.
Comment 11 Fridolin Somers 2020-09-01 14:18:49 UTC
/me only did a quick rebase on current master
Comment 12 David Nind 2020-09-05 23:29:50 UTC
Sorry Matthias and Fridolin, the tests fail 8-( :

root@kohadevbox:koha(bz21284)$ prove t/db_dependent/ILSDI_Services.t
t/db_dependent/ILSDI_Services.t .. 2/11     # Looks like you planned 8 tests but ran 2.
t/db_dependent/ILSDI_Services.t .. 3/11 
#   Failed test 'GetPatronInfo test for holds'
#   at t/db_dependent/ILSDI_Services.t line 224.
Can't use string ("T_ILSDI") as a HASH ref while "strict refs" in use at /kohadevbox/koha/C4/Reserves.pm line 177.
# Looks like your test exited with 255 just after 3.
t/db_dependent/ILSDI_Services.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 9/11 subtests 

Test Summary Report
-------------------
t/db_dependent/ILSDI_Services.t (Wstat: 65280 Tests: 3 Failed: 1)
  Failed test:  3
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 11 tests but ran 3.
Files=1, Tests=3,  4 wallclock secs ( 0.03 usr  0.00 sys +  3.08 cusr  0.35 csys =  3.46 CPU)
Result: FAIL
Comment 13 David Nind 2020-09-05 23:47:28 UTC
Here is an updated test plan that I used:

This patch adds two new entries to the loans section of an ILS-DI GetPatronInfo response:

- itemonhold: number of holds on a specific item.
- recordonhold: number of holds on the record.

It allows an ILS-DI client to know if a loaned item is already on hold by someone else, and how many holds there are.

Test plan:
1. Apply the patch.
2. Enable the ILS-DI system preference.
3. Check out an item for a patron and make sure there no other holds at either an item or record level.
4. Check that the new itemonhold and recordonhold entries displayed are equal to zero (example: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=19&show_contact=0&show_loans=1).
5. Add either a record or item level hold for the record used in step 2.
6. Check that itemonhold and recordonhold values are incremented accordingly.
   Note: a hold at an item level counts as a hold at a record level, but not vice-versa.
7. Run the tests and make sure they pass: prove t/db_dependent/ILSDI_Services.t
8. Sign-off!

Also, does any text need to be updated for the description information in ILS-DI (http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=Describe&verb=GetPatronInfo)? For example, add entries for itemonhold and recordonhold to the item response example.
Comment 14 Fridolin Somers 2023-09-11 20:42:47 UTC
Surely now it can use Koha::Holds via item->holds and biblio->holds
Comment 15 Matthias Meusburger 2023-10-23 13:18:06 UTC
Created attachment 157648 [details] [review]
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

This patch adds two new entries in the loans section of GetPatronInfo response:

 - itemonhold: number of holds on this specific item.
 - recordonhold: number of holds on the record.

It allows an ILS-DI client to know if a loaned item is already on hold by someone else, and how many holds there are.

Test plan:
1. Apply the patch.
2. Enable the ILS-DI system preference.
3. Check out an item for a patron and make sure there no other holds at either an item or record level.
4. Check that the new itemonhold and recordonhold entries displayed are equal to zero (example: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=19&show_contact=0&show_loans=1).
5. Add either a record or item level hold for the record used in step 2.
6. Check that itemonhold and recordonhold values are incremented accordingly.
   Note: a hold at an item level counts as a hold at a record level, but not vice-versa.
7. Run the tests and make sure they pass: prove t/db_dependent/ILSDI_Services.t
8. Sign-off!
Comment 16 Matthias Meusburger 2023-10-23 13:21:06 UTC
Hi David and Frido, thanks for the feedback!

The unit tests have been updated and work again.

"Shouldn't we use Koha::Holds->search() ?"

=> Done.

"Also, does any text need to be updated for the description information in ILS-DI (http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=Describe&verb=GetPatronInfo)? For example, add entries for itemonhold and recordonhold to the item response example."

=> Done.

Also, I updated the commit message with your test plan, David.
Comment 17 David Nind 2023-10-23 20:50:44 UTC
Thanks!

Getting sha1 and fake ancestor error when applying the patch 8-(..

kohadev-koha@kohadevbox:koha(bz21284)$ git bz apply 21284

Bug 21284 - ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

157648 - Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.
error: sha1 information is lacking or useless (t/db_dependent/ILSDI_Services.t).
error: could not build fake ancestor
Patch failed at 0001 Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.
Comment 18 Matthias Meusburger 2023-10-24 08:41:28 UTC
Created attachment 157727 [details] [review]
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

This patch adds two new entries in the loans section of GetPatronInfo response:

 - itemonhold: number of holds on this specific item.
 - recordonhold: number of holds on the record.

It allows an ILS-DI client to know if a loaned item is already on hold by someone else, and how many holds there are.

Test plan:
1. Apply the patch.
2. Enable the ILS-DI system preference.
3. Check out an item for a patron and make sure there no other holds at either an item or record level.
4. Check that the new itemonhold and recordonhold entries displayed are equal to zero (example: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=19&show_contact=0&show_loans=1).
5. Add either a record or item level hold for the record used in step 2.
6. Check that itemonhold and recordonhold values are incremented accordingly.
   Note: a hold at an item level counts as a hold at a record level, but not vice-versa.
7. Run the tests and make sure they pass: prove t/db_dependent/ILSDI_Services.t
8. Sign-off!
Comment 19 Matthias Meusburger 2023-10-24 08:42:49 UTC
Sorry about that! It should be ok now.
Comment 20 David Nind 2023-10-24 09:04:45 UTC
Created attachment 157728 [details] [review]
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

This patch adds two new entries in the loans section of GetPatronInfo response:

 - itemonhold: number of holds on this specific item.
 - recordonhold: number of holds on the record.

It allows an ILS-DI client to know if a loaned item is already on hold by someone else, and how many holds there are.

Test plan:
1. Apply the patch.
2. Enable the ILS-DI system preference.
3. Check out an item for a patron and make sure there no other holds at either an item or record level.
4. Check that the new itemonhold and recordonhold entries displayed are equal to zero (example: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=19&show_contact=0&show_loans=1).
5. Add either a record or item level hold for the record used in step 2.
6. Check that itemonhold and recordonhold values are incremented accordingly.
   Note: a hold at an item level counts as a hold at a record level, but not vice-versa.
7. Run the tests and make sure they pass: prove t/db_dependent/ILSDI_Services.t
8. Sign-off!

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 Kyle M Hall 2023-11-03 16:47:59 UTC
Created attachment 158351 [details] [review]
Bug 21284: ILS-DI: Allow GetPatronInfo to tell if a loaned item is on hold by someone else.

This patch adds two new entries in the loans section of GetPatronInfo response:

 - itemonhold: number of holds on this specific item.
 - recordonhold: number of holds on the record.

It allows an ILS-DI client to know if a loaned item is already on hold by someone else, and how many holds there are.

Test plan:
1. Apply the patch.
2. Enable the ILS-DI system preference.
3. Check out an item for a patron and make sure there no other holds at either an item or record level.
4. Check that the new itemonhold and recordonhold entries displayed are equal to zero (example: http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=19&show_contact=0&show_loans=1).
5. Add either a record or item level hold for the record used in step 2.
6. Check that itemonhold and recordonhold values are incremented accordingly.
   Note: a hold at an item level counts as a hold at a record level, but not vice-versa.
7. Run the tests and make sure they pass: prove t/db_dependent/ILSDI_Services.t
8. Sign-off!

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2023-11-03 16:48:05 UTC
Created attachment 158352 [details] [review]
Bug 21284: (QA follow-up) Rename itemonhold and recordonhold

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2023-11-03 16:48:08 UTC
Created attachment 158353 [details] [review]
Bug 21284: (QA follow-up) Fix QA script issues

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Tomás Cohen Arazi 2023-11-08 14:42:25 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 25 Fridolin Somers 2023-11-11 01:16:14 UTC
Small enhancement, I choose to backport.

Pushed to 23.05.x for 23.05.06
Comment 26 Fridolin Somers 2023-11-13 19:04:51 UTC
Created attachment 158899 [details] [review]
Bug 21284: [23.05] (follow-up) Fix unit test with unblessed patron
Comment 27 Fridolin Somers 2023-11-13 19:07:14 UTC
Unit test was failing :

#   Failed test 'No tests run for subtest "AuthenticatePatron test"'
#   at t/db_dependent/ILSDI_Services.t line 124.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'opacusercss' in 'field list' at /kohadevbox/koha/t/lib/TestBuilder.pm line 384

Fixed in 23.05.x with follow-up :
AddIssue() still uses unblessed patron object.