Various workflows and circumstances can result in the "last returned by" value stored by StoreLastBorrower getting overwritten sooner than a library might like. For example, a patron may find the item they've checked out was damaged by the previous patron. They put the item in the bookdrop on their way into the library and then alert staff to the damage when they check out their next batch of items. By that time, the damaged item has been checked in, our imaginary patron shows as "last returned by," and we have no record there of who had it previously. We could alleviate this by adding a third option to the StoreLastBorrower system preference, so that one can choose "Do not store," "store 1," or "store 2." This entire feature would still be optional for libraries that don't want to keep this data. Additionally, bug 23260 is adding the ability to clear this data after a period of time using cleanup_database. These factors help mitigate the privacy concerns inherent to the matter.
From an accountability perspective, having the option for libraries to be able to "track" an item's [relatively recent] check-out history would be very helpful. We're not asking to breach anyone's privacy regarding their reading preferences; i.e., we're not asking to see a patron's reading history. We're merely trying to maintain a item's history (at least for a while) to determine accountability for damages. I like the idea of a scheduled clean-up database option (that can be determined by library as a cron job of sorts) as a method of making an effort to offer good-faith efforts of privacy. All this said, though, [our] items are government property, and we have an obligation to the taxpayers to be able to manage and track accountability of those items (at least for a window of time).
If we are extending from 1 to 2, why not extend to a defined limit or for a defined period of time? Makes it a bit more flexible?
We would love to see this feature. It has come up several times that the item was damaged by the 2nd-to last patron, and we have no record of who that was.
Here are two reports we use. They are effective unless the patron marks their account to not keep their history. Check-Outs in a date range 1 SELECT i.barcode as 'Item Barcode', 2 b.title as 'Title', 3 i.itemcallnumber as 'Item Call Number', 4 a.timestamp as 'Date and Time', 5 concat(borr.surname,", ",borr.firstname) as 'Patron Name', 6 borr.cardnumber as 'Patron Barcode' 7 FROM action_logs a 8 LEFT JOIN borrowers borr on (borr.borrowernumber=a.object) 9 LEFT JOIN items i on (i.itemnumber=a.info) 10 INNER JOIN biblio b on (b.biblionumber=i.biblionumber) 11 WHERE a.action='ISSUE' AND DATE(a.timestamp) BETWEEN <<Between (yyyy-mm-dd)|date>> AND <<and (yyyy-mm-dd)|date>> 12 ORDER BY borr.firstname asc Circulation History of an Item. NOTE: Must edit/save the SQL of the report with the target item’s barcode (in line 14 below). 1 SELECT 2 borrowers.cardnumber as "Cardnumber", 3 borrowers.othernames as "Othername", 4 borrowers.surname as "Surname", 5 items.itype, 6 items.barcode, 7 items.itemcallnumber as "Call Number", 8 date(statistics.datetime) as "Transaction Date", 9 statistics.type as "Transaction Type" 10 FROM borrowers 11 LEFT JOIN statistics on (statistics.borrowernumber=borrowers.borrowernumber) 12 LEFT JOIN items on (items.itemnumber = statistics.itemnumber) 13 LEFT JOIN biblioitems on (biblioitems.biblioitemnumber = items.biblioitemnumber) 14 WHERE items.barcode='21900004456' 15 ORDER BY statistics.datetime DESC
Created attachment 186773 [details] [review] Bug 26993: Database updates
Created attachment 186774 [details] [review] Bug 26993: Schema changes
Created attachment 186775 [details] [review] Bug 26993: System preference updates
Created attachment 186776 [details] [review] Bug 26993: Implement changes Test plan: 1. APPLY patch, restart_all, updatedatabase 2. Search for the StoreLastBorrower system preference, it should now allow you to enter a number of borrowers to store 3. Set the pref to 3. 4. Check an item out to any patron. Then immediately check it back in. 5. On catalogue/moredetail.pl?biblionumber=X ( where X is you biblionumber ) you should see a "Last returned by:" entry with the correct cardnumber for the borrower. 7. Confirm this is right in the database with: SELECT * FROM items_last_borrower where itemnumber = X; ( Where X is your itemnumber ) 8. Repeat step 4 to a different borrower. 9. Repeat steps 5 and 6. You should see 2 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X 10. Repeat step 4 with another new borrower. 11. Repeat steps 5 and 6. You should see 3 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X 12. Choose a different item and repeat steps 4 - 11 for this item. 13. Now set the StoreLastBorrower preference to 0. 14. Using the original item from step 4, check something out to a patron and immediately check it back in. 15. catalogue/moredetail.pl?biblionumber=X should NOT have an entry for "Last returned by:" 16. Check the database, all entries for the itemnumber from step 14 should be deleted.
Note: I removed the if ( C4::Context->preference('StoreLastBorrower') ) {} check from Circulation.pm The reason is because if the pref is off, I still want to delete any stored borrowers in the 'items_last_borrower', in the event that the system preference changed to 0 or nothing. I think it's important to delete these borrowers when things change and it is decided to NOT keep this history.
(In reply to Lucas Gass (lukeg) from comment #9) > The reason is because if the pref is off, I still want to delete any stored > borrowers in the 'items_last_borrower', in the event that the system > preference changed to 0 or nothing. > > I think it's important to delete these borrowers when things change and it > is decided to NOT keep this history. I agree, it is important that the data collection settings we define are consistent to what we actually store in the database. Out of scope for this bug but it would be nice if we had a mechanism for executing a task after a system preference has changed. But since we don't, perhaps we could add items_last_borrower cleanup based on StoreLastBorrower preference in cleanup_database.pl? Great work btw!
Created attachment 186801 [details] [review] Bug 26993: Implement changes Test plan: 1. APPLY patch, restart_all, updatedatabase 2. Search for the StoreLastBorrower system preference, it should now allow you to enter a number of borrowers to store 3. Set the pref to 3. 4. Check an item out to any patron. Then immediately check it back in. 5. On catalogue/moredetail.pl?biblionumber=X ( where X is you biblionumber ) you should see a "Last returned by:" entry with the correct cardnumber for the borrower. 7. Confirm this is right in the database with: SELECT * FROM items_last_borrower where itemnumber = X; ( Where X is your itemnumber ) 8. Repeat step 4 to a different borrower. 9. Repeat steps 5 and 6. You should see 2 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X 10. Repeat step 4 with another new borrower. 11. Repeat steps 5 and 6. You should see 3 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X 12. Choose a different item and repeat steps 4 - 11 for this item. 13. Now set the StoreLastBorrower preference to 0. 14. Using the original item from step 4, check something out to a patron and immediately check it back in. 15. catalogue/moredetail.pl?biblionumber=X should NOT have an entry for "Last returned by:" 16. Check the database, all entries for the itemnumber from step 14 should be deleted.
(In reply to Lari Taskula from comment #10) > But since we don't, perhaps we could add items_last_borrower cleanup based > on StoreLastBorrower preference in cleanup_database.pl? Bug 23260 shall be forthcoming for deletion of old last borrower info via cleanup_database :)
Created attachment 186805 [details] [review] Bug 26993: Database updates
Created attachment 186806 [details] [review] Bug 26993: Schema changes
Created attachment 186807 [details] [review] Bug 26993: System preference updates
Created attachment 186808 [details] [review] Bug 26993: Implement changes Test plan: 1. APPLY patch, restart_all, updatedatabase 2. Search for the StoreLastBorrower system preference, it should now allow you to enter a number of borrowers to store 3. Set the pref to 3. 4. Check an item out to any patron. Then immediately check it back in. 5. On catalogue/moredetail.pl?biblionumber=X ( where X is you biblionumber ) you should see a "Last returned by:" entry with the correct cardnumber for the borrower. 7. Confirm this is right in the database with: SELECT * FROM items_last_borrower where itemnumber = X; ( Where X is your itemnumber ) 8. Repeat step 4 to a different borrower. 9. Repeat steps 5 and 6. You should see 2 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X 10. Repeat step 4 with another new borrower. 11. Repeat steps 5 and 6. You should see 3 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X 12. Choose a different item and repeat steps 4 - 11 for this item. 13. Now set the StoreLastBorrower preference to 0. 14. Using the original item from step 4, check something out to a patron and immediately check it back in. 15. catalogue/moredetail.pl?biblionumber=X should NOT have an entry for "Last returned by:" 16. Check the database, all entries for the itemnumber from step 14 should be deleted.
Created attachment 186809 [details] [review] Bug 26993: Unit tests
Created attachment 186844 [details] [review] Bug 26993: (follow-up) Fix template issue when last_returned_by_all returns 1 result
Created attachment 186853 [details] [review] Bug 26993: last_returned_by_all should return an array ref
Created attachment 187311 [details] [review] Bug 26993: Database updates Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Created attachment 187312 [details] [review] Bug 26993: Schema changes Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Created attachment 187313 [details] [review] Bug 26993: System preference updates Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Created attachment 187314 [details] [review] Bug 26993: Implement changes Test plan: 1. APPLY patch, restart_all, updatedatabase 2. Search for the StoreLastBorrower system preference, it should now allow you to enter a number of borrowers to store 3. Set the pref to 3. 4. Check an item out to any patron. Then immediately check it back in. 5. On catalogue/moredetail.pl?biblionumber=X ( where X is you biblionumber ) you should see a "Last returned by:" entry with the correct cardnumber for the borrower. 7. Confirm this is right in the database with: SELECT * FROM items_last_borrower where itemnumber = X; ( Where X is your itemnumber ) 8. Repeat step 4 to a different borrower. 9. Repeat steps 5 and 6. You should see 2 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X 10. Repeat step 4 with another new borrower. 11. Repeat steps 5 and 6. You should see 3 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X 12. Choose a different item and repeat steps 4 - 11 for this item. 13. Now set the StoreLastBorrower preference to 0. 14. Using the original item from step 4, check something out to a patron and immediately check it back in. 15. catalogue/moredetail.pl?biblionumber=X should NOT have an entry for "Last returned by:" 16. Check the database, all entries for the itemnumber from step 14 should be deleted. Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Created attachment 187315 [details] [review] Bug 26993: Unit tests Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Created attachment 187316 [details] [review] Bug 26993: last_returned_by_all should return an array ref Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
QA comment: 1. I think removing the FK check in the db rev is enough (see previous patch) 2. last_returned_by_all should return a set (not an arrayref), and $self->_result->last_returned_by->delete_all; should be $self->last_returned_by_all->delete; 3. Maybe last_returned_by_all should be renamed last_borrowers? :) 4. (note) There is certainly something to improve in last_returned_by when removing the last X patrons, but I didn't find something that won't remove readability. 5. (not blocker, thought) not sure we should deal with the purge in this method # If StoreLastBorrower is 0 or disabled, bail without storing anything. Also delete any remaining rows from the table. if ( $max_stored_borrowers == 0 ) { $self->_result->last_returned_by->delete_all; return $self; } 6. last_returned_by is not concurrency-safe, but it shouldn't be a problem here. However we should at least use a transaction.
Created attachment 187459 [details] [review] Bug 26993: Use FOREIGN_KEY_CHECKS
Created attachment 187460 [details] [review] Bug 26993: Use a txn
Created attachment 187483 [details] [review] Bug 26993: (follow-up) Return Koha::Patron resultset from last_returned_by_all
Created attachment 187484 [details] [review] Bug 26993: (follow-up) Rename last_returned_by_all as last_borrowers
Thanks Jonathan for looking and for the follow-ups. I have also included 2 follow-up that I think address all the QA concerns. Setting back to "Signed-off"
Does it make sense to have both "Last returned by" and "Last borrower"/"Previous borrower"? https://snipboard.io/dAvq9o.jpg
(In reply to Jonathan Druart from comment #32) > Does it make sense to have both "Last returned by" and "Last > borrower"/"Previous borrower"? > > https://snipboard.io/dAvq9o.jpg These have been confusingly labeled for as long as StoreLastBorrower has been a thing, but they're distinct pieces of data and showing them both is longstanding behavior. Clarifying this display somehow is a great idea that should be pursued in another bug.
(In reply to Andrew Fuerste-Henry from comment #33) > (In reply to Jonathan Druart from comment #32) > > Does it make sense to have both "Last returned by" and "Last > > borrower"/"Previous borrower"? > > > > https://snipboard.io/dAvq9o.jpg > > These have been confusingly labeled for as long as StoreLastBorrower has > been a thing, but they're distinct pieces of data and showing them both is > longstanding behavior. Clarifying this display somehow is a great idea that > should be pursued in another bug. I have filed Bug 40963.
(In reply to Andrew Fuerste-Henry from comment #23) > 13. Now set the StoreLastBorrower preference to 0. > 14. Using the original item from step 4, check something out to a patron and > immediately check it back in. After doing this, I have problems. items_last_borrower recorded my patron from step 14, even though the syspref is set to zero -- I don't have a "Last returned by" line on moredetail.pl, but the data exists in the database. More distressingly, old_issues has lost the borrowernumber for every checkout to any patron other than the patron used in step 14. old_issues.borrowernumber is NULL for these checkouts.
(In reply to Andrew Fuerste-Henry from comment #35) > (In reply to Andrew Fuerste-Henry from comment #23) > > 13. Now set the StoreLastBorrower preference to 0. > > 14. Using the original item from step 4, check something out to a patron and > > immediately check it back in. > > After doing this, I have problems. items_last_borrower recorded my patron > from step 14, even though the syspref is set to zero -- I don't have a "Last > returned by" line on moredetail.pl, but the data exists in the database. > > More distressingly, old_issues has lost the borrowernumber for every > checkout to any patron other than the patron used in step 14. > old_issues.borrowernumber is NULL for these checkouts. I think the problem here is `$self->last_borrowers->delete` which calls Koha::Patrons->delete()
(In reply to Lucas Gass (lukeg) from comment #36) > (In reply to Andrew Fuerste-Henry from comment #35) > > (In reply to Andrew Fuerste-Henry from comment #23) > > > 13. Now set the StoreLastBorrower preference to 0. > > > 14. Using the original item from step 4, check something out to a patron and > > > immediately check it back in. > > > > After doing this, I have problems. items_last_borrower recorded my patron > > from step 14, even though the syspref is set to zero -- I don't have a "Last > > returned by" line on moredetail.pl, but the data exists in the database. > > > > More distressingly, old_issues has lost the borrowernumber for every > > checkout to any patron other than the patron used in step 14. > > old_issues.borrowernumber is NULL for these checkouts. > > I think the problem here is `$self->last_borrowers->delete` which calls > Koha::Patrons->delete() SHOW CREATE TABLE old_issues: CONSTRAINT `old_issues_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE SET NULL
Created attachment 191264 [details] [review] Bug 26993: Use last_returned_by->delete_all
Created attachment 191624 [details] [review] Bug 26993: Database updates Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 191625 [details] [review] Bug 26993: Schema changes Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 191626 [details] [review] Bug 26993: System preference updates Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 191628 [details] [review] Bug 26993: Implement changes Test plan: 1. APPLY patch, restart_all, updatedatabase 2. Search for the StoreLastBorrower system preference, it should now allow you to enter a number of borrowers to store 3. Set the pref to 3. 4. Check an item out to any patron. Then immediately check it back in. 5. On catalogue/moredetail.pl?biblionumber=X ( where X is you biblionumber ) you should see a "Last returned by:" entry with the correct cardnumber for the borrower. 7. Confirm this is right in the database with: SELECT * FROM items_last_borrower where itemnumber = X; ( Where X is your itemnumber ) 8. Repeat step 4 to a different borrower. 9. Repeat steps 5 and 6. You should see 2 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X 10. Repeat step 4 with another new borrower. 11. Repeat steps 5 and 6. You should see 3 cardnumbers of each of the borrowers on catalogue/moredetail.pl?biblionumber=X 12. Choose a different item and repeat steps 4 - 11 for this item. 13. Now set the StoreLastBorrower preference to 0. 14. Using the original item from step 4, check something out to a patron and immediately check it back in. 15. catalogue/moredetail.pl?biblionumber=X should NOT have an entry for "Last returned by:" 16. Check the database, all entries for the itemnumber from step 14 should be deleted. Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 191630 [details] [review] Bug 26993: Unit tests Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 191631 [details] [review] Bug 26993: last_returned_by_all should return an array ref Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 191632 [details] [review] Bug 26993: Use FOREIGN_KEY_CHECKS Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 191633 [details] [review] Bug 26993: Use a txn Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 191634 [details] [review] Bug 26993: (follow-up) Return Koha::Patron resultset from last_returned_by_all Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 191635 [details] [review] Bug 26993: (follow-up) Rename last_returned_by_all as last_borrowers Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 191636 [details] [review] Bug 26993: Use last_returned_by->delete_all Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Nice work everyone! Pushed to main for 26.05