Flag/setting for Items with Specific Statuses to Display on Holds Pull List

Just for my own curiosity - why did you opt to create a Status for Storage rather than a Shelving Location? (Our libraries using shelving locations for this type of thing.)

We also use Shelving locations for Storage items, and those that are accessible have status Available and will appear on the pull list.

However, it would be valuable to be able to configure which statuses appear on the pull list. We have added a Display status, used for items that are put on Displays around the library. We would like items in that status to appear on the pull list. The advantage of using a status for the Display situation is that it clears when the item circulates.

Marking this Confirmed.

Ah, the Display status is a nice idea!

Storage status would work the same. Once checked out and back in, the item is no longer "in storage."

Thank you for all the comments!

We do suggest creating a shelving location for the Display and Storage situations. As pointed out, one benefit is avoiding the problem of the status clearing at check out and check in. Locations work well for Display at libraries with multiple display locations.

The status Storage was used in former systems we have used over the years and although we explore using a location with some of our libraries, they prefer the workflow of using the status.

Ah, one of those situations where flexibility is both good/bad at the same time :)

I could see where having the status clear might be a good thing. If you used the status in conjunction with a storage shelving location, you could keep an eye on items in that location with status available and reassess whether they should be moved back to the collection or kept in storage.

We have a display status, and a display location. We had to create the latter because of the inability of items in the status to be flagged for the pull list.

But the status is ideal for temporary displays for which you don't care if the item gets put back into display upon its return.

Copy status already has an is_available field making use of that should not be too difficult.

I have a branch which uses the holdable and is_available fields on the copy status to determine if items should appear on the pull list or be targeted for hold. This change only affects the copy status filters where statuses 0 and 7 were previously used. It should not affect any other criteria.

I'm waiting on getting this tested locally before linking the branch and adding the pullrequest tag here.

This may be related - https://bugs.launchpad.net/evergreen/+bug/1616170

I'm opening this up for general testing. We're not going to get to look at it again locally for some time.

If it needs a rebase, let me know!

If you're going to ask people to look at your code, it would be useful to add a link to the code so they can find it. Here's the branch all nice and rebased on master:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1904737-expand-pull-list-copy-status

Testing on Mobius Test Server for Feedback Fest. When I try to place a hold through the Angular catalog, the status stays at "Hold Pending." Holds are also not targeting and appearing on the pull list. When attempting to place a hold through the Traditional Catalog, I get an Internal Server Error. I'm unsure if this is due to the code, or something else.

Thanks for testing, Jessica!

The branch is a year old, and while rebased cleanly on master, that doesn't mean it still works. I did some light testing a year ago, but we never got around to thorough testing at CWMARS.

I'll take this bug back and have another look, myself.

Removing the 3.10 target for now, since it doesn't seem quite ready for the slush. If you wrap up your investigation by the feature freeze date, though, Jason, feel free to add that target back.

I have rebased the branch on latest master and tested it with a copy of CW MARS production data. I do not see either issue reported by Jessica Woolford in comment #14. Everything works as I expect it to work.

I placed holds through both the Bootstrap OPAC and the Angular Staff Client, both worked and had targets assigned immediately.

I also tested the hold targeter, and it adds potential copies based on the holdable and is_available flags on the copy status.

I am replacing needswork with the pullrequest tag.

Since I wrote the previous comment, I added a release notes document to the Circlation release notes to cover the change in behavior and the new hold targeter application setting (status_cache_time).

The branch is in the same place:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1904737-expand-pull-list-copy-status

I think the idea makes sense. Upon reading the code, some areas to poke at are:

