Display aged circulations for copies (was: "virtually aged circulations")

[Bill Erickson]

This wishlist item is bug #1453974 on steroids.

Here's the idea..

When circulation aging (anonymization) is enabled, all circulations that are eligible for aging, regardless whether they are actually anonymized, should be treated as such in the staff client. To put that another way, just because a patron opts in to retaining their circ history does not mean their entire circ history should be visible to staff, short of running a report.

Fortunately, there aren't many places in the staff client where closed circulations are retrieved. Those that come to mind are the copy status interface, the copy "most recent circulations" interface, and the patron billing history interface.

Looking specifically at the copy status UI, I imagine it working something like this:

1. API looks for most recent circulation, aged or otherwise.
2. If the most recent is aged, then the aged circ is returned.
3. If the most recent is not aged, but would have been, had the patron not opted in, then translate the circ on-the-fly into an aged circulation (in the ML, not the DB) and return that.
4. Otherwise, return the active circulation.
5. The UI would be modified to look for aged circulations. When it received one, it would avoid rendering any user-related info. For example, the "Patron" field in the circ history tab would be empty. When staff selected the "show last few circulations" action from the menu, the circulations would appear, but the patron name / barcode would not be listed.

Input on the general idea appreciated.

[Jason Boyer (jboyer)]

Do the interfaces that retrieve closed circs all call the same API to retrieve them? It seems to me that it's better to make the changes in the actual API layer rather than teaching the UI that some circs have to be displayed differently.

(If they don't all call the same API, or worse, just throw around JSONQ's or PCRUD calls, perhaps a usable API could be defined to handle this on the back end)

[Bill Erickson (berick)]

Jason, I absolutely agree it should be handled and enforced in the API. My point about the UI's is only that they will have to change to support rendering aged circulations that have slightly different fields, i.e. no "usr" field, and use a different IDL class, etc.

These are the API's that would have to be modified to affect the interfaces previously listed. Others may exist, of course.

open-ils.circ.copy_details.retrieve (copy status)
open-ils.circ.copy_checkout_history.retrieve (copy recent circs)
open-ils.actor.user.transaction.fleshed.retrieve.authoritative (patron billing history)

[Kathy Lussier (klussier)]

I already gave this feedback in IRC, but I'm adding it here for the record.

- I love the general idea presented here

- I know it's really not part of the scope of the project you described, but, while you're in there, it would be great if you could also have the copy status screen honor the "Maximum Previous Checkouts" OU Setting. See https://bugs.launchpad.net/evergreen/+bug/1378383

- When that setting is set to 1 or 0, I think rendering the circ info without user-related data, as Bill describes for the should-be-aged circulations, would work very well.

Thanks Bill!

[Jason Boyer (jboyer)]

"they will have to change to support rendering aged circulations that have slightly different fields" - Oh, of course, I wasn't putting 2 and 2 together; apologies. Actually looking at all of the pieces, it all makes much more sense as presented.

I wonder if there might be a way to keep the information that patrons actually care about and not even bother to keep the expired circs around? A table only for tracking opt-in circ history (that also holds similarly simplified holds history), perhaps a materialized view of the information returned to a patron so that even deleting items and bibs have no effect on their history. On disabling the tracking option every entry for that patron could simply be deleted. I don't know that it's 100% a good solution, but it seems like it would cut down on some special cases.

[Bill Erickson (berick)]

Other advantages to decoupling the patron's checkout history from action.circulation is it would reduce the likelihood of that data leaking to the UI, e.g., via too-liberal pcrud queries, as referenced above. It would also allow us to to implement bug #1312699 (editable checkout history) in a way that actually removes the data instead of hiding it.

For the feature at hand, such a change would mean all circs older than X interval would be stored as aged_circulation's, so there would be no need to inspect each circulation to see if it should be anonymized and then (potentially) virtually anonymizing on the fly. In other words, we would not have to rearrange (to expose) or reimplement the logic of action.purge_circulations in the middle layer, which would be nice.

[Bill Erickson (berick)]

Noting for reference, this bug may affect on how we handle bug #1378383 ("Maximum previous checkouts displayed").

[Bill Erickson (berick)]

After thinking about this a little more, I'm even more convinced that storing patron reading history in a separate table / materialized view is the right way to go.

I agree the table should track everything the patron might need to see as it existed at the time of circulation to avoid problems with bibs, etc. being deleted or modified after the fact.

Jason, can you clarify "and not even bother to keep the expired circs around"?

[Jason Boyer (jboyer)]

I just meant that there would be no reason to keep the circs un-expired as they are now, which lead to the bug. They may still be kept according to the "keep X circs" OU setting, but there would be (at least) one less thing to check before aging a circ out.

[Jason Boyer (jboyer)]

Keep X Circs per item, that is...

[Bill Erickson (berick)]

Opening a separate bug to address decoupling checkout history from action.circulation. This bug will then depend on the new bug. Will link here once created.

[Bill Erickson (berick)]

Now that bug #1527342 (decouple checkout history) is done, the plan for this ticket becomes much simpler. I believe now we're only talking about the copy status UI (which lists recent circulations for a copy) and the copy "most recent circulations" interface.

1. The API(s) in question will return the X most recent circs found in action.all_circulation (or similar) -- the goal being to include active and aged circulations.

