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)
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?
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.
Created attachment 156974 [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.
Created attachment 156977 [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.
Created attachment 163627 [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: Laura Escamilla <laura.escamilla@bywatersolutions.com>
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.
Created attachment 165585 [details] [review] Bug 20644: [DO NOT PUSH] DBIC Update Current status: Failed QA
Created attachment 165586 [details] [review] Bug 20644: Add checkprevcheckout on item types
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.
Created attachment 165662 [details] [review] Bug 20644: Add checkprevcheckout on item types
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?
(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"
Created attachment 170619 [details] [review] Bug 20644: [DO NOT PUSH] DBIC Update
Created attachment 170620 [details] [review] Bug 20644: Add checkprevcheckout on item types
Created attachment 170621 [details] [review] Bug 20644: [DO NOT PUSH] DBIC Update
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.
(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
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
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?
Created attachment 176538 [details] [review] Bug 20644: [DO NOT PUSH] DBIC Update
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.
Rebased