Simplified hold pull list interface

[Lebbeous Fogle-Weekley]

For Evergreen master:

I'm pretty sure many sites agree that the holds pull list interface as it works today is kind of hopelessly slow. Here's a new one based on the new flattener service and friends that should load faster for large datasets, provide smarter paging (never needlessly loading hundreds of holds if only a relative few are going to be in view, etc).

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/senator/simplified-hold-pull-list

[Lebbeous Fogle-Weekley (lebbeous)]

Here's an improved version (mainly improved in printing, which before this was missing the column labels and column ordering of the regular visual representation) rebased against current master.

Also adding forgotten pullrequest tag.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/senator/simplified-hold-pull-list

[Bill Erickson (berick)]

Gave it a quick test. It all worked well for me. I'm not much of an end user, though, so I'll delay merging a bit for comments.

[Michael Peters (mrpeters)]

Lebeous,

This is very nice! It's a bit of a return to the "original" pull list format, I think.

It's fantastic to be able to reorder the columns, re-size them, etc. This is a great improvement. I vote for merging to master.

One little bug, however, is that I found mousing over the column ends in an attempt to resize them, the pointer remained as the "important" pointer, instead of changing to the arrows you'd expect. Just a visual cue I think people would find helpful, if it's possible to fix.

Thank you, for a great new feature!

-Mike

[Michael Peters (mrpeters)]

Added my signoff. Tested and working well!

user/mrpeters-isl/simplified-hold-pull-list_signoff
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=3e1b74bbcb8f718785b008d26da09073f0ebaf66

[Lebbeous Fogle-Weekley (lebbeous)]

Thanks Mike. The column resizing and reordering, and what happens to the mouse pointer, probably happens the same way to any Dojo DataGrids we have with column reordering enabled (assuming we do have any other such grids). We may be able to override the behavior we don't like in Dojo, and it's also possible that our (hopefully not too far off now!) upgrade to a modern version of Dojo will just take care of this for us.

Since you signed off, does this mean you're cool with it going into master as-is, leaving the mouse pointer issue to be addressed later? FWIW I do confirm seeing the same behavior you've reported.

Thanks,

Lebbeous

[Michael Peters (mrpeters)]

I don't see the pointer as a blocker. This is good stuff, I think it should go in.

Maybe put a message at the top of the page hinting users that they can drag, drop, re-size the columns? That would sufficiently take care of any issue with the pointer, IMO.

[Thomas Berezansky (tsbere)]

I can see our users complaining (they love to complain) about the following:

1 - Even though this is arguably an improvement over it, the lack of the alternate strategy button.
2 - The lack of lines in the printed table to split cells, horizontally and vertically.
3 - The "limited" column selection.

The first I am less concerned about, but the second I suspect will be considered a major issue.

The third is because they don't understand how pull lists actually work and want things like the pickup library for the hold on the pull list.

*I* would like to see no patron or hold specific data (as that is thrown out at capture) on the list of options, though I could make an argument for hold type as far as that goes (for a title level hold you may care less about the "right" barcode, but a copy level you need the correct item no matter what).

[Lebbeous Fogle-Weekley (lebbeous)]

Ok, how about this:

For 1, I can put back the alternate strategy button. Although when we get nearer some future post 2.2 release, we may wish to consider, based on feedback, whether to remove some of the various pull list interfaces. Users don't like things being taken away, but they also don't like having too many ways to do one thing (see serials).

For 2, can you live with an amendment to the commit message that tells site admins if they want something like that, they just need to add CSS in FlatFielder.xsl, which controls print output for Flattener-based grids?

For 3, I can add hold type; that's a good one. But if there's a less specific sense that there aren't enough columns on the pull list, I don't think that's something we have to worry about yet if we're not taking away the other interfaces. Where there are too many columns, users can hide them by control clicking the column headers to get the column picker dialog. Changes in that dialog should persist per-user if I add a user setting for it in a DB upgrade script to go along with this commit, so that's one more action item for me.

How's that?

[Thomas Berezansky (tsbere)]

