Bug 9755

Summary: Record merge code needs to be refactored
Product: Koha Reporter: Jared Camins-Esakov <jcamins>
Component: Architecture, internals, and plumbingAssignee: Jared Camins-Esakov <jcamins>
Status: CLOSED FIXED QA Contact: Chris Cormack <chris>
Severity: enhancement    
Priority: P5 - low CC: chris, gmcharlt, katrin.fischer, mathsabypro, paul.poulain
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10551
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 5202, 8064, 10650    
Attachments: Bug 9755: Refactor record merge functionality
Bug 9755: Refactor record merge functionality
Bug 9755: Refactor record merge functionality
Bug 9755 QA follow-up: move MARC-specific functionality to utility class
Bug 9755 QA follow-up: move MARC-specific functionality to utility class
Bug 9755 QA follow-up: move MARC-specific functionality to utility class
Bug 9755 QA follow-up: move MARC-specific functionality to utility class
Subject: [PATCH][SIGNED OFF] Bug 9755 QA follow-up: move MARC-specific functionality to utility class
Bug 9755 QA follow-up: fix template compliance
[PASSED QA] Bug 9755: Refactor record merge functionality
[PASSED QA] Bug 9755 QA follow-up: move MARC-specific functionality to utility class
[PASSED QA] Bug 9755 QA follow-up: fix template compliance

Description Jared Camins-Esakov 2013-03-06 15:24:45 UTC
In order to make it possible to merge authority records, the record merge functionality in Koha needs to be refactored so that there is less duplicated code and the code can be used for both bibs and authorities.
Comment 1 Jared Camins-Esakov 2013-03-06 16:52:53 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2013-03-06 20:55:14 UTC
Patch tested with a sandbox, by Mathieu Saby <mathieu.saby@univ-rennes2.fr>
Comment 3 Paul Poulain 2013-03-06 20:55:32 UTC Comment hidden (obsolete)
Comment 4 Mathieu Saby 2013-03-06 20:58:39 UTC
I tested on sandbox 3. 
I merged records biblionumber 94 and 97, keeping the 94.
I tried to copy some fields of the 97, and to suppress some fields of the 94.
I tried to copy some non repeatable fields.

It works as before.

M. Saby
Comment 5 Chris Cormack 2013-05-28 19:32:27 UTC
QAing this today
Comment 6 Mathieu Saby 2013-05-28 19:33:17 UTC
I try it today, there is a little conflict.

M. Saby
Comment 7 Chris Cormack 2013-05-28 22:15:12 UTC
Passes the QA scripts

 OK     Koha/Authority.pm
 OK     Koha/Record.pm
 OK     cataloguing/merge.pl
 OK     t/Koha_Record.t
 OK     koha-tmpl/intranet-tmpl/prog/en/includes/merge-record.inc
 OK     koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt

perl t/Koha_Record.t            
ok 1 - use Koha::Record;
ok 2 - Created valid Koha::Record object
ok 3 - Generated hash correctly
ok 4 - No duplicate keys

Unit tests pass

prove t passes all tests

Testing functionality now
Comment 8 Chris Cormack 2013-05-28 22:34:10 UTC
Merge worked fine in the interface.
Comment 9 Chris Cormack 2013-05-28 22:34:43 UTC Comment hidden (obsolete)
Comment 10 Mathieu Saby 2013-05-29 07:13:45 UTC
Jared, could you tell my why you exclude 999 field in this line of createMarcHash ?
I don't know how the 999 is used by Koha...

