Easy one to move.
Created attachment 169058 [details] [review] Bug 37380: Unit tests
Created attachment 169059 [details] [review] Bug 37380: Add get_control_number() to the metadata extractor This simple patch implements C4::Biblio::GetMarcControlnumber in the right spot on the Koha::* namespace. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 169060 [details] [review] Bug 37380: Replace uses of GetMarcControlnumber for get_control_number()
Created attachment 169777 [details] [review] Bug 37380: Unit tests Signed-off-by: Sam Lau <samalau@gmail.com>
Created attachment 169778 [details] [review] Bug 37380: Add get_control_number() to the metadata extractor This simple patch implements C4::Biblio::GetMarcControlnumber in the right spot on the Koha::* namespace. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Sam Lau <samalau@gmail.com>
Created attachment 169779 [details] [review] Bug 37380: Replace uses of GetMarcControlnumber for get_control_number() Signed-off-by: Sam Lau <samalau@gmail.com>
+ my $control_num = $biblio->get_control_number(); This is not correct.
+ is( $extractor->get_control_number, undef, 'Returns undef if no 001' ); - is( $controlnumber, '', 'GetMarcControlnumber handles records without 001' ); Change in behaviour here, did you investigate it?
(In reply to Jonathan Druart from comment #7) > + my $control_num = $biblio->get_control_number(); > > This is not correct. You're right! (In reply to Jonathan Druart from comment #8) > + is( $extractor->get_control_number, undef, 'Returns undef if no > 001' ); > > - is( $controlnumber, '', 'GetMarcControlnumber handles records without > 001' ); > > Change in behaviour here, did you investigate it? I did! But I will re-do just in case!
Created attachment 171099 [details] [review] Bug 37380: Unit tests Signed-off-by: Sam Lau <samalau@gmail.com>
Created attachment 171100 [details] [review] Bug 37380: Add get_control_number() to the metadata extractor This simple patch implements C4::Biblio::GetMarcControlnumber in the right spot on the Koha::* namespace. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Sam Lau <samalau@gmail.com>
Created attachment 171101 [details] [review] Bug 37380: Replace uses of GetMarcControlnumber for get_control_number() Signed-off-by: Sam Lau <samalau@gmail.com>
Hi, I rolled back with making it return undef. As I noticed the tradeoff was not good. We ended up having to add `// q{}` in a bunch of places, which is not worth. Tests and code have been adjusting to go back to the old behavior. I wanted to share that I made some exhaustive tests on MARC::Record and MARC::Field behavior... ``` perl -MDDP \ -MMARC::File::XML \ -MMARC::Record \ -MMARC::Field \ -e 'my $f = MARC::Field->new( "001", undef); p($f->data); my $r = MARC::Record->new(); $r->append_fields($f); $r->append_fields(MARC::Field->new("245", "", "", a => "Stuff")); my $xml = $r->as_xml; $r = MARC::Record->new_from_xml($xml); p($r->as_xml);' ``` My conclusion is that MARC::Field allows adding `undef` values but it will then warn when appending them. But I also found out that once the record gets serialized as XML, the `undef` is replaced by an empty string and that's how it is going to stay forever, with our code. So having `undef` is a weird artificially created scenario.
Created attachment 171189 [details] [review] Bug 37380: Unit tests Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 171190 [details] [review] Bug 37380: Add get_control_number() to the metadata extractor This simple patch implements C4::Biblio::GetMarcControlnumber in the right spot on the Koha::* namespace. To test: 1. Apply this patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 171191 [details] [review] Bug 37380: Replace uses of GetMarcControlnumber for get_control_number() Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Looking at catalogue/export.pl: my $marc = $biblio->metadata->record({ embed_items => 1 }); my $metadata_extractor = $biblio->metadata_extractor; We are going to fetch metadata twice. We should certainly improve it at some point. Not considering blocker for this change however.
I originally introduced this method in bug 5430 which which seems like ages ago now. But I won't miss it :)
Pushed for 24.11! Well done everyone, thank you!
Skipping for release notes as this is quite technical and kind of self-explanatory to developers.
This is more developer focused, and is not covered in the manual - so no update to the manual required.