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.
Created attachment 15913 [details] [review] 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.
Patch tested with a sandbox, by Mathieu Saby <mathieu.saby@univ-rennes2.fr>
Created attachment 15922 [details] [review] 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>
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
QAing this today
I try it today, there is a little conflict. M. Saby
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
Merge worked fine in the interface.
Created attachment 18446 [details] [review] 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>
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
As 995 is excluded (for UNIMARC), I suppose 952 should have do (for Marc21) ? M. Saby
(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
(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.
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
(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.
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
(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.
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.
(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 !
(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.
(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.
Just to know, what kind of records in Koha are not MARC records? I don't understand. M. Saby
Created attachment 18512 [details] [review] Bug 9755 QA follow-up: move MARC-specific functionality to utility class This follow-up moves all the MARC-specific functionality of Koha::Record to a Koha::Util::MARC utility class. To test, run relevant unit tests: > prove t/Koha_Record.t t/Koha_Util_MARC.t t/db_dependent/Authority.t and optionally try to merge a record.
Created attachment 18513 [details] [review] Bug 9755 QA follow-up: move MARC-specific functionality to utility class This follow-up moves all the MARC-specific functionality of Koha::Record to a Koha::Util::MARC utility class. To test, run relevant unit tests: > prove t/Koha_Record.t t/Koha_Util_MARC.t t/db_dependent/Authority.t and optionally try to merge a record. Re-uploading since my last upload missed some of the changes (code tidying in the new file only).
(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.
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
(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?
Created attachment 18531 [details] [review] 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/Authority.t and optionally try to merge a record.
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
Created attachment 18909 [details] [review] 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. This latest test corrects the test plan, which was wrong and mentioned Authority.t rather than the correct Koha_Authority.t.
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
Collision of comments ;-) So, everything is OK, and I sign off the followup. Mathieu
Created attachment 18910 [details] [review] Subject: [PATCH][SIGNED OFF] Bug 9755 QA follow-up: move MARC-specific functionality to utility class
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.
Created attachment 19038 [details] [review] 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
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.
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>
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>
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.
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!