Bug 15358 - merge.pl does not populate values to merge
Summary: merge.pl does not populate values to merge
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P1 - high blocker (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 15370 15410 (view as bug list)
Depends on: 8064 15578
Blocks: 11315
  Show dependency treegraph
 
Reported: 2015-12-11 23:08 UTC by Ian Palko
Modified: 2017-06-14 22:18 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Blank authorities merge screen after selecting reference record (64.68 KB, image/png)
2015-12-14 15:40 UTC, Ian Palko
Details
Bug 15358: Repopulate fields for authority merge (10.51 KB, patch)
2015-12-17 11:34 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15358: Attempt to fix authorities merge (10.21 KB, patch)
2016-01-06 18:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 15358: Build result preview after page load (966 bytes, patch)
2016-01-07 14:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 15358: Authorities merge - submit only result record (1.07 KB, patch)
2016-01-12 12:52 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 15358: Attempt to fix authorities merge (8.65 KB, patch)
2016-01-14 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15358: Build result preview after page load (966 bytes, patch)
2016-01-14 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15358: Authorities merge - submit only result record (1.07 KB, patch)
2016-01-14 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15358: Attempt to fix authorities merge (8.70 KB, patch)
2016-02-01 15:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15358: Build result preview after page load (1022 bytes, patch)
2016-02-01 15:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15358: Authorities merge - submit only result record (1.12 KB, patch)
2016-02-01 15:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15358: Attempt to fix authorities merge (8.77 KB, patch)
2016-02-02 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15358: Build result preview after page load (1.07 KB, patch)
2016-02-02 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15358: Authorities merge - submit only result record (1.19 KB, patch)
2016-02-02 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15358: Fix authorities merge (9.26 KB, patch)
2016-02-02 10:27 UTC, Julian Maurice
Details | Diff | Splinter Review
[3.22.x] Bug 15358: Fix authorities merge (7.75 KB, patch)
2016-02-04 11:15 UTC, Julian Maurice
Details | Diff | Splinter Review
[Signed-off] [3.22.x] Bug 15358: Fix authorities merge (7.81 KB, patch)
2016-02-04 13:19 UTC, Josef Moravec
Details | Diff | Splinter Review
[3.22.x] Bug 15358: Fix authorities merge (7.87 KB, patch)
2016-02-17 10:30 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 Ian Palko 2015-12-11 23:08:06 UTC
In 3.22.0 when attempting to use merge.pl, there is strange behavior.

1) When merging biblios, the records appear, but the Desitation Record is not automatically populated. This despite that all the checkboxes on the reference record are checked. One must manually check each box. Unchecking a box resets the whole list.

As a side note: the new report generated after the merge forces the librarian to click a link to go to the new record. Previously, it merge simply redirected to the merged record. The report gives no new information. Perhaps I don't understand, but the report seems to add a needless step without significant reason. We can only merge two records at a time, so the merge report will always be quite trivial.

2) When attempting to merge authorities, merge.pl does not display the contents of either records, breaking merging of authorities.

I have repeated this behavior on two different systems.

I wonder if this has something to do with the new behavior pushed with Bug 8064.
Comment 1 Ian Palko 2015-12-14 01:22:21 UTC
Further research to help resolve this problem shows:

1) merge.pl for biblios does populate the ajax destination record, but this takes nearly 5 times longer than it did previously. (<1 sec in 3.18.6, now 7-10 sec in 3.22.0)

So, that part of the bug just stems from my own impatience. Though perhaps recent changes have made this process more demanding on the server.

I still suspect some problem with the fix to Bug 8064. I get the behavior in <a href="http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8064#c18">Comment 18</a> when merging biblios.

2) The screen after choosing the reference record and framework on merge.pl for authorities displays a totally blank ajax table.

Looking at the changes 8064 made to /cataloguing/merge.tt there do not seem to be corresponding changes made to /authorities/merge.tt.

Obviously some of the changes will not apply (the mandatory fields part), but looking at cataloguing/merge.tt around line 88, and then authorities/merge.tt around line 22, there seem to be some inconsistencies, though my coding experience is quite limited.

I will attempt on my test server to repair what I can and see about reporting back as time permits.
Comment 2 Ian Palko 2015-12-14 15:40:19 UTC
Created attachment 45666 [details]
Blank authorities merge screen after selecting reference record

