Bug 38505 - CirculateILL checkout broken if existing biblio's item does not have a barcode
Summary: CirculateILL checkout broken if existing biblio's item does not have a barcode
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Pedro Amorim
QA Contact: Testopia
URL:
Keywords:
Depends on: 37448
Blocks:
  Show dependency treegraph
 
Reported: 2024-11-21 13:33 UTC by Pedro Amorim
Modified: 2024-11-22 13:50 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 38505: CanBookBeIssued preparation: Add tests for falsy barcode (1.61 KB, patch)
2024-11-21 15:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38505: CanBookBeIssued preparation: Appropriate behavior when barcode is falsy (1.45 KB, patch)
2024-11-21 15:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38505: ill-requests.tt: Use .alert-warning when .alert exists (6.28 KB, patch)
2024-11-21 15:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38505: CirculateILL: Add UNKNOWN_BARCODE warning message (3.27 KB, patch)
2024-11-21 15:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38505: CirculateILL: Add ILL::Request::check_out regression tests (3.62 KB, patch)
2024-11-21 15:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38505: CirculateILL: Add ILL::Request::check_out regression tests (3.66 KB, patch)
2024-11-22 11:19 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 38505: CanBookBeIssued preparation: Add tests for falsy barcode (1.65 KB, patch)
2024-11-22 13:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38505: CanBookBeIssued preparation: Appropriate behavior when barcode is falsy (1.49 KB, patch)
2024-11-22 13:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38505: ill-requests.tt: Use .alert-warning when .alert exists (6.32 KB, patch)
2024-11-22 13:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38505: CirculateILL: Add UNKNOWN_BARCODE warning message (3.31 KB, patch)
2024-11-22 13:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38505: CirculateILL: Add ILL::Request::check_out regression tests (3.66 KB, patch)
2024-11-22 13:50 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-11-21 13:33:24 UTC
Setting this minor as the conditions to reproduce this are seemingly quite niche.
Comment 1 Pedro Amorim 2024-11-21 15:56:12 UTC
Created attachment 174892 [details] [review]
Bug 38505: CanBookBeIssued preparation: Add tests for falsy barcode

prove t/db_dependent/Circulation.t
Comment 2 Pedro Amorim 2024-11-21 15:56:15 UTC
Created attachment 174893 [details] [review]
Bug 38505: CanBookBeIssued preparation: Appropriate behavior when barcode is falsy

Before applying this patch, apply the CanBookBeIssued preparation: tests patch and run that,
Verify it fails, apply this patch. Run that same tests file again.

----- About this change -----

CanBookBeIssued wants to find one item given a barcode.
If a falsy (undef, empty) barcode is supplied, UNKNOWN_BARCODE should be returned.

Ensure this change does not introduce any regressions:
prove t/db_dependent/Circulation*
prove t/db_dependent/DecreaseLoanHighHolds.t
prove t/db_dependent/rollingloans.t
prove t/db_dependent/api/v1/checkouts.t
prove t/db_dependent/Patron/Borrower_PrevCheckout.t
Comment 3 Pedro Amorim 2024-11-21 15:56:18 UTC
Created attachment 174894 [details] [review]
Bug 38505: ill-requests.tt: Use .alert-warning when .alert exists

Cosmetic changes only.
Comment 4 Pedro Amorim 2024-11-21 15:56:20 UTC
Created attachment 174895 [details] [review]
Bug 38505: CirculateILL: Add UNKNOWN_BARCODE warning message

Test plan, before applying patches:
1) Enable ILLModule system preferece.
2) Create a book ILL request:
  <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard
3) After creating a Book ILL request, go to its detail page and click on the newly created linked biblio
4) On the biblio detail, click 'NEW' -> 'New item', add type and home library + current library (dont enter a barcode)
5) Repeat 2) -> Click confirm request and continue.
6) Click "Check out" -> If theres a problem with the patron (expired or max checkouts hit) it'll say "A problem with patron occurred". Fix that adn click c"check out" again.
7) Notice the message "An unknown error occurred while trying to checkout the item"
8) Apply patches. Repeat. Notice the message you now get is 'The bibliographic record's item contains an unknown (or empty) barcode.'.

This patch also adds a link to the respective biblio for Staff members'
convenience.
Comment 5 Pedro Amorim 2024-11-21 15:56:22 UTC
Created attachment 174896 [details] [review]
Bug 38505: CirculateILL: Add ILL::Request::check_out regression tests

