Description
Andrew Fuerste-Henry
2020-11-10 16:25:37 UTC
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. |