UpdateItemWhenLostFromHoldList lets one update any value in the items table when cancelling a hold from the Holds to Pull list. That includes things that aren't normally editable. For instance, one can use it to change itemnumbers. It doesn't seem likely anyone will set it up to do so, but the potential damage is pretty severe.
What do you suggest, Andrew? Should we list the attributes that could be edited?
Back on Bug 19287, I only see folks asking for the ability to change itemlost and/or notforloan (though it's a long bug, I could be missing a request for something else). Honestly, my inclination would be to limit this syspref to only those two fields. If, on the other hand, we want to be permissive and allow fields to be edited unless there's a compelling reason to exclude them, then here are the fields I think we should definitely prevent changes to: - itemnumber - biblionumber - biblioitemnumber - barcode - dateaccessioned - datelastborrowed - datelastseen - damaged_on - itemlost_on - withdrawn_on - issues - renewals - reserves - paidfor - permanent_location - itype Those are fields that either we generally let the system manage or have the potential to be really disruptive if changed.
Hey Andrew and Jonathan, just looking for some guidance here. How do you envisage this working? We agree that many items fields should not be able to be edited, so we thought about adding checkboxes for the fields that can be edited such as itemlost and notforloan. But then we realised that itemlost will = 1 if the hold item gets marked as lost anyway so including that field feels redundant.. which only leaves notforloan. So do we change the whole syspref to 'UpdateItemNotForLoanWhenLostFromHoldList'? Or are there still other fields we should include? And how should this look? What do you think?
I am certainly the one to blame here, the original request on bug 19287 was about notforloan. I've asked Séverine few months ago to confirm they were using it only for notforloan, what she did. My dev's POV was to make it generic to not restrict the feature. So basically I don't mind renaming the syspref to make it work only for notforloan, the problem is... what do we do if there is something else in it? Maybe we should ask the list first? I am lowering the status of this bug report, it's not a major one. It is only problematic if the pref is intentionally filled in with bad values (which is true for other sysprefs as well).
(In reply to Jonathan Druart from comment #4) > I am certainly the one to blame here, the original request on bug 19287 was > about notforloan. Did you mean notforloan or lost? The bug suggests lost. I'm ok with re-working this syspref so it only controls one value, rather than being configurable to work with any/all parts of the item record.
(In reply to Andrew Fuerste-Henry from comment #5) > (In reply to Jonathan Druart from comment #4) > > I am certainly the one to blame here, the original request on bug 19287 was > > about notforloan. > > Did you mean notforloan or lost? The bug suggests lost. > I'm ok with re-working this syspref so it only controls one value, rather > than being configurable to work with any/all parts of the item record. I meant itemlost!
Hey. What if there were separate system preferences for setting itemlost and notforloan when items are removed from the hold list? To me, that'd make more sense than a checkbox system because it means a librarian cannot set itemlost to both 0 and 1, and it'd also ensure they cannot update any other values. I've started work on a checkbox system already, but I feel like its probably best to scrap it and replace it with 2 system preferences. If everyone agrees that that's the better option, I'll remove myself as the assignee because (right now) I don't really have time to do it. However if the checkbox system would work, I could probably get it done today.
I am not sure if this has not turned out as a non-issue over time by now with people not breaking things badly, although they could. They also might be using more than notforloan and itemlost... I think it could be nice to add an internal note too. I believe we cannot always prevent people from doing bad things. I feel like we might want to close WONTFIX. But if people disagree: instead of limiting the pref, could we use an allowlist/denylist of fields that will be updated in the background instead? I agree with Andrew's list in general, but would allow changing itype.
(In reply to Katrin Fischer from comment #8) > I am not sure if this has not turned out as a non-issue over time by now > with people not breaking things badly, although they could. They also might > be using more than notforloan and itemlost... I think it could be nice to > add an internal note too. > > I believe we cannot always prevent people from doing bad things. > > I feel like we might want to close WONTFIX. > > But if people disagree: instead of limiting the pref, could we use an > allowlist/denylist of fields that will be updated in the background instead? > > I agree with Andrew's list in general, but would allow changing itype.