[Bug 31224] New: Koha::Biblio::Metadata->record should use the EmbedItems filter
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Bug ID: 31224 Summary: Koha::Biblio::Metadata->record should use the EmbedItems filter Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: normal 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 Bug 29697 removed GetMarcBiblio in favor of using Koha::Biblio::Metadata->record. YAY! But it added a redundant Koha::Biblio::Metadata::_embed_items method. It also adds a suspicious 'record' param. I say suspicious because it looks like a trick to have a given MARC::Record object added the items information. Which feels like wrong API design. Specially when we have the Koha::Filter::MARC::EmbedItems processor filter for the task. Now that things are 'modernized' we really need to revisit this code for a cleanup. -- 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=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |29697 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29697 [Bug 29697] Replace GetMarcBiblio occurrences with $biblio->metadata->record -- 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=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23247 -- 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=31224 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Ok, I submitted to bug 23247 and implementation that doesn't rely on the new embed_items param, so we ponder how coding with RecordProcessor looks like in the controllers. I will now submit a version that uses RecordProcessor in the method. -- 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=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com, | |m.de.rooy@rijksmuseum.nl, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com Status|NEW |In Discussion -- 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=31224 --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 138060 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138060&action=edit [POC] Bug 31224: Add Koha::Biblio->metadata_record This patch introduces a higher-level method for retrieving a 'prepared' record for using in specific contexts. In particular, I only focused on embedding item information and OPAC view filtering. But we could add a way to get the record through the ViewPolicy also for staff. The virtue of this patch is that it explains better than my words why I think having the *embed_items* and *opac* parameters in Koha::Biblio::Metadata->record is not ideal. And makes the other implementation feel like is done at the wrong level. I know it's been done like that to act as a drop-in replacement fro GetMarcBiblio, which was a good first step. But we need to revisit how it is used, in general, to come up with a more refined and useful design. So we do not port the same design problems from C4::* into Koha::*. Try to think how it would be used in contexts like opac-MARCdetail.pl or any other OPAC controller script. Thanks! -- 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=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #138060|0 |1 is obsolete| | --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 138061 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138061&action=edit [POC] Bug 31224: Add Koha::Biblio->metadata_record This patch introduces a higher-level method for retrieving a 'prepared' record for using in specific contexts. In particular, I only focused on embedding item information and OPAC view filtering. But we could add a way to get the record through the ViewPolicy also for staff. The virtue of this patch is that it explains better than my words why I think having the *embed_items* and *opac* parameters in Koha::Biblio::Metadata->record is not ideal. And makes the other implementation feel like is done at the wrong level. I know it's been done like that to act as a drop-in replacement fro GetMarcBiblio, which was a good first step. But we need to revisit how it is used, in general, to come up with a more refined and useful design. So we do not port the same design problems from C4::* into Koha::*. Try to think how it would be used in contexts like opac-MARCdetail.pl or any other OPAC controller script. Thanks! -- 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=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@helsinki.fi --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Resubmitted as an unrelated change slipped in. -- 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=31224 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 138062 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138062&action=edit [POC] Bug 31224: Use metadata_record in opac-MARCdetail.pl -- 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=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #138061|0 |1 is obsolete| | -- 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=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #138062|0 |1 is obsolete| | -- 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=31224 --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 138063 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138063&action=edit [POC] Bug 31224: Add Koha::Biblio->metadata_record This patch introduces a higher-level method for retrieving a 'prepared' record for using in specific contexts. In particular, I only focused on embedding item information and OPAC view filtering. But we could add a way to get the record through the ViewPolicy also for staff. The virtue of this patch is that it explains better than my words why I think having the *embed_items* and *opac* parameters in Koha::Biblio::Metadata->record is not ideal. And makes the other implementation feel like is done at the wrong level. I know it's been done like that to act as a drop-in replacement fro GetMarcBiblio, which was a good first step. But we need to revisit how it is used, in general, to come up with a more refined and useful design. So we do not port the same design problems from C4::* into Koha::*. Try to think how it would be used in contexts like opac-MARCdetail.pl or any other OPAC controller script. Thanks! -- 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=31224 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 138064 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=138064&action=edit [POC] Bug 31224: Use metadata_record in opac-MARCdetail.pl -- 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=31224 --- Comment #8 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Ok, I added a sample usage. -- 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=31224 --- Comment #9 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Then, what's next? Please make sure to work on top of bug 27272 please. -- 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=31224 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #9)
Then, what's next? Please make sure to work on top of bug 27272 please.
If you all agree with this proposed implementation, once we fix breakages caused by bug 29697, and bug 27272 pushed, I will implement this. -- 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=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |ASSIGNED Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- 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=31224 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I lost track of this one, sorry.. I agree with the approach here, would you like me to have a look at the other cases Tomas? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Bug 31224 depends on bug 29697, which changed state. Bug 29697 Summary: Replace GetMarcBiblio occurrences with $biblio->metadata->record https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29697 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Martin Renvoize from comment #11)
I lost track of this one, sorry.. I agree with the approach here, would you like me to have a look at the other cases Tomas?
Please do. I can too, after the release. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 142137 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142137&action=edit Bug 31224: Add Koha::Biblio->metadata_record This patch introduces a higher-level method for retrieving a 'prepared' record for using in specific contexts. In particular, I only focused on embedding item information and OPAC view filtering. But we could add a way to get the record through the ViewPolicy also for staff. The virtue of this patch is that it explains better than my words why I think having the *embed_items* and *opac* parameters in Koha::Biblio::Metadata->record is not ideal. And makes the other implementation feel like is done at the wrong level. I know it's been done like that to act as a drop-in replacement fro GetMarcBiblio, which was a good first step. But we need to revisit how it is used, in general, to come up with a more refined and useful design. So we do not port the same design problems from C4::* into Koha::*. Try to think how it would be used in contexts like opac-MARCdetail.pl or any other OPAC controller script. Thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 142138 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142138&action=edit Bug 31224: Use metadata_record in opac-MARCdetail.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 142139 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142139&action=edit Bug 31224: (QA follow-up) Fix syntax error -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 142140 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142140&action=edit Bug 31224: Convert C4::Record::marcrecord2csv Update marcrecord2csv to user RecordProcessor directly for embedding listed items. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 142141 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142141&action=edit Bug 31224: Support 'interface' on metadata_record We replace the 'opac' boolean with an 'interface' param here to allow for ViewPolicy filtering depending on 'opac', 'intranet' or 'nothing' being passed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 142142 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142142&action=edit Bug 31224: Update instances of metadata->record We can now call metadata_record directly on the Koha::Biblio object. This aptch updates all modules and controllers to use Koha::Biblio->metadata_record directly where appropriate. The exceptions are where we don't require any filtering or the filtering makes sense to do after the initial fetch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #138063|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #138064|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- OK.. So I've worked through a grep of `metadata->record(` to catch all cases where we were applying filters to the record (well.. all cases that did it directly on fetch and not as a second step) for controllers and modules only at this point.. I will follow-up and do tests next. After working through I decided to update the new method to accept an 'interface' parameter instead of using 'opac' as a boolean.. this allows for 'opac', 'intranet' and 'undefined' as options giving us a little more control for the ViewPolicy filter use case. I opted not to port the optional passing of a list of itemnumbers to embed as there's only one use case of it.. so instead I added the RecordProcesser to the controller in that case. Let me know what you think.. I'm also still somewhat open on the method name.. 'metadata_record' vs 'filtered_record' or 'record_filtered' or something along those lines? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Also.. should I work through and try to replace ALL calls to ->metadata->record with ->metadata_record.. and check for superfluous RecordProcessor instantiations now we can do it inline? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|tomascohen@gmail.com |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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142142|0 |1 is obsolete| | --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 142143 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142143&action=edit Bug 31224: Update instances of metadata->record We can now call metadata_record directly on the Koha::Biblio object. This aptch updates all modules and controllers to use Koha::Biblio->metadata_record directly where appropriate. The exceptions are where we don't require any filtering or the filtering makes sense to do after the initial fetch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Please fix: root@master:/usr/share/koha# git grep EmbedItemsInMarcBiblio C4/Biblio.pm: EmbedItemsInMarcBiblio Exported not existing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=31889 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #23 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Marcel de Rooy from comment #22)
Please fix:
root@master:/usr/share/koha# git grep EmbedItemsInMarcBiblio C4/Biblio.pm: EmbedItemsInMarcBiblio
Exported not existing
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|martin.renvoize@ptfs-europe |tomascohen@gmail.com |.com | Status|Failed QA |ASSIGNED --- Comment #24 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Marcel de Rooy from comment #22)
Please fix:
root@master:/usr/share/koha# git grep EmbedItemsInMarcBiblio C4/Biblio.pm: EmbedItemsInMarcBiblio
Exported not existing
I don't understand this comment or why this is FQA (because of it?). This anyway doesn't apply anymore, and the last patch needs to be redone entirely. Will poke around it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Such a shame things like this just die instead of getting support. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #26 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Martin Renvoize from comment #20)
Also.. should I work through and try to replace ALL calls to ->metadata->record with ->metadata_record.. and check for superfluous RecordProcessor instantiations now we can do it inline?
I'm sorry. Yes to all your suggestions :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |33083 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33083 [Bug 33083] Handle generic collection of records methods -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #27 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Master moved a chunk.. this needs rebasing on at least bug 33083 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |23247 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23247 [Bug 23247] Use EmbedItems in opac-MARCdetail.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |3150 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=3150 [Bug 3150] Move emails for sending cart and list contents into notices tool -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142137|0 |1 is obsolete| | --- Comment #28 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161933 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161933&action=edit Bug 31224: Add Koha::Biblio->metadata_record This patch introduces a higher-level method for retrieving a 'prepared' record for using in specific contexts. In particular, I only focused on embedding item information and OPAC view filtering. But we could add a way to get the record through the ViewPolicy also for staff. The virtue of this patch is that it explains better than my words why I think having the *embed_items* and *opac* parameters in Koha::Biblio::Metadata->record is not ideal. And makes the other implementation feel like is done at the wrong level. I know it's been done like that to act as a drop-in replacement fro GetMarcBiblio, which was a good first step. But we need to revisit how it is used, in general, to come up with a more refined and useful design. So we do not port the same design problems from C4::* into Koha::*. Try to think how it would be used in contexts like opac-MARCdetail.pl or any other OPAC controller script. Thanks! 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142138|0 |1 is obsolete| | --- Comment #29 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161934 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161934&action=edit Bug 31224: Use metadata_record in opac-MARCdetail.pl 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142139|0 |1 is obsolete| | --- Comment #30 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161935 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161935&action=edit Bug 31224: (QA follow-up) Fix syntax error 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142140|0 |1 is obsolete| | --- Comment #31 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161936 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161936&action=edit Bug 31224: Convert C4::Record::marcrecord2csv Update marcrecord2csv to user RecordProcessor directly for embedding listed items. 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142141|0 |1 is obsolete| | --- Comment #32 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161937 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161937&action=edit Bug 31224: Support 'interface' on metadata_record We replace the 'opac' boolean with an 'interface' param here to allow for ViewPolicy filtering depending on 'opac', 'intranet' or 'nothing' being passed. 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142143|0 |1 is obsolete| | --- Comment #33 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161938 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161938&action=edit Bug 31224: Update instances of metadata->record We can now call metadata_record directly on the Koha::Biblio object. This aptch updates all modules and controllers to use Koha::Biblio->metadata_record directly where appropriate. The exceptions are where we don't require any filtering or the filtering makes sense to do after the initial fetch. 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize <martin.renvoize@ptfs-europe.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=31224 --- Comment #34 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Rebased -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161933|0 |1 is obsolete| | --- Comment #35 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161939 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161939&action=edit Bug 31224: Add Koha::Biblio->metadata_record This patch introduces a higher-level method for retrieving a 'prepared' record for using in specific contexts. In particular, I only focused on embedding item information and OPAC view filtering. But we could add a way to get the record through the ViewPolicy also for staff. The virtue of this patch is that it explains better than my words why I think having the *embed_items* and *opac* parameters in Koha::Biblio::Metadata->record is not ideal. And makes the other implementation feel like is done at the wrong level. I know it's been done like that to act as a drop-in replacement fro GetMarcBiblio, which was a good first step. But we need to revisit how it is used, in general, to come up with a more refined and useful design. So we do not port the same design problems from C4::* into Koha::*. Try to think how it would be used in contexts like opac-MARCdetail.pl or any other OPAC controller script. Thanks! 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161934|0 |1 is obsolete| | --- Comment #36 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161940 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161940&action=edit Bug 31224: Use metadata_record in opac-MARCdetail.pl 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161935|0 |1 is obsolete| | --- Comment #37 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161941 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161941&action=edit Bug 31224: (QA follow-up) Fix syntax error 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161936|0 |1 is obsolete| | --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161942 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161942&action=edit Bug 31224: Convert C4::Record::marcrecord2csv Update marcrecord2csv to user RecordProcessor directly for embedding listed items. 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161937|0 |1 is obsolete| | --- Comment #39 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161943 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161943&action=edit Bug 31224: Support 'interface' on metadata_record We replace the 'opac' boolean with an 'interface' param here to allow for ViewPolicy filtering depending on 'opac', 'intranet' or 'nothing' being passed. 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=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161938|0 |1 is obsolete| | --- Comment #40 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 161944 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161944&action=edit Bug 31224: Update instances of metadata->record We can now call metadata_record directly on the Koha::Biblio object. This aptch updates all modules and controllers to use Koha::Biblio->metadata_record directly where appropriate. The exceptions are where we don't require any filtering or the filtering makes sense to do after the initial fetch. 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=31224 --- Comment #41 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Right.. we're all rebased and QA scripts are passing.. However, not all tests are.. we need to account for the bad xml chars stuff.. whilst we wrap things in evals as part of the rebase.. the errors thrown aren't propagating properly yet so we're failing tests. I also think we should merge Koha::Biblio->record and Koha::Biblio->metadata_record rather than having both. Passing the ball to the next dev however.. it's time for me to clock off for the evening. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=35104 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrewfh@dubcolib.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |ASSIGNED --- Comment #42 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Martin Renvoize from comment #41)
Right.. we're all rebased and QA scripts are passing..
However, not all tests are.. we need to account for the bad xml chars stuff.. whilst we wrap things in evals as part of the rebase.. the errors thrown aren't propagating properly yet so we're failing tests.
I also think we should merge Koha::Biblio->record and Koha::Biblio->metadata_record rather than having both.
Passing the ball to the next dev however.. it's time for me to clock off for the evening.
Thanks Martin for resurrecting this. It is a relief to see there's still hope on stuff. I'll finish this once I'm back from this short time off. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161939|0 |1 is obsolete| | Attachment #161940|0 |1 is obsolete| | Attachment #161941|0 |1 is obsolete| | Attachment #161942|0 |1 is obsolete| | Attachment #161943|0 |1 is obsolete| | Attachment #161944|0 |1 is obsolete| | --- Comment #43 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173530 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173530&action=edit Bug 31224: Add Koha::Biblio->metadata_record This patch introduces a higher-level method for retrieving a 'prepared' record for using in specific contexts. In particular, I only focused on embedding item information and OPAC view filtering. But we could add a way to get the record through the ViewPolicy also for staff. The virtue of this patch is that it explains better than my words why I think having the *embed_items* and *opac* parameters in Koha::Biblio::Metadata->record is not ideal. And makes the other implementation feel like is done at the wrong level. I know it's been done like that to act as a drop-in replacement fro GetMarcBiblio, which was a good first step. But we need to revisit how it is used, in general, to come up with a more refined and useful design. So we do not port the same design problems from C4::* into Koha::*. Try to think how it would be used in contexts like opac-MARCdetail.pl or any other OPAC controller script. Thanks! 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=31224 --- Comment #44 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173531 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173531&action=edit Bug 31224: Use metadata_record in opac-MARCdetail.pl 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=31224 --- Comment #45 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173532 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173532&action=edit Bug 31224: Convert C4::Record::marcrecord2csv Update marcrecord2csv to user RecordProcessor directly for embedding listed items. 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=31224 --- Comment #46 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173533 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173533&action=edit Bug 31224: Support 'interface' on metadata_record We replace the 'opac' boolean with an 'interface' param here to allow for ViewPolicy filtering depending on 'opac', 'intranet' or 'nothing' being passed. 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=31224 --- Comment #47 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173534 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173534&action=edit Bug 31224: Update instances of metadata->record We can now call metadata_record directly on the Koha::Biblio object. This aptch updates all modules and controllers to use Koha::Biblio->metadata_record directly where appropriate. The exceptions are where we don't require any filtering or the filtering makes sense to do after the initial fetch. 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=31224 --- Comment #48 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173535 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173535&action=edit Bug 31224: EmbedItems takes an arrayref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #49 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Ok. Rebased, and found a non-minor glitch with `EmbedItems`. I think this is mostly solid, I didn't find broken tests (yet). Tested with the advanced editor (which uses /svc/[new_]bib) I think the patch for `marcrecord2csv` is not correct as we don't have an 'itemnumbers' param being used. I haven't found any use of that method in the code, though. On a follow-up dev, I would make the `EmbedItems` filter just get a `Koha::Items` resultset. I think this patchset is fixing a couple leaks too. If this is not gonna move, I might fix them on a separate bug. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #50 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #49)
Tested with the advanced editor (which uses /svc/[new_]bib)
I meant that adding/changing a biblio worked correctly. Both ES and Zebra indexing worked as expected. And all views work as well. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #51 from Jonathan Druart <jonathan.druart@gmail.com> --- Shouldn't we filter by default? 166 if ( $params->{embed_items} ) { 167 push @filters, 'EmbedItems'; 168 if ( $params->{interface} && $params->{interface} eq 'opac' ) { 169 $options->{items} = $self->items->filter_by_visible_in_opac( If we don't pass 'opac' we do not filter. If we forget it then it's not filtered either. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #52 from Jonathan Druart <jonathan.druart@gmail.com> --- Shouldn't Koha::OAI::Server::Repository->get_biblio_marcxml call this with expand_coded_fields? Same for Koha::REST::V1::Biblios->get_public, use metadata_record and remove 'ViewPolicy'? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #53 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #51)
Shouldn't we filter by default?
166 if ( $params->{embed_items} ) { 167 push @filters, 'EmbedItems'; 168 if ( $params->{interface} && $params->{interface} eq 'opac' ) { 169 $options->{items} = $self->items->filter_by_visible_in_opac(
If we don't pass 'opac' we do not filter. If we forget it then it's not filtered either.
The original implementation falled back to 'opac'. If we want to make sure, we could make the `interface` parameter mandatory and throw an exception if missing. I'd agree with a follow-up implementing it. (In reply to Jonathan Druart from comment #52)
Shouldn't Koha::OAI::Server::Repository->get_biblio_marcxml call this with expand_coded_fields?
Yes.
Same for Koha::REST::V1::Biblios->get_public, use metadata_record and remove 'ViewPolicy'?
Yes. --- Overall, I implemented `metadata_record` and provided a sample usage patch. Martin's attempt to use it everywhere might have missed some places. But some might better be handled on their own bugs. As we tried to keep the scope small (i.e. replace `$biblio->metadata->record(*)` with `$biblio->metadata_record(*)`. I agree with all your remarks, Jonathan. And can work on that if you don't beat me to it. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #54 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #51)
Shouldn't we filter by default?
166 if ( $params->{embed_items} ) { 167 push @filters, 'EmbedItems'; 168 if ( $params->{interface} && $params->{interface} eq 'opac' ) { 169 $options->{items} = $self->items->filter_by_visible_in_opac(
If we don't pass 'opac' we do not filter. If we forget it then it's not filtered either.
I just remembered: in some context we want to apply the ViewPolicy filter (i.e. OPAC and some staff interface views) but in some others we don't want filtering out things i.e. exporting records, etc. That's why it was built like that. Maybe we should force the `interface` param and also add a `skip_view_policy` parameter for select use cases (e.g. record indexing). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #173535|0 |1 is obsolete| | --- Comment #55 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173588 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173588&action=edit Bug 31224: EmbedItems takes an arrayref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #56 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173597 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173597&action=edit Bug 31224: Expand coded values using metadata_record in Server.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #57 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 173598 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173598&action=edit Bug 31224: Adapt GET /public/biblios/:biblio_id controller Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=38286 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Jonathan Druart <jonathan.druart@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=31224 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #173530|0 |1 is obsolete| | Attachment #173531|0 |1 is obsolete| | Attachment #173532|0 |1 is obsolete| | Attachment #173533|0 |1 is obsolete| | Attachment #173534|0 |1 is obsolete| | Attachment #173588|0 |1 is obsolete| | Attachment #173597|0 |1 is obsolete| | Attachment #173598|0 |1 is obsolete| | --- Comment #58 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 173636 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173636&action=edit Bug 31224: Add Koha::Biblio->metadata_record This patch introduces a higher-level method for retrieving a 'prepared' record for using in specific contexts. In particular, I only focused on embedding item information and OPAC view filtering. But we could add a way to get the record through the ViewPolicy also for staff. The virtue of this patch is that it explains better than my words why I think having the *embed_items* and *opac* parameters in Koha::Biblio::Metadata->record is not ideal. And makes the other implementation feel like is done at the wrong level. I know it's been done like that to act as a drop-in replacement fro GetMarcBiblio, which was a good first step. But we need to revisit how it is used, in general, to come up with a more refined and useful design. So we do not port the same design problems from C4::* into Koha::*. Try to think how it would be used in contexts like opac-MARCdetail.pl or any other OPAC controller script. Thanks! Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #59 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 173637 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173637&action=edit Bug 31224: Use metadata_record in opac-MARCdetail.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #60 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 173638 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173638&action=edit Bug 31224: Convert C4::Record::marcrecord2csv Update marcrecord2csv to user RecordProcessor directly for embedding listed items. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #61 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 173639 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173639&action=edit Bug 31224: Support 'interface' on metadata_record We replace the 'opac' boolean with an 'interface' param here to allow for ViewPolicy filtering depending on 'opac', 'intranet' or 'nothing' being passed. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #62 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 173640 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173640&action=edit Bug 31224: Update instances of metadata->record We can now call metadata_record directly on the Koha::Biblio object. This aptch updates all modules and controllers to use Koha::Biblio->metadata_record directly where appropriate. The exceptions are where we don't require any filtering or the filtering makes sense to do after the initial fetch. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #63 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 173641 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173641&action=edit Bug 31224: EmbedItems takes an arrayref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #64 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 173642 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173642&action=edit Bug 31224: Expand coded values using metadata_record in Server.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #65 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 173643 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173643&action=edit Bug 31224: Adapt GET /public/biblios/:biblio_id controller Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #66 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 173644 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173644&action=edit Bug 31224: (follow-up) Update instances of metadata->record Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #67 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- I'm tempted to call this PQA to keep things moving here.. we've got Tomas, Jonathan and I all onboard with it and though I agree to the comments left, I also agree they make sense as follow-ups on new bugs. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Martin Renvoize (ashimema) <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=31224 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #68 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #69 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- This broke tests, please fix URGENTLY: git bisect: [8c5135dadbe8cd6d1df61e46026be8996f97f27c] Bug 31224: Add Koha::Biblio->metadata_record * t_db_dependent_Record_marcrecord2csv_t * t_db_dependent_Exporter_Record_t. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #70 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Katrin Fischer from comment #69)
This broke tests, please fix URGENTLY:
git bisect: [8c5135dadbe8cd6d1df61e46026be8996f97f27c] Bug 31224: Add Koha::Biblio->metadata_record
* t_db_dependent_Record_marcrecord2csv_t * t_db_dependent_Exporter_Record_t.
Ok! I will fix them! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |38406 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38406 [Bug 38406] Item fields should not be stored on the DB -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #71 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 174262 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174262&action=edit Bug 31224: Move item field removal up to $metadata->record This patch addresse the fact Koha needs to strip out stray item field in MARC records coming from the DB. While this is not ideal, it has worked so far, limiting the negative effects of bad data to Koha users. I put a FIXME because I think it deserves to be revisited colectively at some point. I filed bug 38406 to track the discussion around this. The `marcmarcrecord2csv.t` tests cover this behavior and I thought removing it was not in the scope of this bug. I decided to move the removal one step up in the call chain so it applies to all calls to `$metadata->record`, not only those that ask to include items. This is the right thing to do while we keep this behavior. To test: 1. Run: $ ktd --shell k$ prove t/db_dependent/Record/marcrecord2csv.t => FAIL: Item information gets extracted from more items than expected (i.e. the item in the MARC record is not stripped out so 'Withdrawn' shows 3 times instead of 2). 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass! 4. Run: k$ prove t/db_dependent/Koha/Biblio/Metadata.t => SUCCESS: Tests pass! The behavior change in $metadata->record works as intended! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #72 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 174263 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174263&action=edit Bug 31224: Revert to $metadata->record in `marcrecord2csv` This patch reverts the change for marcrecord2csv as `$biblio->metadata_record` doesn't support a list of itemnumbers, and it feels we should discuss in another context whether to add it as a parameter, or make it a Koha::Items iterator. The Koha::Item object is actually retrieved in marc2csv so there's room for easy improvements without adding technical debt (i.e. bad method signatures for a single use case). This fixes a feature: 1. Run: $ ktd --shell k$ prove t/db_dependent/Exporter/Record.t => FAIL: Test fails because the 'items' are not being filtered on the passed itemnumbers (i.e. all items are picked for generating the CSV). 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #73 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Ok, my patches highlight the problem. The first one fixes some, adding a FIXME for a situation that is explained in the commit message. And then, the second one acknowledges the fact we need to discuss/think a little bit more on how/who will refactor the marc2csv code. Which I think should happen on its own bug. Gotta go to bed now. Hope others can revisit what I've just submitted and hopefully come up with better solutions :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #74 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #73)
Ok, my patches highlight the problem. The first one fixes some, adding a FIXME for a situation that is explained in the commit message. And then, the second one acknowledges the fact we need to discuss/think a little bit more on how/who will refactor the marc2csv code. Which I think should happen on its own bug.
Gotta go to bed now. Hope others can revisit what I've just submitted and hopefully come up with better solutions :-D
Thanks Tomas! I assumed it was about repeated fields, but didn't think it was for items, very interesting. It sounds like this is the way to go for now, but needs more work/thought in future. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 --- Comment #75 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Picked 2 follow-up patches ("Move item field..." and "Revert to $metadata->record...") for push to main. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Status|Pushed to main |Needs documenting --- Comment #76 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- 24.05.x merge conflicts, no backport. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED CC| |caroline.cyr-la-rose@inlibr | |o.com --- Comment #77 from Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> --- Internal architecture, nothing to add/edit in the manual. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |39857 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39857 [Bug 39857] OAI expanded_avs option broken -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31224 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |40622 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40622 [Bug 40622] Bug 38233 not properly applied to 24.11.x+ -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org