Bug 34739 - Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading
Summary: Linked biblios should not be merged (updated) when changes to an authority do...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Janusz Kaczmarek
QA Contact: Thomas Klausner
URL:
Keywords:
: 38742 (view as bug list)
Depends on:
Blocks: 39700
  Show dependency treegraph
 
Reported: 2023-09-07 17:04 UTC by Nick Clemens (kidclamp)
Modified: 2025-05-16 15:30 UTC (History)
6 users (show)

See Also:
GIT URL:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 34739: Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading (5.45 KB, patch)
2024-12-18 18:41 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 34739: Unit tests (5.22 KB, patch)
2024-12-18 18:41 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 34739: Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading (5.50 KB, patch)
2024-12-18 19:04 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 34739: Unit tests (5.27 KB, patch)
2024-12-18 19:04 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 34739: Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading (5.52 KB, patch)
2025-03-31 11:45 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 34739: Unit tests (5.14 KB, patch)
2025-03-31 11:45 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 34739: Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading (5.58 KB, patch)
2025-03-31 20:08 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 34739: Unit tests (5.18 KB, patch)
2025-04-01 13:31 UTC, Thomas Klausner
Details | Diff | Splinter Review
Bug 34739: Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading (5.62 KB, patch)
2025-04-01 13:31 UTC, Thomas Klausner
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-09-07 17:04:54 UTC
If an authority record is modified in any way, we trigger a merge to update all biblios using the authority.

This also happens if we merge a duplciate authority into one that is in use, all biblios already attached to the correct authority are updated.

If the heading or controlled indicators or thesaurus have changed, this makes sense, but if not, we are triggering many biblio modifications and reindexes that are not needed.

We also need to consider the case where a See-From is is added and IncludeSeeFromInSearches is enabled
Comment 1 Marcel de Rooy 2023-09-11 06:35:31 UTC
There is a development about adding URIs from 024 in auth to bib record when merging. A Biblibre report, forgot the number. This would be an exception.
Comment 2 Phil Ringnalda 2023-09-12 20:40:19 UTC
Bug 22972
Comment 3 Janusz Kaczmarek 2024-12-18 18:41:04 UTC
Created attachment 175665 [details] [review]
Bug 34739: Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading

Currently, the function C4::AuthoritiesMarc::merge calls ModBiblio for
all bibliographic records linked to the modified authority record.  It
could seem a just behavior but we know that this could lead to
performance problems.  This is why AuthorityMergeLimit
systempreference with merge_authorities.pl was introduced (earlier:
dontmerge).

Although this partially solves the performance problems, for
moderately large and large catalogs (500+ K) and authority records
linked with a large number of bibliographic records, performance
problems still occur, even with overnight processing with
merge_authorities.pl.  This problem is particularly apparent when
updating authority records from central databases, whose modifications
are beyond our control.

It turns out, however, that many such modifications to authority
records involve fields other than the heading field (1XX in MARC 21),
so these updates in fact do not modify bibliographic records.
However, with the current merge function, every touch on an
authority record forces a call to the ModBiblio function.  The
number of relatively expensive ModBiblio calls could be
reduced by checking whether there has been an actual change in
the field associated with the authority record.

