[Bug 24480] New: Fields added with Marc modifications templates are not added in an ordered way
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Bug ID: 24480 Summary: Fields added with Marc modifications templates are not added in an ordered way Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Tools Assignee: koha-bugs@lists.koha-community.org Reporter: arthur.suzuki@biblibre.com QA Contact: testopia@bugs.koha-community.org they are instead appended to the end of the record. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |arthur.suzuki@biblibre.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #1 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 97695 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=97695&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #97695|0 |1 is obsolete| | --- Comment #2 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 97944 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=97944&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #3 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 97945 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=97945&action=edit Bug 24480: Unit test -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 ByWater Sandboxes <bws.sandboxes@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #97944|0 |1 is obsolete| | --- Comment #4 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 98975 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98975&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 ByWater Sandboxes <bws.sandboxes@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #97945|0 |1 is obsolete| | --- Comment #5 from ByWater Sandboxes <bws.sandboxes@gmail.com> --- Created attachment 98976 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=98976&action=edit Bug 24480: Unit test Signed-off-by: Kelly <kelly@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Kelly McElligott <kelly@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kelly@bywatersolutions.com Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Arthur Suzuki <arthur.suzuki@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #7 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- Created attachment 106621 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=106621&action=edit Bug 24480: (QA follow-up) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #8 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- done, sry for the delay :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Kelly McElligott <kelly@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #9 from Kelly McElligott <kelly@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- *** Bug 25908 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Michael Sutherland <sudrland@vt.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sudrland@vt.edu --- Comment #11 from Michael Sutherland <sudrland@vt.edu> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #12 from Michael Sutherland <sudrland@vt.edu> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Failed QA --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I am adjusting status (FQA) regarding comment 9. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |phil@chetcolibrary.org --- Comment #14 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #15 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Phil, sorry I misunderstand, but are you suggesting we handle fields and subfields on separate bug reports? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #16 from Phil Ringnalda <phil@chetcolibrary.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #17 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #18 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #19 from Phil Ringnalda <phil@chetcolibrary.org> --- 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 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #98976|0 |1 is obsolete| | --- Comment #20 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 112311 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=112311&action=edit Bug 24480: Unit test (rebased) Signed-off-by: Kelly <kelly@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #21 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #112311|0 |1 is obsolete| | --- Comment #22 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 112312 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=112312&action=edit Bug 24480: Unit test (rebased) Signed-off-by: Kelly <kelly@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #98976|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #112312|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #23 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- is this patch still failed QA? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 mathieu saby <mathsabypro@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mathsabypro@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Esther Melander <estherm@sodaspringsid.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |estherm@sodaspringsid.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Fields added with Marc |Fields added with MARC |modifications templates are |modifications templates are |not added in an ordered way |not added in an ordered way -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #98975|0 |1 is obsolete| | Attachment #98976|0 |1 is obsolete| | Attachment #106621|0 |1 is obsolete| | --- Comment #24 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 150665 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150665&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #25 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 150666 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150666&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #26 from Phil Ringnalda <phil@chetcolibrary.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #27 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #28 from Phil Ringnalda <phil@chetcolibrary.org> --- Comment on attachment 150666 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150666 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #29 from Phil Ringnalda <phil@chetcolibrary.org> --- Like Failed QA wrong. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #30 from Phil Ringnalda <phil@chetcolibrary.org> --- Nick: comment 28, why? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #31 from Lucas Gass <lucas@bywatersolutions.com> --- Should this still be FQA? I'd like to see it move along! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #32 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #33 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Arthur, can you provide feedback here please? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150665|0 |1 is obsolete| | Attachment #150666|0 |1 is obsolete| | --- Comment #34 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 155714 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155714&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #35 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 155715 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155715&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #36 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 155716 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155716&action=edit 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #37 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 155717 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155717&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff --- Comment #38 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|arthur.suzuki@biblibre.com |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #39 from Phil Ringnalda <phil@chetcolibrary.org> --- Test pass? Copy, move, and insert all have the same ordering behavior? THAT'S THE PATCHSET I'VE BEEN LOOKING FOR! Thanks, Nick! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155714|0 |1 is obsolete| | --- Comment #40 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 155721 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155721&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155715|0 |1 is obsolete| | --- Comment #41 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 155722 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155722&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155716|0 |1 is obsolete| | --- Comment #42 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 155723 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155723&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155717|0 |1 is obsolete| | --- Comment #43 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 155724 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155724&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |katrin.fischer@bsz-bw.de |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155721|0 |1 is obsolete| | --- Comment #44 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 158130 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158130&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155722|0 |1 is obsolete| | --- Comment #45 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 158131 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158131&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155723|0 |1 is obsolete| | --- Comment #46 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 158132 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158132&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155724|0 |1 is obsolete| | --- Comment #47 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 158133 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158133&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |23.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 --- Comment #48 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|23.11.00 |23.11.00,23.05.06 released in| | CC| |fridolin.somers@biblibre.co | |m --- Comment #49 from Fridolin Somers <fridolin.somers@biblibre.com> --- Small and useful enhancement, I choose to backport. Pushed to 23.05.x for 23.05.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24480 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED CC| |pedro.amorim@ptfs-europe.co | |m Status|Pushed to stable |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org