|
Description
Andrew Fuerste-Henry
2022-07-19 14:27:58 UTC
Created attachment 146589 [details] [review] Bug 31187: Preserve permanent_location if it is mapped to a field in the editor To test: 1 - Map permanent_location to a marc field 2 - Expose that field in the item editor 3 - Cataloging -> Batch item modification 4 - Enter the barcode for an item 5 - Change the location of the item and retain the permanent location on saving 6 - note that permanent location is set to locatoin 7 - Apply patch 8 - Repeat 9 - Note permanent location is retained It seems we keep running into this kind of issue in some variations. Actually, I am also confused: When do we want to update the permanent item location and when do we not? My impression was that whenever the location is updated and the value is not CART or PROC, the we'd also like to make the permanent location match. It's been moved to a new location, so if it was checked out/checked in, set to CART, it should be able to go back to the new location. But now it feels a bit like we might have different uses of the field depending on configuration maybe? And if we do... should we do that? And if so, is this documented somewhere? Do we expect people to always set both manually if they are linked to MARC fields? What if $k was set to hidden in the item form? See bug 33301 for discussion the use case for this and a more formal way to configure temporary shelving locations. (In reply to Katrin Fischer from comment #2) > It seems we keep running into this kind of issue in some variations. This is why I add a test here, to prevent future breakage > Actually, I am also confused: > When do we want to update the permanent item location and when do we not? If it is visible in the form, but has no value then we want to preserve the original value and not set it to location > My impression was that whenever the location is updated and the value is not > CART or PROC, the we'd also like to make the permanent location match. It's > been moved to a new location, so if it was checked out/checked in, set to > CART, it should be able to go back to the new location. This was the case originally, however, it became possible to add both to the editors and set individual values. > But now it feels a bit like we might have different uses of the field > depending on configuration maybe? And if we do... should we do that? And if > so, is this documented somewhere? It may not be documented, but it has become a fairly common practice. I think we should document and support it. > Do we expect people to always set both manually if they are linked to MARC > fields? What if $k was set to hidden in the item form? If mapped and hidden, then it operates where a location update always updates both. If exposed in the form, i.e. we receive a value in the scripts, then we need to assess if the value is blank and preserve pre-edited value rather than matching to location I feel strongly that this has started as a 'hack', because it kinda worked... and now we are in trouble. I feel that making a behavior change dependent on a framework setting will always keep causing us trouble - it will always lead to bug reports about things being broken one way or the other for some, but others expecting it to work that way. And I see no easy way to fix this with comments or hints in framework/Koha-to-MARC mappings page Could we imagine making this more solid somehow if people really need it? What's the use case? Can you still use CART/PROC or is this exclusive? I think a system preference would work better and allow us to tie in with the other features using this database field in terms of documentation/linking. (In reply to Katrin Fischer from comment #5) > Could we imagine making this more solid somehow if people really need it? > What's the use case? Can you still use CART/PROC or is this exclusive? I'm not against moving this into a real feature rather than a possible modification. The use case here is a temporary shelving location to show that an item has been placed in a display. Items are pulled from the stacks and put in a display, the shelving location is set to DISPLAY so staff/patrons know the item is available but not at its usual spot. If those items are checked out, we don't want to try to get them back to the display on return -- that's too much staff hassle and likely the display will have been changed out before the item is returned anyway. This workaround makes it so that when the item is returned its shelving location automatically reverts to its regular value. This workflow does not impact the usability of CART or PROC. (In reply to Katrin Fischer from comment #5) > I feel strongly that this has started as a 'hack', because it kinda > worked... and now we are in trouble. This is how features develop :-) Covering it with tests and preventing breakages are a fair thing to do > I feel that making a behavior change dependent on a framework setting will > always keep causing us trouble - it will always lead to bug reports about > things being broken one way or the other for some, but others expecting it > to work that way. And I see no easy way to fix this with comments or hints > in framework/Koha-to-MARC mappings page We have plugins and mappings that are defined in the frameworks that alter behavior. I don't think this is unexpected. Editing the frameworks can have consequences and users should be aware, but we do expect Koha to work differently when the frameworks are altered. > Could we imagine making this more solid somehow if people really need it? > What's the use case? Can you still use CART/PROC or is this exclusive? I agree with Andrew - a fully fledged feature would be great here. But I don't think that is a blocker to this patch set > I think a system preference would work better and allow us to tie in with > the other features using this database field in terms of > documentation/linking. I am not against adding a syspref, but I am not sure what the switch would do here? I am strongly in favor of this moving forward, and a new feature/sypref being developed on a new bug report I am ok to continue discussion elsewhere and it's not the ideal way for features to develop - but it happens, I just want to deal with it in a saner way to avoid having more bugs like this :) Created attachment 150042 [details] [review] Bug 31187: Preserve permanent_location if it is mapped to a field in the editor To test: 1 - Map permanent_location to a marc field 2 - Expose that field in the item editor 3 - Cataloging -> Batch item modification 4 - Enter the barcode for an item 5 - Change the location of the item and retain the permanent location on saving 6 - note that permanent location is set to locatoin 7 - Apply patch 8 - Repeat 9 - Note permanent location is retained Created attachment 150046 [details] [review] Bug 31187: Preserve permanent_location if it is mapped to a field in the editor To test: 1 - Map permanent_location to a marc field 2 - Expose that field in the item editor 3 - Cataloging -> Batch item modification 4 - Enter the barcode for an item 5 - Change the location of the item and retain the permanent location on saving 6 - note that permanent location is set to locatoin 7 - Apply patch 8 - Repeat 9 - Note permanent location is retained Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org> FAIL forbidden patterns: tab char Created attachment 150213 [details] [review] Bug 31187: (follow-up) Fix whitespace Created attachment 150278 [details] [review] Bug 31187: Make sure to not count twice There is another modified++ right after, we don't want to count it twice Added a patch to add a next, it seems safe to have it (no idea if it's really needed). Also adjusting the 'reset' call to make it looks like other similar statements. I am not sure we are doing things correctly, the number of items modified displayed in the report of the batch tool won't be correct as we are considering each items of the batch is modified. Not enough brain power for now to go further with this one... (In reply to Jonathan Druart from comment #14) > Also adjusting the 'reset' call to make it looks like other > similar statements. Well, forget that, that's not "all" other statements, but half of them... Can be dropped. I am not sure we actually need the modified++ you have added, do we? (In reply to Jonathan Druart from comment #16) > I am not sure we actually need the modified++ you have added, do we? You are correct, I think. We are preserviing the old value, not modifying a field. You can remove, or I can provide a patch Please do, squash everything. I think we also need more tests... Created attachment 150296 [details] [review] Bug 31187: Preserve permanent_location if it is mapped to a field in the editor To test: 1 - Map permanent_location to a marc field 2 - Expose that field in the item editor 3 - Cataloging -> Batch item modification 4 - Enter the barcode for an item 5 - Change the location of the item and retain the permanent location on saving 6 - note that permanent location is set to locatoin 7 - Apply patch 8 - Repeat 9 - Note permanent location is retained Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org> Squashed and amended to fix modified count: Bug 31187: (follow-up) Fix whitespace Bug 31187: Make sure to not count twice With this patch applied, I think we now have a regression for the other use case: select kohafield from marc_subfield_structure where tagfield = 952 and kohafield like "%perm%"; = empty, no results I haven't linked the subfield in the frameworks. But: * location and permanent location are both = FIC * Edit location (c) in the item editor: both are now GEN * Batch mod the same item. Set location (c) to FIC. * We now have location = FIC / permanent.locaton = GEN The item editor and the batch mod should behave the same way, shouldn't they? How do you check on the link? I still had k set to the auth_value, but I think that should not matter. It works as expected without the patch applied. Created attachment 150597 [details] [review] Bug 31187: (follow-up) Check if input in form, not if values passed This patch updates the check to see if there was a form input present, not if a value was passed. $attr contains an item colum @form_attr contains the form inputs for the attribute @values (renamed from @v) contains the uniq values passed in to the form We want to know if permanent_location is in the form, not if it had values, to know if we should preserve the DB value To test: 1 - Apply patches 2 - Make sure permanent_location is not mapped to a marc field 3 - Batch edit an item, changing shelving location 4 - Confirm both location and permanent_location are changed Koha-2-MARC-Mappings are now always taken from the default framework (you can no longer set them by framework). So instead of checking the form, could we not use the link in the framework? It seems safer than to check on the form, where the field could just be hidden or altered in some way. My thought was: if permanent_location is linked to a MARC field = treat the subfields individually, else treat them as before. There also needs to be some special treatment for CART and PROC I guess. I wanted to share how our library uses items.permanent_location and items.location to show items in display locations. We have both fields mapped to MARC fields (items.location -> 'c' and items.permanent_location -> 'C'). An example of our workflow is: We want to create a new display of children's book. We batch modify the items.location field for all the items on the new display to show '2ND FLOOR DISPLAY' and the items.permanent_location remains unchanged showing the location of the collection it is usually shelved with. We do not mark/sticker the items that are going on display to save staff time. Little Johnny comes in and sees a book on the display that he wants. He checks it out (Nothing changes in the location fields). Later, Little Johnny returns the book and it is checked in. At this time the items.location field is set to CART (and the items.permanent_location field still remains unchanged). Because we have not marked/stickered the item, public services staff shelve the item in its usual location, not the display location. Some time later the items.location field is updated from CART to whatever was in the items.permanent_location which is still the original location of the item which is where it got shelved. This workflow keeps the items physical shelving location and the shelving location in Koha matching. Currently this workflow works if you use the item editor but when using batch modification, the permanent_location gets updated with the location field. Item editor and batch modification should work the same. |