Bug 11700

Summary: C4::AuthoritiesMarc::merge enhancements/fixes
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: MARC Authority data supportAssignee: Julian Maurice <julian.maurice>
Status: CLOSED DUPLICATE QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: abl, bgkriegel, fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, kyle, m.de.rooy, martin.renvoize, mtompset
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13762
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13760
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5572
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 13706, 7419, 17908    
Attachments: Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge
Bug 11700: Fix unit tests when index mode is 'dom'
Bug 11700: Avoid to try to delete the authority twice
Bug 11700: Avoid to try to delete the authority twice
Bug 11700: Fix unit test when index mode is not defined
[SIGNED_OFF] Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
[SIGNED_OFF] Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
[SIGNED_OFF] Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge
[SIGNED_OFF] Bug 11700: Fix unit tests when index mode is 'dom'
[SIGNED_OFF] Bug 11700: Avoid to try to delete the authority twice
[SIGNED_OFF] Bug 11700: Fix unit test when index mode is not defined
[SIGNED_OFF] Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
[SIGNED_OFF] Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
[SIGNED_OFF] Bug 11700: Avoid to try to delete the authority twice
[SIGNED_OFF] Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge
Bug 11700: Add description to unit tests
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: Avoid to try to delete the authority twice
Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge
Bug 11700: Add description to unit tests
Bug 11700 [QA Followup] - Tidy merge a bit, fix warning triggered during unit tests
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: Avoid to try to delete the authority twice
Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge
Bug 11700: Add description to unit tests
Bug 11700 [QA Followup] - Tidy merge a bit, fix warning triggered during unit tests
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes
Bug 11700: Avoid to try to delete the authority twice
Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge
Bug 11700: Add description to unit tests
Bug 11700 [QA Followup] - Tidy merge a bit, fix warning triggered during unit tests

Description Julian Maurice 2014-02-06 15:18:10 UTC
Bug 7419 patches contains several enhancements and fixes for subroutine C4::AuthoritiesMarc::merge.
This bug is complicated to test so I will remove changes to this subroutine from bug 7419 and put it here. So maybe it will increase the chances to push these patches into master.
Comment 1 Julian Maurice 2014-02-06 15:18:44 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2014-02-18 17:21:45 UTC
Should not a change in functionality of core routine C4 or Koha mean that there should be an associated test for the routine somewhere?

I notice that this routine seems to be only used on authorities/merge.pl, and it doesn't even check the return value currently.
Comment 3 Mark Tompsett 2014-02-18 18:07:31 UTC
Please also note:
http://wiki.koha-community.org/wiki/Coding_Guidelines#PERL6:_Indentation
"If you want to reindent large amounts of existing code in connection with a bugfix or enhancement, you must do so in a separate patch from the one with code changes (unless the reindent is required due to the addition or removal of an indentation level (for example, an additional if added around a block)."

1422 -- not necessary.
There are several other lines, which I am assuming are white space fixes.
1497-1512 -- fixed indention was not necessary.
Comment 4 Julian Maurice 2014-02-19 08:23:36 UTC Comment hidden (obsolete)
Comment 5 Julian Maurice 2014-02-19 08:23:39 UTC Comment hidden (obsolete)
Comment 6 Julian Maurice 2014-02-19 08:27:09 UTC
I split patch in two parts: 1) whitespace fixes 2) real changes

I'll try to write unit tests asap
Comment 7 Julian Maurice 2014-02-19 10:32:01 UTC Comment hidden (obsolete)
Comment 8 Bernardo Gonzalez Kriegel 2014-04-22 19:41:02 UTC
Small conflict on t/db_dependent/AuthoritiesMarc.t, but problem is
that tests fail

