A common request for some borrowers is to be alerted by library staff, when issuing items, to whether they have previously issued an item before this enhancement provides this feature, and allows the librarian to set the checkout alert, at a per-user level
oops, that should read... A common request for some borrowers is to be alerted by library staff, when issuing items, to whether they have previously issued *that* item before :) this enhancement provides this feature, and allows the librarian to set the checkout alert, at a per-user level
Created attachment 5531 [details] 'reading-history' setting is enabled by librarian, for specific user
Created attachment 5532 [details] 'reading-history' setting is now displayed in staff sidebar area
Created attachment 5533 [details] librarian attempts to issue a previously issued item... librarian attempts to issue a previously issued item, and a yes/no warning prompt is displayed
Grammar issue - 'has' isn't necessary in that warning box. Do you have a patch for this? Or are you just suggesting it? Nicole
Oh and we might want to call it something other than 'reading history' since the reading history can be controlled by a system preference. Maybe 'reading history warning' would be more accurate.
(In reply to comment #5) > Grammar issue - 'has' isn't necessary in that warning box. Do you have a patch > for this? Or are you just suggesting it? > > Nicole heya, yeah - its an feature i did for Kapiti patch coming soon... :)
Hm, any update on this?
*** Bug 10812 has been marked as a duplicate of this bug. ***
Hello, In preparation of work on bug 5670, I have implemented the functionality discussed in this bug in a generic, granular fashion. I will shortly be uploading my patch for (hopefully) sign-off and qa. Let me know if you have any questions. Best wishes, Alex
Created attachment 27558 [details] [review] Patch to implement described functionality New module: provide granular means to configure warnings about items that have been issued to a particular borrower before, according to their loan history. - Global syspref ('CheckPrevIssueByDefault'), set to 'No' by default, allows users to enable this feature library wide. - Per category pref allow libraries to create overrides per category, falling back on the global setting by default. - Per borrower pref allows switching the functionality on at the level of borrowers. Fall-back to category settings by default. * Koha/Borrowers/CheckPrevIssue.pm: new module. * C4/Circulation.pm (CanBookBeIssued): introduce CheckPrevIssue check. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: Add 'CHECKPREVISSUE' confirmation message. * installer/data/mysql/kohastructure.sql: modify structure of 'categories', 'borrowers'. * installer/data/mysql/sysprefs.sql: add 'CheckPrevIssueByDefault' (exposed in patrons.pref). * installer/data/mysql/updatedatabase.pl: provide upgrade path. * admin/categorie.pl: add 'checkprevissuebycategory' to sql queries; pass it to template. * .../admin/categorie.tt: .../members/memberentrygen.tt: .../members/moremember.tt: add content. * CheckPrevIssue.t: new file with unit tests. Test plan: - Apply patch. - Run db updateds from updatedatabase.pl manually (I have not updated the database version yet — not sure whether I should have done?) - Optionally, run Unit Tests (t/CheckPrevIssue.t) - Verify 'CheckPrevIssueByDefault' is visible in Patrons sysprefs and can be switched 'on' and 'off'. + Issue previously issued items to a borrower, checking the message appears as expected. - Verify 'Check previous loans' setting appears on the borrower category pages and can be modified per borrower category. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the default setting). - Verify 'Check previous loans' setting appears on individual borrower pages and can be modified. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the category setting and the default setting).
Comment on attachment 27558 [details] [review] Patch to implement described functionality Review of attachment 27558 [details] [review]: ----------------------------------------------------------------- Pretty happy with this in general. Just a few minor points regarding style.. but then, I could be missing something ;) ::: installer/data/mysql/kohastructure.sql @@ +265,4 @@ > `altcontactphone` varchar(50) default NULL, -- the phone number for the alternate contact for the patron/borrower > `smsalertnumber` varchar(50) default NULL, -- the mobile phone number where the patron/borrower would like to receive notices (if SNS turned on) > `privacy` integer(11) DEFAULT '1' NOT NULL, -- patron/borrower's privacy settings related to their reading history > + `checkprevissuebyborrower` varchar(7) NOT NULL default 'inherit', -- produce a warning for this borrower if this item has previously been issued to this borrower if 'yes', not if 'no', defer to category setting if 'inherit'. I think I would lose the 'byborrower' part of 'checkprevissuebyborrower' here, I think 'checkprevissue' is descriptive enough, and we already know it's in the borrower context due to the table it's found in. @@ +466,5 @@ > `issuelimit` smallint(6) default NULL, -- unused in Koha > `reservefee` decimal(28,6) default NULL, -- cost to place holds > `hidelostitems` tinyint(1) NOT NULL default '0', -- are lost items shown to this category (1 for yes, 0 for no) > + `category_type` varchar(1) NOT NULL default 'A', -- type of Koha patron (Adult, Child, Professional, Organizational, Statistical, Staff), > + `checkprevissuebycategory` varchar(7) NOT NULL default 'inherit', -- produce a warning for this borrower category if this item has previously been issued to this borrower if 'yes', not if 'no', defer to category setting if 'inherit'. Do you mean "defer to 'syspref' setting if inherit." here? I think I would lose the 'bycategory' part of 'checkprevissuebycategory' here, I think 'checkprevissue' is descriptive enough, and we already know it's in the category context due to the table it's found in. ::: installer/data/mysql/updatedatabase.pl @@ +8202,4 @@ > SetVersion($DBversion); > } > > +# FIXME: change $DBversion The 'normal' way of doing this now is to put '$DBversion = "3.15.00.XXX";' These happen all the time, so the RM and QA people are used to these types of conflicts ;) ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref @@ +50,4 @@ > class: multi > - (separate multiple choices with |) > - > + - pref: CheckPrevIssueByDefault As you can probably tell by now, I'm a fan of shorter, but still descriptive, names. Do we really need the 'ByDefault' here?
Created attachment 27606 [details] [review] Bug 6906 - show 'Borrower has previously... New module: provide granular means to configure warnings about items that have been issued to a particular borrower before, according to their loan history. - Global syspref ('CheckPrevIssue'), set to 'No' by default, allows - users to enable this feature library wide. - Per category pref allow libraries to create overrides per category, falling back on the global setting by default. - Per borrower pref allows switching the functionality on at the level of borrowers. Fall-back to category settings by default. * Koha/Borrowers/CheckPrevIssue.pm: new module. * C4/Circulation.pm (CanBookBeIssued): introduce CheckPrevIssue check. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: Add 'CHECKPREVISSUE' confirmation message. * installer/data/mysql/kohastructure.sql: modify structure of 'categories', 'borrowers'. * installer/data/mysql/sysprefs.sql: add 'CheckPrevIssue' (exposed in patrons.pref). * installer/data/mysql/updatedatabase.pl: provide upgrade path. * admin/categorie.pl: add 'checkprevissue' to sql queries; pass it to template. * .../admin/categorie.tt: .../members/memberentrygen.tt: .../members/moremember.tt: add content. * CheckPrevIssue.t: new file with unit tests. Test plan: - Apply patch. - Run db updates from updatedatabase.pl manually. - Verify 'CheckPrevIssue' is visible in Patrons sysprefs and can be switched 'on' and 'off'. + Issue previously issued items to a borrower, checking the message appears as expected. - Verify 'Check previous loans' setting appears on the borrower category pages and can be modified per borrower category. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the default setting). - Verify 'Check previous loans' setting appears on individual borrower pages and can be modified. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the category setting and the default setting).
Applying: Bug 6906 - show 'Borrower has previously... Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/categorie.tt Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/kohastructure.sql CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql Auto-merging admin/categorie.pl CONFLICT (content): Merge conflict in admin/categorie.pl Auto-merging C4/Circulation.pm Failed to merge in the changes. Patch failed at 0001 Bug 6906 - show 'Borrower has previously...
Created attachment 30680 [details] [review] Bug 6906 - show 'Borrower has previously... New module: provide granular means to configure warnings about items that have been issued to a particular borrower before, according to their loan history. - Global syspref ('CheckPrevIssue'), set to 'No' by default, allows - users to enable this feature library wide. - Per category pref allow libraries to create overrides per category, falling back on the global setting by default. - Per borrower pref allows switching the functionality on at the level of borrowers. Fall-back to category settings by default. * Koha/Borrowers/CheckPrevIssue.pm: new module. * C4/Circulation.pm (CanBookBeIssued): introduce CheckPrevIssue check. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: Add 'CHECKPREVISSUE' confirmation message. * installer/data/mysql/kohastructure.sql: modify structure of 'categories', 'borrowers'. * installer/data/mysql/sysprefs.sql: add 'CheckPrevIssue' (exposed in patrons.pref). * installer/data/mysql/updatedatabase.pl: provide upgrade path. * admin/categorie.pl: add 'checkprevissue' to sql queries; pass it to template. * .../admin/categorie.tt: .../members/memberentrygen.tt: .../members/moremember.tt: add content. * CheckPrevIssue.t: new file with unit tests. Test plan: - Apply patch. - Run db updates from updatedatabase.pl manually. - Verify 'CheckPrevIssue' is visible in Patrons sysprefs and can be switched 'on' and 'off'. + Issue previously issued items to a borrower, checking the message appears as expected. - Verify 'Check previous loans' setting appears on the borrower category pages and can be modified per borrower category. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the default setting). - Verify 'Check previous loans' setting appears on individual borrower pages and can be modified. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the category setting and the default setting).
Re-based against current master. Would be awesome if you could take another look Nick :)
Created attachment 30681 [details] [review] [Signded-off] Bug 6906 - show 'Borrower has previously... New module: provide granular means to configure warnings about items that have been issued to a particular borrower before, according to their loan history. - Global syspref ('CheckPrevIssue'), set to 'No' by default, allows - users to enable this feature library wide. - Per category pref allow libraries to create overrides per category, falling back on the global setting by default. - Per borrower pref allows switching the functionality on at the level of borrowers. Fall-back to category settings by default. * Koha/Borrowers/CheckPrevIssue.pm: new module. * C4/Circulation.pm (CanBookBeIssued): introduce CheckPrevIssue check. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: Add 'CHECKPREVISSUE' confirmation message. * installer/data/mysql/kohastructure.sql: modify structure of 'categories', 'borrowers'. * installer/data/mysql/sysprefs.sql: add 'CheckPrevIssue' (exposed in patrons.pref). * installer/data/mysql/updatedatabase.pl: provide upgrade path. * admin/categorie.pl: add 'checkprevissue' to sql queries; pass it to template. * .../admin/categorie.tt: .../members/memberentrygen.tt: .../members/moremember.tt: add content. * CheckPrevIssue.t: new file with unit tests. Test plan: - Apply patch. - Run db updates from updatedatabase.pl manually. - Verify 'CheckPrevIssue' is visible in Patrons sysprefs and can be switched 'on' and 'off'. + Issue previously issued items to a borrower, checking the message appears as expected. - Verify 'Check previous loans' setting appears on the borrower category pages and can be modified per borrower category. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the default setting). - Verify 'Check previous loans' setting appears on individual borrower pages and can be modified. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the category setting and the default setting). Followed test plan. Patch behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Comment on attachment 30681 [details] [review] [Signded-off] Bug 6906 - show 'Borrower has previously... Review of attachment 30681 [details] [review]: ----------------------------------------------------------------- ::: Koha/Borrower/CheckPrevIssue.pm @@ +29,5 @@ > +); > + > +=head1 Koha::Borrower::CheckPrevIssue > + > +Koha::Borrower::Debarments - Manage Previous Issue preferences & searches. (trivial) Should be Koha::Borrower::CheckPrevIssue :) @@ +65,5 @@ > +sub _WantsCheckPrevIssueByCat { > + my ( $borrowernumber ) = @_; > + my $dbh = C4::Context->dbh; > + my $query = ' > +SELECT categories.checkprevissue (not a blocker) Could be rewritten in a follow-up to use DBIC. ::: installer/data/mysql/kohastructure.sql @@ +265,4 @@ > `altcontactphone` varchar(50) default NULL, -- the phone number for the alternate contact for the patron/borrower > `smsalertnumber` varchar(50) default NULL, -- the mobile phone number where the patron/borrower would like to receive notices (if SNS turned on) > `privacy` integer(11) DEFAULT '1' NOT NULL, -- patron/borrower's privacy settings related to their reading history > + `checkprevissue` varchar(7) NOT NULL default 'inherit', -- produce a warning for this borrower if this item has previously been issued to this borrower if 'yes', not if 'no', defer to category setting if 'inherit'. (normal) This needs to be added to deletedborrowers as well as both tables need to be kept in sync. ::: installer/data/mysql/updatedatabase.pl @@ +8606,5 @@ > +$DBversion = "3.17.00.XXX"; > +if ( CheckVersion($DBversion) ) { > + $dbh->do("INSERT INTO systempreferences (variable,value,options,explanation,type) VALUES('CheckPrevIssue','0','','By default, for every item issued, should we warn if the patron has borrowed that item in the past?','YesNo')"); > + $dbh->do("ALTER TABLE categories ADD (`checkprevissue` varchar(7) NOT NULL default 'inherit')"); > + $dbh->do("ALTER TABLE borrowers ADD (`checkprevissue` varchar(7) NOT NULL default 'inherit')"); (normal) deletedborrowers need to be added here as well.
Some additions from further testing: (trivial) Test file All tests pass :) I think the file name would be a bit better including Koha_Borrowers.* in front like the other test files. There is also a Koha directory it could fit into. (trivial) Database update When adding new columns to the tables it's best practice to use AFTER or BEFORE making sure they go in the same spot for new and updated installations. Otherwise looking good so far.
Created attachment 50071 [details] [review] Revised, reworked implementation (Koha::Objects, better granularity) This is a pretty comprehensive rewrite including all feedback previously passed to it. It also makes use of Koha::Objects (which is why it depends on the above objects patches. Let me know if you spot any problems! Alex
Created attachment 50127 [details] [review] Don't depend on duplicate bug. I now correct my mistake of depending on my own implementation of Koha::Categories, as I discovered that these objects already exist (pushed to master) in the form of Koha::Patron::Categories. This patch uses the latter.
Created attachment 50128 [details] [review] Bug 6906 - show 'Borrower has previously issued... New feature: provide granular means to configure warnings about items that have been issued to a particular borrower before, according to their loan history. - Global syspref ('CheckPrevIssue'), set to 'hardno' by default, allows users to enable this feature library wide. - Per patron category pref allows libraries to create overrides per category, falling back on the global setting by default. - Per patron pref allows switching the functionality on at the level of patron. Fall-back to category settings by default. * Koha/Patron/CheckPrevIssue.pm: New file * C4/Circulation.pm (CanBookBeIssued): Introduce CheckPrevIssue check. * admin/categories.pl: Pass along checkprevissue. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt: Expose CheckPrevIssue per category setting. * koha-tmpl/intranet-tmpl/prog/en/modules/preferences/patrons.pref: Expose CheckPrevIssue syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: Expose per patron CheckPrevIssue preference. * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Expose per patron CheckPrevIssue preference. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: Add 'CHECKPREVISSUE' confirmation message. * installer/data/mysql/kohastructure.sql: Modify structure of 'categories', 'borrowers'. * installer/data/mysql/sysprefs.sql: Add 'CheckPrevIssue'. * installer/data/mysql/atomicupdate/checkPrevIssue.sql: New file. * t/db_dependent/Patron/CheckPrevIssue.t: New file with unit tests. Test plan: - Apply patch. - Run updatedatabase. - Regenerate Koha Schema files. - Run the unit tests. - Verify 'CheckPrevIssue' is visible in Patrons sysprefs and can be switched to 'hardyes', 'softyes', 'softno' and 'hardno'. + Check out previously issued items to a patron, checking the message appears as expected. - Verify no 'Check previous loans' setting appears on the borrower category pages if the syspref is set to a 'hard' option. - Verify 'Check previous loans' setting appears on the borrower category pages and can be modified per borrower category. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the default setting if that is set to a 'soft' option). - Verify no 'Check previous loans' setting appears on the individual borrower pages if the syspref is set to a 'hard' option. - Verify 'Check previous loans' setting appears on individual borrower pages and can be modified. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the category setting and the default setting if the latter is set to a 'soft' option). Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
QA comments: On the whole, this patch is great. Tests are provided and the code is clean and pleasant to read. Great job Alex! 1/ I am wondering if the 2 subroutines in Koha::Patron::CheckPrevIssue should not be methods of Koha::Patron 2/ I think that "Loans"/"Loaned" should be replaced with checkouts/check out (see http://wiki.koha-community.org/wiki/Terminology) 3/ Please provide test coverage for the changes you made in C4::Circulation::CanBookBeIssued 4/ In the update DB entry, please add the "after" clause to make sure the columns will be added at the same place than new installs (kohastructure).
Created attachment 50243 [details] [review] Post-QA revised patch This revised patch includes all feedback from QA, plus a consistent renaming of the feature away from checkprevissue to checkprevcheckout. - The functionality is now exposed as methods to Patron. - The unit tests test CanBookBeIssued. - I have renamed references to loans/loan and issues. - I have added `after`clauses to the atomicupdate. Let me know if there are any outstanding issues. Alex
When I go to check something out I get this error: Can't locate object method "find" via package "Koha::Patron::Categories" (perhaps you forgot to load "Koha::Patron::Categories"?) at Koha/Patron.pm line 122. This happens when the preference is either "softyes" or "softno." If the preferences is "hardyes" or "hardno" it works without an error.
Created attachment 50286 [details] [review] Fixed Post-QA revised patch. Apologies — during the refactoring for this feature to be implemented as methods upon Patron, I lost the import of Koha::Patron::Categories. The unit tests did not catch this as I was importing Categories independently there. Both have now been fixed. Thanks for testing! Alex
Inspecting the last attachment 50286 [details] [review] I cannot find if periodical biblios are well processed. This special case concerns a scenario where a borrower is about to get another item of the same periodical title. In such a situation we need to check if the same item was already issued and not the same biblio only. There are borrowers ckecking-out several items if the same title at once. It seems to me that the current solution would ask for N-1 items to "Checkout anyway?". We should ask for repeating items of periodicals only.
Created attachment 50323 [details] [review] Post-QA, specifically avoid Serials Hi Radek, Thanks for your interest in this feature! You are quite right in your assessment of the current situation, and I can also see that the use case you describe is a valid one. Unfortunately I cannot currently invest the time to implement these additional checks. As a compromise, here a revised patch that I believe should take serials out of the loop entirely. Best wishes Alex
Hi Alex, can I ask how the check for the serial is done?
I 'fear' that you are checking biblio.serial which I have no idea how it's set in a MARC21 db, I have always thought it to be a UNIMARC related field as it's always 0 or NULL in our databases.
Hello, Indeed — thanks for checking that out Cait. I have now obsoleted that patch and re-activated the patch containing the QA fixes. I would propose we put the feature in as is, as it is: - purely opt-in - I do not have too much time to investigate the question of serials, and potential other complications this might bring in - the feature does what it is intended to do for our purposes — though I would be very pleased to see, and help, with any follow up patches that try to address the serials question. WDYT? Alex
Few weeks ago we developed our internal patch (not-publishable yet) which checks serials by the bibliographic framework code they use. In addition, we used to set a special preference value which enlists these serial framework codes separated by pipe. The checking procedure then decides whether to check biblios or items and works pretty well. Unfortunately our solution is a bit outdated now, because this bug (#6906) is about to be solved and published.
Hm, I am not sure about using the frameworkcode - it seems yet another way of doing it. We already have a preference that checks for the existence of a subscription AllowMultipleIssuesOnABiblio and other prefs use the itemtype on biblio level (holds). What I would like better is using a list of itemtypes and/or checking the LDR for the serial flag (and a similar field for UNIMARC, I am sure there is one). I think this would be more in line with how things currently work. The frameworkcode is problematic for libraries exporting records from a union catalog - tbh, it wouldn't work for most of our libraries at all.
Hm, I should add - I feel like this is ok to go in without the serial (being optional as Alex pointed out), but we should open another bug to discuss the serials/multi-volume works and similar problem there.
Ah, another thing to note: if you are using the advanced cataloguing editor ("Rancor") I think there is no use for the frameworkcode, so it might always default to the "default" (iirc)
With these new points of view I agree with the proposal of making serial checking optional for now and to open a new bug for the multi-volumes
I have created a new bug for the discussion of the additional Serial feature at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16284 Alex
Created attachment 51378 [details] [review] Rebase on Master Same patch, rebased on master.
Testing: - patch applied - dbupdate done - schema updated Test 1: - System preferences CheckPrevCheckout is: 'Unless overridden, DO...' - Category X has: 'NO and try to override system preferences' - Patron (of category X) has: Check previous checkouts: inherited Result: Warning 'This item has previously been checked out to this patron.' Expected: No warning. Test 2: - System preferences CheckPrevCheckout is: 'Unless overridden, DO NOT...' - Category X has: 'YES and try to override system preferences' - Patron (of category X) has: Check previous checkouts: inherited Result: No warning Expected: Warning If I read correctly, I would expect that the patron category settings override the system preferences.
Hi Marc, Thank you very much for taking the time to test the patch. I am currently not able to reproduce the problem you describe on my test system (with a fresh install of Koha and this patch). Would you be able to run the test suite to see if that passes for you (prove t/db_dependent/Patron/CheckPrevCheckout.t)? Bizarre thing is, your test results are exactly the opposite of what is supposed to happen. You are right in thinking the determining factor in this context should be the category setting *only*. Alex
Hi Alex, I get: t/db_dependent/Patron/CheckPrevCheckout.t .. ok All tests successful. Files=1, Tests=59, 3 wallclock secs ( 0.03 usr 0.01 sys + 2.37 cusr 0.31 csys = 2.72 CPU) Result: PASS However, I did not test on a fresh install. I will try to find out what happens on my site.
Hmm... thanks for your quick response. The tests essentially go through all permutations of the user, category, syspref combinations — and they pass. It seems interesting the error occurs in the UI for you. I'd be very interested in hearing further results from your side! Alex
Hi Alex, I'm sorry... I messed up the patron categories while testing... Mea culpa! Will do some more testing.
Created attachment 51393 [details] [review] Bug 6906 - show 'Borrower has previously issued... New feature: provide granular means to configure warnings about items that have been issued to a particular borrower before, according to their checkout history. - Global syspref ('CheckPrevCheckout'), set to 'hardno' by default, allows users to enable this feature library wide. - Per patron category pref allows libraries to create overrides per category, falling back on the global setting by default. - Per patron pref allows switching the functionality on at the level of patron. Fall-back to category settings by default. * Koha/Patron (wantsCheckPrevCheckout, doCheckPrevCheckout): New methods. * C4/Circulation.pm (CanBookBeIssued): Introduce CheckPrevCheckout check. * admin/categories.pl: Pass along checkprevcheckout. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt: Expose CheckPrevCheckout per category setting. * koha-tmpl/intranet-tmpl/prog/en/modules/preferences/patrons.pref: Expose CheckPrevCheckout syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: Expose per patron CheckPrevCheckout preference. * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Expose per patron CheckPrevCheckout preference. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: Add 'CHECKPREVCHECKOUT' confirmation message. * installer/data/mysql/kohastructure.sql: Modify structure of 'categories', 'borrowers', 'oldborrowers'. * installer/data/mysql/sysprefs.sql: Add 'CheckPrevCheckout'. * installer/data/mysql/atomicupdate/checkPrevCheckout.sql: New file. * t/db_dependent/Patron/CheckPrevCheckout.t: New file with unit tests. Test plan: - Apply patch. - Run updatedatabase. - Regenerate Koha Schema files. - Run the unit tests. - Verify 'CheckPrevCheckout' is visible in Patrons sysprefs and can be switched to 'hardyes', 'softyes', 'softno' and 'hardno'. + Check out previously checked out items to a patron, checking the message appears as expected. - Verify no 'Check previous checkouts' setting appears on the borrower category pages if the syspref is set to a 'hard' option. - Verify 'Check previous checkouts' setting appears on the borrower category pages and can be modified per borrower category. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the default setting if that is set to a 'soft' option). - Verify no 'Check previous checkouts' setting appears on the individual borrower pages if the syspref is set to a 'hard' option. - Verify 'Check previous checkouts' setting appears on individual borrower pages and can be modified. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the category setting and the default setting if the latter is set to a 'soft' option). Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 52278 [details] [review] Bug 6906 - show 'Borrower has previously issued... New feature: provide granular means to configure warnings about items that have been issued to a particular borrower before, according to their checkout history. - Global syspref ('CheckPrevCheckout'), set to 'hardno' by default, allows users to enable this feature library wide. - Per patron category pref allows libraries to create overrides per category, falling back on the global setting by default. - Per patron pref allows switching the functionality on at the level of patron. Fall-back to category settings by default. * Koha/Patron (wantsCheckPrevCheckout, doCheckPrevCheckout): New methods. * C4/Circulation.pm (CanBookBeIssued): Introduce CheckPrevCheckout check. * admin/categories.pl: Pass along checkprevcheckout. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt: Expose CheckPrevCheckout per category setting. * koha-tmpl/intranet-tmpl/prog/en/modules/preferences/patrons.pref: Expose CheckPrevCheckout syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: Expose per patron CheckPrevCheckout preference. * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Expose per patron CheckPrevCheckout preference. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: Add 'CHECKPREVCHECKOUT' confirmation message. * installer/data/mysql/kohastructure.sql: Modify structure of 'categories', 'borrowers', 'oldborrowers'. * installer/data/mysql/sysprefs.sql: Add 'CheckPrevCheckout'. * installer/data/mysql/atomicupdate/checkPrevCheckout.sql: New file. * t/db_dependent/Patron/CheckPrevCheckout.t: New file with unit tests. Test plan: - Apply patch. - Run updatedatabase. - Regenerate Koha Schema files. - Run the unit tests. - Verify 'CheckPrevCheckout' is visible in Patrons sysprefs and can be switched to 'hardyes', 'softyes', 'softno' and 'hardno'. + Check out previously checked out items to a patron, checking the message appears as expected. - Verify no 'Check previous checkouts' setting appears on the borrower category pages if the syspref is set to a 'hard' option. - Verify 'Check previous checkouts' setting appears on the borrower category pages and can be modified per borrower category. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the default setting if that is set to a 'soft' option). - Verify no 'Check previous checkouts' setting appears on the individual borrower pages if the syspref is set to a 'hard' option. - Verify 'Check previous checkouts' setting appears on individual borrower pages and can be modified. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the category setting and the default setting if the latter is set to a 'soft' option). Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 52279 [details] [review] Bug 6906: Tests - Do not assume CPL exists Since bug 14878, tests must create the data they need to pass and not assume they exist. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 52280 [details] [review] Bug 6906: Add the warning message to the batch checkout Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I'm getting 8 failing subtests out of 59. Also, please rename the method wantsCheckPrevCheckout to something like should_check_for_previous_checkout and doCheckPrevCheckout to check_for_previous_checkout. As per the Koha Objects pages linked from the Coding Guidelines, all Koha::Object(s) derived classes should use snake case. Thanks!
Created attachment 53104 [details] [review] Bug 6906: Follow coding guidelines in terminology. * Koha/Patron.pm (wantsCheckPrevCheckout, doCheckPrevCheckout): Rename to... (wants_check_for_previous_checkout, do_check_for_previous_checkout): ...this. * C4/Circulation.pm: Use new names. * t/db_dependent/Patron/CheckPrevCheckout.t: Renamed to... * t/db_dependent/Patron/Borrower_PrevCheckout.t: ...this.
Created attachment 53105 [details] [review] Bug 6906: Add updated schema files. * Koha/Schema/Result/Borrower.pm: Updated schema file. * Koha/Schema/Result/Category.pm: Updated schema file. * Koha/Schema/Result/Deletedborrower.pm: Updated schema file.
Hi Kyle, I've added 2 more patches to resolve the issues you raised. - One patch now adds updated schema files for dbic. This will fix the test error (previously I assumed the tester would do this, but since the last hackfest I no longer make that assumption). - The other patch renames the methods as requested. Hopefully this addresses the final issues for this bug? Thanks for the review! Alex
(In reply to Alex Sassmannshausen from comment #49) > * t/db_dependent/Patron/CheckPrevCheckout.t: Renamed to... > * t/db_dependent/Patron/Borrower_PrevCheckout.t: ...this. Why? :) (In reply to Alex Sassmannshausen from comment #51) > - One patch now adds updated schema files for dbic. This will fix the test > error (previously I assumed the tester would do this, but since the last > hackfest I no longer make that assumption). AFAIK that is still the case. Actually I personally submit a patch with a DONT_PUSH pattern in the commit message and let the RM generate the schema by himself.
Note that an occurrence of 'wantsCheckPrevCheckout' has been left in the POD of Koha::Patron.
Created attachment 53136 [details] [review] Bug 6906: Fix POD reference to old method name. * Koha/Patron.pm (wants_check_for_previous_checkout): Fix POD.
(In reply to Jonathan Druart from comment #53) > Note that an occurrence of 'wantsCheckPrevCheckout' has been left in the POD > of Koha::Patron. Good catch, cheers.
(In reply to Jonathan Druart from comment #52) > (In reply to Alex Sassmannshausen from comment #49) > > * t/db_dependent/Patron/CheckPrevCheckout.t: Renamed to... > > * t/db_dependent/Patron/Borrower_PrevCheckout.t: ...this. > > Why? :) It seems that you missed this one
(In reply to Jonathan Druart from comment #52) > (In reply to Alex Sassmannshausen from comment #49) > > * t/db_dependent/Patron/CheckPrevCheckout.t: Renamed to... > > * t/db_dependent/Patron/Borrower_PrevCheckout.t: ...this. > > Why? :) It struck me there was a pattern to the naming of the test files, I decided to stick with that. > (In reply to Alex Sassmannshausen from comment #51) > > - One patch now adds updated schema files for dbic. This will fix the test > > error (previously I assumed the tester would do this, but since the last > > hackfest I no longer make that assumption). > > AFAIK that is still the case. > Actually I personally submit a patch with a DONT_PUSH pattern in the commit > message and let the RM generate the schema by himself. Interesting — I see the reason why you'd add DONT_PUSH — presumably to make it clear to the RM that they should generate the schema files themselves… To clarify: do you literally just dump DONT_PUSH somewhere in the commit message? Or is it part of the subject line or has a special location? Alex
(In reply to Alex Sassmannshausen from comment #57) > (In reply to Jonathan Druart from comment #52) > > (In reply to Alex Sassmannshausen from comment #49) > > > * t/db_dependent/Patron/CheckPrevCheckout.t: Renamed to... > > > * t/db_dependent/Patron/Borrower_PrevCheckout.t: ...this. > > > > Why? :) > > It struck me there was a pattern to the naming of the test files, I decided > to stick with that. Ha yes, got it! Actually the methods of Koha::Patron are tested in subtests of t/db_dependent/Koha/Patrons.t So it would make to move yours there. But it's 400 lines, so maybe not... Actually we have t/db_dependent/Patron.t t/db_dependent/Patrons.t t/db_dependent/Patron/*.t t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Patron/*.t It's a bit... confusing, I agree :) So let's keep it as it, and I will try to clean that on another bug report. > > (In reply to Alex Sassmannshausen from comment #51) > > > - One patch now adds updated schema files for dbic. This will fix the test > > > error (previously I assumed the tester would do this, but since the last > > > hackfest I no longer make that assumption). > > > > AFAIK that is still the case. > > Actually I personally submit a patch with a DONT_PUSH pattern in the commit > > message and let the RM generate the schema by himself. > > Interesting — I see the reason why you'd add DONT_PUSH — presumably to make > it clear to the RM that they should generate the schema files themselves… > > To clarify: do you literally just dump DONT_PUSH somewhere in the commit > message? > > Or is it part of the subject line or has a special location? Beginning of the commit message, something like: Bug 12345: [DO NOT PUSH] Schema files That could be a topic for the next meeting, in order to clarify things for everybody. Kyle, any thoughts?
Created attachment 53144 [details] [review] Bug 6906 - show 'Borrower has previously issued... New feature: provide granular means to configure warnings about items that have been issued to a particular borrower before, according to their checkout history. - Global syspref ('CheckPrevCheckout'), set to 'hardno' by default, allows users to enable this feature library wide. - Per patron category pref allows libraries to create overrides per category, falling back on the global setting by default. - Per patron pref allows switching the functionality on at the level of patron. Fall-back to category settings by default. * Koha/Patron (wantsCheckPrevCheckout, doCheckPrevCheckout): New methods. * C4/Circulation.pm (CanBookBeIssued): Introduce CheckPrevCheckout check. * admin/categories.pl: Pass along checkprevcheckout. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt: Expose CheckPrevCheckout per category setting. * koha-tmpl/intranet-tmpl/prog/en/modules/preferences/patrons.pref: Expose CheckPrevCheckout syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: Expose per patron CheckPrevCheckout preference. * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Expose per patron CheckPrevCheckout preference. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: Add 'CHECKPREVCHECKOUT' confirmation message. * installer/data/mysql/kohastructure.sql: Modify structure of 'categories', 'borrowers', 'oldborrowers'. * installer/data/mysql/sysprefs.sql: Add 'CheckPrevCheckout'. * installer/data/mysql/atomicupdate/checkPrevCheckout.sql: New file. * t/db_dependent/Patron/CheckPrevCheckout.t: New file with unit tests. Test plan: - Apply patch. - Run updatedatabase. - Regenerate Koha Schema files. - Run the unit tests. - Verify 'CheckPrevCheckout' is visible in Patrons sysprefs and can be switched to 'hardyes', 'softyes', 'softno' and 'hardno'. + Check out previously checked out items to a patron, checking the message appears as expected. - Verify no 'Check previous checkouts' setting appears on the borrower category pages if the syspref is set to a 'hard' option. - Verify 'Check previous checkouts' setting appears on the borrower category pages and can be modified per borrower category. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the default setting if that is set to a 'soft' option). - Verify no 'Check previous checkouts' setting appears on the individual borrower pages if the syspref is set to a 'hard' option. - Verify 'Check previous checkouts' setting appears on individual borrower pages and can be modified. + Issue previously issued items to a borrower, checking the message appears as expected (This setting should override the category setting and the default setting if the latter is set to a 'soft' option). Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 53145 [details] [review] Bug 6906: Tests - Do not assume CPL exists Since bug 14878, tests must create the data they need to pass and not assume they exist. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 53146 [details] [review] Bug 6906: Add the warning message to the batch checkout Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 53147 [details] [review] Bug 6906: Follow coding guidelines in terminology. * Koha/Patron.pm (wantsCheckPrevCheckout, doCheckPrevCheckout): Rename to... (wants_check_for_previous_checkout, do_check_for_previous_checkout): ...this. * C4/Circulation.pm: Use new names. * t/db_dependent/Patron/CheckPrevCheckout.t: Renamed to... * t/db_dependent/Patron/Borrower_PrevCheckout.t: ...this. Bug 6906: Fix POD reference to old method name. * Koha/Patron.pm (wants_check_for_previous_checkout): Fix POD. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 53148 [details] [review] Bug 6906: Add updated schema files. * Koha/Schema/Result/Borrower.pm: Updated schema file. * Koha/Schema/Result/Category.pm: Updated schema file. * Koha/Schema/Result/Deletedborrower.pm: Updated schema file. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I have squashed 2 patches, for readability. Note that QA tools complains: FAIL t/db_dependent/Patron/CheckPrevCheckout.t FAIL git manipulation The file has been added and deleted in the same patchset But first patch has been signed off and the other one is a qa-followup, so it's ok by me.
> Beginning of the commit message, something like: > Bug 12345: [DO NOT PUSH] Schema files > That could be a topic for the next meeting, in order to clarify things for > everybody. > Kyle, any thoughts? That works for me!
Pushed to master for 16.11, thanks Alex, Jonathan!