Bug 21708 - Editing a record moves field 999 to first in the marcxml
Summary: Editing a record moves field 999 to first in the marcxml
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
: 21881 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-10-29 07:12 UTC by paxed
Modified: 2020-04-16 21:49 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05, 19.05.10


Attachments
999 should not be in the first field (2.56 KB, application/x-perl)
2018-12-18 13:03 UTC, Jonathan Druart
Details
Bug 21708: Sort MARC fields in TransformHtmlToMarc (674 bytes, patch)
2020-03-10 15:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24544: Add script to insert persistent identifiers (3.80 KB, patch)
2020-03-11 13:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21708: Add a few tests in TransformHtmlToMarc.t (5.34 KB, patch)
2020-03-11 13:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21708: Sort MARC fields in TransformHtmlToMarc (1.10 KB, patch)
2020-03-11 13:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21708: Add a few tests in TransformHtmlToMarc.t (5.31 KB, patch)
2020-03-21 12:29 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 21708: Sort MARC fields in TransformHtmlToMarc (1.10 KB, patch)
2020-03-21 12:29 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 21708: Add a few tests in TransformHtmlToMarc.t (5.37 KB, patch)
2020-03-22 14:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21708: Sort MARC fields in TransformHtmlToMarc (1.15 KB, patch)
2020-03-22 14:19 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 paxed 2018-10-29 07:12:28 UTC
1) Import or create a new record.
2) Examine the marcxml of that record in the database - the fields are in the correct numerical order.
3) Edit the record - no need to change any data, just save it.
4) Examine the marcxml in the database - the 999 field is now right after leader and before 001.

This is due to TransformHtmlToMarc in C4/Biblio.pm, specifically the $param eq 'biblionumber' case, which pushes the param in the array first, and then the sub uses $record->append_field()

Perhaps the array should be sorted by field tag before appending the fields?

AFAIK, this doesn't really show up anywhere in Koha, but if you ever edit the records via a script using MARC::Record, new fields cannot be easily inserted into correct position.
Comment 1 David Cook 2018-10-30 02:25:46 UTC
I've noticed this as well recently. I was wondering if it was intentional or not...
Comment 2 Jonathan Druart 2018-12-17 17:11:42 UTC
Is this really a regression? If it is, could you tell me a version which was behaving as expected?
Comment 3 Jonathan Druart 2018-12-17 17:12:00 UTC

