Bug 28832 - Batch modification always clears permanent_location if it is mapped in frameworks
Summary: Batch modification always clears permanent_location if it is mapped in framew...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: 21.05
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 27837
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-09 16:02 UTC by Nick Clemens
Modified: 2021-11-24 16:06 UTC (History)
6 users (show)

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


Attachments
Bug 28832: Don't batch modify fields that we don't update (2.32 KB, patch)
2021-08-09 16:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28832: Don't batch modify fields that we don't update (2.39 KB, patch)
2021-08-18 16:32 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28832: Don't batch modify fields that we don't update (2.51 KB, patch)
2021-08-19 20:40 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28832: (follow-up) Handle regex fields too (1015 bytes, patch)
2021-09-20 17:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28832: Don't batch modify fields that we don't update (2.58 KB, patch)
2021-09-20 17:33 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28832: (follow-up) Handle regex fields too (1.06 KB, patch)
2021-09-20 17:33 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28832: (follow-up) Don't remove false values, only empty strings (1.12 KB, patch)
2021-11-24 16:05 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 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