For 1 I am starting to wonder if we shouldn't just remove all the buttons and turn it into a menupopup instead. Less clutter, but more clicks to get places.

For 2 I would be happier with the ability to specify an interface-specific xsl file to the backend (instead of a single hardcoded one) so that each interface wanting to spit data out as HTML would be able to have a different overall html output for style reasons, but notes on what to do are ok for now. If you *do* go nuts, for bonus points add an OU setting per xsl file for CSS to include for library specific styling. ;)

For 3 that sounds ok for the moment.

[Thomas Berezansky (tsbere)]

In talking about this internally we just noticed that "part" doesn't appear anywhere (alternate strategy has "barcode/part"). That can, I believe, be very important information as well.

[Kathy Lussier (klussier)]

I think we would need call number affixes here as well.

[Thomas Berezansky (tsbere)]

If I understand the code correctly the displayed call number is already being constructed from the prefix, call number itself, and suffix, with spaces between them.

[Lebbeous Fogle-Weekley (lebbeous)]

Thomas is right regarding displayed call number. It does already incorporate call number prefixes and suffixes.

Here's a compromise branch for now. It includes the original features, plus the hold_type column, plus enough CSS in the XSLT stylesheet that printouts have table cell borders. It also keeps the "alternate strategy" button. If the idea is to take nothing away from users for now, I think we shouldn't hide it under a menupopup either, although that could always be changed later.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/senator/simplified-hold-pull-list-compromise

Thomas, I am totally open to per-interface or per-OU XSLT being added in the future, but surely nothing is lost by adding what works now.

Later, in a separate LP bug, I will add features that automatically provide (hidden by default) columns from all the ahr core fields and all the other fields found by default in the existing pull list column picker (there are users that do want this, it turns out; my mistake).

But again I want to keep that separate in the name of one-thing-at-a-time. Let's just merge this and add the rest in separate commits when the rest is ready.

[Thomas Berezansky (tsbere)]

I have no objections to the lack of "bonus points" things I mentioned before. I also have no objection to adding minimal additional things (as most things are, IMO, useless and wasteful on a pull list). For the most part what is there is perfectly fine with me (even if those I work for would likely disagree).

HOWEVER, I do continue to object to the lack of part information! That can be very important, as comparing specific barcodes across an entire DVD season cataloged on a single bib and call number would be time consuming.

[Lebbeous Fogle-Weekley (lebbeous)]

Ok Thomas. Because somebody might use it and be dissatisfied with a missing feature, even though nothing's stopping them from just continuing to use the old interface, we won't merge it to master.

As I said in my last note, I'll be adding all the columns from ahr and all the rest of the things that appear in the existing pull list interface (including part information). Instead of doing it in a separate LP bug, I'll just update again here.

[Lebbeous Fogle-Weekley (lebbeous)]

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/senator/simplified-hpl

