Bug 28832

Summary: [21.05] Batch modification always clears permanent_location if it is mapped in frameworks
Product: Koha Reporter: Nick Clemens <nick>
Component: ToolsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: major    
Priority: P5 - low CC: andrewfh, dstephen, jonathan.druart, joonas.kylmala, martin.renvoize, nick, rkuiper, victor, wainuiwitikapark
Version: 21.05   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27526
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.11,20.11.15
Bug Depends on: 27837    
Bug Blocks:    
Attachments: Bug 28832: Don't batch modify fields that we don't update
Bug 28832: Don't batch modify fields that we don't update
Bug 28832: Don't batch modify fields that we don't update
Bug 28832: (follow-up) Handle regex fields too
Bug 28832: Don't batch modify fields that we don't update
Bug 28832: (follow-up) Handle regex fields too
Bug 28832: (follow-up) Don't remove false values, only empty strings
Bug 28832: Don't batch modify fields that we don't update
Bug 28832: (follow-up) Handle regex fields too
Bug 28832: (follow-up) Don't remove false values, only empty strings
Bug 28832: (QA follow-up) Tiny comment spelling fix

Description Nick Clemens 2021-08-09 16:02:25 UTC
On bug 27837 we ensured that the value for permanent_location was always passed through to the edit

Unfortunately - when performing a batch mod the field is not set if we don't want to change it or clear it

Forcing the pass through then means we clear the field on every update

To recreate:
1 - In frameworks add a new subfield to 952, "C" - make it editable in items tab and visible in intranet/editor
2 - In mappings map that subfield to items.permanent_location
3 - Set an item to have differing shelving location and permanent_location
4 - This saves correctly for an individual item
5 - Edit the item using Tools->batch item modification, updating the note field
6 - Note the permanet_location is removed
7 - Check the DB, the field is set to NULL
Comment 1 Nick Clemens 2021-08-09 16:32:03 UTC
Created attachment 123654 [details] [review]
Bug 28832: Don't batch modify fields that we don't update

In bug 27837 we made sure to always pass through the permanent_location if it was passed in unlike
other fields which are not passed if they have no value.
During batch mod, however, fields that aren't editied have no value, so forcing permanent_location
to pass through forces blanking it.

This patch alters the script to only pass thgouhr for edit the fields that have been updated or
cleared.

To test:
 1 - In frameworks add a new subfield to 952, "C" - make it editable in items tab and visible in intranet/editor
 2 - In mappings map that subfield to items.permanent_location
 3 - Set an item to have differing shelving location and permanent_location
 4 - This saves correctly for an individual item
 5 - Edit the item using Tools->batch item modification, updating the note field
 6 - Note the permanet_location is removed
 7 - Check the DB, the field is set to NULL
 8 - Apply patch
 9 - Repeat
10 - Permanent location is not cleared
11 - Verify that clearing fields works as before, including permanent_location
Comment 2 Andrew Fuerste-Henry 2021-08-18 16:32:10 UTC
Created attachment 123966 [details] [review]
Bug 28832: Don't batch modify fields that we don't update

In bug 27837 we made sure to always pass through the permanent_location if it was passed in unlike
other fields which are not passed if they have no value.
During batch mod, however, fields that aren't editied have no value, so forcing permanent_location
to pass through forces blanking it.

This patch alters the script to only pass thgouhr for edit the fields that have been updated or
cleared.

To test:
 1 - In frameworks add a new subfield to 952, "C" - make it editable in items tab and visible in intranet/editor
 2 - In mappings map that subfield to items.permanent_location
 3 - Set an item to have differing shelving location and permanent_location
 4 - This saves correctly for an individual item
 5 - Edit the item using Tools->batch item modification, updating the note field
 6 - Note the permanet_location is removed
 7 - Check the DB, the field is set to NULL
 8 - Apply patch
 9 - Repeat
10 - Permanent location is not cleared
11 - Verify that clearing fields works as before, including permanent_location

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 3 Andrew Fuerste-Henry 2021-08-19 20:40:15 UTC
Created attachment 123982 [details] [review]
Bug 28832: Don't batch modify fields that we don't update

