Bug 31889 - Insert 952 tags in correct order when embedding items
Summary: Insert 952 tags in correct order when embedding items
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-10-20 13:03 UTC by Marcel de Rooy
Modified: 2023-06-08 22:28 UTC (History)
1 user (show)

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


Attachments
Bug 31889: Remove exported ghost routine from Biblio module (739 bytes, patch)
2022-10-20 14:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31889: Add unit test to show problem (1.89 KB, patch)
2022-10-20 14:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31889: Replace append_fields in _embed_items (965 bytes, patch)
2022-10-20 14:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31889: Remove exported ghost routine from Biblio module (748 bytes, patch)
2022-10-20 18:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 31889: Add unit test to show problem (1.90 KB, patch)
2022-10-20 18:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 31889: Replace append_fields in _embed_items (974 bytes, patch)
2022-10-20 18:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 31889: Remove exported ghost routine from Biblio module (806 bytes, patch)
2022-11-02 19:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31889: Add unit test to show problem (1.95 KB, patch)
2022-11-02 19:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31889: Replace append_fields in _embed_items (1.01 KB, patch)
2022-11-02 19:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31889: (follow-up) Correct item order (1.23 KB, patch)
2022-11-07 09:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31889: (follow-up) Add test for explicit warning (1.80 KB, patch)
2022-11-07 11:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-10-20 13:03:49 UTC
I would like to see 942 952 999 instead of 942 999 952.
Tiny fix.
Comment 1 Marcel de Rooy 2022-10-20 14:35:27 UTC
Created attachment 142254 [details] [review]
Bug 31889: Remove exported ghost routine from Biblio module

It has been moved to a Koha object.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2022-10-20 14:35:31 UTC
Created attachment 142255 [details] [review]
Bug 31889: Add unit test to show problem

Test plan:
Run t/db_dependent/Koha/Biblio/Metadata.t
Fails with:
  not ok 10 - 951-952s-953 in 245,100,942,951,953,999,952,952,952,952,952,952,952,952

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2022-10-20 14:35:35 UTC
Created attachment 142256 [details] [review]
Bug 31889: Replace append_fields in _embed_items

Test plan:
Run t/db_dependent/Koha/Biblio/Metadata.t again. Should pass.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 David Nind 2022-10-20 18:19:34 UTC
Created attachment 142273 [details] [review]
Bug 31889: Remove exported ghost routine from Biblio module

It has been moved to a Koha object.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2022-10-20 18:19:38 UTC
Created attachment 142274 [details] [review]
Bug 31889: Add unit test to show problem

Test plan:
Run t/db_dependent/Koha/Biblio/Metadata.t
Fails with:
  not ok 10 - 951-952s-953 in 245,100,942,951,953,999,952,952,952,952,952,952,952,952

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2022-10-20 18:19:43 UTC
Created attachment 142275 [details] [review]
Bug 31889: Replace append_fields in _embed_items

Test plan:
Run t/db_dependent/Koha/Biblio/Metadata.t again. Should pass.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Marcel de Rooy 2022-10-21 06:07:01 UTC
(In reply to David Nind from comment #6)
> Signed-off-by: David Nind <david@davidnind.com>

Thx David for your fast signoff.
Comment 8 Katrin Fischer 2022-11-02 19:53:01 UTC
Created attachment 143035 [details] [review]
Bug 31889: Remove exported ghost routine from Biblio module

It has been moved to a Koha object.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2022-11-02 19:53:07 UTC
Created attachment 143036 [details] [review]
Bug 31889: Add unit test to show problem

Test plan:
Run t/db_dependent/Koha/Biblio/Metadata.t
Fails with:
  not ok 10 - 951-952s-953 in 245,100,942,951,953,999,952,952,952,952,952,952,952,952

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2022-11-02 19:53:12 UTC
Created attachment 143037 [details] [review]
Bug 31889: Replace append_fields in _embed_items

Test plan:
Run t/db_dependent/Koha/Biblio/Metadata.t again. Should pass.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Tomás Cohen Arazi 2022-11-04 22:25:40 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 12 Tomás Cohen Arazi 2022-11-04 23:29:05 UTC
Marcel, this broke some tests because of the order in which items are added by the exporter:

https://jenkins.koha-community.org/job/Koha_Master_D11_My8/1082/

In my opinion it looks suspicios as items should be introduced ordered by id.
I'm not too worried because we actually plan to remove the use of this method (_embed_items) for good.

But please, fix the failing tests for now.
Comment 13 Marcel de Rooy 2022-11-07 09:49:10 UTC
Created attachment 143348 [details] [review]
Bug 31889: (follow-up) Correct item order

MARC::Record::insert_fields_ordered reverses the order of fields
with the same tag. So we need to reverse the input order here.

This made Exporter/Record.t fail.

Test plan:
Run t/db_dependent/Koha/Biblio/Metadata.t
Run t/db_dependent/Exporter/Record.t (should pass now)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2022-11-07 09:57:44 UTC
@RM: Last patch only ;)
Comment 15 Tomás Cohen Arazi 2022-11-07 10:52:27 UTC
(In reply to Marcel de Rooy from comment #14)
> @RM: Last patch only ;)

I'm not sure why we should be sorting this fields at all. And we are now doing even more math/CPU use.

Generally, I think using MARC::Record->insert_grouped_field is good enough. As I said before, this method needs to go away soon in favor of the RecordProcessor, which right now is appending things, so moving the discussion there.

Thanks for the patch fixing the tests.
Comment 16 Tomás Cohen Arazi 2022-11-07 10:54:37 UTC
Follow-up pushed to master.
Comment 17 Tomás Cohen Arazi 2022-11-07 11:31:42 UTC
Created attachment 143350 [details] [review]
Bug 31889: (follow-up) Add test for explicit warning

On bug 29697, an explicit call to 'warn' was added, but no tests were
added for that behavior.

This patch adds that.

To test:

1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Biblio/Metadata.t
=> FAIL: There's a warn, no tests for it :-(
2. Apply this patch
3. Repeat 1
=> SUCCESS: A new test was added, no more warns printed
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>