Bug 15030 - Certain values in serials' items are lost on next edit
Summary: Certain values in serials' items are lost on next edit
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Blou
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-19 16:15 UTC by Blou
Modified: 2019-06-27 09:24 UTC (History)
18 users (show)

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


Attachments
Bug 15030 - Certain values in serials' items are lost on next edit (2.80 KB, patch)
2015-10-19 16:17 UTC, Blou
Details | Diff | Splinter Review
Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value (2.70 KB, patch)
2015-10-21 18:39 UTC, Blou
Details | Diff | Splinter Review
Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied (1.48 KB, patch)
2015-10-22 20:49 UTC, Blou
Details | Diff | Splinter Review
Bug 15030: Add tests (3.60 KB, patch)
2015-11-12 12:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15030: Add tests (3.60 KB, patch)
2015-12-01 22:26 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 15030: Add tests (3.70 KB, patch)
2015-12-17 10:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15030 - Certain values in serials' items are lost on next edit (2.86 KB, patch)
2016-04-05 12:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value (2.76 KB, patch)
2016-04-05 12:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied (1.53 KB, patch)
2016-04-05 12:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15030: Add tests (3.75 KB, patch)
2016-04-05 12:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Fix the uninitialized warnings in ClassSortRoutine::_get_class_sort_key (1.17 KB, patch)
2016-07-15 20:57 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 15030: Fix the uninitialized warnings in ClassSortRoutine::_get_class_sort_key (1.18 KB, patch)
2016-07-15 20:58 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 15030 - Certain values in serials' items are lost on next edit (2.86 KB, patch)
2016-07-16 16:10 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value (2.76 KB, patch)
2016-07-16 16:10 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied (1.53 KB, patch)
2016-07-16 16:11 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15030: Add tests (3.71 KB, patch)
2016-07-16 16:11 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15030 - Certain values in serials' items are lost on next edit (2.86 KB, patch)
2017-01-31 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value (2.76 KB, patch)
2017-01-31 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied (1.53 KB, patch)
2017-01-31 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15030: Add tests (3.76 KB, patch)
2017-01-31 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15030 - Certain values in serials' items are lost on next edit (2.93 KB, patch)
2017-02-01 09:08 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value (2.83 KB, patch)
2017-02-01 09:08 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied (1.59 KB, patch)
2017-02-01 09:09 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15030: Add tests (3.83 KB, patch)
2017-02-01 09:09 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15030 - Certain values in serials' items are lost on next edit (2.99 KB, patch)
2017-02-02 11:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value (2.89 KB, patch)
2017-02-02 11:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied (1.65 KB, patch)
2017-02-02 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15030: Add tests (3.89 KB, patch)
2017-02-02 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2015-10-19 16:15:29 UTC
When editing serials subscription, we can edit them but some values are not pulled from the DB correctly to be put in the edit box.  If not noticed, the value will be overwritten on the next save.

Test:
- Create a subscription
- Edit itemcallnumber (952o?) and make sure to have a different value than the default one.
- Save.
- Edit it again
- The saved value is not there.

This is true for itemcallnumber and a few other fields.
Comment 1 Blou 2015-10-19 16:17:50 UTC Comment hidden (obsolete)
Comment 2 Blou 2015-10-21 18:39:22 UTC Comment hidden (obsolete)
Comment 3 Blou 2015-10-22 20:49:38 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2015-10-28 13:48:14 UTC
Comment on attachment 43601 [details] [review]
Bug 15030 - Certain values in serials' items are lost on next edit

Review of attachment 43601 [details] [review]:
-----------------------------------------------------------------

