Bug 20644

Summary: Per itemtype setting for CheckPrevCheckout
Product: Koha Reporter: Magnus Enger <magnus>
Component: CirculationAssignee: Shi Yao Wang <shi-yao.wang>
Status: Pushed to main --- QA Contact: Lisette Scheer <lisette>
Severity: enhancement    
Priority: P5 - low CC: adolfo.rodriguez, andrew, caroline.cyr-la-rose, david, emmi.takkinen, gmcharlt, hebah, kyle, laurence.rault, lisette, lucas, nick, philippe.blouin, shi-yao.wang, william.lavoie
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function:
Bug Depends on: 40337    
Bug Blocks:    
Attachments: Bug 20644: Add checkprevcheckout on item types
Bug 20644: Add checkprevcheckout on item types
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: (QA follow-up) Remove duplicate variable
Bug 20644: (QA follow-up) Remove duplicate variable
Bug 20644: (QA follow-up) [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: (QA follow-up) Remove duplicate variable
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Change checkprevcheckout as ENUM at DB level
Bug 20644: Add checkprevcheckout on item types
Bug 20644: (QA follow-up) Remove duplicate variable
Bug 20644: Change checkprevcheckout as ENUM at DB level
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: Add checkprevcheckout on item types
Bug 20644: (QA follow-up) Remove duplicate variable
Bug 20644: Change checkprevcheckout as ENUM at DB level
Bug 20644: [DO NOT PUSH] DBIC Update
Bug 20644: (follow-up) Dont allow NULL value when the dropdown doesnt exist
Bug 20644: (follow-up) Fix atomicupdate
Bug 20644: Update swagger definitions for checkprevcheck
Bug 20644: (follow-up) Dont allow NULL value when the dropdown doesnt exist
Bug 20644: (follow-up) Fix atomicupdate
Bug 20644: Update swagger definitions for checkprevcheck
Bug 20644: (follow-up) Fix swagger description + put "inherit" by default

Description Magnus Enger 2018-04-23 20:44:33 UTC
Bug 6906 introduced the new CheckPrevCheckout syspref. The description says: 

"New feature: provide granular means to configure warnings about items
that have been issued to a particular borrower before, according to
their checkout history."

I assume "items" here is shorthand for "items connected to the same bibliographic record"? Just checking on the item level would not be all that useful, if there are more than one copy of e.g. a book. 

On the other hand, this does not work so well for periodicals/journals where individual issues are entered as separate items. If a patron borrows the last three issues of a journal, the second one will give a warning that the patron has previously borrowed this "title". 

Could it be possible to have a setting on the itemtypes to choose one of these two?
- Check if a patron has previously borrowed individual items (suitable for periodicals/journals)
- Check if a patron has previously borrowed any items connected to a bib record (suitable for books, DVDs etc)
Comment 1 Caroline Cyr La Rose 2023-04-13 16:16:51 UTC
Is this still valid after bug 16284?

The need is specifically for serials, and that is covered in bug 16284. Would a library want to control CheckPrevCheckout per itemtype for other types of materials?
Comment 2 hebah 2023-04-20 18:23:03 UTC
This granularity of CheckPrevCheckout by itemtype would still be useful functionality beyond serials - many libraries have unique things that they circulate that they would like to put a limitation on checkout within a timeframe in the interest of fairness of access - like laptops, hotspots, or local passes. Libraries may not want to turn on CheckPrevCheckout for a whole collection when they really only need it for certain subsets of items.
Comment 3 Emily-Rose Francoeur 2023-10-12 14:35:24 UTC Comment hidden (obsolete)
Comment 4 Emily-Rose Francoeur 2023-10-12 14:47:32 UTC Comment hidden (obsolete)
Comment 5 Laura Escamilla 2024-03-21 15:13:53 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens (kidclamp) 2024-04-23 13:46:28 UTC
Must fix:
In wants_check_for_previous_checkout we should pass the item _object rather than unblessed. This would let us use the effective_itemtype routine to get the itemtype (in the case where Record Level itemtypes might be in use)

The schema changes should be in their own patch to allow the RM to regenerate

In the tests it says new types are generated as NULL but it should say 'inherit'

QA tool complains about tidiness

Should fix:
Why does the item type choice say :try and override"? Looks like the item type setting trumps all except hard syspref, and it won't display if hard syspref set?

Could fix:
You can use testbuilder to generate the types for testing, there is no need to specify item type code, just check against what is generated.


Would fix:
Test could be a bit more verbose to be readable, the map is not as clear as a straightforward loop would be, but I like the laid out matrix explaining the results

The update could test for existence of column and not run when exists, but the construction you use does work.
Comment 7 Matthias Le Gac 2024-04-25 19:46:46 UTC Comment hidden (obsolete)
Comment 8 Matthias Le Gac 2024-04-25 19:46:48 UTC Comment hidden (obsolete)
Comment 9 Matthias Le Gac 2024-04-26 15:12:24 UTC
Created attachment 165661 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.
Comment 10 Matthias Le Gac 2024-04-26 15:12:26 UTC
Created attachment 165662 [details] [review]
Bug 20644: Add checkprevcheckout on item types
Comment 11 Matthias Le Gac 2024-04-26 15:13:27 UTC Comment hidden (obsolete)
Comment 12 Matthias Le Gac 2024-04-26 15:13:48 UTC
I changed "item_unblessed" to "item_object".
Put the shema changes in a different patch.
Adjusted the tidiness.

But for the point where in the tests it's marked NULL and not "inherit" I don't know what you're talking about, because for me it's already marked "inherit" as requested.

For the "should fix" point, could you be more precise? I don't know which part you're talking about. 
The same goes for the "would fix" part. Could you be more specific about how to improve test comprehension?

For the "could fix" part, I've tried to change using the builder, but with the builder you can't create an "ItemType" with the value "checkprevcheckout" at "undef" because this field can't be empty, so the builder throws an error.
I tried this:
my $itemType1 = $builder->build_object({ 
    class => 'Koha::ItemTypes',
    value => {
        checkprevcheckout => undef,
    }
});
and I get this error:
"Null value for checkprevcheckout in Itemtype not allowed at /inlibro/git/koha-master-dev-matthias/t/lib/TestBuilder.pm line 430."
Tell me if there is another solution with the builder or if we leave it as it is now?

Finally, for the last point about the script that adds the column, there's already something that tests whether the column exists, isn't there? with :
"ALTER TABLE itemtypes ADD IF NOT EXISTS checkprevcheckout"
Does "ADD IF NOT EXISTS" do this?
Comment 13 Nick Clemens (kidclamp) 2024-05-03 12:22:42 UTC
(In reply to Matthias Le Gac from comment #12)
> 
> But for the point where in the tests it's marked NULL and not "inherit" I
> don't know what you're talking about, because for me it's already marked
> "inherit" as requested.

t/db_dependent/Koha/ItemTypes.t:
180 'Two item types with \'checkprevcheckout\' set to \'undef\' have been added'
It's just the comment there



> 
> For the "should fix" point, could you be more precise? I don't know which
> part you're talking about. 

When editing an itemtype, it the system preference is 'soft' the options are:
Yes and try to override system preference
No and try to override system preference
Inherit from system preference

From what I understand it does not 'try' to override, it always does
I think we should also display the system preference when set to 'Hard' otherwise this can be a somewhat hidden feature

> The same goes for the "would fix" part. Could you be more specific about how
> to improve test comprehension?

For me, 4 nested 'map' calls is a bit obfuscated. Clearer would be something like:
foreach my $syspref_setting ('hardno','hardyes','softno','softyes'){
    foreach my $itemtype_setting( 'inherit','no','yes'){
    ...

The output from your tests is clear, the matrix in the comments is clear, if large, it's a matter of preference but not a blocker


> 
> For the "could fix" part, I've tried to change using the builder, but with
> the builder you can't create an "ItemType" with the value
> "checkprevcheckout" at "undef" because this field can't be empty, so the
> builder throws an error.
> I tried this:
> my $itemType1 = $builder->build_object({ 
>     class => 'Koha::ItemTypes',
>     value => {
>         checkprevcheckout => undef,
>     }
> });
> and I get this error:
> "Null value for checkprevcheckout in Itemtype not allowed at
> /inlibro/git/koha-master-dev-matthias/t/lib/TestBuilder.pm line 430."
> Tell me if there is another solution with the builder or if we leave it as
> it is now?

t/db_dependent/Patron/Borrower_PrevCheckout.t
49   itemtype          => 'YESIT',
59   itemtype          => 'NOIT',
69   itemtype          => 'INHERITIT'
I just meant you don't need to specify the codes here, you use them later, but could save the generated codes as variables in the hash and reference them - this just ensures the tests won't have a conflict with existing data - rare, but slightly possible if tests die and rollback fails, etc. Again, not a blocker

> Finally, for the last point about the script that adds the column, there's
> already something that tests whether the column exists, isn't there? with :
> "ALTER TABLE itemtypes ADD IF NOT EXISTS checkprevcheckout"
> Does "ADD IF NOT EXISTS" do this?

It does, but it is inconsistent with how we handle this in Koha:
if( !column_exists( 'borrowers', 'middle_name' ) ) {
I am not sure if your construction was blocked for being a MySQL-ism, but I on second glance it seems we should stick with the established pattern
git grep column_exists
git grep "ADD IF NOT EXISTS"
Comment 14 Hammat wele 2024-08-22 21:02:04 UTC Comment hidden (obsolete)
Comment 15 Hammat wele 2024-08-22 21:02:07 UTC Comment hidden (obsolete)
Comment 16 Hammat wele 2024-08-22 21:07:05 UTC
Created attachment 170621 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update
Comment 17 Hammat wele 2024-08-22 21:07:09 UTC
Created attachment 170622 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.
Comment 18 Hammat wele 2024-08-22 21:08:54 UTC
(In reply to Nick Clemens (kidclamp) from comment #13)
> 
> t/db_dependent/Koha/ItemTypes.t:
> 180 'Two item types with \'checkprevcheckout\' set to \'undef\' have been
> added'
> It's just the comment there
    ==> Fixed

> 
> When editing an itemtype, it the system preference is 'soft' the options are:
> Yes and try to override system preference
> No and try to override system preference
> Inherit from system preference
> 
> From what I understand it does not 'try' to override, it always does
> I think we should also display the system preference when set to 'Hard'
> otherwise this can be a somewhat hidden feature

    ==> I removed the try in the options 
     I don't think we should display the system preference when set to 'Hard' because when it's 'Hard' we can't get only two values ​​"1" when it's hardyes and "0" when hardno, regardless of the value of "Check for previous checkouts:"

> 
> For me, 4 nested 'map' calls is a bit obfuscated. Clearer would be something
> like:
> foreach my $syspref_setting ('hardno','hardyes','softno','softyes'){
>     foreach my $itemtype_setting( 'inherit','no','yes'){
>     ...
> 
    ==> Fixed

> 
> t/db_dependent/Patron/Borrower_PrevCheckout.t
> 49   itemtype          => 'YESIT',
> 59   itemtype          => 'NOIT',
> 69   itemtype          => 'INHERITIT'
> I just meant you don't need to specify the codes here, you use them later,
> but could save the generated codes as variables in the hash and reference

    ==> Fixed we don't specify itemtype no more when creating a itemtype

> It does, but it is inconsistent with how we handle this in Koha:
> if( !column_exists( 'borrowers', 'middle_name' ) ) {

    ==> I updated the script by adding the condition to not add the column if it exists
Comment 19 hebah 2024-12-11 20:50:47 UTC
Patch doesn't apply:
Patch failed at 0001 Bug 20644: [DO NOT PUSH] DBIC Update
[33mhint: Use 'git am --show-current-patch=diff' to see the failed patch[m
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-20644-DO-NOT-PUSH-DBIC-Update-khukogbt.patch
Comment 20 Lucas Gass (lukeg) 2024-12-11 20:58:00 UTC
It looks like a pretty simple rebase, just need to deal with bookings. Can the database updates be done in a separate commit when this is rebased?
Comment 21 William Lavoie 2025-01-14 20:35:39 UTC
Created attachment 176538 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update
Comment 22 William Lavoie 2025-01-14 20:35:41 UTC
Created attachment 176539 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.
Comment 23 William Lavoie 2025-01-14 20:36:05 UTC
Rebased
Comment 24 William Lavoie 2025-02-14 14:00:09 UTC
Created attachment 178093 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update
Comment 25 William Lavoie 2025-02-14 14:14:40 UTC
Created attachment 178094 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update
Comment 26 Shi Yao Wang 2025-03-27 20:27:34 UTC
Created attachment 179812 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update
Comment 27 Shi Yao Wang 2025-03-27 20:27:38 UTC
Created attachment 179813 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.
Comment 28 Shi Yao Wang 2025-03-27 20:31:54 UTC
Created attachment 179814 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update
Comment 29 Shi Yao Wang 2025-03-27 20:31:57 UTC
Created attachment 179815 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.
Comment 30 Shi Yao Wang 2025-03-27 20:32:30 UTC
rebased
Comment 31 Biblibre Sandboxes 2025-04-02 13:46:48 UTC
Created attachment 180375 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.

Signed-off-by: Laurence <laurence.rault@biblibre.com>
Comment 32 Laurence Rault 2025-04-02 13:48:31 UTC
Functionality is working as expected
Test plan 18) Verify that all tests pass successfully : could not be tested on sandbox
Comment 33 Emmi Takkinen 2025-04-04 10:33:29 UTC
Tested this and in staff interface everything works as intended. However tests to not pass, they throw following errors:

t/db_dependent/Patron/Borrower_PrevCheckout.t .. 152/170
#   Failed test 'PostIssue: Same item, same patron [1]'
#   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 385.
#          got: '0'
#     expected: '1'

#   Failed test 'PostIssue: Diff item, same bib, same patron [1]'
#   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 385.
#          got: '0'
#     expected: '1'

#   Failed test 'patron, wants_check_for_previous_checkout, do_check_for_previous_checkout'
#   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 555.
#          got: undef
#     expected: '1'

    #   Failed test 'Check only one item if bibliographic record is serial'
    #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 582.
    #          got: '0'
    #     expected: '1'
    # Looks like you failed 1 test of 2.

#   Failed test 'Check previous checkouts for serial'
#   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 592.

#   Failed test 'no warnings'
#   at /usr/share/perl/5.34/Test/Builder.pm line 193.
# There were 73 warning(s)
.....

And:

t/db_dependent/Koha/ItemTypes.t .. 1/19 
#   Failed test 'item types should be sorted by translated description'
#   at t/db_dependent/Koha/ItemTypes.t line 115.
#          got: '14 vrk av'
#     expected: 'a translated itemtype desc'

#   Failed test 'no warnings'
#   at /usr/share/perl/5.34/Test/Builder.pm line 193.
# There were 9 warning(s)
Comment 34 Shi Yao Wang 2025-04-04 13:00:12 UTC
(In reply to Emmi Takkinen from comment #33)
> Tested this and in staff interface everything works as intended. However
> tests to not pass, they throw following errors:
> 
> t/db_dependent/Patron/Borrower_PrevCheckout.t .. 152/170
> #   Failed test 'PostIssue: Same item, same patron [1]'
> #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 385.
> #          got: '0'
> #     expected: '1'
> 
> #   Failed test 'PostIssue: Diff item, same bib, same patron [1]'
> #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 385.
> #          got: '0'
> #     expected: '1'
> 
> #   Failed test 'patron, wants_check_for_previous_checkout,
> do_check_for_previous_checkout'
> #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 555.
> #          got: undef
> #     expected: '1'
> 
>     #   Failed test 'Check only one item if bibliographic record is serial'
>     #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 582.
>     #          got: '0'
>     #     expected: '1'
>     # Looks like you failed 1 test of 2.
> 
> #   Failed test 'Check previous checkouts for serial'
> #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 592.
> 
> #   Failed test 'no warnings'
> #   at /usr/share/perl/5.34/Test/Builder.pm line 193.
> # There were 73 warning(s)
> .....
> 
> And:
> 
> t/db_dependent/Koha/ItemTypes.t .. 1/19 
> #   Failed test 'item types should be sorted by translated description'
> #   at t/db_dependent/Koha/ItemTypes.t line 115.
> #          got: '14 vrk av'
> #     expected: 'a translated itemtype desc'
> 
> #   Failed test 'no warnings'
> #   at /usr/share/perl/5.34/Test/Builder.pm line 193.
> # There were 9 warning(s)

Strange. I just ran these 2 tests and they pass with no warnings.
Can we get a third input on this?

Putting back to Needs Signoff.
Comment 35 Emmi Takkinen 2025-04-07 05:48:54 UTC
(In reply to Shi Yao Wang from comment #34)
> (In reply to Emmi Takkinen from comment #33)
> > Tested this and in staff interface everything works as intended. However
> > tests to not pass, they throw following errors:
> > 
> > t/db_dependent/Patron/Borrower_PrevCheckout.t .. 152/170
> > #   Failed test 'PostIssue: Same item, same patron [1]'
> > #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 385.
> > #          got: '0'
> > #     expected: '1'
> > 
> > #   Failed test 'PostIssue: Diff item, same bib, same patron [1]'
> > #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 385.
> > #          got: '0'
> > #     expected: '1'
> > 
> > #   Failed test 'patron, wants_check_for_previous_checkout,
> > do_check_for_previous_checkout'
> > #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 555.
> > #          got: undef
> > #     expected: '1'
> > 
> >     #   Failed test 'Check only one item if bibliographic record is serial'
> >     #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 582.
> >     #          got: '0'
> >     #     expected: '1'
> >     # Looks like you failed 1 test of 2.
> > 
> > #   Failed test 'Check previous checkouts for serial'
> > #   at t/db_dependent/Patron/Borrower_PrevCheckout.t line 592.
> > 
> > #   Failed test 'no warnings'
> > #   at /usr/share/perl/5.34/Test/Builder.pm line 193.
> > # There were 73 warning(s)
> > .....
> > 
> > And:
> > 
> > t/db_dependent/Koha/ItemTypes.t .. 1/19 
> > #   Failed test 'item types should be sorted by translated description'
> > #   at t/db_dependent/Koha/ItemTypes.t line 115.
> > #          got: '14 vrk av'
> > #     expected: 'a translated itemtype desc'
> > 
> > #   Failed test 'no warnings'
> > #   at /usr/share/perl/5.34/Test/Builder.pm line 193.
> > # There were 9 warning(s)
> 
> Strange. I just ran these 2 tests and they pass with no warnings.
> Can we get a third input on this?
> 
> Putting back to Needs Signoff.

Tested again, tests pass on my clean community database but not in my (much messier) production database. And this happens even when this patch isn't applied, so comes from somewhere else in the tests.
Comment 36 Shi Yao Wang 2025-04-29 17:35:09 UTC
Created attachment 181680 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update
Comment 37 Shi Yao Wang 2025-04-29 17:35:12 UTC
Created attachment 181681 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.

Signed-off-by: Laurence <laurence.rault@biblibre.com>
Comment 38 Shi Yao Wang 2025-04-29 17:35:39 UTC
Rebased
Comment 39 Shi Yao Wang 2025-05-14 20:21:07 UTC
Created attachment 182469 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update
Comment 40 Shi Yao Wang 2025-05-14 20:21:12 UTC
Created attachment 182470 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.

Signed-off-by: Laurence <laurence.rault@biblibre.com>
Comment 41 Emmi Takkinen 2025-05-15 08:33:37 UTC
Created attachment 182476 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 42 Emmi Takkinen 2025-05-15 08:34:04 UTC
Created attachment 182477 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.

Signed-off-by: Laurence <laurence.rault@biblibre.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 43 Shi Yao Wang 2025-05-26 19:54:17 UTC
Created attachment 182787 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 44 Shi Yao Wang 2025-05-26 19:54:21 UTC
Created attachment 182788 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.

Signed-off-by: Laurence <laurence.rault@biblibre.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 45 Shi Yao Wang 2025-05-26 19:54:37 UTC
rebase
Comment 46 Lisette Scheer 2025-06-20 20:31:32 UTC
Created attachment 183403 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 47 Lisette Scheer 2025-06-20 20:32:07 UTC
Created attachment 183404 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.

Signed-off-by: Laurence <laurence.rault@biblibre.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 48 Lisette Scheer 2025-06-20 20:32:36 UTC
Created attachment 183405 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 49 Lisette Scheer 2025-06-20 20:33:03 UTC
Created attachment 183406 [details] [review]
Bug 20644: (QA follow-up) Remove duplicate variable

I removed a duplicate variable and tidied.
Comment 50 Lisette Scheer 2025-06-20 20:33:35 UTC
Created attachment 183407 [details] [review]
Bug 20644: (QA follow-up) Remove duplicate variable

I removed a duplicate variable and tidied.
Comment 51 Lisette Scheer 2025-06-20 20:34:26 UTC
Created attachment 183408 [details] [review]
Bug 20644: (QA follow-up) [DO NOT PUSH] DBIC Update

Tidied the DBIC update
Comment 52 Lucas Gass (lukeg) 2025-06-24 22:03:43 UTC
A couple problems:

-The database update is not tidy
-Tests are now failing, can you have a look?
Comment 53 Shi Yao Wang 2025-08-12 17:27:26 UTC
atomic update file seems tidy and tests seems to pass for me.

could someone else check?
Comment 54 Lisette Scheer 2025-08-12 19:27:36 UTC
I'm not seeing un-tidyness in the current db update.

Tests failed for me as well:
Test Summary Report
-------------------
 /kohadevbox/koha/t/db_dependent/Koha/ItemTypes.t (Wstat: 2816 (exited 11) Tests: 16 Failed: 0)
  Non-zero exit status: 11
   Parse errors: Bad plan.  You planned 19 tests but ran 16.
 Files=1, Tests=16,  1 wallclock secs ( 0.02 usr  0.00 sys +  1.08 cusr  0.12 csys =  1.22 CPU)
 Result: FAIL

        * Proving /kohadevbox/koha/t/db_dependent/Patron/Borrower_PrevCheckout.t FAIL
Error: value hash contains unrecognized columns: checkprevcheckout at /kohadevbox/koha/t/lib/TestBuilder.pm line 311.
 # Looks like your test exited with 11 just after 2.
 /kohadevbox/koha/t/db_dependent/Patron/Borrower_PrevCheckout.t .. 
Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 170/172 subtests 
 
Test Summary Report
-------------------
 /kohadevbox/koha/t/db_dependent/Patron/Borrower_PrevCheckout.t (Wstat: 2816 (exited 11) Tests: 2 Failed: 0)
  Non-zero exit status: 11
   Parse errors: Bad plan.  You planned 172 tests but ran 2.
Files=1, Tests=2,  1 wallclock secs ( 0.01 usr  0.00 sys +  0.91 cusr  0.08 csys =  1.00 CPU)
 Result: FAIL
Comment 55 Shi Yao Wang 2025-08-12 21:03:58 UTC
(In reply to Lisette Scheer from comment #54)
> I'm not seeing un-tidyness in the current db update.
> 
> Tests failed for me as well:
> Test Summary Report
> -------------------
>  /kohadevbox/koha/t/db_dependent/Koha/ItemTypes.t (Wstat: 2816 (exited 11)
> Tests: 16 Failed: 0)
>   Non-zero exit status: 11
>    Parse errors: Bad plan.  You planned 19 tests but ran 16.
>  Files=1, Tests=16,  1 wallclock secs ( 0.02 usr  0.00 sys +  1.08 cusr 
> 0.12 csys =  1.22 CPU)
>  Result: FAIL
> 
>         * Proving
> /kohadevbox/koha/t/db_dependent/Patron/Borrower_PrevCheckout.t FAIL
> Error: value hash contains unrecognized columns: checkprevcheckout at
> /kohadevbox/koha/t/lib/TestBuilder.pm line 311.
>  # Looks like your test exited with 11 just after 2.
>  /kohadevbox/koha/t/db_dependent/Patron/Borrower_PrevCheckout.t .. 
> Dubious, test returned 11 (wstat 2816, 0xb00)
> Failed 170/172 subtests 
>  
> Test Summary Report
> -------------------
>  /kohadevbox/koha/t/db_dependent/Patron/Borrower_PrevCheckout.t (Wstat: 2816
> (exited 11) Tests: 2 Failed: 0)
>   Non-zero exit status: 11
>    Parse errors: Bad plan.  You planned 172 tests but ran 2.
> Files=1, Tests=2,  1 wallclock secs ( 0.01 usr  0.00 sys +  0.91 cusr  0.08
> csys =  1.00 CPU)
>  Result: FAIL

Here is what I get:
>Test Summary Report
>-------------------
>t/00-check-atomic-updates.t               (Wstat: 256 (exited 1) Tests: 2 >Failed: 1)
>  Failed test:  1
>  Non-zero exit status: 1
>t/00-deprecated.t                         (Wstat: 256 (exited 1) Tests: 2 >Failed: 1)
>  Failed test:  2
>  Non-zero exit status: 1
>t/Creators.t                              (Wstat: 512 (exited 2) Tests: 42 >Failed: 2)
>  Failed tests:  12, 14
>  Non-zero exit status: 2
>Files=76, Tests=2220, 37 wallclock secs ( 0.27 usr  0.08 sys + 29.19 cusr  3.41 >csys = 32.95 CPU)
>Result: FAIL

the tests that fail except "t/00-check-atomic-updates.t" were already failing before the patches
I'm not using kohadevbox so maybe that's why?
Comment 56 Lisette Scheer 2025-08-12 21:14:59 UTC
> the tests that fail except "t/00-check-atomic-updates.t" were already
> failing before the patches
> I'm not using kohadevbox so maybe that's why?

All the tests are successful for me on main:

kohadev-koha@kohadevbox:koha(main)$ prove t/00-check-atomic-updates.t 
t/00-check-atomic-updates.t .. ok   
All tests successful.
Files=1, Tests=2,  0 wallclock secs ( 0.01 usr  0.01 sys +  0.03 cusr  0.01 csys =  0.06 CPU)
Result: PASS
kohadev-koha@kohadevbox:koha(main)$ prove t/00-deprecated.t 
t/00-deprecated.t .. ok   
All tests successful.
Files=1, Tests=2,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.15 cusr  0.16 csys =  0.33 CPU)
Result: PASS
kohadev-koha@kohadevbox:koha(main)$ prove t/Creators.t 
t/Creators.t .. ok     
All tests successful.
Files=1, Tests=42,  1 wallclock secs ( 0.01 usr  0.01 sys +  0.57 cusr  0.06 csys =  0.65 CPU)
Result: PASS
kohadev-koha@kohadevbox:koha(main)$ prove t/db_dependent/Patron/Borrower_PrevCheckout.t 
t/db_dependent/Patron/Borrower_PrevCheckout.t .. ok     
All tests successful.
Files=1, Tests=64,  5 wallclock secs ( 0.01 usr  0.01 sys +  3.59 cusr  0.70 csys =  4.31 CPU)
Result: PASS
kohadev-koha@kohadevbox:koha(main)$ prove t/db_dependent/Koha/ItemTypes.t 
t/db_dependent/Koha/ItemTypes.t .. ok     
All tests successful.
Files=1, Tests=16,  2 wallclock secs ( 0.01 usr  0.01 sys +  1.17 cusr  0.12 csys =  1.31 CPU)
Result: PASS
Comment 57 Lucas Gass (lukeg) 2025-08-12 21:27:11 UTC
Please see Bug 40337 - checkprevcheckout must be defined as ENUM at DB level

Also, your changes in Koha/Patrons.pm need tests to cover those changes.
Comment 58 Shi Yao Wang 2025-08-15 13:22:28 UTC
Created attachment 185448 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.

Signed-off-by: Laurence <laurence.rault@biblibre.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 59 Shi Yao Wang 2025-08-15 13:22:33 UTC
Created attachment 185449 [details] [review]
Bug 20644: (QA follow-up) Remove duplicate variable

I removed a duplicate variable and tidied.
Comment 60 Shi Yao Wang 2025-08-15 13:22:37 UTC
Created attachment 185450 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update

Tidied
Comment 61 Shi Yao Wang 2025-08-15 13:22:41 UTC
Created attachment 185451 [details] [review]
Bug 20644: Change checkprevcheckout as ENUM at DB level
Comment 62 Shi Yao Wang 2025-08-15 13:27:52 UTC
(In reply to Lucas Gass (lukeg) from comment #57)
> Please see Bug 40337 - checkprevcheckout must be defined as ENUM at DB level
> 
> Also, your changes in Koha/Patrons.pm need tests to cover those changes.

checkprevcheckout is now an ENUM at DB level

There are no changes in Koha/Patrons.pm. Maybe you meant Koha/Patron.pm?
Those changes are tested in t/db_dependent/Patron/Borrower_PrevCheckout.t
Comment 63 Shi Yao Wang 2025-08-15 13:34:48 UTC
I see the aforementioned tests are failing. Looking into it
Comment 64 Shi Yao Wang 2025-08-15 20:36:38 UTC
Created attachment 185478 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.

Signed-off-by: Laurence <laurence.rault@biblibre.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 65 Shi Yao Wang 2025-08-15 20:36:41 UTC
Created attachment 185479 [details] [review]
Bug 20644: (QA follow-up) Remove duplicate variable

I removed a duplicate variable and tidied.
Comment 66 Shi Yao Wang 2025-08-15 20:36:45 UTC
Created attachment 185480 [details] [review]
Bug 20644: Change checkprevcheckout as ENUM at DB level
Comment 67 Shi Yao Wang 2025-08-15 20:36:48 UTC
Created attachment 185481 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update
Comment 68 Shi Yao Wang 2025-08-15 20:40:53 UTC
For some reason the previous `Bug 20644: [DO NOT PUSH] DBIC Update` was just a tidy on the atomic update file? so I squashed it into the previous commit `Bug 20644: (QA follow-up) Remove duplicate variable` to make way for an actual DBIC Update that was the missing piece to fixing the tests.
Comment 69 Lisette Scheer 2025-08-19 17:35:34 UTC
Created attachment 185557 [details] [review]
Bug 20644: Add checkprevcheckout on item types

I have added the "Check for previous checkouts" option to item types. This option is visible only when the system preference "CheckPrevCheckout" is set to either :
- "Unless overridden by patron category or by item type, do"
or
- "Unless overridden by patron category or by item type, do not."

This option takes precedence over patron and patron category preferences.

TEST PLAN
1) Apply the patch.
2) Perform an atomic update (installer/data/mysql/updatedatabase.pl)
3) Set the "CheckPrevCheckout" system preference to "Unless overridden by patron category or by item type, do".
4) Go to "Koha administration > Item types > + New item type".
5) Fill in the necessary fields; the "Check for previous checkouts" field should be set to "Inherit from system preferences".
6) Click the "Save changes" button.
7) Create an item with the type you have created.
8) Check out this item for a patron, then check it in.
9) Check out the same item for the same patron again.
10) The message "Patron has previously checked out this title: <Title>. Check out anyway?" should appear.
11) Go to "Koha administration > Item types".
12) Edit the created item type.
13) Change the value of the "Check for previous checkouts" field to "No and try to override system preferences".
14) Click the "Save changes" button.
15) Check out the same item for the same patron.
16) The message should not appear.
17) Run the tests "Borrower_PrevCheckout.t" (prove t/db_dependent/Patron/Borrower_PrevCheckout.t) and "ItemTypes.t" (prove t/db_dependent/Koha/ItemTypes.t).
18) Verify that all tests pass successfully.