2. The interface code will be modified to handle action.aged_circulation in addition to action.circulation objects. In short, when an aged_circulation is encountered, the code should avoid referencing the "usr" column (and possibly others).

[Bill Erickson (berick)]

WIP branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1497335-copy-sees-aged-circs-wip

1. Adds a null-able 'usr' column to the action.all_circulation class/view. A value in the 'usr' column indicates the all_circulation object in question represents an active circulation. Otherwise, it represents an aged_circulation. Also adds a few other columns to the IDL that were present in the view but missing from the IDL .

2. Adds DB functions action.all_circ_chain and action.summarize_all_circ_chain, which behave similarly to their non _all_ versions, but pull data from action.all_circulation instead of action.circulation.

3. Modified several API calls to return all_circulation objects instead of circulation objects.

open-ils.circ.renewal_chain.retrieve_by_circ
open-ils.circ.prev_renewal_chain.retrieve_by_circ
open-ils.circ.copy_checkout_history.retrieve
open-ils.circ.copy_details.retrieve

4. Modified others to optionally support returning all_circulation objects.

open-ils.circ.retrieve

5. Modifies the XUL code to handle cases where responses to these API calls returned objects with NULL usr values.

==

Returning all_circulation objects, which contain (now) all of the fields (and them some) of action.circulation, turns out out be fairly painless for migrating to the modified API. The only work involved is checking for NULL values in the 'usr' field. I believe I have covered all of these in the XUL client. Need to do a little more testing...

===

For the browser client, we are using almost exclusively PCRUD to perform the actions performed by the API calls above. In the one case where we use two of the calls above (open-ils.circ.[prev_]renewal_chain.retrieve_by_circ.summary), we are retrieving summary objects, which are not affected by this change. (They will just now include aged circ summaries too, which is good).

Will need to open a new ticket to address implementing the same changes in the browser client, which will likely involve some IDL changes (to provide access to all_circulation) and some client-side PCRUD changes and/or moving back to using API's where needed. Will look at that after confirming this is a sane path forward.

[Bill Erickson (berick)]

Rebased code in progress to master. Starting on browser client integration with this commit:

1. Use all_circulation (combcirc) class (now accessible via pcrud) to render the item Circ History List tab. In cases where a combcirc object has no 'usr' value, the interface displays <Aged Circulation> where the patron's name would normally be.

2. Handle null 'usr' values in the item status Recent Circ History tab. When a renewal chain summary has no 'usr' value, the interface displays <Aged Circulation> where the patron's name would normally be.

==

Alternate label suggestions welcome. More to follow as I find places where 'combcirc' might be required.

[Bill Erickson (berick)]

Cleaned, better documented, rebased branch pushed. Includes release notes.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1497335-aged-circs-staff-integration

Recapping from the release notes:

==
The browser and XUL clients now better represent copy checkout history by honoring and displaying information from aged circulations.

 * Browser client 'Recent Circ History' and the analogous XUL client 'Circulation History' tabs show summary data for aged circulations as well as regular/active circulations. When aged circulation data is displayed, any references to patron names are replaced by the string "<Aged Circulation>".

 * Browser client 'Circ History List' and the analogous XUL client 'Last Few Circulations' tabs behave as above, plus their 'Add Billing' buttons are disabled when displaying aged circulation data.

 * XUL client 'Retrieve Last Patron' actions from various UI's report, "Item XXX circulation is an aged circulation and has no linked user". Browser client analog uses 'Circ History List' instead, no additional changes required.
==

I believe it's ready for prime time. Adding pullrequest.

[Kathy Lussier (klussier)]

Hi Bill,

I've been testing this branch on the xul client. Overall it looks good, but I think I may have found one problem.

From the 'Last Few Circulations' tab in Xul, if the most recent circ is aged (in this case, I had set the count for aging to 0), when I click the 'Retrieve Last Patron' option, I just get bounced back to whatever client tab I was on before going to 'Last Few Circulations.' Is this one of those places where we should be seeing the "Item XXX circulation is an aged circulation and has no linked user" message?

Kathy

[Bill Erickson (berick)]

Thanks Kathy,

When Retrieve Last Patron or Retrieve All These Patrons actions are selected, the Last Few Circs modal window closes, then opens the requested patron tabs. In this case, there are no new tabs to open. I had not anticipated this one...

Feedback on the following solutions appreciated:

1. If we can't open any tabs because there are no non-aged circs, disable both Retrieve buttons.

2. If the most recent circ is aged, disable the Retrieve Last Patron button. My thinking here is that no other patrons can really be thought of as the "Last Patron", since an unknown patron came after.

3. If 'Retrieve All These Patrons' is selected, but only some of the circs have linked patrons, alert that not all circs can be retrieved, then retrieve the linked patrons in new tabs as before. Would this alert be more annoying than helpful? Other options?

[Kathy Lussier (klussier)]

1 and 2 sound great!

After consulting with mmorgan, I would say #3 is unnecessary.

Thanks Bill!

[Bill Erickson (berick)]

Changes pushed to (rebased) branch. Change also includes a fix to an open-ils.circ API call to repair a sort bug introduced earlier by this branch.

[Mike Rylander (mrylander)]

Master has this now. Thanks, Bill!