Bug 22437 - Subsequent authority merges in cron may cause biblios to lose authority information
Summary: Subsequent authority merges in cron may cause biblios to lose authority infor...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-28 16:53 UTC by Pedro Amorim
Modified: 2020-05-18 09:47 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06, 19.05.11


Attachments
fix-for-subsequent-authorities-merge (914 bytes, patch)
2019-11-26 15:57 UTC, Pedro Amorim
Details | Diff | Splinter Review
Previous attachment had a typo (914 bytes, patch)
2019-11-27 13:11 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 22437: Delete older merge requests at new merge time (1.78 KB, patch)
2020-04-03 10:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22437: Delete older merge requests at new merge time (1.78 KB, patch)
2020-04-06 13:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22437: Add test case to Merge.t (2.64 KB, patch)
2020-04-06 13:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22437: Delete older merge requests at new merge time (1.80 KB, patch)
2020-04-20 13:57 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 22437: Add test case to Merge.t (2.66 KB, patch)
2020-04-20 13:57 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 22437: Add test case to Merge.t (2.19 KB, patch)
2020-05-08 07:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22437: Delete older merge requests at new merge time (1.84 KB, patch)
2020-05-08 07:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22437: (follow-up) Move deletion of merge requests to DelAuthority (2.44 KB, patch)
2020-05-08 07:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22437: Add test case to Merge.t (2.26 KB, patch)
2020-05-11 13:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22437: Delete older merge requests at new merge time (1.87 KB, patch)
2020-05-11 13:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22437: (follow-up) Move deletion of merge requests to DelAuthority (2.47 KB, patch)
2020-05-11 13:29 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2019-02-28 16:53:32 UTC
Here's how to reproduce the problem:
AuthorityMergeLimit syspref is default 50. 
Here's an example list of 3 authorities (A, B and C) that should be merged together - resulting in 1 authority - and the total count of biblios linked to each respective authority (similar to the interface you get when searching authorities):

A	100
B	60
C	2

The good scenario:
If you merge the authorities like this:
First: C -> A
Second: B -> A

All is good, after the first merge (C -> A), A has 102 biblios. 
After second merge (B -> A), B no longer exists and A still has 102 - as expected, because the merge is scheduled to the cron.
After forcing the cron koha-foreach --chdir --enabled /usr/share/koha/bin/cronjobs/merge_authorities.pl -b -v authority A now has 162 biblios, as expected.

The bad scenario:
The problem occurs if authorities are merged like this:
First: C -> B
Second: B -> A

After the first merge (C -> B), B has 62 biblios. 
After the second merge (B -> A), B no longer exists and A still has 100 - as expected, because the merge is scheduled to the cron.
However, after forcing the cron koha-foreach --chdir --enabled /usr/share/koha/bin/cronjobs/merge_authorities.pl -b -v authority A still has 100 biblios, and the 62 biblios that were previously linked to B have effectively lost the authority information they previously had.

After some testing, I noticed that Koha will schedule an authority merge for the same authority (on itself), even when the biblios count doesn't surpass AuthorityMergeLimit, I'll explain:
For the C -> B merge, the 2 biblios from C will be updated immediately in the application (because 2 < 50), but simultaneously Koha will schedule the cron merge B -> B.
I think this happens because the merge.pl script first modifies the reference authority using ModAuthority (that calls the merge function), and then calls the merge function, can be seen here:
https://github.com/Koha-Community/Koha/blob/18.11.x/authorities/merge.pl#L79

Which means that Koha will run the merge function, twice, for each authority merge made in the application.
I think this is by design, because if B gets modified, Koha will have to update 60 biblios with the new authority information, updated subfields information etc, and this should be indeed postponed to the cron, hence the merge on itself B -> B caused by ModAuthority.
However, because B -> A happened (deleting B) after C -> B and before the cron had the chance to run, by the time the cron has to run B -> B, B has already been deleted, cleaning the authority information from the 60 (or 62) biblio records.
Comment 1 Pedro Amorim 2019-02-28 17:01:10 UTC
I've added the following fix:

https://github.com/GovernoRegionalAcores/Koha/pull/1/commits/eebddb09c95ba857df8df516d00b4bb96808e59e

Tested it, works and resolves the problem.

If anyone would be so kind to confirm this while I go through the process of learning and attempting to submit a patch through this method https://wiki.koha-community.org/wiki/Git_bz_configuration

Thank you very much,

