Bug 17913 - Merge three authority merge fixes
Summary: Merge three authority merge fixes
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 5572 11315 11700 14026 (view as bug list)
Depends on: 17909
Blocks: 17908
  Show dependency treegraph
 
Reported: 2017-01-16 09:38 UTC by Marcel de Rooy
Modified: 2017-06-14 22:18 UTC (History)
9 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
Bug 17913: Add AuthorityMergeMode preference (3.69 KB, patch)
2017-01-16 12:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Use replace_with instead of insert_grouped_field (1.21 KB, patch)
2017-01-16 13:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Use AuthorityMergeMode pref in sub merge (1.51 KB, patch)
2017-01-16 13:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Adjust merge test for AuthorityMergeMode (2.75 KB, patch)
2017-01-16 13:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Fix the new field tag in merge when changing type (11.87 KB, patch)
2017-01-17 07:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Fix the new field tag in merge when changing type (12.11 KB, patch)
2017-01-17 07:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Do not keep a cleared subfield in loose merge mode (4.15 KB, patch)
2017-01-17 08:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Add AuthorityMergeMode preference (3.69 KB, patch)
2017-01-17 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Use replace_with instead of insert_grouped_field (1.21 KB, patch)
2017-01-17 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Use AuthorityMergeMode pref in sub merge (1.51 KB, patch)
2017-01-17 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Adjust merge test for AuthorityMergeMode (2.82 KB, patch)
2017-01-17 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Fix the new field tag in merge when changing type (11.37 KB, patch)
2017-01-17 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Do not keep a cleared subfield in loose merge mode (4.15 KB, patch)
2017-01-17 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Remove possible duplicates in strict merge mode (4.47 KB, patch)
2017-01-17 10:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Run perltidy on the inner foreach loop (4.49 KB, patch)
2017-01-17 11:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: We always need some housekeeping (6.97 KB, patch)
2017-01-17 13:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Run perltidy on the inner foreach loop (4.49 KB, patch)
2017-01-17 13:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: Add AuthorityMergeMode preference (3.71 KB, patch)
2017-01-17 21:13 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: Use replace_with instead of insert_grouped_field (1.24 KB, patch)
2017-01-17 21:13 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: Use AuthorityMergeMode pref in sub merge (1.54 KB, patch)
2017-01-17 21:13 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: Adjust merge test for AuthorityMergeMode (2.85 KB, patch)
2017-01-17 21:13 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: Fix the new field tag in merge when changing type (11.40 KB, patch)
2017-01-17 21:13 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: Do not keep a cleared subfield in loose merge mode (4.17 KB, patch)
2017-01-17 21:13 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: Remove possible duplicates in strict merge mode (4.50 KB, patch)
2017-01-17 21:13 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: We always need some housekeeping (7.00 KB, patch)
2017-01-17 21:14 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: Run perltidy on the inner foreach loop (4.52 KB, patch)
2017-01-17 21:14 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17913: Add AuthorityMergeMode preference (3.74 KB, patch)
2017-01-20 10:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Use replace_with instead of insert_grouped_field (1.27 KB, patch)
2017-01-20 10:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Use AuthorityMergeMode pref in sub merge (1.56 KB, patch)
2017-01-20 10:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Adjust merge test for AuthorityMergeMode (2.87 KB, patch)
2017-01-20 10:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Fix the new field tag in merge when changing type (11.81 KB, patch)
2017-01-20 10:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Do not keep a cleared subfield in loose merge mode (4.20 KB, patch)
2017-01-20 10:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Remove possible duplicates in strict merge mode (4.53 KB, patch)
2017-01-20 10:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: We always need some housekeeping (7.03 KB, patch)
2017-01-20 10:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Run perltidy on the inner foreach loop (4.55 KB, patch)
2017-01-20 10:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: [Follow-up] Another small fix for UNIMARC (1.16 KB, patch)
2017-01-20 11:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: [Follow-up] Fix duplicate $9s after merging in loose mode (1.09 KB, patch)
2017-01-20 12:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: Add AuthorityMergeMode preference (3.78 KB, patch)
2017-01-20 13:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: Use replace_with instead of insert_grouped_field (1.29 KB, patch)
2017-01-20 13:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: Use AuthorityMergeMode pref in sub merge (1.59 KB, patch)
2017-01-20 13:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: Adjust merge test for AuthorityMergeMode (2.90 KB, patch)
2017-01-20 13:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: Fix the new field tag in merge when changing type (11.84 KB, patch)
2017-01-20 13:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: Do not keep a cleared subfield in loose merge mode (4.23 KB, patch)
2017-01-20 13:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: Remove possible duplicates in strict merge mode (4.56 KB, patch)
2017-01-20 13:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: We always need some housekeeping (7.06 KB, patch)
2017-01-20 13:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: Run perltidy on the inner foreach loop (4.57 KB, patch)
2017-01-20 13:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: [Follow-up] Another small fix for UNIMARC (1.18 KB, patch)
2017-01-20 13:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: [Follow-up] Fix duplicate $9s after merging in loose mode (1.12 KB, patch)
2017-01-20 13:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 17913: [16.11.x] Authority merge fix (16.88 KB, patch)
2017-01-23 13:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: [16.05.x] Authority merge fix (16.88 KB, patch)
2017-01-23 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: [3.22.x] Authority merge fix (16.79 KB, patch)
2017-01-23 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: [16.11.x] Authority merge fix (16.90 KB, patch)
2017-02-08 22:02 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17913: [16.11.x] followup (1.32 KB, patch)
2017-02-08 22:02 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17913: Add syspref to sysprefs.sql (1.48 KB, patch)
2017-02-16 14:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17913: [16.11.x] Add syspref to sysprefs.sql (1.49 KB, patch)
2017-02-16 14:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-01-16 09:38:47 UTC
Best from 5572/11315/11700 comes here
Comment 1 Marcel de Rooy 2017-01-16 12:45:00 UTC
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>
Comment 2 Marcel de Rooy 2017-01-16 13:02:27 UTC
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>
Comment 3 Marcel de Rooy 2017-01-16 13:23:20 UTC
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>
Comment 4 Marcel de Rooy 2017-01-16 13:38:53 UTC
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>
Comment 5 Marcel de Rooy 2017-01-16 14:25:40 UTC
Work in progress: few follow-ups coming later this week. Please wait.
Comment 6 Marcel de Rooy 2017-01-17 07:44:10 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2017-01-17 07:47:04 UTC
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>
Comment 8 Marcel de Rooy 2017-01-17 08:45:42 UTC
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>
Comment 9 Marcel de Rooy 2017-01-17 09:54:39 UTC
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>
Comment 10 Marcel de Rooy 2017-01-17 09:54:42 UTC
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>
Comment 11 Marcel de Rooy 2017-01-17 09:54:46 UTC
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>
Comment 12 Marcel de Rooy 2017-01-17 09:54:49 UTC
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>
Comment 13 Marcel de Rooy 2017-01-17 09:54:53 UTC
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>
Comment 14 Marcel de Rooy 2017-01-17 09:54:57 UTC
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>
Comment 15 Marcel de Rooy 2017-01-17 09:55:22 UTC
Rebased on top of three small typos :)
Comment 16 Marcel de Rooy 2017-01-17 10:27:39 UTC
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>
Comment 17 Marcel de Rooy 2017-01-17 11:25:04 UTC
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>
Comment 18 Marcel de Rooy 2017-01-17 11:49:43 UTC
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 !
Comment 19 Josef Moravec 2017-01-17 12:21:10 UTC
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
Comment 20 Marcel de Rooy 2017-01-17 13:01:34 UTC
(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
Comment 21 Marcel de Rooy 2017-01-17 13:08:56 UTC
(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..
Comment 22 Marcel de Rooy 2017-01-17 13:17:16 UTC
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>
Comment 23 Marcel de Rooy 2017-01-17 13:17:20 UTC
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>
Comment 24 Marcel de Rooy 2017-01-17 13:18:51 UTC
OK It seems that I forgot the housekeeping patch (patch 8)
Please have another try
Comment 25 Josef Moravec 2017-01-17 21:13:18 UTC
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>
Comment 26 Josef Moravec 2017-01-17 21:13:34 UTC
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>
Comment 27 Josef Moravec 2017-01-17 21:13:40 UTC
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>
Comment 28 Josef Moravec 2017-01-17 21:13:45 UTC
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>
Comment 29 Josef Moravec 2017-01-17 21:13:49 UTC
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>
Comment 30 Josef Moravec 2017-01-17 21:13:54 UTC
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>
Comment 31 Josef Moravec 2017-01-17 21:13:58 UTC
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>
Comment 32 Josef Moravec 2017-01-17 21:14:04 UTC
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>
Comment 33 Josef Moravec 2017-01-17 21:14:10 UTC
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>
Comment 34 Marcel de Rooy 2017-01-18 07:41:14 UTC
(In reply to Josef Moravec from comment #33)
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Great! Thanks
Comment 35 Marcel de Rooy 2017-01-20 10:45:36 UTC
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>
Comment 36 Marcel de Rooy 2017-01-20 10:45:40 UTC
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>
Comment 37 Marcel de Rooy 2017-01-20 10:45:44 UTC
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>
Comment 38 Marcel de Rooy 2017-01-20 10:45:48 UTC
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>
Comment 39 Marcel de Rooy 2017-01-20 10:45:52 UTC
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>
Comment 40 Marcel de Rooy 2017-01-20 10:45:56 UTC
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>
Comment 41 Marcel de Rooy 2017-01-20 10:46:00 UTC
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>
Comment 42 Marcel de Rooy 2017-01-20 10:46:04 UTC
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>
Comment 43 Marcel de Rooy 2017-01-20 10:46:08 UTC
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>
Comment 44 Marcel de Rooy 2017-01-20 10:46:52 UTC
Rebased on top of the UNIMARC fix on 17909
Comment 45 Marcel de Rooy 2017-01-20 11:56:07 UTC
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.
Comment 46 Marcel de Rooy 2017-01-20 12:58:30 UTC
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>
Comment 47 Julian Maurice 2017-01-20 13:09:20 UTC
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>
Comment 48 Julian Maurice 2017-01-20 13:09:27 UTC
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>
Comment 49 Julian Maurice 2017-01-20 13:09:33 UTC
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>
Comment 50 Julian Maurice 2017-01-20 13:09:39 UTC
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>
Comment 51 Julian Maurice 2017-01-20 13:09:44 UTC
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>
Comment 52 Julian Maurice 2017-01-20 13:09:50 UTC
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>
Comment 53 Julian Maurice 2017-01-20 13:09:55 UTC
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>
Comment 54 Julian Maurice 2017-01-20 13:10:00 UTC
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>
Comment 55 Julian Maurice 2017-01-20 13:10:06 UTC
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>
Comment 56 Julian Maurice 2017-01-20 13:10:13 UTC
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>
Comment 57 Julian Maurice 2017-01-20 13:10:20 UTC
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>
Comment 58 Julian Maurice 2017-01-20 13:11:51 UTC
Everything looks good. Tested strict and loose mode.
Passed QA
Comment 59 Marcel de Rooy 2017-01-20 13:12:54 UTC
(In reply to Julian Maurice from comment #58)
> Everything looks good. Tested strict and loose mode.
> Passed QA

Thanks, Julian !
Comment 60 Kyle M Hall 2017-01-20 13:55:22 UTC
Pushed to master for 17.05, thanks Frederic, Maxime, Marcel!
Comment 61 Katrin Fischer 2017-01-22 20:07:27 UTC
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
Comment 62 Marcel de Rooy 2017-01-23 07:32:38 UTC
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.
Comment 63 Marcel de Rooy 2017-01-23 08:06:40 UTC
*** Bug 5572 has been marked as a duplicate of this bug. ***
Comment 64 Marcel de Rooy 2017-01-23 08:09:09 UTC
*** Bug 11315 has been marked as a duplicate of this bug. ***
Comment 65 Marcel de Rooy 2017-01-23 08:10:27 UTC
*** Bug 11700 has been marked as a duplicate of this bug. ***
Comment 66 Marcel de Rooy 2017-01-23 13:51:27 UTC
Created attachment 59411 [details] [review]
Bug 17913: [16.11.x] Authority merge fix

[PUSHED_17.05]

Squashed into one patch for 16.11.x
Comment 67 Marcel de Rooy 2017-01-23 13:52:06 UTC
Created attachment 59412 [details] [review]
Bug 17913: [16.05.x] Authority merge fix

[PUSHED_17.05]

Squashed into one patch for 16.05.x
Comment 68 Marcel de Rooy 2017-01-23 13:52:37 UTC
Created attachment 59413 [details] [review]
Bug 17913: [3.22.x] Authority merge fix

[PUSHED_17.05]

Squashed into one patch for 3.22.x.
Comment 69 Marcel de Rooy 2017-01-23 13:56:34 UTC
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.
Comment 70 Marcel de Rooy 2017-01-23 13:59:06 UTC
And obviously, please push 17909 to 3.22/16.05 before trying the above.
Comment 71 Marcel de Rooy 2017-01-31 14:57:44 UTC
Katrin: Better add the syspref.sql addition too in the dbrev, although it technically speaking is not a must..
Comment 72 Marcel de Rooy 2017-02-07 08:53:07 UTC
*** Bug 14026 has been marked as a duplicate of this bug. ***
Comment 73 Josef Moravec 2017-02-08 22:02:12 UTC
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>
Comment 74 Josef Moravec 2017-02-08 22:02:29 UTC
Created attachment 60043 [details] [review]
[SIGNED-OFF] Bug 17913: [16.11.x] followup

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 75 Katrin Fischer 2017-02-14 21:37:10 UTC
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?
Comment 76 Marcel de Rooy 2017-02-16 14:02:00 UTC Comment hidden (obsolete)
Comment 77 Marcel de Rooy 2017-02-16 14:02:36 UTC
Created attachment 60342 [details] [review]
Bug 17913: [16.11.x] Add syspref to sysprefs.sql

See comment 69.
Comment 78 Marcel de Rooy 2017-02-16 14:05:56 UTC
(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..
Comment 79 Marcel de Rooy 2017-02-16 14:07:13 UTC
(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.
Comment 80 Katrin Fischer 2017-02-19 21:19:30 UTC
I have pushed the last follow-up for sysprefs.sql to 16.11.x, will be in 16.11.04.
Comment 81 Julian Maurice 2017-02-20 11:02:46 UTC
Thanks for the squashed patch :-)

Pushed to 3.22.x for 3.22.17
Comment 82 Marcel de Rooy 2017-02-20 11:06:16 UTC
ping mtj mason
Comment 83 Mason James 2017-02-22 22:20:14 UTC
(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?
Comment 84 Katrin Fischer 2017-02-22 22:30:32 UTC
Hi Mason, mabe you will need something similar to the 16.11.x follow-up Joseph did for me?
Comment 85 Marcel de Rooy 2017-02-23 13:37:21 UTC
(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.
Comment 86 Marcel de Rooy 2017-02-24 07:53:58 UTC
Mason: Looks good to me. Sent you a pm.