https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #2 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #4 from Pedro Amorim <pjamorim91@gmail.com> --- Created attachment 95848 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=95848&action=edit Previous attachment had a typo -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Andreas Roussos <arouss1980@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |arouss1980@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Good catch, Pedro. I will be looking into it. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #6 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Should this be Need Signoff? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #95848|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 102338 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102338&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102338|0 |1 is obsolete| | --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 102444 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102444&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Updating severity for potential data loss (although the C->B, B->A merge might not be that common, fortunately) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |phil@chetcolibrary.org -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #18 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 :)

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #102445|0 |1 is obsolete| | --- Comment #23 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 103299 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103299&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #25 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #27 from Phil Ringnalda <phil@chetcolibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #29 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Have you thought of using some sort of semaphore, like adding a column to authority records: 'waiting_merge' and act depending on its value? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #31 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #33 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #35 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #37 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #36)

in merge then? :)

Hmm. Thats tougher. Need to think about that. Sounds like possible. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103298|0 |1 is obsolete| | Attachment #103299|0 |1 is obsolete| | --- Comment #39 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 104557 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104557&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #41 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 104559 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104559&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104557|0 |1 is obsolete| | Attachment #104558|0 |1 is obsolete| | Attachment #104559|0 |1 is obsolete| | --- Comment #42 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 104698 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104698&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 --- Comment #44 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 104700 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104700&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |20.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com Version(s)|20.05.00 |20.05.00, 19.11.06 released in| | Status|Pushed to master |Pushed to stable --- Comment #47 from Joy Nelson <joy@bywatersolutions.com> --- Pushed to 19.11.x for 19.11.06 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22437 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_05_candidate | -- You are receiving this mail because: You are watching all bug changes.