|
Description
Marcel de Rooy
2022-10-20 13:03:49 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> 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> 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> 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> 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> 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> (In reply to David Nind from comment #6) > Signed-off-by: David Nind <david@davidnind.com> Thx David for your fast signoff. 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> 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> 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> Pushed to master for 22.11. Nice work everyone, thanks! 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. 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> @RM: Last patch only ;) (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. Follow-up pushed to master. 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> |