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.

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.

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

(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.

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.

(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 :\

(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.

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.

Last patch fixes the issues. Thanks Kyle.
Marked as Passed QA.

(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 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.

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?

(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.

(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.

(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)

(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?

(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.

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.

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.

Kyle, do you have a new test plan, or is the old one still the one to go with?

Christopher

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

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

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

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.

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.

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.

(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.

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

(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).

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

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

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.

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.

Working on this now.

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.

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.

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.

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.

This is now blocked by 9303 database update problems.

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).

*** Bug 13831 has been marked as a duplicate of this bug. ***

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

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>

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>

Created attachment 39814 [details] [review]
Bug 9011 - Add support for StoreLastBorrower to AnonymiseIssueHistory

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Created attachment 39815 [details] [review]
Bug 9011 [QA Followup]

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

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 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.

(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);

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!

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

(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.

(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'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?

(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.

(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?

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.

(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.

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.

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.

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

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.

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

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.

Thx you all for sharing your opinions and commenting!

Chris D. - you didn't overstep at all - please keep commenting on bugzila :)

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.

(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.

(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

(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.

(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?

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.

*** This bug has been marked as a duplicate of bug 14945 ***

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.

(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!