Bug 26993 - Allow StoreLastBorrower to retain a locally-defined number of previous borrowers
Summary: Allow StoreLastBorrower to retain a locally-defined number of previous borrowers
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P2 enhancement
Assignee: Lucas Gass (lukeg)
QA Contact: Testopia
URL:
Keywords:
Depends on: 14945
Blocks: 23260
  Show dependency treegraph
 
Reported: 2020-11-10 16:25 UTC by Andrew Fuerste-Henry
Modified: 2025-10-07 12:09 UTC (History)
11 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement expands the StoreLastBorrower system preference to allow for the retention of a locally defined number of previous borrowers for a given item. This enhancement also introduces a mechanism that will delete any extra borrowers stored in the items_last_borrower table that exceed the number set in the StoreLastBorrower system preference.
Version(s) released in:
Circulation function:


Attachments
Bug 26993: Database updates (2.48 KB, patch)
2025-09-22 21:45 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: Schema changes (1.04 KB, patch)
2025-09-22 21:45 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: System preference updates (3.60 KB, patch)
2025-09-22 21:45 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: Implement changes (7.37 KB, patch)
2025-09-22 21:45 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: Implement changes (7.44 KB, patch)
2025-09-23 13:31 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: Database updates (2.39 KB, patch)
2025-09-23 13:57 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: Schema changes (1.04 KB, patch)
2025-09-23 13:57 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: System preference updates (3.59 KB, patch)
2025-09-23 13:57 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: Implement changes (7.51 KB, patch)
2025-09-23 13:58 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: Unit tests (6.85 KB, patch)
2025-09-23 13:58 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: (follow-up) Fix template issue when last_returned_by_all returns 1 result (1.28 KB, patch)
2025-09-23 20:04 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: last_returned_by_all should return an array ref (5.15 KB, patch)
2025-09-23 20:36 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: Database updates (2.45 KB, patch)
2025-10-02 18:39 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26993: Schema changes (1.09 KB, patch)
2025-10-02 18:39 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26993: System preference updates (3.65 KB, patch)
2025-10-02 18:39 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26993: Implement changes (7.57 KB, patch)
2025-10-02 18:39 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26993: Unit tests (6.91 KB, patch)
2025-10-02 18:39 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26993: last_returned_by_all should return an array ref (5.21 KB, patch)
2025-10-02 18:39 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26993: Use FOREIGN_KEY_CHECKS (1.78 KB, patch)
2025-10-06 11:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26993: Use a txn (3.01 KB, patch)
2025-10-06 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26993: (follow-up) Return Koha::Patron resultset from last_returned_by_all (6.73 KB, patch)
2025-10-06 16:50 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 26993: (follow-up) Rename last_returned_by_all as last_borrowers (5.25 KB, patch)
2025-10-06 16:54 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-11-10 16:25:37 UTC
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.
Comment 1 Wally DesChamps 2020-11-10 19:14:45 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).
Comment 2 Marcel de Rooy 2020-11-18 12:08:44 UTC
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?
Comment 3 Erik Stevens 2022-08-17 19:35:46 UTC
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.
Comment 4 Wally DesChamps 2022-08-17 20:05:44 UTC
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
Comment 5 Lucas Gass (lukeg) 2025-09-22 21:45:07 UTC
Created attachment 186773 [details] [review]
Bug 26993: Database updates
Comment 6 Lucas Gass (lukeg) 2025-09-22 21:45:10 UTC
Created attachment 186774 [details] [review]
Bug 26993: Schema changes
Comment 7 Lucas Gass (lukeg) 2025-09-22 21:45:13 UTC
Created attachment 186775 [details] [review]
Bug 26993: System preference updates
Comment 8 Lucas Gass (lukeg) 2025-09-22 21:45:15 UTC
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.
Comment 9 Lucas Gass (lukeg) 2025-09-22 22:08:38 UTC
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.
Comment 10 Lari Taskula 2025-09-23 08:27:35 UTC
(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!
Comment 11 Lucas Gass (lukeg) 2025-09-23 13:31:51 UTC
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.
Comment 12 Andrew Fuerste-Henry 2025-09-23 13:33:46 UTC
(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 :)
Comment 13 Lucas Gass (lukeg) 2025-09-23 13:57:52 UTC
Created attachment 186805 [details] [review]
Bug 26993: Database updates
Comment 14 Lucas Gass (lukeg) 2025-09-23 13:57:56 UTC
Created attachment 186806 [details] [review]
Bug 26993: Schema changes
Comment 15 Lucas Gass (lukeg) 2025-09-23 13:57:59 UTC
Created attachment 186807 [details] [review]
Bug 26993: System preference updates
Comment 16 Lucas Gass (lukeg) 2025-09-23 13:58:03 UTC
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.
Comment 17 Lucas Gass (lukeg) 2025-09-23 13:58:06 UTC
Created attachment 186809 [details] [review]
Bug 26993: Unit tests
Comment 18 Lucas Gass (lukeg) 2025-09-23 20:04:02 UTC
Created attachment 186844 [details] [review]
Bug 26993: (follow-up) Fix template issue when last_returned_by_all returns 1 result
Comment 19 Lucas Gass (lukeg) 2025-09-23 20:36:23 UTC
Created attachment 186853 [details] [review]
Bug 26993: last_returned_by_all should return an array ref
Comment 20 Andrew Fuerste-Henry 2025-10-02 18:39:31 UTC
Created attachment 187311 [details] [review]
Bug 26993: Database updates

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Comment 21 Andrew Fuerste-Henry 2025-10-02 18:39:34 UTC
Created attachment 187312 [details] [review]
Bug 26993: Schema changes

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Comment 22 Andrew Fuerste-Henry 2025-10-02 18:39:37 UTC
Created attachment 187313 [details] [review]
Bug 26993: System preference updates

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Comment 23 Andrew Fuerste-Henry 2025-10-02 18:39:40 UTC
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>
Comment 24 Andrew Fuerste-Henry 2025-10-02 18:39:44 UTC
Created attachment 187315 [details] [review]
Bug 26993: Unit tests

Signed-off-by: Trevor Diamond <trevor.diamond@mainlib.org>
Comment 25 Andrew Fuerste-Henry 2025-10-02 18:39:47 UTC
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>
Comment 26 Jonathan Druart 2025-10-06 11:22:37 UTC
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.
Comment 27 Jonathan Druart 2025-10-06 11:26:56 UTC
Created attachment 187459 [details] [review]
Bug 26993: Use FOREIGN_KEY_CHECKS
Comment 28 Jonathan Druart 2025-10-06 11:37:42 UTC
Created attachment 187460 [details] [review]
Bug 26993: Use a txn
Comment 29 Lucas Gass (lukeg) 2025-10-06 16:50:23 UTC
Created attachment 187483 [details] [review]
Bug 26993: (follow-up) Return Koha::Patron resultset from last_returned_by_all
Comment 30 Lucas Gass (lukeg) 2025-10-06 16:54:06 UTC
Created attachment 187484 [details] [review]
Bug 26993: (follow-up) Rename last_returned_by_all as last_borrowers
Comment 31 Lucas Gass (lukeg) 2025-10-06 16:55:21 UTC
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"
Comment 32 Jonathan Druart 2025-10-07 08:29:50 UTC
Does it make sense to have both "Last returned by" and "Last borrower"/"Previous borrower"?

https://snipboard.io/dAvq9o.jpg
Comment 33 Andrew Fuerste-Henry 2025-10-07 11:37:13 UTC
(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.
Comment 34 Lucas Gass (lukeg) 2025-10-07 12:09:31 UTC
(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.