Bug 6906 - Show 'Borrower has previously issued $ITEM' alert on checkout
Summary: Show 'Borrower has previously issued $ITEM' alert on checkout
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Alex Sassmannshausen
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
: 10812 (view as bug list)
Depends on: 14836 16086 16087
Blocks: 16284 17471 19457 16870 16917 17069 17472
  Show dependency treegraph
 
Reported: 2011-09-22 06:54 UTC by Mason James
Modified: 2018-04-10 18:36 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
'reading-history' setting is enabled by librarian, for specific user (144.98 KB, image/png)
2011-09-22 07:03 UTC, Mason James
Details
'reading-history' setting is now displayed in staff sidebar area (206.52 KB, image/png)
2011-09-22 07:05 UTC, Mason James
Details
librarian attempts to issue a previously issued item... (206.14 KB, image/png)
2011-09-22 07:10 UTC, Mason James
Details
Patch to implement described functionality (29.71 KB, patch)
2014-04-24 20:10 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 6906 - show 'Borrower has previously... (29.33 KB, patch)
2014-04-25 10:41 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 6906 - show 'Borrower has previously... (25.72 KB, patch)
2014-08-11 06:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
[Signded-off] Bug 6906 - show 'Borrower has previously... (25.91 KB, patch)
2014-08-11 08:01 UTC, Marc Véron
Details | Diff | Splinter Review
Revised, reworked implementation (Koha::Objects, better granularity) (35.22 KB, patch)
2016-04-08 12:16 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Don't depend on duplicate bug. (35.26 KB, patch)
2016-04-11 12:12 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 6906 - show 'Borrower has previously issued... (35.31 KB, patch)
2016-04-11 13:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Post-QA revised patch (36.47 KB, patch)
2016-04-14 15:58 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Fixed Post-QA revised patch. (36.41 KB, patch)
2016-04-15 16:29 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Post-QA, specifically avoid Serials (38.31 KB, patch)
2016-04-18 11:03 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Rebase on Master (36.44 KB, patch)
2016-05-10 12:07 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 6906 - show 'Borrower has previously issued... (36.62 KB, patch)
2016-05-10 17:57 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 6906 - show 'Borrower has previously issued... (36.96 KB, patch)
2016-06-12 10:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6906: Tests - Do not assume CPL exists (1.29 KB, patch)
2016-06-12 10:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6906: Add the warning message to the batch checkout (1.27 KB, patch)
2016-06-12 10:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6906: Follow coding guidelines in terminology. (6.74 KB, patch)
2016-07-05 15:47 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 6906: Add updated schema files. (4.38 KB, patch)
2016-07-05 15:48 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 6906: Fix POD reference to old method name. (769 bytes, patch)
2016-07-06 15:13 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 6906 - show 'Borrower has previously issued... (36.96 KB, patch)
2016-07-06 16:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6906: Tests - Do not assume CPL exists (1.29 KB, patch)
2016-07-06 16:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6906: Add the warning message to the batch checkout (1.27 KB, patch)
2016-07-06 16:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6906: Follow coding guidelines in terminology. (6.93 KB, patch)
2016-07-06 16:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6906: Add updated schema files. (4.45 KB, patch)
2016-07-06 16:16 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mason James 2011-09-22 06:54:23 UTC
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
Comment 1 Mason James 2011-09-22 06:56:14 UTC
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
Comment 2 Mason James 2011-09-22 07:03:02 UTC
Created attachment 5531 [details]
'reading-history' setting is enabled by librarian, for specific user
Comment 3 Mason James 2011-09-22 07:05:42 UTC
Created attachment 5532 [details]
'reading-history' setting is now displayed in staff sidebar area
Comment 4 Mason James 2011-09-22 07:10:41 UTC
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
Comment 5 Nicole C. Engard 2011-09-29 15:50:44 UTC
Grammar issue - 'has' isn't necessary in that warning box.  Do you have a patch for this? Or are you just suggesting it?

