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.
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.
Created attachment 138060 [details] [review] [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!
Created attachment 138061 [details] [review] [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!
Resubmitted as an unrelated change slipped in.
Created attachment 138062 [details] [review] [POC] Bug 31224: Use metadata_record in opac-MARCdetail.pl
Created attachment 138063 [details] [review] [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!
Created attachment 138064 [details] [review] [POC] Bug 31224: Use metadata_record in opac-MARCdetail.pl
Ok, I added a sample usage.
Then, what's next? Please make sure to work on top of bug 27272 please.
(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.
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?
(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.
Created attachment 142137 [details] [review] 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!
Created attachment 142138 [details] [review] Bug 31224: Use metadata_record in opac-MARCdetail.pl
Created attachment 142139 [details] [review] Bug 31224: (QA follow-up) Fix syntax error
Created attachment 142140 [details] [review] Bug 31224: Convert C4::Record::marcrecord2csv Update marcrecord2csv to user RecordProcessor directly for embedding listed items.
Created attachment 142141 [details] [review] 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.
Created attachment 142142 [details] [review] 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.
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?
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?
Created attachment 142143 [details] [review] 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.
Please fix: root@master:/usr/share/koha# git grep EmbedItemsInMarcBiblio C4/Biblio.pm: EmbedItemsInMarcBiblio Exported not existing
(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
(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.
Such a shame things like this just die instead of getting support.
(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
Master moved a chunk.. this needs rebasing on at least bug 33083
Created attachment 161933 [details] [review] 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>
Created attachment 161934 [details] [review] Bug 31224: Use metadata_record in opac-MARCdetail.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161935 [details] [review] Bug 31224: (QA follow-up) Fix syntax error Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161936 [details] [review] 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>
Created attachment 161937 [details] [review] 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>
Created attachment 161938 [details] [review] 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>
Rebased
Created attachment 161939 [details] [review] 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>
Created attachment 161940 [details] [review] Bug 31224: Use metadata_record in opac-MARCdetail.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161941 [details] [review] Bug 31224: (QA follow-up) Fix syntax error Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 161942 [details] [review] 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>
Created attachment 161943 [details] [review] 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>
Created attachment 161944 [details] [review] 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>
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.
(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.