Bug 16018 - Merge.pl code cleanup
Summary: Merge.pl code cleanup
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 17908 18070
  Show dependency treegraph
 
Reported: 2016-03-09 13:27 UTC by Rémi Mayrand-Provencher
Modified: 2017-02-20 07:27 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug16018 - Skip ModAuthority sub in AuthoritiesMarc.pm to make merges work properly (1.50 KB, text/plain)
2016-03-09 16:51 UTC, Rémi Mayrand-Provencher
Details
Bug 16018 - Merges in AuthoritiesMarc do not work (1.55 KB, patch)
2017-01-03 18:39 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 16018: Merge.pl does not update biblios linked to the old authority (3.57 KB, patch)
2017-01-24 13:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16018: Add a TODO for need_merge_authorities in merge.pl (1.35 KB, patch)
2017-01-24 13:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16018: Merge.pl does not update biblios linked to the old authority (3.57 KB, patch)
2017-01-24 22:41 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 16018: Add a TODO for need_merge_authorities in merge.pl (1.35 KB, patch)
2017-01-24 22:47 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 16018: Add a TODO for need_merge_authorities in merge.pl (1.62 KB, patch)
2017-01-25 12:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16018: [Follow-up] Code cleaning (3.04 KB, patch)
2017-02-06 13:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16018: Merge.pl code cleanup (4.50 KB, patch)
2017-02-06 14:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16018: Merge.pl code cleanup (4.53 KB, patch)
2017-02-06 15:13 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Rémi Mayrand-Provencher 2016-03-09 13:27:26 UTC
As the summary says, merges are not modifying biblios properly.
Test plan to reproduce the bug:
1)Go to authority, search for anything
2)Click merge on two authorities of the same type that are used at least one time
3)Validate which subfields are corresponding to each authorities in the biblios that uses them
4)Do the whole merge process
5)Validate that the biblios are updated properly (which they are not according to my tests.
Comment 1 Rémi Mayrand-Provencher 2016-03-09 13:41:45 UTC
forgot to mention an important detail in test plan : "dontmerge" system preference needs to be set to "do" to be able to test this bug.
Comment 2 Julian Maurice 2016-03-09 13:49:45 UTC
Could you provide real examples of the data you used in your tests ?
As I said on the mailing list I was not able to reproduce the problem, so please be specific about what you expect and what happens.
Comment 3 Rémi Mayrand-Provencher 2016-03-09 14:06:32 UTC
Let's say I want to merge these two authorities:
1)Adieux à la reine (Film) (176802)
2)Les actuels (36112)

Both are of the same type. If I go look at any biblios using these authorities:

In subfield 640 of the biblio that uses first authority (176802):
1)    $a Titre uniforme Adieux à la reine (Film)
      $9 9 (RLIN)       176802

In subfield 440 of the biblio that uses second authority (36112):
2)    $a Titre uniforme Les Actuels.
      $9 9 (RLIN)       36112

Then, I proceed with the merge using 176802 as the reference and get this as a result for the biblios:

In subfield 640 for first biblio (176802):
1)    $a Titre uniforme Adieux à la reine (Film)
      $9 9 (RLIN)       176802

In subfield 440 for second biblio (36112):
2)    $a Titre uniforme Les Actuels.
      $9 9 (RLIN)       176802

As you can see, the authority number changes but not the corresponding field. the $a in the second biblio should have changed when $9 changed.
Comment 4 Rémi Mayrand-Provencher 2016-03-09 16:51:27 UTC Comment hidden (obsolete)
Comment 5 Julian Maurice 2016-03-10 08:45:48 UTC
Sorry, I didn't manage to reproduce the problem. At the end my biblios are updated correctly.
Can anyone else give a try ?

Regarding your patch, what makes you think the problem comes from ModAuthority ?
Comment 6 Rémi Mayrand-Provencher 2016-03-10 19:24:32 UTC
Tested it again because I was confused, and I realised that modAuthority is not the cause of the bug. Still, I can't always get the biblios to change correcly. Sometimes it works, sometimes it doesn't. I'd like it very much if someone could confirm that the bug happens on their end. Thank you!
Comment 7 Blou 2016-10-07 12:27:45 UTC
Another way of asking for testers, please.
Comment 8 M. Tompsett 2017-01-03 18:39:05 UTC
Created attachment 58578 [details] [review]
Bug 16018 - Merges in AuthoritiesMarc do not work

Skip ModAuthority sub in AuthoritiesMarc.pm to make merges
work properly.

