Seamless checkout of non-standard copy status AKA single-use copy statuses

Bug #1464709 reported by Bill Erickson on 2015-06-12
22
This bug affects 5 people
Affects Status Importance Assigned to Milestone
Evergreen
Wishlist
Unassigned

Bug Description

Wishlist:

Relevant IRC discussion: http://irc.evergreen-ils.org/evergreen/2015-04-28#i_172493

Desired work flow:

1. Create a custom copy status named (for example) "Grand Opening".

2. Configure the copy status to allow seamless checkouts (via new mechanism).

3. Checking out an item with status "Grand Opening" occurs with no alerts, overrides, or special permissions. It behaves just like an "available" copy.

4. Once checked out, the copy behaves as any other copy and traverses the usual "checked out" -> "reshelving" -> "available" statuses.

For our part, the "Grand Opening" status is used to prevent holds on each copy at new branches until each has been checked out once -- IOW "local patron dibs". I've heard from others that supporting this with their local "Display" and other statuses would be useful.

--

My proposal is to add a new column to config.copy_status called "checkout_ok" (or whatever -- labels are hard). When set to TRUE, the circ matrix allows the checkout just like it would an available (or reshelving or holds shelf) copy. It does not "fail" on the copy status.

Code to implement this en route. No staff client changes are needed apart from the copy status admin UI.

An open question is whether this bool should be changeable on stock statuses. I propose yes, since it could be useful. For simple overrides, like MISSING item checkout, this would also work, assuming you want global seamless checkout. It would not work for LOST, though, since that has extra layers of logic beyond the circ matrix warning.

--

An alternate approach discussed in IRC was to use a client-side auto-magic overrride mechanism based on http://masslnc.org/node/2764#AL-03:_Library_Setting_for_Some_System_Alerts

I opted instead for the other route, because overrides represent a noteworthy event. The whole point is that these items are not noteworthy to anyone. They circulate like anything else. Also, overrides require an extra network round-trip, adding overhead, which makes little sense for events that will always be overridden. And, finally, the masslnc code linked above will be browser client only, which is certainly years away for us. So, instead of coding a local hack, I'm posting my alternate solution. Comments appreciated as always.

Bill Erickson (berick) wrote :

FWIW, we're running this code in production now. Works as advertised. Branch includes pgtap tests, but still needs release notes. Will add those and rebase to current master.

Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
milestone: none → 2.next
tags: added: pullrequest
tags: added: needsreleasenote
removed: pullrequest
Changed in evergreen:
milestone: 2.next → none
assignee: nobody → Bill Erickson (berick)
Galen Charlton (gmc) on 2015-08-26
summary: - Seemless checkout of non-standard copy status AKA single-use copy
+ Seamless checkout of non-standard copy status AKA single-use copy
statuses
Mike Rylander (mrylander) wrote :

There are several places in the code that would like to know what statuses are "available-equivalent". Should we use this in all those?

One drawback I can come up with: the hold targetter could end up pulling useless copies to test. But it will still look at the status' holdable flag, and allows for more flexibility in configuration. It would also keep us from missing the inclusion of "reserves" as has happened in some places.

If we wanted to do that, the flag should probably be renamed to "is_available" or the like.

Thoughts?

Dan Wells (dbw2) wrote :

I agree with Mike's comment. We've had a "New Book" status for a while (which could, in some ways, be better served as a temporary location, but that's a whole nother issue). We didn't want to limit holds on these, and we had to tweak a number of places (in addition to those the current branch touches) to get the items to target and show up on the pull list. It makes a lot of sense to at least consider all of the current 0/7/8 test spots to make this new flag a little more comprehensive.

P.S. Unless we somehow reach an impasse, I have no intention to try and help with the label :)

Bill Erickson (berick) wrote :

I like it. I can push a field renaming commit if that's helpful. I could use some assistance on the remaining bits, though.

Dan Wells (dbw2) wrote :

Bill, here is a sample branch with the three commits we've used locally. I'm sure Mike knows these areas like the back of his hand and can speak better for the accuracy and completeness of the changes, but it's what's worked for us so far, so I thought I'd throw it out there as something to work from.

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

working/collab/dbwells/sample_available_status_code

Dan Wells (dbw2) wrote :

P.S. It is three commits. The comments explain what each is trying to do.

Mike Rylander (mrylander) wrote :

/me looks at hand ...

Dan, those are the major spots, for sure. Starting with those seems sane, and then we can trawl the code for stragglers.

Thanks!

Bill Erickson (berick) wrote :

Thanks, Dan!

Pushed a new master-rebased branch.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1464709-copy-status-is-available

Includes renaming commit and commit implementing Dan's holds-related changes. Holds changes also include change to the IDL version of the pull list generator.

Code is untested, just wanted to get it published. Will test soon.

Bill Erickson (berick) wrote :

Pushed some fixes and started testing. Confirmed copies with a custom status with is_available=true are targeted for holds and appear on all 3 varieties of holds pull list accessible from the staff client. Also confirmed checkout without alerts are still working.

