Bug 29486

Summary: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: dcook, fridolin.somers, joonas.kylmala, m.de.rooy, magnus, martin.renvoize, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32332
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.23
Bug Depends on:    
Bug Blocks: 29307, 29697, 33603    
Attachments: Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio
Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio
Bug 29486: Fix tests
Bug 29486: Add check in search_for_data_inconsistencies.pl
Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio
Bug 29486: Fix tests
Bug 29486: Add check in search_for_data_inconsistencies.pl
Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio
Bug 29486: Fix tests
Bug 29486: Add check in search_for_data_inconsistencies.pl

Description Jonathan Druart 2021-11-15 15:05:38 UTC
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?
Comment 1 David Cook 2021-11-15 23:08:45 UTC
Good question. My guess might be coincidental record repair but that's not necessarily a good reason to keep it there.
Comment 2 Jonathan Druart 2021-11-16 08:52:23 UTC
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.
Comment 3 Jonathan Druart 2021-11-16 08:56:33 UTC
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?
Comment 4 Fridolin Somers 2021-12-02 21:38:35 UTC
A check to search_for_data_inconsistencies.pl +1
Comment 5 Jonathan Druart 2022-01-19 13:41:15 UTC
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.
Comment 6 Jonathan Druart 2022-01-19 13:41:19 UTC
Created attachment 129609 [details] [review]
Bug 29486: Fix tests
Comment 7 Jonathan Druart 2022-01-19 13:41:23 UTC
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
Comment 8 Jonathan Druart 2022-01-21 09:44:36 UTC
Note that we already have a script to fix the records: misc/batchRepairMissingBiblionumbers.pl
Comment 9 Marcel de Rooy 2022-03-18 09:23:52 UTC
-    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 ?
Comment 10 Marcel de Rooy 2022-03-18 09:30:58 UTC
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>
Comment 11 Marcel de Rooy 2022-03-18 09:31:02 UTC
Created attachment 131862 [details] [review]
Bug 29486: Fix tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2022-03-18 09:31:06 UTC
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>
Comment 13 Martin Renvoize 2022-03-18 10:14:02 UTC
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>
Comment 14 Martin Renvoize 2022-03-18 10:14:06 UTC
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>
Comment 15 Martin Renvoize 2022-03-18 10:14:10 UTC
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>
Comment 16 Martin Renvoize 2022-03-18 10:14:45 UTC
This makes a lot of sense and works as expected, no regressions found, tests passing and QA script happy

Passing QA
Comment 17 Fridolin Somers 2022-03-22 20:18:49 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 18 Tomás Cohen Arazi 2023-06-12 15:36:21 UTC
Pushed to master for 21.11.

Nice work everyone, thanks!