This has more fields. Including parts fields just like the alternate strategy interface (although I'm not sure that parts field is really right in either interface. Thomas, if you have P-type holds you can test this on, it would be much appreciated).

There is still a glitch with me not using the columnpicker quite right. Once you save your preferred display fields, you lose the ability to see the others anymore. I need to fix that, probably with Bill Erickson's help since he wrote the GridColumnPicker dialog.

This adds a ton of previously missing fields. Here are the fields that are not carried over from the traditional pull list interface (for good reason):

  cancel cause (makes no sense, canceled holds don't appear on the pull list)

  available time (makes no sense, holds on the shelf don't appear on the pull list)

  hold status (should always be 'waiting for capture' on the pull list)

  pickup_lib name/shortname (already visible with the "show the pull list for" dropdown, and displayed holds should always match that)

  transit source, transit dest, transit anything (holds on the pull list don't have transits yet)

All those fields were on the original pull list interface only because the same interface doubled as the view-hold-shelf interface.

Removing the pullrequest tag since there's still the glitch with saving columns, but still requesting testing, Thomas. Especially if you can let me know if it's still fast compared to the old interface with a lot of holds in the system. The queue_position approximation has been reworked to be hopefully much much faster than before.

[Thomas Berezansky (tsbere)]

My overall view:

The way the pull list works there is no guarantee that the hold that resulted in pull list inclusion is the hold that will be captured for. Thus, I see no reason for non-copy derived information on the pull list to begin with, with one exception: The Hold Type. Other than it I think everything on the pull list should be based on the copy, with the assumption that anything not tied to the copy on the list has the potential to change before checkin and capture happens.

I only think Hold Type is important because for a Title hold you may be able to get away with grabbing any of several copies, but for a Copy hold you need the exact copy.

Everything else that I see as important *for a pull list* can be obtained from the copy:

Barcode is on the copy, as is the Copy Location.
Copy is attached to the Call Number, which is attached to the bib for author/title info.
Copy is also linked to the Part.

Though I have seen requests for publication date, I think the reasoning behind those requests can be covered with proper use of locations, and regardless is also linked via the Copy->Call Number->Bib Record link if it were to be included.

The patron and additional hold information is nothing more than filler data and is, IMO, a waste of space and bandwidth for the pull list (though I would like to see the hold ID returned regardless, even though there isn't any reason to print it, as I have thoughts on future use of the resulting pull list view that would need it).

If we *do* include patron/hold specific fields (which I am sure some people want if only due to misunderstandings about how holds in Evergreen work) I highly disagree with calculated items like queue position (you are pulling a copy to fill a hold, it is going to fill position #1 no matter what, right?) and things like the request date.

Unfortunately for my own sanity, if we *do* include patron fields, I can make arguments for name fields, home library, and permission group. For additional hold information I could see pickup library....and that is about it.

Also:

'pickup_lib name/shortname (already visible with the "show the pull list for" dropdown, and displayed holds should always match that)'

The thing visible in the dropdown is the copy's circ library, not the pickup library.

[Mike Rylander (mrylander)]

Thomas, as part of this development we agreed to include everything that is available in the XUL pull list today.

Also, this work may very well form the basis of a faster replacement interface for holds-on-this-record (where non-copy fields are critical) among other interfaces, and ripping some things out just to write them is a silly waste of time.

Unless something is not functioning (other than the already identified column picker issue, to be fixed asap) or you can show empirically that there is a very substantial speed increase to be had by not including what is available in prior interfaces, then let's leave this as-is, please.

--miker

[Thomas Berezansky (tsbere)]

Ok Mike, I decided to put the new code to the test in the database, as I understand how it would be run by anyone building a query through IDL-based calls.

Thus, I took the new database code from the branch and installed it in a test DB copied from production data based on the upgrade script.

I then took the ahopl query from fm_IDL.xml and built a very simple query around it:

SELECT * FROM (ahopl query) AS ahopl JOIN asset.copy acp ON ahopl.target_copy = acp.id
WHERE acp.circ_lib = 33

I gave up waiting 5 minutes in and canceled the query on the basis that the client would have timed out by then and wanting to see what would happen after removing a few things I see no use for on the pull list.

To keep things simple and on the assumption that they are the biggest problem I commented out only the ahawt and ahqa elements of ahopl and re-ran the query, rather than reducing it to only the fields I feel belong on a pull list. It returned 136 holds in 3 seconds.

I then took 4 of those hold IDs, added them to the end of the query with an "AND ahopl.id IN (id1, id2, id3, id4)".

With the ahawt/ahqa elements commented out it returned in 42ms.

With the ahawt/ahqa elements restored to the query it returned in 15 minutes.

I also ran the ahopl query with the ahawt/ahqa elements commented out standalone. 1983 rows, under 4 seconds.

Same bit, but exactly as it appears in fm_IDL.xml I gave up on after half an hour.

I then explain analysed the ahopl as it exists in fm_IDL wrapped in a query limiting to one hold ID. The output from that is here:
http://pastebin.com/PLjZmfsh

All of the tests were done on a static database not being touched by outside queries with 949378 rows in action.hold_request, 87308 of which represent non-canceled, non-captured, non-fulfilled holds.

I also then checked against frozen holds, and removal of them reduces the row count for viable holds to 75532, but none of the new code in the branch appears to filter on that.

I believe the above is evidence of a "very substantial speed increase" as well as a likely case of the code not functioning, the latter because I suspect the client will never get results before it gives up with the latest code presented, at least on our data.

[Mike Rylander (mrylander)]

ahopl doesn't have a field in the IDL called target_copy. Perhaps you're testing something else accidentally? What, by the way, is the purpose of the attempted test query? Sorry, I think your test is fundamentally incorrect.

[Thomas Berezansky (tsbere)]

The column name was a mistake when typing up the response in Launchpad, I meant current_copy. Likely due to me looking at other parts of the system while waiting on queries. If I had been running the actual queries with the wrong column names I would have gotten errors back pretty much right away.

As for my test queries, the overall goal was to see what would happen if one attempted to use the ahopl IDL view to generate a pull list. To do so you would need to either:

1 - Link to the asset.copy to get the circ library of the copy and filter on that

or

2 - Get *all* "pull list" holds from everywhere so that you could do said filtering client side (requesting copy information as you go along, or fleshing copy information and filtering as you get results back).

I am not sure which method the flattener stuff uses. The first method was tested with my first couple of queries with the join to asset.copy and limit the circ library, the second was tested with the last couple of queries before the explain analyse where I ran the query without any wrappers or limits.

[Mike Rylander (mrylander)]

On (1), fair enough:

diff --git a/Open-ILS/examples/fm_IDL.xml b/Open-ILS/examples/fm_IDL.xml
index 1ff2e35..2c7226e 100644
--- a/Open-ILS/examples/fm_IDL.xml
+++ b/Open-ILS/examples/fm_IDL.xml
@@ -4864,7 +4864,8 @@ SELECT usr,
                                ahqa.queue_position::INTEGER,
                                au.home_ou
                        ) AS estimated_wait_time,
- (ahr.usr <> ahr.requestor) AS is_staff_hold
+ (ahr.usr <> ahr.requestor) AS is_staff_hold,
+ acp.circ_lib AS copy_circ_lib
                FROM action.hold_request ahr
                JOIN action.hold_avg_wait_time ahawt ON (ahawt.hold = ahr.id)
                JOIN action.hold_queue_approximation ahqa
@@ -4935,6 +4936,7 @@ SELECT usr,
                        <field reporter:label="Number of Potential Copies" name="num_potentials" reporter:datatype="int" />
                        <field reporter:label="Estimated Wait Time" name="estimated_wait_time" reporter:datatype="interval" />
                        <field reporter:label="Is Staff Hold?" name="is_staff_hold" reporter:datatype="bool" />
+ <field reporter:label="Copy Circ Lib" name="copy_circ_lib" reporter:datatype="org_unit" />
                </fields>
                <links>
                        <link field="fulfillment_lib" reltype="has_a" key="id" map="" class="aou"/>
@@ -4952,6 +4954,7 @@ SELECT usr,
                        <link field="cancel_cause" reltype="might_have" key="id" map="" class="ahrcc"/>
                        <link field="notes" reltype="has_many" key="hold" map="" class="ahrn"/>
                        <link field="current_shelf_lib" reltype="has_a" key="id" map="" class="aou"/>
+ <link field="copy_circ_lib" reltype="has_a" key="id" map="" class="aou"/>
                        <link field="sms_carrier" reltype="has_a" key="id" map="" class="csc"/>
                </links>
                <permacrud xmlns="http://open-ils.org/spec/opensrf/IDL/permacrud/v1">
diff --git a/Open-ILS/src/templates/circ/hold_pull_list.tt2 b/Open-ILS/src/templates/circ/hold_pull_list.tt2
index 3e9cccf..1f0a4e2 100644
--- a/Open-ILS/src/templates/circ/hold_pull_list.tt2
+++ b/Open-ILS/src/templates/circ/hold_pull_list.tt2
@@ -6,10 +6,6 @@
     dojo.require("openils.widget.FlattenerGrid");

     var map_extras = {
- "copy_circ_lib": {
- "path": "current_copy.circ_lib",
- "filter": true
- },
         "call_number_sort_key": {
             "path": "current_copy.call_number.label_sortkey",
             "sort" :true
@@ -86,6 +82,7 @@
                 <th field="parts" fpath="current_copy.parts.label" fsort="false">Parts</th>
                 <th field="notes" fpath="notes.body" fsort="false" _visible="false">Hold Notes</th>
                 <th field="patron_barcode" fpath="usr.card.barcode" _visible="false">Patron Barcode</th>
+ <th field="copy_circ_lib" fpath="copy_circ_lib" _visible="false">Copy Circulating Library</th>
                 <th field="request_lib_name" fpath="request_lib.name" _visible="false">Request Library</th>
                 <th ...

[Thomas Berezansky (tsbere)]

Moving the circ library into the ahopl view does not solve the problem with the estimated wait time and queue position related pieces slowing the query down until it is unusable by the staff client, as that slowness has nothing to do with the extra join to asset.copy. It does, however, nicely solve the problem of things possibly grabbing the entire pull list for everywhere and then filtering it after the DB has returned later.

Going from "response in seconds" without the estimated wait time and queue position pieces to "no response in minutes" with them is an argument for one or both of the following:

1 - Not including those pieces in a view intended for a pull list, at all

2 - The methods being used to generate them are too inefficient and/or time consuming and need significant reworking

To quote you:

"Unless something is not functioning (other than the already identified column picker issue, to be fixed asap) or you can show empirically that there is a very substantial speed increase to be had by not including what is available in prior interfaces, then let's leave this as-is, please."

I believe I have shown that:

1 - There is a very substantial speed increase to be had by not including estimated wait time and queue position related information. Minutes compared to Seconds or even Milliseconds is substantial. I should note that the shared XUL interface is, to my knowledge, the only one that had this information in particular, and that said information was likely never intended for pull lists.

2 - Something is not functioning, because the client will time out before it ever renders the pull list on our production data when the estimated wait time and queue position information is being included.

I will concede that libraries want useless information about patrons and the hold itself on their pull lists for reasons I cannot understand, but I *will* argue against the inclusion of calculated fields that provide nothing of value for a pull list and are slowing the generation of said list down significantly.

[Lebbeous Fogle-Weekley (lebbeous)]

Thomas,

(By the way, it was nice to work with you in person at the conference).

How does this do to address the most crucial issues? http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/senator/simplified-hpl-unslow

It removes all the fields deriving from ahawt and ahqa, i.e. the hopelessly slow stuff. It restores pickup_lib name and shortname, since as you point out, those are indeed useful.

There are some other bugfixes not mentioned here I think, including ones to the column picker thanks to Bill.

We actually don't need to push this to master for now, because we're in the freeze. Back to rel_2_2 stuff after this, but I'd appreciate your feedback when you can test it.

[Lebbeous Fogle-Weekley (lebbeous)]

Thomas,

The freeze on new features to master being over, is this branch now in a state that you can live with? See my comment #24.

Thanks,

Lebbeous

[Dan Scott (denials)]

I'd personally like to see a changeset hit the Release Notes as part
of this branch (and future branches, in general) prior to merging.

[Thomas Berezansky (tsbere)]

I placed this in my crappy dev VM, and then placed 363398 holds via direct DB activity.

After waiting a while for the hold targeter to target a good portion of those holds I ended up with 687 holds at BR1 for pulling.

The server still under loads of 4+ I was able to load the new interface for viewing in a few seconds (5-6 tops) and it took around 56 seconds from clicking on "Print Pull List" to actually getting a print dialog. The server load had gone up to 5-6 or so during that time.

I suspect it is fast enough now.

[Lebbeous Fogle-Weekley (lebbeous)]

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/senator/simplified-hpl-unslow-2012-05-24

Just a rebased update, no real changes.

[Mike Rylander (mrylander)]

Merged to master. SPEEEEEEEEEEEEEED!