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.
Created attachment 25086 [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 - Indentation and whitespace fixes
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.
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.
Created attachment 25413 [details] [review] Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes Indentation and whitespace fixes
Created attachment 25414 [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
I split patch in two parts: 1) whitespace fixes 2) real changes I'll try to write unit tests asap
Created attachment 25426 [details] [review] Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge
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.
Created attachment 27970 [details] [review] Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes Rebased on master
Created attachment 27971 [details] [review] Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes Rebased on master
Created attachment 27972 [details] [review] Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge Rebased on master
Created attachment 27973 [details] [review] Bug 11700: Fix unit tests when index mode is 'dom'
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.
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?
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.
(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.
Created attachment 28176 [details] [review] Bug 11700: Avoid to try to delete the authority twice
(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...
Created attachment 28177 [details] [review] Bug 11700: Avoid to try to delete the authority twice
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.
Hm. I cannot find any reference to field 109 in unimarc too?
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..
(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.
(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.
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.
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.
Created attachment 35107 [details] [review] Bug 11700: Fix unit test when index mode is not defined Use the same default value as everywhere (dom)
(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.
Created attachment 39264 [details] [review] [SIGNED_OFF] Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes Indentation and whitespace fixes Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 39265 [details] [review] [SIGNED_OFF] 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>
Created attachment 39266 [details] [review] [SIGNED_OFF] Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 39267 [details] [review] [SIGNED_OFF] Bug 11700: Fix unit tests when index mode is 'dom' Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 39268 [details] [review] [SIGNED_OFF] Bug 11700: Avoid to try to delete the authority twice Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 39269 [details] [review] [SIGNED_OFF] Bug 11700: Fix unit test when index mode is not defined Use the same default value as everywhere (dom) Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Tested mergin auths, no problems Test runs Ok No koha-qa errors
Julian, can you please add a test plan? Also, please make the new tests output something useful :)
Created attachment 40968 [details] [review] [SIGNED_OFF] Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes Rebased on master
Created attachment 40969 [details] [review] [SIGNED_OFF] Bug 11700: C4::AuthoritiesMarc::merge enhancements/fixes Rebased on master
Created attachment 40970 [details] [review] [SIGNED_OFF] Bug 11700: Avoid to try to delete the authority twice Rebased on master
Created attachment 40971 [details] [review] [SIGNED_OFF] Bug 11700: Add unit tests for C4::AuthoritiesMarc::merge Rebased on master and squashed with others unit tests patches
Created attachment 40972 [details] [review] Bug 11700: Add description to unit tests
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)
This is a bit of a mixed bag of things - can you please add test plans to the 2nd and 3rd patch?
Created attachment 45048 [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>
Created attachment 45049 [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>
Created attachment 45050 [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>
Created attachment 45051 [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>
Created attachment 45052 [details] [review] Bug 11700: Add description to unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 45053 [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>
Patch set no longer applies cleanly, please rebase and reset status to passed qa!
Merging authorities is broken currently - see bug 15358
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>
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>
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>
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>
Created attachment 46401 [details] [review] Bug 11700: Add description to unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
All patches rebased on master. Resetting status to Signed off.
Setting this to BLOCKED because of bug 13762
needs rebase
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>
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>
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>
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>
Created attachment 56473 [details] [review] Bug 11700: Add description to unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
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?
(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
(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?
(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 :)
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.
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.
Can be closed now: see omnibus bug 17908 and bug 17913 *** This bug has been marked as a duplicate of bug 17913 ***