Still needs release notes and maybe some squashing.

Mike Rylander (mrylander) wrote :

The #available modifier in QueryParser should probably look at this, too. If someone else doesn't get to it, I'll try to do so soon.

Dan Wells (dbw2) wrote :

Does anyone have more thoughts about the checkin side? I think it makes sense for these statuses to default to "reshelving" if checked in (that is, unless the given status has specified handling already). Right now I think it is going to throw a "bad status" exception, which doesn't seem ideal.

Bill Erickson (berick) wrote :

Dan, I agree they should go to "reshelving" at checkin, since that's what they would do now, minus special handling. I also think it makes sense to avoid the bad-status exception.

Bill Erickson (berick) on 2016-08-15
Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
Mike Rylander (mrylander) wrote :

Bill, your change to the bug brought this back into view for me. Have you looked at the overlap with your in-db hold targetting patch?

Bill Erickson (berick) wrote :

Hi Mike, I did have this in the back of my mind when working on bug #1596595. It would be a trivial change to keep #1596595 synchronized, very similar to the changes applied to the current hold targeter in the branch for this bug. If we move forward with these, I'll create a version of bug #1596595 that sits atop this branch.

Mike Rylander (mrylander) wrote :

Almost a year later ...

I've taught search about the new flag in this branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp1464709-copy-status-is-available

Looks like it needs a rebase...

Bill Erickson (berick) wrote :

Thanks, Mike.

I started re-reviewing the code for rebase and testing...

I have some concerns about treating is_available copies as targetable. (A little late, I know). Saying it's OK to check out an item is not the same as saying it should appear on the holds pull list. I fear this might cause some confusion.

I can imagine scenarios where it makes perfect sense to put a copy into a temporary status where the items can check out and be considered as potential copies (for op-capture), but not appear on the pull list. In such cases, when the items are checked in, holds capture just works (w/o requiring any manual checkin retargeting, which has its own drawbacks).

This suggests two flags, is_available and is_targetable (the latter a subset of the former). Am I just overthinking this?

In any event, the code we have now will have to be modified to treat on-holds-shelf as a special snowflake, since such items should not be targetable or considered #available in the catalog.

I can look at the on-holds-shelf changes, but will wait for feedback on my other comments first...

Mike Rylander (mrylander) wrote :

Hrm... unless I'm mistaken, that's entirely orthogonal. What I mean is, wouldn't the holdable flag on the status allow disqualifying the copy at targetting time? I'm just looking to replace all checks for "available/reshelving/sometimes-reserved" (of which the hold targetter is only one offender) with a dynamic status list based on this flag.

Thomas Berezansky (tsbere) wrote :

IMO, "Can be checked out" should not be the same thing as "Can show up on the pull list". "On Holds Shelf" is a perfect example of that, in fact. You can check the copy out, but it shouldn't be on the pull list.

I see the holdable flag as covering "Can be on the *potentials* list". As this covers whether or not there are possible copies for a hold, already placed or otherwise, this is important to allow even for things you don't want on the pull list right now, but will be available to fill the hold later. This covers things like "On Order" and "In Process" that shouldn't be checked out or on the pull list, but can be on the potentials list.

Based on that, I personally think that we should either keep the hold targeter hardcoded to 0 and 7 or we should have a flag for "this status makes a copy eligible for the pull list if it is also a potential copy".

Michele Morgan (mmorgan) wrote :

I share Bill's concern also:

"Saying it's OK to check out an item is not the same as saying it should appear on the holds pull list."

The holdable flag on the copy status is too strict, often requiring staff to retarget to capture a hold.

It seems to me that is_available and is_targetable are really two different things, so I would be in favor of the two flags.

I would also be in favor of removing the hardcoding of 0 and 7 in favor of an is_targetable flag.

Bill Erickson (berick) wrote :

I think we're preaching to similar choirs, Thomas. The specific examples of "On Order" and "In Process" would not be effected by this code, though, since they would be configured as is_available=false -- no seamless checkout or pull-list.

I can't think of any stock statuses that would not play nicely with the proposed code, minus on-holds-shelf, which will require custom handling regardless.

The scenario I'm describing is basically a new feature. In that sense, yes, Mike I do see it as sort of orthogonal. But I also see it as very closely related, so I wanted to make sure we aired our grievances.

Mike Rylander (mrylander) wrote :

I do think that an "is_targetable" flag is a good feature, for all the reasons stated above. But, to be clear, the original post says:

  For our part, the "Grand Opening" status is used to prevent holds on
  each copy at new branches until each has been checked out once -- IOW
  "local patron dibs". I've heard from others that supporting this
  with their local "Display" and other statuses would be useful.

Given the original scope, I think adding the second flag now definitely expands the the scope of the code significantly by touching hold targetting in a new way. While there's little time left, I'm happy with the code at the top of http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp1464709-copy-status-is-available (if someone would like to test the new search part, that is) and would like to see it go into 2.11, but I think the addition of is_targetable will be a blocker to that with only today and tomorrow left to code and test.