Test plan:
1) Go to authority, search for anything
2) Click merge on two authorities of the same type that are
   used at least one time
3) Validate which subfields are corresponding to each
   authorities in the biblios that uses them
4) Do the whole merge process
5) Validate that the biblios are updated properly
Comment 9 M. Tompsett 2017-01-03 18:39:51 UTC
Comment on attachment 48889 [details]
Bug16018 - Skip ModAuthority sub in AuthoritiesMarc.pm to make merges work properly

This wasn't a proper patch format which was easy to test. I, Mark Tompsett, fixed that.
Comment 10 Marcel de Rooy 2017-01-14 12:33:00 UTC
Interesting patch! I will add it to the omnibus bug 17908 in order to get all authority merge reports further.
I temporarily change the status of this patch to Blocked, but will be coming back to it (hopefully) shortly.
You refer to a new pref overwriteSubfieldsOnMerge that does not yet exist and comes in several flavors: AuthCleanBiblios or AuthorityMergeMode etc.
If we get the pref in and some changes in merge, this should follow.
Have some patience. Thanks.
Comment 11 Marcel de Rooy 2017-01-23 10:54:51 UTC
Planning to get back here this week
Comment 12 Marcel de Rooy 2017-01-24 13:10:58 UTC
Created attachment 59497 [details] [review]
Bug 16018: Merge.pl does not update biblios linked to the old authority

Original patch from Remi Mayrand-Provencher.
Amended by Marcel de Rooy (January 2017), part of omnibus bug 17908.

EDIT:
We cannot remove the ModAuthority call, since the reference record is
the result of the merge and should be saved.
Variable $overwrite is not used and can be removed.

The ModAuthority call triggers a merge like: recordid1, original ref record,
recordid1, new ref record. This only updates all biblio records that were
already attached to the reference record.
As Remi discovered correctly, we still need another merge for the biblios
attached to recordid2 (the record to be deleted).
This patch adds that merge.

Note: The results of this merge operation should have been improved by the
changes to C4::AuthoritiesMarc::merge.

Test plan:
[1] Restart Plack and restart koha-indexer. (I had some problems related to
    indexing as a result of the marcxml transition.)
[2] Go to Authorities.
    Select two authorities to merge, start with two of the same type.
    Make sure that both are used in a few biblio records.
    Select at least one subfield from the authority to be removed for
    inclusion in the authority to be kept ("reference record").
[3] Click Merge. You come back to the kept authority record.
    Note: The number of attached records may not yet be updated. This depends
    on the speed of koha-indexer. If you wait a bit and refresh, you should
    have the correct number.
    First check the authority record again for the inserted subfield.
    Now check a biblio that was ALREADY attached to the reference record.
    And check a biblio that was FORMERLY attached to the deleted one.
    Note: See bug 17913 to see how pref AuthorityMergeMode impacts this.
[4] Try steps 2 and 3 again for a merge of two authority records that do not
    have the same authority type noting the remarks below.
    For instance try to merge from a CORPO_NAME to a PERSO_NAME record.
    Make a change in say 040$a of the record to be deleted. And select that
    field later on the merge form.
    Normally, you would use the PERSO_NAME framework in the merge now.
    When checking the authority record, look at 040a. When checking the
    record that was linked to the CORPO_NAME, check if it contains the
    PERSO_NAME authority in the correct field (say 100, 600, 700).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2017-01-24 13:14:13 UTC
The test plan may scare you away, but please stay. It is really not that hard to test this change. Manipulating two authorities and a few biblios in say 10 to 15 minutes should be enough.
Comment 14 Marcel de Rooy 2017-01-24 13:18:08 UTC
Still see something. Wait!
Comment 15 Marcel de Rooy 2017-01-24 13:55:54 UTC
Created attachment 59502 [details] [review]
Bug 16018: Add a TODO for need_merge_authorities in merge.pl

It is not useful to postpone a merge to the cron job here, since the
old authority record is deleted and merge_authority cannot yet handle
that correctly. But no harm either to leave it here.

Note: This will be addressed shortly on bug 9988. So please help!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Mehdi Hamidi 2017-01-24 22:41:00 UTC
Created attachment 59531 [details] [review]
Bug 16018: Merge.pl does not update biblios linked to the old authority

Original patch from Remi Mayrand-Provencher.
Amended by Marcel de Rooy (January 2017), part of omnibus bug 17908.

EDIT:
We cannot remove the ModAuthority call, since the reference record is
the result of the merge and should be saved.
Variable $overwrite is not used and can be removed.