+        if ((!defined($tagslib) || $tagslib->{$fieldtag}->{'tab'} >= 0) && $fieldtag ne '995' && $fieldtag ne '999') {


M. Saby
Comment 11 Mathieu Saby 2013-05-29 07:15:50 UTC
As 995 is excluded (for UNIMARC), I suppose 952 should have do (for Marc21) ?

M. Saby
Comment 12 Chris Cormack 2013-05-29 07:52:39 UTC
(In reply to comment #10)
> Jared, could you tell my why you exclude 999 field in this line of
> createMarcHash ?
> I don't know how the 999 is used by Koha...
> 
> +        if ((!defined($tagslib) || $tagslib->{$fieldtag}->{'tab'} >= 0) &&
> $fieldtag ne '995' && $fieldtag ne '999') {
> 
> 
> M. Saby

999 is where the biblioitemnumber and biblionumber are stored (at least in MARC21) 999$c and 999$d
Comment 13 Chris Cormack 2013-05-29 08:00:33 UTC
(In reply to comment #11)
> As 995 is excluded (for UNIMARC), I suppose 952 should have do (for Marc21) ?
> 
> M. Saby

995 was excluded originally (if you look at merge.pl). So we are still excluding it.
However for quite a while the item data has not been stored in the MARCXML, and the way we fetch the MARC biblio is by calling

my $record = GetMarcBiblio($biblionumber); 

So the item data is not added, if it was called with
my $record = GetMarcBiblio($biblionumber, 1); 
Then items would be added so 952.

So in our case, we have the MARC object, with no item data ie no 952 (in theory no 995 as well, but im not sure about that so left that exclusion in there).

So no need to exclude the 952. I hope this helps.
Comment 14 Mathieu Saby 2013-05-29 08:07:27 UTC
OK, I thought it was in 090a, like in UNIMARC.
So, if it useful NOT to merge 999 for Marc21, maybe it will be usefull NOT t to merge 090 too, for UNIMARC?

But 999 could also be used for other purpose I suppose, in Marc21 and in UNIMARC.
For example, in UNIMARC, Academic libraries can use this subfield freely, for their own use.
"Cette zone peut être utilisée librement pour des données ne correspondant à aucune zone du format d'échange standard, suivant les besoins de chaque bibliothèque."
http://documentation.abes.fr/sudoc/formats/loc/zones/999.htm#UtilisationActuelle

M. Saby
Comment 15 Chris Cormack 2013-05-29 08:09:57 UTC
(In reply to comment #14)
> OK, I thought it was in 090a, like in UNIMARC.
> So, if it useful NOT to merge 999 for Marc21, maybe it will be usefull NOT t
> to merge 090 too, for UNIMARC?
> 
> But 999 could also be used for other purpose I suppose, in Marc21 and in
> UNIMARC.
> For example, in UNIMARC, Academic libraries can use this subfield freely,
> for their own use.
> "Cette zone peut être utilisée librement pour des données ne correspondant à
> aucune zone du format d'échange standard, suivant les besoins de chaque
> bibliothèque."
> http://documentation.abes.fr/sudoc/formats/loc/zones/999.
> htm#UtilisationActuelle
> 
> M. Saby

It could but that would be dangerous of them, because 999 is used for Koha specific data, and merging them makes no sense, you cant have a record that has 2 biblionumbers that would break things badly.
Maybe you could send a follow up patch that doesn't exclude 999 if the flavour is UNIMARC.
This is still a major improvement though, and makes it much less likely for the merge to create an invalid record.
Comment 16 Mathieu Saby 2013-05-29 08:34:14 UTC
OK, so the solution could probably be 
- exclude merge of 999 for MARC21 / NORMARC
- exclude merge of 090 for UNIMARC

Am I right?

For the 995, as you said, it is no more in main marc record, so I suppose we could drop the exclusion of 995.

M. Saby
Comment 17 Chris Cormack 2013-05-29 08:41:49 UTC
(In reply to comment #16)
> OK, so the solution could probably be 
> - exclude merge of 999 for MARC21 / NORMARC
> - exclude merge of 090 for UNIMARC
> 
> Am I right?
> 
> For the 995, as you said, it is no more in main marc record, so I suppose we
> could drop the exclusion of 995.
> 
> M. Saby

Yep I'd wait for this improvement to be pushed, then open a new bug to do that follow up.
Comment 18 Galen Charlton 2013-05-29 15:05:18 UTC
Couple comments:

- Please rename the 'Koha::Record' class; as it is, the name is too generic.  After all, Koha has lots of record types, many of which are not MARC.  I suggest either 'Koha::MARC::Record' (following MARC::Record) or perhaps 'Koha::Metadata::MARC' (allowing for Koha::Metadata::MODS, etc.) or 'Koha::Util::MARC'.

- I think references to the 999 and 955 tags would be better handled by looking up the tag that the MARC framework uses to store biblio.biblionumber rather than using magic numbers.

Setting back to in discussion, as I do not intend to push this until (at least) the class has a better name.
Comment 19 Paul Poulain 2013-05-29 15:12:31 UTC
(In reply to comment #18)
> - I think references to the 999 and 955 tags would be better handled by
> looking up the tag that the MARC framework uses to store biblio.biblionumber
> rather than using magic numbers.
I fully agree with this !
Comment 20 Chris Cormack 2013-05-29 19:36:55 UTC
(In reply to comment #19)
> (In reply to comment #18)
> > - I think references to the 999 and 955 tags would be better handled by
> > looking up the tag that the MARC framework uses to store biblio.biblionumber
> > rather than using magic numbers.
> I fully agree with this !

I agree too, but this is not new in this patch, so I don't think that is a valid reason for stopping it. The Class name might be though.

We've already discussed a followup for different MARC flavours that could be extended to use the mappings instead. But I do think that this is a huge code improvement over what we had, and that a follow up to change to using mappings would be better outside of this refactoring patch anyway.
Comment 21 Jared Camins-Esakov 2013-05-30 10:13:37 UTC
(In reply to comment #18)
> Couple comments:
> 
> - Please rename the 'Koha::Record' class; as it is, the name is too generic.
> After all, Koha has lots of record types, many of which are not MARC.  I
> suggest either 'Koha::MARC::Record' (following MARC::Record) or perhaps
> 'Koha::Metadata::MARC' (allowing for Koha::Metadata::MODS, etc.) or
> 'Koha::Util::MARC'.

I've decided I'm attached to Koha::Record, because my reasoning behind using it was that it would be nice to be able to merge non-MARC records, so I've gone a bit further in a follow-up and made the class schema-sensitive, moving the MARC-specific code to Koha::Util::MARC.

> - I think references to the 999 and 955 tags would be better handled by
> looking up the tag that the MARC framework uses to store biblio.biblionumber
> rather than using magic numbers.

Fixed.

> Setting back to in discussion, as I do not intend to push this until (at
> least) the class has a better name.
Comment 22 Mathieu Saby 2013-05-30 10:26:43 UTC
Just to know, what kind of records in Koha are not MARC records? I don't understand.

M. Saby
Comment 23 Jared Camins-Esakov 2013-05-30 10:27:57 UTC Comment hidden (obsolete)
Comment 24 Jared Camins-Esakov 2013-05-30 10:31:29 UTC Comment hidden (obsolete)
Comment 25 Galen Charlton 2013-05-30 17:23:44 UTC
(In reply to comment #22)
> Just to know, what kind of records in Koha are not MARC records? I don't
> understand.

Patron records, loan records, vendor records, etc.  The word 'record' by itself is about as nonspecific as you get can get when referring to entities in a DBMS-backed application, IMO.
Comment 26 Mathieu Saby 2013-05-30 17:26:19 UTC
Of course! In french, record is translated by "notice" only for bib and auth record. For the other meanings, we use different words (fiche, enregistrement, etc), so I did not see it as so a wide concept...

M. Saby
Comment 27 Galen Charlton 2013-05-30 17:29:11 UTC
(In reply to comment #21)
> I've decided I'm attached to Koha::Record, because my reasoning behind using
> it was that it would be nice to be able to merge non-MARC records, so I've
> gone a bit further in a follow-up and made the class schema-sensitive,
> moving the MARC-specific code to Koha::Util::MARC.

For the reason expressed in my previous comment, 'Koha::Record' is still too non-specific for my taste.  Would 'Koha::MetadataRecord' be acceptable to you?
Comment 28 Jared Camins-Esakov 2013-05-30 23:46:56 UTC Comment hidden (obsolete)
Comment 29 Mathieu Saby 2013-06-12 21:31:41 UTC
Sorry Jared, the 3rd test went wrong


$prove t/db_dependent/Authority.t
Cannot detect source of 't/db_dependent/Authority.t'! at /usr/share/perl/5.14/TAP/Parser/IteratorFactory.pm line 263.
	TAP::Parser::IteratorFactory::detect_source('TAP::Parser::IteratorFactory=HASH(0xa40f730)', 'TAP::Parser::Source=HASH(0xa40d2d0)') called at /usr/share/perl/5.14/TAP/Parser/IteratorFactory.pm line 213
	TAP::Parser::IteratorFactory::make_iterator('TAP::Parser::IteratorFactory=HASH(0xa40f730)', 'TAP::Parser::Source=HASH(0xa40d2d0)') called at /usr/share/perl/5.14/TAP/Parser.pm line 469
	TAP::Parser::_initialize('TAP::Parser=HASH(0xa40d12c)', 'HASH(0xa1fea00)') called at /usr/share/perl/5.14/TAP/Object.pm line 58
	TAP::Object::new('TAP::Parser', 'HASH(0xa1fea00)') called at /usr/share/perl/5.14/TAP/Object.pm line 133
	TAP::Object::_construct('TAP::Harness=HASH(0xa326618)', 'TAP::Parser', 'HASH(0xa1fea00)') called at /usr/share/perl/5.14/TAP/Harness.pm line 779
	TAP::Harness::make_parser('TAP::Harness=HASH(0xa326618)', 'TAP::Parser::Scheduler::Job=HASH(0xa3277e8)') called at /usr/share/perl/5.14/TAP/Harness.pm line 578
	TAP::Harness::_aggregate_single('TAP::Harness=HASH(0xa326618)', 'TAP::Parser::Aggregator=HASH(0xa32d4b4)', 'TAP::Parser::Scheduler=HASH(0xa327680)') called at /usr/share/perl/5.14/TAP/Harness.pm line 670
	TAP::Harness::aggregate_tests('TAP::Harness=HASH(0xa326618)', 'TAP::Parser::Aggregator=HASH(0xa32d4b4)', 't/db_dependent/Authority.t') called at /usr/share/perl/5.14/TAP/Harness.pm line 485
	TAP::Harness::__ANON__() called at /usr/share/perl/5.14/TAP/Harness.pm line 498
	TAP::Harness::runtests('TAP::Harness=HASH(0xa326618)', 't/db_dependent/Authority.t') called at /usr/share/perl/5.14/App/Prove.pm line 553
	App::Prove::_runtests('App::Prove=HASH(0xa0b1db0)', 'HASH(0xa30d3fc)', 'TAP::Harness', 't/db_dependent/Authority.t') called at /usr/share/perl/5.14/App/Prove.pm line 511
	App::Prove::run('App::Prove=HASH(0xa0b1db0)') called at /usr/bin/prove line 11



M. Saby
Comment 30 Jared Camins-Esakov 2013-06-12 21:46:43 UTC Comment hidden (obsolete)
Comment 31 Mathieu Saby 2013-06-12 21:48:31 UTC
Maybe you meant Koha_Authority.t ?

This test is correct ;

$prove Koha_Authority.t 
Koha_Authority.t .. ok   
All tests successful.
Files=1, Tests=7,  1 wallclock secs ( 0.03 usr  0.01 sys +  0.26 cusr  0.04 csys =  0.34 CPU)
Result: PASS


I also tried to merge records, everything works well, and I don't have anymore the ability to put two 090 fieds in the destination record.

So, could you confirm the UT to test is Koha_Authority.t  ?

If it so, I can sign off.

Mathieu
Comment 32 Mathieu Saby 2013-06-12 21:49:12 UTC
Collision of comments ;-)
So, everything is OK, and I sign off the followup.

Mathieu
Comment 33 Mathieu Saby 2013-06-12 21:52:27 UTC Comment hidden (obsolete)
Comment 34 Katrin Fischer 2013-06-16 09:38:39 UTC
The QA script points out a problem with template validity that I think was introduced with these patches. Bit hard to make out the changes here, as large parts of the code have been deleted, rewritten and moved.

 * koha-tmpl/intranet-tmpl/prog/en/includes/merge-record.inc                FAIL
	forbidden patterns          OK
	tt_valid                    FAIL
		lines 10, 24
	valid_template              OK

Jared, please fix in a follow up and switch back to signed-off.
Comment 35 Jared Camins-Esakov 2013-06-16 11:56:05 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2013-07-08 06:17:46 UTC
I found a little something: For non-repeatable subfields, no warning is triggered. I tested with 245$a and was able to create a record with 2 245$a subfields without problems or warnings.
Warnings for control fields and non repeatable fields fields work nicely. Also if a field is not existant in the new record and you want to add one of the subfields you are warned.

I made sure I was using the default framework and 245$a was configured correctly.

This is also on master but should be fixed - either here as a follow-up or on a separate bug.
Comment 37 Katrin Fischer 2013-07-08 06:21:05 UTC
Created attachment 19464 [details] [review]
[PASSED QA] Bug 9755: Refactor record merge functionality

This patch refactors the merge record interface and code a little bit
in preparation for making it possible to merge authority records.

To test:
1) Apply patch.
2) Try merging two records:
    a) Create a list.
    b) Add two records you would like to (or be willing to) merge
       to said list.
    c) View said list.
    d) Check the checkboxes next to the two records you added.
    e) Click "Merge selected records."
    f) Choose a merge reference.
    g) Choose fields from each record that you want to keep.
    h) Click "Merge."
3) Confirm that your merged record has the fields and subfields you
   wanted.
