Bug 9011 - Add the ability to store the last patron to return an item
Summary: Add the ability to store the last patron to return an item
Status: RESOLVED DUPLICATE of bug 14945
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
: 13831 (view as bug list)
Depends on: 13790
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-05 18:27 UTC by Kyle M Hall
Modified: 2020-02-04 07:38 UTC (History)
18 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9011 - Add the ability to store the last patron to return an item (9.45 KB, patch)
2012-11-05 19:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (10.90 KB, patch)
2013-02-06 18:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9011 - Add the ability to store the last patron to return an item (12.02 KB, patch)
2013-02-08 20:26 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (11.92 KB, patch)
2013-03-21 15:37 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (12.01 KB, patch)
2013-03-22 11:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item - Add privacy statement (1.54 KB, patch)
2013-03-22 11:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item - Followup - Remove foreign key constrant (1.51 KB, patch)
2013-06-05 14:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (12.05 KB, patch)
2013-08-08 18:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (13.04 KB, patch)
2013-08-08 18:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - QA Followup (4.50 KB, patch)
2013-08-14 13:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (13.15 KB, patch)
2013-08-14 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9011 - QA Followup (4.62 KB, patch)
2013-08-14 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9011: Update GPL license (1.50 KB, patch)
2013-08-14 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (13.34 KB, patch)
2013-10-30 12:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - QA Followup (4.56 KB, patch)
2013-10-30 12:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011: Update GPL license (1.55 KB, patch)
2013-10-30 12:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation (8.70 KB, patch)
2014-03-05 16:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (6.32 KB, patch)
2014-03-05 16:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation (8.67 KB, patch)
2014-03-05 16:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (6.32 KB, patch)
2014-03-05 16:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation (8.67 KB, patch)
2014-03-05 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (6.13 KB, patch)
2014-03-05 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory (1.31 KB, patch)
2014-03-05 16:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory (1.31 KB, patch)
2014-03-05 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation (8.67 KB, patch)
2014-06-18 11:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (6.13 KB, patch)
2014-06-18 11:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory (1.31 KB, patch)
2014-06-18 11:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup] (917 bytes, patch)
2014-06-26 18:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 2] - Add message to OPAC (2.64 KB, patch)
2014-07-16 12:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 2] - Add message to OPAC (3.05 KB, patch)
2014-07-16 12:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 2] - Add message to OPAC (3.05 KB, patch)
2014-07-16 12:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 2] - Add message to OPAC (4.03 KB, patch)
2014-07-21 14:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 3] - Update database schema files (3.31 KB, patch)
2014-07-21 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 4] - Add issue_id to issues tables (1.67 KB, patch)
2014-08-19 11:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation (8.74 KB, patch)
2014-08-19 20:17 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9011 - Add the ability to store the last patron to return an item (6.21 KB, patch)
2014-08-19 20:19 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory (1.37 KB, patch)
2014-08-19 20:19 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9011 [QA Followup] (983 bytes, patch)
2014-08-19 20:19 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9011 [QA Followup 2] - Add message to OPAC (4.12 KB, patch)
2014-08-19 20:19 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9011 [QA Followup 3] - Update database schema files (3.38 KB, patch)
2014-08-19 20:19 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9011 [QA Followup 4] - Add issue_id to issues tables (1.74 KB, patch)
2014-08-19 20:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation (8.72 KB, patch)
2014-10-16 16:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (6.19 KB, patch)
2014-10-16 16:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory (1.37 KB, patch)
2014-10-16 16:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup] (971 bytes, patch)
2014-10-16 16:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 2] - Add message to OPAC (3.39 KB, patch)
2014-10-16 16:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 3] - Update database schema files (2.61 KB, patch)
2014-10-16 16:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 4] - Add issue_id to issues tables (1.70 KB, patch)
2014-10-16 16:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation (8.72 KB, patch)
2014-10-16 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (6.19 KB, patch)
2014-10-16 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory (1.37 KB, patch)
2014-10-16 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup] (971 bytes, patch)
2014-10-16 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 2] - Add message to OPAC (3.39 KB, patch)
2014-10-16 16:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 3] - Update database schema files (2.61 KB, patch)
2014-10-16 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 4] - Add issue_id to issues tables (1.70 KB, patch)
2014-10-16 16:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 5] - Depend on bug 9303 for issue_id (1.35 KB, patch)
2014-10-30 15:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation (8.73 KB, patch)
2015-03-23 11:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (6.19 KB, patch)
2015-03-23 11:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory (1.37 KB, patch)
2015-03-23 11:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup] (971 bytes, patch)
2015-03-23 11:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 2] - Add message to OPAC (3.39 KB, patch)
2015-03-23 11:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation (8.75 KB, patch)
2015-04-23 17:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (6.33 KB, patch)
2015-04-23 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory (1.37 KB, patch)
2015-04-23 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup] (971 bytes, patch)
2015-04-23 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 2] - Add message to OPAC (3.39 KB, patch)
2015-04-23 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation (8.75 KB, patch)
2015-06-03 18:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add the ability to store the last patron to return an item (5.91 KB, patch)
2015-06-03 18:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory (1.37 KB, patch)
2015-06-03 18:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup] (971 bytes, patch)
2015-06-03 18:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9011 [QA Followup 2] - Add message to OPAC (3.39 KB, patch)
2015-06-03 18:20 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-11-05 18:27:29 UTC
Currently if the AnonymousPatron system preference is in use, all patron data is
anonymized. Some libraries would like to be able to see the last patron who
returned out an item ( in case of damage ) but still keep all other patrons anonymized.

* Add the field items.last_borrower, foreign key to borrowers.borrowernumber
* Add new system preference StoreLastBorrower
* If StoreLastBorrower is enabled, upon checkin have Koha set this new field to the patron who last returned this item.
* If this new field has a value, link to the patron from the item details page.
Comment 1 Kyle M Hall 2012-11-05 19:42:52 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-11-05 19:46:27 UTC
Test plan:
1) Apply patch
2) Run updatedatabase.pl
3) Enable StoreLastBorrower
4) Issue an item to a patron and return said item
5) Issue the same item to a second patron, do not return it.
6) View moredetail.pl for the given bib, find the given item. There should be a new field in the history list 'Last returned by' with a link to the given borrowernumber.

Optionally, you can also verify this works even if patron issuing history has been set to anonymize issues upon return.
Comment 3 Owen Leonard 2013-02-06 16:44:01 UTC
This passes your test plan perfectly, but I see one minor problem: On moredetail.pl it shows the "last returned by" as a borrowernumber, whereas the "last borrower" and "previous borrower" use cardnumber. I think these should be consistent.
Comment 4 Kyle M Hall 2013-02-06 18:10:44 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2013-02-08 20:26:29 UTC Comment hidden (obsolete)
Comment 6 Paul Poulain 2013-03-21 15:37:12 UTC
QA comment: I haven't tested this patch yet, but:
 1 koha-qa.pl work fine
 2 shouldn't we warn users on opac-privacy page that their information will kept as long as they're the last checking-out the book, in case of a damaged book ?
 3 should we keep the information limitless ? In France, I think it would be illegal, as all libraries must use tools/cleanborrowers.pl. Shouldn't we add an option in this script to remove information after a specific period, even if the patron is still the last borrower ? 