- The cp_available_by_circ_lib_idx index on asset.copy is a partial index on statuses 0 and 7; would be good to see if it actually matters for the relevant queries
- The action.copy_related_hold_stats() stored function also hard-codes 0, 7 and 12 (the new reserves status); this affects hold ratio calculations when applying hold policies
- Likewise for asset.staff_lasso_record_copy_count() and asset.staff_ou_metarecord_copy_count() and their counts of items available to fill hold requests
- The ahopl IDL view - this is no longer used for the pull list, but theoretically could be used by some for reporting
- eg-holds-grids hard-codes a default grid filter and cp.status 0 and 7

I think these mostly amount to details to fill in as opposed to anything fatal to the proposed patch, though it also suggests to me that (separately) that the Reserves item status should have its magic made a bit more explicit.

Thanks for pointing those things out, Galen. I will take a look and see what additional changes need to be made.

Testing has so far shown that the cp_available_by_circ_lib_idx is relevant to the pull list query from Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm at least.

Using the new logic, we end up with a parallel seq scans on asset.copy and serial.unit whereas the hard coded status of 0 and 7 to bitmap heap scans on the indexes. The newer logic has consistently taken about 0.5 to 0.8 seconds longer than the logic with hard coded status values on my database with a copy of production data.

I have tried adding an index on config.copy_status where holdable and is_available are true. It makes no difference. It is not used by the new query logic.

I will check the other queries that have so far been modified.

I am not sure if I can come up with a modification to the new logic that will erase the performance degradation.

I'm attaching a zip file with 3 sample pull list queries that I've used to test the performance of the new approach and the effect of the cp_available_by_circ_lib_idx on performance in my previous comment.

I'm sharing these in case anyone else wants to have a look. This does not mean that I have given up on this feature.

I have new WIP branch that addresses all of Galen's concerns in comment #19.

I have also worked out a solution to the query performance. By adding a new index on asset.copy.circ_lib where deleted is false, the performance of the new queries is actually improved over the queries using the original index. This new index also improves the performance of the original queries when they are modified to use it.

I have also attached a revised query bundle that includes a modified version of the new query to exercise the new index on asset.copy so that others can compare the performance. The README in the bundle has also been modified to include the new index definition.

I have more testing and possible tweaking to do, but I expect to have a pull request branch ready in a week or so. I will rename the branch when I share it.

I have pushed a new branch to the working repository: user/dyrcona/lp1904737-expand-pull-list-copy-status-rebase -- https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1904737-expand-pull-list-copy-status-rebase

This is a rebase of the WIP branch mentioned above with PgTap regression tests for the index changes to the database as well as 64 additional tests in the Perl live tests for the hold targeter to exercies the new code for the hold targeter. More tests would be welcome.

You can test this via the staff client by running the hold targeter on the back end, then running a pull list. If you modify the statuses on some of the copies to those that are not holdable and not available, then you should get different results when you run pull list again. You can modify an existing status or add a new status that is holdable and avaialable, run the hold targeter, and then run the pull list again. Depending on circumstances you may see different copies on the pull list.

The code to calculate available copies for holds is also modified. This code is also not so obvious to test, but if you're starting from a known baseline, similar changes to copies and copy statuses as above should show up in the appropriate places in the OPAC and other interfaces.

John Amundson of CW MARS has tested the changes and said it was OK for me to add his signoff, so I did.

I want to add something that I mentioned in IRC today.

I think the discussion of whether statuses or copy locations is better suited for what folks are trying to do is irrelevant to the merit of the code and this feature.

The desired change stands on its own because it removes the hard coded values of statuses 0 and 7 being able to fill holds. These have been referred to as "magical statuses."

This code removes the magic and makes it obvious/configurable what statuses can fill holds. This can help us in the future if new default statuses come into being. If it is supposed to be available for a hold, we won't have to hunt through the code to make changes in many places. All we have to do is set the flag, and it's done.

I hope that others can see the value in that, regardless of any other discussion going on around this bug.

There is so much to like in this patch: good test coverage, removing hardcoded values in favor of configuration, and increased performance! Thanks, Jason and John. Checks all the boxes. Pushed to main for inclusion in 3.12.