The ModAuthority call triggers a merge like: recordid1, original ref record,
recordid1, new ref record. This only updates all biblio records that were
already attached to the reference record.
As Remi discovered correctly, we still need another merge for the biblios
attached to recordid2 (the record to be deleted).
This patch adds that merge.

Note: The results of this merge operation should have been improved by the
changes to C4::AuthoritiesMarc::merge.

Test plan:
[1] Restart Plack and restart koha-indexer. (I had some problems related to
    indexing as a result of the marcxml transition.)
[2] Go to Authorities.
    Select two authorities to merge, start with two of the same type.
    Make sure that both are used in a few biblio records.
    Select at least one subfield from the authority to be removed for
    inclusion in the authority to be kept ("reference record").
[3] Click Merge. You come back to the kept authority record.
    Note: The number of attached records may not yet be updated. This depends
    on the speed of koha-indexer. If you wait a bit and refresh, you should
    have the correct number.
    First check the authority record again for the inserted subfield.
    Now check a biblio that was ALREADY attached to the reference record.
    And check a biblio that was FORMERLY attached to the deleted one.
    Note: See bug 17913 to see how pref AuthorityMergeMode impacts this.
[4] Try steps 2 and 3 again for a merge of two authority records that do not
    have the same authority type noting the remarks below.
    For instance try to merge from a CORPO_NAME to a PERSO_NAME record.
    Make a change in say 040$a of the record to be deleted. And select that
    field later on the merge form.
    Normally, you would use the PERSO_NAME framework in the merge now.
    When checking the authority record, look at 040a. When checking the
    record that was linked to the CORPO_NAME, check if it contains the
    PERSO_NAME authority in the correct field (say 100, 600, 700).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: mehdi <mehdi@inlibro.com>
Comment 17 Mehdi Hamidi 2017-01-24 22:47:48 UTC
Created attachment 59532 [details] [review]
Bug 16018: Add a TODO for need_merge_authorities in merge.pl

It is not useful to postpone a merge to the cron job here, since the
old authority record is deleted and merge_authority cannot yet handle
that correctly. But no harm either to leave it here.

Note: This will be addressed shortly on bug 9988. So please help!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: mehdi <mehdi@inlibro.com>
Comment 18 Marcel de Rooy 2017-01-25 12:11:23 UTC
Created attachment 59547 [details] [review]
Bug 16018: Add a TODO for need_merge_authorities in merge.pl

It is not useful to postpone a merge to the cron job here, since the
old authority record is deleted and merge_authority cannot yet handle
that correctly. But no harm either to leave it here.

