Bug 38356 - CheckPrevCheckout should also check current loans
Summary: CheckPrevCheckout should also check current loans
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Emmi Takkinen
QA Contact: Lucas Gass (lukeg)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-11-05 13:04 UTC by Tuomas Kunttu
Modified: 2025-05-26 15:08 UTC (History)
7 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
If `AllowMultipleIssuesOnABiblio` is enabled patrons can checkout multiple items from same record but these check outs were not checked by Koha. This patch changes `CheckPrevCheckout` system preference functionality so that it also checks patrons current check outs and displays new confirmation message "Patron has this title currently checked out:..." so that librarians now have to confirm if item will be check out to patron.
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 38356: Check patrons current checkouts separately (9.68 KB, patch)
2025-04-02 07:35 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 38356: Check patrons current checkouts separately (9.73 KB, patch)
2025-04-02 11:57 UTC, Felicie
Details | Diff | Splinter Review
Bug 38356: Check patrons current checkouts separately (9.79 KB, patch)
2025-05-02 17:45 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38356: (QA follow-up) Fix spelling mistake (1.09 KB, patch)
2025-05-02 17:45 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
(follow-up) Bug 38356: Fix failing Circulation.t tests (1.64 KB, patch)
2025-05-08 08:45 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 38356: (follow-up) Fix failing Circulation.t tests (1.64 KB, patch)
2025-05-08 09:43 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 38356: (follow-up) Add code block to handle CURRENTISSUE message (1.36 KB, patch)
2025-05-08 09:43 UTC, Emmi Takkinen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tuomas Kunttu 2024-11-05 13:04:09 UTC
At this moment, with the system preference CheckPrevCheckout, Koha will look at the patron’s circulation history to see if they have checked this item out before. It would be helpful if Koha could also check the customer’s current loans and provide a notification if attempting to lend a book that the customer already has on loan.

Of course, this need wouldn’t exist if the AllowMultipleIssuesOnABiblio setting were set to 'don’t allow,' but there are other requirements that necessitate keeping it as 'allow'.
Comment 1 Emmi Takkinen 2025-04-02 07:35:55 UTC
Created attachment 180239 [details] [review]
Bug 38356: Check patrons current checkouts separately

If syspref CheckPrevCheckout is enabled, Koha checks if patron
has previously checked out item within timeperiod set in
syspref CheckPrevCheckoutDelay. This however doesn't take into
account cases where item is currently checked out for patron and
they are trying to check out another item from same record (can
be done if syspref AllowMultipleIssuesOnABiblio is enabled). We
should always check if item is currently checked out for the
patron if CheckPrevCheckout is enabled.

To test:
1. Enable syspref CheckPrevCheckout and set CheckPrevCheckoutDelay
as e.g. 10. Also enable syspref AllowMultipleIssuesOnABiblio.
2. Find a record that has been checked out for patron over 10 days ago.
3. Attempt to check out item from same record for same patron.
=> Note that you have to confirm if you want to check out item for
the patron.
4. Find record that is currently checked out for a patron.
5. Attempt to check out item from same record for same patron.
=> Note that item is checked out for patron without confirm message.
6. Apply this patch and restart services if needed.
7. Repeat steps 2. and 3.
=> Confirm that you still have to confirm if you want to check out
item for the patron.
8. Repeat steps 4. and 5.
=> New confirm message "Patron has this title currently checked out:..."
should be displayed and you have to confirm if you want to check out
item for the patron.

Also prove t/db_dependent/Patron/Borrower_PrevCheckout.t

Sponsored-by: Koha-Suomi Oy
Comment 2 Felicie 2025-04-02 11:57:19 UTC
Created attachment 180305 [details] [review]
Bug 38356: Check patrons current checkouts separately

If syspref CheckPrevCheckout is enabled, Koha checks if patron
has previously checked out item within timeperiod set in
syspref CheckPrevCheckoutDelay. This however doesn't take into
account cases where item is currently checked out for patron and
they are trying to check out another item from same record (can
be done if syspref AllowMultipleIssuesOnABiblio is enabled). We
should always check if item is currently checked out for the
patron if CheckPrevCheckout is enabled.