In bug 27837 we made sure to always pass through the permanent_location if it was passed in unlike
other fields which are not passed if they have no value.
During batch mod, however, fields that aren't editied have no value, so forcing permanent_location
to pass through forces blanking it.

This patch alters the script to only pass thgouhr for edit the fields that have been updated or
cleared.

To test:
 1 - In frameworks add a new subfield to 952, "C" - make it editable in items tab and visible in intranet/editor
 2 - In mappings map that subfield to items.permanent_location
 3 - Set an item to have differing shelving location and permanent_location
 4 - This saves correctly for an individual item
 5 - Edit the item using Tools->batch item modification, updating the note field
 6 - Note the permanet_location is removed
 7 - Check the DB, the field is set to NULL
 8 - Apply patch
 9 - Repeat
10 - Permanent location is not cleared
11 - Verify that clearing fields works as before, including permanent_location

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Deb Stephenson <dstephen@dubuque.lib.ia.us>
Comment 4 Joonas Kylmälä 2021-09-01 13:15:04 UTC
All the repeated fields need to be spliced off for this to work. Now for example this breaks the regex replace functionality. If I try to change the copynumber with:

 t - Copy number s/.*/ TTT /

using the batchmod tool it doesn't work after this patch. I think the indicator field modification might be broken now as well (didn't check but looking at the batchMod.pl code it seems so)
Comment 5 Nick Clemens 2021-09-20 17:23:11 UTC
Created attachment 125075 [details] [review]
Bug 28832: (follow-up) Handle regex fields too
Comment 6 Andrew Fuerste-Henry 2021-09-20 17:33:15 UTC
Created attachment 125076 [details] [review]
Bug 28832: Don't batch modify fields that we don't update

In bug 27837 we made sure to always pass through the permanent_location if it was passed in unlike
other fields which are not passed if they have no value.
During batch mod, however, fields that aren't editied have no value, so forcing permanent_location
to pass through forces blanking it.

This patch alters the script to only pass thgouhr for edit the fields that have been updated or
cleared.

To test:
 1 - In frameworks add a new subfield to 952, "C" - make it editable in items tab and visible in intranet/editor
 2 - In mappings map that subfield to items.permanent_location
 3 - Set an item to have differing shelving location and permanent_location
 4 - This saves correctly for an individual item
 5 - Edit the item using Tools->batch item modification, updating the note field
 6 - Note the permanet_location is removed
 7 - Check the DB, the field is set to NULL
 8 - Apply patch
 9 - Repeat
10 - Permanent location is not cleared
11 - Verify that clearing fields works as before, including permanent_location

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Deb Stephenson <dstephen@dubuque.lib.ia.us>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 7 Andrew Fuerste-Henry 2021-09-20 17:33:19 UTC
Created attachment 125077 [details] [review]
Bug 28832: (follow-up) Handle regex fields too

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 8 Andrew Fuerste-Henry 2021-09-20 17:34:26 UTC
Repeated the original test plan and then did a second batch edit, successfully editing a field via regex, confirmed that edit worked and did not mess up the shelving locations.
Comment 9 Joonas Kylmälä 2021-09-26 07:04:16 UTC
The indicator code was not handled here but on a closer look it seems unused, and for some reason the same indicator code is copied all over Koha codebase unused as well. I will open a separate ticket to remove the indicator related code with items.
Comment 10 Joonas Kylmälä 2021-09-26 07:18:21 UTC
After applying the patches modifying the item fields to values that evaluate in perl to false is impossible. The conditions in 

> unless( $values[$i] || $searches[$i] || grep { $subfields[$i] } @disabled ){

should be the same as just above it, i.e. if the value is empty string or not.

To reproduce:
 1) Try to change inventory field to 0 and notice it is not modified.