Example of the output of authorities/merge.pl after selecting the reference authority and clicking "Merge" to populate the tabs.
Comment 3 Bernardo Gonzalez Kriegel 2015-12-14 22:57:27 UTC
Verified in packages install. This is bad, rising severity.
Comment 4 Ian Palko 2015-12-15 14:49:25 UTC
Another update from some digging around :

I am now quite certain Bug 8064 is the main culprit.

The changes affect key files shared between the authorities and biblio merge systems.

The changes were not benign to authorities. And looking at how things were modified, it seems the authority merging functionality was forgotten.

This is most evident is the new code added in the shared merge-record.inc. Its changes are very biblio specific. But also clearly affected are merge.tt, merge.pl, etc.

I suspect more is involved, though as replacing with older versions of the files I was not able to get authorities merging back on a test server, however, I spent only a few hours trying to hack it out, and have precious little experience.
Comment 5 Bernardo Gonzalez Kriegel 2015-12-16 00:28:00 UTC
Bug 8064 has it's merits in this problem, but is not alone.

If you go back far enough( git reset --hard HEAD~144, just before 8064), you get populated fields, but the merge will fail with "Subfields are only for data fields (generally, just tags >= 010) at /home/bgkriegel/kohaclone/C4/AuthoritiesMarc.pm line 1536."

You need to go way back ( git reset --hard HEAD~774, just before Bug 14639 ) to have a successful auth merge.

In authorities/merge.pl, $recordObj1->authtype method returns 'undef',
it's a Koha::MetadataRecord object. That was modified in Bug 14639
Comment 6 Bernardo Gonzalez Kriegel 2015-12-17 11:34:47 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2015-12-17 21:43:55 UTC
*** Bug 15370 has been marked as a duplicate of this bug. ***
Comment 8 Ian Palko 2015-12-19 17:10:49 UTC
Bernardo,

With Marc planning to move Koha::Authorities over to Koha::MetadataRecord::Authorities, will this help resolve the issues you're seeing once the Bug 8064 changes are reverted?

I would hate to be trying to resolve this problem if his fix over at Bug 15380 will further complicate matters.

Perhaps, though, it will solve part of the problem and make your patch work.

Also, as a fix, perhaps the easier short-term solution would be to separate the biblios and authorities merging systems. We could revert authorities merging to the older files if the MetadataRecord issue could be ironed out, but leave the biblios system as is.

Looks like a lot of work to try to harmonize biblios and authorities merge at this point given all the changes that were made before this was noticed.
Comment 9 Katrin Fischer 2015-12-27 20:30:44 UTC
*** Bug 15410 has been marked as a duplicate of this bug. ***
Comment 10 Jonathan Druart 2016-01-06 11:37:49 UTC
Julian, could you please have a look at this blocker please?
I would like a better solution than the Bernardo's patch.
Comment 11 Julian Maurice 2016-01-06 18:15:35 UTC Comment hidden (obsolete)
Comment 12 Julian Maurice 2016-01-07 14:11:43 UTC Comment hidden (obsolete)
Comment 13 Ian Palko 2016-01-07 14:54:43 UTC
I don't have a git setup yet, but a 3.22.0 package setup on a test server, so for what it's worth ...

I have not updated yet to 3.22.1, but will do so tomorrow morning, so can try to test it then on the latest packages.

I've manually edited the relevant files in accordance with Julian's patches here. It does seem to fix things back to merging, but probably more thorough test are certainly in order from those who actually are competent here. I'm just a librarian and armchair hacker ...

