Bug 18710 - Wrong subfield modified in batch item modification
Summary: Wrong subfield modified in batch item modification
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin Somers
QA Contact: Marcel de Rooy
URL:
Keywords:
: 23715 (view as bug list)
Depends on: 18706 18707
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-31 14:43 UTC by Fridolin Somers
Modified: 2020-06-04 20:34 UTC (History)
9 users (show)

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


Attachments
Bug 18710 - Wrong subfield modified in batch item modification (2.49 KB, patch)
2017-05-31 15:12 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 18710 - batchMod-edit.tt is missing form end tag (809 bytes, patch)
2017-05-31 15:20 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 18710 - Wrong subfield modified in batch item modification (2.55 KB, patch)
2019-09-24 17:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18710 - Wrong subfield modified in batch item modification (2.65 KB, patch)
2019-09-27 06:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2017-05-31 14:43:57 UTC
In Tools > Batch item modification, one can modify or delete a subfield.
When deleting a subfield et modifying another, the wrong subfield gets modified.
Comment 1 Fridolin Somers 2017-05-31 15:04:59 UTC
Needs Bug 18706 and Bug 18707 to be tested.
Comment 2 Fridolin Somers 2017-05-31 15:12:10 UTC
Created attachment 63879 [details] [review]
Bug 18710 - Wrong subfield modified in batch item modification

In Tools > Batch item modification, one can modify or delete a subfield.
When deleting a subfield et modifying another, the wrong subfield gets modified.

This is caused by the fact that disabled inputs are not posted in form.
So for a subfield to delete :
Input name=field_value is not posted. But input name=subfield is posted with subfield code.
So these 2 arrays does not have the same size :
    my @subfields = $input->multi_param('subfield');
    my @values    = $input->multi_param('field_value');

For exemple, deleting $2, not changing $u and modidying $v will modify $u with value for $v o_O

This patch correts but setting disabled all inputs of a deleted subfield : field_value, tag, subfield and mandatory. Like it was not present in the form.