However, with IncludeSeeFromInSearches or IncludeSeeAlsoFromInSearches
preferences enabled, the record should be refreshed in the search
engine just in case (should the 4XX, 5XX fields have changed, which we
don't know at this point).

Test plan:
==========
1. Make sure to have CataloguingLog set to Log and
   IncludeSeeFromInSearches set to Include.
2. Have a bibliographic record with an authority record linked. Open
   the authority record in a separate tab, open it in the editor and
   save, without modifying it. Repeat this 2-3 times. In the
   bibliographic record tab open the Modification log and see a number of
   entries - for each “empty” modification of the linked authority
   record.
3. Apply the patch ; restart_all.
4. Repat p. 2. Note that there are no new entries in the log.
5. Edit the authority record again, this time putting in one of the
   proper 4XX $a subfield an uncommon word (but do not  touch the 1XX
   field). Save the authority record. In the bibliographic record
   tab control that there are no new entries in the modification log.
   Make a search for the word put in 4XX field. You should get the
   bibliographic record(s) linked to the edited authority record, even
   though the bibliographic record itself has not been modified.
6. Edit the authority record again, this time changing the 1XX field.
   Save the authority record. In the bibliographic record tab control
   that there are is new entry in the modification log.

Sponsored-by: Ignatianum University in Cracow
Comment 4 Janusz Kaczmarek 2024-12-18 18:41:07 UTC
Created attachment 175666 [details] [review]
Bug 34739: Unit tests

BTW, there is a tiny adjustment to one of the previous subtests to
fit to the modified merge behaviour.
Comment 5 Janusz Kaczmarek 2024-12-18 18:42:26 UTC
*** Bug 38742 has been marked as a duplicate of this bug. ***
Comment 6 Roman Dolny 2024-12-18 19:04:46 UTC
Created attachment 175667 [details] [review]
Bug 34739: Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading

Currently, the function C4::AuthoritiesMarc::merge calls ModBiblio for
all bibliographic records linked to the modified authority record.  It
could seem a just behavior but we know that this could lead to
performance problems.  This is why AuthorityMergeLimit
systempreference with merge_authorities.pl was introduced (earlier:
dontmerge).

Although this partially solves the performance problems, for
moderately large and large catalogs (500+ K) and authority records
linked with a large number of bibliographic records, performance
problems still occur, even with overnight processing with
merge_authorities.pl.  This problem is particularly apparent when
updating authority records from central databases, whose modifications
are beyond our control.

It turns out, however, that many such modifications to authority
records involve fields other than the heading field (1XX in MARC 21),
so these updates in fact do not modify bibliographic records.
However, with the current merge function, every touch on an
authority record forces a call to the ModBiblio function.  The
number of relatively expensive ModBiblio calls could be
reduced by checking whether there has been an actual change in
the field associated with the authority record.

However, with IncludeSeeFromInSearches or IncludeSeeAlsoFromInSearches
preferences enabled, the record should be refreshed in the search
engine just in case (should the 4XX, 5XX fields have changed, which we
don't know at this point).

Test plan:
==========
1. Make sure to have CataloguingLog set to Log and
   IncludeSeeFromInSearches set to Include.
2. Have a bibliographic record with an authority record linked. Open
   the authority record in a separate tab, open it in the editor and
   save, without modifying it. Repeat this 2-3 times. In the
   bibliographic record tab open the Modification log and see a number of
   entries - for each “empty” modification of the linked authority
   record.
3. Apply the patch ; restart_all.
4. Repat p. 2. Note that there are no new entries in the log.
5. Edit the authority record again, this time putting in one of the
   proper 4XX $a subfield an uncommon word (but do not  touch the 1XX
   field). Save the authority record. In the bibliographic record
   tab control that there are no new entries in the modification log.
   Make a search for the word put in 4XX field. You should get the
   bibliographic record(s) linked to the edited authority record, even
   though the bibliographic record itself has not been modified.
6. Edit the authority record again, this time changing the 1XX field.
   Save the authority record. In the bibliographic record tab control
   that there are is new entry in the modification log.

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 7 Roman Dolny 2024-12-18 19:04:49 UTC
Created attachment 175668 [details] [review]
Bug 34739: Unit tests

BTW, there is a tiny adjustment to one of the previous subtests to
fit to the modified merge behaviour.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 8 Marcel de Rooy 2025-01-31 10:31:48 UTC
# Subtest: ModBiblio calls from merge
    1..4
    not ok 1 - real modification of bibliographic record, ModBiblio called
    #   Failed test 'real modification of bibliographic record, ModBiblio called'
    #   at t/db_dependent/Authority/Merge.t line 612.
    # didn't find a warning
    # expected to find warning: ModBiblio called
    not ok 2 - no real modification of bibliographic record, index_records called
    #   Failed test 'no real modification of bibliographic record, index_records called'
    #   at t/db_dependent/Authority/Merge.t line 626.
    # didn't find a warning
    # expected to find warning: index_records called
    ok 3 - no real modification of bibliographic record, index_records not called
    not ok 4 - Only one bibliographic record updated while merging two authorities
    #   Failed test 'Only one bibliographic record updated while merging two authorities'
    #   at t/db_dependent/Authority/Merge.t line 655.
    #          got: '0'
    #     expected: '1'
    # Looks like you failed 3 tests of 4.
not ok 13 - ModBiblio calls from merge
Comment 9 Janusz Kaczmarek 2025-01-31 17:00:08 UTC
(In reply to Marcel de Rooy from comment #8)
> not ok 13 - ModBiblio calls from merge

This is somehow strange... Could you provide some more detailed information or repeat the test, please?


I have tried anew on two separate fresh ktd installation -- everything looks clear:

kohadev-koha@kohadevbox:koha(bug_34739)$ qa
testing 2 commit(s) (applied to da08fc5 '1a Bug 31270: (follow-up) Fix descrip')

Processing files before patches
|========================>| 2 / 2 (100.00%)
Processing files after patches
|========================>| 2 / 2 (100.00%)

 OK     C4/AuthoritiesMarc.pm

 OK     t/db_dependent/Authority/Merge.t


Processing additional checks OK!

Running tests (1)
        * Proving /kohadevbox/koha/t/db_dependent/Authority/Merge.t OK!
Comment 10 Marcel de Rooy 2025-02-07 07:38:09 UTC
Running tests (1)
        * Proving /usr/share/koha/t/db_dependent/Authority/Merge.t KO!

    #   Failed test 'real modification of bibliographic record, ModBiblio called'
    #   at /usr/share/koha/t/db_dependent/Authority/Merge.t line 612.
     # didn't find a warning
     # expected to find warning: ModBiblio called

    #   Failed test 'no real modification of bibliographic record, index_records called'
    #   at /usr/share/koha/t/db_dependent/Authority/Merge.t line 626.
     # didn't find a warning
     # expected to find warning: index_records called

    #   Failed test 'Only one bibliographic record updated while merging two authorities'
    #   at /usr/share/koha/t/db_dependent/Authority/Merge.t line 655.
     #          got: '0'
    #     expected: '1'
     # Looks like you failed 3 tests of 4.


NOTE: Without these patches, the same test passes with me.
Comment 11 Janusz Kaczmarek 2025-02-07 08:16:28 UTC
(In reply to Marcel de Rooy from comment #10)
> Running tests (1)
>         * Proving /usr/share/koha/t/db_dependent/Authority/Merge.t KO!

So it's strange. I don't know... I am helpless.

Maybe you could have a look at the test -- is ModBiblio properly mocked in the test snippet added to Merge.t?
Comment 12 Marcel de Rooy 2025-02-17 14:16:33 UTC
I now had this one:

# Subtest: Test merge A1 to modified A1, test strict mode
    1..12
    not ok 1 - Both records are updated now
    #   Failed test 'Both records are updated now'
    #   at t/db_dependent/Authority/Merge.t line 174.
    #          got: '1'
    #     expected: '2'

And that is an existing test. Without your patch it passes.
Please have another look here.
Comment 13 Marcel de Rooy 2025-02-17 14:17:24 UTC
And the following ones are new (iiuc):

# Subtest: ModBiblio calls from merge
    1..4
    not ok 1 - real modification of bibliographic record, ModBiblio called
    #   Failed test 'real modification of bibliographic record, ModBiblio called'
    #   at t/db_dependent/Authority/Merge.t line 716.
    # didn't find a warning
    # expected to find warning: ModBiblio called
    not ok 2 - no real modification of bibliographic record, index_records called
    #   Failed test 'no real modification of bibliographic record, index_records called'
    #   at t/db_dependent/Authority/Merge.t line 730.
    # didn't find a warning
    # expected to find warning: index_records called
    ok 3 - no real modification of bibliographic record, index_records not called
    not ok 4 - Only one bibliographic record updated while merging two authorities
    #   Failed test 'Only one bibliographic record updated while merging two authorities'
    #   at t/db_dependent/Authority/Merge.t line 759.
    #          got: '0'
    #     expected: '1'
    # Looks like you failed 3 tests of 4.

That could indeed be a bad mock or so ?
Comment 14 Marcel de Rooy 2025-02-17 14:26:46 UTC
+    my $biblio_module = Test::MockModule->new('C4::AuthoritiesMarc');

This is confusing btw :)
Comment 15 Janusz Kaczmarek 2025-03-31 11:32:39 UTC
(In reply to Marcel de Rooy from comment #14)
> +    my $biblio_module = Test::MockModule->new('C4::AuthoritiesMarc');
> 
> This is confusing btw :)

Maybe a little... This snippet is taken from one of the previous tests (cf. commit 3954808ac5052a53d5fa40502a4f7bd991be80ab).
Comment 16 Janusz Kaczmarek 2025-03-31 11:45:39 UTC
Created attachment 179979 [details] [review]
Bug 34739: Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading

Currently, the function C4::AuthoritiesMarc::merge calls ModBiblio for
all bibliographic records linked to the modified authority record.  It
could seem a just behavior but we know that this could lead to
performance problems.  This is why AuthorityMergeLimit
systempreference with merge_authorities.pl was introduced (earlier:
dontmerge).

Although this partially solves the performance problems, for
moderately large and large catalogs (500+ K) and authority records
linked with a large number of bibliographic records, performance
problems still occur, even with overnight processing with
merge_authorities.pl.  This problem is particularly apparent when
updating authority records from central databases, whose modifications
are beyond our control.

It turns out, however, that many such modifications to authority
records involve fields other than the heading field (1XX in MARC 21),
so these updates in fact do not modify bibliographic records.
However, with the current merge function, every touch on an
authority record forces a call to the ModBiblio function.  The
number of relatively expensive ModBiblio calls could be
reduced by checking whether there has been an actual change in
the field associated with the authority record.

However, with IncludeSeeFromInSearches or IncludeSeeAlsoFromInSearches
preferences enabled, the record should be refreshed in the search
engine just in case (should the 4XX, 5XX fields have changed, which we
don't know at this point).

Test plan:
==========
1. Make sure to have CataloguingLog set to Log and
   IncludeSeeFromInSearches set to Include.
2. Have a bibliographic record with an authority record linked. Open
   the authority record in a separate tab, open it in the editor and
   save, without modifying it. Repeat this 2-3 times. In the
   bibliographic record tab open the Modification log and see a number of
   entries - for each “empty” modification of the linked authority
   record.
3. Apply the patch ; restart_all.
4. Repat p. 2. Note that there are no new entries in the log.
5. Edit the authority record again, this time putting in one of the
   proper 4XX $a subfield an uncommon word (but do not  touch the 1XX
   field). Save the authority record. In the bibliographic record
   tab control that there are no new entries in the modification log.
   Make a search for the word put in 4XX field. You should get the
   bibliographic record(s) linked to the edited authority record, even
   though the bibliographic record itself has not been modified.
6. Edit the authority record again, this time changing the 1XX field.
   Save the authority record. In the bibliographic record tab control
   that there are is new entry in the modification log.

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 17 Janusz Kaczmarek 2025-03-31 11:45:42 UTC
Created attachment 179980 [details] [review]
Bug 34739: Unit tests

BTW, there is a tiny adjustment to one of the previous subtests to
fit to the modified merge behaviour.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 18 Marcel de Rooy 2025-03-31 11:48:37 UTC
(In reply to Marcel de Rooy from comment #14)
> +    my $biblio_module = Test::MockModule->new('C4::AuthoritiesMarc');
> 
> This is confusing btw :)

Loading authorities and calling it biblio, I mean specifically :)
Comment 19 Thomas Klausner 2025-03-31 13:52:02 UTC
I'm not sure how expensive calling C4::Context->preference('IncludeSeeFromInSearches') is, but probably it would be better fetch both sysprefs once, put them into a variable and inside the loop just access those vars:

so instead of

        if (
              !$update
            && $reindex_if_needed
            && (   C4::Context->preference('IncludeSeeFromInSearches')
                || C4::Context->preference('IncludeSeeAlsoFromInSearches') )
            ) 

do

    my $syspref_include_see_from = C4::Context->preference('IncludeSeeFromInSearches') || C4::Context->preference('IncludeSeeAlsoFromInSearches') || 0;
 
    while ( my $biblio = $biblios->next ) {
        ...

        if (
              !$update
            && $reindex_if_needed
            && $syspref_include_see
            )



Also maybe $update should be renamed to $update_biblio?
Comment 20 Janusz Kaczmarek 2025-03-31 20:08:42 UTC
Created attachment 180083 [details] [review]
Bug 34739: Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading

Currently, the function C4::AuthoritiesMarc::merge calls ModBiblio for
all bibliographic records linked to the modified authority record.  It
could seem a just behavior but we know that this could lead to
performance problems.  This is why AuthorityMergeLimit
systempreference with merge_authorities.pl was introduced (earlier:
dontmerge).

Although this partially solves the performance problems, for
moderately large and large catalogs (500+ K) and authority records
linked with a large number of bibliographic records, performance
problems still occur, even with overnight processing with
merge_authorities.pl.  This problem is particularly apparent when
updating authority records from central databases, whose modifications
are beyond our control.

It turns out, however, that many such modifications to authority
records involve fields other than the heading field (1XX in MARC 21),
so these updates in fact do not modify bibliographic records.
However, with the current merge function, every touch on an
authority record forces a call to the ModBiblio function.  The
number of relatively expensive ModBiblio calls could be
reduced by checking whether there has been an actual change in
the field associated with the authority record.

However, with IncludeSeeFromInSearches or IncludeSeeAlsoFromInSearches
preferences enabled, the record should be refreshed in the search
engine just in case (should the 4XX, 5XX fields have changed, which we
don't know at this point).

Test plan:
==========
1. Make sure to have CataloguingLog set to Log and
   IncludeSeeFromInSearches set to Include.
2. Have a bibliographic record with an authority record linked. Open
   the authority record in a separate tab, open it in the editor and
   save, without modifying it. Repeat this 2-3 times. In the
   bibliographic record tab open the Modification log and see a number of
   entries - for each “empty” modification of the linked authority
   record.
3. Apply the patch ; restart_all.
4. Repat p. 2. Note that there are no new entries in the log.
5. Edit the authority record again, this time putting in one of the
   proper 4XX $a subfield an uncommon word (but do not  touch the 1XX
   field). Save the authority record. In the bibliographic record
   tab control that there are no new entries in the modification log.
   Make a search for the word put in 4XX field. You should get the
   bibliographic record(s) linked to the edited authority record, even
   though the bibliographic record itself has not been modified.
6. Edit the authority record again, this time changing the 1XX field.
   Save the authority record. In the bibliographic record tab control
   that there are is new entry in the modification log.

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 21 Janusz Kaczmarek 2025-03-31 20:13:07 UTC
(In reply to Thomas Klausner from comment #19)
> I'm not sure how expensive calling
> C4::Context->preference('IncludeSeeFromInSearches') is, but probably it
> would be better fetch both sysprefs once, put them into a variable and
> inside the loop just access those vars:

OK, corrected according to your suggestion. I wanted to make the checking conditional but at the end it should be far better to check in once and remember.

> Also maybe $update should be renamed to $update_biblio?

This comes from the old code -- I would prefer not to touch the existing code if not necessary.
Comment 22 Thomas Klausner 2025-04-01 13:31:37 UTC
Created attachment 180187 [details] [review]
Bug 34739: Unit tests

BTW, there is a tiny adjustment to one of the previous subtests to
fit to the modified merge behaviour.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Thomas Klausner <domm@plix.at>
Comment 23 Thomas Klausner 2025-04-01 13:31:40 UTC
Created attachment 180188 [details] [review]
Bug 34739: Linked biblios should not be merged (updated) when changes to an authority don't change the authorized heading

Currently, the function C4::AuthoritiesMarc::merge calls ModBiblio for
all bibliographic records linked to the modified authority record.  It
could seem a just behavior but we know that this could lead to
performance problems.  This is why AuthorityMergeLimit
systempreference with merge_authorities.pl was introduced (earlier:
dontmerge).

Although this partially solves the performance problems, for
moderately large and large catalogs (500+ K) and authority records
linked with a large number of bibliographic records, performance
problems still occur, even with overnight processing with
merge_authorities.pl.  This problem is particularly apparent when
updating authority records from central databases, whose modifications
are beyond our control.

It turns out, however, that many such modifications to authority
records involve fields other than the heading field (1XX in MARC 21),
so these updates in fact do not modify bibliographic records.
However, with the current merge function, every touch on an
authority record forces a call to the ModBiblio function.  The
number of relatively expensive ModBiblio calls could be
reduced by checking whether there has been an actual change in
the field associated with the authority record.

However, with IncludeSeeFromInSearches or IncludeSeeAlsoFromInSearches
preferences enabled, the record should be refreshed in the search
engine just in case (should the 4XX, 5XX fields have changed, which we
don't know at this point).

Test plan:
==========
1. Make sure to have CataloguingLog set to Log and
   IncludeSeeFromInSearches set to Include.
2. Have a bibliographic record with an authority record linked. Open
   the authority record in a separate tab, open it in the editor and
   save, without modifying it. Repeat this 2-3 times. In the
   bibliographic record tab open the Modification log and see a number of
   entries - for each “empty” modification of the linked authority
   record.
3. Apply the patch ; restart_all.
4. Repat p. 2. Note that there are no new entries in the log.
5. Edit the authority record again, this time putting in one of the
   proper 4XX $a subfield an uncommon word (but do not  touch the 1XX
   field). Save the authority record. In the bibliographic record
   tab control that there are no new entries in the modification log.
   Make a search for the word put in 4XX field. You should get the
   bibliographic record(s) linked to the edited authority record, even
   though the bibliographic record itself has not been modified.
6. Edit the authority record again, this time changing the 1XX field.
   Save the authority record. In the bibliographic record tab control
   that there are is new entry in the modification log.

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Signed-off-by: Thomas Klausner <domm@plix.at>
Comment 24 Thomas Klausner 2025-04-01 13:34:16 UTC
Work! Very nice, this will remove a lot of unnecessary updates on biblios.
Comment 25 Katrin Fischer 2025-04-11 07:55:41 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 26 Katrin Fischer 2025-04-11 09:52:28 UTC
We have some broken tests, please check ASAP (lots waiting to be pushed):

Subtest: ModBiblio calls from merge
    1..5
    not ok 1 - real modification of bibliographic record, ModBiblio called
    not ok 2 - no real modification of bibliographic record, index_records called
    ok 3 - no real modification of bibliographic record, index_records not called
    not ok 4 - real modification of bibliographic record, ModBiblio called
    not ok 5 - only one bibliographic record updated while merging two authorities
Comment 27 Katrin Fischer 2025-04-11 09:53:08 UTC
Note: I suspect it comes from this patch set as it relates to merges, but please verify.
Comment 28 Thomas Klausner 2025-04-18 16:23:39 UTC
when I run prove -lv t/db_dependent/Biblio_holdsqueue.t or t/db_dependent/Biblio.t and then t/db_dependent/Authority/Merge.t I get the failures. If then do reset_all, Merge.t works again. But I haven't had time to take a closer look why this happens
Comment 29 Jonathan Druart 2025-04-22 12:15:30 UTC
If you run t/db_dependent/Biblio.t first, then Merge.t will fail.

I guess there are modifications made by Biblio.t to the MSS (marc subfield structure), either in DB or cached.

I have noticed this suspicious stuff:

698     $biblio_record->insert_fields_ordered(
699         MARC::Field->new( '609', '1', '1', a => 'Brown,', 'c' => 'Father', 9 => $authid ) );
700     my $biblionumber = AddBiblio( $biblio_record, '', { skip_record_index => 1 } );

if you dump $biblio_record->as_formatted you see the 609 field.

BUT, if you add the following line right after:
my $test = Koha::Biblios->find($biblionumber)->metadata->record
then dump $test->as_formatted

=> 609 is not there (??)
Comment 30 Marcel de Rooy 2025-04-22 12:22:30 UTC
(In reply to Jonathan Druart from comment #29)
> If you run t/db_dependent/Biblio.t first, then Merge.t will fail.
> 
> I guess there are modifications made by Biblio.t to the MSS (marc subfield
> structure), either in DB or cached.
> 
> I have noticed this suspicious stuff:
> 
> 698     $biblio_record->insert_fields_ordered(
> 699         MARC::Field->new( '609', '1', '1', a => 'Brown,', 'c' =>
> 'Father', 9 => $authid ) );
> 700     my $biblionumber = AddBiblio( $biblio_record, '', {
> skip_record_index => 1 } );
> 
> if you dump $biblio_record->as_formatted you see the 609 field.
> 
> BUT, if you add the following line right after:
> my $test = Koha::Biblios->find($biblionumber)->metadata->record
> then dump $test->as_formatted
> 
> => 609 is not there (??)

Sounds like a framework issue.
Comment 31 Thomas Klausner 2025-04-22 19:15:05 UTC
The new test case calls AddBiblio. And AddBiblio returns ($biblionumber, $biblioitemnumber). But the test calls it like this: my $biblionumber = AddBiblio;


AddBiblio returns a list, but the calling code only stores a single value, so it get's the last value, i.e. the biblioitemnumber.


Now, when running the test alone, the DB sequences for biblio and biblioitem are in sync and both return the same value so the test works (by accident)


But some tests add biblios with multiple items (or biblios without items), thus the sequences are no longer in sync and when the test tries to load a biblio via a biblioitemid, it finds nothing, which causes the fail


bad: my $biblionumber = AddBiblio( $biblio_record, '', { skip_record_index => 1 } );

ok-ish: my ($biblionumber) = AddBiblio( $biblio_record, '', { skip_record_index => 1 } );
Comment 32 Thomas Klausner 2025-04-22 19:42:42 UTC
See Bug 39700 for the fix
Comment 33 Janusz Kaczmarek 2025-04-23 09:14:58 UTC
My fault :( Thanks, Thomas!
Comment 34 Katrin Fischer 2025-05-16 15:30:20 UTC
Fixed on separate bug, thanks!