Comment 11 Jonathan Druart 2021-11-19 11:03:09 UTC
After bug 28445 I don't recreate the problem. Can you confirm, Nick?
Comment 12 Nick Clemens 2021-11-24 15:01:32 UTC
(In reply to Jonathan Druart from comment #11)
> After bug 28445 I don't recreate the problem. Can you confirm, Nick?

Yes, however, it still affects 21.05
Comment 13 Nick Clemens 2021-11-24 16:05:53 UTC
Created attachment 127982 [details] [review]
Bug 28832: (follow-up) Don't remove false values, only empty strings
Comment 14 Nick Clemens 2021-11-24 16:06:33 UTC
(In reply to Joonas Kylmälä from comment #10)
> After applying the patches modifying the item fields to values that evaluate
> in perl to false is impossible. The conditions in 
> 
> > unless( $values[$i] || $searches[$i] || grep { $subfields[$i] } @disabled ){
> 
> should be the same as just above it, i.e. if the value is empty string or
> not.

Updated
 

> To reproduce:
>  1) Try to change inventory field to 0 and notice it is not modified.

I actually find this is true from the item editor as well - it is not caused by this patchset
Comment 15 Katrin Fischer 2021-12-19 11:10:04 UTC
I am sorry, this no longer applies - can you please rebase?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 28832: Don't batch modify fields that we don't update
Using index info to reconstruct a base tree...
M	tools/batchMod.pl
Falling back to patching base and 3-way merge...
Auto-merging tools/batchMod.pl
CONFLICT (content): Merge conflict in tools/batchMod.pl
error: Failed to merge in the changes.
Patch failed at 0001 Bug 28832: Don't batch modify fields that we don't update
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-28832-Dont-batch-modify-fields-that-we-dont-up-dHzj6t.patch
Comment 16 Nick Clemens 2021-12-22 11:08:42 UTC
(In reply to Katrin Fischer from comment #15)
> I am sorry, this no longer applies - can you please rebase?
> 
Applies cleanly to 21.05.x for me
Comment 17 Katrin Fischer 2022-01-01 13:08:45 UTC
I missed that this was for an older version only - I have added to the description to make it more visible.
Comment 18 Rhonda Kuiper 2022-01-28 16:00:40 UTC
We are eagerly waiting for this bug.  Any status update?
Comment 19 Martin Renvoize 2022-01-31 10:19:57 UTC
Created attachment 129991 [details] [review]
Bug 28832: Don't batch modify fields that we don't update

In bug 27837 we made sure to always pass through the permanent_location if it was passed in unlike
other fields which are not passed if they have no value.
During batch mod, however, fields that aren't editied have no value, so forcing permanent_location
to pass through forces blanking it.

This patch alters the script to only pass thgouhr for edit the fields that have been updated or
cleared.

To test:
 1 - In frameworks add a new subfield to 952, "C" - make it editable in items tab and visible in intranet/editor
 2 - In mappings map that subfield to items.permanent_location
 3 - Set an item to have differing shelving location and permanent_location
 4 - This saves correctly for an individual item
 5 - Edit the item using Tools->batch item modification, updating the note field
 6 - Note the permanet_location is removed
 7 - Check the DB, the field is set to NULL
 8 - Apply patch
 9 - Repeat
10 - Permanent location is not cleared
11 - Verify that clearing fields works as before, including permanent_location

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Deb Stephenson <dstephen@dubuque.lib.ia.us>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2022-01-31 10:20:01 UTC
Created attachment 129992 [details] [review]
Bug 28832: (follow-up) Handle regex fields too

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2022-01-31 10:20:07 UTC
Created attachment 129993 [details] [review]
Bug 28832: (follow-up) Don't remove false values, only empty strings

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2022-01-31 10:21:42 UTC
Created attachment 129994 [details] [review]
Bug 28832: (QA follow-up) Tiny comment spelling fix

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2022-01-31 10:22:33 UTC
This works as expected for me at 21.05.  QA scripts are also happy now I've fixed a minor typo spelling error.

I'm happy this works now, Passing QA
Comment 24 Andrew Fuerste-Henry 2022-02-16 16:21:01 UTC
Pushed to 21.05.x for 21.05.11
Comment 25 Victor Grousset/tuxayo 2022-02-20 22:17:36 UTC
20.11.x also affected.

Backported: Pushed to 20.11.x branch for 20.11.15
Comment 26 wainuiwitikapark 2022-02-22 00:59:17 UTC
Not backported to 19.11.x