To test:
1. Enable syspref CheckPrevCheckout and set CheckPrevCheckoutDelay
as e.g. 10. Also enable syspref AllowMultipleIssuesOnABiblio.
2. Find a record that has been checked out for patron over 10 days ago.
3. Attempt to check out item from same record for same patron.
=> Note that you have to confirm if you want to check out item for
the patron.
4. Find record that is currently checked out for a patron.
5. Attempt to check out item from same record for same patron.
=> Note that item is checked out for patron without confirm message.
6. Apply this patch and restart services if needed.
7. Repeat steps 2. and 3.
=> Confirm that you still have to confirm if you want to check out
item for the patron.
8. Repeat steps 4. and 5.
=> New confirm message "Patron has this title currently checked out:..."
should be displayed and you have to confirm if you want to check out
item for the patron.

Also prove t/db_dependent/Patron/Borrower_PrevCheckout.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Felicie <felicie.thiery@biblibre.com>
Comment 3 Lucas Gass (lukeg) 2025-05-02 17:45:35 UTC
Created attachment 181883 [details] [review]
Bug 38356: Check patrons current checkouts separately

If syspref CheckPrevCheckout is enabled, Koha checks if patron
has previously checked out item within timeperiod set in
syspref CheckPrevCheckoutDelay. This however doesn't take into
account cases where item is currently checked out for patron and
they are trying to check out another item from same record (can
be done if syspref AllowMultipleIssuesOnABiblio is enabled). We
should always check if item is currently checked out for the
patron if CheckPrevCheckout is enabled.

To test:
1. Enable syspref CheckPrevCheckout and set CheckPrevCheckoutDelay
as e.g. 10. Also enable syspref AllowMultipleIssuesOnABiblio.
2. Find a record that has been checked out for patron over 10 days ago.
3. Attempt to check out item from same record for same patron.
=> Note that you have to confirm if you want to check out item for
the patron.
4. Find record that is currently checked out for a patron.
5. Attempt to check out item from same record for same patron.
=> Note that item is checked out for patron without confirm message.
6. Apply this patch and restart services if needed.
7. Repeat steps 2. and 3.
=> Confirm that you still have to confirm if you want to check out
item for the patron.
8. Repeat steps 4. and 5.
=> New confirm message "Patron has this title currently checked out:..."
should be displayed and you have to confirm if you want to check out
item for the patron.

Also prove t/db_dependent/Patron/Borrower_PrevCheckout.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Felicie <felicie.thiery@biblibre.com>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 4 Lucas Gass (lukeg) 2025-05-02 17:45:38 UTC
Created attachment 181884 [details] [review]
Bug 38356: (QA follow-up) Fix spelling mistake

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 5 Katrin Fischer 2025-05-06 16:53:54 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 6 Jonathan Druart 2025-05-07 12:14:11 UTC
git bisect blames this patch, it broke t/db_dependent/SIP/Message.t

t/db_dependent/SIP/Message.t .. 1/23                                                                                                                                                                         [15/727]
    #   Failed test 'Found AH field as timestamp in response'
    #   at t/db_dependent/SIP/Message.t line 1320.
                                                     
    #   Failed test 'Found AH field as SQL date in response'
    #   at t/db_dependent/SIP/Message.t line 1325.                                                        
    # Looks like you failed 2 tests of 8.                                                                 
t/db_dependent/SIP/Message.t .. 3/23 
#   Failed test 'Checkout V2'
#   at t/db_dependent/SIP/Message.t line 80.
                                                     
    #   Failed test 'Desensitize flag was set for patron category not in inhouse_patron_categories'
    #   at t/db_dependent/SIP/Message.t line 1717.
    #          got: 'N'  
    #     expected: 'Y'                                                                                   
                                                     
    #   Failed test 'Desensitize flag was set for empty inhouse_patron_categories'
    #   at t/db_dependent/SIP/Message.t line 1723.
    #          got: 'N'
    #     expected: 'Y'

    #   Failed test 'Desensitize flag was set for item type not in inhouse_item_types'
    #   at t/db_dependent/SIP/Message.t line 1737.
    #          got: 'N'
    #     expected: 'Y'

    #   Failed test 'Desensitize flag was set for empty inhouse_item_types'
    #   at t/db_dependent/SIP/Message.t line 1743.
    #          got: 'N'
    #     expected: 'Y'
    # Looks like you failed 4 tests of 6.
