Bug 24480 - Fields added with MARC modifications templates are not added in an ordered way
Summary: Fields added with MARC modifications templates are not added in an ordered way
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 20 votes (vote)
Assignee: Nick Clemens
QA Contact: Katrin Fischer
URL:
Keywords:
: 25908 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-01-22 09:17 UTC by Arthur Suzuki
Modified: 2023-11-14 15:11 UTC (History)
10 users (show)

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


Attachments
Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC (1.39 KB, patch)
2020-01-22 09:54 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC (2.80 KB, patch)
2020-01-25 22:36 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 24480: Unit test (2.79 KB, patch)
2020-01-25 22:37 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC (2.85 KB, patch)
2020-02-14 16:39 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24480: Unit test (2.75 KB, patch)
2020-02-14 16:39 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24480: (QA follow-up) (8.27 KB, patch)
2020-07-07 00:32 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 24480: Unit test (rebased) (2.84 KB, patch)
2020-10-23 21:37 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 24480: Unit test (rebased) (2.84 KB, patch)
2020-10-23 21:49 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC (2.85 KB, patch)
2023-05-04 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24480: Unit test (12.13 KB, patch)
2023-05-04 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC (2.85 KB, patch)
2023-09-15 20:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24480: Unit test (12.13 KB, patch)
2023-09-15 20:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24480: (follow-up) Shift new fields into array and add after all are copied (2.53 KB, patch)
2023-09-15 20:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24480: (follow-up) Adjust tests (4.24 KB, patch)
2023-09-15 20:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC (2.91 KB, patch)
2023-09-15 21:35 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 24480: Unit test (12.18 KB, patch)
2023-09-15 21:35 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 24480: (follow-up) Shift new fields into array and add after all are copied (2.58 KB, patch)
2023-09-15 21:35 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 24480: (follow-up) Adjust tests (4.29 KB, patch)
2023-09-15 21:35 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 24480: Fix insert_fields_ordered instead of append_fields in SimpleMARC (2.96 KB, patch)
2023-10-31 14:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24480: Unit test (12.24 KB, patch)
2023-10-31 14:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24480: (follow-up) Shift new fields into array and add after all are copied (2.64 KB, patch)
2023-10-31 14:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24480: (follow-up) Adjust tests (4.35 KB, patch)
2023-10-31 14:51 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Suzuki 2020-01-22 09:17:58 UTC
they are instead appended to the end of the record.
Comment 1 Arthur Suzuki 2020-01-22 09:54:26 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
Comment 2 Arthur Suzuki 2020-01-25 22:36:27 UTC
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
Comment 3 Arthur Suzuki 2020-01-25 22:37:02 UTC
Created attachment 97945 [details] [review]
Bug 24480: Unit test
Comment 4 ByWater Sandboxes 2020-02-14 16:39:01 UTC
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>
Comment 5 ByWater Sandboxes 2020-02-14 16:39:03 UTC
Created attachment 98976 [details] [review]
Bug 24480: Unit test

Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 6 Jonathan Druart 2020-02-24 11:59:49 UTC
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.
Comment 7 Arthur Suzuki 2020-07-07 00:32:42 UTC
Created attachment 106621 [details] [review]
Bug 24480: (QA follow-up)
Comment 8 Arthur Suzuki 2020-07-07 00:33:29 UTC
done, sry for the delay :)
Comment 9 Kelly McElligott 2020-07-30 11:18:23 UTC
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.
Comment 10 Jonathan Druart 2020-08-05 09:12:21 UTC
*** Bug 25908 has been marked as a duplicate of this bug. ***
Comment 11 Michael Sutherland 2020-08-05 12:18:08 UTC
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.
Comment 12 Michael Sutherland 2020-08-05 12:23:22 UTC
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.
Comment 13 Jonathan Druart 2020-08-05 12:46:45 UTC
I am adjusting status (FQA) regarding comment 9.
Comment 14 Phil Ringnalda 2020-10-16 06:09:40 UTC
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.
Comment 15 Katrin Fischer 2020-10-16 10:59:10 UTC
Hi Phil,

sorry I misunderstand, but are you suggesting we handle fields and subfields on separate bug reports?
Comment 16 Phil Ringnalda 2020-10-16 16:06:46 UTC
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
Comment 17 Phil Ringnalda 2020-10-16 16:48:23 UTC
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.
Comment 18 Katrin Fischer 2020-10-21 00:30:28 UTC
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
Comment 19 Phil Ringnalda 2020-10-21 05:38:21 UTC
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.
Comment 20 Phil Ringnalda 2020-10-23 21:37:54 UTC
Created attachment 112311 [details] [review]
Bug 24480: Unit test (rebased)

Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 21 Phil Ringnalda 2020-10-23 21:44:01 UTC
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.
Comment 22 Phil Ringnalda 2020-10-23 21:49:16 UTC
Created attachment 112312 [details] [review]
Bug 24480: Unit test (rebased)

Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 23 Arthur Suzuki 2021-01-15 08:20:38 UTC
is this patch still failed QA?
Comment 24 Nick Clemens 2023-05-04 13:43:53 UTC
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>
Comment 25 Nick Clemens 2023-05-04 13:43:55 UTC
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>
Comment 26 Phil Ringnalda 2023-05-04 14:49:57 UTC
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?
Comment 27 Phil Ringnalda 2023-07-03 00:54:25 UTC
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 28 Phil Ringnalda 2023-07-03 00:59:14 UTC
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.
Comment 29 Phil Ringnalda 2023-07-03 00:59:44 UTC
Like Failed QA wrong.
Comment 30 Phil Ringnalda 2023-07-03 01:04:33 UTC
Nick: comment 28, why?
Comment 31 Lucas Gass 2023-08-16 16:34:42 UTC
Should this still be FQA? I'd like to see it move along!
Comment 32 Phil Ringnalda 2023-08-16 17:09:34 UTC
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.
Comment 33 Jonathan Druart 2023-08-30 09:07:29 UTC
Arthur, can you provide feedback here please?
Comment 34 Nick Clemens 2023-09-15 20:21:18 UTC
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>
Comment 35 Nick Clemens 2023-09-15 20:21:21 UTC
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>
Comment 36 Nick Clemens 2023-09-15 20:21:23 UTC
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)
Comment 37 Nick Clemens 2023-09-15 20:21:26 UTC
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
Comment 38 Nick Clemens 2023-09-15 20:24:02 UTC
(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
Comment 39 Phil Ringnalda 2023-09-15 21:16:40 UTC
Test pass? Copy, move, and insert all have the same ordering behavior?

THAT'S THE PATCHSET I'VE BEEN LOOKING FOR!

Thanks, Nick!
Comment 40 Phil Ringnalda 2023-09-15 21:35:01 UTC
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>
Comment 41 Phil Ringnalda 2023-09-15 21:35:05 UTC
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>
Comment 42 Phil Ringnalda 2023-09-15 21:35:08 UTC
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>
Comment 43 Phil Ringnalda 2023-09-15 21:35:12 UTC
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>
Comment 44 Katrin Fischer 2023-10-31 14:51:29 UTC
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>
Comment 45 Katrin Fischer 2023-10-31 14:51:32 UTC
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>
Comment 46 Katrin Fischer 2023-10-31 14:51:36 UTC
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>
Comment 47 Katrin Fischer 2023-10-31 14:51:39 UTC
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>
Comment 48 Tomás Cohen Arazi 2023-11-03 15:05:42 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 49 Fridolin Somers 2023-11-10 01:59:30 UTC
Small and useful enhancement, I choose to backport.

Pushed to 23.05.x for 23.05.06