Bug 30909 - Regression, Permanent shelving location is always updated when editing location VIA ADDITEM.PL if both are mapped to MARC fields
Summary: Regression, Permanent shelving location is always updated when editing locati...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Joonas Kylmälä
URL:
Keywords: regression
Depends on: 27526 27837
Blocks: 31187
  Show dependency treegraph
 
Reported: 2022-06-06 13:31 UTC by Andrew Fuerste-Henry
Modified: 2023-06-08 22:26 UTC (History)
7 users (show)

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


Attachments
Bug 30909: Retain permanent_location if mapped (1.83 KB, patch)
2022-06-07 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30909: Retain permanent_location if mapped (1.88 KB, patch)
2022-07-19 14:25 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 30909: Retain permanent_location if mapped (2.02 KB, patch)
2022-07-19 18:21 UTC, Joonas Kylmälä
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-06-06 13:31:58 UTC
This issue was corrected in bug 27837, but bug 27526 led to a regression of that fix. 

To recreate:
1 - Map permanent_location to a marc field
2 - Expose that field in the item editor
3 - Change the location of the item and retain the permanent location on saving
4 - note that permanent location has changed anyway
Comment 1 Jonathan Druart 2022-06-07 09:29:24 UTC
Created attachment 135748 [details] [review]
Bug 30909: Retain permanent_location if mapped

If items.permanent_location is mapped to a MARC subfields we must retain
the value, not using items.location

Test plan:
1 - Map permanent_location to a marc field
2 - Expose that field in the item editor
3 - Change the location of the item and retain the permanent location on saving
4 - note that permanent location has not changed

