C4::Biblio::_koha_marc_update_bib_ids is adding biblionumber and biblioitemnumber to the MARC record. When it may still be needed when a bibliographic record is added/edited, why do we need it for GetMarcBiblio?
Good question. My guess might be coincidental record repair but that's not necessarily a good reason to keep it there.
Created attachment 127682 [details] [review] Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio This subroutine is ensuring that the biblionumber and biblioitemnumber will be part of the MARC record. We should not need that, unless there is something broken somewhere else. This line has been added by the following commit: commit 4e95e94727b09b33d2f6c597bdd218a59dcc3681 Bug 6789: biblios with many items can result in broken search results link """ To this end, it also moves the fix_biblio_ids portion of get_corrected_marc_record out of rebuild_zebra.pl, and makes it a part of GetMarcBiblio (right before EmbedItemsInMarcBiblio, so the 952s still come last). fix_biblio_ids is kept as a subroutine for the deletion portion of rebuild_zebra.pl, which still uses it. """ But it does not explain why it's better to have it in GetMarcBiblio. If we need it for the reindexation process, we shouldn't impact GetMarcBiblio which is used from several different places. We might then consider adding the fix_biblio_ids call to rebuild_zebra.pl, but I am failing to understand in which cases it could be useful.
This patch makes some tests fail, see t/db_dependent/Biblio.t When an update of biblio[item]number is made manually, the MARC record is (of course) not modified, and GetMarcRecord returns an "incorrect" record (ie. without the correct 999$c$d). Should we insist and remove this call? If so, should we provide a migration script and add a check to search_for_data_inconsistencies.pl? Which other situations this line was handling?
A check to search_for_data_inconsistencies.pl +1
Created attachment 129608 [details] [review] Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio This subroutine is ensuring that the biblionumber and biblioitemnumber will be part of the MARC record. We should not need that, unless there is something broken somewhere else. This line has been added by the following commit: commit 4e95e94727b09b33d2f6c597bdd218a59dcc3681 Bug 6789: biblios with many items can result in broken search results link """ To this end, it also moves the fix_biblio_ids portion of get_corrected_marc_record out of rebuild_zebra.pl, and makes it a part of GetMarcBiblio (right before EmbedItemsInMarcBiblio, so the 952s still come last). fix_biblio_ids is kept as a subroutine for the deletion portion of rebuild_zebra.pl, which still uses it. """ But it does not explain why it's better to have it in GetMarcBiblio. If we need it for the reindexation process, we shouldn't impact GetMarcBiblio which is used from several different places. We might then consider adding the fix_biblio_ids call to rebuild_zebra.pl, but I am failing to understand in which cases it could be useful.
Created attachment 129609 [details] [review] Bug 29486: Fix tests
Created attachment 129610 [details] [review] Bug 29486: Add check in search_for_data_inconsistencies.pl If the MARC record does not contain the correct biblionumber of biblioitemnumber, the script will display the following warning: == Bibliographic records have MARCXML without biblionumber or biblioitemnumber == * Biblionumber 4242 has '1' in 999$c * Biblionumber 4242 has biblioitemnumber '4242' but should be '1' in 999$d => The bibliographic records must have the biblionumber and biblioitemnumber in MARCXML
Note that we already have a script to fix the records: misc/batchRepairMissingBiblionumbers.pl
- my $newincbiblioitemnumber=$biblioitemnumber+1; - $dbh->do("UPDATE biblioitems SET biblioitemnumber = ? WHERE biblionumber = ?;", undef, $newincbiblioitemnumber, $biblionumber ); my $updatedrecord = GetMarcBiblio({ Name of $updatedrecord might be a bit confusing now ?
Created attachment 131861 [details] [review] Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio This subroutine is ensuring that the biblionumber and biblioitemnumber will be part of the MARC record. We should not need that, unless there is something broken somewhere else. This line has been added by the following commit: commit 4e95e94727b09b33d2f6c597bdd218a59dcc3681 Bug 6789: biblios with many items can result in broken search results link """ To this end, it also moves the fix_biblio_ids portion of get_corrected_marc_record out of rebuild_zebra.pl, and makes it a part of GetMarcBiblio (right before EmbedItemsInMarcBiblio, so the 952s still come last). fix_biblio_ids is kept as a subroutine for the deletion portion of rebuild_zebra.pl, which still uses it. """ But it does not explain why it's better to have it in GetMarcBiblio. If we need it for the reindexation process, we shouldn't impact GetMarcBiblio which is used from several different places. We might then consider adding the fix_biblio_ids call to rebuild_zebra.pl, but I am failing to understand in which cases it could be useful. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 131862 [details] [review] Bug 29486: Fix tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 131863 [details] [review] Bug 29486: Add check in search_for_data_inconsistencies.pl If the MARC record does not contain the correct biblionumber of biblioitemnumber, the script will display the following warning: == Bibliographic records have MARCXML without biblionumber or biblioitemnumber == * Biblionumber 4242 has '1' in 999$c * Biblionumber 4242 has biblioitemnumber '4242' but should be '1' in 999$d => The bibliographic records must have the biblionumber and biblioitemnumber in MARCXML Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 131865 [details] [review] Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio This subroutine is ensuring that the biblionumber and biblioitemnumber will be part of the MARC record. We should not need that, unless there is something broken somewhere else. This line has been added by the following commit: commit 4e95e94727b09b33d2f6c597bdd218a59dcc3681 Bug 6789: biblios with many items can result in broken search results link """ To this end, it also moves the fix_biblio_ids portion of get_corrected_marc_record out of rebuild_zebra.pl, and makes it a part of GetMarcBiblio (right before EmbedItemsInMarcBiblio, so the 952s still come last). fix_biblio_ids is kept as a subroutine for the deletion portion of rebuild_zebra.pl, which still uses it. """ But it does not explain why it's better to have it in GetMarcBiblio. If we need it for the reindexation process, we shouldn't impact GetMarcBiblio which is used from several different places. We might then consider adding the fix_biblio_ids call to rebuild_zebra.pl, but I am failing to understand in which cases it could be useful. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131866 [details] [review] Bug 29486: Fix tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131867 [details] [review] Bug 29486: Add check in search_for_data_inconsistencies.pl If the MARC record does not contain the correct biblionumber of biblioitemnumber, the script will display the following warning: == Bibliographic records have MARCXML without biblionumber or biblioitemnumber == * Biblionumber 4242 has '1' in 999$c * Biblionumber 4242 has biblioitemnumber '4242' but should be '1' in 999$d => The bibliographic records must have the biblionumber and biblioitemnumber in MARCXML Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This makes a lot of sense and works as expected, no regressions found, tests passing and QA script happy Passing QA
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to master for 21.11. Nice work everyone, thanks!