Maintain patron reading history in a dedicated table.

What data do we want to track for checkout history? We'll need the following columns at minimum on our new table for parity with the TPAC checkout history display.

* id
* xact_start
  -- Set to xact_start from the first checkout in the renewal chain.
* usr
* target_copy
* due_date
  -- Set to due_date of most recent checkout in the renewal chain.
* checkin_time
  -- Checkin time of the most recent checkout in the renewal chain.
  -- Will be NULL until final checkin.
* source_circ
  -- Points to the first circulation in the renewal chain.
  -- Useful for locating the history object during renewal and checkin for modifying dynamic fields (e.g. due_date).
  -- Set to NULL when source circ is aged.

I could see adding these as well:

* circ_lib
  -- Set to circ_lib from the first checkout in the renewal chain.
* checkin_lib
  -- Set to checkin_lib from the last checkout in the renewal chain.
  -- Remains NULL until final checkin.

Alternatively, we could mimic action.circulation entirely and track the entire circulation chain, but that seems like overkill and may be storing more data than we really want to retain.

I would expect rows in the new table being added and updated via DB trigger, which fires on create/update/delete for action.circulation.

Also, there was conversation in bug #1497335 about maintaining static copies of copy/call number/bib data as part of moving checkout history out to its own table, the idea being that a bib record could be modified or deleted, thus changing the history after the fact. I think this is a worthy goal, but it also adds a lot of questions and complications to the implementation. Since the current checkout history implementation does not protect against these kinds of issues, I'm not planning to address it here. That could be added to this new structure later if desired.

I think we should only include the data that might be useful to the end user and nothing more, so I agree that mimicking action.circulation is excessive. The columns you identified for feature parity look good.

 I'm not terribly excited about circ_lib and checkin_lib, but I'm not necessarily opposed to it either. I could see a use case where a patron is trying to track down a title of a book they read and can only remember that they checked it out/in from a certain location in a certain timeframe. The circ_lib / checkin_lib might be useful here. A downside might be that if somebody did gain unauthorized access to the person's account, they will not only see a history of their reading, but would also get a general idea of which libraries they visit and when.

I can't think of any other fields we would need here.

In terms of maintaining static copy/call number/bib data (I know you're not addressing it, but I'll give my two cents anyway), I tried to think of a reason why a user would want the call number/barcode information in their reading history. Call number could be useful if they want to find an item to check out again, but, without the copy location, they may still need to click through to the bib record anyway f. In this instance, then, they would want to see the existing call number for the copy, not what the call number was when they checked it out. I can't think of any reason why they would want to see what the call number was at the time it was in their hands.

I don't know why anyone would want to see the barcode after they've returned the item. If we were building this from scratch, I might suggest that we don't need the call number and barcode to display, but, if we're going for feature parity, I prefer that the data not be static.

For those deleted call numbers, copies, bibs, would it be useful here to check for that and add styling or a notation that says the item is no longer available? I don't feel strongly about it, but it might be one way to address the issue.

Thanks Bill! I definitely think this enhancement is a step in the right direction!

Thanks for the comments Kathy.

* Let's drop circ_lib and checkin_lib, then. It's probably of minimal value to the patron.

* For the static data, my main concern really was avoiding the off chance that a MARC record changed after it was linked to a patron's checkout history, creating a false history. This seems like such an unlikely edge case (and similar things could already happen 100 different ways) that I'm no longer concerned about it. Worst case scenario... auditor tables to the rescue.

Getting started here:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1527342-decouple-co-history

This includes upgrade SQL and TPAC changes for a new usr_circ_history table. If a user has either of the circ retention values set, each (original) checkout will add a row to the table, plus any time the due_date or checkin_time on the last circ in the circ chain is modified, the same fields on the circ history object are updated to reflect the change. TPAC display works as before.

TODO:

* Remove usr_visible_circs function and remove it from circ aging process.
* Modify open-ils.actor.history.circ.visible.* API for new structure and make the data accessible only to the owning user.
* Modify print/email A/T circ history templates to use new table instead of action.circulation.
* Copy upgrade SQL to schema SQL.

TODO: implement circ history removal when patrons opt out of circ history.

TODO: I'd like to see a message when users disable circ/hold history warning that their history will be deleted and that the operation is irreversible.

Code for most of the above pushed, plus pgtap test.

TODO as of today:

1. Warn users that circ history delete is irreversible (when circ history exists).
2. Release notes.

TODO items above done. Rebased and squashed branch pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1527342-decouple-co-history-squash1

I believe the code is done, but testing the upgrade script, which migrates checkout history from the current setup to the new table, will need a good bit of testing to be safe.

Add pullrequest.

I like the idea of moving the reading history to a separate table.