Test plan :
- Go to Tools > Batch item modification
- Enter a barcode and submit
- Lets say there are subfields : $2 (not mandatory), $u and $v
- Check the subfield $2 to be deleted
=> $2 input is disabled
- Enter a text in $v
- Submit the form
=> Without patch, the text for $v gets into $u
=> With patch, the text for $v gets into $v
- Check the subfield $2 is well deleted
Comment 3 Fridolin Somers 2017-05-31 15:14:28 UTC
This bug also appears with no background job Bug 17240.
We use this for nginx + plack
Comment 4 Fridolin Somers 2017-05-31 15:20:48 UTC
Created attachment 63880 [details] [review]
Bug 18710 - batchMod-edit.tt is missing form end tag
Comment 5 Lee Jamison 2017-06-02 18:32:28 UTC
(In reply to Fridolin SOMERS from comment #4)
> Created attachment 63880 [details] [review] [review]
> Bug 18710 - batchMod-edit.tt is missing form end tag

Patch application of attachment 63880 [details] [review] fails under both 'git --apply' and 'git bz apply' at line 274.
Comment 6 Josef Moravec 2017-06-05 11:05:01 UTC
(In reply to Lee Jamison from comment #5)
> (In reply to Fridolin SOMERS from comment #4)
> > Created attachment 63880 [details] [review] [review] [review]
> > Bug 18710 - batchMod-edit.tt is missing form end tag
> 
> Patch application of attachment 63880 [details] [review] [review] fails under both
> 'git --apply' and 'git bz apply' at line 274.

The patches applies cleanly on top of bug 18706 and bug 18707 for me...
Comment 7 Josef Moravec 2017-06-05 11:05:45 UTC
But I am not able to reproduce this issue...
Comment 8 Lee Jamison 2017-06-05 13:40:57 UTC
Yes, I apologize...it was my test box acting funny. The patches do apply. However, prior to applying the patches I also couldn't reproduce the issue either.
Comment 9 Baptiste 2017-06-05 15:31:41 UTC
The issue is quite hard to explain/reproduce,

I'll try to do my best following:

- the issue is: if you check the field which makes appear "This subfield will be deleted", all following fields will be shifted upside when you'll submit the form

BUT

- sometime it doesn't appear if hidden fields are located up to the targeted field, so please check in the source code that no hidden field is present.
- the dataTable at the end of the batch modification suffers from another bug: is isn't refreshed with new modifications, to see them, you have to try a new modification, check in the database or on the items interface.

However if your modification are all done correctly while a checkbox is checked, this is weird, this is quite weird and I invite you to post the whole precise testplan you followed.
Comment 10 Lee Jamison 2017-06-16 16:49:33 UTC
Unfortunately still unable to replicate the problem without a concrete, repeatable test case.
Comment 11 Macon 2017-06-23 03:17:24 UTC
Was able to test during KohaCon 17 with Lauren of SIL and Paul Poulain as the mentor.
Comment 12 Nick Clemens 2017-06-23 11:14:21 UTC Comment hidden (obsolete)
Comment 13 Lee Jamison 2017-06-23 11:37:25 UTC
(In reply to Nick Clemens from comment #12)
> Paul, can you share your test plan?
> 
> I tried 
> 1 - Find a patron with city, state, and zip defined
> 2 - Select patron for batch modification
> 3 - Set city = "NEWCITY" STATE-checked to delete ZIP="NEWZIP"
> 4 - Modification succeeds as expected

I haven't been able to reproduce the bug myself so I'd love to know Paul's test plan as well. Though I believe I'm reading it correctly that the bug occurs in batch item modification instead of batch patron modification :)
Comment 14 Nick Clemens 2017-06-23 13:36:11 UTC
Paul, can you share your test plan?

I tried 
1 - Find an item with copynumber,URI, replacementprice defined (subs t,u,v)
2 - Select item fro modification
3 - Set copynumber = "NEWCOP" URI-checked to delete replacementprice="32"
4 - Modification succeeds as expected
Comment 15 Nick Clemens 2019-09-24 17:36:13 UTC
Created attachment 93145 [details] [review]
Bug 18710 - Wrong subfield modified in batch item modification

In Tools > Batch item modification, one can modify or delete a subfield.
When deleting a subfield et modifying another, the wrong subfield gets modified.

This is caused by the fact that disabled inputs are not posted in form.
So for a subfield to delete :
Input name=field_value is not posted. But input name=subfield is posted with subfield code.
So these 2 arrays does not have the same size :
    my @subfields = $input->multi_param('subfield');
    my @values    = $input->multi_param('field_value');

For exemple, deleting $2, not changing $u and modidying $v will modify $u with value for $v o_O

This patch correts but setting disabled all inputs of a deleted subfield : field_value, tag, subfield and mandatory. Like it was not present in the form.

Test plan :
- Go to Tools > Batch item modification
- Enter a barcode and submit
- Lets say there are subfields : $2 (not mandatory), $u and $v
- Check the subfield $2 to be deleted
=> $2 input is disabled
- Enter a text in $v
- Submit the form
=> Without patch, the text for $v gets into $u
=> With patch, the text for $v gets into $v
- Check the subfield $2 is well deleted

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2019-09-24 17:39:14 UTC
I was able to recreate with settings in batch mod:
Not for loan - checked to delete
Collection code - REF (Reference)
Permanent location - LPL (Liberty)

Before the patch the value for permanent location ended up in ccode field

Disabling the field removes the input and causes off by 1 errors - removing all the fields seems to fix this
Comment 17 Marcel de Rooy 2019-09-27 06:07:26 UTC
Created attachment 93179 [details] [review]
Bug 18710 - Wrong subfield modified in batch item modification

In Tools > Batch item modification, one can modify or delete a subfield.
When deleting a subfield et modifying another, the wrong subfield gets modified.

This is caused by the fact that disabled inputs are not posted in form.
So for a subfield to delete :
Input name=field_value is not posted. But input name=subfield is posted with subfield code.
So these 2 arrays does not have the same size :
    my @subfields = $input->multi_param('subfield');
    my @values    = $input->multi_param('field_value');

For exemple, deleting $2, not changing $u and modidying $v will modify $u with value for $v o_O

This patch correts but setting disabled all inputs of a deleted subfield : field_value, tag, subfield and mandatory. Like it was not present in the form.

Test plan :
- Go to Tools > Batch item modification
- Enter a barcode and submit
- Lets say there are subfields : $2 (not mandatory), $u and $v
- Check the subfield $2 to be deleted
=> $2 input is disabled
- Enter a text in $v
- Submit the form
=> Without patch, the text for $v gets into $u
=> With patch, the text for $v gets into $v
- Check the subfield $2 is well deleted

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Martin Renvoize 2019-09-27 12:59:23 UTC
Nice work!

Pushed to master for 19.11.00
Comment 19 Katrin Fischer 2019-10-02 06:33:58 UTC
*** Bug 23715 has been marked as a duplicate of this bug. ***
Comment 20 Christopher Brannon 2019-10-02 15:28:43 UTC
Please push this to stable and old stable.  I consider this a critical bug that can do damage in mass.
Comment 21 Fridolin Somers 2019-10-08 06:56:33 UTC
Pushed to 19.05.x for 19.05.05
Comment 22 Christopher Brannon 2019-10-08 14:26:42 UTC
(In reply to Fridolin SOMERS from comment #21)
> Pushed to 19.05.x for 19.05.05

+1
Comment 23 Lucas Gass 2019-10-17 19:55:57 UTC
backported to 18.11.x for 18.11.11