Nicole
Comment 6 Nicole C. Engard 2011-09-29 15:51:55 UTC
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.
Comment 7 Mason James 2011-10-13 17:29:27 UTC
(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... :)
Comment 8 Katrin Fischer 2014-04-18 14:15:56 UTC
Hm, any update on this?
Comment 9 Katrin Fischer 2014-04-18 14:26:55 UTC
*** Bug 10812 has been marked as a duplicate of this bug. ***
Comment 10 Alex Sassmannshausen 2014-04-24 19:54:51 UTC
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
Comment 11 Alex Sassmannshausen 2014-04-24 20:10:18 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2014-04-25 06:31:37 UTC
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?
Comment 13 Alex Sassmannshausen 2014-04-25 10:41:19 UTC Comment hidden (obsolete)
Comment 14 Nick Clemens 2014-08-09 04:40:10 UTC
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...
Comment 15 Martin Renvoize 2014-08-11 06:36:28 UTC Comment hidden (obsolete)
Comment 16 Martin Renvoize 2014-08-11 06:37:25 UTC
Re-based against current master.  Would be awesome if you could take another look Nick :)
Comment 17 Marc Véron 2014-08-11 08:01:17 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2014-08-15 09:04:28 UTC
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.
Comment 19 Katrin Fischer 2014-08-15 09:14:17 UTC
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.
Comment 20 Alex Sassmannshausen 2016-04-08 12:16:30 UTC Comment hidden (obsolete)
Comment 21 Alex Sassmannshausen 2016-04-11 12:12:38 UTC Comment hidden (obsolete)
Comment 22 Nick Clemens 2016-04-11 13:24:41 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2016-04-13 15:20:03 UTC
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).
Comment 24 Alex Sassmannshausen 2016-04-14 15:58:10 UTC Comment hidden (obsolete)
Comment 25 Owen Leonard 2016-04-15 15:57:35 UTC
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.
Comment 26 Alex Sassmannshausen 2016-04-15 16:29:27 UTC Comment hidden (obsolete)
Comment 27 Radek Šiman (R-Bit Technology, s.r.o.) 2016-04-15 19:44:22 UTC
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.
Comment 28 Alex Sassmannshausen 2016-04-18 11:03:07 UTC Comment hidden (obsolete)
Comment 29 Katrin Fischer 2016-04-18 14:20:45 UTC
Hi Alex, can I ask how the check for the serial is done?
Comment 30 Katrin Fischer 2016-04-18 14:22:37 UTC
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.
Comment 31 Alex Sassmannshausen 2016-04-18 14:46:29 UTC
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
Comment 32 Radek Šiman (R-Bit Technology, s.r.o.) 2016-04-18 14:51:47 UTC
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.
Comment 33 Katrin Fischer 2016-04-18 14:55:54 UTC
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.
Comment 34 Katrin Fischer 2016-04-18 14:56:36 UTC
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.
Comment 35 Katrin Fischer 2016-04-18 14:59:31 UTC
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)
Comment 36 Radek Šiman (R-Bit Technology, s.r.o.) 2016-04-18 15:06:01 UTC
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
Comment 37 Alex Sassmannshausen 2016-04-18 15:27:25 UTC
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
Comment 38 Alex Sassmannshausen 2016-05-10 12:07:46 UTC Comment hidden (obsolete)
Comment 39 Marc Véron 2016-05-10 15:00:41 UTC
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.
Comment 40 Alex Sassmannshausen 2016-05-10 16:06:22 UTC
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
Comment 41 Marc Véron 2016-05-10 16:26:24 UTC
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.
Comment 42 Alex Sassmannshausen 2016-05-10 16:33:47 UTC
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
Comment 43 Marc Véron 2016-05-10 16:42:24 UTC
Hi Alex,

I'm sorry... I messed up the patron categories while testing... Mea culpa!

Will do some more testing.
Comment 44 Marc Véron 2016-05-10 17:57:23 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2016-06-12 10:44:16 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2016-06-12 10:44:21 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2016-06-12 10:44:38 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2016-06-17 15:20:05 UTC
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!
Comment 49 Alex Sassmannshausen 2016-07-05 15:47:48 UTC Comment hidden (obsolete)
Comment 50 Alex Sassmannshausen 2016-07-05 15:48:03 UTC Comment hidden (obsolete)
Comment 51 Alex Sassmannshausen 2016-07-05 15:50:51 UTC
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
Comment 52 Jonathan Druart 2016-07-06 14:29:23 UTC
(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.
Comment 53 Jonathan Druart 2016-07-06 14:31:05 UTC
Note that an occurrence of 'wantsCheckPrevCheckout' has been left in the POD of Koha::Patron.
Comment 54 Alex Sassmannshausen 2016-07-06 15:13:08 UTC Comment hidden (obsolete)
Comment 55 Alex Sassmannshausen 2016-07-06 15:13:50 UTC
(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.
Comment 56 Jonathan Druart 2016-07-06 15:43:09 UTC
(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
Comment 57 Alex Sassmannshausen 2016-07-06 15:57:24 UTC
(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
Comment 58 Jonathan Druart 2016-07-06 16:12:07 UTC
(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?
Comment 59 Jonathan Druart 2016-07-06 16:15:55 UTC
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>
Comment 60 Jonathan Druart 2016-07-06 16:16:05 UTC
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>
Comment 61 Jonathan Druart 2016-07-06 16:16:13 UTC
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>
Comment 62 Jonathan Druart 2016-07-06 16:16:18 UTC
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>
Comment 63 Jonathan Druart 2016-07-06 16:16:23 UTC
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>
Comment 64 Jonathan Druart 2016-07-06 16:19:33 UTC
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.
Comment 65 Kyle M Hall 2016-07-08 13:35:07 UTC
> 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!
Comment 66 Kyle M Hall 2016-07-08 13:41:58 UTC
Pushed to master for 16.11, thanks Alex, Jonathan!