*** This bug has been marked as a duplicate of bug 21881 ***
Comment 4 David Cook 2018-12-18 03:53:46 UTC
(In reply to Jonathan Druart from comment #3)
> 
> *** This bug has been marked as a duplicate of bug 21881 ***

I think maybe 21881 should've been marked as a duplicate of this bug? Should we move the discussion over to 21881?
Comment 5 David Cook 2018-12-18 03:54:49 UTC
(In reply to Jonathan Druart from comment #2)
> Is this really a regression? If it is, could you tell me a version which was
> behaving as expected?

I'd say it's a regression. As for a version which was behaving as expected... I'd have to double-check, but I think it was OK in Koha 3.20 but not Koha 17.05.
Comment 6 Jonathan Druart 2018-12-18 12:31:19 UTC
*** Bug 21881 has been marked as a duplicate of this bug. ***
Comment 7 Jonathan Druart 2018-12-18 12:32:11 UTC
(In reply to David Cook from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > 
> > *** This bug has been marked as a duplicate of bug 21881 ***
> 
> I think maybe 21881 should've been marked as a duplicate of this bug? Should
> we move the discussion over to 21881?

Sorry, I meant to do the reverse.
Comment 8 Jonathan Druart 2018-12-18 13:03:12 UTC
Created attachment 83365 [details]
999 should not be in the first field
Comment 9 Jonathan Druart 2018-12-18 13:04:49 UTC
(In reply to David Cook from comment #5)
> (In reply to Jonathan Druart from comment #2)
> > Is this really a regression? If it is, could you tell me a version which was
> > behaving as expected?
> 
> I'd say it's a regression. As for a version which was behaving as
> expected... I'd have to double-check, but I think it was OK in Koha 3.20 but
> not Koha 17.05.

I have written a quick test to confirm that: it seems that 3.20 behaved as it already.
Comment 10 Marcel de Rooy 2020-03-10 15:12:41 UTC
Created attachment 100469 [details] [review]
Bug 21708: Sort MARC fields in TransformHtmlToMarc

Instead of pushing 999 in the front :)
Comment 11 Marcel de Rooy 2020-03-10 15:13:47 UTC
Still adding small test and benchmarking
Comment 12 Marcel de Rooy 2020-03-10 15:23:32 UTC
Record with 25 fields: 0.000038 sec
Comment 13 Marcel de Rooy 2020-03-10 16:00:21 UTC
(In reply to paxed from comment #0)
> This is due to TransformHtmlToMarc in C4/Biblio.pm, specifically the $param
> eq 'biblionumber' case, which pushes the param in the array first, and then
> the sub uses $record->append_field()
> 
> Perhaps the array should be sorted by field tag before appending the fields?

Yes, I saw that too :)
Comment 14 Marcel de Rooy 2020-03-11 13:38:32 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2020-03-11 13:39:22 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2020-03-11 13:40:10 UTC
Created attachment 100557 [details] [review]
Bug 21708: Add a few tests in TransformHtmlToMarc.t

Test plan:
Run t/db_dependent/Biblio/TransformHtmlToMarc.t without the module change
patch. Verify that the test fails on the order of fields.
Go to test plan of the second patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2020-03-11 13:40:14 UTC
Created attachment 100558 [details] [review]
Bug 21708: Sort MARC fields in TransformHtmlToMarc

Instead of pushing 999 in the front :)

Test plan:
Do not yet apply this patch.
Edit a record. Save. And open MARC view/view plain on opac-detail.
Verify that the biblionumber field (MARC21 999) follows leader.
Apply this patch. Restart Plack.
Edit a record. Save. And open MARC view/view plain on opac-detail.
Verify that the biblionumber field is in right position.
Run t/db_dependent/Biblio/TransformHtmlToMarc.t. Should pass now.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Bernardo Gonzalez Kriegel 2020-03-21 12:29:00 UTC
Created attachment 101334 [details] [review]
Bug 21708: Add a few tests in TransformHtmlToMarc.t

Test plan:
Run t/db_dependent/Biblio/TransformHtmlToMarc.t without the module change
patch. Verify that the test fails on the order of fields.
Go to test plan of the second patch.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test fails
Comment 19 Bernardo Gonzalez Kriegel 2020-03-21 12:29:05 UTC
Created attachment 101335 [details] [review]
Bug 21708: Sort MARC fields in TransformHtmlToMarc

Instead of pushing 999 in the front :)

Test plan:
Do not yet apply this patch.
Edit a record. Save. And open MARC view/view plain on opac-detail.
Verify that the biblionumber field (MARC21 999) follows leader.
Apply this patch. Restart Plack.
Edit a record. Save. And open MARC view/view plain on opac-detail.
Verify that the biblionumber field is in right position.
Run t/db_dependent/Biblio/TransformHtmlToMarc.t. Should pass now.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test pass, correct tag order.
Comment 20 Katrin Fischer 2020-03-22 14:19:08 UTC
Created attachment 101377 [details] [review]
Bug 21708: Add a few tests in TransformHtmlToMarc.t

Test plan:
Run t/db_dependent/Biblio/TransformHtmlToMarc.t without the module change
patch. Verify that the test fails on the order of fields.
Go to test plan of the second patch.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test fails

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2020-03-22 14:19:13 UTC
Created attachment 101378 [details] [review]
Bug 21708: Sort MARC fields in TransformHtmlToMarc

Instead of pushing 999 in the front :)

Test plan:
Do not yet apply this patch.
Edit a record. Save. And open MARC view/view plain on opac-detail.
Verify that the biblionumber field (MARC21 999) follows leader.
Apply this patch. Restart Plack.
Edit a record. Save. And open MARC view/view plain on opac-detail.
Verify that the biblionumber field is in right position.
Run t/db_dependent/Biblio/TransformHtmlToMarc.t. Should pass now.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Test pass, correct tag order.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Marcel de Rooy 2020-03-23 07:01:43 UTC
Thx Bernardo and Katrin
Comment 23 Martin Renvoize 2020-03-24 09:29:59 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 24 Joy Nelson 2020-04-03 21:32:39 UTC
backported to 19.11.x for 19.11.05
Comment 25 Lucas Gass 2020-04-13 20:13:34 UTC
backported to 19.05.x for 19.05.10
Comment 26 Hayley Mapley 2020-04-16 21:49:33 UTC
Minor behaviour patch will not be backported to 18.11.x series.