Signed-off-by: Laurence <laurence.rault@biblibre.com>
Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 70 Lisette Scheer 2025-08-19 17:35:38 UTC
Created attachment 185558 [details] [review]
Bug 20644: (QA follow-up) Remove duplicate variable

I removed a duplicate variable and tidied.

Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 71 Lisette Scheer 2025-08-19 17:35:42 UTC
Created attachment 185559 [details] [review]
Bug 20644: Change checkprevcheckout as ENUM at DB level

Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 72 Lisette Scheer 2025-08-19 17:35:45 UTC
Created attachment 185560 [details] [review]
Bug 20644: [DO NOT PUSH] DBIC Update

Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 73 Lucas Gass (lukeg) 2025-08-20 19:45:31 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 74 Lucas Gass (lukeg) 2025-08-20 22:09:52 UTC
This is now causing t/db_dependent/api/v1/item_types.t to fail. Please follow-up.
Comment 75 Caroline Cyr La Rose 2025-08-21 16:03:04 UTC
I think this causes a problem when editing an item type. I was trying to make an existing itemtype "bookable" and got the error message "An error occurred when updating this item type. Perhaps the value already exists.". When I looked into plack-intranet-error.log, it said "[2025/08/21 15:48:59] [WARN] DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Column 'checkprevcheckout' cannot be null at /kohadevbox/koha/Koha/O    bject.pm line 174"