4) Run the unit tests for the two files that were changed:
    prove t/Koha_Record.t t/db_dependent/Koha_Authority.t
5) Sign off.

Signed-off-by: Mathieu Saby <mathieu.saby@univ-rennes2.fr>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 38 Katrin Fischer 2013-07-08 06:21:17 UTC
Created attachment 19465 [details] [review]
[PASSED QA] Bug 9755 QA follow-up: move MARC-specific functionality to utility class

This follow-up moves all the MARC-specific functionality of Koha::Record
(now renamed to Koha::MetadataRecord) to a Koha::Util::MARC utility class.

To test, run relevant unit tests:
> prove t/Koha_MetadataRecord.t t/Koha_Util_MARC.t t/db_dependent/Koha_Authority.t
and optionally try to merge a record.

Signed-off-by: Mathieu Saby <mathieu.saby@univ-rennes2.fr>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 39 Katrin Fischer 2013-07-08 06:21:25 UTC
Created attachment 19466 [details] [review]
[PASSED QA] Bug 9755 QA follow-up: fix template compliance

Fix the following test failure:
* koha-tmpl/intranet-tmpl/prog/en/includes/merge-record.inc                FAIL
    forbidden patterns          OK
    tt_valid                    FAIL
            lines 10, 24
    valid_template              OK

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
Found a pre-existing problem with non-repeating subfields
that i noted on the bug report.
All other tests were ok and merging records worked nicely.
Comment 40 Galen Charlton 2013-07-23 23:12:29 UTC
Pushed to master, along with a follow-up to quell a warning that shows up now that the merge hash code is in a module that uses the warnings stricture.

Thanks, Jared!