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.
Is it a duplicate of bug 11496? (Or the opposite :) )
(In reply to Jonathan Druart from comment #1) > Is it a duplicate of bug 11496? (Or the opposite :) ) Do not think so.
(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.
*** Bug 5821 has been marked as a duplicate of this bug. ***
*** Bug 12791 has been marked as a duplicate of this bug. ***
Created attachment 59158 [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 a lower number. Test plan: Run the database revision or a new install. In case of an upgrade, check the new pref value. Should be 10 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.
Created attachment 59159 [details] [review] Bug 9988: Add Koha objects for table need_merge_authorities Test plan: Run t/db_dependent/Koha/Authorities.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Just a run-up. More patches coming, but needing rebase on other work coming from the omnibus bug 17908.
Created attachment 60256 [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 a lower number. Test plan: Run the database revision or a new install. In case of an upgrade, check the new pref value. Should be 10 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.
Created attachment 60257 [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 two additional fields in the table. One existing field is renamed. 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.
Created attachment 60258 [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>
Created attachment 60259 [details] [review] Bug 9988: Add Koha objects for table need_merge_authorities This patch adds two Koha objects: MergeRequest(s). A method add_merge_request is added to Koha::Authority. A class method reporting_tag_xml is added to MergeRequests.pm. Both routines are tested in Authorities.t. Test plan: Run t/db_dependent/Koha/Authorities.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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.
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.
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>
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>
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>
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>
Created attachment 60600 [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. Test plan: [1] Run Merge.t [2] Merge two authorities in the Authorities module Note: I also tested script merge_authority.pl with -b parameter. This script will still need further attention. I manually deleted an authority record, inserted a record in need_merge_authorities, and ran the script with -b while adding -f and -t with an existing authority number. This record number is ignored in batch mode. The biblio record with the deleted authority was correctly cleaned up. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60601 [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>
Created attachment 60602 [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 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>
Created attachment 60603 [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>
Created attachment 60604 [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.
Created attachment 60605 [details] [review] Bug 9988: Fix a small last minute bug in merge When you postpone an 'authtype crossing' authority merge from A to B, the record for A will be deleted. In that case the condition for tags_new in sub merge does not work well. We should allow an empty authtypefrom here in order to get a new field belonging to authority B. Simply put, if we go from PERSO to CORPO, all linked records should go from X00 to X10 etc. Test plan: [1] Run Merge.t [2] Select two authorities of different type with linked records. [3] Set AuthorityMergeLimit to zero. And merge both auth records. Note: Do not select Default. See also bug 17380. [4] Run the cron job and check the linked records (especially the ones that were linked to the deleted record). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 60606 [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>
Created attachment 60607 [details] [review] Bug 9988: Final polishing [1] DelAuthority: There is actually no need to call GetAuthority in this routine. The subfields of the old record are not relevant when removing all references in the linked biblio records. [2] Added a few POD lines to merge. [3] Removed a trailing space in a comment line in merge. Test plan: [1] Run Merge.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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
Still thinking about a possible issue here. Hang on :)
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>
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>
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>
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>
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>
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>
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.
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>
Back into NSO. Please use the COMBINED test script in comment27.
Sounds like another use case for having a task scheduler and message queue in the background...
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).
(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 :)
Especially the "Few subtle changes" patch makes a few additional refinements in this regard.
(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.
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.
(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..
Blocking temporarily
(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..
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; + }
And for UNIMARC it should be 2xx
(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..
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>
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>
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.
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>
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!
Rebasing this one now
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.
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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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.
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>
Went through test plans from comment #68 and comment #69, works as described.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
(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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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 ;).
(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.
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
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, Julian. Will come back to these points very soon.
(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.
(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.
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>
Back to SO queue
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
koha-qa is happy, prove is happy, and I couldn't find any issue during my tests, so I'm happy too :) Passed QA.
Thanks Julian
Pushed to master for 17.05, thanks Marcel!
This won't get ported back to 16.11.x as it is an enhancement.