Bug 20644 - Per itemtype setting for CheckPrevCheckout
Summary: Per itemtype setting for CheckPrevCheckout
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Hammat wele
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-23 20:44 UTC by Magnus Enger
Modified: 2025-01-14 20:36 UTC (History)
9 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 20644: Add checkprevcheckout on item types (35.03 KB, patch)
2023-10-12 14:35 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review
Bug 20644: Add checkprevcheckout on item types (36.22 KB, patch)
2023-10-12 14:47 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review
Bug 20644: Add checkprevcheckout on item types (36.27 KB, patch)
2024-03-21 15:13 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 20644: [DO NOT PUSH] DBIC Update (1.31 KB, patch)
2024-04-25 19:46 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 20644: Add checkprevcheckout on item types (33.58 KB, patch)
2024-04-25 19:46 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 20644: [DO NOT PUSH] DBIC Update (1.31 KB, patch)
2024-04-26 15:12 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 20644: Add checkprevcheckout on item types (34.39 KB, patch)
2024-04-26 15:12 UTC, Matthias Le Gac
Details | Diff | Splinter Review
Bug 20644: [DO NOT PUSH] DBIC Update (1.31 KB, patch)
2024-08-22 21:02 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 20644: Add checkprevcheckout on item types (20.79 KB, patch)
2024-08-22 21:02 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 20644: [DO NOT PUSH] DBIC Update (1.31 KB, patch)
2024-08-22 21:07 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 20644: Add checkprevcheckout on item types (22.34 KB, patch)
2024-08-22 21:07 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 20644: [DO NOT PUSH] DBIC Update (1.55 KB, patch)
2025-01-14 20:35 UTC, William Lavoie
Details | Diff | Splinter Review
Bug 20644: Add checkprevcheckout on item types (22.39 KB, patch)
2025-01-14 20:35 UTC, William Lavoie
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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