However, a thought that occurred to me upon reading the bug (I've not looked at the code yet): two words I've not seen yet are "overdue fines". One person's "reading list" may be another person's "tool to track how much I'm 'donating' to the library" may be another person's "ammunition against the circulation desk if the 'my account' view doesn't match what I've been billed for".

That last use case might suggest including circ_lib and checkin_lib in the reading history. Emphasis on "might", however; I would be curious to hear from folks whether the reading history view gets invoked by patrons during disputes over overdue fines often enough to matter.

Thanks for the thoughts, Galen.

I'm not quite following the overdue fine scenario, though. If fines are owed, the source circulation will still be alive and accessible to staff. Are you referring to a scenario where the patron pays a fine, then after the circ is aged, the patron returns to dispute the paid fine?

> I'm not quite following the overdue fine scenario, though. If fines are
> owed, the source circulation will still be alive and accessible to
> staff. Are you referring to a scenario where the patron pays a fine,
> then after the circ is aged, the patron returns to dispute the paid
> fine?

Something like that. At the moment, to me that's a hypothetical
scenario, and one for which library policies could be written to limit
that sort of abuse (e.g., no refunds ever after the aging interval has
passed). I'd still like to hear from circ desk managers whether this
is actually an issue in practice.

Force-pushed a fix to support deleting circ history on user purge.

Galen: I don't see that issue in practice and I certainly handle enough
front line complaints.

Like Kathy I don't see a need for circ_lib and checkin_lib but I'm not
opposed to them.

On Wed, Dec 30, 2015 at 12:15 PM, Bill Erickson <email address hidden> wrote:

> Force-pushed a fix to support deleting circ history on user purge.
>
> --
> You received this bug notification because you are subscribed to
> Evergreen.
> Matching subscriptions: evergreenbugs
> https://bugs.launchpad.net/bugs/1527342
>
> Title:
> Maintain patron reading history in a dedicated table.
>
> Status in Evergreen:
> New
>
> Bug description:
> Evergreen all versions circa 2.9.
>
> This bug was derived from discussion in bug #1497335.
>
> Patron reading history is derived directly from action.circulation.
> This creates a number of complications and privacy concerns:
>
> 1. It complicates the circ aging process, since we cannot age circs
> that represent entries in an active reading history.
>
> 2. Circs that persist for reading history are accessible via API and
> appear in staff interfaces when they otherwise would not. The goal of
> reading history is to allow patrons to see their own history, but as
> is, it also allows staff to see a patron's reading history. There are
> some protections in place here, but they are not applied universally
> and are easily and sometimes unintentionally bypassed via API.
>
> 2. Patrons cannot (truly) remove specific items from their history,
> since they have no way of deleting circulations, short of disabling
> all reading history. (See also bug #1312699).
>
> I propose we track patron reading history in a separate database
> table. The table (or materialized view) may be similar to action
> circulation, though likely with much fewer columns.
>
> More to follow. Comments appreciated.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/1527342/+subscriptions
>

Hi Bill,

Overall, this is looking good, but I'm having some trouble with the CSV download. I noticed that you made adjustments to the action-trigger event defintions and hooks for circ.history.email and circ.history.print, but that no adjustments were made to Circ History CSV definition or the circ.history.format.csv hook.

When I download the user's circ history, the titles in the CSV file don't match what's in the user's circ history.

Aha, the code that calls the CSV template needed some tweaks. I've rebased the whole shebang to master and pushed a fix to the tip of user/berick/lp1527342-decouple-co-history-squash1.

Thanks, Kathy!

Thank you Bill! The CSV download is working much better now. I found one remaining issue just before I was about to sign off on the code.

After successfully tracking a user's circ history, I logged into the user's account and removed the checkmark to keep a history of checked out items. When I click Save, I don't see the warning described above. Also, the user's circ history remains in the database. When I return to account preferences, the checkbox for keeping circ history is selected again.

As discussed in IRC, the problem Kathy noted above has to do with the fact that the warning message was not very visible, plus it was confusing because the update success message was also displaying. I pushed a change to avoid displaying the success message when the warning is also displayed and to push the warning message and confirm actions up to the top of the form for visibility.

Thanks again Bill! I test the following:

- When user sets the circ history to enabled, history is now saved in action.usr_circ_history
- When the transaction is deleted, either through the purge history script or a direct database deletion, the circ id is set to null in the action.usr_circ_history row
- CSV download works correctly, both before and after the source circulation is deleted.
- The search history tagging continues to work as expected, both before and after the source circulation is deleted.
- When the patron is deleted, their action.usr_circ_history rows are removed.
- When the patron disable circ history tracing, they receive a warning. When they click through the warning, their rows in action.usr_circ_history are removed.

Merged to master for inclusion in 2.10