Bug 31889

Summary: Insert 952 tags in correct order when embedding items
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: normal    
Priority: P5 - low CC: tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224
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
Bug 31889: Add unit test to show problem
Bug 31889: Replace append_fields in _embed_items
Bug 31889: Remove exported ghost routine from Biblio module
Bug 31889: Add unit test to show problem
Bug 31889: Replace append_fields in _embed_items
Bug 31889: Remove exported ghost routine from Biblio module
Bug 31889: Add unit test to show problem
Bug 31889: Replace append_fields in _embed_items
Bug 31889: (follow-up) Correct item order
Bug 31889: (follow-up) Add test for explicit warning

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>