Bug 29486 - _koha_marc_update_bib_ids no longer needed for GetMarcBiblio
Summary: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 29307 29697
  Show dependency treegraph
 
Reported: 2021-11-15 15:05 UTC by Jonathan Druart
Modified: 2022-11-24 13:59 UTC (History)
8 users (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.05.00


Attachments
Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio (2.45 KB, patch)
2021-11-16 08:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio (2.45 KB, patch)
2022-01-19 13:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29486: Fix tests (1.67 KB, patch)
2022-01-19 13:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29486: Add check in search_for_data_inconsistencies.pl (3.90 KB, patch)
2022-01-19 13:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio (2.54 KB, patch)
2022-03-18 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29486: Fix tests (1.77 KB, patch)
2022-03-18 09:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29486: Add check in search_for_data_inconsistencies.pl (4.00 KB, patch)
2022-03-18 09:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29486: _koha_marc_update_bib_ids no longer needed for GetMarcBiblio (2.57 KB, patch)
2022-03-18 10:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29486: Fix tests (1.79 KB, patch)
2022-03-18 10:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29486: Add check in search_for_data_inconsistencies.pl (4.02 KB, patch)
2022-03-18 10:14 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 🦄