There's one change I'd like to see, which I can make a master commit time if I can get buy-in: I think Reserves should be is_available=TRUE.

Thoughts? Thanks.

Thomas Berezansky (tsbere) wrote :

I am going to clarify my previous comment: I believe that the proposed code should either *not change* the hold targeter (remove the commits changing it to check is_available) or should introduce a new flag for it to check (which I won't argue being out of scope here). I personally do not see using "is_available" as written as something that should be used by the hold targeter for "can be on the pull list".

For the record, that is currently the only real problem I have with the proposed changes and I have no problem with something like is_targetable (though perhaps "is_pullable" would be a better name?) being a different project entirely.

Going back to the original description regarding the "Grand Opening" status: I read it as "should not be pulled for holds". That, to me, does *not* say "this copy should not be considered a possible copy for placing holds" as it should, by definition, go back to a holdable status automatically later, so it will fill holds once a local patron has checked it out. Thus, in my interpretation of things, the "holdable" flag blocks *too much* and is not suitable.

By using the "is_available" flag for the pull list you are either forcing the status to block holds *entirely* when all copies are in it (holdable false) or forcing libraries to ignore it on the pull list if they aren't supposed to be pulling it (holdable true, but they don't want to pull it).

Bill Erickson (berick) wrote :

Agreed is_targetable is an expansion of the scope of this bug. But so was changing checkout_ok to is_available to cover hold targeting and #available. The door has been opened..

Before any branches are merged, we have to update the working branch to set is_available=false for On Holds Shelf and tweak the checkout SQL to allow their checkout. Otherwise, On Holds Shelf items would be targetable and #available. I'll look at this today if I can.

For Reserves, are we confident no one is setting holdable=true for the Reserves status locally? (I would not expect it, but I really don't know) If they are, setting is_available=true would cause Reserves items to appear on the pull list, which would probably be unexpected. At minimum, the release notes would need to make this very obvious.

If we agree is_targetable would be a valuable addition, my preference would be to implement it as part of this bug and delay the feature if necessary, mainly because I see the existing code as a potential foot-gun. I'll help push it along either way, though.

Mike Rylander (mrylander) wrote :

Bill,

I'd call that a thinko in implementation, personally, and it belongs to me as well. This started as a circ-only change, but we (you, Dan, and I) let it slip into holds without recognizing the on-hold-shelf problem. If we rein it back in to cover only circ, and leave is_targetable to another branch and bug, I think this can go into 2.11. To do that, I think we only need to elide the changes to action.pm.

Opinions?

Bill Erickson (berick) wrote :

Mike, agreed, moving forward with just the circ (not hold) changes sounds good.

To recap:

1. Revert the holds bits (targeter, pull list changes).
2. Keep the link between #available and is_available (yes?)
3. Assuming #2, add a special case for on-holds-shelf so they can freely circulate w/o appearing as #available in the catalog. (Setting is_available=false for on-holds-shelf and tweaking circ SQL).
4. Release notes.
5. Rebase/squash

Mike Rylander (mrylander) wrote :

Regarding (3), I assume that's because of http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=5990d303faba89ca5ca593d6554d9078ecedc8d6#patch3 right? If so, I would turn it around, and make the stored proc do this:

...
    IF NOT renewal AND item_object.status <> 8 AND item_object.status NOT IN (
      (SELECT id FROM config.copy_status WHERE checkout_ok) ) THEN
...

and remove on-hold-shelf from is_available. I can't see on-hold-shelf-ness becoming a flag of it's own, and statuses are still magic numbers, so this doesn't hurt me too bad. Eh?

If you agree, I think the plan is:

1. Revert hold bits
2. Set is_available=F for on-hold-shelf
2. Adjust circ stored proc as described here
3. Keep #available as is, no special case
4. Release notes
5. Rebase

I can help with 1-3 if you want.

Bill Erickson (berick) wrote :

Yes, cool, we're on the same page. (That's what I was trying to hand-wavily say with "setting is_available=false for on-holds-shelf and tweaking circ SQL").

I'll take you up on your offer, Mike, to help with 1-3, then I'll take care of 4 & 5.

Thanks!

Mike Rylander (mrylander) wrote :

I've updated my branch with a commit that takes care of 1-3, I believe. Please eyeball!

Bill Erickson (berick) wrote :

New branch pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1464709-copy-status-is-available-rebase1

It incorporates both of your commits, Mike, with sign-offs. A few more cleanup bits were needed (pull list stuff, pgtap fix), so during rebase, I simply avoiding committing any of the hold targeter/pull list changes. Less cruft this way. The 2nd/cleanup commit was trimmed to just the on-holds-shelf status repairs.

Includes release notes. I believe once my commits are signed off, it will be ready for merge.

Mike Rylander (mrylander) wrote :

So it is written, so it shall be done. Master updated. Thanks, Bill!

Changed in evergreen:
status: New → Fix Committed
Changed in evergreen:
milestone: none → 2.11-beta
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers