|
Description
Matthias Meusburger
2018-08-29 13:34:31 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. 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. |