|
Description
Arthur Suzuki
2020-01-22 09:17:58 UTC
Created attachment 97695 [details] [review] Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC Test plan : - create a marc modification template which add a new field to a record - apply that template to some records - verify that the new field is always appended at the bottom of the record, even if some fields are higher. - apply patch - apply template to another set of records - verify the added field is now ordered within the already existing fields in the records Created attachment 97944 [details] [review] Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC Test plan : - create a marc modification template which add a new field to a record - apply that template to some records - verify that the new field is always appended at the bottom of the record, even if some fields are higher. - apply patch - apply template to another set of records - verify the added field is now ordered within the already existing fields in the records Created attachment 97945 [details] [review] Bug 24480: Unit test Created attachment 98975 [details] [review] Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC Test plan : - create a marc modification template which add a new field to a record - apply that template to some records - verify that the new field is always appended at the bottom of the record, even if some fields are higher. - apply patch - apply template to another set of records - verify the added field is now ordered within the already existing fields in the records Signed-off-by: Kelly <kelly@bywatersolutions.com> Created attachment 98976 [details] [review] Bug 24480: Unit test Signed-off-by: Kelly <kelly@bywatersolutions.com> Arthur, the tests from t/SimpleMARC.t will need to be adjusted as well. # Failed test 'copy_and_replace_field' # at t/SimpleMARC.t line 1471. # Looks like you failed 2 tests of 11. Created attachment 106621 [details] [review] Bug 24480: (QA follow-up) done, sry for the delay :) I am using Update Existing Field as the action, this is not working as expected. If a Marc Record has a 521$b already in the record, and I use the action to add a 521$a, this process is not placing the $a before the $b in the final record. *** Bug 25908 has been marked as a duplicate of this bug. *** When processing a Batch Record Modification, specifically adding a new marc field, Koha adds this new marc field to the bottom of the record and not in order with the other fields. It would be useful to have newly added marc fields to be in numeric order when the marc record is saved. A user would generally look for the marc field in the record where it should be and not look to the bottom of the record. I recently modified a few hundred thousand records and Koha put all the 582$b tags after the 999 tags not in order. It should be easily understandable that 5xx tags do not come after 9xx tags in MARC 21. I am adjusting status (FQA) regarding comment 9. It doesn't seem reasonable or productive to fail QA for comment 9. Fixing what this bug was filed for, where an entire added field goes, just involves switching which existing MARC::Record method to call, and the current behavior of sticking new fields at the end of the record is never going to be right (unless you are adding a second 999 field, which would be rather odd behavior). Fixing the already present behavior which comment 9 brings up about subfields rather than fields requires first specifying how subfields ought to be ordered (alphabetical would maybe be right in a few more cases than at the end, but there are a very large number of cases (e.g. 100 $a$q$d) where alphabetical is completely wrong, so a useful implementation as opposed to a differently-useless one would need to allow for a truly awful UI with choices to add before or after the first, last, and perhaps nth instance of subfield __), and then implementing that in MARC::Field since it only has the one subfield method which adds at the end, and then requiring that new version of MARC::Field, and adding a bunch of subfield choices to SimpleMARC.pm and maybe renaming it NotSoSimpleMARC.pm as a result. Far better to file that as a separate enhancement request, and take this patch perhaps after FQAing it over changing the behavior of the SimpleMARC API instead of adding to it, or for the unexpected way it is using insert_fields_ordered() to put a new 520 after any existing 520s except in the case where rather than Add new you Move an existing 502 to 520, in which case it uses insert_grouped_field() to put the 520 after an existing 586 or 590. Hi Phil, sorry I misunderstand, but are you suggesting we handle fields and subfields on separate bug reports? Katrin: yes, please! Given the code and the complexity of the problems, there are three separate bugs: * this one about changing field insertion from clearly wrong to the better of two other choices, which we could have landed in a day or two because it requires no UI or backend changes * one that would require complicated UI changes but nearly no backend changes to allow specifying whether to insert a field in numerical order or grouped by hundreds (a new 502 placed after an existing 590, which apparently some people like) or before or after a specified other field * one that would require complicated UI changes and also backend changes to do anything better about inserting subfields, because inserting them alphabetically is no more correct than inserting them at the end I filed bug 26716 for more choices about where to insert fields, and bug 26717 for choices about where to insert subfields. Leaving at Failed QA for the mix of insert_fields_ordered and insert_grouped_field; I don't have any opinion about how stable the SimpleMARC API should be. Phil, let's give this another go. I wanted to set back to "Needs Signoff", but the tests are failing. Can you please have a look at them first? Test Summary Report ------------------- t/db_dependent/MarcModificationTemplates.t (Wstat: 1024 Tests: 126 Failed: 4) Failed tests: 98, 115-116, 126 Non-zero exit status: 4 Files=2, Tests=137, 2 wallclock secs ( 0.07 usr 0.02 sys + 1.43 cusr 0.40 csys = 1.92 CPU) Result: FAIL There are two causes for the test failures. The first is that git bz apply uses far too much fuzz, and happily applies the first hunk of attachment 98976 [details] [review] to sub new_record rather than to sub expected_record_1 where it belongs. Just needs a rebased patch. The second is that the test added by bug 17510 needs to have its expected_record adjusted, but it coincidentally tests the behavior this patch introduces of using insert_grouped_field for moves while using insert_fields_ordered for additions, so right now it needs to be adjusted to put the 600 below the 650 instead of having them in order, and since I don't yet see any reason for that behavior, I don't want to attach a patch which "successfully" tests that we do it. Created attachment 112311 [details] [review] Bug 24480: Unit test (rebased) Signed-off-by: Kelly <kelly@bywatersolutions.com> Well, that's fun: I put it in expected_record_1 where it belongs, and wound up with a patch that actually says it's in new_record rather than one that says it isn't but winds up putting it there. Created attachment 112312 [details] [review] Bug 24480: Unit test (rebased) Signed-off-by: Kelly <kelly@bywatersolutions.com> is this patch still failed QA? Created attachment 150665 [details] [review] Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC Test plan : - create a marc modification template which add a new field to a record - apply that template to some records - verify that the new field is always appended at the bottom of the record, even if some fields are higher. - apply patch - apply template to another set of records - verify the added field is now ordered within the already existing fields in the records Signed-off-by: Kelly <kelly@bywatersolutions.com> Created attachment 150666 [details] [review] Bug 24480: Unit test This updates the SimpleMARC tests and MarcModificationTemplates tests to expect the new order of fields Signed-off-by: Kelly <kelly@bywatersolutions.com> Why use insert_grouped_field for move rather than insert_fields_ordered? My vague memory of looking at it in 2020 says that it's because just using $record->insert_fields_ordered( @new_fields ); winds up putting them in reversed, so that if you say to move all 502 to 520 you go from 502 $aFirst 502 $aSecond to 520 $aSecond 520 $aFirst but if that was all it was, why not just reverse @new_fields before calling insert_fields_ordered? And that does work: $record->insert_fields_ordered( reverse( @new_fields ) ); will copy/move "First" "Second" to "First" "Second". Just requires a little more test adjustment. Comment on attachment 150666 [details] [review] Bug 24480: Unit test >diff --git a/Koha/SimpleMARC.pm b/Koha/SimpleMARC.pm >index 0c95ccf804..d3c05647ff 100644 >--- a/Koha/SimpleMARC.pm >+++ b/Koha/SimpleMARC.pm >@@ -575,6 +575,13 @@ sub _copy_move_field { > @from_fields = map { $_ <= @from_fields ? $from_fields[ $_ - 1 ] : () } @$field_numbers; > } > >+ if ( $action eq 'replace' ) { >+ my @to_fields = $record->field( $toFieldName ); >+ for my $to_field ( @to_fields ) { >+ $record->delete_field( $to_field ); >+ } >+ } >+ > for my $from_field ( @from_fields ) { > my $new_field = $from_field->clone; > $new_field->{_tag} = $toFieldName; # Should be replaced by set_tag, introduced by MARC::Field 2.0.4 >@@ -588,12 +595,6 @@ sub _copy_move_field { > if ( $action eq 'move' ) { > $record->delete_field( $from_field ) > } >- elsif ( $action eq 'replace' ) { >- my @to_fields = $record->field( $toFieldName ); >- if ( @to_fields ) { >- $record->delete_field( $to_fields[0] ); >- } >- } > $record->insert_grouped_field( $new_field ); > } > } Changing the meaning of a Copy and replace action from "copy n from fields, replacing the first n to fields" to "copy n from fields, replacing every to field" may be a reasonable change (I don't have an opinion, since I never use Copy and replace), but doing so in a patch with the summary "Unit test" seems very wrong. Like Failed QA wrong. Nick: comment 28, why? Should this still be FQA? I'd like to see it move along! Do things typically become not-FQA from reasons outside the bug? The patches in this bug have always chosen to implement move with insert_grouped_field, which puts a 600 after a 690, rather than insert_field_ordered, which puts a 600 with 600s, before 610. Nobody has ever responded with a reason why, going back to 2020, only asked "is this still FQA?" Nick's update of the patches included a change to the behavior of copy and replace, from replace the first n to replace all, in a patch called "Unit test." No response as to why, or even whether it was intentional or accidental. Arthur, can you provide feedback here please? Created attachment 155714 [details] [review] Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC Test plan : - create a marc modification template which add a new field to a record - apply that template to some records - verify that the new field is always appended at the bottom of the record, even if some fields are higher. - apply patch - apply template to another set of records - verify the added field is now ordered within the already existing fields in the records Signed-off-by: Kelly <kelly@bywatersolutions.com> Created attachment 155715 [details] [review] Bug 24480: Unit test This updates the SimpleMARC tests and MarcModificationTemplates tests to expect the new order of fields Signed-off-by: Kelly <kelly@bywatersolutions.com> Created attachment 155716 [details] [review] Bug 24480: (follow-up) Shift new fields into array and add after all are copied The updated code removed the pushing of fields to an array - this caused a bug. When multiple fields were copied and replaced and there were more fields added than existed originally we were adding the new field to the end, then removing the first occurence of the original field. If we tried to move 2 650 fields to 651 and the record had no 651 we would: - Delete the first 651, there were none, so nothing happened - Take the first 650, add it to the end of the 651 group (there were none, so it became the first 651) - Delete the first 651, which was the field we just copied - Take the second 650 and add it to the end of the 651 group (whihc had none, because we deleted it) I re-add the line, but do as suggesed by Phil and reverse the order (unshift vs push) To test: 1 - Apply other patches 2 - prove -v t/SimpleMARC.t 3 - It fails 4 - Apply this patch 5 - prove -v t/SimpleMARC.t 6 - It still fails, but more tests pass (there's another patch to follow) Created attachment 155717 [details] [review] Bug 24480: (follow-up) Adjust tests With the way the patchset alters to use insert_fields_ordered, we now add fields to the beginning of their number group as opposed to appending before. We just need ot shift the order we expect the fields to end up in to make the tests pass To test: 1 - Apply all patches except this 2 - prove -v t/SimpleMARC.t t/db_dependent/MarcModificationTemplates.t 3 - It fails 4 - Apply this 5 - prove -v t/SimpleMARC.t t/db_dependent/MarcModificationTemplates.t 6 - It passes (In reply to Phil Ringnalda from comment #30) > Nick: comment 28, why? I think I was trying to make the tests pass, and thought it was the actual intended behavior. I do still think it's right, but agree that was not the best place to fix that. I have fixed the test here without changing behavior, or at least not as mich as before ;-) The comment on the bug should explain The second test patch highlights another change from this patchset, and updates the patches accordingly. Moving back to NSO for feedback Test pass? Copy, move, and insert all have the same ordering behavior? THAT'S THE PATCHSET I'VE BEEN LOOKING FOR! Thanks, Nick! Created attachment 155721 [details] [review] Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC Test plan : - create a marc modification template which add a new field to a record - apply that template to some records - verify that the new field is always appended at the bottom of the record, even if some fields are higher. - apply patch - apply template to another set of records - verify the added field is now ordered within the already existing fields in the records Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Created attachment 155722 [details] [review] Bug 24480: Unit test This updates the SimpleMARC tests and MarcModificationTemplates tests to expect the new order of fields Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Created attachment 155723 [details] [review] Bug 24480: (follow-up) Shift new fields into array and add after all are copied The updated code removed the pushing of fields to an array - this caused a bug. When multiple fields were copied and replaced and there were more fields added than existed originally we were adding the new field to the end, then removing the first occurence of the original field. If we tried to move 2 650 fields to 651 and the record had no 651 we would: - Delete the first 651, there were none, so nothing happened - Take the first 650, add it to the end of the 651 group (there were none, so it became the first 651) - Delete the first 651, which was the field we just copied - Take the second 650 and add it to the end of the 651 group (whihc had none, because we deleted it) I re-add the line, but do as suggesed by Phil and reverse the order (unshift vs push) To test: 1 - Apply other patches 2 - prove -v t/SimpleMARC.t 3 - It fails 4 - Apply this patch 5 - prove -v t/SimpleMARC.t 6 - It still fails, but more tests pass (there's another patch to follow) Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Created attachment 155724 [details] [review] Bug 24480: (follow-up) Adjust tests With the way the patchset alters to use insert_fields_ordered, we now add fields to the beginning of their number group as opposed to appending before. We just need ot shift the order we expect the fields to end up in to make the tests pass To test: 1 - Apply all patches except this 2 - prove -v t/SimpleMARC.t t/db_dependent/MarcModificationTemplates.t 3 - It fails 4 - Apply this 5 - prove -v t/SimpleMARC.t t/db_dependent/MarcModificationTemplates.t 6 - It passes Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Created attachment 158130 [details] [review] Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC Test plan : - create a marc modification template which add a new field to a record - apply that template to some records - verify that the new field is always appended at the bottom of the record, even if some fields are higher. - apply patch - apply template to another set of records - verify the added field is now ordered within the already existing fields in the records Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 158131 [details] [review] Bug 24480: Unit test This updates the SimpleMARC tests and MarcModificationTemplates tests to expect the new order of fields Signed-off-by: Kelly <kelly@bywatersolutions.com> Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 158132 [details] [review] Bug 24480: (follow-up) Shift new fields into array and add after all are copied The updated code removed the pushing of fields to an array - this caused a bug. When multiple fields were copied and replaced and there were more fields added than existed originally we were adding the new field to the end, then removing the first occurence of the original field. If we tried to move 2 650 fields to 651 and the record had no 651 we would: - Delete the first 651, there were none, so nothing happened - Take the first 650, add it to the end of the 651 group (there were none, so it became the first 651) - Delete the first 651, which was the field we just copied - Take the second 650 and add it to the end of the 651 group (whihc had none, because we deleted it) I re-add the line, but do as suggesed by Phil and reverse the order (unshift vs push) To test: 1 - Apply other patches 2 - prove -v t/SimpleMARC.t 3 - It fails 4 - Apply this patch 5 - prove -v t/SimpleMARC.t 6 - It still fails, but more tests pass (there's another patch to follow) Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 158133 [details] [review] Bug 24480: (follow-up) Adjust tests With the way the patchset alters to use insert_fields_ordered, we now add fields to the beginning of their number group as opposed to appending before. We just need ot shift the order we expect the fields to end up in to make the tests pass To test: 1 - Apply all patches except this 2 - prove -v t/SimpleMARC.t t/db_dependent/MarcModificationTemplates.t 3 - It fails 4 - Apply this 5 - prove -v t/SimpleMARC.t t/db_dependent/MarcModificationTemplates.t 6 - It passes Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 23.11. Nice work everyone, thanks! Small and useful enhancement, I choose to backport. Pushed to 23.05.x for 23.05.06 |