prove -v t/db_dependent/AuthoritiesMarc.t
t/db_dependent/AuthoritiesMarc.t ..
1..15
ok 1 - use C4::AuthoritiesMarc;
"my" variable $dbh masks earlier declaration in same scope at t/db_dependent/AuthoritiesMarc.t line 196.
ok 2 - Built linked authtrees hierarchy string
ok 3 - Generated hierarchy data structure for linked hierarchy
ok 4 - Built unlinked authtrees hierarchy string
ok 5 - Generated hierarchy data structure for unlinked hierarchy
ok 6 - test BuildSummary for MARC21
ok 7 - BuildSummary does not generate warning if main heading subfield not present
ok 8 - test BuildSummary for UNIMARC
Can't call method "subfield" on an undefined value at /home/bgkriegel/kohaclone/C4/AuthoritiesMarc.pm line 1476.
# Looks like you planned 15 tests but ran 8.
Comment 9 Julian Maurice 2014-05-06 14:01:19 UTC Comment hidden (obsolete)
Comment 10 Julian Maurice 2014-05-06 14:01:33 UTC Comment hidden (obsolete)
Comment 11 Julian Maurice 2014-05-06 14:01:41 UTC Comment hidden (obsolete)
Comment 12 Julian Maurice 2014-05-06 14:01:47 UTC Comment hidden (obsolete)
Comment 13 Julian Maurice 2014-05-06 14:07:29 UTC
Problem was the mock for ZOOM::Record::raw was always returning iso2709 records whereas new_record_from_zebra expect MARCXML when biblio index mode is 'dom'.
Tests should pass now.
Comment 14 Martin Renvoize 2014-05-08 14:00:46 UTC
How does this bug relate to bug 5572?

The description of this bug is rather generic, could you specify what 'enhancements' this actually brings to the table please?
Comment 15 Galen Charlton 2014-05-08 15:50:32 UTC
This patch series should be accompanied by an update to authorities/merge.pl for two reasons:

