Best from 5572/11315/11700 comes here
Created attachment 59009 [details] [review] Bug 17913: Add AuthorityMergeMode preference Original patch from bug 5572, dating back to 2011! Amended by Marcel de Rooy January 2017. Renamed the pref. The fix on this report is based on this preference. Depending on the pref, subfields will be deleted or kept. Test plan: Run the dbrev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59010 [details] [review] Bug 17913: Use replace_with instead of insert_grouped_field Original fix from a patch on bug 5572. Amended by Marcel de Rooy January 2017. Note: This does not yet resolve the field order when merging to another auth type, but is a good start. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59016 [details] [review] Bug 17913: Use AuthorityMergeMode pref in sub merge Original fix from a patch on bug 11315. Amended by Marcel de Rooy January 2017. Test plan: If you set mode to loose, the test will still pass. If you set mode to strict, one test will fail. (Fixed later.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59017 [details] [review] Bug 17913: Adjust merge test for AuthorityMergeMode Original fix from a patch on bug 11315. Amended by Marcel de Rooy January 2017. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Work in progress: few follow-ups coming later this week. Please wait.
Created attachment 59065 [details] [review] Bug 17913: Fix the new field tag in merge when changing type Originally aimed for 9988, adjusted for this report. Old behavior was: pick the first tag. This is definitely wrong. If you (would) merge 610 to 611, you don't want to get a 111. This patch resolves the problem by determining the new tag in a small helper routine _merge_newtag, and corrects the position of the new field in the MARC record with append_fields_ordered. Too bad that MARC::Record does not have such a function; it looks like insert_fields_ordered, but it is different in case of multiple fields with the same tag. Note: These two small helper functions are not tested separately, since they should not be called outside of merge. They are implicitly tested by the adjusted tests in Merge.t. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59066 [details] [review] Bug 17913: Fix the new field tag in merge when changing type Originally aimed for 9988, adjusted for this report. Old behavior was: pick the first tag. This is definitely wrong. If you (would) merge 610 to 611, you don't want to get a 111. This patch resolves the problem by determining the new tag in a small helper routine _merge_newtag, and corrects the position of the new field in the MARC record with append_fields_ordered. Too bad that MARC::Record does not have such a function; it looks like insert_fields_ordered, but it is different in case of multiple fields with the same tag. Note: These two small helper functions are not tested separately, since they should not be called outside of merge. They are implicitly tested by the adjusted tests in Merge.t. Note: In adding tests for this fix, I chose to simplify compare_field_count (no need for the pass parameter), and replace the pass parameter of sub compare_field_order by an exclude parameter, a hash of fields to exclude in counting fields. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59073 [details] [review] Bug 17913: Do not keep a cleared subfield in loose merge mode If you modify an authority and clear a specific subfield, you expect that merge respects your edit and clears this subfield too in the biblio records. It does in the new strict mode, but it does not yet in the default loose mode. This patch fixes that by adjusting the code around $exclude so that it uses a new hash skip_subfields, built from the reporting tags from the old and the new authority record. This is supported again by some changes in the unit test. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59078 [details] [review] Bug 17913: Add AuthorityMergeMode preference Original patch from bug 5572, dating back to 2011! Amended by Marcel de Rooy January 2017. Renamed the pref. The fix on this report is based on this preference. Depending on the pref, subfields will be deleted or kept. Test plan: Run the dbrev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59079 [details] [review] Bug 17913: Use replace_with instead of insert_grouped_field Original fix from a patch on bug 5572. Amended by Marcel de Rooy January 2017. Note: This does not yet resolve the field order when merging to another auth type, but is a good start. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59080 [details] [review] Bug 17913: Use AuthorityMergeMode pref in sub merge Original fix from a patch on bug 11315. Amended by Marcel de Rooy January 2017. Test plan: If you set mode to loose, the test will still pass. If you set mode to strict, one test will fail. (Fixed later.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59081 [details] [review] Bug 17913: Adjust merge test for AuthorityMergeMode Original fix from a patch on bug 11315. Amended by Marcel de Rooy January 2017. Test plan: Run t/db_dependent/Authorities/Merge.t in both loose and strict mode. Should no longer make a difference. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59082 [details] [review] Bug 17913: Fix the new field tag in merge when changing type Originally aimed for 9988, adjusted for this report. Old behavior was: pick the first tag. This is definitely wrong. If you (would) merge 610 to 611, you don't want to get a 111. This patch resolves the problem by determining the new tag in a small helper routine _merge_newtag, and corrects the position of the new field in the MARC record with append_fields_ordered. Too bad that MARC::Record does not have such a function; it looks like insert_fields_ordered, but it is different in case of multiple fields with the same tag. Note: These two small helper functions are not tested separately, since they should not be called outside of merge. They are implicitly tested by the adjusted tests in Merge.t. Note: In adding tests for this fix, I chose to simplify compare_field_count (no need for the pass parameter), and replace the pass parameter of sub compare_field_order by an exclude parameter, a hash of fields to exclude in counting fields. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59083 [details] [review] Bug 17913: Do not keep a cleared subfield in loose merge mode If you modify an authority and clear a specific subfield, you expect that merge respects your edit and clears this subfield too in the biblio records. It does in the new strict mode, but it does not yet in the default loose mode. This patch fixes that by adjusting the code around $exclude so that it uses a new hash skip_subfields, built from the reporting tags from the old and the new authority record. This is supported again by some changes in the unit test. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Rebased on top of three small typos :)
Created attachment 59084 [details] [review] Bug 17913: Remove possible duplicates in strict merge mode Since strict mode does not allow additional subfields that would make identical fields linked to the same authority different, there is no need to keep them while merging. We achieve this goal by simply: [1] Count the number of same fields linked to mergefrom in strict mode to eliminate duplicates. [2] Replaces the if-statement on auth_number by a next. (Tidy follows.) Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59085 [details] [review] Bug 17913: Run perltidy on the inner foreach loop Kept the same number of lines. You could verify with diff -w. Test plan: [1] Run t/db_dependent/Authorities/Merge.t [2] As the last patch in this series, also test the interface: Set AuthorityMergeMode to loose. Set dontmerge to Do. Modify an authority record attached to multiple biblios. Edit a subfield, clear a subfield and add a subfield. Save. Wait a bit for the merge and Zebra update. Verify that the changes are merged properly into biblio records. [3] Repeat step 2 with AuthorityMergeMode to strict. Remember that this affects the extra subfields in biblio records. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
When testing, run the db revision (from the first patch) and look at the test plan of the last patch. Only if you are interested in following each individual fix (like in QA), you can run the merge test while applying patches individually. Thanks !
I am sorry, I got "fatal: sha1 information is lacking or useless (C4/AuthoritiesMarc.pm)." on the last patch (perltidy on the inner loop)... Applying on top of 17909
(In reply to Josef Moravec from comment #19) > I am sorry, I got "fatal: sha1 information is lacking or useless > (C4/AuthoritiesMarc.pm)." on the last patch (perltidy on the inner loop)... > > Applying on top of 17909 Taking a look
(In reply to Josef Moravec from comment #19) > I am sorry, I got "fatal: sha1 information is lacking or useless > (C4/AuthoritiesMarc.pm)." on the last patch (perltidy on the inner loop)... > > Applying on top of 17909 Yes, I somehow messing something up between patch 8 and patch 9. Please wait..
Created attachment 59089 [details] [review] Bug 17913: We always need some housekeeping Remove some commented warnings Remove the commented old code at the end of sub merge Explicitly set merge mode in the first subtest Move the return to loose mode from the second subtest to the third Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59090 [details] [review] Bug 17913: Run perltidy on the inner foreach loop Kept the same number of lines. You could verify with diff -w. Test plan: [1] Run t/db_dependent/Authorities/Merge.t [2] As the last patch in this series, also test the interface: Set AuthorityMergeMode to loose. Set dontmerge to Do. Modify an authority record attached to multiple biblios. Edit a subfield, clear a subfield and add a subfield. Save. Wait a bit for the merge and Zebra update. Verify that the changes are merged properly into biblio records. [3] Repeat step 2 with AuthorityMergeMode to strict. Remember that this affects the extra subfields in biblio records. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
OK It seems that I forgot the housekeeping patch (patch 8) Please have another try
Created attachment 59102 [details] [review] [SIGNED-OFF] Bug 17913: Add AuthorityMergeMode preference Original patch from bug 5572, dating back to 2011! Amended by Marcel de Rooy January 2017. Renamed the pref. The fix on this report is based on this preference. Depending on the pref, subfields will be deleted or kept. Test plan: Run the dbrev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59103 [details] [review] [SIGNED-OFF] Bug 17913: Use replace_with instead of insert_grouped_field Original fix from a patch on bug 5572. Amended by Marcel de Rooy January 2017. Note: This does not yet resolve the field order when merging to another auth type, but is a good start. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59104 [details] [review] [SIGNED-OFF] Bug 17913: Use AuthorityMergeMode pref in sub merge Original fix from a patch on bug 11315. Amended by Marcel de Rooy January 2017. Test plan: If you set mode to loose, the test will still pass. If you set mode to strict, one test will fail. (Fixed later.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59105 [details] [review] [SIGNED-OFF] Bug 17913: Adjust merge test for AuthorityMergeMode Original fix from a patch on bug 11315. Amended by Marcel de Rooy January 2017. Test plan: Run t/db_dependent/Authorities/Merge.t in both loose and strict mode. Should no longer make a difference. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59106 [details] [review] [SIGNED-OFF] Bug 17913: Fix the new field tag in merge when changing type Originally aimed for 9988, adjusted for this report. Old behavior was: pick the first tag. This is definitely wrong. If you (would) merge 610 to 611, you don't want to get a 111. This patch resolves the problem by determining the new tag in a small helper routine _merge_newtag, and corrects the position of the new field in the MARC record with append_fields_ordered. Too bad that MARC::Record does not have such a function; it looks like insert_fields_ordered, but it is different in case of multiple fields with the same tag. Note: These two small helper functions are not tested separately, since they should not be called outside of merge. They are implicitly tested by the adjusted tests in Merge.t. Note: In adding tests for this fix, I chose to simplify compare_field_count (no need for the pass parameter), and replace the pass parameter of sub compare_field_order by an exclude parameter, a hash of fields to exclude in counting fields. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59107 [details] [review] [SIGNED-OFF] Bug 17913: Do not keep a cleared subfield in loose merge mode If you modify an authority and clear a specific subfield, you expect that merge respects your edit and clears this subfield too in the biblio records. It does in the new strict mode, but it does not yet in the default loose mode. This patch fixes that by adjusting the code around $exclude so that it uses a new hash skip_subfields, built from the reporting tags from the old and the new authority record. This is supported again by some changes in the unit test. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59108 [details] [review] [SIGNED-OFF] Bug 17913: Remove possible duplicates in strict merge mode Since strict mode does not allow additional subfields that would make identical fields linked to the same authority different, there is no need to keep them while merging. We achieve this goal by simply: [1] Count the number of same fields linked to mergefrom in strict mode to eliminate duplicates. [2] Replaces the if-statement on auth_number by a next. (Tidy follows.) Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59109 [details] [review] [SIGNED-OFF] Bug 17913: We always need some housekeeping Remove some commented warnings Remove the commented old code at the end of sub merge Explicitly set merge mode in the first subtest Move the return to loose mode from the second subtest to the third Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59110 [details] [review] [SIGNED-OFF] Bug 17913: Run perltidy on the inner foreach loop Kept the same number of lines. You could verify with diff -w. Test plan: [1] Run t/db_dependent/Authorities/Merge.t [2] As the last patch in this series, also test the interface: Set AuthorityMergeMode to loose. Set dontmerge to Do. Modify an authority record attached to multiple biblios. Edit a subfield, clear a subfield and add a subfield. Save. Wait a bit for the merge and Zebra update. Verify that the changes are merged properly into biblio records. [3] Repeat step 2 with AuthorityMergeMode to strict. Remember that this affects the extra subfields in biblio records. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
(In reply to Josef Moravec from comment #33) > Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Great! Thanks
Created attachment 59325 [details] [review] Bug 17913: Add AuthorityMergeMode preference Original patch from bug 5572, dating back to 2011! Amended by Marcel de Rooy January 2017. Renamed the pref. The fix on this report is based on this preference. Depending on the pref, subfields will be deleted or kept. Test plan: Run the dbrev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59326 [details] [review] Bug 17913: Use replace_with instead of insert_grouped_field Original fix from a patch on bug 5572. Amended by Marcel de Rooy January 2017. Note: This does not yet resolve the field order when merging to another auth type, but is a good start. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59327 [details] [review] Bug 17913: Use AuthorityMergeMode pref in sub merge Original fix from a patch on bug 11315. Amended by Marcel de Rooy January 2017. Test plan: If you set mode to loose, the test will still pass. If you set mode to strict, one test will fail. (Fixed later.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59328 [details] [review] Bug 17913: Adjust merge test for AuthorityMergeMode Original fix from a patch on bug 11315. Amended by Marcel de Rooy January 2017. Test plan: Run t/db_dependent/Authorities/Merge.t in both loose and strict mode. Should no longer make a difference. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59329 [details] [review] Bug 17913: Fix the new field tag in merge when changing type Originally aimed for 9988, adjusted for this report. Old behavior was: pick the first tag. This is definitely wrong. If you (would) merge 610 to 611, you don't want to get a 111. This patch resolves the problem by determining the new tag in a small helper routine _merge_newtag, and corrects the position of the new field in the MARC record with append_fields_ordered. Too bad that MARC::Record does not have such a function; it looks like insert_fields_ordered, but it is different in case of multiple fields with the same tag. Note: These two small helper functions are not tested separately, since they should not be called outside of merge. They are implicitly tested by the adjusted tests in Merge.t. Note: In adding tests for this fix, I chose to simplify compare_field_count (no need for the pass parameter), and replace the pass parameter of sub compare_field_order by an exclude parameter, a hash of fields to exclude in counting fields. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59330 [details] [review] Bug 17913: Do not keep a cleared subfield in loose merge mode If you modify an authority and clear a specific subfield, you expect that merge respects your edit and clears this subfield too in the biblio records. It does in the new strict mode, but it does not yet in the default loose mode. This patch fixes that by adjusting the code around $exclude so that it uses a new hash skip_subfields, built from the reporting tags from the old and the new authority record. This is supported again by some changes in the unit test. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59331 [details] [review] Bug 17913: Remove possible duplicates in strict merge mode Since strict mode does not allow additional subfields that would make identical fields linked to the same authority different, there is no need to keep them while merging. We achieve this goal by simply: [1] Count the number of same fields linked to mergefrom in strict mode to eliminate duplicates. [2] Replaces the if-statement on auth_number by a next. (Tidy follows.) Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59332 [details] [review] Bug 17913: We always need some housekeeping Remove some commented warnings Remove the commented old code at the end of sub merge Explicitly set merge mode in the first subtest Move the return to loose mode from the second subtest to the third Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 59333 [details] [review] Bug 17913: Run perltidy on the inner foreach loop Kept the same number of lines. You could verify with diff -w. Test plan: [1] Run t/db_dependent/Authorities/Merge.t [2] As the last patch in this series, also test the interface: Set AuthorityMergeMode to loose. Set dontmerge to Do. Modify an authority record attached to multiple biblios. Edit a subfield, clear a subfield and add a subfield. Save. Wait a bit for the merge and Zebra update. Verify that the changes are merged properly into biblio records. [3] Repeat step 2 with AuthorityMergeMode to strict. Remember that this affects the extra subfields in biblio records. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Rebased on top of the UNIMARC fix on 17909
Created attachment 59352 [details] [review] Bug 17913: [Follow-up] Another small fix for UNIMARC Adding another delete for field 100. Will mock GetMarcBiblio on a new report.
Created attachment 59358 [details] [review] Bug 17913: [Follow-up] Fix duplicate $9s after merging in loose mode We need to add $9 to the skip_subfields hash too. Formerly, it was added to $exclude as well. Thanks, Julian, for catching this one. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59360 [details] [review] Bug 17913: Add AuthorityMergeMode preference Original patch from bug 5572, dating back to 2011! Amended by Marcel de Rooy January 2017. Renamed the pref. The fix on this report is based on this preference. Depending on the pref, subfields will be deleted or kept. Test plan: Run the dbrev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59361 [details] [review] Bug 17913: Use replace_with instead of insert_grouped_field Original fix from a patch on bug 5572. Amended by Marcel de Rooy January 2017. Note: This does not yet resolve the field order when merging to another auth type, but is a good start. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59362 [details] [review] Bug 17913: Use AuthorityMergeMode pref in sub merge Original fix from a patch on bug 11315. Amended by Marcel de Rooy January 2017. Test plan: If you set mode to loose, the test will still pass. If you set mode to strict, one test will fail. (Fixed later.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59363 [details] [review] Bug 17913: Adjust merge test for AuthorityMergeMode Original fix from a patch on bug 11315. Amended by Marcel de Rooy January 2017. Test plan: Run t/db_dependent/Authorities/Merge.t in both loose and strict mode. Should no longer make a difference. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59364 [details] [review] Bug 17913: Fix the new field tag in merge when changing type Originally aimed for 9988, adjusted for this report. Old behavior was: pick the first tag. This is definitely wrong. If you (would) merge 610 to 611, you don't want to get a 111. This patch resolves the problem by determining the new tag in a small helper routine _merge_newtag, and corrects the position of the new field in the MARC record with append_fields_ordered. Too bad that MARC::Record does not have such a function; it looks like insert_fields_ordered, but it is different in case of multiple fields with the same tag. Note: These two small helper functions are not tested separately, since they should not be called outside of merge. They are implicitly tested by the adjusted tests in Merge.t. Note: In adding tests for this fix, I chose to simplify compare_field_count (no need for the pass parameter), and replace the pass parameter of sub compare_field_order by an exclude parameter, a hash of fields to exclude in counting fields. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59365 [details] [review] Bug 17913: Do not keep a cleared subfield in loose merge mode If you modify an authority and clear a specific subfield, you expect that merge respects your edit and clears this subfield too in the biblio records. It does in the new strict mode, but it does not yet in the default loose mode. This patch fixes that by adjusting the code around $exclude so that it uses a new hash skip_subfields, built from the reporting tags from the old and the new authority record. This is supported again by some changes in the unit test. Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59366 [details] [review] Bug 17913: Remove possible duplicates in strict merge mode Since strict mode does not allow additional subfields that would make identical fields linked to the same authority different, there is no need to keep them while merging. We achieve this goal by simply: [1] Count the number of same fields linked to mergefrom in strict mode to eliminate duplicates. [2] Replaces the if-statement on auth_number by a next. (Tidy follows.) Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59367 [details] [review] Bug 17913: We always need some housekeeping Remove some commented warnings Remove the commented old code at the end of sub merge Explicitly set merge mode in the first subtest Move the return to loose mode from the second subtest to the third Test plan: Run t/db_dependent/Authorities/Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59368 [details] [review] Bug 17913: Run perltidy on the inner foreach loop Kept the same number of lines. You could verify with diff -w. Test plan: [1] Run t/db_dependent/Authorities/Merge.t [2] As the last patch in this series, also test the interface: Set AuthorityMergeMode to loose. Set dontmerge to Do. Modify an authority record attached to multiple biblios. Edit a subfield, clear a subfield and add a subfield. Save. Wait a bit for the merge and Zebra update. Verify that the changes are merged properly into biblio records. [3] Repeat step 2 with AuthorityMergeMode to strict. Remember that this affects the extra subfields in biblio records. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59369 [details] [review] Bug 17913: [Follow-up] Another small fix for UNIMARC Adding another delete for field 100. Will mock GetMarcBiblio on a new report. Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59370 [details] [review] Bug 17913: [Follow-up] Fix duplicate $9s after merging in loose mode We need to add $9 to the skip_subfields hash too. Formerly, it was added to $exclude as well. Thanks, Julian, for catching this one. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Everything looks good. Tested strict and loose mode. Passed QA
(In reply to Julian Maurice from comment #58) > Everything looks good. Tested strict and loose mode. > Passed QA Thanks, Julian !
Pushed to master for 17.05, thanks Frederic, Maxime, Marcel!
This adds a new system preference, but is marked as a bug (not enh). I think too dangerous now to push right before release as I lack time for properly testing this myself. Please find me and let's talk about what this patch set does fix, if you want it included in 16.11.x
Makes sense not to push it one day before release. But apart from the new syspref, it might well be a patch set to consider for backporting. Solves several authority merge errors.
*** Bug 5572 has been marked as a duplicate of this bug. ***
*** Bug 11315 has been marked as a duplicate of this bug. ***
*** Bug 11700 has been marked as a duplicate of this bug. ***
Created attachment 59411 [details] [review] Bug 17913: [16.11.x] Authority merge fix [PUSHED_17.05] Squashed into one patch for 16.11.x
Created attachment 59412 [details] [review] Bug 17913: [16.05.x] Authority merge fix [PUSHED_17.05] Squashed into one patch for 16.05.x
Created attachment 59413 [details] [review] Bug 17913: [3.22.x] Authority merge fix [PUSHED_17.05] Squashed into one patch for 3.22.x.
NOTE FOR RELEASE MAINTAINERS: I backported the master version to my own 3.22.x branch. Have tested it on 3.22.x. Will start using it in production soon. Have rebased it afterwards on 16.05 and 16.11 WITHOUT testing it. It still needs a DBREV that adds the pref. You can copy that from the first master patch here. Please verify the patch by 1) using the Merge.t and 2) following the test plan on the "Run perltidy" master patch. Note: The patches for housekeeping and perltidy have not been applied to the AuthoritiesMarc.pm code. Hope this is helpful.
And obviously, please push 17909 to 3.22/16.05 before trying the above.
Katrin: Better add the syspref.sql addition too in the dbrev, although it technically speaking is not a must..
*** Bug 14026 has been marked as a duplicate of this bug. ***
Created attachment 60042 [details] [review] [SIGNED-OFF] Bug 17913: [16.11.x] Authority merge fix [PUSHED_17.05] Squashed into one patch for 16.11.x Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 60043 [details] [review] [SIGNED-OFF] Bug 17913: [16.11.x] followup Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Thx a lot Josef for retesting this on 16.11 specifically. I have pushed Marcel's patch and Josef's follow-up up to 16.11.x for 16.11.04 now. Marcel, could you take a look maybe too?
Created attachment 60341 [details] [review] Bug 17913: Add syspref to sysprefs.sql See comment 69.
Created attachment 60342 [details] [review] Bug 17913: [16.11.x] Add syspref to sysprefs.sql See comment 69.
(In reply to Katrin Fischer from comment #75) > Thx a lot Josef for retesting this on 16.11 specifically. > I have pushed Marcel's patch and Josef's follow-up up to 16.11.x for > 16.11.04 now. > > Marcel, could you take a look maybe too? Tested it on a clean 16.11. Had no problems with the merge. Small struggle with Zebra, not related to this patch..
(In reply to Marcel de Rooy from comment #77) > Created attachment 60342 [details] [review] [review] > Bug 17913: [16.11.x] Add syspref to sysprefs.sql > > See comment 69. And comment 71.
I have pushed the last follow-up for sysprefs.sql to 16.11.x, will be in 16.11.04.
Thanks for the squashed patch :-) Pushed to 3.22.x for 3.22.17
ping mtj mason
(In reply to Marcel de Rooy from comment #82) > ping mtj mason hi Marcel i get a compile error after applying the 16.5.x patch ------------------ $ perl -c C4/AuthoritiesMarc.pm Global symbol "$authtypecodefrom" requires explicit package name at C4/AuthoritiesMarc.pm line 1462. Global symbol "$authtypecodeto" requires explicit package name at C4/AuthoritiesMarc.pm line 1464. Global symbol "$authtypecodefrom" requires explicit package name at C4/AuthoritiesMarc.pm line 1464. Global symbol "$authtypecodeto" requires explicit package name at C4/AuthoritiesMarc.pm line 1465. C4/AuthoritiesMarc.pm had compilation errors. ------------------ any ideas folks?
Hi Mason, mabe you will need something similar to the 16.11.x follow-up Joseph did for me?
(In reply to Mason James from comment #83) > (In reply to Marcel de Rooy from comment #82) > > ping mtj mason > > hi Marcel > > i get a compile error after applying the 16.5.x patch > > ------------------ > $ perl -c C4/AuthoritiesMarc.pm > Global symbol "$authtypecodefrom" requires explicit package name at > C4/AuthoritiesMarc.pm line 1462. > Global symbol "$authtypecodeto" requires explicit package name at > C4/AuthoritiesMarc.pm line 1464. > Global symbol "$authtypecodefrom" requires explicit package name at > C4/AuthoritiesMarc.pm line 1464. > Global symbol "$authtypecodeto" requires explicit package name at > C4/AuthoritiesMarc.pm line 1465. > C4/AuthoritiesMarc.pm had compilation errors. > ------------------ > > any ideas folks? Seems that you indeed miss the patch from Josef here. Will test it tomorrow on a 16.05 tweak install.
Mason: Looks good to me. Sent you a pm.