prove t/db_dependent/Koha/ILL/Requests.t
Comment 6 Ray Delahunty 2024-11-22 06:44:43 UTC
I tried to sign this one off just now but I think I have to wait until the sandbox is at 24.11 (not long now) and so incorporates the fix in bug 35570. Having that fix should see ILL bugs signed off much quicker… really looking forward to being able to use sandboxes that have the standard backend already installed.
Comment 7 Katrin Fischer 2024-11-22 09:01:23 UTC
(In reply to Ray Delahunty from comment #6)
> I tried to sign this one off just now but I think I have to wait until the
> sandbox is at 24.11 (not long now) and so incorporates the fix in bug 35570.
> Having that fix should see ILL bugs signed off much quicker… really looking
> forward to being able to use sandboxes that have the standard backend
> already installed.

When creating a sandbox, it will always use the latest "main" (next version to be). So bug 35570 is already available on sandboxes.
Comment 8 PTFS Europe Sandboxes 2024-11-22 11:19:54 UTC
Created attachment 174924 [details] [review]
Bug 38505: CirculateILL: Add ILL::Request::check_out regression tests

prove t/db_dependent/Koha/ILL/Requests.t

Signed-off-by: R <r.delahunty@arts.ac.uk>
Comment 9 Ray Delahunty 2024-11-22 11:25:07 UTC
Thanks Katrin- a silly mistake on my part. I had enabled CirculateILL but hadn’t saved the further change to enable the ILL feature, so I wasn’t seeing the feature. Fixed that and followed the test plan, repeating from step 2 after applying the patch and was able to do the sign-off.
Comment 10 Katrin Fischer 2024-11-22 11:26:53 UTC
(In reply to Ray Delahunty from comment #9)
> Thanks Katrin- a silly mistake on my part. I had enabled CirculateILL but
> hadn’t saved the further change to enable the ILL feature, so I wasn’t
> seeing the feature. Fixed that and followed the test plan, repeating from
> step 2 after applying the patch and was able to do the sign-off.

Thanks!
Comment 11 Pedro Amorim 2024-11-22 13:08:14 UTC
Hi Ray, I've taken a second look at my test plan and it appears I forgot to specify that the CirculateILL sys pref needs to be enabled. Apologies.

If you're tested and are okay to sign-off, I'll add your sign-off the other patches and update the bug. For some reason it seems the sandbox sign-off only did it for the last patch, not all 5 of them.
Comment 12 Ray Delahunty 2024-11-22 13:36:46 UTC
Yes, my second try at this worked perfectly, so it would be great to have this set to signed off. I didn't repeat from far enough back when following the test plan: 'Apply patches. Repeat' but once I thought about the unexpected result I realised  I should have gone back to point 2 in the test plan and repeat from there.
Comment 13 Pedro Amorim 2024-11-22 13:50:34 UTC
Created attachment 174928 [details] [review]
Bug 38505: CanBookBeIssued preparation: Add tests for falsy barcode

prove t/db_dependent/Circulation.t

Signed-off-by: R <r.delahunty@arts.ac.uk>
Comment 14 Pedro Amorim 2024-11-22 13:50:36 UTC
Created attachment 174929 [details] [review]
Bug 38505: CanBookBeIssued preparation: Appropriate behavior when barcode is falsy

Before applying this patch, apply the CanBookBeIssued preparation: tests patch and run that,
Verify it fails, apply this patch. Run that same tests file again.

----- About this change -----

CanBookBeIssued wants to find one item given a barcode.
If a falsy (undef, empty) barcode is supplied, UNKNOWN_BARCODE should be returned.

Ensure this change does not introduce any regressions:
prove t/db_dependent/Circulation*
prove t/db_dependent/DecreaseLoanHighHolds.t
prove t/db_dependent/rollingloans.t
prove t/db_dependent/api/v1/checkouts.t
prove t/db_dependent/Patron/Borrower_PrevCheckout.t

Signed-off-by: R <r.delahunty@arts.ac.uk>
Comment 15 Pedro Amorim 2024-11-22 13:50:39 UTC
Created attachment 174930 [details] [review]
Bug 38505: ill-requests.tt: Use .alert-warning when .alert exists

Cosmetic changes only.

Signed-off-by: R <r.delahunty@arts.ac.uk>
Comment 16 Pedro Amorim 2024-11-22 13:50:42 UTC
Created attachment 174931 [details] [review]
Bug 38505: CirculateILL: Add UNKNOWN_BARCODE warning message

Test plan, before applying patches:
1) Enable ILLModule system preferece.
2) Create a book ILL request:
  <staff_url>/cgi-bin/koha/ill/ill-requests.pl?method=create&backend=Standard
3) After creating a Book ILL request, go to its detail page and click on the newly created linked biblio
4) On the biblio detail, click 'NEW' -> 'New item', add type and home library + current library (dont enter a barcode)
5) Repeat 2) -> Click confirm request and continue.
6) Click "Check out" -> If theres a problem with the patron (expired or max checkouts hit) it'll say "A problem with patron occurred". Fix that adn click c"check out" again.
7) Notice the message "An unknown error occurred while trying to checkout the item"
8) Apply patches. Repeat. Notice the message you now get is 'The bibliographic record's item contains an unknown (or empty) barcode.'.

This patch also adds a link to the respective biblio for Staff members'
convenience.

Signed-off-by: R <r.delahunty@arts.ac.uk>
Comment 17 Pedro Amorim 2024-11-22 13:50:45 UTC
Created attachment 174932 [details] [review]
Bug 38505: CirculateILL: Add ILL::Request::check_out regression tests

prove t/db_dependent/Koha/ILL/Requests.t

Signed-off-by: R <r.delahunty@arts.ac.uk>