Pedro Amorim
Comment 2 David Cook 2019-03-01 03:28:47 UTC
I don't have time to look at this, but I applaud you on your efforts!

If you have any issues with https://wiki.koha-community.org/wiki/Git_bz_configuration, visit #koha on the OFTC IRC network, or email koha-devel@lists.koha-community.org.
Comment 3 Pedro Amorim 2019-11-26 15:57:31 UTC
Created attachment 95836 [details] [review]
fix-for-subsequent-authorities-merge

This patch makes it so that Koha will skip the merge process if the destination authority has been deleted before the cron has had the chance to run.
Comment 4 Pedro Amorim 2019-11-27 13:11:20 UTC
Created attachment 95848 [details] [review]
Previous attachment had a typo
Comment 5 Marcel de Rooy 2020-03-16 07:13:57 UTC
Good catch, Pedro. I will be looking into it.
Comment 6 Katrin Fischer 2020-04-01 20:34:18 UTC
Should this be Need Signoff?
Comment 7 Marcel de Rooy 2020-04-02 06:04:40 UTC
(In reply to Katrin Fischer from comment #6)
> Should this be Need Signoff?

No this needs more attention. On my list.
Comment 8 Marcel de Rooy 2020-04-03 06:51:51 UTC
Pedro,
The simplest way to explain why your patch is not yet ready, is running the test t/db_dependent/Authority/Merge.t
Without your patch it passes, with your patch we see several fails.
We need to continue in the !$authto case, since it should be possible to delete an authority and cleanup biblio records.
As you explained very well, we do need to make sure that scenario BB cron, CB direct, AA cron, and BA cron does not remove the 62 links.
Comment 9 Marcel de Rooy 2020-04-03 06:52:59 UTC
Working on it now
Comment 10 Marcel de Rooy 2020-04-03 07:42:28 UTC
The idea is:

If we must perform a BB merge in the cron job while B has been deleted BUT we also have a following BA merge request, we should skip the (destructive) BB merge. The following BA merge will do the job.

Or similarly: If we do a BA merge, we should delete BB merge requests in the queue. Since deleting B will make the BB destructive.
Comment 11 Marcel de Rooy 2020-04-03 10:12:24 UTC
Created attachment 102338 [details] [review]
Bug 22437: Delete older merge requests at new merge time

If you merge A to B, we should remove older AA merges (regular
merge to itself) in the queue before deleting A.

Test plan:
[1] Set merge limit in prefs to say X.
[2] Find three authorities A, B, C with linkcount >X, >X, <X.
[3] Merge C to B. This creates a 'BB merge' in the queue.
    Auth C is deleted.
[4] Merge B to A. Creating AA and BA in the queue.
    Auth B is deleted.
    Optionally verify that BB is no longer in the queue.
[5] Run the merge_authorities.pl cron job.
    Verify that the linkcount to A is the former total of ABC.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2020-04-03 10:13:19 UTC
The first patch can be tested or signed.
But I will still add a patch for update_authorities doing the same and add a test to mimic whats happening here.
So I did not yet change status.
Comment 13 Marcel de Rooy 2020-04-06 12:30:42 UTC
(In reply to Marcel de Rooy from comment #12)
> The first patch can be tested or signed.
> But I will still add a patch for update_authorities doing the same and add a
> test to mimic whats happening here.
> So I did not yet change status.

There is no change needed in update_authorities since the merge is executed with override flag, so it is not postponed.
Comment 14 Marcel de Rooy 2020-04-06 13:47:44 UTC
Created attachment 102444 [details] [review]
Bug 22437: Delete older merge requests at new merge time

If you merge A to B, we should remove older AA merges (regular
merge to itself) in the queue before deleting A.

Test plan:
[1] Set merge limit in prefs to say X.
[2] Find three authorities A, B, C with linkcount >X, >X, <X.
[3] Merge C to B. This creates a 'BB merge' in the queue.
    Auth C is deleted.
[4] Merge B to A. Creating AA and BA in the queue.
    Auth B is deleted.
    Optionally verify that BB is no longer in the queue.
[5] Run the merge_authorities.pl cron job.
    Verify that the linkcount to A is the former total of ABC.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2020-04-06 13:47:48 UTC
Created attachment 102445 [details] [review]
Bug 22437: Add test case to Merge.t

The scenario outlined on Bugzilla is simulated and proves that we need
to remove a merge B->B when B is deleted during a merge.

Test plan:
Run t/db_dependent/Authority/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2020-04-06 13:49:29 UTC
Updating severity for potential data loss (although the C->B, B->A merge might not be that common, fortunately)
Comment 17 Victor Grousset/tuxayo 2020-04-19 23:00:44 UTC
Notes on the test, because it's my first time diving in the authorities:
- AuthorityMergeLimit set to 2

> [2] Find three authorities A, B, C with linkcount >X, >X, <X.

I got A used in 6 records. As shown in /cgi-bin/koha/authorities/detail.pl?authid=NNNN
Is that was the linkcount is?

I got B used in 9 records.

I got C used in 1 records.

> Merge C to B

Koha UI «The record chosen as reference will be kept, and the other will be deleted.»

So I keep B.

Note: They don't have the same framework, it that an issue (the 3 a a different framework)

> [3] Merge C to B. This creates a 'BB merge' in the queue.

select * from need_merge_authorities \G
shows the merge but I don't know how to check about 'BB merge' is that:
    authid: 35 (id of B)
authid_new: 35
?

> [4] Merge B to A. Creating AA and BA in the queue.
select * from need_merge_authorities \G
    authid: 329 (id of 329)
authid_new: 329 (id of 329)

    authid: 35 (id of B)
authid_new: 329 (id of 329)

Look good, and look that answers my previous question.

> Optionally verify that BB is no longer in the queue.

yup :)

> [5] Run the merge_authorities.pl cron job.

Improvisation about how to run this:

kohadev-koha@ff1accab6912:/kohadevbox/koha$ misc/cronjobs/merge_authorities.pl -b -v
Started merging
Merging auth 329 to 329.
Merging auth 35 to 329.
Done in 1.47680187225342 seconds

Looks good :D

>     Verify that the linkcount to A is the former total of ABC.

6+9+1 != 15 record(s)

Noooooo

Wait, isn't that expected due to AuthorityMergeLimit = 2 ?
Maybe I'm mixing things up.

Also, I reset_all and the check the linked records and there is not overlap between the records of the ABC

== request for comments ==

- Given the above notes, was there a mistake in the execution of the test plan?
- What does the last result mean?
- given comment 7 should the status of this bug be Needs signoff ? (@Marcel)
Comment 18 Phil Ringnalda 2020-04-20 03:13:53 UTC
I'm not convinced that the sample data is actually linked in a way that lends itself to linking tests. At least for me, yes, authid 35 is linked to 9 bibs, but none of them actually should be linked to it, since it's a uniform title in Portuguese and the linked bibs are all Perl books, claiming that the subject heading for Perl is linked to that Saramago uniform title. There might be an interesting bug in what happens when that authority is merged (and by being merged, gets relinked), but not this one, exactly.

By cheating and being a cataloger by day (who has burned himself with this bug without realizing what happened at the time), so I don't mind creating my own records, I went with the alternate test plan of

1. Create the personal name authorities One, Three, and Four
2. Create one bib linked to One from its author, three linked to Three, four linked to Four
3. Merge One to Three, then Three to Four
4. run the cronjob

Without the patch, I end up with only four bibs linked to Four; with the patch, I end up with eight bibs linked to it, all saying in the bib that they are by Four. So for the C->B->A case when all three are linked to things they should be linked to, this patch does indeed work perfectly.
Comment 19 Katrin Fischer 2020-04-20 06:25:15 UTC
Hi Phil, do you want to add your sign-off then?
Comment 20 Marcel de Rooy 2020-04-20 06:33:39 UTC
(In reply to Victor Grousset/tuxayo from comment #17)

Thx Victor for testing in the first place !

> Note: They don't have the same framework, it that an issue (the 3 a a
> different framework)
AFAIC that should not be relevant.

> select * from need_merge_authorities \G
> shows the merge but I don't know how to check about 'BB merge' is that:
>     authid: 35 (id of B)
> authid_new: 35

I do not know the record numbers. But that looks to me as BB merge, yes.

> >     Verify that the linkcount to A is the former total of ABC.
> 6+9+1 != 15 record(s)
> Noooooo
> Wait, isn't that expected due to AuthorityMergeLimit = 2 ?
> Maybe I'm mixing things up.

6+9+1= 16 If you got 15, then it could be that one of the records formerly linked to B or C already contained a link to A too. 
AuthorityMergeLimit=2 means that a merge XY where linkcount X>=2 is, will be postponed to the cron job instead of being executed rightaway. Which is an essential part of test plan and bug fix.

> - Given the above notes, was there a mistake in the execution of the test
> plan?
The only thing I am thinking of, is that you might have missed that one record was already linked to A. Which would explain the difference between 15 and 16.

> - What does the last result mean?
The result of 15 means that all records were merged correctly. If you would have doen the same without this patch, you would have lost links to A in a number of records. See comment1.

> - given comment 7 should the status of this bug be Needs signoff ? (@Marcel)
Comment7 is obsolete now. It applied to the first patch on this report. If you look in the history, you can see that I submitted an alternate patch and changed status after that. So no worries :)
Comment 21 Marcel de Rooy 2020-04-20 06:35:44 UTC
(In reply to Phil Ringnalda from comment #18)
> I'm not convinced that the sample data is actually linked in a way that
> lends itself to linking tests. At least for me, yes, authid 35 is linked to
> 9 bibs, but none of them actually should be linked to it, since it's a
> uniform title in Portuguese and the linked bibs are all Perl books, claiming
> that the subject heading for Perl is linked to that Saramago uniform title.
> There might be an interesting bug in what happens when that authority is
> merged (and by being merged, gets relinked), but not this one, exactly.
> 
> By cheating and being a cataloger by day (who has burned himself with this
> bug without realizing what happened at the time), so I don't mind creating
> my own records, I went with the alternate test plan of
> 
> 1. Create the personal name authorities One, Three, and Four
> 2. Create one bib linked to One from its author, three linked to Three, four
> linked to Four
> 3. Merge One to Three, then Three to Four
> 4. run the cronjob
> 
> Without the patch, I end up with only four bibs linked to Four; with the
> patch, I end up with eight bibs linked to it, all saying in the bib that
> they are by Four. So for the C->B->A case when all three are linked to
> things they should be linked to, this patch does indeed work perfectly.

Thanks for testing Phil. As Katrin said in the meantime, could you please add a signoff line? Or should I do it for you?
Comment 22 Phil Ringnalda 2020-04-20 13:57:05 UTC
Created attachment 103298 [details] [review]
Bug 22437: Delete older merge requests at new merge time

If you merge A to B, we should remove older AA merges (regular
merge to itself) in the queue before deleting A.

Test plan:
[1] Set merge limit in prefs to say X.
[2] Find three authorities A, B, C with linkcount >X, >X, <X.
[3] Merge C to B. This creates a 'BB merge' in the queue.
    Auth C is deleted.
[4] Merge B to A. Creating AA and BA in the queue.
    Auth B is deleted.
    Optionally verify that BB is no longer in the queue.
[5] Run the merge_authorities.pl cron job.
    Verify that the linkcount to A is the former total of ABC.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <philringnalda@gmail.com>
Comment 23 Phil Ringnalda 2020-04-20 13:57:08 UTC
Created attachment 103299 [details] [review]
Bug 22437: Add test case to Merge.t

The scenario outlined on Bugzilla is simulated and proves that we need
to remove a merge B->B when B is deleted during a merge.

Test plan:
Run t/db_dependent/Authority/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <philringnalda@gmail.com>
Comment 24 Victor Grousset/tuxayo 2020-04-20 21:29:14 UTC
Thanks Marcel for clarifications.

(In reply to Marcel de Rooy from comment #20)
> (In reply to Victor Grousset/tuxayo from comment #17)
> > >     Verify that the linkcount to A is the former total of ABC.
> > 6+9+1 != 15 record(s)
> > Noooooo
[...]
> 
> 6+9+1= 16 If you got 15, then it could be that one of the records formerly
> linked to B or C already contained a link to A too. 
[...]
> 
> > - Given the above notes, was there a mistake in the execution of the test
> > plan?
> The only thing I am thinking of, is that you might have missed that one
> record was already linked to A. Which would explain the difference between
> 15 and 16.

I forgot to write that I checked that there was no overlap between the linked biblios of ABC (in my case).

Is it worth it to retry? With more coherent data or with the same data but doing another check to find why the total mismatches (I don't have a lead on that)

Or is this patch enough tested and the test data issue isn't a big deal? (compared to other pending stuff)
Comment 25 Phil Ringnalda 2020-04-20 22:48:58 UTC
I can reproduce Victor's results, but I don't believe they are this bug at all.

Starting state is:

A. authid 329, the genre Film adaptations, linked to 6 bibs but the $9329 is on a 710 for the Library of Congress Copyright Collection

B. authid 35, the uniform title Colecç~ao "O Campo da palavra", linked to 9 bibs but the $935 is on a 650 for Perl

C. any mislinked 1-bib will do, I used a topic Abuduction--Drama, with the one linked bib having the authid in subfield 9 of a 700 name-title field for Flaubert's Madame Bovary

The problem comes right at the start, the C->B merge. Because C is under the limit, the merge processes it immediately while saving B to do later. But after the merge, B still says it only has 9 linked bibs, not 10 like it should after a live merge, and indeed the Madame Bovary bib field is untouched. Then after the run of the cronjob, we have 15 bibs linked to A: the 6 originals which still have their 710 for the LC falsely claiming to be linked to a Film adaptations authority, and the 9 that had a Perl subject, which now do not have a Perl subject and do have a Film adaptations genre.

I think it's probably a bug that "in a live merge of a mislinked authority to another mislinked authority, neither the one being merged nor the one being retained have their linked bibs altered, but in a deferred merge the one being merged has its linked bibs altered, while again the one being retained does not," but luckily that's nothing to do with this bug.
Comment 26 Marcel de Rooy 2020-04-21 13:20:29 UTC
The problem Victor describes (with bad data) is interesting but I agree with Phil that is not in the scope of this report.
Please open up a new report and add clear steps to reproduce.

Since Phil and I tested with good data too, I do not see the need to repeat that. It is a very small and clear fix imho.
Comment 27 Phil Ringnalda 2020-04-24 06:23:44 UTC
Filed bug 25272 for that case - it very much isn't anything to do with this bug.

This bug, though,

(In reply to Marcel de Rooy from comment #16)
> Updating severity for potential data loss (although the C->B, B->A merge
> might not be that common, fortunately)

It should be a pretty common thing, really. When I need to merge, say, the genre Western stories to Western fiction, my first step is a search for Western s, to pick up all the decades of typos of sorties and stoires and whatnot, merging them to Western stories since they're all in one search, before merging it to Western fiction. And that's exactly a C->B, B->A with a live C merge, deferred B merge, deferred B and A merge.
Comment 28 Marcel de Rooy 2020-04-24 06:34:24 UTC
(In reply to Phil Ringnalda from comment #27)
> It should be a pretty common thing, really. 

Well, probably depends on your data, work procedures etc. For us it wasnt. But it just emphasizes that we need the fix in this coming release.
Comment 29 Tomás Cohen Arazi 2020-05-04 15:48:44 UTC
Have you thought of using some sort of semaphore, like adding a column to authority records: 'waiting_merge' and act depending on its value?
Comment 30 Marcel de Rooy 2020-05-04 20:07:52 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> Have you thought of using some sort of semaphore, like adding a column to
> authority records: 'waiting_merge' and act depending on its value?

Waiting merge = record in needs_merge with done = 0. Kind of the same. We can allow a merge of something that waits for a merge. But we must be careful. Thats the crux of this patch set.
Comment 31 Tomás Cohen Arazi 2020-05-04 20:10:34 UTC
(In reply to Marcel de Rooy from comment #30)
> (In reply to Tomás Cohen Arazi from comment #29)
> > Have you thought of using some sort of semaphore, like adding a column to
> > authority records: 'waiting_merge' and act depending on its value?
> 
> Waiting merge = record in needs_merge with done = 0. Kind of the same. We
> can allow a merge of something that waits for a merge. But we must be
> careful. Thats the crux of this patch set.

I didn't spot that by only reading the patch. I should've applied it and looked at it in context. Sorry for that.
Comment 32 Jonathan Druart 2020-05-07 09:55:14 UTC
Marcel, the tests do not cover your changes, they pass without the first patch.
Comment 33 Marcel de Rooy 2020-05-07 10:03:41 UTC
(In reply to Jonathan Druart from comment #32)
> Marcel, the tests do not cover your changes, they pass without the first
> patch.

Thanks for looking to this report.
There is no module change here, so there is no test to prove it like you have in mind.
But I added a testcase to illustrate my point. Yes, it passes with and without the patch. But if you look to the test, it demonstrates that we need the change applied to the script.
Maybe overcomplete, but imo no reason to fail QA.
Comment 34 Jonathan Druart 2020-05-07 10:36:04 UTC
Then why don't we have that code in DelAuthority?
Comment 35 Marcel de Rooy 2020-05-07 11:30:05 UTC
(In reply to Jonathan Druart from comment #34)
> Then why don't we have that code in DelAuthority?

Since deleting is not the same as merging. In a delete we want to clean up biblio records, which we in this specific case actually want to prevent.
Comment 36 Jonathan Druart 2020-05-07 11:44:12 UTC
in merge then? :)
Comment 37 Marcel de Rooy 2020-05-07 11:58:54 UTC
(In reply to Jonathan Druart from comment #36)
> in merge then? :)

Hmm. Thats tougher. Need to think about that. Sounds like possible.
Comment 38 Marcel de Rooy 2020-05-08 06:35:13 UTC
(In reply to Marcel de Rooy from comment #37)
> (In reply to Jonathan Druart from comment #36)
> > in merge then? :)


(In reply to Marcel de Rooy from comment #35)
> (In reply to Jonathan Druart from comment #34)
> > Then why don't we have that code in DelAuthority?
> 
> Since deleting is not the same as merging. In a delete we want to clean up
> biblio records, which we in this specific case actually want to prevent.

Hmm. They can be moved there of course. Had something else in mind..
Comment 39 Marcel de Rooy 2020-05-08 07:19:59 UTC
Created attachment 104557 [details] [review]
Bug 22437: Add test case to Merge.t

Test proves now that a 'destructive merge' is removed by
DelAuthority.

Test plan:
Run t/db_dependent/Authority/Merge.t. Should fail when you
do not apply the other two patches and pass with them.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Marcel de Rooy 2020-05-08 07:20:04 UTC
Created attachment 104558 [details] [review]
Bug 22437: Delete older merge requests at new merge time

If you merge A to B, we should remove older AA merges (regular
merge to itself) in the queue before deleting A.

Test plan:
[1] Set merge limit in prefs to say X.
[2] Find three authorities A, B, C with linkcount >X, >X, <X.
[3] Merge C to B. This creates a 'BB merge' in the queue.
    Auth C is deleted.
[4] Merge B to A. Creating AA and BA in the queue.
    Auth B is deleted.
    Optionally verify that BB is no longer in the queue.
[5] Run the merge_authorities.pl cron job.
    Verify that the linkcount to A is the former total of ABC.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <philringnalda@gmail.com>
Comment 41 Marcel de Rooy 2020-05-08 07:20:09 UTC
Created attachment 104559 [details] [review]
Bug 22437: (follow-up) Move deletion of merge requests to DelAuthority

Lines can be moved. Deletion can be done too if skip_merge is not set.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Jonathan Druart 2020-05-11 13:29:27 UTC
Created attachment 104698 [details] [review]
Bug 22437: Add test case to Merge.t

Test proves now that a 'destructive merge' is removed by
DelAuthority.

Test plan:
Run t/db_dependent/Authority/Merge.t. Should fail when you
do not apply the other two patches and pass with them.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Amended-patch: Fix spelling dont ==> don't
Comment 43 Jonathan Druart 2020-05-11 13:29:33 UTC
Created attachment 104699 [details] [review]
Bug 22437: Delete older merge requests at new merge time

If you merge A to B, we should remove older AA merges (regular
merge to itself) in the queue before deleting A.

Test plan:
[1] Set merge limit in prefs to say X.
[2] Find three authorities A, B, C with linkcount >X, >X, <X.
[3] Merge C to B. This creates a 'BB merge' in the queue.
    Auth C is deleted.
[4] Merge B to A. Creating AA and BA in the queue.
    Auth B is deleted.
    Optionally verify that BB is no longer in the queue.
[5] Run the merge_authorities.pl cron job.
    Verify that the linkcount to A is the former total of ABC.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <philringnalda@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Jonathan Druart 2020-05-11 13:29:40 UTC
Created attachment 104700 [details] [review]
Bug 22437: (follow-up) Move deletion of merge requests to DelAuthority

Lines can be moved. Deletion can be done too if skip_merge is not set.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 45 Marcel de Rooy 2020-05-11 13:33:49 UTC
(In reply to Jonathan Druart from comment #44)

Thanks
Comment 46 Martin Renvoize 2020-05-12 10:21:54 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 47 Joy Nelson 2020-05-13 16:17:03 UTC
Pushed to 19.11.x for 19.11.06
Comment 48 Lucas Gass 2020-05-15 22:36:19 UTC
backported to 19.05.x for 19.05.11