If CheckPrevCheckout is set to one of the 'Unless overridden by...' option, I can edit and save my itemtype. If CheckPrevCheckout is set to 'Do' or 'Do not', I can't edit an itemtype.
Comment 76 Lucas Gass (lukeg) 2025-08-21 17:31:12 UTC
(In reply to Caroline Cyr La Rose from comment #75)
> I think this causes a problem when editing an item type. I was trying to
> make an existing itemtype "bookable" and got the error message "An error
> occurred when updating this item type. Perhaps the value already exists.".
> When I looked into plack-intranet-error.log, it said "[2025/08/21 15:48:59]
> [WARN] DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception:
> DBD::mysql::st execute failed: Column 'checkprevcheckout' cannot be null at
> /kohadevbox/koha/Koha/O    bject.pm line 174"
> 
> If CheckPrevCheckout is set to one of the 'Unless overridden by...' option,
> I can edit and save my itemtype. If CheckPrevCheckout is set to 'Do' or 'Do
> not', I can't edit an itemtype.

I'll need a fix for this, and the tests I mention, before Monday. Or I will need to revert this, sorry.
Comment 77 Lucas Gass (lukeg) 2025-08-21 20:27:30 UTC
(In reply to Caroline Cyr La Rose from comment #75)
> I think this causes a problem when editing an item type. I was trying to
> make an existing itemtype "bookable" and got the error message "An error
> occurred when updating this item type. Perhaps the value already exists.".
> When I looked into plack-intranet-error.log, it said "[2025/08/21 15:48:59]
> [WARN] DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception:
> DBD::mysql::st execute failed: Column 'checkprevcheckout' cannot be null at
> /kohadevbox/koha/Koha/O    bject.pm line 174"
> 
> If CheckPrevCheckout is set to one of the 'Unless overridden by...' option,
> I can edit and save my itemtype. If CheckPrevCheckout is set to 'Do' or 'Do
> not', I can't edit an itemtype.

This is because when CheckPrevCheckout is softyes of softno the dropdown appears. When there is no dropdown it tries to save a NULL value which isn't allowed.
Comment 78 Shi Yao Wang 2025-08-21 20:30:19 UTC
I got the code fix for Caroline's issue. I am trying to fix the tests as well to push both solutions in the same commit
Comment 79 Lucas Gass (lukeg) 2025-08-21 20:38:02 UTC
Created attachment 185665 [details] [review]
Bug 20644: (follow-up) Dont allow NULL value when the dropdown doesnt exist
Comment 80 Lucas Gass (lukeg) 2025-08-21 20:38:53 UTC
We also need to fix the atomicupdate, ADD IF NOT EXISTS isn't right.
Comment 81 Lucas Gass (lukeg) 2025-08-21 20:40:39 UTC
Created attachment 185666 [details] [review]
Bug 20644: (follow-up) Fix atomicupdate
Comment 82 Lucas Gass (lukeg) 2025-08-21 20:54:16 UTC
Created attachment 185669 [details] [review]
Bug 20644: Update swagger definitions for checkprevcheck
Comment 83 Lucas Gass (lukeg) 2025-08-21 20:56:28 UTC
I think these 3 follow-ups fix all the problems here. Can I get a sanity check please?
Comment 84 Shi Yao Wang 2025-08-21 21:08:57 UTC
(In reply to Lucas Gass (lukeg) from comment #79)
> Created attachment 185665 [details] [review] [review]
> Bug 20644: (follow-up) Dont allow NULL value when the dropdown doesnt exist

Would it be better if we assigned "inherit" by default at line 102?


(In reply to Lucas Gass (lukeg) from comment #82)
> Created attachment 185669 [details] [review] [review]
> Bug 20644: Update swagger definitions for checkprevcheck

t/db_dependent/api/v1/item_types.t tests are still failing.
and it would be more accurate to say:

> description: Produce a warning for this item type if this item has previously 
> been checked out to this patron if ''yes'', not if ''no'', defer to patron 
> setting if ''inherit''.'

instead of

> description: Produce a warning for this item type if this item has previously 
> been checked out to this patron if ''yes'', not if ''no'', defer to syspref 
> setting if ''inherit''.'
Comment 85 David Nind 2025-08-21 21:14:12 UTC
Created attachment 185670 [details] [review]
Bug 20644: (follow-up) Dont allow NULL value when the dropdown doesnt exist

Signed-off-by: David Nind <david@davidnind.com>
Comment 86 David Nind 2025-08-21 21:14:17 UTC
Created attachment 185671 [details] [review]
Bug 20644: (follow-up) Fix atomicupdate

Signed-off-by: David Nind <david@davidnind.com>
Comment 87 David Nind 2025-08-21 21:14:22 UTC
Created attachment 185672 [details] [review]
Bug 20644: Update swagger definitions for checkprevcheck

Signed-off-by: David Nind <david@davidnind.com>
Comment 88 David Nind 2025-08-21 21:17:35 UTC
I added my signoffs to the three follow-ups (using KTD):

1. Applied follow-up patches (using interactive mode)
2. Updated the database (no messages output): updatedatabase
3. Updated things: yarn build
4. Restarted everything: restart_all
5. Tested this things:
   5.1 Making an item type bookable
   5.2 Repeat 5.1 with all the options for the CheckPrevCheckout system preference
Comment 89 David Nind 2025-08-21 21:27:41 UTC
These tests all pass for me:

prove t/db_dependent/api/v1/item_types.t
prove t/db_dependent/Koha/ItemTypes.t
prove t/db_dependent/Patron/Borrower_PrevCheckout.t
Comment 90 Shi Yao Wang 2025-08-21 21:33:19 UTC
Nevermind about the tests not passing for me. I forgot to run `yarn api:bundle`. Now it works

I will add a follow-up patch to correct the description in swagger definition and assign "inherit" by default
Comment 91 Shi Yao Wang 2025-08-21 21:40:58 UTC
Created attachment 185673 [details] [review]
Bug 20644: (follow-up) Fix swagger description + put "inherit" by default
Comment 92 Adolfo Rodríguez Taboada 2025-08-22 10:49:00 UTC
Is the IF NOT EXISTS really necessary in the file installer/data/mysql/db_revs/250600010.pl? 

It's already being checked before applying the SQL with !column_exists( 'itemtypes', 'checkprevcheckout' ) and it returns an error with versions of MySQL such as Ver 15.1 Distrib 10.1.48-MariaDB and Ver 8.0.31


Error message:

ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'IF NOT EXISTS checkprevcheckout enum('yes', 'no', 'inherit') NOT NULL DEFAULT' at line 1
Comment 93 Lucas Gass (lukeg) 2025-08-22 13:45:54 UTC
Follow-ups here are pushed to main.
Comment 94 Lucas Gass (lukeg) 2025-08-22 13:46:24 UTC
We still need release notes here, please add them.