will update a rebased patch (updatedatabase conflicts), but not changing patch status, until i've answers to Q2 and Q3
Comment 7 Paul Poulain 2013-03-21 15:37:40 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2013-03-22 10:55:29 UTC
(In reply to comment #6)
> QA comment: I haven't tested this patch yet, but:

>  2 shouldn't we warn users on opac-privacy page that their information will
> kept as long as they're the last checking-out the book, in case of a damaged
> book ?

I think we can do this. It seems like a good and reasonable idea.

>  3 should we keep the information limitless ? In France, I think it would be
> illegal, as all libraries must use tools/cleanborrowers.pl. Shouldn't we add
> an option in this script to remove information after a specific period, even
> if the patron is still the last borrower ? 

This I don't think will work, as the data never expiring what this is all about! This feature is not enabled by default so I don't think that should be a problem, right?

> will update a rebased patch (updatedatabase conflicts), but not changing
> patch status, until i've answers to Q2 and Q3

Sounds good. I'll post a followup asap.
Comment 9 Kyle M Hall 2013-03-22 11:02:28 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2013-03-22 11:15:37 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-05-30 13:21:50 UTC
QA comment:
I found an possible issue:
If the borrowernumber stored in the new field items.last_returned_by is deleted, the dbms raises an error:

DBD::mysql::st execute failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha_coderun`.`items`, CONSTRAINT `items_ibfk_4` FOREIGN KEY (`last_returned_by`) REFERENCES `borrowers` (`borrowernumber`)) at /home/koha/src/C4/Members.pm line 1819.

You should add a "on delete cascade" option (set null ?) or remove this constraint.
It is a dilemma: in one case we loose the information, in other one we loose the constraint :-/

Marked as Failed QA.
Comment 12 Paul Poulain 2013-05-30 13:28:41 UTC
(In reply to comment #11)
> You should add a "on delete cascade" option (set null ?) or remove this
> constraint.
> It is a dilemma: in one case we loose the information, in other one we loose
> the constraint :-/
Not cascade, but set null, I don't see the dilemna: with cascade, you delete the item if the borrower is deleted. This is bad :\
Comment 13 Jonathan Druart 2013-05-30 13:38:27 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > You should add a "on delete cascade" option (set null ?) or remove this
> > constraint.
> > It is a dilemma: in one case we loose the information, in other one we loose
> > the constraint :-/
> Not cascade, but set null, I don't see the dilemna: with cascade, you delete
> the item if the borrower is deleted. This is bad :\

Yes, not cascade, of course.
But with set NULL, we loose the information. If the borrowernumber is kept in the last_returned_by field, we can retrieve it using the deletedborrowers table.
Comment 14 Kyle M Hall 2013-06-05 14:10:59 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-08-08 18:38:49 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2013-08-08 18:39:47 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-08-14 09:29:08 UTC
QA comment:

1/ The screen is not obvious:

  Last borrowed:14/08/2013 
  Last returned by: 23529001223640 
  Last borrower: 23529001223640 
  Previous borrower: 23529001223641

The "last returned by" and the "last borrower" is the same value when AnonymousPatron is not in use.

2/ Could you use the template plugin to get the syspref value (rather than to pass it from the pl script).

Marked as Failed QA.
Comment 18 Kyle M Hall 2013-08-14 13:47:38 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2013-08-14 15:14:28 UTC
Last patch fixes the issues. Thanks Kyle.
Marked as Passed QA.
Comment 20 Jonathan Druart 2013-08-14 15:14:48 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2013-08-14 15:14:55 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2013-08-14 15:15:00 UTC Comment hidden (obsolete)
Comment 23 Galen Charlton 2013-08-19 15:34:23 UTC
(In reply to Kyle M Hall from comment #8)
> >  3 should we keep the information limitless ? In France, I think it would be
> > illegal, as all libraries must use tools/cleanborrowers.pl. Shouldn't we add
> > an option in this script to remove information after a specific period, even
> > if the patron is still the last borrower ? 
> 
> This I don't think will work, as the data never expiring what this is all
> about! This feature is not enabled by default so I don't think that should
> be a problem, right?

There's a difference between keeping a link to the last borrower for a longer period of time and keeping it forever.  Consequently, I agree with Paul's suggestion that cleanborrowers.pl should have an option to clear issues.last_returned_by after a specified amount of time.

However, since the new feature is not on by default, I do not consider this a blocker for pushing the patch.
Comment 24 Galen Charlton 2013-08-19 16:13:09 UTC
Comment on attachment 20343 [details] [review]
Bug 9011 - QA Followup

Review of attachment 20343 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt
@@ +214,4 @@
>  
>                  <li><span class="label">Last seen:</span>[% IF ( ITEM_DAT.datelastseen ) %][% ITEM_DAT.datelastseen | $KohaDates %] [%END %]&nbsp;</li>
>                  <li><span class="label">Last borrowed:</span>[% IF (ITEM_DAT.datelastborrowed ) %][% ITEM_DAT.datelastborrowed | $KohaDates %][% END %]&nbsp;</li>
> +                [% IF ( ITEM_DAT.last_returned_by && Koha.Preference('StoreLastBorrower') && !Koha.Preference('AnonymousPatron') ) %]<li><span class="label">Last returned by:</span> <a href="/cgi-bin/koha/circ/circulation.pl?borrowernumber=[% ITEM_DAT.last_returned_by %]">[% KohaBorrowers.BorrowernumberToCardnumber( ITEM_DAT.last_returned_by ) %]</a>&nbsp;</li>[% END %]

Is "Koha.Preference('AnonymousPatron')" what is meant, not "!Koha.Preference('AnonymousPatron')"?

The AnonymousPatron system preference is not a Boolean, it's the borrowernumber of the record to which anonymized transactions should be linked.

Consequently, I think the QA follow-up has exactly the opposite of the intended effect.
Comment 25 Galen Charlton 2013-08-19 16:16:50 UTC
Setting to In Discussion and requesting a response from Jonathan to the question I asked in my previous comment.

Broader question: do we really need another field in the items table?  Why not just tweak AnonymiseIssueHistory() so that if StoreLastBorrower is set, old_issue rows that are the most recent return for their items are not anonymized?
Comment 26 Jonathan Druart 2013-08-20 07:18:43 UTC
(In reply to Galen Charlton from comment #24)
> Comment on attachment 20343 [details] [review] [review]
> Bug 9011 - QA Followup
> 
> Review of attachment 20343 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt
> @@ +214,4 @@
> >  
> >                  <li><span class="label">Last seen:</span>[% IF ( ITEM_DAT.datelastseen ) %][% ITEM_DAT.datelastseen | $KohaDates %] [%END %]&nbsp;</li>
> >                  <li><span class="label">Last borrowed:</span>[% IF (ITEM_DAT.datelastborrowed ) %][% ITEM_DAT.datelastborrowed | $KohaDates %][% END %]&nbsp;</li>
> > +                [% IF ( ITEM_DAT.last_returned_by && Koha.Preference('StoreLastBorrower') && !Koha.Preference('AnonymousPatron') ) %]<li><span class="label">Last returned by:</span> <a href="/cgi-bin/koha/circ/circulation.pl?borrowernumber=[% ITEM_DAT.last_returned_by %]">[% KohaBorrowers.BorrowernumberToCardnumber( ITEM_DAT.last_returned_by ) %]</a>&nbsp;</li>[% END %]
> 
> Is "Koha.Preference('AnonymousPatron')" what is meant, not
> "!Koha.Preference('AnonymousPatron')"?
> 
> The AnonymousPatron system preference is not a Boolean, it's the
> borrowernumber of the record to which anonymized transactions should be
> linked.
> 
> Consequently, I think the QA follow-up has exactly the opposite of the
> intended effect.

If the pref AnonymousPatron is set to 0, that means that the feature is disabled. Else, it contains a borrowernumber and the feature is enabled.
So !Koha.Preference('AnonymousPatron') means: the feature is enabled, we want to display the "last returned by" value.
Comment 27 Jonathan Druart 2013-08-20 07:25:39 UTC
(In reply to Galen Charlton from comment #25)
> Setting to In Discussion and requesting a response from Jonathan to the
> question I asked in my previous comment.
> 
> Broader question: do we really need another field in the items table?  Why
> not just tweak AnonymiseIssueHistory() so that if StoreLastBorrower is set,
> old_issue rows that are the most recent return for their items are not
> anonymized?

If anonymouspatron is enabled, C4::Circ::MarkIssueReturn can change the borrowernumber with the anonymouspatron value. So the information does not exist in the old_issues table.
Comment 28 Kyle M Hall 2013-10-30 12:03:11 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2013-10-30 12:03:23 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2013-10-30 12:03:36 UTC Comment hidden (obsolete)
Comment 31 Paul Poulain 2013-10-30 15:14:59 UTC
(In reply to Galen Charlton from comment #25)
> Broader question: do we really need another field in the items table?  Why
> not just tweak AnonymiseIssueHistory() so that if StoreLastBorrower is set,
> old_issue rows that are the most recent return for their items are not
> anonymized?

The more I think of it, the less I like this patch : it introduces a duplicate information in our database: the last patron is in old_issues and in items.last_borrower

I agree that the old_issues information can be removed by anonymisation. But a better option, imho, would be to improve anonymization, to have something like:
 * anonymize
 * anonymize except the last patron
 * don't anonymize

However, I'm not the RM, so Galen final cut. But I really think that, on the long term, this patch introduces some inconsistencies, and thus, pain.

(Sorry not to have written this earlier)
Comment 32 Galen Charlton 2013-11-15 16:34:08 UTC
(In reply to Paul Poulain from comment #31)
> I agree that the old_issues information can be removed by anonymisation. But
> a better option, imho, would be to improve anonymization, to have something
> like:
>  * anonymize
>  * anonymize except the last patron
>  * don't anonymize

I prefer this approach as well, though I think I've heard of cases where some libraries might want the second option to be "anonymize all except the last N patrons", where N is set by the library.

Kyle, Paul, Jonathan: do any of you have the wherewithal to run with that idea for 3.16?
Comment 33 Jonathan Druart 2013-11-28 10:17:21 UTC
(In reply to Galen Charlton from comment #32)
> Kyle, Paul, Jonathan: do any of you have the wherewithal to run with that
> idea for 3.16?

I don't plan to work on that.
Comment 34 Katrin Fischer 2014-01-06 12:54:49 UTC
I have taken a longer look at this and have been trying to make up my mind.
I also looked at the code and did some testing.

- I agree that adding one more place that has to be taken into account when trying to anonymize the reading history seems problematic. We already have the information in statistics, action_logs and old_issues.

- Choosing to build this on the existing anonymizing scripts would give us more flexibility in the future. Like Galen suggested, it would be possible to keep the "last n borrowers".

- There is currently no automatic way to delete this information from items/deleteditems once you choose to deactivate the feature. This worries me a bit.

Some general thoughts: When we discussed this kind of problem at work, there was a point made that the data is not useful after a certain amount of time. Can you blame a borrower for any damage you find after accepting the item at the circulation desk or reshelving it after a selfcheck return? As we are only allowed to keep data that is actually needed, it would be good if there was an option to delete after x days.

About the code:
1) Database update needs to be '15' and contain the bug number.
2) Diacritics in the card number on the items tab get mangled.
3) I think the syspref description could be improved a bit. I didn't understand at first what was meant by "This setting is independent of opacreadinghistory/AnonymousPatron". I think something like "the data will be stored independent on your settings for..." might be a bit more clear and also it might be good to warn that the data will be kept for older items/issues when turning it off again.
Comment 35 Kyle M Hall 2014-03-05 16:25:57 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2014-03-05 16:26:07 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2014-03-05 16:28:27 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2014-03-05 16:28:36 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2014-03-05 16:30:31 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2014-03-05 16:30:44 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2014-03-05 16:45:39 UTC Comment hidden (obsolete)
Comment 42 Kyle M Hall 2014-03-05 16:48:03 UTC
This new patch set re-engineers the entire feature. It ditches the new fields and instead simply retains the borrowernumber for the last old_issue for each patron, even if the would have been anonymized. Upon the next return of the item, that patron is then anonymized if the patron's privacy is set to 2.
Comment 43 Kyle M Hall 2014-03-05 16:48:51 UTC Comment hidden (obsolete)
Comment 44 Christopher Brannon 2014-06-11 20:42:36 UTC
Kyle, do you have a new test plan, or is the old one still the one to go with?

Christopher
Comment 45 Christopher Brannon 2014-06-11 20:58:50 UTC
Kyle,
Ran into problems applying this patch to sandbox:

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 9011:
<h1>Something went wrong !</h1>Applying: Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation
fatal: sha1 information is lacking or useless (Koha/Schema/Result/OldIssue.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation 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.
Bug 9011 - Add the ability to store the last patron to return an item

25870 - Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation
25871 - Bug 9011 - Add the ability to store the last patron to return an item
25873 - Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-9011---Re-engineer-circ-anonymization-move-to--EetlgO.patch .


Christopher
Comment 46 Kyle M Hall 2014-06-18 11:14:19 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2014-06-18 11:14:38 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2014-06-18 11:14:58 UTC Comment hidden (obsolete)
Comment 49 Christopher Brannon 2014-06-19 19:42:29 UTC
On master when I turn on the AnonymousPatron preference, I can check items in.  With patch applied and AnonymousPatron on, I get Error 500 screens on check in.  Items won't check in with patch.  This is true checking in from moremember.pl or circulation.pl from the list of items checked out, or from returns.pl.

Christopher
Comment 50 Kyle M Hall 2014-06-25 12:40:45 UTC
This bug depends on bug 9303, the patches for that bug must be applied prior to testing this patch set.

(In reply to Christopher Brannon from comment #49)
> On master when I turn on the AnonymousPatron preference, I can check items
> in.  With patch applied and AnonymousPatron on, I get Error 500 screens on
> check in.  Items won't check in with patch.  This is true checking in from
> moremember.pl or circulation.pl from the list of items checked out, or from
> returns.pl.
> 
> Christopher
Comment 51 Nick Clemens 2014-06-25 15:37:46 UTC
After applying both patches and updating the database the feature seems to work as expected, however, when I log in to any patron account on the opac and click the 'immediate deletion' button under 'your privacy' I get the error:
"The deletion of your reading history failed, because there is a problem with the configuration of this feature. Please help to fix the system by informing your library of this error."

I double checked my system settings without the patches and immediate deletion works.
Comment 52 Kyle M Hall 2014-06-26 18:55:17 UTC Comment hidden (obsolete)
Comment 53 Nick Clemens 2014-06-27 18:34:22 UTC
I tested again, feature works, immediate deletion works as I think it should with this feature (i.e. Any items where last patron is being kept are not removed from history) but it does seem like when this preference is active there should be an explanation on the general privacy page and in the success message from immediate deletion to explain to the patron why everything may not be cleared even if they have privacy set to 'never' and hit immediate deletion.
Comment 54 Kyle M Hall 2014-07-16 12:05:52 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2014-07-16 12:11:12 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2014-07-16 12:12:53 UTC Comment hidden (obsolete)
Comment 57 Nick Clemens 2014-07-16 23:23:57 UTC
I tested but am not seeing history stored if patron privacy preference is set to 'never'

Patron also seems to be able to clear last borrower history using 'Immediate deletion' button

Bug says depends on 11891, but that was pushed to master, wanted to check before clearing though.
Comment 58 Kyle M Hall 2014-07-21 14:48:47 UTC Comment hidden (obsolete)
Comment 59 Kyle M Hall 2014-07-21 15:33:50 UTC Comment hidden (obsolete)
Comment 60 Kyle M Hall 2014-07-21 15:37:07 UTC
(In reply to Nick Clemens from comment #57)
> I tested but am not seeing history stored if patron privacy preference is
> set to 'never'
> Patron also seems to be able to clear last borrower history using 'Immediate
> deletion' button

Please retest! I believe the issue was caused by the database schema not being updated.
 
> Bug says depends on 11891, but that was pushed to master, wanted to check
> before clearing though.

I've gone ahead and cleared that blocker.
Comment 61 Nick Clemens 2014-07-24 16:53:30 UTC
Hi Kyle,

I tried to test on a sandbox (3.17.00.009) and am getting Error 500 for any checkins

I was going to test on my VM (3.17.00.010) but patch needs rebased for current master
Comment 62 Kyle M Hall 2014-08-15 17:10:38 UTC
(In reply to Nick Clemens from comment #61)
> Hi Kyle,
> 
> I tried to test on a sandbox (3.17.00.009) and am getting Error 500 for any
> checkins
> 
> I was going to test on my VM (3.17.00.010) but patch needs rebased for
> current master

Not sure what's going on, the patches apply to master just fine for me (3.17.00.014).
Comment 63 Nick Clemens 2014-08-15 18:46:24 UTC
You are correct, looks like problem was on my end.  

I am getting a software error at check-in:
DBIx::Class::ResultSet::next(): Unknown column 'me.issue_id' in 'field list' at /home/nick/koha/C4/Circulation.pm line 3808

My steps:

1 - Applied to new VM with no errors
2 - Ran updatedatabase.pl - successful
3 - Enable StoreLastBorrower
4 - Check out an item to patron
5 - Attempt to check in item - error as above

If I checkout master I can return the items

Let me know if I am doing something wrong
Comment 64 Kyle M Hall 2014-08-19 11:32:41 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2014-08-19 11:34:10 UTC
Looks like I forgot to create the issue_id field with my db update! This latest followup should solve the problem.

(In reply to Nick Clemens from comment #63)
> You are correct, looks like problem was on my end.  
> 
> I am getting a software error at check-in:
> DBIx::Class::ResultSet::next(): Unknown column 'me.issue_id' in 'field list'
> at /home/nick/koha/C4/Circulation.pm line 3808
> 
> My steps:
> 
> 1 - Applied to new VM with no errors
> 2 - Ran updatedatabase.pl - successful
> 3 - Enable StoreLastBorrower
> 4 - Check out an item to patron
> 5 - Attempt to check in item - error as above
> 
> If I checkout master I can return the items
> 
> Let me know if I am doing something wrong
Comment 66 Nick Clemens 2014-08-19 20:17:33 UTC Comment hidden (obsolete)
Comment 67 Nick Clemens 2014-08-19 20:19:14 UTC Comment hidden (obsolete)
Comment 68 Nick Clemens 2014-08-19 20:19:24 UTC Comment hidden (obsolete)
Comment 69 Nick Clemens 2014-08-19 20:19:35 UTC Comment hidden (obsolete)
Comment 70 Nick Clemens 2014-08-19 20:19:44 UTC Comment hidden (obsolete)
Comment 71 Nick Clemens 2014-08-19 20:19:52 UTC Comment hidden (obsolete)
Comment 72 Nick Clemens 2014-08-19 20:20:04 UTC Comment hidden (obsolete)
Comment 73 Nick Clemens 2014-08-19 20:29:45 UTC
I tested with patrons using privacy settings of Forever, Never and Default.

In all cases history was stored as expected before enabling StoreLastBorrower and Immediate deletion button worked with no warnings

After enabling StoreLastBorrower 'immediate deletion' button offered warning that last returned items would not be deleted.

For patrons with privacy set to 'Never', items were anonymised as soon as returned by the next patron.  For others, items history was stored until patron clicked 'immediate deletion'

The 'Last borrower' link appeared in the items histories and was only anonymised upon return by next patron (for those with privacy 'Never') or upon forced deletion by patrons with other settings.

I did have one or two instances of an item getting stuck in history as I changed many settings, but could not recreate the issue purposefully.  In all cases either saving patron history again or checking the item out and back in cleared the issue.
Comment 74 Katrin Fischer 2014-10-10 02:32:19 UTC
Hi Kyle, I am having problems applying these patches - I fixed the conflicts in the second, but then can't get QA follow-up 2 to apply.
Comment 75 Kyle M Hall 2014-10-16 16:21:04 UTC Comment hidden (obsolete)
Comment 76 Kyle M Hall 2014-10-16 16:21:20 UTC Comment hidden (obsolete)
Comment 77 Kyle M Hall 2014-10-16 16:21:23 UTC Comment hidden (obsolete)
Comment 78 Kyle M Hall 2014-10-16 16:21:27 UTC Comment hidden (obsolete)
Comment 79 Kyle M Hall 2014-10-16 16:21:30 UTC Comment hidden (obsolete)
Comment 80 Kyle M Hall 2014-10-16 16:21:34 UTC Comment hidden (obsolete)
Comment 81 Kyle M Hall 2014-10-16 16:21:39 UTC Comment hidden (obsolete)
Comment 82 Kyle M Hall 2014-10-16 16:30:39 UTC Comment hidden (obsolete)
Comment 83 Kyle M Hall 2014-10-16 16:30:49 UTC Comment hidden (obsolete)
Comment 84 Kyle M Hall 2014-10-16 16:30:52 UTC Comment hidden (obsolete)
Comment 85 Kyle M Hall 2014-10-16 16:30:55 UTC Comment hidden (obsolete)
Comment 86 Kyle M Hall 2014-10-16 16:30:59 UTC Comment hidden (obsolete)
Comment 87 Kyle M Hall 2014-10-16 16:31:02 UTC Comment hidden (obsolete)
Comment 88 Kyle M Hall 2014-10-16 16:31:05 UTC Comment hidden (obsolete)
Comment 89 Katrin Fischer 2014-10-26 13:16:53 UTC
Working on this now.
Comment 90 Katrin Fischer 2014-10-26 19:21:06 UTC
Hi Kyle,

I will continue testing for now, but the new tests are failing for me:

1..6
ok 1 - use C4::Circulation;
ok 2 - use Koha::Database;
Use of uninitialized value in subroutine entry at /home/katrin/kohaclone/C4/Charset.pm line 181.
DBD::mysql::st execute failed: Duplicate entry '8' for key 'PRIMARY' at /home/katrin/kohaclone/C4/Circulation.pm line 2073.
DBD::mysql::st execute failed: Duplicate entry '8' for key 'PRIMARY' at /home/katrin/kohaclone/C4/Circulation.pm line 2073.
# Looks like you planned 6 tests but ran 2.
# Looks like your test exited with 255 just after 2.
Comment 91 Katrin Fischer 2014-10-26 20:08:47 UTC
Hi Kyle, 

there is something going wrong for me:
Patron: store never
StoreLastPatron: Yes
OpacPrivacy: Yes
opacreadinghistory: Yes
AnonymousPatron: set to borrowernumber

I check out a book and check it back in.

Expectation: The table old_issues should show a new entry with the patron's borrowernumber.

But: there is no entry in old_issues...

Instead I have a DB error in the logs:
[Sun Oct 26 17:03:20.914868 2014] [cgi:error] [pid 28492] [client 127.0.0.1:41501] AH01215: [Sun Oct 26 21:03:20 2014] checkin: DBD::mysql::st execute failed: Duplicate entry '15' for key 'PRIMARY' at /home/katrin/kohaclone/C4/Circulation.pm line 2072., referer: http://localhost:8080/cgi-bin/koha/circ/circulation.pl

I think the way the new issue_id is set does not work correctly. The new issue gets 16 - while I already have ids way above 30 in both tables.
After the update it looked ok, but it's not working right for new ids.
Comment 92 Katrin Fischer 2014-10-26 20:20:41 UTC
I tried it again... restored my database pre-patch, ran the update again, new checkout = issue_id = 4 :( Should have been 27.

I have a feeling of de ja vu... didn't we fight this problem on another patch? Looks like bug 9303 is also adding an issue_id.
Comment 93 Kyle M Hall 2014-10-30 15:03:03 UTC Comment hidden (obsolete)
Comment 94 Kyle M Hall 2014-10-30 15:04:29 UTC
You are correct! And it does a much better job of it too. I'm making bug 9303 a dependency for this bug.

(In reply to Katrin Fischer from comment #92)
> I tried it again... restored my database pre-patch, ran the update again,
> new checkout = issue_id = 4 :( Should have been 27.
> 
> I have a feeling of de ja vu... didn't we fight this problem on another
> patch? Looks like bug 9303 is also adding an issue_id.
Comment 95 Katrin Fischer 2014-10-30 19:55:27 UTC
This is now blocked by 9303 database update problems.
Comment 96 Kyle M Hall 2015-03-23 11:59:19 UTC Comment hidden (obsolete)
Comment 97 Kyle M Hall 2015-03-23 11:59:29 UTC Comment hidden (obsolete)
Comment 98 Kyle M Hall 2015-03-23 11:59:32 UTC Comment hidden (obsolete)
Comment 99 Kyle M Hall 2015-03-23 11:59:44 UTC Comment hidden (obsolete)
Comment 100 Kyle M Hall 2015-03-23 11:59:48 UTC Comment hidden (obsolete)
Comment 101 Katrin Fischer 2015-04-22 23:20:03 UTC
Hi Kyle,

this doesn't apply on top of 13790 or master:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation
fatal: sha1 information is lacking or useless (Koha/Schema/Result/OldIssue.pm).
Comment 102 Kyle M Hall 2015-04-23 17:46:59 UTC Comment hidden (obsolete)
Comment 103 Kyle M Hall 2015-04-23 17:47:07 UTC Comment hidden (obsolete)
Comment 104 Kyle M Hall 2015-04-23 17:47:12 UTC Comment hidden (obsolete)
Comment 105 Kyle M Hall 2015-04-23 17:47:15 UTC Comment hidden (obsolete)
Comment 106 Kyle M Hall 2015-04-23 17:47:18 UTC Comment hidden (obsolete)
Comment 107 Katrin Fischer 2015-05-15 22:00:08 UTC
*** Bug 13831 has been marked as a duplicate of this bug. ***
Comment 108 Jonathan Druart 2015-05-26 14:07:02 UTC
Applying: Bug 9011 - Add the ability to store the last patron to return an item
fatal: sha1 information is lacking or useless (C4/Circulation.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 9011 - Add the ability to store the last patron to return an item
Comment 109 Kyle M Hall 2015-06-03 18:20:18 UTC
Created attachment 39812 [details] [review]
Bug 9011 - Re-engineer circ anonymization, move to subroutine in C4::Circulation

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 110 Kyle M Hall 2015-06-03 18:20:36 UTC
Created attachment 39813 [details] [review]
Bug 9011 - Add the ability to store the last patron to return an item

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 111 Kyle M Hall 2015-06-03 18:20:40 UTC
Created attachment 39814 [details] [review]
Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 112 Kyle M Hall 2015-06-03 18:20:44 UTC
Created attachment 39815 [details] [review]
Bug 9011 [QA Followup]

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 113 Kyle M Hall 2015-06-03 18:20:48 UTC
Created attachment 39816 [details] [review]
Bug 9011 [QA Followup 2] - Add message to OPAC

* Adds warning to OPAC if StoreLastBorrower is enabled
* Updates db revision
* Fixes POD error

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 114 Jonathan Druart 2015-06-19 13:10:42 UTC
Comment on attachment 39815 [details] [review]
Bug 9011 [QA Followup]

Review of attachment 39815 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +3198,5 @@
>      }
>  
>      if ( C4::Context->preference('StoreLastBorrower') ) {
> +        # Do not delete the newest old_issue for any itemnumber
> +        $query .= " AND issue_id NOT IN ( SELECT issue_id FROM ( SELECT * FROM old_issues ORDER BY issue_id DESC ) AS oi GROUP BY itemnumber ) ";

I think that the NOT IN clause should be avoid here.
It can cause the query to be very slow.
See bug 13740.
Comment 115 Kyle M Hall 2015-06-29 15:09:54 UTC
(In reply to Jonathan Druart from comment #114)
> Comment on attachment 39815 [details] [review] [review]
> Bug 9011 [QA Followup]
> 
> Review of attachment 39815 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Circulation.pm
> @@ +3198,5 @@
> >      }
> >  
> >      if ( C4::Context->preference('StoreLastBorrower') ) {
> > +        # Do not delete the newest old_issue for any itemnumber
> > +        $query .= " AND issue_id NOT IN ( SELECT issue_id FROM ( SELECT * FROM old_issues ORDER BY issue_id DESC ) AS oi GROUP BY itemnumber ) ";
> 
> I think that the NOT IN clause should be avoid here.
> It can cause the query to be very slow.
> See bug 13740.

As far as I can tell there is no way around this. I tried to use a left join but you cannot use a group by in an update statement.

Here is my proof of concept diff that doesn't work, for posterity:

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index b174a12..bc3e28e 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -3183,11 +3183,15 @@ sub AnonymiseIssueHistory {
     my $date           = shift;
     my $borrowernumber = shift;
     my $dbh            = C4::Context->dbh;
-    my $query          = "
-        UPDATE old_issues
-        SET    borrowernumber = ?
-        WHERE  returndate < ?
-          AND borrowernumber IS NOT NULL
+
+    my $StoreLastBorrower = C4::Context->preference('StoreLastBorrower');
+
+    my $query = "UPDATE old_issues";
+    $query .= " LEFT JOIN old_issues last_checkout_for_item USING ( itemnumber )" if $StoreLastBorrower;
+    $query .= "
+        SET    old_issues.borrowernumber = ?
+        WHERE  old_issues.returndate < ?
+          AND  old_issues.borrowernumber IS NOT NULL
     ";

     # The default of 0 does not work due to foreign key constraints
@@ -3196,16 +3200,18 @@ sub AnonymiseIssueHistory {
     my @bind_params = ($anonymouspatron, $date);

     if (defined $borrowernumber) {
-       $query .= " AND borrowernumber = ?";
+       $query .= " AND old_issues.borrowernumber = ?";
        push @bind_params, $borrowernumber;
     } else {
        $query .= " AND (SELECT privacy FROM borrowers WHERE borrowers.borrowernumber=old_issues.borrowernumber) <> 0";
     }

-    if ( C4::Context->preference('StoreLastBorrower') ) {
-        # Do not delete the newest old_issue for any itemnumber
-        $query .= " AND issue_id NOT IN ( SELECT issue_id FROM ( SELECT * FROM old_issues ORDER BY issue_id DESC ) AS oi GROUP BY itemnumber ) ";
-    }
+    # Do not delete the newest old_issue for any itemnumber
+    $query .= "
+        AND old_issues.issue_id <> last_checkout_for_item.issue_id
+        GROUP BY last_checkout_for_item.itemnumber
+        ORDER BY last_checkout_for_item.issue_id DESC
+    " if $StoreLastBorrower;

     my $sth = $dbh->prepare($query);
     $sth->execute(@bind_params);
Comment 116 Kyle M Hall 2015-06-29 15:12:55 UTC
Jonathan, given this, I would be more than happy to add a warning to the system preference about possible performance issues. The other option would be to go back to my original patch set where the last patron to checkout an item was stored in a new database column. That would give us the same feature without the performance issue.

Please let me know which choice you think is best:
A) Add performance warning to syspref
B) Switch back to the original patch set ( using a new db column to store the last borrower to check out an item )

Thanks!
Comment 117 Nick Clemens 2015-06-30 17:19:56 UTC
I tried testing the query as a report on our test system (where old_issues has 3+ years of data for 50+ libraries) and can't get results, report times out.

I think the performance concern here is great enough that it's not a feasible method.  My vote would be for option B
Comment 118 Jonathan Druart 2015-07-02 10:18:26 UTC
(In reply to Kyle M Hall from comment #116)
> B) Switch back to the original patch set ( using a new db column to store
> the last borrower to check out an item )

It seems that at least 3 persons (Paul, Galen and Katrin) have raised concerns about it, so I'm not sure it's a good idea.
But it would be great to get their opinions.
Comment 119 Jonathan Druart 2015-07-09 13:18:10 UTC
(In reply to Kyle M Hall from comment #115)
> As far as I can tell there is no way around this. I tried to use a left join
> but you cannot use a group by in an update statement.

Kyle,
Just a quick thought: wouldn't it be possible to execute another update statement after the existing one, instead of adding complexity to it?
Comment 120 Katrin Fischer 2015-07-19 22:52:43 UTC
I'd really like to avoid us storing patron related information in the items table and potentially double the information to yet another place. It would be great if more people could take a look here to help figure out if there is a "fast" way of doing this with the existing data.

What about Jonathan's suggestion in comment#119?
Comment 121 Kyle M Hall 2015-08-07 13:44:03 UTC
(In reply to Jonathan Druart from comment #119)
> (In reply to Kyle M Hall from comment #115)
> > As far as I can tell there is no way around this. I tried to use a left join
> > but you cannot use a group by in an update statement.
> 
> Kyle,
> Just a quick thought: wouldn't it be possible to execute another update
> statement after the existing one, instead of adding complexity to it?

I do not believe this is possible. The addition isn't adding additional changes, it's *preventing* additional changes. We can't create a second query that prevents updates caused by the first one.

I think we have only two choices here, either accept the speed cost or switch back to the original version. I have no problem with either choice, but one must be made. We can't hold this feature in discussion forever. This is not complicated as a choice, especially since it seems that the speed cost is unacceptable.
Comment 122 Kyle M Hall 2015-08-07 13:46:30 UTC
(In reply to Katrin Fischer from comment #120)
> I'd really like to avoid us storing patron related information in the items
> table and potentially double the information to yet another place. It would
> be great if more people could take a look here to help figure out if there
> is a "fast" way of doing this with the existing data.

Katrin, can you explain the justification for not wanting option B aside from the fact that it's storing data in another place? What are your actual concerns about this practice?
Comment 123 Katrin Fischer 2015-08-07 13:52:11 UTC
Basically a big pile of paperwork where I have to document every table that has in any form a connection to patron data and the duplication of the information. Also the possibility that this will leak out into the OPAC with the other item data when someone maps it to a 952 field not thinking about the consequences.
Comment 124 Kyle M Hall 2015-08-07 14:22:55 UTC
(In reply to Katrin Fischer from comment #123)
> Basically a big pile of paperwork where I have to document every table that
> has in any form a connection to patron data and the duplication of the
> information. Also the possibility that this will leak out into the OPAC with
> the other item data when someone maps it to a 952 field not thinking about
> the consequences.

Thanks! I'm not sure we've any way around that. The connection won't be made unless the feature is specifically enabled. Likewise, this data would never be revealed in the opac unless some specifically map the column to a field. To accidentally reveal the data would definitely require jumping through some hoops, but we have no way to absolutely prevent such an occurrence. 

On the other hand I don't feel that this should be a blocker considering we could do the exact same thing in other areas of Koha. Consider how trivial it would be to create a patrons report and then mark it public? Just because that's possible doesn't mean we stopped the public reports feature from making it into Koha.

That being said, I'll defer to your judgement, but someone has to make a decision. Either we go with choice A or choice B.
Comment 125 Katrin Fischer 2015-08-09 22:42:56 UTC
I think I can totally imagine a library trying to make this visible in the editor and item view pages - which would require mapping it. And not being aware that the hidden fields (opac visibility most of all) are basically broken. Or not thinking about Z39.50, OAI-PMH... etc.

We also deactivated the feature to make reports public in the GUI for our customers because of privacy concerns.

I have no problem with the idea of this feature in itself - it's optional. I just think there might be a third option we haven't investigated yet. Or a way to improve performance. I am not sure I dare to suggest,... but what about a separate table? That wouldn't fix the duplicate information issue - so not ideal, but it could help fixing the other concerns.
Comment 126 HB-NEKLS 2015-08-10 22:40:36 UTC
I skimmed through the conversation, made it to the bottom, and am pondering Katrin's latest comment. 

What ABOUT a new table to store this data? That seems to be the simplest solution.

Something that is storing the fields id, borrowernumber, itemnumber, date, and "reason" or something like that, where the "reason" field or whatever it could be called is the reason for this data being stored -- "last borrower" in this case. And a field like that could open the door for this table to be used for a few other things. It could be used to store data like the statistics.type fieldd oes, that stores issue, return, renew, localuse data. 

Could something be written to delete this table for those countries/locations that need to anonymize all patron data, due to privacy concerns/laws; and then leave the data in this table for those libraries that need this data? 

And this may be way outside the scope of this conversation, but could a new table like this open the door to also be used [after future development] to store (staff) borrower data for tracking items adds/edits/deletes/branch transfers to items, using the above "reason" field? Because while this particular data is stored in the cataloging logs, my system is way too big to keep those logs turned on, and I'd love a way to be able to track that type of data. 

I propose this additional suggestion, so maybe this table could possibly take on additional uses, other than just the one way to balance privacy differences for those who want to keep last borrower data and anonymize the rest of the data vs those who can't keep any of these data due to local policies and laws. 

Will be curious to see where this conversation goes.
Comment 127 Christopher Brannon 2015-08-10 23:04:28 UTC
I LOVE the idea of having a separate table for this function.  Perhaps it can be designed in such a way that this function would have an option to store the last x patrons.  It could also have a timestamp and feature to remove the data if it is x days old.

It definitely should have the ability to be turned off if needed.

And to address what Heather suggested, yes, it could be used for other things, but I think having a tables for specific functions lends itself to a more efficient database rather than trying to design a table that encompasses many things.  I think the more a table tries to cover, makes the table bigger and less efficient.  There is more stuff to wade through.  Something that needs to be considered for any table in the database.  The more you throw in a table, the less effective the database, as a whole, becomes.

My Two Cents,
Christopher
Comment 128 Kim Gnerre 2015-08-11 14:31:39 UTC
We're just going through the migration process but we currently have that feature with Innovative's Millenium and it is helpful when something is returned damaged or something like a dvd case is returned without the dvd inside.  So, speaking for my Access Services Librarian, it would be a feature that we would be interested in having with our KOHA system.
Comment 129 Christopher Davis 2015-08-12 15:11:32 UTC
Dear Colleagues,

I am new to Bugzilla, so I hope that this comment will not overstep my bounds. In my opinion, this bug is a slippery slope on the mountain of professional ethics. I would rather error on the side of patron privacy and confidentiality than I would on the side of trying to recuperate lost or damaged items. If we say that we anonymize patron data, then we should go all the way without exception.

FWIW,

Christopher Davis
Comment 130 Nick Clemens 2015-08-12 19:50:23 UTC
My consortium sponsored this development, and it has taken some time to progress it this far forward. If MySQL performance is the only blocker right now I would really like to see a solution that addresses that and unsticks the bug rather than requiring a rewrite of the feature.

I played around with the query and the below, while still containing a NOT IN, eliminates the GROUP BY and improves speed significantly:
if ( C4::Context->preference('StoreLastBorrower') ) {
        # Do not delete the newest old_issue for any itemnumber
        $query .= " AND issue_id NOT IN ( SELECT issue_id FROM (Select oi.issue_id from old_issues oi LEFT OUTER JOIN old_issues oi2 ON oi.itemnumber=oi2.itemnumber AND oi2.issue_id>oi.issue_id WHERE oi.borrowernumber= ? AND oi2.borrowernumber IS NULL) AS oi ) ";
        push @bind_params, $borrowernumber;

Another option is to just use the query labeleed as 'oi' above in a separate update.  It effectively finds all the 'last returned' info for a borrower and would allow you to mark those somehow (say invert borrower number to negative of borrowernumber) then perform the regular anonymizing, and then update any rows with a negative borrowrnumber to a positive borrowernumber (or whatever marker was used)

I think Chris's comment #129 has been addressed, the feature is optional (syspref) and gives a big warning to the patron when it is enabled for full transparency. And I don't think sharing your opinion is overstepping :-) 

If the above queries don't work, a new table could work, though I do wonder about the redundancy.  I wonder if it would be possible to use the 'return' column in old_issues to mark 'lastreturned' or something similar to allow for easy SQL selection.
Comment 131 Katrin Fischer 2015-08-12 20:27:57 UTC
Thx you all for sharing your opinions and commenting!

Chris D. - you didn't overstep at all - please keep commenting on bugzila :)
Comment 132 Chris Rohde 2015-08-12 20:41:00 UTC
Another point to make is that if a library is using RFID/AMH (Automated Materials Handling), the materials are checked in before anyone has a chance to catch damages.  We don't charge for damages, unless an item is completely unusable.  However, when items such as a storytime kit go in, it jams the sorter.  In our case, we offer the anonymization option to our patrons, to honor their privacy choice.  I'd be fine with using a timestamp on the lastpatron table, and a chron job which slides along datewise, deleting returns older than 'x' days.
Comment 133 Chris Cormack 2015-08-12 20:43:29 UTC
(In reply to Christopher Davis from comment #129)
> Dear Colleagues,
> 
> I am new to Bugzilla, so I hope that this comment will not overstep my
> bounds. In my opinion, this bug is a slippery slope on the mountain of
> professional ethics. I would rather error on the side of patron privacy and
> confidentiality than I would on the side of trying to recuperate lost or
> damaged items. If we say that we anonymize patron data, then we should go
> all the way without exception.
> 
I agree with this, I think we should be aiming to have Koha being the most privacy respecting system that exists.  And that we should treat privacy issues like we treat security ones. They are the same thing.
Comment 134 Christopher Brannon 2015-08-12 20:49:08 UTC
(In reply to Chris Cormack from comment #133)
> (In reply to Christopher Davis from comment #129)
> > Dear Colleagues,
> > 
> > I am new to Bugzilla, so I hope that this comment will not overstep my
> > bounds. In my opinion, this bug is a slippery slope on the mountain of
> > professional ethics. I would rather error on the side of patron privacy and
> > confidentiality than I would on the side of trying to recuperate lost or
> > damaged items. If we say that we anonymize patron data, then we should go
> > all the way without exception.
> > 
> I agree with this, I think we should be aiming to have Koha being the most
> privacy respecting system that exists.  And that we should treat privacy
> issues like we treat security ones. They are the same thing.

It should not be Koha's job to enforce privacy rules.  Rules come and go, and vary from place to place.  It should be a system preference, not something forced by the system.  Koha needs to be able to handle multiple scenarios.

Christopher
Comment 135 Chris Cormack 2015-08-12 20:53:48 UTC
(In reply to Christopher Brannon from comment #134)

> 
> It should not be Koha's job to enforce privacy rules.  Rules come and go,
> and vary from place to place.  It should be a system preference, not
> something forced by the system.  Koha needs to be able to handle multiple
> scenarios.
> 

I'm saying the default should be privacy, and you have to opt in to not respecting privacy as a conscious choice. Which I think the suggestion of having a cron to delete it after x days, is a workable compromise.

The system does of course have to enforce it, but I agree with you that if a library wants to opt out of respecting privacy, they can do that by turning on the system preference.
Comment 136 Katrin Fischer 2015-08-13 07:30:53 UTC
(In reply to Nick Clemens from comment #130)
> My consortium sponsored this development, and it has taken some time to
> progress it this far forward. If MySQL performance is the only blocker right
> now I would really like to see a solution that addresses that and unsticks
> the bug rather than requiring a rewrite of the feature.
> 
> I played around with the query and the below, while still containing a NOT
> IN, eliminates the GROUP BY and improves speed significantly:
> if ( C4::Context->preference('StoreLastBorrower') ) {
>         # Do not delete the newest old_issue for any itemnumber
>         $query .= " AND issue_id NOT IN ( SELECT issue_id FROM (Select
> oi.issue_id from old_issues oi LEFT OUTER JOIN old_issues oi2 ON
> oi.itemnumber=oi2.itemnumber AND oi2.issue_id>oi.issue_id WHERE
> oi.borrowernumber= ? AND oi2.borrowernumber IS NULL) AS oi ) ";
>         push @bind_params, $borrowernumber;

I haven't tested if this works, but I am hopeful it does and improves speed. 

> Another option is to just use the query labeleed as 'oi' above in a separate
> update.  It effectively finds all the 'last returned' info for a borrower
> and would allow you to mark those somehow (say invert borrower number to
> negative of borrowernumber) then perform the regular anonymizing, and then
> update any rows with a negative borrowrnumber to a positive borrowernumber
> (or whatever marker was used)

This seems a bit hacky... but doesn't mean it couldn't work.

> If the above queries don't work, a new table could work, though I do wonder
> about the redundancy.  I wonder if it would be possible to use the 'return'
> column in old_issues to mark 'lastreturned' or something similar to allow
> for easy SQL selection.

Hm not sure how the return could be used - can you explain?

Kyle, could you take a look at these options?
Comment 137 Kyle M Hall 2015-08-24 11:04:00 UTC
I like this idea, but I think it won't work because it requires the borrowernumber to be known. Because of that, the last item checked out will not be retained if a library were to run misc/cronjobs/batch_anonymise.pl or tools/cleanborrowers.pl

(In reply to Nick Clemens from comment #130)
> My consortium sponsored this development, and it has taken some time to
> progress it this far forward. If MySQL performance is the only blocker right
> now I would really like to see a solution that addresses that and unsticks
> the bug rather than requiring a rewrite of the feature.
> 
> I played around with the query and the below, while still containing a NOT
> IN, eliminates the GROUP BY and improves speed significantly:
> if ( C4::Context->preference('StoreLastBorrower') ) {
>         # Do not delete the newest old_issue for any itemnumber
>         $query .= " AND issue_id NOT IN ( SELECT issue_id FROM (Select
> oi.issue_id from old_issues oi LEFT OUTER JOIN old_issues oi2 ON
> oi.itemnumber=oi2.itemnumber AND oi2.issue_id>oi.issue_id WHERE
> oi.borrowernumber= ? AND oi2.borrowernumber IS NULL) AS oi ) ";
>         push @bind_params, $borrowernumber;
> 
> Another option is to just use the query labeleed as 'oi' above in a separate
> update.  It effectively finds all the 'last returned' info for a borrower
> and would allow you to mark those somehow (say invert borrower number to
> negative of borrowernumber) then perform the regular anonymizing, and then
> update any rows with a negative borrowrnumber to a positive borrowernumber
> (or whatever marker was used)
> 
> I think Chris's comment #129 has been addressed, the feature is optional
> (syspref) and gives a big warning to the patron when it is enabled for full
> transparency. And I don't think sharing your opinion is overstepping :-) 
> 
> If the above queries don't work, a new table could work, though I do wonder
> about the redundancy.  I wonder if it would be possible to use the 'return'
> column in old_issues to mark 'lastreturned' or something similar to allow
> for easy SQL selection.
Comment 138 Kyle M Hall 2015-10-02 17:27:22 UTC

*** This bug has been marked as a duplicate of bug 14945 ***
Comment 139 Kyle M Hall 2015-10-02 17:28:08 UTC
I've moved this to a fresh bug for the new implementation that uses a separate table to track the last patron to return an item.
Comment 140 Christopher Brannon 2015-10-02 18:34:03 UTC
(In reply to Kyle M Hall from comment #139)
> I've moved this to a fresh bug for the new implementation that uses a
> separate table to track the last patron to return an item.

Yay!