t/db_dependent/SIP/Message.t .. 4/23 
#   Failed test 'Test checkout desensitize'
#   at t/db_dependent/SIP/Message.t line 89.

    #   Failed test 'Desensitize flag was set for patron category not in inhouse_patron_categories'
    #   at t/db_dependent/SIP/Message.t line 1917.
    #          got: 'N'
    #     expected: 'Y'

    #   Failed test 'Desensitize flag was set for empty inhouse_patron_categories'
    #   at t/db_dependent/SIP/Message.t line 1923.
    #          got: 'N'
    #     expected: 'Y'

    #   Failed test 'Desensitize flag was set for item type not in inhouse_item_types'
    #   at t/db_dependent/SIP/Message.t line 1931.
    #          got: 'N'
    #     expected: 'Y'

    #   Failed test 'Desensitize flag was set for empty inhouse_item_types'
    #   at t/db_dependent/SIP/Message.t line 1937.
    #          got: 'N'
    #     expected: 'Y'
    # Looks like you failed 4 tests of 6.
t/db_dependent/SIP/Message.t .. 5/23 
#   Failed test 'Test renew desensitize'
#   at t/db_dependent/SIP/Message.t line 98.
t/db_dependent/SIP/Message.t .. 21/23 # Looks like you failed 3 tests of 23.
Comment 7 Jonathan Druart 2025-05-07 12:17:12 UTC
Also 
t/db_dependent/Circulation.t .. 68/76     # No tests run!
t/db_dependent/Circulation.t .. 70/76 
#   Failed test 'No tests run for subtest "CanBookBeIssued + needsconfirmation message"'
#   at t/db_dependent/Circulation.t line 7094.
DBIC result _type  isn't of the _type BiblioMetadata at /kohadevbox/koha/Koha/Patron.pm line 854.
# Looks like your test exited with 11 just after 71.
Comment 8 Katrin Fischer 2025-05-07 15:26:21 UTC
Emmi, can you provide a fix for the failing tests?
Comment 9 Emmi Takkinen 2025-05-08 08:26:27 UTC
(In reply to Jonathan Druart from comment #7)
> Also 
> t/db_dependent/Circulation.t .. 68/76     # No tests run!
> t/db_dependent/Circulation.t .. 70/76 
> #   Failed test 'No tests run for subtest "CanBookBeIssued +
> needsconfirmation message"'
> #   at t/db_dependent/Circulation.t line 7094.
> DBIC result _type  isn't of the _type BiblioMetadata at
> /kohadevbox/koha/Koha/Patron.pm line 854.
> # Looks like your test exited with 11 just after 71.

As far as I can tell t/db_dependent/Circulation.t tests are now failing randomly. They passed for me 3 times out of 5. But if I backtrack to the previous patch added to the main before these patches (bug 39775: Remove no-sort def), tests seem to pass every time. Also the line where they fail points to $biblio->is_serial which tries to check biblios metadata and is not related to the changes made here. Could it be possible that build_object sometimes fails to create metadata for the biblio object? If that's the case here it's weird that moving code around in the do_check_for_previous_checkout method has this kind of effect to building the test objects.
Comment 10 Katrin Fischer 2025-05-08 08:31:07 UTC
(In reply to Emmi Takkinen from comment #9)
> (In reply to Jonathan Druart from comment #7)
> > Also 
> > t/db_dependent/Circulation.t .. 68/76     # No tests run!
> > t/db_dependent/Circulation.t .. 70/76 
> > #   Failed test 'No tests run for subtest "CanBookBeIssued +
> > needsconfirmation message"'
> > #   at t/db_dependent/Circulation.t line 7094.
> > DBIC result _type  isn't of the _type BiblioMetadata at
> > /kohadevbox/koha/Koha/Patron.pm line 854.
> > # Looks like your test exited with 11 just after 71.
> 
> As far as I can tell t/db_dependent/Circulation.t tests are now failing
> randomly. They passed for me 3 times out of 5. But if I backtrack to the
> previous patch added to the main before these patches (bug 39775: Remove
> no-sort def), tests seem to pass every time. Also the line where they fail
> points to $biblio->is_serial which tries to check biblios metadata and is
> not related to the changes made here. Could it be possible that build_object
> sometimes fails to create metadata for the biblio object? If that's the case
> here it's weird that moving code around in the
> do_check_for_previous_checkout method has this kind of effect to building
> the test objects.

Hi Emmi, thanks! I wonder if is_serial breaks because of Bug 39835 - Tidy is_boolean / tinyint?
Comment 11 Emmi Takkinen 2025-05-08 08:38:12 UTC
(In reply to Katrin Fischer from comment #10)
> (In reply to Emmi Takkinen from comment #9)
> > (In reply to Jonathan Druart from comment #7)
> > > Also 
> > > t/db_dependent/Circulation.t .. 68/76     # No tests run!
> > > t/db_dependent/Circulation.t .. 70/76 
> > > #   Failed test 'No tests run for subtest "CanBookBeIssued +
> > > needsconfirmation message"'
> > > #   at t/db_dependent/Circulation.t line 7094.
> > > DBIC result _type  isn't of the _type BiblioMetadata at
> > > /kohadevbox/koha/Koha/Patron.pm line 854.
> > > # Looks like your test exited with 11 just after 71.
> > 
> > As far as I can tell t/db_dependent/Circulation.t tests are now failing
> > randomly. They passed for me 3 times out of 5. But if I backtrack to the
> > previous patch added to the main before these patches (bug 39775: Remove
> > no-sort def), tests seem to pass every time. Also the line where they fail
> > points to $biblio->is_serial which tries to check biblios metadata and is
> > not related to the changes made here. Could it be possible that build_object
> > sometimes fails to create metadata for the biblio object? If that's the case
> > here it's weird that moving code around in the
> > do_check_for_previous_checkout method has this kind of effect to building
> > the test objects.
> 
> Hi Emmi, thanks! I wonder if is_serial breaks because of Bug 39835 - Tidy
> is_boolean / tinyint?

My guess is that probably not. I changed the way test item is build in "CanBookBeIssued + needsconfirmation message" tests and now tests pass every time I run them. Will provide patch shortly so someone else can also confirm this.
Comment 12 Katrin Fischer 2025-05-08 08:40:08 UTC
Thanks! 

t/db_dependent/SIP/Message.t seems to fail "reliably" at least, so hopefully easier.
Comment 13 Emmi Takkinen 2025-05-08 08:45:09 UTC
Created attachment 182064 [details] [review]
(follow-up) Bug 38356: Fix failing Circulation.t tests

After pushing bug 38356 to main, Circulation.t tests
started to fail. Fail happened randomly and was related
to sub is_serial trying to fetch biblios metadata. This
patch changes the way test item is build in
"CanBookBeIssued + needsconfirmation message" tests. It
seems more reliable to use build_sample_item rather than
building item using biblio and biblioitem data.

To test prove t/db_dependent/Circulation.t.
Comment 14 Katrin Fischer 2025-05-08 09:00:52 UTC
Sorry, hope not bothering you: patch looks good to me. But I'd like to push together with the fixes for SIP if possible to save some Jenkins time. Are you working on these as well?
Comment 15 Emmi Takkinen 2025-05-08 09:05:44 UTC
(In reply to Katrin Fischer from comment #14)
> Sorry, hope not bothering you: patch looks good to me. But I'd like to push
> together with the fixes for SIP if possible to save some Jenkins time. Are
> you working on these as well?

Ah, forgot to say that I'm now checking those SIP tests. I'll add separate patch when I'm done.
Comment 16 Emmi Takkinen 2025-05-08 09:27:59 UTC
(In reply to Jonathan Druart from comment #6)
> git bisect blames this patch, it broke t/db_dependent/SIP/Message.t
> 
> t/db_dependent/SIP/Message.t .. 1/23                                        
> [15/727]
>     #   Failed test 'Found AH field as timestamp in response'
>     #   at t/db_dependent/SIP/Message.t line 1320.
>                                                      
>     #   Failed test 'Found AH field as SQL date in response'
>     #   at t/db_dependent/SIP/Message.t line 1325.                          
> 
>     # Looks like you failed 2 tests of 8.                                   
> 
> t/db_dependent/SIP/Message.t .. 3/23 
> #   Failed test 'Checkout V2'
> #   at t/db_dependent/SIP/Message.t line 80.
>                                                      
>     #   Failed test 'Desensitize flag was set for patron category not in
> inhouse_patron_categories'
>     #   at t/db_dependent/SIP/Message.t line 1717.
>     #          got: 'N'  
>     #     expected: 'Y'                                                     
> 
>                                                      
>     #   Failed test 'Desensitize flag was set for empty
> inhouse_patron_categories'
>     #   at t/db_dependent/SIP/Message.t line 1723.
>     #          got: 'N'
>     #     expected: 'Y'
> 
>     #   Failed test 'Desensitize flag was set for item type not in
> inhouse_item_types'
>     #   at t/db_dependent/SIP/Message.t line 1737.
>     #          got: 'N'
>     #     expected: 'Y'
> 
>     #   Failed test 'Desensitize flag was set for empty inhouse_item_types'
>     #   at t/db_dependent/SIP/Message.t line 1743.
>     #          got: 'N'
>     #     expected: 'Y'
>     # Looks like you failed 4 tests of 6.
> t/db_dependent/SIP/Message.t .. 4/23 
> #   Failed test 'Test checkout desensitize'
> #   at t/db_dependent/SIP/Message.t line 89.
> 
>     #   Failed test 'Desensitize flag was set for patron category not in
> inhouse_patron_categories'
>     #   at t/db_dependent/SIP/Message.t line 1917.
>     #          got: 'N'
>     #     expected: 'Y'
> 
>     #   Failed test 'Desensitize flag was set for empty
> inhouse_patron_categories'
>     #   at t/db_dependent/SIP/Message.t line 1923.
>     #          got: 'N'
>     #     expected: 'Y'
> 
>     #   Failed test 'Desensitize flag was set for item type not in
> inhouse_item_types'
>     #   at t/db_dependent/SIP/Message.t line 1931.
>     #          got: 'N'
>     #     expected: 'Y'
> 
>     #   Failed test 'Desensitize flag was set for empty inhouse_item_types'
>     #   at t/db_dependent/SIP/Message.t line 1937.
>     #          got: 'N'
>     #     expected: 'Y'
>     # Looks like you failed 4 tests of 6.
> t/db_dependent/SIP/Message.t .. 5/23 
> #   Failed test 'Test renew desensitize'
> #   at t/db_dependent/SIP/Message.t line 98.
> t/db_dependent/SIP/Message.t .. 21/23 # Looks like you failed 3 tests of 23.

Package C4::SIP::ILS::Transaction::Checkout is missing block to handle new "CURRENTISSUE" message and thus tests fail.
Comment 17 Emmi Takkinen 2025-05-08 09:43:03 UTC
Created attachment 182065 [details] [review]
Bug 38356: (follow-up) Fix failing Circulation.t tests

After pushing bug 38356 to main, Circulation.t tests
started to fail. Fail happened randomly and was related
to sub is_serial trying to fetch biblios metadata. This
patch changes the way test item is build in
"CanBookBeIssued + needsconfirmation message" tests. It
seems more reliable to use build_sample_item rather than
building item using biblio and biblioitem data.

To test prove t/db_dependent/Circulation.t.
Comment 18 Emmi Takkinen 2025-05-08 09:43:27 UTC
Created attachment 182066 [details] [review]
Bug 38356: (follow-up) Add code block to handle CURRENTISSUE message

Package C4::SIP::ILS::Transaction::Checkout was missing block to
handle new "CURRENTISSUE" message. This patch adds this block
so tests should no longer fail.

To test prove t/db_dependent/SIP/Message.t
Comment 19 Emmi Takkinen 2025-05-08 09:44:45 UTC
Both tests should now pass. Had to modify Circulation.t patches title so that QA tool is happy with it.
Comment 20 Katrin Fischer 2025-05-08 09:59:38 UTC
Hi Emmi, 

thanks a lot!

Please check you are running latest ktd and maybe "nuke" your node_modules directory once to avoid these:

 OK	C4/SIP/ILS/Transaction/Checkout.pm

 FAIL	t/db_dependent/Circulation.t
   FAIL	  tidiness
		File is not tidy, please run `perl misc/devel/tidy.pl t/db_dependent/Circulation.t`

I've tidied the patch before pushing, but it should happen automatically on commit with the hooks now.

Follow-ups pushed to main.
Comment 21 Emmi Takkinen 2025-05-08 10:04:40 UTC
(In reply to Katrin Fischer from comment #20)
> Hi Emmi, 
> 
> thanks a lot!
> 
> Please check you are running latest ktd and maybe "nuke" your node_modules
> directory once to avoid these:
> 
>  OK	C4/SIP/ILS/Transaction/Checkout.pm
> 
>  FAIL	t/db_dependent/Circulation.t
>    FAIL	  tidiness
> 		File is not tidy, please run `perl misc/devel/tidy.pl
> t/db_dependent/Circulation.t`
> 
> I've tidied the patch before pushing, but it should happen automatically on
> commit with the hooks now.
> 
> Follow-ups pushed to main.

Thanks for pointing this out. I'm not using ktd but my qa tool was indeed pointing out those messy lines. Thank you for fixing them!
Comment 22 Emmi Takkinen 2025-05-23 05:26:29 UTC
Added release notes.