Test the different other situation, when adding and editing: Empty the field, change only location, change only permanent_location, etc.
Comment 2 Jonathan Druart 2022-06-07 09:30:33 UTC
Hum, maybe the batch mod tool is affected as well.
Comment 3 Andrew Fuerste-Henry 2022-06-07 13:48:33 UTC
(In reply to Jonathan Druart from comment #2)
> Hum, maybe the batch mod tool is affected as well.

I can confirm this issue also exists in the batch mod tool. Thanks!
Comment 4 David Nind 2022-06-18 22:13:59 UTC
I would like to test this, but I'm not familiar enough with locations and MARC to do step 1 and 2 in the bug test plan.

Could you advise on how to do steps 1(c) and 2(c) in my revised test plan (based on bug 27837)? Apologies for such a basic question!

Also, does this bug address editing using batch item modification, or is an updated patch required?

Thanks!

Revised test plan: 
1. Map permanent_location to a marc field:
   a. go to Administration > Catalog > Koha to MARC mapping
   b. locate items.permanent_location then select Add
   c. QUESTION - which field code and subfield code do I map it to?
2. Expose that field in the item editor:
   a. go to Administration > Catalog > MARC bibliographic framework
   b. select the MARC framework for BKS
   c. QUESTION - which MARC field do I use for permanent_location to get this to show in the item editor?
3. Edit an item, changing location to X and permanent location to Y, save.
4. Confirm record shows location=X, permanent_location=Y.
5. Edit item again, change location to Z, do not change permanent location, save.
6. Confirm location and permanent_location both equal Z.
7. Run through steps 3-6 using batch item modification for a record with multiple items (record details page > Select all the items under the holdings tab > Modify selected items).
8. Apply patch, flush_memcached, restart_all.
9. Repeat steps 3-7.
10. Confirm location=Z, permanent location=Y.
11. Run the tests: prove t/db_dependent/Items.t
Comment 5 Katrin Fischer 2022-06-18 22:18:38 UTC
I believe you need to map to a free 952. An option could be a new subfield L you create and make visible in the BKS framework. It needs to be on tab 10 (items).
Comment 6 David Nind 2022-06-18 22:59:07 UTC
(In reply to Katrin Fischer from comment #5)
> I believe you need to map to a free 952. An option could be a new subfield L
> you create and make visible in the BKS framework. It needs to be on tab 10
> (items).

Thanks Katrin! I will that.
Comment 7 David Nind 2022-06-19 07:10:30 UTC
I couldn't get steps 1 and 2 of the bug test plan to work correctly - will leave for someone who knows what they should be doing here. I'm sure it is something relatively simple that I'm not getting!

For step 2c below, after I press OK the page reloads but there is nothing shown for permanent_location. When I test the frameworks, I get this error:
items.permanent_location mapped: The following mappings exist for items.permanent_location, and they shouldn't.
Framework code 	Framework description 	Tag 	Subfield
BKS 	Books, Booklets, Workbooks 	952 	L

This is what I did/intended to do: 
1. Create and expose a new 952 subfield in the item editor:
   a. Administration > Catalog > MARC bibliographic framework
   b. Select the BKS MARC framework
   c. Add a new subfield for 952 - L, use the same settings as 952$c, except name as Permanent shelving location
2. Map permanent_location to a marc field:
   a. Administration > Catalog > Koha to MARC mapping
   b. Locate items.permanent_location then select Add
   c. Map to a new 952 field, for example 952,L
   d. Rebuild the marc mappings: remove the duplicate 010 for record 72, then misc/batchRebuildBiblioTables.pl -c will run without errors
3. Edit an item, changing location to X and permanent location to Y, save (for example: location = New materials shelf, Permanent shelving location = General stacks).
4. Confirm record shows location=X, permanent_location=Y (for example: location = New materials shelf, Permanent shelving location = General stacks).
5. Edit item again, change location to Z, do not change permanent location, save (for example: Children's area).
6. Run through steps 3-6 using batch item modification for a record with multiple items (record details page > Select all the items under the holdings tab > Modify selected items).
   ==> Not: new 952 subfield is not shown, so you can't use the batch item modification
8. Apply patch, flush_memcached, restart_all.
9. Repeat steps 3-6.
10. Confirm location=Z, permanent location=Y.
11. Run the tests: prove t/db_dependent/Items.t

Notes:
- When you add a mapping: the instructions say:
"Adding a mapping for: items.permanent_location.
Please enter field tag and subfield code, separated by a comma. (For control fields: add '@' as subfield code.)
The change will be applied immediately."
- However, at the top of the page it says: "If you make any change to the mappings, you must ask your administrator to run misc/batchRebuildBiblioTables.pl."
- Not sure which is right here and hwteher this shouldbe logged as a new bug.
Comment 8 Katrin Fischer 2022-07-18 21:09:57 UTC
I have tried to test this, but I can't replicate the problem, could this have been fixed otherwise?

To recreate:
1 - Map permanent_location to a marc field
2 - Expose that field in the item editor

I used 952k, tab 10, visibile in editor, AV = LOC.
Updated Koha-to-MARC-mappings to map to permanent_location.

3 - Change the location of the item and retain the permanent location on saving

My item had no locations set. I set permanent location and locatoon to different values and saved.

4 - note that permanent location has changed anyway

The fields kept the values I set, nothing was changed unexectedly.

Maybe I am missing something?
Comment 9 Jonathan Druart 2022-07-19 13:15:00 UTC
(In reply to Katrin Fischer from comment #8)
> I have tried to test this, but I can't replicate the problem, could this
> have been fixed otherwise?
> 
> To recreate:
> 1 - Map permanent_location to a marc field
> 2 - Expose that field in the item editor
> 
> I used 952k, tab 10, visibile in editor, AV = LOC.
> Updated Koha-to-MARC-mappings to map to permanent_location.
> 
> 3 - Change the location of the item and retain the permanent location on
> saving
> 
> My item had no locations set. I set permanent location and locatoon to
> different values and saved.
> 
> 4 - note that permanent location has changed anyway
> 
> The fields kept the values I set, nothing was changed unexectedly.
> 
> Maybe I am missing something?

Create an item, set location=Audio Visual (AV), save
=> permanent location is AV
Edit again, set location=Child's area (CHILD), don't modify permanent location, save
=> permanent location is CHILD
Comment 10 Andrew Fuerste-Henry 2022-07-19 14:15:10 UTC
Confirming current incorrect behavior: 

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 11 Andrew Fuerste-Henry 2022-07-19 14:17:34 UTC
> Notes:
> - When you add a mapping: the instructions say:
> "Adding a mapping for: items.permanent_location.
> Please enter field tag and subfield code, separated by a comma. (For control
> fields: add '@' as subfield code.)
> The change will be applied immediately."
> - However, at the top of the page it says: "If you make any change to the
> mappings, you must ask your administrator to run
> misc/batchRebuildBiblioTables.pl."
> - Not sure which is right here and hwteher this shouldbe logged as a new bug.

Agreed on all of this. The format of "952,k" is confusingly nonstandard, I'd love to see it normalized to "952$k."

I believe those two notes about immediate changes and rebuilds are meant to convey that any mapping change will be applied to new and edited records immediately but that existing records will need a rebuild to reflect the change. This could certainly be explained more clearly.
Comment 12 ByWater Sandboxes 2022-07-19 14:25:17 UTC
Created attachment 137873 [details] [review]
Bug 30909: Retain permanent_location if mapped

If items.permanent_location is mapped to a MARC subfields we must retain
the value, not using items.location

Test plan:
1 - Map permanent_location to a marc field
2 - Expose that field in the item editor
3 - Change the location of the item and retain the permanent location on saving
4 - note that permanent location has not changed

Test the different other situation, when adding and editing: Empty the field, change only location, change only permanent_location, etc.

Signed-off-by: andrew <andrewfh@dubcolib.org>
Comment 13 Joonas Kylmälä 2022-07-19 18:21:18 UTC
Created attachment 137884 [details] [review]
Bug 30909: Retain permanent_location if mapped

If items.permanent_location is mapped to a MARC subfields we must retain
the value, not using items.location

Test plan:
1 - Map permanent_location to a marc field
2 - Expose that field in the item editor
3 - Change the location of the item and retain the permanent location on saving
4 - note that permanent location has not changed

Test the different other situation, when adding and editing: Empty the field, change only location, change only permanent_location, etc.

Signed-off-by: andrew <andrewfh@dubcolib.org>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 14 Joonas Kylmälä 2022-07-19 18:24:26 UTC
Fixes the issue as advertised, passing QA.

On an unrelated note whilst testing this bug I noticed that we seem to have a cache invalidation problem with Koha to MARC mapping. After removing the 952$k mapping to items.permanent_location I had to restart all koha services (i guess memcached would have been plenty) for the functionality to return to original, that is, when location is updated also permanent_location is updated.
Comment 15 Jonathan Druart 2022-07-20 07:22:28 UTC
(In reply to Joonas Kylmälä from comment #14)
> Fixes the issue as advertised, passing QA.
> 
> On an unrelated note whilst testing this bug I noticed that we seem to have
> a cache invalidation problem with Koha to MARC mapping. After removing the
> 952$k mapping to items.permanent_location I had to restart all koha services
> (i guess memcached would have been plenty) for the functionality to return
> to original, that is, when location is updated also permanent_location is
> updated.

I guess it's because we are flushing the marc structure for the default framework only in admin/koha2marclinks.pl.
Comment 16 Jonathan Druart 2022-07-20 07:36:21 UTC
Opened bug 31194.
Comment 17 Tomás Cohen Arazi 2022-07-20 12:09:55 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 18 Lucas Gass 2022-08-23 19:49:56 UTC
Missing dependencies for 22.05.x
Comment 19 Lucas Gass 2022-09-16 15:19:32 UTC
I'm wrong this is NOT missing 22.05 dependencies. 

Backported to 22.05.x for 22.05.05
Comment 20 Arthur Suzuki 2022-09-21 15:09:19 UTC
Thanks!

Pushed to 21.11 for 21.11.12
Comment 21 Victor Grousset/tuxayo 2022-10-13 14:54:03 UTC
Missing dependencies for 21.05.x, it shouldn't be affected, no backport.
Nothing to document it seems, marking resolved.