Created attachment 87928 [details] [review] Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio This patch makes $biblio->metadata be prefetchable as the Coding guidelines require. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Koha/Biblio.t => SUCCESS: No regressions! - Sign off :-D
Created attachment 87931 [details] [review] Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio This patch makes $biblio->metadata be prefetchable as the Coding guidelines require. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Koha/Biblio.t => SUCCESS: No regressions! - Sign off :-D
Created attachment 87943 [details] [review] Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio This patch makes $biblio->metadata be prefetchable as the Coding guidelines require. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Koha/Biblio.t => SUCCESS: No regressions! - Sign off :-D Signed-off-by: Michal Denar <black23@gmail.com>
Comment on attachment 87943 [details] [review] Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio Review of attachment 87943 [details] [review]: ----------------------------------------------------------------- This doesn't feel entirely correct yet I'm afraid.. though I'm not close enough to the biblio_metadatas table to entirely understand whether we expect it to every be a more than one-to-one relationship with the biblio's table. ::: Koha/Biblio.pm @@ +74,4 @@ > sub metadata { > my ( $self ) = @_; > > + my $metadata_rs = $self->_result->biblio_metadatas->single; Surprised this doesn't throw a warning? https://metacpan.org/pod/DBIx::Class::ResultSet#single implies that calling 'single' on a 'has_many' (when prefetched) should throw one. Also, as discussed in bug 22407 it would be nice to rename the underlying dbic relationship to match the Koha::Object relation accessor (we could also code said relationship to explicitly return a single related row rather than the possibility of a ResultSet)
(In reply to Martin Renvoize from comment #4) > Comment on attachment 87943 [details] [review] [review] > Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio > > Review of attachment 87943 [details] [review] [review]: > ----------------------------------------------------------------- > > This doesn't feel entirely correct yet I'm afraid.. though I'm not close > enough to the biblio_metadatas table to entirely understand whether we > expect it to every be a more than one-to-one relationship with the biblio's > table. It is a one-to-one relationship. > ::: Koha/Biblio.pm > @@ +74,4 @@ > > sub metadata { > > my ( $self ) = @_; > > > > + my $metadata_rs = $self->_result->biblio_metadatas->single; > > Surprised this doesn't throw a warning? > > > https://metacpan.org/pod/DBIx::Class::ResultSet#single implies that calling > 'single' on a 'has_many' (when prefetched) should throw one. For a warning it requires more than one result. Something the schema allows, but the code doesn't. > Also, as discussed in bug 22407 it would be nice to rename the underlying > dbic relationship to match the Koha::Object relation accessor (we could also > code said relationship to explicitly return a single related row rather than > the possibility of a ResultSet) I agree with this, but I wouldn't consider this a blocker for this one. I will give it a try, though.
Created attachment 88355 [details] [review] Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio This patch makes $biblio->metadata be prefetchable as the Coding guidelines require. A new has_one relationship is added in the Biblio schema, named as the accessor to also convey to proposed guidelines for readability. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Koha/Biblio.t => SUCCESS: No regressions! - Sign off :-D Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 88492 [details] [review] Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio This patch makes $biblio->metadata be prefetchable as the Coding guidelines require. A new has_one relationship is added in the Biblio schema, named as the accessor to also convey to proposed guidelines for readability. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Koha/Biblio.t => SUCCESS: No regressions! - Sign off :-D Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I like this version :). Passing QA.
Awesome work all! Pushed to master for 19.05
(In reply to Tomás Cohen Arazi from comment #5) > (In reply to Martin Renvoize from comment #4) > > Comment on attachment 87943 [details] [review] [review] [review] > > Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio > > > > Review of attachment 87943 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > This doesn't feel entirely correct yet I'm afraid.. though I'm not close > > enough to the biblio_metadatas table to entirely understand whether we > > expect it to every be a more than one-to-one relationship with the biblio's > > table. > > It is a one-to-one relationship. Hum, really? If it was, we would have not create a new table for that. We could have different representation of the record (MARC, MARCXML, etc.), for different MARC profiles. IMO this is completely wrong, but I may have missed a previous discussion.
Also note that Koha::Biblio->metadata was wrong prior to this patch...
(In reply to Jonathan Druart from comment #10) > (In reply to Tomás Cohen Arazi from comment #5) > > (In reply to Martin Renvoize from comment #4) > > > Comment on attachment 87943 [details] [review] [review] [review] [review] > > > Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio > > > > > > Review of attachment 87943 [details] [review] [review] [review] [review]: > > > ----------------------------------------------------------------- > > > > > > This doesn't feel entirely correct yet I'm afraid.. though I'm not close > > > enough to the biblio_metadatas table to entirely understand whether we > > > expect it to every be a more than one-to-one relationship with the biblio's > > > table. > > > > It is a one-to-one relationship. > > Hum, really? > If it was, we would have not create a new table for that. > We could have different representation of the record (MARC, MARCXML, etc.), > for different MARC profiles. > IMO this is completely wrong, but I may have missed a previous discussion. It is 1:1 right now - splitting it off biblioitems was for performance reasons, not database design IIRC: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17196
(In reply to Jonathan Druart from comment #10) > (In reply to Tomás Cohen Arazi from comment #5) > > (In reply to Martin Renvoize from comment #4) > > > Comment on attachment 87943 [details] [review] [review] [review] [review] > > > Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio > > > > > > Review of attachment 87943 [details] [review] [review] [review] [review]: > > > ----------------------------------------------------------------- > > > > > > This doesn't feel entirely correct yet I'm afraid.. though I'm not close > > > enough to the biblio_metadatas table to entirely understand whether we > > > expect it to every be a more than one-to-one relationship with the biblio's > > > table. > > > > It is a one-to-one relationship. > > Hum, really? > If it was, we would have not create a new table for that. > We could have different representation of the record (MARC, MARCXML, etc.), > for different MARC profiles. > IMO this is completely wrong, but I may have missed a previous discussion. What would you say is the source of truth if there were more than one representation? I'm not against some vaccine of different representativa, just saying many places assume THE metadata is the source for populating other columns and our design doesn't consider a way to determine which representation is the one to be used as basis. As far as I can tell, many times it's been said that the table was splitted to save space in join operations where the metadata wasn't really needed.
I think Tomas has a good point - having multiple representations of the same record could get messy fast - we haven't worked out a concept for that yet. What we had discussed I think was storing data in other formats than MARC - so still 1:1. We do not have code for that yet I think (display, indexing, mapping...).
(In reply to Katrin Fischer from comment #14) > What we had discussed I think was storing data in other formats than MARC - > so still 1:1 No, it would become 1:n then. What about renaming the method in Koha::S::R::Biblio to ->marc_xml_metadata (adding the conditions in it) and Koha::Biblio->metadata to ->marc_xml (make it returns the MARX XML directly)?
(In reply to Jonathan Druart from comment #15) > (In reply to Katrin Fischer from comment #14) > > What we had discussed I think was storing data in other formats than MARC - > > so still 1:1 > > No, it would become 1:n then. I meant still only 1 metadata entry - either MARC or Dublincore or... not having both or multiple formats. It would allow to load data without converting to MARC.
I do sort of agree that 'metadata' feels like a bit a generic term if in reality it's always marc.. As an aside, do we have any inflators setup with our dbic schemata?
(In reply to Martin Renvoize from comment #17) > As an aside, do we have any inflators setup with our dbic schemata? Like: Koha/ItemTypes.pm: $Koha::Schema::Result::Itemtype::LANGUAGE = $language; ?
Enhancement will not be backported to 18.11.x series.
(In reply to Jonathan Druart from comment #18) > (In reply to Martin Renvoize from comment #17) > > As an aside, do we have any inflators setup with our dbic schemata? > > Like: > Koha/ItemTypes.pm: $Koha::Schema::Result::Itemtype::LANGUAGE = > $language; > ? No, I meant like DBIC::InflateColumn::DateTime.. or in this case we could easily have DBIC::::InflateColumn::MARC and similar
(In reply to Jonathan Druart from comment #15) > What about renaming the method in Koha::S::R::Biblio to ->marc_xml_metadata > (adding the conditions in it) and Koha::Biblio->metadata to ->marc_xml (make > it returns the MARX XML directly)? Should we go with that for now?
I think that makes sense to me.. but in a new bug pretty please so we don't taint the release notes (and so I don't completely lose the change by forgetting about this bug as it's RESOLVED FIXED already ;)
(In reply to Martin Renvoize from comment #22) > I think that makes sense to me.. but in a new bug pretty please so we don't > taint the release notes (and so I don't completely lose the change by > forgetting about this bug as it's RESOLVED FIXED already ;) I have opened bug 23235.