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.
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.
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.
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.
Created attachment 48889 [details] Bug16018 - 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
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 ?
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!
Another way of asking for testers, please.
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 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.
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.
Planning to get back here this week
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>
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.
Still see something. Wait!
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>
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>
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>
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.
(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.
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.
(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.
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.
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 :)
I opened bug 18063 for your remark on unused code in tools/manage-marc-import.tt
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.
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>
Pushed to master for 17.05, thanks Remi!
Is this correctly classified as an enh?
(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.
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 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 :)
(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.
Thx, Marcel!