Note: This will be addressed shortly on bug 9988. So please help!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: mehdi <mehdi@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
EDIT: Typo recordid1 => recordid2 for new record in table.
Comment 19 Marcel de Rooy 2017-01-25 12:11:50 UTC
(In reply to Mehdi Hamidi from comment #17)
> Created attachment 59532 [details] [review] [review]
> Bug 16018: Add a TODO for need_merge_authorities in merge.pl
> 
> It is not useful to postpone a merge to the cron job here, since the
> old authority record is deleted and merge_authority cannot yet handle
> that correctly. But no harm either to leave it here.
> 
> Note: This will be addressed shortly on bug 9988. So please help!
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
> Signed-off-by: mehdi <mehdi@inlibro.com>

Great, mehdi. Thanks.
Comment 20 Julian Maurice 2017-02-06 09:52:31 UTC
I don't understand what these patches are trying to fix.
I followed the test plan on master and it works.

Looking at the code, the first patch adds a call to merge, but there is already the same call a few lines below. So merge is called twice with the same arguments if syspref 'dontmerge' is false.
About that syspref, I believe it shouldn't be used here. An authority will be deleted so we must update the biblios regardless of the syspref value.
Comment 21 Marcel de Rooy 2017-02-06 10:44:04 UTC
(In reply to Julian Maurice from comment #20)
> I don't understand what these patches are trying to fix.
> I followed the test plan on master and it works.
> 
> Looking at the code, the first patch adds a call to merge, but there is
> already the same call a few lines below. So merge is called twice with the
> same arguments if syspref 'dontmerge' is false.
> About that syspref, I believe it shouldn't be used here. An authority will
> be deleted so we must update the biblios regardless of the syspref value.

Will add a follow-up.
Comment 22 Marcel de Rooy 2017-02-06 13:15:46 UTC
Created attachment 59927 [details] [review]
Bug 16018: [Follow-up] Code cleaning

It is not useful to postpone a merge to the cron job here, since the
old authority record is deleted. (Note that bug 9988 will address this
subject too.)

Removing the call to C4::ImportBatch::SetImportRecordStatus: Since
$recordid2 is not an import_record_id, this is useless and potentially wrong.
Removed the if statement for @errors, since it is not used at this point;
putting the result of a comparison into $error is not useful either.

Note: there is a lot of code in merge.pl around breeding that is actually
unused. Template tools/manage-marc-import.tt contains two unused blocks
final_match_link and match_link that contain a (unreachable) call to
authorities/merge.pl with parameters:
mergereference=breeding&authid=[% record_lis.match_id %]&authid=[% record_lis.import_record_id %]
It seems that this would show the records correctly, but would not merge
them correctly since that code expects two authority records.

Leaving the signoff of Mehdi since he essentially tested the merge call.

Test plan: See first patch.

Signed-off-by: mehdi <mehdi@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested merging PERSO_NAME to PERSO_NAME and PERSO_NAME to CORPO_NAME.
Comment 23 Julian Maurice 2017-02-06 13:44:48 UTC
There is a typo (dontpref => dontmerge syspref).
Also, the first patch doesn't make much sense now (a lot of it is removed in the second patch) so if you could squash the 2 patches that would be perfect :)
Comment 24 Julian Maurice 2017-02-06 14:00:55 UTC
I opened bug 18063 for your remark on unused code in tools/manage-marc-import.tt
Comment 25 Marcel de Rooy 2017-02-06 14:07:12 UTC
Created attachment 59930 [details] [review]
Bug 16018: Merge.pl code cleanup

Original patch from Remi Mayrand-Provencher.
Amended by Marcel de Rooy (January 2017), part of omnibus bug 17908.

EDIT:
We cannot remove the ModAuthority call, since the reference record is
the result of the merge and should be saved.
Variable $overwrite is not used and can be removed.

It is not useful to postpone a merge to the cron job here, since the
old authority record is deleted. (Note that bug 9988 will address this
subject too.)

Removing the call to C4::ImportBatch::SetImportRecordStatus: Since
$recordid2 is not an import_record_id, this is useless and potentially wrong.
Removed the if statement for @errors, since it is not used at this point;
putting the result of a comparison into $error is not useful either.

Note: there is a lot of code in merge.pl around breeding that is actually
unused. Template tools/manage-marc-import.tt contains two unused blocks
final_match_link and match_link that contain a (unreachable) call to
authorities/merge.pl with parameters:
mergereference=breeding&authid=[% record_lis.match_id %]&authid=[% record_lis.import_record_id %]
It seems that this would show the records correctly, but would not merge
them correctly since that code expects two authority records.

Leaving the signoff of Mehdi since he essentially tested the merge call.

Test plan:
[1] Restart Plack and restart koha-indexer. (I had some problems related to
    indexing as a result of the marcxml transition.)
[2] Go to Authorities.
    Select two authorities to merge, start with two of the same type.
    Make sure that both are used in a few biblio records.
    Select at least one subfield from the authority to be removed for
    inclusion in the authority to be kept ("reference record").
[3] Click Merge. You come back to the kept authority record.
    Note: The number of attached records may not yet be updated. This depends
    on the speed of koha-indexer. If you wait a bit and refresh, you should
    have the correct number.
    First check the authority record again for the inserted subfield.
    Now check a biblio that was ALREADY attached to the reference record.
    And check a biblio that was FORMERLY attached to the deleted one.
    Note: See bug 17913 to see how pref AuthorityMergeMode impacts this.
[4] Try steps 2 and 3 again for a merge of two authority records that do not
    have the same authority type noting the remarks below.
    For instance try to merge from a CORPO_NAME to a PERSO_NAME record.
    Make a change in say 040$a of the record to be deleted. And select that
    field later on the merge form.
    Normally, you would use the PERSO_NAME framework in the merge now.
    When checking the authority record, look at 040a. When checking the
    record that was linked to the CORPO_NAME, check if it contains the
    PERSO_NAME authority in the correct field (say 100, 600, 700).

Signed-off-by: mehdi <mehdi@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested merging PERSO_NAME to PERSO_NAME and PERSO_NAME to CORPO_NAME.
Comment 26 Julian Maurice 2017-02-06 15:13:21 UTC
Created attachment 59935 [details] [review]
Bug 16018: Merge.pl code cleanup

Original patch from Remi Mayrand-Provencher.
Amended by Marcel de Rooy (January 2017), part of omnibus bug 17908.

EDIT:
We cannot remove the ModAuthority call, since the reference record is
the result of the merge and should be saved.
Variable $overwrite is not used and can be removed.

It is not useful to postpone a merge to the cron job here, since the
old authority record is deleted. (Note that bug 9988 will address this
subject too.)

Removing the call to C4::ImportBatch::SetImportRecordStatus: Since
$recordid2 is not an import_record_id, this is useless and potentially wrong.
Removed the if statement for @errors, since it is not used at this point;
putting the result of a comparison into $error is not useful either.

Note: there is a lot of code in merge.pl around breeding that is actually
unused. Template tools/manage-marc-import.tt contains two unused blocks
final_match_link and match_link that contain a (unreachable) call to
authorities/merge.pl with parameters:
mergereference=breeding&authid=[% record_lis.match_id %]&authid=[% record_lis.import_record_id %]
It seems that this would show the records correctly, but would not merge
them correctly since that code expects two authority records.

Leaving the signoff of Mehdi since he essentially tested the merge call.

Test plan:
[1] Restart Plack and restart koha-indexer. (I had some problems related to
    indexing as a result of the marcxml transition.)
[2] Go to Authorities.
    Select two authorities to merge, start with two of the same type.
    Make sure that both are used in a few biblio records.
    Select at least one subfield from the authority to be removed for
    inclusion in the authority to be kept ("reference record").
[3] Click Merge. You come back to the kept authority record.
    Note: The number of attached records may not yet be updated. This depends
    on the speed of koha-indexer. If you wait a bit and refresh, you should
    have the correct number.
    First check the authority record again for the inserted subfield.
    Now check a biblio that was ALREADY attached to the reference record.
    And check a biblio that was FORMERLY attached to the deleted one.
    Note: See bug 17913 to see how pref AuthorityMergeMode impacts this.
[4] Try steps 2 and 3 again for a merge of two authority records that do not
    have the same authority type noting the remarks below.
    For instance try to merge from a CORPO_NAME to a PERSO_NAME record.
    Make a change in say 040$a of the record to be deleted. And select that
    field later on the merge form.
    Normally, you would use the PERSO_NAME framework in the merge now.
    When checking the authority record, look at 040a. When checking the
    record that was linked to the CORPO_NAME, check if it contains the
    PERSO_NAME authority in the correct field (say 100, 600, 700).

Signed-off-by: mehdi <mehdi@inlibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested merging PERSO_NAME to PERSO_NAME and PERSO_NAME to CORPO_NAME.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 27 Kyle M Hall 2017-02-17 13:35:46 UTC
Pushed to master for 17.05, thanks Remi!
Comment 28 Katrin Fischer 2017-02-19 20:31:12 UTC
Is this correctly classified as an enh?
Comment 29 M. Tompsett 2017-02-20 01:14:18 UTC
(In reply to Katrin Fischer from comment #28)
> Is this correctly classified as an enh?

It isn't code equivalent and may change behaviours. I would think so, Katrin.
Comment 30 Katrin Fischer 2017-02-20 06:27:57 UTC
As this started with "As the summary says, merges are not modifying biblios properly" but has evolved since, I was not sure. Was wondering about it being a bug more than an enh - a behaviour change for the better would be expected then.
Comment 31 Marcel de Rooy 2017-02-20 07:24:27 UTC
(In reply to Katrin Fischer from comment #30)
> As this started with "As the summary says, merges are not modifying biblios
> properly" but has evolved since, I was not sure. Was wondering about it
> being a bug more than an enh - a behaviour change for the better would be
> expected then.

(In reply to M. Tompsett from comment #29)
> (In reply to Katrin Fischer from comment #28)
> > Is this correctly classified as an enh?
> 
> It isn't code equivalent and may change behaviours. I would think so, Katrin.

Bug fixes would meet your definition too :)
Comment 32 Marcel de Rooy 2017-02-20 07:26:00 UTC
(In reply to Katrin Fischer from comment #30)
> As this started with "As the summary says, merges are not modifying biblios
> properly" but has evolved since, I was not sure. Was wondering about it
> being a bug more than an enh - a behaviour change for the better would be
> expected then.

No need to backport this one (as we finally concluded); the problem in the summary was fixed already on a preceding report.
Comment 33 Katrin Fischer 2017-02-20 07:27:00 UTC
Thx, Marcel!