Bug 31224 - Koha::Biblio::Metadata->record should use the EmbedItems filter
Summary: Koha::Biblio::Metadata->record should use the EmbedItems filter
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 3150 23247 29697 33083
Blocks:
  Show dependency treegraph
 
Reported: 2022-07-22 19:05 UTC by Tomás Cohen Arazi
Modified: 2024-02-13 14:02 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
[POC] Bug 31224: Add Koha::Biblio->metadata_record (3.79 KB, patch)
2022-07-22 20:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[POC] Bug 31224: Add Koha::Biblio->metadata_record (2.87 KB, patch)
2022-07-22 20:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[POC] Bug 31224: Use metadata_record in opac-MARCdetail.pl (1.35 KB, patch)
2022-07-22 20:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[POC] Bug 31224: Add Koha::Biblio->metadata_record (3.16 KB, patch)
2022-07-22 21:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[POC] Bug 31224: Use metadata_record in opac-MARCdetail.pl (1.35 KB, patch)
2022-07-22 21:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31224: Add Koha::Biblio->metadata_record (3.16 KB, patch)
2022-10-19 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Use metadata_record in opac-MARCdetail.pl (1.34 KB, patch)
2022-10-19 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: (QA follow-up) Fix syntax error (1.69 KB, patch)
2022-10-19 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Convert C4::Record::marcrecord2csv (1.24 KB, patch)
2022-10-19 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Support 'interface' on metadata_record (2.04 KB, patch)
2022-10-19 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Update instances of metadata->record (17.18 KB, patch)
2022-10-19 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Update instances of metadata->record (18.41 KB, patch)
2022-10-19 09:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Add Koha::Biblio->metadata_record (3.24 KB, patch)
2024-02-08 16:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Use metadata_record in opac-MARCdetail.pl (2.51 KB, patch)
2024-02-08 16:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: (QA follow-up) Fix syntax error (1.75 KB, patch)
2024-02-08 16:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Convert C4::Record::marcrecord2csv (1.31 KB, patch)
2024-02-08 16:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Support 'interface' on metadata_record (2.11 KB, patch)
2024-02-08 16:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Update instances of metadata->record (16.55 KB, patch)
2024-02-08 16:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Add Koha::Biblio->metadata_record (3.24 KB, patch)
2024-02-08 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Use metadata_record in opac-MARCdetail.pl (2.51 KB, patch)
2024-02-08 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: (QA follow-up) Fix syntax error (1.75 KB, patch)
2024-02-08 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Convert C4::Record::marcrecord2csv (1.01 KB, patch)
2024-02-08 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Support 'interface' on metadata_record (2.43 KB, patch)
2024-02-08 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31224: Update instances of metadata->record (16.55 KB, patch)
2024-02-08 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-07-22 19:05:13 UTC
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.
Comment 1 Tomás Cohen Arazi 2022-07-22 20:14:35 UTC
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.
Comment 2 Tomás Cohen Arazi 2022-07-22 20:50:58 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2022-07-22 20:52:32 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2022-07-22 20:53:22 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2022-07-22 20:59:55 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2022-07-22 21:06:36 UTC
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!
Comment 7 Tomás Cohen Arazi 2022-07-22 21:06:40 UTC
Created attachment 138064 [details] [review]
[POC] Bug 31224: Use metadata_record in opac-MARCdetail.pl
Comment 8 Tomás Cohen Arazi 2022-07-22 21:08:20 UTC
Ok, I added a sample usage.
Comment 9 Jonathan Druart 2022-07-23 06:05:24 UTC
Then, what's next? Please make sure to work on top of bug 27272 please.
Comment 10 Tomás Cohen Arazi 2022-07-25 11:15:29 UTC
(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.
Comment 11 Martin Renvoize 2022-10-13 07:04:59 UTC
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?
Comment 12 Tomás Cohen Arazi 2022-10-18 12:08:51 UTC
(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.
Comment 13 Martin Renvoize 2022-10-19 09:20:27 UTC
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!
Comment 14 Martin Renvoize 2022-10-19 09:20:32 UTC
Created attachment 142138 [details] [review]
Bug 31224: Use metadata_record in opac-MARCdetail.pl
Comment 15 Martin Renvoize 2022-10-19 09:20:37 UTC
Created attachment 142139 [details] [review]
Bug 31224: (QA follow-up) Fix syntax error
Comment 16 Martin Renvoize 2022-10-19 09:20:41 UTC
Created attachment 142140 [details] [review]
Bug 31224: Convert C4::Record::marcrecord2csv

Update marcrecord2csv to user RecordProcessor directly for embedding
listed items.
Comment 17 Martin Renvoize 2022-10-19 09:20:46 UTC
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.
Comment 18 Martin Renvoize 2022-10-19 09:20:50 UTC
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.
Comment 19 Martin Renvoize 2022-10-19 09:25:12 UTC
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?
Comment 20 Martin Renvoize 2022-10-19 09:26:09 UTC
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?
Comment 21 Martin Renvoize 2022-10-19 09:38:55 UTC
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.
Comment 22 Marcel de Rooy 2022-10-20 12:52:11 UTC
Please fix:

root@master:/usr/share/koha# git grep  EmbedItemsInMarcBiblio
C4/Biblio.pm:        EmbedItemsInMarcBiblio

Exported not existing
Comment 23 Katrin Fischer 2023-06-17 12:35:00 UTC
(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
Comment 24 Tomás Cohen Arazi 2023-08-02 20:16:02 UTC
(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.
Comment 25 Martin Renvoize 2024-02-02 14:50:12 UTC
Such a shame things like this just die instead of getting support.
Comment 26 Tomás Cohen Arazi 2024-02-02 14:55:34 UTC
(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
Comment 27 Martin Renvoize 2024-02-08 16:11:50 UTC
Master moved a chunk.. this needs rebasing on at least bug 33083
Comment 28 Martin Renvoize 2024-02-08 16:53:29 UTC
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>
Comment 29 Martin Renvoize 2024-02-08 16:53:32 UTC
Created attachment 161934 [details] [review]
Bug 31224: Use metadata_record in opac-MARCdetail.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2024-02-08 16:53:35 UTC
Created attachment 161935 [details] [review]
Bug 31224: (QA follow-up) Fix syntax error

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2024-02-08 16:53:37 UTC
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>
Comment 32 Martin Renvoize 2024-02-08 16:53:40 UTC
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>
Comment 33 Martin Renvoize 2024-02-08 16:53:43 UTC
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>
Comment 34 Martin Renvoize 2024-02-08 16:55:51 UTC
Rebased
Comment 35 Martin Renvoize 2024-02-08 17:17:02 UTC
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>
Comment 36 Martin Renvoize 2024-02-08 17:17:05 UTC
Created attachment 161940 [details] [review]
Bug 31224: Use metadata_record in opac-MARCdetail.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2024-02-08 17:17:07 UTC
Created attachment 161941 [details] [review]
Bug 31224: (QA follow-up) Fix syntax error

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2024-02-08 17:17:10 UTC
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>
Comment 39 Martin Renvoize 2024-02-08 17:17:13 UTC
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>
Comment 40 Martin Renvoize 2024-02-08 17:17:16 UTC
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>
Comment 41 Martin Renvoize 2024-02-08 17:21:40 UTC
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.
Comment 42 Tomás Cohen Arazi 2024-02-13 14:02:37 UTC
(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.