DB Doesn't protect against precat merges

[Mike Rylander]

Following on from bug #792595, the UI now protects the magical precat bib (id=-1) from going into buckets, which is a primary entry point for being able to merge it. However, it's still possible to merge it in other ways (such as directly in the db, via the stored proc).

We should consider protecting all magic objects (bre.id=-1, acn.id=-1, acl.id=1, aou.id=1, au.id=1, etc) from merging, deletion, etc.

[Chris Sharp (chrissharp123)]

Similar, subsequent bug 1849334 - scoped to precat volume editing.

[Rogan Hamby (rogan-hamby)]

Should we prevent editing of bre -1 and acn -1 as well?

[Rogan Hamby (rogan-hamby)]

For the purpose of this bug I am going to focus on bre, acn and acl issues.

The UI for aou doesn't allow deletion unless all child orgs are deleted and the same will happen in the db. At that point I don't think protecting aou.id 1 is going to matter much if everything else is gone anyway.

I'm not sure about protecting au.id 1, it isn't always the admin user though it often is. Is often enough to encode those protections for it given that for some databases we may be encasing in stone a random user? I'm open to a new lp for it if we decide it is.

[Rogan Hamby (rogan-hamby)]

patch at https://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=ee1e7279543790435c7de453b02c76804803ad1c

What this patch does:

* adds rules to biblio.record_entry id -1, asset.copy_location id 1, asset.call_number id -1 so that edits are rejected for those ids on those tables

* asset.merge_record_assets has a condition at the top that returns 0 if either record is -1 to protect against db merges

* the staff client marc editor will hide the delete button if it is id -1 and disable the save button

I did not disable the save button for asset.copy_location id 1 because I didn't feel that those kinds of changes to the fm-editor component were worth it for such a niche case and probably should be handled more comprehensively than one off exceptions if we want to think about special exceptions that the component may handle and it is protected on the db level anyway

[Terran McCanna (tmccanna)]

This one appears to break my test server when I try to apply it to current master (3.7 beta). This is in the logs:

psql:800.fkeys.sql:40: ERROR: rule "protect_acl_id_1" for relation "copy_location" already exists

[Rogan Hamby (rogan-hamby)]

hmmm I'll apply it to a fresh test box over the weekend. I find it hard to imagine you already have a rule named that so I suspect that's not the actual issue.

[Rogan Hamby (rogan-hamby)]

Removing the pullrequest while we do some sleuthing.

[Rogan Hamby (rogan-hamby)]

Apparently copying and pasting is hard. Fix pushed to tip of bf94d78f859e8e7bdeb8265ec0a758cb9c21dad0

[Elaine Hardy (ehardy)]

Test on https://terran-master.gapines.org/eg/staff/

I created a precat record and attempted to add to a record bucket. Record would not add to the bucket (no message to indicate adding to a record bucket failed). Tested both with and without another record in the record bucket, just in case.

I could add the precat item to an item bucket and bring it up in item status. Also able to process as normal to transfer to a correct record. All good.

I have tested this and agree to sign off with my name, Elaine Hardy, and email - <email address hidden>.

[Elaine Hardy (ehardy)]

Thanks, Rogan!

[Terran McCanna (tmccanna)]

This seems like more of a bugfix than a wishlist to me so I've targeted to 3.6.4 instead of 3.next.

[Jason Boyer (jboyer)]

I dig it, and have pushed a squashed version of this to master, rel_3_7, and rel_3_6. Thanks Rogan and Elaine!

[PD Inc Support (pdinc-support)]

This patch conflicts with administration - and is not documented (in docs).

Would it not be better to disallow record deletion and PK updates only?

evergreen=# update asset.copy_location set deleted='t' where id=1;
UPDATE 0

The UI is frustrating users/admins as "Stacks" is not a nomenclature for this environment and they keep trying to rename it, disable visibility in OPAC, or delete it. The UI says saved, the action has no results.

[Galen Charlton (gmc)]

I agree that the rule that protects against deleting asset.copy_location ID 1 goes too far. I've opened a new bug (bug 2023314) for this, as generally speaking follow up issues on a fix-released bug should get a separate report.

[Mike Rylander (mrylander)]

Thanks for opening the new bug, Galen!

In the mean time before a full fix (though I see one is proposed on the other bug), for PD Inc Support, if it would be an improvement to the current situation to be able to use a string other than "Stacks" for the display label, you can supply a translation to change what is displayed by going to Administration -> Local Administration -> Shelving Locations Editor, selecting the "+ Ancestors" checkbox so that you can see the Stacks location, double-click the Stacks row, and use the Translate button by the Name field to supply an alternate string.