[1] Why change the return of C4::AuthoritiesMarc::merge if the return value is going to be thrown away?
[2] authorities/merge.pl has a call to DelAuthority, but that would be made redundant by this patch series.
Comment 16 Jonathan Druart 2014-05-12 11:35:10 UTC
(In reply to Galen Charlton from comment #15)
> [1] Why change the return of C4::AuthoritiesMarc::merge if the return value
> is going to be thrown away?

This return value is used by UT and by the script dedup_authorities.pl introduced by bug 7419.

> [2] authorities/merge.pl has a call to DelAuthority, but that would be made
> redundant by this patch series.

Yes it is. Patch coming.
Comment 17 Jonathan Druart 2014-05-12 11:48:34 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2014-05-12 11:49:15 UTC
(In reply to Jonathan Druart from comment #17)
> Created attachment 28176 [details] [review] [review]
> Bug 11700: Avoid to try to delete the authority twice

Not sure dying is the best thing to do...
Comment 19 Jonathan Druart 2014-05-12 11:59:29 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2015-01-12 11:58:34 UTC
Patch still applies!
+1 for unit tests.
I have some hesitations however on signing this off:

[1] The unit test contains:
$dbh->do("INSERT INTO auth_types(authtypecode, authtypetext, auth_tag_to_report, summary) VALUES('TEST_PERSO', 'Personal Name', '109', 'Personal Names');");
This is only valid for UNIMARC. What about MARC21?

[2] You do not include test plans. This could be a reason why this report has not received much attention lately.

Changing status to Failed QA.
Comment 21 Marcel de Rooy 2015-01-12 12:09:56 UTC
Hm. I cannot find any reference to field 109 in unimarc too?
Comment 22 Marcel de Rooy 2015-01-12 12:25:29 UTC
OK. I had a closer look. But we are not there yet:

I added some debug prints to the unit test:

warn "L251a"; my @biblios = C4::AuthoritiesMarc::merge($authid2, undef, $authid1, undef); warn "L251b";

produces this:

L251a at t/db_dependent/AuthoritiesMarc.t line 251.
Use of uninitialized value $self in pattern match (m//) at /usr/local/share/perl5/MARC/File/XML.pm line 441.
Can't call method "subfield" on an undefined value at /usr/share/koha/testclone/C4/AuthoritiesMarc.pm line 1483.
# Looks like you planned 15 tests but ran 8.
# Looks like your test exited with 255 just after 8.

Since the 251b warn is not issued anymore, the error is somewhere in the call to merge..
Comment 23 Jonathan Druart 2015-01-12 12:34:25 UTC
(In reply to M. de Rooy from comment #21)
> Hm. I cannot find any reference to field 109 in unimarc too?

Julian has certainly chosen 109 arbitrarily.
The tests pass on a MARC21 installation.
Comment 24 Marcel de Rooy 2015-01-12 12:39:49 UTC
(In reply to Jonathan Druart from comment #23)
> (In reply to M. de Rooy from comment #21)
> > Hm. I cannot find any reference to field 109 in unimarc too?
> 
> Julian has certainly chosen 109 arbitrarily.
> The tests pass on a MARC21 installation.

Not always. See comment22.
Comment 25 Julian Maurice 2015-01-12 15:26:49 UTC
Hi Marcel,

Unfortunately I am not able to reproduce your problem.
Could you add some debug prints in the mock of ZOOM::Record::raw (line 242 of t/db_dependent/AuthoritiesMarc.t) ?
Maybe something like this, just before the return:
  warn "biblionumber: " . $this->{_rec}->{biblionumber};
  warn "marc: " . $marcs->[0];

This should tell us if the mocks don't work, if the record is not inserted in database, or if it's something else.
Comment 26 Julian Maurice 2015-01-12 15:50:18 UTC
Jonathan reproduced the bug on one of the sandbox, so no need to do this on your side.

I think I've found the problem and will submit a patch.
Comment 27 Julian Maurice 2015-01-12 15:55:29 UTC Comment hidden (obsolete)
Comment 28 Fridolin Somers 2015-02-26 14:51:50 UTC
(In reply to Jonathan Druart from comment #19)
> Created attachment 28177 [details] [review] [review]
> Bug 11700: Avoid to try to delete the authority twice
There is also a delete after merge in misc/migration_tools/merge_authority.pl.

(In reply to Jonathan Druart from comment #18)
> (In reply to Jonathan Druart from comment #17)
> > Created attachment 28176 [details] [review] [review] [review]
> > Bug 11700: Avoid to try to delete the authority twice
> 
> Not sure dying is the best thing to do...
Same for me, a warn is enough.
Comment 29 Bernardo Gonzalez Kriegel 2015-05-17 22:53:03 UTC Comment hidden (obsolete)
Comment 30 Bernardo Gonzalez Kriegel 2015-05-17 22:53:17 UTC Comment hidden (obsolete)
Comment 31 Bernardo Gonzalez Kriegel 2015-05-17 22:53:24 UTC Comment hidden (obsolete)
Comment 32 Bernardo Gonzalez Kriegel 2015-05-17 22:53:31 UTC Comment hidden (obsolete)
Comment 33 Bernardo Gonzalez Kriegel 2015-05-17 22:53:38 UTC Comment hidden (obsolete)
Comment 34 Bernardo Gonzalez Kriegel 2015-05-17 22:53:45 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2015-06-07 21:34:05 UTC
Julian, can you please add a test plan? 
Also, please make the new tests output something useful :)
Comment 36 Julian Maurice 2015-07-13 14:08:47 UTC Comment hidden (obsolete)
Comment 37 Julian Maurice 2015-07-13 14:09:09 UTC Comment hidden (obsolete)
Comment 38 Julian Maurice 2015-07-13 14:09:34 UTC Comment hidden (obsolete)
Comment 39 Julian Maurice 2015-07-13 14:10:16 UTC Comment hidden (obsolete)
Comment 40 Julian Maurice 2015-07-13 14:10:26 UTC Comment hidden (obsolete)
Comment 41 Julian Maurice 2015-07-13 14:13:50 UTC
Test plan:
1/ Launch unit tests: prove t/db_dependent/AuthoritiesMarc.t
2/ Check the authorities merge feature still works (search for authorities in staff interface and click 'Merge' on two different authorities, possibly used by 1 or more biblios)
Comment 42 Katrin Fischer 2015-11-20 14:31:45 UTC
This is a bit of a mixed bag of things - can you please add test plans to the 2nd and 3rd patch?
Comment 43 Kyle M Hall 2015-11-20 14:43:04 UTC Comment hidden (obsolete)
Comment 44 Kyle M Hall 2015-11-20 14:43:14 UTC Comment hidden (obsolete)
Comment 45 Kyle M Hall 2015-11-20 14:43:18 UTC Comment hidden (obsolete)
Comment 46 Kyle M Hall 2015-11-20 14:43:21 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2015-11-20 14:43:25 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2015-11-20 14:43:29 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2015-12-30 12:20:32 UTC
Patch set no longer applies cleanly, please rebase and reset status to passed qa!
Comment 50 Katrin Fischer 2015-12-30 12:23:16 UTC
Merging authorities is broken currently - see bug 15358
Comment 51 Julian Maurice 2016-01-07 15:13:27 UTC
Created attachment 46397 [details] [review]
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes

Indentation and whitespace fixes

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 52 Julian Maurice 2016-01-07 15:13:31 UTC
Created attachment 46398 [details] [review]
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes

- Returns the list of modified biblio records
- Retrieve MARC records from DB if not given in parameters
- Delete source authority if source and destination records are
  different

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 53 Julian Maurice 2016-01-07 15:13:35 UTC
Created attachment 46399 [details] [review]
Bug 11700: Avoid to try to delete the authority twice

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 54 Julian Maurice 2016-01-07 15:13:39 UTC
Created attachment 46400 [details] [review]
Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 55 Julian Maurice 2016-01-07 15:13:43 UTC
Created attachment 46401 [details] [review]
Bug 11700: Add description to unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 56 Julian Maurice 2016-01-07 15:13:47 UTC
Created attachment 46402 [details] [review]
Bug 11700 [QA Followup] - Tidy merge a bit, fix warning triggered during unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 57 Julian Maurice 2016-01-07 15:14:58 UTC
All patches rebased on master.
Resetting status to Signed off.
Comment 58 Katrin Fischer 2016-01-08 09:31:02 UTC
Setting this to BLOCKED because of bug 13762
Comment 59 Fridolin Somers 2016-10-10 14:28:00 UTC
needs rebase
Comment 60 Julian Maurice 2016-10-14 08:17:57 UTC
Created attachment 56469 [details] [review]
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes

Indentation and whitespace fixes

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 61 Julian Maurice 2016-10-14 08:18:03 UTC
Created attachment 56470 [details] [review]
Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes

- Returns the list of modified biblio records
- Retrieve MARC records from DB if not given in parameters
- Delete source authority if source and destination records are
  different

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 62 Julian Maurice 2016-10-14 08:18:11 UTC
Created attachment 56471 [details] [review]
Bug 11700: Avoid to try to delete the authority twice

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 63 Julian Maurice 2016-10-14 08:18:17 UTC
Created attachment 56472 [details] [review]
Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 64 Julian Maurice 2016-10-14 08:18:22 UTC
Created attachment 56473 [details] [review]
Bug 11700: Add description to unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 65 Julian Maurice 2016-10-14 08:18:28 UTC
Created attachment 56474 [details] [review]
Bug 11700 [QA Followup] - Tidy merge a bit, fix warning triggered during unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 66 Jonathan Druart 2016-11-25 09:16:16 UTC
All the indentation changes make this patchset really hard to read, but basically it seems that there are only 2 changes:

1/
Add
  +    $MARCfrom ||= GetAuthority($mergefrom);
  +    $MARCto ||= GetAuthority($mergeto);
in C4::AuthoritiesMarc::merge
In which cases that could be useful?

2/ Move DelAuthority to C4::AuthoritiesMarc::merge
Sounds ok, but should not change anything in the behavior.

So, why all these changes just for that?
Comment 67 Jacek Ablewicz 2016-11-25 10:02:04 UTC
(In reply to Jonathan Druart from comment #66)


> 1/
> Add
>   +    $MARCfrom ||= GetAuthority($mergefrom);

$MARCfrom (and @record_from) are currently not used in this function for anything at all
Comment 68 Jacek Ablewicz 2016-11-25 10:11:52 UTC
(In reply to Jonathan Druart from comment #66)
> All the indentation changes make this patchset really hard to read, but
> basically it seems that there are only 2 changes:

> So, why all these changes just for that?

There are a lot of (conflicting) reports in queue, dealing with various issues with authority merge (including some with critical fixes), but they don't seem to be going forward for a long long time and will need to be rebased / adjusted anyway, maybe indentation fixes and tidying up by itself will be a good start for that?
Comment 69 Jonathan Druart 2016-11-25 10:15:29 UTC
(In reply to Jacek Ablewicz from comment #68)
> (In reply to Jonathan Druart from comment #66)
> > All the indentation changes make this patchset really hard to read, but
> > basically it seems that there are only 2 changes:
> 
> > So, why all these changes just for that?
> 
> There are a lot of (conflicting) reports in queue, dealing with various
> issues with authority merge (including some with critical fixes), but they
> don't seem to be going forward for a long long time and will need to be
> rebased / adjusted anyway, maybe indentation fixes and tidying up by itself
> will be a good start for that?

I'd say the opposite, to avoid all of the other patches to conflict for indentation changes, it would be better to tidy it later :)
Comment 70 Martin Renvoize 2016-11-28 09:56:03 UTC
I often tidy the file for my own sanity and commit just that tidy.. then I commit the actual fixes in the next commit(s) and backport them to the untidy branch if needed for submission.
Comment 71 Jonathan Druart 2016-12-12 11:37:39 UTC
Could someone answer me and tell me what the whole point of this patch set and what is it suppose to do?
Removed from the QA queue.
Comment 72 Marcel de Rooy 2017-01-23 08:10:27 UTC
Can be closed now: see omnibus bug 17908 and bug 17913

*** This bug has been marked as a duplicate of bug 17913 ***