Bug 31187 - Regression, Permanent shelving location is always updated when editing location VIA BATCH ITEM MOD if both are mapped to MARC fields
Summary: Regression, Permanent shelving location is always updated when editing locati...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords: regression
Depends on: 30909
Blocks:
  Show dependency treegraph
 
Reported: 2022-07-19 14:27 UTC by Andrew Fuerste-Henry
Modified: 2023-06-07 15:55 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 31187: Preserve permanent_location if it is mapped to a field in the editor (3.85 KB, patch)
2023-02-13 16:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31187: Preserve permanent_location if it is mapped to a field in the editor (3.84 KB, patch)
2023-04-21 14:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31187: Preserve permanent_location if it is mapped to a field in the editor (3.90 KB, patch)
2023-04-21 14:37 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 31187: (follow-up) Fix whitespace (3.97 KB, patch)
2023-04-25 15:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31187: Make sure to not count twice (1.75 KB, patch)
2023-04-27 05:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31187: Preserve permanent_location if it is mapped to a field in the editor (4.33 KB, patch)
2023-04-27 13:15 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31187: (follow-up) Check if input in form, not if values passed (3.23 KB, patch)
2023-05-03 15:10 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2022-07-19 14:27:58 UTC
Bug 30909 handled this bug in the single item editor. It also exists in batch item mod.

Same test plan applies, but use item batch modification for all of the edits:

1: create new subfield in DEFAULT framework, 952$k, visible in editor, managed in tab 10, tied to LOC auth value
2: In Koha to MARC mapping, find items.permanent_location and add a mapping to 952$k
3: Find a bib and item in the default framework
4: Edit the item, see that both the 952$c and 952$k are visible in the editor -- In my test, both fields started at a value of "General Stacks"
5: change both values and save the item, confirm both values change and save successfully -- I set 952$c to Audio Visual and 952$k to Fiction
6: edit again, change only 952$c, save -- I changed 952$c to On Display and left 952$k set to Fiction

THIS IS WHERE THINGS GO WRONG. After saving that change, both 952$c AND 952$k are set to On Display, even though I only changed the 952$c.
Comment 1 Nick Clemens 2023-02-13 16:41:36 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
Comment 2 Katrin Fischer 2023-02-20 22:33:29 UTC
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?
Comment 3 Andrew Fuerste-Henry 2023-03-22 17:52:43 UTC
See bug 33301 for discussion the use case for this and a more formal way to configure temporary shelving locations.
Comment 4 Nick Clemens 2023-04-20 11:53:21 UTC
(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
Comment 5 Katrin Fischer 2023-04-20 19:05:36 UTC
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.
Comment 6 Andrew Fuerste-Henry 2023-04-20 19:14:09 UTC
(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.
Comment 7 Nick Clemens 2023-04-20 19:51:30 UTC
(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
Comment 8 Katrin Fischer 2023-04-20 20:02:14 UTC
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 :)
Comment 9 Nick Clemens 2023-04-21 14:23:12 UTC
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
Comment 10 Andrew Fuerste-Henry 2023-04-21 14:37:42 UTC
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>
Comment 11 Jonathan Druart 2023-04-25 14:08:30 UTC
FAIL   forbidden patterns: tab char
Comment 12 Nick Clemens 2023-04-25 15:30:10 UTC
Created attachment 150213 [details] [review]
Bug 31187: (follow-up) Fix whitespace
Comment 13 Jonathan Druart 2023-04-27 05:22:20 UTC
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
Comment 14 Jonathan Druart 2023-04-27 05:27:00 UTC
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...
Comment 15 Jonathan Druart 2023-04-27 05:28:02 UTC
(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.
Comment 16 Jonathan Druart 2023-04-27 05:42:32 UTC
I am not sure we actually need the modified++ you have added, do we?
Comment 17 Nick Clemens 2023-04-27 12:13:59 UTC
(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
Comment 18 Jonathan Druart 2023-04-27 12:47:25 UTC
Please do, squash everything. I think we also need more tests...
Comment 19 Nick Clemens 2023-04-27 13:15:38 UTC
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
Comment 20 Katrin Fischer 2023-05-01 21:51:07 UTC
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.
Comment 21 Nick Clemens 2023-05-03 15:10:39 UTC
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
Comment 22 Katrin Fischer 2023-05-03 20:26:06 UTC
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.
Comment 23 Katrin Fischer 2023-05-03 20:28:12 UTC
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.
Comment 24 Rhonda Kuiper 2023-06-07 15:55:50 UTC
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.