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.

Created attachment 78269 [details] [review]
Bug 21284: Unit tests

Hi Matts, please don't forget to assign the bug to yourself when providing a patch!

 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

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.

Patch rebased and use C4::Reserves removed.

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

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

Created attachment 109438 [details] [review]
Bug 21284: Unit tests

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.

/me only did a quick rebase on current master

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

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.

Surely now it can use Koha::Holds via item->holds and biblio->holds

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!

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.

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.

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!

Sorry about that! It should be ok now.

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>

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>

Created attachment 158352 [details] [review]
Bug 21284: (QA follow-up) Rename itemonhold and recordonhold

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 158353 [details] [review]
Bug 21284: (QA follow-up) Fix QA script issues

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Pushed to master for 23.11.

Nice work everyone, thanks!

Small enhancement, I choose to backport.

Pushed to 23.05.x for 23.05.06

Created attachment 158899 [details] [review]
Bug 21284: [23.05] (follow-up) Fix unit test with unblessed patron

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.

No changes required to the manual.