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.
Created attachment 43601 [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.
Created attachment 43701 [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.
Created attachment 43785 [details] [review] Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied
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.
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).
Created attachment 44798 [details] [review] Bug 15030: Add tests This test will prevent regression on the lost of data when items.itemcallnumber is linked with a plugin.
This is not "blocker".
Hi, could you be more specific with test plan? Thank you
There is conflict in t/db_dependent/Serials.t, could you please rebase?
Created attachment 45297 [details] [review] Bug 15030: Add tests This test will prevent regression on the lost of data when items.itemcallnumber is linked with a plugin.
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?!
Created attachment 45753 [details] [review] Bug 15030: Add tests This test will prevent regression on the lost of data when items.itemcallnumber is linked with a plugin.
(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!
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.
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
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
Created attachment 49908 [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>
Created attachment 49909 [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>
Created attachment 49910 [details] [review] Bug 15030 - continue. The enumchron value was overwritten by 'header' even when value supplied Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 49911 [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>
(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
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.
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
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.
Created attachment 53449 [details] [review] Fix the uninitialized warnings in ClassSortRoutine::_get_class_sort_key
Created attachment 53450 [details] [review] Bug 15030: Fix the uninitialized warnings in ClassSortRoutine::_get_class_sort_key
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 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.
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>
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>
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>
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>
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.
Mark says not enough tests, no reason to have this in the needs SO queue.
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.
Losing data without noticing it... upping severity.
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.
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>
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>
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>
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>
(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
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>
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>
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>
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>
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>
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>
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>
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>
Pushed to master for 17.05, thanks Philippe, Jonathan!
Glad to see this resolved! These patches have been pushed to 16.11.x and will be in 16.11.04.
Pushed to 3.22.x for 3.22.17
Pushed to 16.05.x, for 16.05.10 release