::: C4/Items.pm
@@ +2944,4 @@
>                  if (   $tagslib->{$tag}->{$subfield}->{kohafield} eq 'items.itemcallnumber'
>                      && $defaultvalues
>                      && $defaultvalues->{'callnumber'} ) {
> +                    if( $itemrecord and $defaultvalues and not $itemrecord->subfield($tag,$subfield) ){

YIKES! Nice find.
Comment 5 Mark Tompsett 2015-10-28 13:57:00 UTC
Okay... this modifies a routine in C4/Items.pm
There is no test coverage directly for PrepareItemrecordDisplay.
However, there is some in t/db_dependent/Serials.t
I would recommend adding a test there to validate your changes as a separate commit. This would allow the test to run before applying the other patches (expecting failure), and after applying them (expecting passing).
Comment 6 Jonathan Druart 2015-11-12 12:31:07 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-11-12 12:39:48 UTC
This is not "blocker".
Comment 8 Lucio Moraes 2015-11-16 20:45:59 UTC
Hi,
could you be more specific with test plan?
Thank you
Comment 9 Josef Moravec 2015-11-20 07:41:06 UTC
There is conflict in t/db_dependent/Serials.t, could you please rebase?
Comment 10 Charles Farmer 2015-12-01 22:26:11 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2015-12-16 14:22:08 UTC
Comment on attachment 45297 [details] [review]
Bug 15030: Add tests

Review of attachment 45297 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Serials.t
@@ +22,5 @@
>      use_ok('C4::Serials');
>  }
>  
>  my $dbh = C4::Context->dbh;
> +$dbh->do(q|UPDATE marc_subfield_structure SET value_builder="callnumber.pl" where kohafield="items.itemcallnumber" and frameworkcode=''|);

This is BEFORE the AutoCommit=0?!
Comment 12 Jonathan Druart 2015-12-17 10:46:08 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2015-12-17 10:46:36 UTC
(In reply to M. Tompsett from comment #11)
> >  my $dbh = C4::Context->dbh;
> > +$dbh->do(q|UPDATE marc_subfield_structure SET value_builder="callnumber.pl" where kohafield="items.itemcallnumber" and frameworkcode=''|);
> 
> This is BEFORE the AutoCommit=0?!

Good catch, fixed!
Comment 14 Bernardo Gonzalez Kriegel 2015-12-17 18:52:00 UTC
Sorry but can't reproduce, or don't know what to test.
My test:

Serials > New Subscription
On subscription details filled some data in 

Manual history
Call number
Library: (select a library) 
Public note:
Nonpublic note:
Patron notification: 
Location:
Grace period:

Is that the data not loaded again?
If yes, it works for me.
If no, please provide a good test plan.
Comment 15 Liz Rea 2016-01-07 22:49:30 UTC
Hi, 

May we please have a comprehensive test plan, it seems there are cases where this bug doesn't manifest and can't be reproduced.

Cheers,
Liz
Comment 16 Hugo Agud 2016-04-05 08:16:12 UTC
I have been able to reproduce the error on koha 3.16.7 (I have not checked in more updated version of koha)

We have verified that If you edit a serial that has already been received, koha delete the existing barcode.

The test plan we suggest.

1. Create one subscription with any serial pattern and create item when receiving an issue.
2. Receive one or two serial, fill the values of the items, in our test we have filled enumchron (automatically loaded from patter), signature (automatically loaded from subscription information) and barcode (manual).
3. Check that the item has been propertly added to biblionumber
4. Go to subscription--> serial collection --> Edit Serail --> select one of the received --> modify field 'Publish on'
5. Now the barcode has been deleted

Hope it helps
Comment 17 Nick Clemens 2016-04-05 12:44:57 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2016-04-05 12:45:03 UTC Comment hidden (obsolete)
Comment 19 Nick Clemens 2016-04-05 12:45:08 UTC Comment hidden (obsolete)
Comment 20 Nick Clemens 2016-04-05 12:45:12 UTC Comment hidden (obsolete)
Comment 21 Nick Clemens 2016-04-05 12:45:59 UTC
(In reply to Hugo Agud from comment #16)
> I have been able to reproduce the error on koha 3.16.7 (I have not checked
> in more updated version of koha)
> 
> We have verified that If you edit a serial that has already been received,
> koha delete the existing barcode.
> 
> The test plan we suggest.
> 
> 1. Create one subscription with any serial pattern and create item when
> receiving an issue.
> 2. Receive one or two serial, fill the values of the items, in our test we
> have filled enumchron (automatically loaded from patter), signature
> (automatically loaded from subscription information) and barcode (manual).
> 3. Check that the item has been propertly added to biblionumber
> 4. Go to subscription--> serial collection --> Edit Serail --> select one of
> the received --> modify field 'Publish on'
> 5. Now the barcode has been deleted
> 
> Hope it helps

Was able to reproduce as described in this comment
Comment 22 Katrin Fischer 2016-04-05 22:17:41 UTC
Good news first:
- Can reproduce the problem!

Bad news:
- I think one of the patches above is doubled up, please check!
- Tests are failing for me: 

$ perl t/db_dependent/Serials.t
1..46
ok 1 - use C4::Serials;
ok 2 - NewSubscription should set notes
ok 3 - NewSubscription should set internalnotes
ok 4 - NewSubscription should not set subscriptionhistory opacnotes
ok 5 - NewSubscription should not set subscriptionhistory librariannotes
ok 6 - A reference of type 'ARRAY' isa 'ARRAY'
ok 7 - A reference of type 'ARRAY' isa 'ARRAY'
ok 8 - A reference of type 'ARRAY' isa 'ARRAY'
ok 9 - A reference of type 'ARRAY' isa 'ARRAY'
ok 10 - expiration date is not NULL
ok 11 - test getting history from sub-scription
ok 12 - Subscription has at least one serial
ok 13 - test getting Serial Status From Serial Id
Can't use an undefined value as a HASH reference at /home/katrin/kohaclone/cataloguing/value_builder/stocknumber.pl line 28.
# Looks like you planned 46 tests but ran 13.
# Looks like your test exited with 255 just after 13.
Comment 23 Mark Tompsett 2016-04-05 23:55:43 UTC
Nick signed off the first one, but didn't obsolete it. I did, these apply. However, now I get:

mtompset@debian:~/kohaclone$ prove t/db_dependent/Serials.t
t/db_dependent/Serials.t .. 1/46 No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
No method update! at /home/mtompset/kohaclone/C4/Serials.pm line 1602.
t/db_dependent/Serials.t .. ok
All tests successful.
Files=1, Tests=46,  1 wallclock secs ( 0.02 usr  0.00 sys +  1.13 cusr  0.02 csys =  1.17 CPU)
Result: PASS
Comment 24 Charles Farmer 2016-07-15 19:41:06 UTC
I'm sorry Mark, I honestly don't know what to make of this. On my system, this is what I get from a fresh install of 16.05 with an empty database:

charles@beauce(inlibro-dev)/inlibro/git/koha-inlibro-dev-inlibro$ prove t/db_dependent/Serials.t 
t/db_dependent/Serials.t .. 1/49 attempting to use non-existent class sorting routine 
Use of uninitialized value $cn_class in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_item in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_class in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_item in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_class in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_item in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_class in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_item in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
t/db_dependent/Serials.t .. 14/49 Use of uninitialized value $cn_class in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_item in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_class in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_item in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_class in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_item in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_class in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
Use of uninitialized value $cn_item in concatenation (.) or string at /inlibro/git/koha-inlibro-dev-inlibro/C4/ClassSortRoutine.pm line 116.
t/db_dependent/Serials.t .. ok     
All tests successful.
Files=1, Tests=49,  2 wallclock secs ( 0.03 usr  0.00 sys +  2.03 cusr  0.11 csys =  2.17 CPU)
Result: PASS

I see you ran 46 test, and we're now up to 48. I don't know if somebody else can try again to shed some new light on this, but I'll try to see what I can do about those empty concat.
Comment 25 Charles Farmer 2016-07-15 20:57:36 UTC Comment hidden (obsolete)
Comment 26 Charles Farmer 2016-07-15 20:58:18 UTC Comment hidden (obsolete)
Comment 27 Mark Tompsett 2016-07-15 21:36:13 UTC
Comment on attachment 53450 [details] [review]
Bug 15030: Fix the uninitialized warnings in ClassSortRoutine::_get_class_sort_key

Review of attachment 53450 [details] [review]:
-----------------------------------------------------------------

::: C4/ClassSortRoutine.pm
@@ +112,5 @@
>  =cut
>  
>  sub _get_class_sort_key {
> +    my $cn_class = shift || '';
> +    my $cn_item = shift || '';

@_ is the better way to assign. Shift on a missing parameter will trigger a warning.
$cn_class //= q{};
$cn_item  //= q{};
Then you could do two assigns like the above to handle if they aren't assigned values.
Comment 28 Mark Tompsett 2016-07-16 16:03:45 UTC
Comment on attachment 53450 [details] [review]
Bug 15030: Fix the uninitialized warnings in ClassSortRoutine::_get_class_sort_key

Had issues with applying when I fixed merge conflicts on a fresh apply. This patch wasn't applied, and error messages have disappeared. No need for this.
Comment 29 Mark Tompsett 2016-07-16 16:10:48 UTC
Created attachment 53456 [details] [review]
Bug 15030 - Certain values in serials' items are lost on next edit

When editing serials subscription, we can edit them but some values are not pulled from the DB correctly to be put in the edit box.  If not noticed, the value will be overwritten on the next save.

Test:
- Create a subscription
- Edit itemcallnumber (952o?) and make sure to have a different value than the default one.
- Save.
- Edit it again
- The saved value is not there.

This is true for itemcallnumber and a few other fields.

This was caused by calls to ->field($subfield).  This would always fail, of course.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Mark Tompsett 2016-07-16 16:10:55 UTC
Created attachment 53457 [details] [review]
Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value

This fixes the remaining fields from serials-edit.pl that were seeing their previously entered values
be oblitarated with each new edit.  The fields associated to a plugin (dateaccessioned and barcode) were
always displaying <empty> with each new edit, losing the previous effort.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Mark Tompsett 2016-07-16 16:11:04 UTC
Created attachment 53458 [details] [review]
Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 32 Mark Tompsett 2016-07-16 16:11:11 UTC
Created attachment 53459 [details] [review]
Bug 15030: Add tests

This test will prevent regression on the lost of data when
items.itemcallnumber is linked with a plugin.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 33 Mark Tompsett 2016-07-17 01:54:34 UTC
SHORT VERSION: Not enough test coverage.

LONGER VERSION: In these code changes, there are multiple blocks changed in C4::Items.

@@ -2890,7 +2890,7 (one if)
@@ -2901,7 +2901,7 (one if)
@@ -2909,7 +2909,7 (one if)
@@ -3003,15 +3003,18 (three pieces of code)

I placed a print STDERR "TRIGGERED!\n"; inside the ifs or below the last line of the block of code. We are still lacking test coverage on C4::Items. Only the first two of three changes were triggered in the last block of changes.

So, while I do believe the added test catches the failure before, I am uncertain it catches all the fixes after.
Comment 34 Liz Rea 2016-07-21 22:16:41 UTC
Mark says not enough tests, no reason to have this in the needs SO queue.
Comment 35 Katrin Fischer 2017-01-27 13:28:39 UTC
Critical bug, can we get this moving again? We had another report for barcode going missing when editing from the serials module. That's bad.
Comment 36 Katrin Fischer 2017-01-30 08:40:11 UTC
Losing data without noticing it... upping severity.
Comment 37 Jonathan Druart 2017-01-31 09:28:43 UTC
I personally think that the tests I have provided (6 months ago because it was critical) cover the changes made by this patchset.

They test C4::Serials::GetSerialInformation, which is the only call to C4::Items::PrepareItemrecordDisplay (in modules).
I do not see the point to struggle to cover and highlight the change made to this ugly subroutine when my 10 lines test do it properly and widely.

Critical one, data loose, waiting for 1 year and a half, I do not think it the kind of bugs we need to think to much when we have a solution already.

Switching back to Needs Signoff, a rebased version is coming.
Comment 38 Jonathan Druart 2017-01-31 09:29:13 UTC
Created attachment 59671 [details] [review]
Bug 15030 - Certain values in serials' items are lost on next edit

When editing serials subscription, we can edit them but some values are not pulled from the DB correctly to be put in the edit box.  If not noticed, the value will be overwritten on the next save.

Test:
- Create a subscription
- Edit itemcallnumber (952o?) and make sure to have a different value than the default one.
- Save.
- Edit it again
- The saved value is not there.

This is true for itemcallnumber and a few other fields.

This was caused by calls to ->field($subfield).  This would always fail, of course.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 39 Jonathan Druart 2017-01-31 09:29:18 UTC
Created attachment 59672 [details] [review]
Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value

This fixes the remaining fields from serials-edit.pl that were seeing their previously entered values
be oblitarated with each new edit.  The fields associated to a plugin (dateaccessioned and barcode) were
always displaying <empty> with each new edit, losing the previous effort.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 40 Jonathan Druart 2017-01-31 09:29:22 UTC
Created attachment 59673 [details] [review]
Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 41 Jonathan Druart 2017-01-31 09:29:27 UTC
Created attachment 59674 [details] [review]
Bug 15030: Add tests

This test will prevent regression on the lost of data when
items.itemcallnumber is linked with a plugin.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 42 Katrin Fischer 2017-01-31 10:38:05 UTC
(In reply to Jonathan Druart from comment #37)
> I personally think that the tests I have provided (6 months ago because it
> was critical) cover the changes made by this patchset.
> 
> They test C4::Serials::GetSerialInformation, which is the only call to
> C4::Items::PrepareItemrecordDisplay (in modules).
> I do not see the point to struggle to cover and highlight the change made to
> this ugly subroutine when my 10 lines test do it properly and widely.
> 
> Critical one, data loose, waiting for 1 year and a half, I do not think it
> the kind of bugs we need to think to much when we have a solution already.
> 
> Switching back to Needs Signoff, a rebased version is coming.

+1
Comment 43 Josef Moravec 2017-02-01 09:08:20 UTC
Created attachment 59723 [details] [review]
[SIGNED-OFF] Bug 15030 - Certain values in serials' items are lost on next edit

When editing serials subscription, we can edit them but some values are not pulled from the DB correctly to be put in the edit box.  If not noticed, the value will be overwritten on the next save.

Test:
- Create a subscription
- Edit itemcallnumber (952o?) and make sure to have a different value than the default one.
- Save.
- Edit it again
- The saved value is not there.

This is true for itemcallnumber and a few other fields.

This was caused by calls to ->field($subfield).  This would always fail, of course.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 44 Josef Moravec 2017-02-01 09:08:56 UTC
Created attachment 59724 [details] [review]
[SIGNED-OFF] Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value

This fixes the remaining fields from serials-edit.pl that were seeing their previously entered values
be oblitarated with each new edit.  The fields associated to a plugin (dateaccessioned and barcode) were
always displaying <empty> with each new edit, losing the previous effort.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 45 Josef Moravec 2017-02-01 09:09:16 UTC
Created attachment 59725 [details] [review]
[SIGNED-OFF] Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 46 Josef Moravec 2017-02-01 09:09:29 UTC
Created attachment 59726 [details] [review]
[SIGNED-OFF] Bug 15030: Add tests

This test will prevent regression on the lost of data when
items.itemcallnumber is linked with a plugin.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 47 Jonathan Druart 2017-02-02 11:22:50 UTC
Created attachment 59792 [details] [review]
Bug 15030 - Certain values in serials' items are lost on next edit

When editing serials subscription, we can edit them but some values are not pulled from the DB correctly to be put in the edit box.  If not noticed, the value will be overwritten on the next save.

Test:
- Create a subscription
- Edit itemcallnumber (952o?) and make sure to have a different value than the default one.
- Save.
- Edit it again
- The saved value is not there.

This is true for itemcallnumber and a few other fields.

This was caused by calls to ->field($subfield).  This would always fail, of course.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 48 Jonathan Druart 2017-02-02 11:22:55 UTC
Created attachment 59793 [details] [review]
Bug 15030 - Fixes the serials fields associated with a plugin, to not overwrite the previously saved value

This fixes the remaining fields from serials-edit.pl that were seeing their previously entered values
be oblitarated with each new edit.  The fields associated to a plugin (dateaccessioned and barcode) were
always displaying <empty> with each new edit, losing the previous effort.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 49 Jonathan Druart 2017-02-02 11:23:02 UTC
Created attachment 59794 [details] [review]
Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 50 Jonathan Druart 2017-02-02 11:23:07 UTC
Created attachment 59795 [details] [review]
Bug 15030: Add tests

This test will prevent regression on the lost of data when
items.itemcallnumber is linked with a plugin.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 51 Kyle M Hall 2017-02-07 17:44:33 UTC
Pushed to master for 17.05, thanks Philippe, Jonathan!
Comment 52 Katrin Fischer 2017-02-07 21:07:22 UTC
Glad to see this resolved!

These patches have been pushed to 16.11.x and will be in 16.11.04.
Comment 53 Julian Maurice 2017-02-09 09:24:48 UTC
Pushed to 3.22.x for 3.22.17
Comment 54 Mason James 2017-02-22 23:07:18 UTC
Pushed to 16.05.x, for 16.05.10 release