Bug 9988 - Leave larger authority merges to merge_authorities cronjob (pref AuthorityMergeLimit)
Summary: Leave larger authority merges to merge_authorities cronjob (pref AuthorityMer...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact:
URL:
Keywords:
: 5821 12791 (view as bug list)
Depends on: 18070
Blocks: 22557 17908 18149 25394 33367
  Show dependency treegraph
 
Reported: 2013-04-04 08:55 UTC by Marcel de Rooy
Modified: 2023-04-07 18:09 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement replaces dontmerge by a limit. The Zebra code in merge is removed. The cron job has been refactored, and is no longer optional; it also supports merges from one authority type to another (with a table revision).
Version(s) released in:


Attachments
Bug 9988: Database revision for AuthorityMergeLimit (5.57 KB, patch)
2017-01-18 13:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add Koha objects for table need_merge_authorities (5.79 KB, patch)
2017-01-18 13:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Database revision for AuthorityMergeLimit (5.54 KB, patch)
2017-02-15 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Table revision for need_merge_authorities (2.99 KB, patch)
2017-02-15 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: DBIx schema changes for need_merge_authorities (1.99 KB, patch)
2017-02-15 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add Koha objects for table need_merge_authorities (8.35 KB, patch)
2017-02-15 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Database revision for AuthorityMergeLimit (5.84 KB, patch)
2017-02-23 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Table revision for need_merge_authorities (3.12 KB, patch)
2017-02-23 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: DBIx schema changes for need_merge_authorities (2.23 KB, patch)
2017-02-23 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add Koha objects for table need_merge_authorities (9.23 KB, patch)
2017-02-23 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add extract_biblionumber to Koha/SearchEngine (6.21 KB, patch)
2017-02-23 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add get_usage_count and linked_biblionumbers to Koha::Authority (9.43 KB, patch)
2017-02-23 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Merge should have a parameter hash (7.45 KB, patch)
2017-02-23 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Remove the Zebra code from sub merge (11.84 KB, patch)
2017-02-23 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Check the merge limit in sub merge (14.41 KB, patch)
2017-02-23 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Remove further references to dontmerge (4.45 KB, patch)
2017-02-23 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Refactor the cron script (12.04 KB, patch)
2017-02-23 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Fix a small last minute bug in merge (1.92 KB, patch)
2017-02-23 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add the cronjob and print an upgrade message (2.98 KB, patch)
2017-02-23 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Final polishing (2.76 KB, patch)
2017-02-23 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Merge should have a parameter hash (12.14 KB, patch)
2017-02-25 15:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Remove the Zebra code from sub merge (11.85 KB, patch)
2017-02-25 15:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Check the merge limit in sub merge (14.40 KB, patch)
2017-02-25 15:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Remove further references to dontmerge (4.45 KB, patch)
2017-02-25 15:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add a subtest for specific postponed merge issues (3.35 KB, patch)
2017-02-25 15:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Few subtle changes for postponed merge (7.21 KB, patch)
2017-02-25 15:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Refactor the cron script (11.14 KB, patch)
2017-02-25 15:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add the cronjob and print an upgrade message (2.98 KB, patch)
2017-02-25 15:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add a subtest for specific postponed merge issues (3.58 KB, patch)
2017-03-20 14:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Few subtle changes for postponed merge (8.07 KB, patch)
2017-03-20 14:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Refactor the cron script (11.14 KB, patch)
2017-03-20 14:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add the cronjob and print an upgrade message (2.98 KB, patch)
2017-03-20 14:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Database revision for AuthorityMergeLimit (5.84 KB, patch)
2017-04-03 08:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Table revision for need_merge_authorities (3.12 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: DBIx schema changes for need_merge_authorities (2.23 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add Koha objects for table need_merge_authorities (9.23 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add extract_biblionumber to Koha/SearchEngine (6.21 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add get_usage_count and linked_biblionumbers to Koha::Authority (9.43 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Merge should have a parameter hash (12.15 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Remove the Zebra code from sub merge (11.85 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Check the merge limit in sub merge (14.40 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Remove further references to dontmerge (4.46 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add a subtest for specific postponed merge issues (3.58 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Few subtle changes for postponed merge (8.07 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Refactor the cron script (11.14 KB, patch)
2017-04-03 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Add the cronjob and print an upgrade message (2.98 KB, patch)
2017-04-03 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Database revision for AuthorityMergeLimit (5.93 KB, patch)
2017-04-05 14:01 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Table revision for need_merge_authorities (3.21 KB, patch)
2017-04-05 14:02 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: DBIx schema changes for need_merge_authorities (2.33 KB, patch)
2017-04-05 14:03 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Add Koha objects for table need_merge_authorities (9.33 KB, patch)
2017-04-05 14:08 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Add extract_biblionumber to Koha/SearchEngine (6.30 KB, patch)
2017-04-05 14:09 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Add get_usage_count and linked_biblionumbers to Koha::Authority (9.52 KB, patch)
2017-04-05 14:11 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Merge should have a parameter hash (12.24 KB, patch)
2017-04-05 14:13 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Remove the Zebra code from sub merge (11.94 KB, patch)
2017-04-05 14:14 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Check the merge limit in sub merge (14.50 KB, patch)
2017-04-05 14:15 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Remove further references to dontmerge (4.55 KB, patch)
2017-04-05 14:16 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Add a subtest for specific postponed merge issues (3.67 KB, patch)
2017-04-05 14:17 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Few subtle changes for postponed merge (8.16 KB, patch)
2017-04-05 14:19 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Refactor the cron script (11.23 KB, patch)
2017-04-05 14:20 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Add the cronjob and print an upgrade message (3.07 KB, patch)
2017-04-05 14:21 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 9988: Database revision for AuthorityMergeLimit (5.98 KB, patch)
2017-04-05 20:09 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Table revision for need_merge_authorities (3.27 KB, patch)
2017-04-05 20:09 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: DBIx schema changes for need_merge_authorities (2.38 KB, patch)
2017-04-05 20:09 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Add Koha objects for table need_merge_authorities (9.38 KB, patch)
2017-04-05 20:09 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Add extract_biblionumber to Koha/SearchEngine (6.36 KB, patch)
2017-04-05 20:09 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Add get_usage_count and linked_biblionumbers to Koha::Authority (9.58 KB, patch)
2017-04-05 20:09 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Merge should have a parameter hash (12.31 KB, patch)
2017-04-05 20:09 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Remove the Zebra code from sub merge (12.00 KB, patch)
2017-04-05 20:10 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Check the merge limit in sub merge (14.55 KB, patch)
2017-04-05 20:10 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Remove further references to dontmerge (4.61 KB, patch)
2017-04-05 20:10 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Add a subtest for specific postponed merge issues (3.73 KB, patch)
2017-04-05 20:10 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Few subtle changes for postponed merge (8.22 KB, patch)
2017-04-05 20:10 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Refactor the cron script (11.29 KB, patch)
2017-04-05 20:10 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: Add the cronjob and print an upgrade message (3.13 KB, patch)
2017-04-05 20:11 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 9988: [QA Follow-up] Satisfy QA issues (6.44 KB, patch)
2017-04-11 14:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9988: Database revision for AuthorityMergeLimit (6.06 KB, patch)
2017-04-13 08:48 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Table revision for need_merge_authorities (3.33 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: DBIx schema changes for need_merge_authorities (2.44 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Add Koha objects for table need_merge_authorities (9.46 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Add extract_biblionumber to Koha/SearchEngine (6.44 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Add get_usage_count and linked_biblionumbers to Koha::Authority (9.65 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Merge should have a parameter hash (12.41 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Remove the Zebra code from sub merge (12.07 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Check the merge limit in sub merge (14.63 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Remove further references to dontmerge (4.68 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Add a subtest for specific postponed merge issues (3.79 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Few subtle changes for postponed merge (8.29 KB, patch)
2017-04-13 08:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Refactor the cron script (11.37 KB, patch)
2017-04-13 08:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: Add the cronjob and print an upgrade message (3.20 KB, patch)
2017-04-13 08:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 9988: [QA Follow-up] Satisfy QA issues (6.47 KB, patch)
2017-04-13 08:50 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2013-04-04 08:55:27 UTC
When saving an authority used in many biblios, Koha may have a timeout due to updating all those bib records.
But in case I just update e.g. a 551 tag for the auth record, there is no need at all to update all those bib records.

We should have a smarter merge function in C4/AuthoritiesMarc.pm.
Note that you could use the pref to move updating all biblios to a cron job. Wouldn't it be nicer if we could do something in between? If we really need to update a handful of records, do it rightaway. If we really (!!) need to update 2000 records (in my example Koha is updating 2000 records for nothing), we could perhaps leave that to a cron job.
Comment 1 Jonathan Druart 2015-04-14 11:24:32 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2015-04-15 11:55:20 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-01-14 10:55:56 UTC
(In reply to Marcel de Rooy from comment #0)
> But in case I just update e.g. a 551 tag for the auth record, there is no
> need at all to update all those bib records.

Will leave this case out. Sometimes we may expect to trigger an authority merge by saving an authority, and perhaps do not want a 'smart' merge to do nothing.
Comment 4 Marcel de Rooy 2017-01-14 11:04:56 UTC
*** Bug 5821 has been marked as a duplicate of this bug. ***
Comment 5 Marcel de Rooy 2017-01-14 12:14:43 UTC
*** Bug 12791 has been marked as a duplicate of this bug. ***
Comment 6 Marcel de Rooy 2017-01-18 13:02:30 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2017-01-18 13:03:51 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2017-01-18 13:06:48 UTC
Just a run-up. More patches coming, but needing rebase on other work coming from the omnibus bug 17908.
Comment 9 Marcel de Rooy 2017-02-15 13:46:15 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2017-02-15 13:46:19 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2017-02-15 13:46:23 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2017-02-15 13:46:27 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2017-02-23 12:15:31 UTC
Created attachment 60594 [details] [review]
Bug 9988: Database revision for AuthorityMergeLimit

Introduce the new preference and remove dontmerge.
If dontmerge was enabled, the new pref will be set to zero and we will
print a message about raising this value now.

Test plan:
Run the database revision or a new install.
In case of an upgrade, check the new pref value. Should be 0 when
dontmerge was enabled, otherwise 50.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested sysprefs.sql on a new database.
Comment 14 Marcel de Rooy 2017-02-23 12:15:35 UTC
Created attachment 60595 [details] [review]
Bug 9988: Table revision for need_merge_authorities

In order to let merge do exactly the same when called from the merge
cron job, we need three additional fields in the table. The use of these
fields will be explained in a follow-up.

Test plan:
Run the dbrev (updatestructure step in webinstaller).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested running kohastructure.sql for this table.
Comment 15 Marcel de Rooy 2017-02-23 12:15:39 UTC
Created attachment 60596 [details] [review]
Bug 9988: DBIx schema changes for need_merge_authorities

As generated by misc/devel/update_dbix_class_files.pl.
No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2017-02-23 12:15:43 UTC
Created attachment 60597 [details] [review]
Bug 9988: Add Koha objects for table need_merge_authorities

This patch adds two Koha objects: MergeRequest(s).
MergeRequest has a new method and an oldmarc method.
A class method reporting_tag_xml is added to MergeRequests.pm.
All new routines are tested in Authorities.t.

Removes a few unneeded modules from Koha::Authority.

Test plan:
Run t/db_dependent/Koha/Authorities.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2017-02-23 12:15:47 UTC
Created attachment 60598 [details] [review]
Bug 9988: Add extract_biblionumber to Koha/SearchEngine

When we will replace the Zebra code in sub merge, we will call SearchEngine
to pass records and we need a routine to extract a biblionumber from
a search result record. A record from Zebra still must be converted to
MARC::Record. This is no longer needed for a ElasticSearch record.

Test plan:
Run t/db_dependent/Koha/SearchEngine/Search.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2017-02-23 12:15:51 UTC
Created attachment 60599 [details] [review]
Bug 9988: Add get_usage_count and linked_biblionumbers to Koha::Authority

When replacing the Zebra code in sub merge, we actually need CountUsage
as well as the results it gets from SearchEngine.

In order to get count and/or results, we now create:

[1] instance methods get_usage_count and linked_biblionumbers in
    Koha::Authority,
[2] class methods of the same name in Koha::Authorities.

The instance method calls the class method. The class method can be used
for deleted authority records, and in 'legacy calls' before refactoring.

Note: On BZ 18149 we will replace all CountUsage calls.

Test plan:
Run t/db_dependent/Koha/Authorities.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2017-02-23 12:15:55 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2017-02-23 12:15:59 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2017-02-23 12:16:03 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2017-02-23 12:16:07 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2017-02-23 12:16:11 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2017-02-23 12:16:15 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2017-02-23 12:16:19 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2017-02-23 12:16:23 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2017-02-23 12:25:33 UTC
If you apply all patches, you may want to use this COMBINED test script:

Run the three db revs.
Run tests:
t/db_dependent/Koha/Authorities.t
t/db_dependent/Authorities/Merge.t
t/db_dependent/Authorities/MergeRequests.t
t/db_dependent/Koha/SearchEngine/Search.t
t/db_dependent/UsageStats.t
Follow test plan: Check the merge limit in sub merge
Follow test plan: Refactor the cron script
Comment 28 Marcel de Rooy 2017-02-23 20:57:40 UTC Comment hidden (obsolete)
Comment 29 Marcel de Rooy 2017-02-25 15:21:46 UTC
Created attachment 60668 [details] [review]
Bug 9988: Merge should have a parameter hash

We will need a few additional parameters for merge later on. This patch
puts the original parameters in a parameter hash.
For the same reason DelAuthority gets a parameter hash here.

Note: We remove the second parameter from the DelAuthority call in
authorities/authorities-home.pl here. It was not used and could have
presented problems in the future.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t.
[2] Run t/db_dependent/Authorities/Merge.t.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2017-02-25 15:21:51 UTC
Created attachment 60669 [details] [review]
Bug 9988: Remove the Zebra code from sub merge

Since we can now call linked_biblionumbers, we can now remove all Zebra
related code from merge. We also add a parameter biblionumbers; we use it
in the test now, but it may be handy too later in the maintenance script
when we want to trigger a merge for specific biblionumber(s). See bug
report 18071.

All mocks for ZOOM, Context::Zconn, Search::new_record_for_zebra in the
merge test can now be replaced by one mock for linked_biblionumbers. Note
that we test the biblionumbers parameter in the last subtest without that
mock.

Remove unused vars $countunmodifiedbiblio, $counterrors from merge.
Renamed zebrarecords to linkedrecords in the Merge test.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Modify an authority record. Check the linked biblio records.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2017-02-25 15:21:55 UTC
Created attachment 60670 [details] [review]
Bug 9988: Check the merge limit in sub merge

At this point, we are replacing dontmerge functionality by the new
AuthorityMergeLimit logic. Instead of doing this check before calling
merge, we just call merge and check it there. In order to let the cron
job do the larger (postponed) merges, we add a parameter override_limit.

A subtest is added in Merge.t to test the 'postponed merge' feature. Since
merge now also calls get_usage_count, an additional mock is added. All
references to dontmerge are removed.

In merge two lines, initializing $dbh and $counteditbiblios, are moved.
The dontmerge test in DelAuthority and ModAuthority is removed. Since this
did not leave much in ModAuthority, I fixed the whitespace on the remaining
lines rightaway (yes, I know).
A minimal set of changes is applied to the cron script; it will get further
attention on a next patch.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Set AuthorityMergeLimit to 2. Modify an authority with two linked
    biblios. Check that the merge was done immediately.
[3] Now modify an authority with more than 2 linked records.
    Verify that the merge was postponed; a record must be inserted in
    the need_merge_authorities table.
[4] Testing of the merge cron job is *postponed* to a next patch.
    Note: I tested a modification, but the script just needs more
    attention.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2017-02-25 15:21:59 UTC
Created attachment 60671 [details] [review]
Bug 9988: Remove further references to dontmerge

[1] The preference was sent to HEA. We can now send both AuthorityMergeMode
    as well as AuthorityMergeLimit.
[2] A comment in authorities/merge.pl is removed. Note that a subsequent
    patch will modify and test the cron job.
[3] Script misc/batchRebuildItemsTables.pl temporarily enabled dontmerge.
    This is equivalent to setting the mergelimit to zero.
    The function defnonull is no longer needed. (If the pref was NULL,
    we restore that value. Sub merge won't mind.)

Test plan:
[1] Run t/db_dependent/UsageStats.t
[2] Run misc/batchRebuildItemsTables.pl -t
    This just ensures you it still compiles; the changes speak for itself.
[3] Now git grep on dontmerge.
    You should only find hits in atomicupdate and misc/translator/po.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2017-02-25 15:22:03 UTC
Created attachment 60672 [details] [review]
Bug 9988: Add a subtest for specific postponed merge issues

This subtest shows that we need a few little tweaks to make merge
work in some specific postponed merge situations.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should show three fails to be fixed in the next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2017-02-25 15:22:09 UTC
Created attachment 60673 [details] [review]
Bug 9988: Few subtle changes for postponed merge

The fails in the previous test showed that we need the first three
changes here. Some final polishing in points 4 to 6.

[1] Sub merge: Refine the condition for initializing $tags_new.
    A postponed 'modify'-merge (A to B) makes that $authtypefrom is not
    defined when running merge later. When crossing the type boundary, we
    need a new field too.
[2] Sub merge: Add condition for an empty @record_to array.
    This indicates also that a field should be removed, since we should
    otherwise only add a $9 subfield.
[3] DelAuthority: Adding a skipmerge parameter to allow the call from
    authorities/merge.pl to skip an unneeded merge.
    This also prevents that the 'delete-merge' would precede the
    'modify-merge' under a hypothetical race condition.
[4] DelAuthority: There is actually no need to call GetAuthority.
    The subfields of the old record are not relevant in this case.
[5] Added a few POD lines to merge.
[6] Removed a trailing space in a comment line in merge.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should no longer fail now.
[2] See test plan of next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2017-02-25 15:22:13 UTC
Created attachment 60674 [details] [review]
Bug 9988: Refactor the cron script

The cron job is moved from migration tools to cronjobs. And renamed to
a plural form. The script is now based on Koha objects. It does no longer
include the code to merge one record. This can be done via the interface,
and will be added to a maintenance script on bug 18071. Should not be part
of this cron job.

Adding a cron_cleanup method to MergeRequests; this method is called from
the cron script to reset older entries still marked in progress and to
also remove old processed entries. Tested in a separate unit test.

Test plan:
[1] Run t/db_dependent/Authorities/MergeRequests.t
[2] Set AuthorityMergeLimit to 0. (All merges are postponed.)
[3] Modify an authority linked to a few records.
[4] Delete an authority linked to a few records with batch delete tool.
[5] And select two auth records with linked records.
    Merge these two records with authority/merge.pl.
    Note: Do not select Default. See also bug 17380.
[6] Check the need_merge_authorities table for inserted records.
[7] Run misc/cronjobs/merge_authorities.pl -b and inspect the linked
    records and the record status in need_merge_authorities.
Comment 36 Marcel de Rooy 2017-02-25 15:22:18 UTC
Created attachment 60675 [details] [review]
Bug 9988: Add the cronjob and print an upgrade message

The Debian cron file and the misc crontab example are updated.
A message is printed when upgrading.

Note: At this moment the merge cron job is run once a day. This is imo a
good starting point. The load for this job greatly depends on the value of
pref AuthorityMergeLimit. Of course you can schedule the job more often,
and if this need is felt more globally, we can adjust it later.

Test plan:
[1] Run the dbrev and see the message.
[2] Read the changes to the cron files.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2017-02-25 15:27:13 UTC
Back into NSO. Please use the COMBINED test script in comment27.
Comment 38 David Cook 2017-03-01 02:46:40 UTC
Sounds like another use case for having a task scheduler and message queue in the background...
Comment 39 Jacek Ablewicz 2017-03-20 10:48:00 UTC
For "real" merge usage case (merging two different authority records), if the merge gets postponed, when merge() is called later by the cron job, authority record with $mergefrom ID is no longer there (it got deleted from the database by authorities/merge.pl script ?). This search in postponed merge() call

   my $authfrom = Koha::Authorities->find($mergefrom);

will fail, and $authtypefrom | $auth_tag_to_report_from | @record_from would be empty. I.e.: old authority record heading, stored in the merge requests table will never be used.

(but there is a strong possibility I'm very much mistaken about that, authorities-related code got changed quite a lot lately).
Comment 40 Marcel de Rooy 2017-03-20 10:53:10 UTC
(In reply to Jacek Ablewicz from comment #39)
> For "real" merge usage case (merging two different authority records), if
> the merge gets postponed, when merge() is called later by the cron job,
> authority record with $mergefrom ID is no longer there (it got deleted from
> the database by authorities/merge.pl script ?). This search in postponed
> merge() call
> 
>    my $authfrom = Koha::Authorities->find($mergefrom);
> 
> will fail, and $authtypefrom | $auth_tag_to_report_from | @record_from would
> be empty. I.e.: old authority record heading, stored in the merge requests
> table will never be used.
> 
> (but there is a strong possibility I'm very much mistaken about that,
> authorities-related code got changed quite a lot lately).

The code is designed to overcome this situation. There are tests simulating it too.
Take a look :)
Comment 41 Marcel de Rooy 2017-03-20 10:54:58 UTC
Especially the "Few subtle changes" patch makes a few additional refinements in this regard.
Comment 42 Jacek Ablewicz 2017-03-20 11:46:36 UTC
(In reply to Marcel de Rooy from comment #41)
> Especially the "Few subtle changes" patch makes a few additional refinements
> in this regard.

I can see an accommodation in this commit for missing $authtypefrom (for a postponed merge, when the old record is already deleted), but it doesn't look quite relevant to empty @record_from ;). I think there may be a problem in the code which is located above that instead, namely

    my $authfrom = Koha::Authorities->find($mergefrom);
    my $authtypefrom = $authfrom ? Koha::Authority::Types->find($authfrom->authtypecode) : undef;
    my $auth_tag_to_report_from = $authtypefrom ? $authtypefrom->auth_tag_to_report : '';
    my @record_from;
    @record_from = $MARCfrom->field($auth_tag_to_report_from)->subfields() if $auth_tag_to_report_from && $MARCfrom && $MARCfrom->field($auth_tag_to_report_from);

If the search

    my $authfrom = Koha::Authorities->find($mergefrom);

fails (I'm guessing it will fail, for the postponed "real" merge ?), $MARCfrom will be ignored, no matter what (it's not used anywhere else in the merge() subroutine) and @record_from will be empty.
Comment 43 Marcel de Rooy 2017-03-20 12:18:57 UTC
I tested this quite extensively. So I am sure it does not fail :) But I welcome any interest in this patch set..

Please look at the merge call in the updated cron job:
+        merge({
+            mergefrom => $req->authid,
+            MARCfrom => scalar $req->oldmarc,
+            mergeto => $req->authid_new,
+            MARCto => $newmarc,
+            override_limit => 1,
+        });

MARCfrom is coming from the adjusted need_merge_authorities table. It contains the report tag (in marcxml).
For creation of that record, see Koha::Authority::MergeRequest->new.
Comment 44 Marcel de Rooy 2017-03-20 12:31:51 UTC
(In reply to Jacek Ablewicz from comment #42)
> (In reply to Marcel de Rooy from comment #41)
> If the search
> 
>     my $authfrom = Koha::Authorities->find($mergefrom);
> 
> fails (I'm guessing it will fail, for the postponed "real" merge ?),
> $MARCfrom will be ignored, no matter what (it's not used anywhere else in
> the merge() subroutine) and @record_from will be empty.

Hmm Some doubts starts to arise. Will dig a little bit further..
Comment 45 Marcel de Rooy 2017-03-20 12:40:52 UTC
Blocking temporarily
Comment 46 Marcel de Rooy 2017-03-20 12:51:48 UTC
(In reply to Jacek Ablewicz from comment #42)
> fails (I'm guessing it will fail, for the postponed "real" merge ?),
> $MARCfrom will be ignored, no matter what (it's not used anywhere else in
> the merge() subroutine) and @record_from will be empty.

Good catch. There still is a "subtle" bug.
Although I send the MARCfrom record and it is only used for possibly clearing out a subfield, it is ignored indeed.
Thinking about an economic way to repair this..
Comment 47 Marcel de Rooy 2017-03-20 13:12:19 UTC
This should be enough.
But will add a unit test too ;)

+    if( !$authfrom && $MARCfrom && $MARCfrom->field('1..') ) { # postponed merge, authfrom was deleted
+        @record_from = ( $MARCfrom->field('1..') )[0]->subfields;
+    } elsif( $auth_tag_to_report_from && $MARCfrom && $MARCfrom->field($auth_tag_to_report_from) ) {
+        @record_from = $MARCfrom->field($auth_tag_to_report_from)->subfields;
+    }
Comment 48 Marcel de Rooy 2017-03-20 13:15:28 UTC
And for UNIMARC it should be 2xx
Comment 49 Jacek Ablewicz 2017-03-20 13:40:16 UTC
(In reply to Marcel de Rooy from comment #48)
> And for UNIMARC it should be 2xx

Looks like the stored one is constructed as one-field-record, so the last (...->[-1]) field in this record should always (?) be heading field. Original tag number can be extracted from it as well, but authtypecode can't..
Comment 50 Marcel de Rooy 2017-03-20 14:25:50 UTC
Created attachment 61306 [details] [review]
Bug 9988: Add a subtest for specific postponed merge issues

This subtest shows that we need a few little tweaks to make merge
work in some specific postponed merge situations.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should fail.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Marcel de Rooy 2017-03-20 14:25:58 UTC
Created attachment 61307 [details] [review]
Bug 9988: Few subtle changes for postponed merge

The fails in the previous test showed that we need the first three
changes here. Some final polishing in points 4 to 6.

[1] Sub merge: Refine the condition for initializing $tags_new.
    A postponed 'modify'-merge (A to B) makes that $authtypefrom is not
    defined when running merge later. When crossing the type boundary, we
    need a new field too.
[2] Sub merge: Add condition for an empty @record_to array.
    This indicates also that a field should be removed, since we should
    otherwise only add a $9 subfield.
[3] Sub merge: Adjust initializing @record_from.
    This change is tested by verifying a cleared subfield in the test.
[4] DelAuthority: Adding a skipmerge parameter to allow the call from
    authorities/merge.pl to skip an unneeded merge.
    This also prevents that the 'delete-merge' would precede the
    'modify-merge' under a hypothetical race condition.
[5] DelAuthority: There is actually no need to call GetAuthority.
    The subfields of the old record are not relevant in this case.
[6] Added a few POD lines to merge.
[7] Removed a trailing space in a comment line in merge.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should no longer fail now.
[2] See test plan of next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Marcel de Rooy 2017-03-20 14:26:03 UTC
Created attachment 61308 [details] [review]
Bug 9988: Refactor the cron script

The cron job is moved from migration tools to cronjobs. And renamed to
a plural form. The script is now based on Koha objects. It does no longer
include the code to merge one record. This can be done via the interface,
and will be added to a maintenance script on bug 18071. Should not be part
of this cron job.

Adding a cron_cleanup method to MergeRequests; this method is called from
the cron script to reset older entries still marked in progress and to
also remove old processed entries. Tested in a separate unit test.

Test plan:
[1] Run t/db_dependent/Authorities/MergeRequests.t
[2] Set AuthorityMergeLimit to 0. (All merges are postponed.)
[3] Modify an authority linked to a few records.
[4] Delete an authority linked to a few records with batch delete tool.
[5] And select two auth records with linked records.
    Merge these two records with authority/merge.pl.
    Note: Do not select Default. See also bug 17380.
[6] Check the need_merge_authorities table for inserted records.
[7] Run misc/cronjobs/merge_authorities.pl -b and inspect the linked
    records and the record status in need_merge_authorities.
Comment 53 Marcel de Rooy 2017-03-20 14:26:09 UTC
Created attachment 61309 [details] [review]
Bug 9988: Add the cronjob and print an upgrade message

The Debian cron file and the misc crontab example are updated.
A message is printed when upgrading.

Note: At this moment the merge cron job is run once a day. This is imo a
good starting point. The load for this job greatly depends on the value of
pref AuthorityMergeLimit. Of course you can schedule the job more often,
and if this need is felt more globally, we can adjust it later.

Test plan:
[1] Run the dbrev and see the message.
[2] Read the changes to the cron files.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 54 Marcel de Rooy 2017-03-20 14:28:08 UTC
Patches "Add a subtest for specific postponed merge issues", and "Few subtle changes for postponed merge" have been slightly adjusted to catch a bug as reported by Jacek.
The unit is adjusted to also include this case.
Thanks, Jacek!
Comment 55 Marcel de Rooy 2017-04-03 08:28:51 UTC
Rebasing this one now
Comment 56 Marcel de Rooy 2017-04-03 08:30:57 UTC
Created attachment 61779 [details] [review]
Bug 9988: Database revision for AuthorityMergeLimit

Introduce the new preference and remove dontmerge.
If dontmerge was enabled, the new pref will be set to zero and we will
print a message about raising this value now.

Test plan:
Run the database revision or a new install.
In case of an upgrade, check the new pref value. Should be 0 when
dontmerge was enabled, otherwise 50.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested sysprefs.sql on a new database.
Comment 57 Marcel de Rooy 2017-04-03 08:31:03 UTC
Created attachment 61780 [details] [review]
Bug 9988: Table revision for need_merge_authorities

In order to let merge do exactly the same when called from the merge
cron job, we need three additional fields in the table. The use of these
fields will be explained in a follow-up.

Test plan:
Run the dbrev (updatestructure step in webinstaller).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested running kohastructure.sql for this table.
Comment 58 Marcel de Rooy 2017-04-03 08:31:08 UTC
Created attachment 61781 [details] [review]
Bug 9988: DBIx schema changes for need_merge_authorities

As generated by misc/devel/update_dbix_class_files.pl.
No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 59 Marcel de Rooy 2017-04-03 08:31:12 UTC
Created attachment 61782 [details] [review]
Bug 9988: Add Koha objects for table need_merge_authorities

This patch adds two Koha objects: MergeRequest(s).
MergeRequest has a new method and an oldmarc method.
A class method reporting_tag_xml is added to MergeRequests.pm.
All new routines are tested in Authorities.t.

Removes a few unneeded modules from Koha::Authority.

Test plan:
Run t/db_dependent/Koha/Authorities.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 60 Marcel de Rooy 2017-04-03 08:31:17 UTC
Created attachment 61783 [details] [review]
Bug 9988: Add extract_biblionumber to Koha/SearchEngine

When we will replace the Zebra code in sub merge, we will call SearchEngine
to pass records and we need a routine to extract a biblionumber from
a search result record. A record from Zebra still must be converted to
MARC::Record. This is no longer needed for a ElasticSearch record.

Test plan:
Run t/db_dependent/Koha/SearchEngine/Search.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 61 Marcel de Rooy 2017-04-03 08:31:22 UTC
Created attachment 61784 [details] [review]
Bug 9988: Add get_usage_count and linked_biblionumbers to Koha::Authority

When replacing the Zebra code in sub merge, we actually need CountUsage
as well as the results it gets from SearchEngine.

In order to get count and/or results, we now create:

[1] instance methods get_usage_count and linked_biblionumbers in
    Koha::Authority,
[2] class methods of the same name in Koha::Authorities.

The instance method calls the class method. The class method can be used
for deleted authority records, and in 'legacy calls' before refactoring.

Note: On BZ 18149 we will replace all CountUsage calls.

Test plan:
Run t/db_dependent/Koha/Authorities.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Marcel de Rooy 2017-04-03 08:31:27 UTC
Created attachment 61785 [details] [review]
Bug 9988: Merge should have a parameter hash

We will need a few additional parameters for merge later on. This patch
puts the original parameters in a parameter hash.
For the same reason DelAuthority gets a parameter hash here.

Note: We remove the second parameter from the DelAuthority call in
authorities/authorities-home.pl here. It was not used and could have
presented problems in the future.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t.
[2] Run t/db_dependent/Authorities/Merge.t.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 63 Marcel de Rooy 2017-04-03 08:31:32 UTC
Created attachment 61786 [details] [review]
Bug 9988: Remove the Zebra code from sub merge

Since we can now call linked_biblionumbers, we can now remove all Zebra
related code from merge. We also add a parameter biblionumbers; we use it
in the test now, but it may be handy too later in the maintenance script
when we want to trigger a merge for specific biblionumber(s). See bug
report 18071.

All mocks for ZOOM, Context::Zconn, Search::new_record_for_zebra in the
merge test can now be replaced by one mock for linked_biblionumbers. Note
that we test the biblionumbers parameter in the last subtest without that
mock.

Remove unused vars $countunmodifiedbiblio, $counterrors from merge.
Renamed zebrarecords to linkedrecords in the Merge test.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Modify an authority record. Check the linked biblio records.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 64 Marcel de Rooy 2017-04-03 08:31:37 UTC
Created attachment 61787 [details] [review]
Bug 9988: Check the merge limit in sub merge

At this point, we are replacing dontmerge functionality by the new
AuthorityMergeLimit logic. Instead of doing this check before calling
merge, we just call merge and check it there. In order to let the cron
job do the larger (postponed) merges, we add a parameter override_limit.

A subtest is added in Merge.t to test the 'postponed merge' feature. Since
merge now also calls get_usage_count, an additional mock is added. All
references to dontmerge are removed.

In merge two lines, initializing $dbh and $counteditbiblios, are moved.
The dontmerge test in DelAuthority and ModAuthority is removed. Since this
did not leave much in ModAuthority, I fixed the whitespace on the remaining
lines rightaway (yes, I know).
A minimal set of changes is applied to the cron script; it will get further
attention on a next patch.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Set AuthorityMergeLimit to 2. Modify an authority with two linked
    biblios. Check that the merge was done immediately.
[3] Now modify an authority with more than 2 linked records.
    Verify that the merge was postponed; a record must be inserted in
    the need_merge_authorities table.
[4] Testing of the merge cron job is *postponed* to a next patch.
    Note: I tested a modification, but the script just needs more
    attention.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 65 Marcel de Rooy 2017-04-03 08:31:43 UTC
Created attachment 61788 [details] [review]
Bug 9988: Remove further references to dontmerge

[1] The preference was sent to HEA. We can now send both AuthorityMergeMode
    as well as AuthorityMergeLimit.
[2] A comment in authorities/merge.pl is removed. Note that a subsequent
    patch will modify and test the cron job.
[3] Script misc/batchRebuildItemsTables.pl temporarily enabled dontmerge.
    This is equivalent to setting the mergelimit to zero.
    The function defnonull is no longer needed. (If the pref was NULL,
    we restore that value. Sub merge won't mind.)

Test plan:
[1] Run t/db_dependent/UsageStats.t
[2] Run misc/batchRebuildItemsTables.pl -t
    This just ensures you it still compiles; the changes speak for itself.
[3] Now git grep on dontmerge.
    You should only find hits in atomicupdate and misc/translator/po.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 66 Marcel de Rooy 2017-04-03 08:31:48 UTC
Created attachment 61789 [details] [review]
Bug 9988: Add a subtest for specific postponed merge issues

This subtest shows that we need a few little tweaks to make merge
work in some specific postponed merge situations.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should fail.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Marcel de Rooy 2017-04-03 08:31:52 UTC
Created attachment 61790 [details] [review]
Bug 9988: Few subtle changes for postponed merge

The fails in the previous test showed that we need the first three
changes here. Some final polishing in points 4 to 6.

[1] Sub merge: Refine the condition for initializing $tags_new.
    A postponed 'modify'-merge (A to B) makes that $authtypefrom is not
    defined when running merge later. When crossing the type boundary, we
    need a new field too.
[2] Sub merge: Add condition for an empty @record_to array.
    This indicates also that a field should be removed, since we should
    otherwise only add a $9 subfield.
[3] Sub merge: Adjust initializing @record_from.
    This change is tested by verifying a cleared subfield in the test.
[4] DelAuthority: Adding a skipmerge parameter to allow the call from
    authorities/merge.pl to skip an unneeded merge.
    This also prevents that the 'delete-merge' would precede the
    'modify-merge' under a hypothetical race condition.
[5] DelAuthority: There is actually no need to call GetAuthority.
    The subfields of the old record are not relevant in this case.
[6] Added a few POD lines to merge.
[7] Removed a trailing space in a comment line in merge.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should no longer fail now.
[2] See test plan of next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 68 Marcel de Rooy 2017-04-03 08:31:57 UTC
Created attachment 61791 [details] [review]
Bug 9988: Refactor the cron script

The cron job is moved from migration tools to cronjobs. And renamed to
a plural form. The script is now based on Koha objects. It does no longer
include the code to merge one record. This can be done via the interface,
and will be added to a maintenance script on bug 18071. Should not be part
of this cron job.

Adding a cron_cleanup method to MergeRequests; this method is called from
the cron script to reset older entries still marked in progress and to
also remove old processed entries. Tested in a separate unit test.

Test plan:
[1] Run t/db_dependent/Authorities/MergeRequests.t
[2] Set AuthorityMergeLimit to 0. (All merges are postponed.)
[3] Modify an authority linked to a few records.
[4] Delete an authority linked to a few records with batch delete tool.
[5] And select two auth records with linked records.
    Merge these two records with authority/merge.pl.
    Note: Do not select Default. See also bug 17380.
[6] Check the need_merge_authorities table for inserted records.
[7] Run misc/cronjobs/merge_authorities.pl -b and inspect the linked
    records and the record status in need_merge_authorities.
Comment 69 Marcel de Rooy 2017-04-03 08:32:02 UTC
Created attachment 61792 [details] [review]
Bug 9988: Add the cronjob and print an upgrade message

The Debian cron file and the misc crontab example are updated.
A message is printed when upgrading.

Note: At this moment the merge cron job is run once a day. This is imo a
good starting point. The load for this job greatly depends on the value of
pref AuthorityMergeLimit. Of course you can schedule the job more often,
and if this need is felt more globally, we can adjust it later.

Test plan:
[1] Run the dbrev and see the message.
[2] Read the changes to the cron files.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 70 Marc Véron 2017-04-05 13:56:06 UTC
Went through test plans from comment #68 and comment #69, works as described.
Comment 71 Marc Véron 2017-04-05 14:01:51 UTC
Created attachment 61890 [details] [review]
Bug 9988: Database revision for AuthorityMergeLimit

Introduce the new preference and remove dontmerge.
If dontmerge was enabled, the new pref will be set to zero and we will
print a message about raising this value now.

Test plan:
Run the database revision or a new install.
In case of an upgrade, check the new pref value. Should be 0 when
dontmerge was enabled, otherwise 50.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested sysprefs.sql on a new database.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 72 Marc Véron 2017-04-05 14:02:46 UTC
Created attachment 61891 [details] [review]
Bug 9988: Table revision for need_merge_authorities

In order to let merge do exactly the same when called from the merge
cron job, we need three additional fields in the table. The use of these
fields will be explained in a follow-up.

Test plan:
Run the dbrev (updatestructure step in webinstaller).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested running kohastructure.sql for this table.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 73 Marc Véron 2017-04-05 14:03:42 UTC
Created attachment 61892 [details] [review]
Bug 9988: DBIx schema changes for need_merge_authorities

As generated by misc/devel/update_dbix_class_files.pl.
No test plan.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 74 Marc Véron 2017-04-05 14:08:41 UTC
Created attachment 61893 [details] [review]
Bug 9988: Add Koha objects for table need_merge_authorities

This patch adds two Koha objects: MergeRequest(s).
MergeRequest has a new method and an oldmarc method.
A class method reporting_tag_xml is added to MergeRequests.pm.
All new routines are tested in Authorities.t.

Removes a few unneeded modules from Koha::Authority.

Test plan:
Run t/db_dependent/Koha/Authorities.t

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 75 Marc Véron 2017-04-05 14:09:52 UTC
Created attachment 61894 [details] [review]
Bug 9988: Add extract_biblionumber to Koha/SearchEngine

When we will replace the Zebra code in sub merge, we will call SearchEngine
to pass records and we need a routine to extract a biblionumber from
a search result record. A record from Zebra still must be converted to
MARC::Record. This is no longer needed for a ElasticSearch record.

Test plan:
Run t/db_dependent/Koha/SearchEngine/Search.t

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 76 Marc Véron 2017-04-05 14:11:15 UTC
Created attachment 61895 [details] [review]
Bug 9988: Add get_usage_count and linked_biblionumbers to Koha::Authority

When replacing the Zebra code in sub merge, we actually need CountUsage
as well as the results it gets from SearchEngine.

In order to get count and/or results, we now create:

[1] instance methods get_usage_count and linked_biblionumbers in
    Koha::Authority,
[2] class methods of the same name in Koha::Authorities.

The instance method calls the class method. The class method can be used
for deleted authority records, and in 'legacy calls' before refactoring.

Note: On BZ 18149 we will replace all CountUsage calls.

Test plan:
Run t/db_dependent/Koha/Authorities.t

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 77 Marc Véron 2017-04-05 14:13:18 UTC
Created attachment 61897 [details] [review]
Bug 9988: Merge should have a parameter hash

We will need a few additional parameters for merge later on. This patch
puts the original parameters in a parameter hash.
For the same reason DelAuthority gets a parameter hash here.

Note: We remove the second parameter from the DelAuthority call in
authorities/authorities-home.pl here. It was not used and could have
presented problems in the future.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t.
[2] Run t/db_dependent/Authorities/Merge.t.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 78 Marc Véron 2017-04-05 14:14:23 UTC
Created attachment 61898 [details] [review]
Bug 9988: Remove the Zebra code from sub merge

Since we can now call linked_biblionumbers, we can now remove all Zebra
related code from merge. We also add a parameter biblionumbers; we use it
in the test now, but it may be handy too later in the maintenance script
when we want to trigger a merge for specific biblionumber(s). See bug
report 18071.

All mocks for ZOOM, Context::Zconn, Search::new_record_for_zebra in the
merge test can now be replaced by one mock for linked_biblionumbers. Note
that we test the biblionumbers parameter in the last subtest without that
mock.

Remove unused vars $countunmodifiedbiblio, $counterrors from merge.
Renamed zebrarecords to linkedrecords in the Merge test.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Modify an authority record. Check the linked biblio records.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 79 Marc Véron 2017-04-05 14:15:32 UTC
Created attachment 61899 [details] [review]
Bug 9988: Check the merge limit in sub merge

At this point, we are replacing dontmerge functionality by the new
AuthorityMergeLimit logic. Instead of doing this check before calling
merge, we just call merge and check it there. In order to let the cron
job do the larger (postponed) merges, we add a parameter override_limit.

A subtest is added in Merge.t to test the 'postponed merge' feature. Since
merge now also calls get_usage_count, an additional mock is added. All
references to dontmerge are removed.

In merge two lines, initializing $dbh and $counteditbiblios, are moved.
The dontmerge test in DelAuthority and ModAuthority is removed. Since this
did not leave much in ModAuthority, I fixed the whitespace on the remaining
lines rightaway (yes, I know).
A minimal set of changes is applied to the cron script; it will get further
attention on a next patch.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Set AuthorityMergeLimit to 2. Modify an authority with two linked
    biblios. Check that the merge was done immediately.
[3] Now modify an authority with more than 2 linked records.
    Verify that the merge was postponed; a record must be inserted in
    the need_merge_authorities table.
[4] Testing of the merge cron job is *postponed* to a next patch.
    Note: I tested a modification, but the script just needs more
    attention.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 80 Marc Véron 2017-04-05 14:16:46 UTC
Created attachment 61900 [details] [review]
Bug 9988: Remove further references to dontmerge

[1] The preference was sent to HEA. We can now send both AuthorityMergeMode
    as well as AuthorityMergeLimit.
[2] A comment in authorities/merge.pl is removed. Note that a subsequent
    patch will modify and test the cron job.
[3] Script misc/batchRebuildItemsTables.pl temporarily enabled dontmerge.
    This is equivalent to setting the mergelimit to zero.
    The function defnonull is no longer needed. (If the pref was NULL,
    we restore that value. Sub merge won't mind.)

Test plan:
[1] Run t/db_dependent/UsageStats.t
[2] Run misc/batchRebuildItemsTables.pl -t
    This just ensures you it still compiles; the changes speak for itself.
[3] Now git grep on dontmerge.
    You should only find hits in atomicupdate and misc/translator/po.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 81 Marc Véron 2017-04-05 14:17:48 UTC
Created attachment 61901 [details] [review]
Bug 9988: Add a subtest for specific postponed merge issues

This subtest shows that we need a few little tweaks to make merge
work in some specific postponed merge situations.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should fail.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 82 Marc Véron 2017-04-05 14:19:05 UTC
Created attachment 61902 [details] [review]
Bug 9988: Few subtle changes for postponed merge

The fails in the previous test showed that we need the first three
changes here. Some final polishing in points 4 to 6.

[1] Sub merge: Refine the condition for initializing $tags_new.
    A postponed 'modify'-merge (A to B) makes that $authtypefrom is not
    defined when running merge later. When crossing the type boundary, we
    need a new field too.
[2] Sub merge: Add condition for an empty @record_to array.
    This indicates also that a field should be removed, since we should
    otherwise only add a $9 subfield.
[3] Sub merge: Adjust initializing @record_from.
    This change is tested by verifying a cleared subfield in the test.
[4] DelAuthority: Adding a skipmerge parameter to allow the call from
    authorities/merge.pl to skip an unneeded merge.
    This also prevents that the 'delete-merge' would precede the
    'modify-merge' under a hypothetical race condition.
[5] DelAuthority: There is actually no need to call GetAuthority.
    The subfields of the old record are not relevant in this case.
[6] Added a few POD lines to merge.
[7] Removed a trailing space in a comment line in merge.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should no longer fail now.
[2] See test plan of next patch.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 83 Marc Véron 2017-04-05 14:20:14 UTC
Created attachment 61903 [details] [review]
Bug 9988: Refactor the cron script

The cron job is moved from migration tools to cronjobs. And renamed to
a plural form. The script is now based on Koha objects. It does no longer
include the code to merge one record. This can be done via the interface,
and will be added to a maintenance script on bug 18071. Should not be part
of this cron job.

Adding a cron_cleanup method to MergeRequests; this method is called from
the cron script to reset older entries still marked in progress and to
also remove old processed entries. Tested in a separate unit test.

Test plan:
[1] Run t/db_dependent/Authorities/MergeRequests.t
[2] Set AuthorityMergeLimit to 0. (All merges are postponed.)
[3] Modify an authority linked to a few records.
[4] Delete an authority linked to a few records with batch delete tool.
[5] And select two auth records with linked records.
    Merge these two records with authority/merge.pl.
    Note: Do not select Default. See also bug 17380.
[6] Check the need_merge_authorities table for inserted records.
[7] Run misc/cronjobs/merge_authorities.pl -b and inspect the linked
    records and the record status in need_merge_authorities.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 84 Marc Véron 2017-04-05 14:21:33 UTC
Created attachment 61904 [details] [review]
Bug 9988: Add the cronjob and print an upgrade message

The Debian cron file and the misc crontab example are updated.
A message is printed when upgrading.

Note: At this moment the merge cron job is run once a day. This is imo a
good starting point. The load for this job greatly depends on the value of
pref AuthorityMergeLimit. Of course you can schedule the job more often,
and if this need is felt more globally, we can adjust it later.

Test plan:
[1] Run the dbrev and see the message.
[2] Read the changes to the cron files.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 85 Marcel de Rooy 2017-04-05 14:24:15 UTC
(In reply to Marc Véron from comment #70)
> Went through test plans from comment #68 and comment #69, works as described.

Great, Marc ! Thanks.
Comment 86 Jacek Ablewicz 2017-04-05 20:09:01 UTC
Created attachment 61912 [details] [review]
Bug 9988: Database revision for AuthorityMergeLimit

Introduce the new preference and remove dontmerge.
If dontmerge was enabled, the new pref will be set to zero and we will
print a message about raising this value now.

Test plan:
Run the database revision or a new install.
In case of an upgrade, check the new pref value. Should be 0 when
dontmerge was enabled, otherwise 50.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested sysprefs.sql on a new database.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 87 Jacek Ablewicz 2017-04-05 20:09:12 UTC
Created attachment 61913 [details] [review]
Bug 9988: Table revision for need_merge_authorities

In order to let merge do exactly the same when called from the merge
cron job, we need three additional fields in the table. The use of these
fields will be explained in a follow-up.

Test plan:
Run the dbrev (updatestructure step in webinstaller).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested running kohastructure.sql for this table.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 88 Jacek Ablewicz 2017-04-05 20:09:20 UTC
Created attachment 61914 [details] [review]
Bug 9988: DBIx schema changes for need_merge_authorities

As generated by misc/devel/update_dbix_class_files.pl.
No test plan.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 89 Jacek Ablewicz 2017-04-05 20:09:28 UTC
Created attachment 61915 [details] [review]
Bug 9988: Add Koha objects for table need_merge_authorities

This patch adds two Koha objects: MergeRequest(s).
MergeRequest has a new method and an oldmarc method.
A class method reporting_tag_xml is added to MergeRequests.pm.
All new routines are tested in Authorities.t.

Removes a few unneeded modules from Koha::Authority.

Test plan:
Run t/db_dependent/Koha/Authorities.t

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 90 Jacek Ablewicz 2017-04-05 20:09:36 UTC
Created attachment 61916 [details] [review]
Bug 9988: Add extract_biblionumber to Koha/SearchEngine

When we will replace the Zebra code in sub merge, we will call SearchEngine
to pass records and we need a routine to extract a biblionumber from
a search result record. A record from Zebra still must be converted to
MARC::Record. This is no longer needed for a ElasticSearch record.

Test plan:
Run t/db_dependent/Koha/SearchEngine/Search.t

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 91 Jacek Ablewicz 2017-04-05 20:09:43 UTC
Created attachment 61917 [details] [review]
Bug 9988: Add get_usage_count and linked_biblionumbers to Koha::Authority

When replacing the Zebra code in sub merge, we actually need CountUsage
as well as the results it gets from SearchEngine.

In order to get count and/or results, we now create:

[1] instance methods get_usage_count and linked_biblionumbers in
    Koha::Authority,
[2] class methods of the same name in Koha::Authorities.

The instance method calls the class method. The class method can be used
for deleted authority records, and in 'legacy calls' before refactoring.

Note: On BZ 18149 we will replace all CountUsage calls.

Test plan:
Run t/db_dependent/Koha/Authorities.t

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 92 Jacek Ablewicz 2017-04-05 20:09:51 UTC
Created attachment 61918 [details] [review]
Bug 9988: Merge should have a parameter hash

We will need a few additional parameters for merge later on. This patch
puts the original parameters in a parameter hash.
For the same reason DelAuthority gets a parameter hash here.

Note: We remove the second parameter from the DelAuthority call in
authorities/authorities-home.pl here. It was not used and could have
presented problems in the future.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t.
[2] Run t/db_dependent/Authorities/Merge.t.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 93 Jacek Ablewicz 2017-04-05 20:10:00 UTC
Created attachment 61919 [details] [review]
Bug 9988: Remove the Zebra code from sub merge

Since we can now call linked_biblionumbers, we can now remove all Zebra
related code from merge. We also add a parameter biblionumbers; we use it
in the test now, but it may be handy too later in the maintenance script
when we want to trigger a merge for specific biblionumber(s). See bug
report 18071.

All mocks for ZOOM, Context::Zconn, Search::new_record_for_zebra in the
merge test can now be replaced by one mock for linked_biblionumbers. Note
that we test the biblionumbers parameter in the last subtest without that
mock.

Remove unused vars $countunmodifiedbiblio, $counterrors from merge.
Renamed zebrarecords to linkedrecords in the Merge test.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Modify an authority record. Check the linked biblio records.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 94 Jacek Ablewicz 2017-04-05 20:10:11 UTC
Created attachment 61920 [details] [review]
Bug 9988: Check the merge limit in sub merge

At this point, we are replacing dontmerge functionality by the new
AuthorityMergeLimit logic. Instead of doing this check before calling
merge, we just call merge and check it there. In order to let the cron
job do the larger (postponed) merges, we add a parameter override_limit.

A subtest is added in Merge.t to test the 'postponed merge' feature. Since
merge now also calls get_usage_count, an additional mock is added. All
references to dontmerge are removed.

In merge two lines, initializing $dbh and $counteditbiblios, are moved.
The dontmerge test in DelAuthority and ModAuthority is removed. Since this
did not leave much in ModAuthority, I fixed the whitespace on the remaining
lines rightaway (yes, I know).
A minimal set of changes is applied to the cron script; it will get further
attention on a next patch.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Set AuthorityMergeLimit to 2. Modify an authority with two linked
    biblios. Check that the merge was done immediately.
[3] Now modify an authority with more than 2 linked records.
    Verify that the merge was postponed; a record must be inserted in
    the need_merge_authorities table.
[4] Testing of the merge cron job is *postponed* to a next patch.
    Note: I tested a modification, but the script just needs more
    attention.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 95 Jacek Ablewicz 2017-04-05 20:10:19 UTC
Created attachment 61921 [details] [review]
Bug 9988: Remove further references to dontmerge

[1] The preference was sent to HEA. We can now send both AuthorityMergeMode
    as well as AuthorityMergeLimit.
[2] A comment in authorities/merge.pl is removed. Note that a subsequent
    patch will modify and test the cron job.
[3] Script misc/batchRebuildItemsTables.pl temporarily enabled dontmerge.
    This is equivalent to setting the mergelimit to zero.
    The function defnonull is no longer needed. (If the pref was NULL,
    we restore that value. Sub merge won't mind.)

Test plan:
[1] Run t/db_dependent/UsageStats.t
[2] Run misc/batchRebuildItemsTables.pl -t
    This just ensures you it still compiles; the changes speak for itself.
[3] Now git grep on dontmerge.
    You should only find hits in atomicupdate and misc/translator/po.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 96 Jacek Ablewicz 2017-04-05 20:10:28 UTC
Created attachment 61922 [details] [review]
Bug 9988: Add a subtest for specific postponed merge issues

This subtest shows that we need a few little tweaks to make merge
work in some specific postponed merge situations.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should fail.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 97 Jacek Ablewicz 2017-04-05 20:10:38 UTC
Created attachment 61923 [details] [review]
Bug 9988: Few subtle changes for postponed merge

The fails in the previous test showed that we need the first three
changes here. Some final polishing in points 4 to 6.

[1] Sub merge: Refine the condition for initializing $tags_new.
    A postponed 'modify'-merge (A to B) makes that $authtypefrom is not
    defined when running merge later. When crossing the type boundary, we
    need a new field too.
[2] Sub merge: Add condition for an empty @record_to array.
    This indicates also that a field should be removed, since we should
    otherwise only add a $9 subfield.
[3] Sub merge: Adjust initializing @record_from.
    This change is tested by verifying a cleared subfield in the test.
[4] DelAuthority: Adding a skipmerge parameter to allow the call from
    authorities/merge.pl to skip an unneeded merge.
    This also prevents that the 'delete-merge' would precede the
    'modify-merge' under a hypothetical race condition.
[5] DelAuthority: There is actually no need to call GetAuthority.
    The subfields of the old record are not relevant in this case.
[6] Added a few POD lines to merge.
[7] Removed a trailing space in a comment line in merge.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should no longer fail now.
[2] See test plan of next patch.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 98 Jacek Ablewicz 2017-04-05 20:10:53 UTC
Created attachment 61924 [details] [review]
Bug 9988: Refactor the cron script

The cron job is moved from migration tools to cronjobs. And renamed to
a plural form. The script is now based on Koha objects. It does no longer
include the code to merge one record. This can be done via the interface,
and will be added to a maintenance script on bug 18071. Should not be part
of this cron job.

Adding a cron_cleanup method to MergeRequests; this method is called from
the cron script to reset older entries still marked in progress and to
also remove old processed entries. Tested in a separate unit test.

Test plan:
[1] Run t/db_dependent/Authorities/MergeRequests.t
[2] Set AuthorityMergeLimit to 0. (All merges are postponed.)
[3] Modify an authority linked to a few records.
[4] Delete an authority linked to a few records with batch delete tool.
[5] And select two auth records with linked records.
    Merge these two records with authority/merge.pl.
    Note: Do not select Default. See also bug 17380.
[6] Check the need_merge_authorities table for inserted records.
[7] Run misc/cronjobs/merge_authorities.pl -b and inspect the linked
    records and the record status in need_merge_authorities.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 99 Jacek Ablewicz 2017-04-05 20:11:03 UTC
Created attachment 61925 [details] [review]
Bug 9988: Add the cronjob and print an upgrade message

The Debian cron file and the misc crontab example are updated.
A message is printed when upgrading.

Note: At this moment the merge cron job is run once a day. This is imo a
good starting point. The load for this job greatly depends on the value of
pref AuthorityMergeLimit. Of course you can schedule the job more often,
and if this need is felt more globally, we can adjust it later.

Test plan:
[1] Run the dbrev and see the message.
[2] Read the changes to the cron files.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Comment 100 Jacek Ablewicz 2017-04-05 20:36:20 UTC
I have tested a bit earlier version of this patchset (from a ~week ago - not exactly following the test plans, but focusing on the possibly "scary stuff" which was not directly covered by the tests instead), and by a look of it there is no need to fully retest it again. I reviewed all the changes in the code once more (well, except for the *.t files, to be honest), and they all look pretty solid to me - the only issue encountered in the previous try got fixed in the meanwhile. Sign off time ;).
Comment 101 Marcel de Rooy 2017-04-06 06:25:43 UTC
(In reply to Jacek Ablewicz from comment #100)
> I have tested a bit earlier version of this patchset (from a ~week ago - not
> exactly following the test plans, but focusing on the possibly "scary stuff"
> which was not directly covered by the tests instead), and by a look of it
> there is no need to fully retest it again. I reviewed all the changes in the
> code once more (well, except for the *.t files, to be honest), and they all
> look pretty solid to me - the only issue encountered in the previous try got
> fixed in the meanwhile. Sign off time ;).

Thanks again Jacek.
Comment 102 Julian Maurice 2017-04-10 19:32:01 UTC
QA:

I haven't tested it yet, but:

1) koha-qa reports a weird error about Koha::Authority::MergeRequest(s) :

$ perl -cw Koha/Authority/MergeRequest.pm
Inconsistent hierarchy during C3 merge of class 'Koha::Authority::MergeRequest':
        current merge results [
                Koha::Authority::MergeRequest,
        ]
        merging failed on 'Koha::Object' at /usr/share/perl/5.20/parent.pm line 26.
BEGIN failed--compilation aborted at /home/koha/env/master/src/Koha/Authority/MergeRequest.pm line 22.
Compilation failed in require at /home/koha/env/master/src/Koha/Authority/MergeRequests.pm line 25.
BEGIN failed--compilation aborted at /home/koha/env/master/src/Koha/Authority/MergeRequests.pm line 25.
Compilation failed in require at Koha/Authority/MergeRequest.pm line 25.
BEGIN failed--compilation aborted at Koha/Authority/MergeRequest.pm line 25.

It seems to be because of circular dependency between the two modules. If I remove 'use Koha::Authority::MergeRequest;' from Koha::Authority::MergeRequests, there is no error.


2) koha-qa also complains about POD:

*** WARNING: empty section in previous paragraph  in file Koha/Authority/MergeRequests.pm


3) t/db_dependent/Koha/Authorities.t fails with a UNIMARC setup

$ prove t/db_dependent/Koha/Authorities.t                                                                                                                 [2/8764]
t/db_dependent/Koha/Authorities.t .. 1/6 substr outside of string at /usr/share/perl5/MARC/File/XML.pm line 561.
Use of uninitialized value $enc in string eq at /usr/share/perl5/MARC/File/XML.pm line 563.
Use of uninitialized value $enc in string eq at /usr/share/perl5/MARC/File/XML.pm line 563.
Use of uninitialized value $enc in string eq at /usr/share/perl5/MARC/File/XML.pm line 565.
Use of uninitialized value $enc in concatenation (.) or string at /usr/share/perl5/MARC/File/XML.pm line 568.
    # Looks like you planned 4 tests but ran 1.

#   Failed test 'New merge request, method oldmarc'
#   at t/db_dependent/Koha/Authorities.t line 97.
Unsupported UNIMARC character encoding [] for XML output for UNIMARCAUTH; 100$a -> a at /usr/share/perl5/MARC/File/XML.pm line 568.
# Looks like your test exited with 255 just after 4.
t/db_dependent/Koha/Authorities.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 3/6 subtests

Test Summary Report
-------------------
t/db_dependent/Koha/Authorities.t (Wstat: 65280 Tests: 4 Failed: 1)
  Failed test:  4
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 6 tests but ran 4.
Files=1, Tests=4,  1 wallclock secs ( 0.01 usr  0.00 sys +  0.84 cusr  0.03 csys =  0.88 CPU)
Result: FAIL


4) Koha::Authorities::get_usage_count and Koha::Authorities::linked_biblionumbers look very similar. Is there a real performance gain to use get_usage_count instead of linked_biblionumbers ? C4::Authorities::merge call them both and it looks like we could save one search request by calling only linked_biblionumbers
Comment 103 Julian Maurice 2017-04-11 06:34:53 UTC
5) When I merge 2 authorities on the staff interface, I get a UNIMARC-related error:

Unsupported UNIMARC character encoding [] for XML output for UNIMARCAUTH; 100$a ->  at /usr/share/perl5/MARC/File/XML.pm line 568.

It's triggered by MARC::Record::as_xml in Koha::Authority::MergeRequests::reporting_tag_xml.

It seems to work correctly when I add the following code before as_xml:

    # Copied from AddAuthority
    my $date = POSIX::strftime("%Y%m%d", localtime);
    my $defaultfield100 = C4::Context->preference('UNIMARCAuthorityField100');
    $newrecord->append_fields(
        MARC::Field->new('100', ' ', ' ' , 'a'=> $date.$defaultfield100)
    );
Comment 104 Marcel de Rooy 2017-04-11 11:54:53 UTC
Thanks, Julian. Will come back to these points very soon.
Comment 105 Marcel de Rooy 2017-04-11 14:08:16 UTC
(In reply to Julian Maurice from comment #103)
> 5) When I merge 2 authorities on the staff interface, I get a
> UNIMARC-related error:
> 
> Unsupported UNIMARC character encoding [] for XML output for UNIMARCAUTH;
> 100$a ->  at /usr/share/perl5/MARC/File/XML.pm line 568.
> 
> It's triggered by MARC::Record::as_xml in
> Koha::Authority::MergeRequests::reporting_tag_xml.
> 
> It seems to work correctly when I add the following code before as_xml:
> 
>     # Copied from AddAuthority
>     my $date = POSIX::strftime("%Y%m%d", localtime);
>     my $defaultfield100 =
> C4::Context->preference('UNIMARCAuthorityField100');
>     $newrecord->append_fields(
>         MARC::Field->new('100', ' ', ' ' , 'a'=> $date.$defaultfield100)
>     );

Thanks. This error actually is the same as point 3 of the previous comment. I will add an absolute minimum 100 with encoding only for UNIMARC to resolve the problem in MARC::File::XML.
Comment 106 Marcel de Rooy 2017-04-11 14:21:18 UTC
(In reply to Julian Maurice from comment #102)
> 4) Koha::Authorities::get_usage_count and
> Koha::Authorities::linked_biblionumbers look very similar. Is there a real
> performance gain to use get_usage_count instead of linked_biblionumbers ?
> C4::Authorities::merge call them both and it looks like we could save one
> search request by calling only linked_biblionumbers

Yes, there is a performance gain depending on the number of linked records. A significant difference between both subs is the second and third parameter of the simple_search_compat call. And the biblionumber extraction of course.
I first worked with one sub doing both, but in the end I felt that separating them resulted in cleaner code. I would prefer not to change them on this report anymore.
Comment 107 Marcel de Rooy 2017-04-11 14:27:11 UTC
Created attachment 62055 [details] [review]
Bug 9988: [QA Follow-up] Satisfy QA issues

[1] See comment102. Moved sub reporting_tag_xml to MergeRequest.pm.
    Adjusted t/db_dependent/Koha/Authorities.t accordingly.
    This resolves the C3 inconsistent hierarchy errors.
[2] Removed empty POD section Instance Methods from MergeRequests.
    This resolves the POD error in comment102 point 2.
[3] Include a tag 100 for UNIMARC in reporting_tag_xml to resolve an error
    on encoding in MARC::File::XML. Subtest for oldmarc and subtest for
    reporting_tag_xml adjusted accordingly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 108 Marcel de Rooy 2017-04-12 11:47:55 UTC
Back to SO queue
Comment 109 Julian Maurice 2017-04-13 08:48:56 UTC
Created attachment 62123 [details] [review]
Bug 9988: Database revision for AuthorityMergeLimit

Introduce the new preference and remove dontmerge.
If dontmerge was enabled, the new pref will be set to zero and we will
print a message about raising this value now.

Test plan:
Run the database revision or a new install.
In case of an upgrade, check the new pref value. Should be 0 when
dontmerge was enabled, otherwise 50.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested sysprefs.sql on a new database.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 110 Julian Maurice 2017-04-13 08:49:03 UTC
Created attachment 62124 [details] [review]
Bug 9988: Table revision for need_merge_authorities

In order to let merge do exactly the same when called from the merge
cron job, we need three additional fields in the table. The use of these
fields will be explained in a follow-up.

Test plan:
Run the dbrev (updatestructure step in webinstaller).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Also tested running kohastructure.sql for this table.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 111 Julian Maurice 2017-04-13 08:49:08 UTC
Created attachment 62125 [details] [review]
Bug 9988: DBIx schema changes for need_merge_authorities

As generated by misc/devel/update_dbix_class_files.pl.
No test plan.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 112 Julian Maurice 2017-04-13 08:49:14 UTC
Created attachment 62126 [details] [review]
Bug 9988: Add Koha objects for table need_merge_authorities

This patch adds two Koha objects: MergeRequest(s).
MergeRequest has a new method and an oldmarc method.
A class method reporting_tag_xml is added to MergeRequests.pm.
All new routines are tested in Authorities.t.

Removes a few unneeded modules from Koha::Authority.

Test plan:
Run t/db_dependent/Koha/Authorities.t

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 113 Julian Maurice 2017-04-13 08:49:19 UTC
Created attachment 62127 [details] [review]
Bug 9988: Add extract_biblionumber to Koha/SearchEngine

When we will replace the Zebra code in sub merge, we will call SearchEngine
to pass records and we need a routine to extract a biblionumber from
a search result record. A record from Zebra still must be converted to
MARC::Record. This is no longer needed for a ElasticSearch record.

Test plan:
Run t/db_dependent/Koha/SearchEngine/Search.t

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 114 Julian Maurice 2017-04-13 08:49:24 UTC
Created attachment 62128 [details] [review]
Bug 9988: Add get_usage_count and linked_biblionumbers to Koha::Authority

When replacing the Zebra code in sub merge, we actually need CountUsage
as well as the results it gets from SearchEngine.

In order to get count and/or results, we now create:

[1] instance methods get_usage_count and linked_biblionumbers in
    Koha::Authority,
[2] class methods of the same name in Koha::Authorities.

The instance method calls the class method. The class method can be used
for deleted authority records, and in 'legacy calls' before refactoring.

Note: On BZ 18149 we will replace all CountUsage calls.

Test plan:
Run t/db_dependent/Koha/Authorities.t

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 115 Julian Maurice 2017-04-13 08:49:29 UTC
Created attachment 62129 [details] [review]
Bug 9988: Merge should have a parameter hash

We will need a few additional parameters for merge later on. This patch
puts the original parameters in a parameter hash.
For the same reason DelAuthority gets a parameter hash here.

Note: We remove the second parameter from the DelAuthority call in
authorities/authorities-home.pl here. It was not used and could have
presented problems in the future.

Test plan:
[1] Run t/db_dependent/AuthoritiesMarc.t.
[2] Run t/db_dependent/Authorities/Merge.t.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 116 Julian Maurice 2017-04-13 08:49:34 UTC
Created attachment 62130 [details] [review]
Bug 9988: Remove the Zebra code from sub merge

Since we can now call linked_biblionumbers, we can now remove all Zebra
related code from merge. We also add a parameter biblionumbers; we use it
in the test now, but it may be handy too later in the maintenance script
when we want to trigger a merge for specific biblionumber(s). See bug
report 18071.

All mocks for ZOOM, Context::Zconn, Search::new_record_for_zebra in the
merge test can now be replaced by one mock for linked_biblionumbers. Note
that we test the biblionumbers parameter in the last subtest without that
mock.

Remove unused vars $countunmodifiedbiblio, $counterrors from merge.
Renamed zebrarecords to linkedrecords in the Merge test.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Modify an authority record. Check the linked biblio records.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 117 Julian Maurice 2017-04-13 08:49:39 UTC
Created attachment 62131 [details] [review]
Bug 9988: Check the merge limit in sub merge

At this point, we are replacing dontmerge functionality by the new
AuthorityMergeLimit logic. Instead of doing this check before calling
merge, we just call merge and check it there. In order to let the cron
job do the larger (postponed) merges, we add a parameter override_limit.

A subtest is added in Merge.t to test the 'postponed merge' feature. Since
merge now also calls get_usage_count, an additional mock is added. All
references to dontmerge are removed.

In merge two lines, initializing $dbh and $counteditbiblios, are moved.
The dontmerge test in DelAuthority and ModAuthority is removed. Since this
did not leave much in ModAuthority, I fixed the whitespace on the remaining
lines rightaway (yes, I know).
A minimal set of changes is applied to the cron script; it will get further
attention on a next patch.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t
[2] Set AuthorityMergeLimit to 2. Modify an authority with two linked
    biblios. Check that the merge was done immediately.
[3] Now modify an authority with more than 2 linked records.
    Verify that the merge was postponed; a record must be inserted in
    the need_merge_authorities table.
[4] Testing of the merge cron job is *postponed* to a next patch.
    Note: I tested a modification, but the script just needs more
    attention.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 118 Julian Maurice 2017-04-13 08:49:45 UTC
Created attachment 62132 [details] [review]
Bug 9988: Remove further references to dontmerge

[1] The preference was sent to HEA. We can now send both AuthorityMergeMode
    as well as AuthorityMergeLimit.
[2] A comment in authorities/merge.pl is removed. Note that a subsequent
    patch will modify and test the cron job.
[3] Script misc/batchRebuildItemsTables.pl temporarily enabled dontmerge.
    This is equivalent to setting the mergelimit to zero.
    The function defnonull is no longer needed. (If the pref was NULL,
    we restore that value. Sub merge won't mind.)

Test plan:
[1] Run t/db_dependent/UsageStats.t
[2] Run misc/batchRebuildItemsTables.pl -t
    This just ensures you it still compiles; the changes speak for itself.
[3] Now git grep on dontmerge.
    You should only find hits in atomicupdate and misc/translator/po.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 119 Julian Maurice 2017-04-13 08:49:50 UTC
Created attachment 62133 [details] [review]
Bug 9988: Add a subtest for specific postponed merge issues

This subtest shows that we need a few little tweaks to make merge
work in some specific postponed merge situations.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should fail.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 120 Julian Maurice 2017-04-13 08:49:56 UTC
Created attachment 62134 [details] [review]
Bug 9988: Few subtle changes for postponed merge

The fails in the previous test showed that we need the first three
changes here. Some final polishing in points 4 to 6.

[1] Sub merge: Refine the condition for initializing $tags_new.
    A postponed 'modify'-merge (A to B) makes that $authtypefrom is not
    defined when running merge later. When crossing the type boundary, we
    need a new field too.
[2] Sub merge: Add condition for an empty @record_to array.
    This indicates also that a field should be removed, since we should
    otherwise only add a $9 subfield.
[3] Sub merge: Adjust initializing @record_from.
    This change is tested by verifying a cleared subfield in the test.
[4] DelAuthority: Adding a skipmerge parameter to allow the call from
    authorities/merge.pl to skip an unneeded merge.
    This also prevents that the 'delete-merge' would precede the
    'modify-merge' under a hypothetical race condition.
[5] DelAuthority: There is actually no need to call GetAuthority.
    The subfields of the old record are not relevant in this case.
[6] Added a few POD lines to merge.
[7] Removed a trailing space in a comment line in merge.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t.
    The last subtest should no longer fail now.
[2] See test plan of next patch.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 121 Julian Maurice 2017-04-13 08:50:02 UTC
Created attachment 62135 [details] [review]
Bug 9988: Refactor the cron script

The cron job is moved from migration tools to cronjobs. And renamed to
a plural form. The script is now based on Koha objects. It does no longer
include the code to merge one record. This can be done via the interface,
and will be added to a maintenance script on bug 18071. Should not be part
of this cron job.

Adding a cron_cleanup method to MergeRequests; this method is called from
the cron script to reset older entries still marked in progress and to
also remove old processed entries. Tested in a separate unit test.

Test plan:
[1] Run t/db_dependent/Authorities/MergeRequests.t
[2] Set AuthorityMergeLimit to 0. (All merges are postponed.)
[3] Modify an authority linked to a few records.
[4] Delete an authority linked to a few records with batch delete tool.
[5] And select two auth records with linked records.
    Merge these two records with authority/merge.pl.
    Note: Do not select Default. See also bug 17380.
[6] Check the need_merge_authorities table for inserted records.
[7] Run misc/cronjobs/merge_authorities.pl -b and inspect the linked
    records and the record status in need_merge_authorities.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 122 Julian Maurice 2017-04-13 08:50:09 UTC
Created attachment 62136 [details] [review]
Bug 9988: Add the cronjob and print an upgrade message

The Debian cron file and the misc crontab example are updated.
A message is printed when upgrading.

Note: At this moment the merge cron job is run once a day. This is imo a
good starting point. The load for this job greatly depends on the value of
pref AuthorityMergeLimit. Of course you can schedule the job more often,
and if this need is felt more globally, we can adjust it later.

Test plan:
[1] Run the dbrev and see the message.
[2] Read the changes to the cron files.

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

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 123 Julian Maurice 2017-04-13 08:50:14 UTC
Created attachment 62137 [details] [review]
Bug 9988: [QA Follow-up] Satisfy QA issues

[1] See comment102. Moved sub reporting_tag_xml to MergeRequest.pm.
    Adjusted t/db_dependent/Koha/Authorities.t accordingly.
    This resolves the C3 inconsistent hierarchy errors.
[2] Removed empty POD section Instance Methods from MergeRequests.
    This resolves the POD error in comment102 point 2.
[3] Include a tag 100 for UNIMARC in reporting_tag_xml to resolve an error
    on encoding in MARC::File::XML. Subtest for oldmarc and subtest for
    reporting_tag_xml adjusted accordingly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 124 Julian Maurice 2017-04-13 08:54:27 UTC
koha-qa is happy, prove is happy, and I couldn't find any issue during my tests, so I'm happy too :)
Passed QA.
Comment 125 Marcel de Rooy 2017-04-13 08:55:31 UTC
Thanks Julian
Comment 126 Kyle M Hall 2017-04-13 12:59:23 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 127 Katrin Fischer 2017-04-16 12:26:51 UTC
This won't get ported back to 16.11.x as it is an enhancement.