[Bug 22700] New: Make biblio_metadata prefetchable from Koha::Biblio
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Bug ID: 22700 Summary: Make biblio_metadata prefetchable from Koha::Biblio Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org Target Milestone: --- -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |22407 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22407 [Bug 22407] OMNIBUS: Use DBIC relations to fetch related object rather than searching for the object -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 87928 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=87928&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |22696 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22696 [Bug 22696] Simplify visibility logic in opac-ISBDdetail.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #87928|0 |1 is obsolete| | --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 87931 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=87931&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org, | |kyle@bywatersolutions.com, | |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Michal Denar <black23@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Michal Denar <black23@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #87931|0 |1 is obsolete| | --- Comment #3 from Michal Denar <black23@gmail.com> --- Created attachment 87943 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=87943&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 87943 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=87943 Bug 22700: Make biblio_metadata prefetchable from Koha::Biblio Review of attachment 87943: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=22700&attachment=87943) ----------------------------------------------------------------- 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Martin Renvoize from comment #4)
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.
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #87943|0 |1 is obsolete| | --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 88355 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=88355&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #88355|0 |1 is obsolete| | --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 88492 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=88492&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I like this version :). Passing QA. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to master for 19.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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] 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.
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Also note that Koha::Biblio->metadata was wrong prior to this patch... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #12 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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] 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.
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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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] 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.
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #14 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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...). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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)? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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; ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to Master |RESOLVED Version(s)| |19.05.00 released in| | --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Enhancement will not be backported to 18.11.x series. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23235 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23235 [Bug 23235] Rename Koha::Biblio->metadata with ->marc_xml to avoid ambiguity -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22700 --- Comment #23 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org