If all works tomorrow when I test on the latest packages, I'll signoff. If anyone is able to test before then, I'm more than happy to let them do it.
Comment 14 Katrin Fischer 2016-01-07 15:17:39 UTC
Ian, your testing is much appreciated!
Comment 15 Jonathan Druart 2016-01-07 15:35:27 UTC
(In reply to Ian Palko from comment #13)
> I've manually edited the relevant files in accordance with Julian's patches
> here.

Please, help yourself, don't do that :)
git is your friend and can help you.
Comment 16 Josef Moravec 2016-01-07 20:32:42 UTC
I just tested the patch, the records and result are displayed as they should. But after merging, the resulted record has all the data from both records!
Comment 17 Katrin Fischer 2016-01-08 08:41:11 UTC
Failed QA last night because of Josef's comment. Glad to see this move forward, hope remaining problems can be solved.
Comment 18 Ian Palko 2016-01-08 16:31:50 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Ian Palko from comment #13)
> > I've manually edited the relevant files in accordance with Julian's patches
> > here.
> 
> Please, help yourself, don't do that :)
> git is your friend and can help you.

I know...

Eventually that will happen. For now, I need a backup server at the ready "just in case", as we have had some issues with the production server. It will be replaced (fingers crossed) thus summer when we move to a new building with a new server waiting.
Comment 19 Ian Palko 2016-01-08 16:32:58 UTC
(In reply to Josef Moravec from comment #16)
> I just tested the patch, the records and result are displayed as they
> should. But after merging, the resulted record has all the data from both
> records!

I can confirm this behavior.

One step closer, though ...
Comment 20 Jonathan Druart 2016-01-12 12:29:05 UTC
Patch on bug 6657 could affect this one too.
Comment 21 Julian Maurice 2016-01-12 12:52:10 UTC Comment hidden (obsolete)
Comment 22 Frédéric Demians 2016-01-12 14:24:51 UTC
It's better. Now the merge form is displayed. Authorities are merged. BUT Biblio records containing the rejected authority are not relinked to the retained authority.
Comment 23 Jonathan Druart 2016-01-14 12:32:13 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2016-01-14 12:32:17 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2016-01-14 12:32:21 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2016-01-14 12:33:19 UTC
Trying to make this moving forward, I have moved some code to bug 15578 (with tests).
Comment 27 Nick Clemens 2016-01-14 17:41:52 UTC
A test plan would be appreciated (even if as simple as 'Merge two authorities')

I get this error when merging authorities:
Software error:

Subfields are only for data fields (generally, just tags >= 010) at /home/vagrant/kohaclone/C4/AuthoritiesMarc.pm line 1488.

For help, please send mail to the webmaster ([no address given]), giving this error message and the time and date of the error.
Comment 28 Jonathan Druart 2016-01-15 10:11:40 UTC
(In reply to Nick Clemens from comment #27)
> A test plan would be appreciated (even if as simple as 'Merge two
> authorities')
> 
> I get this error when merging authorities:
> Software error:
> 
> Subfields are only for data fields (generally, just tags >= 010) at
> /home/vagrant/kohaclone/C4/AuthoritiesMarc.pm line 1488.

I had a very quick look at the code and I'd say that it existed prior to bug 8064, could you test on a 3.20 release and confirm that the bug already existed?
Comment 29 Ian Palko 2016-01-28 19:57:13 UTC
(In reply to Jonathan Druart from comment #28)
> (In reply to Nick Clemens from comment #27)
> > A test plan would be appreciated (even if as simple as 'Merge two
> > authorities')
> > 
> > I get this error when merging authorities:
> > Software error:
> > 
> > Subfields are only for data fields (generally, just tags >= 010) at
> > /home/vagrant/kohaclone/C4/AuthoritiesMarc.pm line 1488.
> 
> I had a very quick look at the code and I'd say that it existed prior to bug
> 8064, could you test on a 3.20 release and confirm that the bug already
> existed?

Equinox is still running a demo server with 3.20.1.

When merging authorities on that demo server, there was no error. The biblios were correctly re-linked to the merged authority as well.
Comment 30 Jonathan Druart 2016-01-29 09:40:43 UTC
Yes, it works. The plan was to test with the same records on 3.20 and master + this patch set
Comment 31 Nick Clemens 2016-02-01 15:46:51 UTC Comment hidden (obsolete)
Comment 32 Nick Clemens 2016-02-01 15:46:58 UTC Comment hidden (obsolete)
Comment 33 Nick Clemens 2016-02-01 15:47:03 UTC Comment hidden (obsolete)
Comment 34 Nick Clemens 2016-02-01 15:47:59 UTC
Retesting I couldn't recreate that error, seems to have been in my records, but I reloaded since then and all tests I tried worked fine.
Comment 35 Jonathan Druart 2016-02-01 16:26:55 UTC
(In reply to Nick Clemens from comment #34)
> Retesting I couldn't recreate that error, seems to have been in my records,
> but I reloaded since then and all tests I tried worked fine.

You error was about control field (<10), did you try that?
Comment 36 Nick Clemens 2016-02-01 16:53:51 UTC
I merged records with control fields and was able to mix and match from separate records with no issue.  I tried creating subfields in control fields in MarcEdit but had no success
Comment 37 Jonathan Druart 2016-02-02 10:01:11 UTC Comment hidden (obsolete)
Comment 38 Jonathan Druart 2016-02-02 10:01:19 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2016-02-02 10:01:23 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2016-02-02 10:07:00 UTC
Julian, I have found a bug when testing:
I a non-repeatable subfield is added when a repeatable field is added, there is no warning.
If I click on the subfield, I get "The subfield is non-repeatable and already exists in the destination record. Therefore, you cannot add it." and it is not added to the destination record.
Comment 41 Julian Maurice 2016-02-02 10:27:54 UTC
Created attachment 47543 [details] [review]
Bug 15358: Fix authorities merge

Bug 8064 (Merge several biblio records) change some code used in both
biblios and authorities merge tool without updating the authorities
merge tool.
This patch fixes that.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 42 Julian Maurice 2016-02-02 10:28:18 UTC
All 3 patches squashed
Comment 43 Julian Maurice 2016-02-02 10:30:57 UTC
(In reply to Jonathan Druart from comment #40)
> Julian, I have found a bug when testing:
> I a non-repeatable subfield is added when a repeatable field is added, there
> is no warning.
> If I click on the subfield, I get "The subfield is non-repeatable and
> already exists in the destination record. Therefore, you cannot add it." and
> it is not added to the destination record.

I think it's normal behaviour if I understand correctly. The subfield is not repeatable in its own field, but here you add a new field in the destination record, so it's valid.
Comment 44 Jonathan Druart 2016-02-02 11:21:11 UTC
(In reply to Julian Maurice from comment #43)
> (In reply to Jonathan Druart from comment #40)
> > Julian, I have found a bug when testing:
> > I a non-repeatable subfield is added when a repeatable field is added, there
> > is no warning.
> > If I click on the subfield, I get "The subfield is non-repeatable and
> > already exists in the destination record. Therefore, you cannot add it." and
> > it is not added to the destination record.
> 
> I think it's normal behaviour if I understand correctly. The subfield is not
> repeatable in its own field, but here you add a new field in the destination
> record, so it's valid.

Indeed!
Comment 45 Brendan Gallagher 2016-02-03 23:04:31 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 46 Julian Maurice 2016-02-04 11:15:04 UTC Comment hidden (obsolete)
Comment 47 Julian Maurice 2016-02-04 11:16:01 UTC
Patch for 3.22.x is slightly different from original patch. I would like to see it tested on 3.22 before I push it.
Comment 48 Julian Maurice 2016-02-04 11:17:01 UTC
Note that it requires [3.22.x] patches from bug 15578
Comment 49 Josef Moravec 2016-02-04 13:19:53 UTC Comment hidden (obsolete)
Comment 50 Ian Palko 2016-02-13 16:32:10 UTC
To clarify (Sorry):

Will this be pushed to 3.22, or just the Master?

It would be nice to have the fix before 3.24 comes out for those of us with lots of authorities to clean up.

If it hasn't been pushed to 3.22, would it be possible to test and push to that branch?

Many thanks.
Comment 51 Julian Maurice 2016-02-15 07:49:10 UTC
Hi Ian,

it will be pushed to 3.22. I will add a comment when it's done.
Comment 52 Jonathan Druart 2016-02-17 10:30:20 UTC
Created attachment 48155 [details] [review]
[3.22.x] Bug 15358: Fix authorities merge

Bug 8064 (Merge several biblio records) change some code used in both
biblios and authorities merge tool without updating the authorities
merge tool.
This patch fixes that.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 53 Julian Maurice 2016-02-17 18:55:08 UTC
Patch pushed to 3.22.x, will be in 3.22.4

Thanks Jonathan and Josef
Comment 54 Frédéric Demians 2016-02-20 08:49:37 UTC
I can't make it work properly on 3.20 branch. Code has diverged too much.
Comment 55 Jacek Ablewicz 2016-04-28 07:58:31 UTC
This fixed authority merging, but biblio records merging is still broken in master and stable (3.22.4 +) branches; see Bug 16373 for the follow-up.
Comment 56 sepas91 2017-02-13 12:24:24 UTC
In koha 3.22, when I merge authorities without records it works. However, if they have some one I get this error:

Software error:
Control fields (generally, just tags below 010) do not have subfields, use data() at /var/www/html/koha/lib/C4/AuthoritiesMarc.pm line 1523
Comment 57 Marcel de Rooy 2017-02-13 12:31:56 UTC
(In reply to sepas91 from comment #56)
> In koha 3.22, when I merge authorities without records it works. However, if
> they have some one I get this error:
> 
> Software error:
> Control fields (generally, just tags below 010) do not have subfields, use
> data() at /var/www/html/koha/lib/C4/AuthoritiesMarc.pm line 1523

Please have a look at the bugs under bug 17908. Including bug 17913.
Pushed to master, but not yet to stable. 
So, some improvements are coming..