Deleting parts breaks hold interfaces

[Thomas Berezansky]

When you delete parts that have holds placed on them interfaces that show holds break with a Network Failure message.

This appears to stem from the fact that the part was the only thing liking the hold to a bib, and the part no longer exists in the database.

I am unsure as to what the solution should be, but I have ideas. They each have issues, however, and are currently based on the fact that to my knowledge the deletions are happening via PCrud.

1 - A database trigger that deletes the unfilled holds when the part is deleted. This has the downside of staff not even knowing what holds may or may not have existed.

2 - A database trigger that prevents deletion so long as unfilled holds exist. This has the downside of staff possibly being incapable of dealing with the holds, especially as canceled holds seem to have the same problem as non-canceled ones. It also has the downside of staff having to do more work.

3 - A combination of the above two: A database trigger that prevents deletion on non-filled non-canceled holds existing, but that deletes the rows for any canceled holds. Still retains most of the downsides of the two.

4 - A database trigger that cancels the holds and converts them to Title holds. This allows the holds to remain with their record link, but may otherwise be invalid. This may be desired for filled holds as well, for reporting/statistical purposes. Issues include staff not realizing that the holds have been canceled to inform patrons.

All of the above have the additional issue of the middle layer not knowing that the hold(s) have been altered, removed, etc so no automated notifications can go out.

5 - Re-write the interface to use something other than PCrud. This would allow for things like a prompt for what to do with the affected holds, "merging" parts, calling the hold targeter, etc. This would likely take the longest, but could be the most complete, as it could ensure that the proper middle layer calls are made for automatic notifications and such.

[Mike Rylander (mrylander)]

6 - A pre-delete UI check for using-holds, which offers to delete the holds IFF the staff member has a reasonable permission.

[Galen Charlton (gmc)]

Just a note that another customer of ours has encountered this bug in the wild. Either #5 or #6 would be my preference, with a slight preference for #5 to avoid a potential race condition.

[Mike Rylander (mrylander)]

Another possible option is to add a "deleted" flag to parts. Most other holdable objects have such a field in order to retain reportability of the original objects. This would touch the most code, but be most consistent with what we do now for bibs, call numbers and copies.

[Mike Rylander (mrylander)]

It ended up not taking too much effort to get something that might work, so I offer a branch implemented fake-delete ("deleted" flag) for mono parts. Consider it a early, untested proof-of-concept.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/fake-delete-parts

[Dan Scott (denials)]

Removing "pullrequest" tag - if it's an "early, untested proof-of-concept", it's not something we want pulled into a release branch.

[Mike Rylander (mrylander)]

That's perfectly fair, Dan!

I don't want to start a long discussion here that should happen on the -dev list, but I should mention that I weep for the lack of more "blessed" tags than pullrequest, particularly for the purpose of getting attention on a bug that I lack the tuits to fully attack, but nonetheless want to assist with.

[Kathy Lussier (klussier)]

Mike,

If I'm understanding this last option correctly, it sounds like it would resolve the problem with the error message, but would likely lead to holds that are unfillable. Let's say a patron has placed a hold on Discs 1-2. For whatever reason, staff need to delete that part and the delete flag is set to true. At this point, the patron's hold is still set for parts 1-2 but can never be filled. I think it would be important to have some kind of warning before staff delete the part letting them know that there are existing holds on that part. They could then manage the holds by transferring them to other parts that would cover their needs (perhaps Discs 1-3 or Discs 1&2 instead of Discs 1-2) or maybe making them all title level.

[Thomas Berezansky (tsbere)]

I don't see why deleting a part should generate warnings when I have been told by staff that you can delete copies, volumes, and bibs without warnings about existing holds that will be unable to be filled. The last record that a metarecord hold could fill from could go away without warning as well.

The same goes for removing the part from the last copy it was on, for that matter. No more copies can fill holds on that part.

Also, the current situation not only leads to holds that are unfillable, but those holds are also *broken* in the process with no record of what they were once on.

I will point out that at least one of our libraries has asked for a warning when a delete will render holds unfillable, regardless of what is being deleted. I don't know how that should be answered, though, as it could be that a copy deleted while there were dozens left on the bib was the last one that could fill holds based on hold rules.

[Thomas Berezansky (tsbere)]

After taking another look, deleting Bibs/Volumes/Copies does look like it may auto-cancel holds, but there is still no warning to staff that this will happen.

So deleting parts should probably trigger that.....but that process happens in the perl layer, and the part deletion stuff happens via PCrud (I believe). So to get that working for parts the interface would need to be changed to use a different API for deleting.

[Jason Etheridge (phasefx)]

One mitigating (if only slightly) event may be
hold_request.cancel.expire_no_target. Does that eventually kick off?
It sends an email notice to the hold recipient.

[Jason Etheridge (phasefx)]

another semi-random thought.. a "pre-delete UI" could become a more generic "transform hold type/target" UI that could be invoked at any time, not just when a hold target is getting deleted

[Kathy Lussier (klussier)]

Hi Jason,

I think a pre-delete UI could be very helpful in many situations. As an example, if there were a way to transfer title holds to parts holds or vice versa, people may find it easier to manage issues with parts holds.

However, we would still need something that automatically brings up that UI when a hold target is being deleted.

Kathy

[Mike Rylander (mrylander)]

I'll be giving this a good poke, so to sum up, the bug-fix plan will be to implement the deleted flag for parts and allow the hold_request.cancel.expire_no_target A/T hook to clean up the unfillables as Jason pointed out above. This brings part holds to parity with other hold types, where having an unfilled expire time setting causes notified cancellation.

The generalized "move parts before deleting objects" popup is a good wishlist feature request, so I'll create that LP bug once the bug fix part is done.

[Mike Rylander (mrylander)]

Here's a branch, updated from my above WIP:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp937789-updated-fake-delete-parts

[Galen Charlton (gmc)]

I've run Mike's patch through its paces and written a follow-up that ensures that acpm gets cleared when a part is deleted. The follow-up patch also adds test cases and a test plan.

The two-patch series is available in the user/gmcharlt/lp937789_logical_parts_deletion branch in the working/Evergreen repository:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/gmcharlt/lp937789_logical_parts_deletion

[Remington Steed (rjs7)]

Tested and works as described. I added a commit to hide the "deleted" field from the Manage Parts dojo table and editor. Here's my branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/rsteed/lp937789_logical_parts_deletion

[Ben Shum (bshum)]

Tested and seems fine for master. Backported to rel_2_9, but rel_2_8 was not a clean merge, apparently cause of differences in the webstaff client. Not sure if we want to fix it there or not, will look more closely when we can.

[Ben Shum (bshum)]

Merged a version of this fix back to rel_2_8 without the webstaff change.

Cheerio.