Created attachment 152152 [details] [review] Bug 33955: Add a test
Created attachment 152153 [details] [review] Bug 33955: Koha::Biblio::normalized_upc
Created attachment 155542 [details] [review] Bug 33955: Add a test Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 155543 [details] [review] Bug 33955: Koha::Biblio::normalized_upc Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 155699 [details] [review] Bug 33955: Add a test Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 155700 [details] [review] Bug 33955: Koha::Biblio::normalized_upc Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I have doubts about the current approach to extracting things from MARC records. It feels really bad that we call $self->metadata->record each time, parsing the MARCXML (I don't claim we hit the DB, but it is probably the case most of the time). I'm setting this in discussion, and will provide an alternate patch for discussing the pattern entirely.
Created attachment 155819 [details] [review] [ALTERNATE] Bug 33955: Introduce Koha::MetadataExtractor and ->get_normalized_upc This patch introduces a new pattern for the different ->get_<thing> methods we've been adding. The aim is that code will look more like: my $metadata_extractor = Koha::MetadataExtractor->new; while ( my $biblio = $biblios->next ) { my $record = $biblio->record; my $schema = $biblio->record_schema; $data->{$biblio->id}->{normalized_upc} = $metadata_extractor->get_normalized_upc( { record => $record, schema => $schema } ); $data->{$biblio->id}->{normalized_ean} = $metadata_extractor->get_normalized_ean( { record => $record, schema => $schema } ); } The key is that we are actually reusing the MARC::Record, and code for each schema is organized cleanly so easier to maintain.
I like where Tomas is going with this, but I wonder if we could make this simpler by storing this info in the DB. We have need for a UPC field in any case, why not have a normalized_upc (and isbn, issn....) If we don't quiote go that far, we could still normalize from the parsed DB value, rather than the record
(In reply to Nick Clemens from comment #9) > We have need for a UPC field in any case, why not have a normalized_upc (and > isbn, issn....) I like that...
(In reply to Tomás Cohen Arazi from comment #10) > (In reply to Nick Clemens from comment #9) > > We have need for a UPC field in any case, why not have a normalized_upc (and > > isbn, issn....) > > I like that... Do we need a Koha to MARC mapping?
This was really a simple patch, as simple as possible to reuse the existing C4 sub, instead of duplicating the code. I think it's a good idea to have something more robust, but this is not the place for that. We should implement it either after the different "Add Koha::Biblio->normalized_*" bugs, or before.
(In reply to Jonathan Druart from comment #12) > This was really a simple patch, as simple as possible to reuse the existing > C4 sub, instead of duplicating the code. I can see that.
Comment on attachment 155819 [details] [review] [ALTERNATE] Bug 33955: Introduce Koha::MetadataExtractor and ->get_normalized_upc Moving the Koha::MetadataExtractor work to its own bug to let this move forward.
Pushed to master for 23.11. Nice work everyone, thanks!
Enhancement not pushed to 23.05.x