|
Description
Tomás Cohen Arazi (tcohen)
2022-07-22 19:05:13 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. 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. Created attachment 173530 [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 173531 [details] [review] Bug 31224: Use metadata_record in opac-MARCdetail.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 173532 [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 173533 [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 173534 [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> Created attachment 173535 [details] [review] Bug 31224: EmbedItems takes an arrayref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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. (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. 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.
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'? (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. (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). Created attachment 173588 [details] [review] Bug 31224: EmbedItems takes an arrayref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 173597 [details] [review] Bug 31224: Expand coded values using metadata_record in Server.pm Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 173598 [details] [review] Bug 31224: Adapt GET /public/biblios/:biblio_id controller Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 173636 [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> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 173637 [details] [review] 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> Created attachment 173638 [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> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 173639 [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> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 173640 [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> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 173641 [details] [review] 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> Created attachment 173642 [details] [review] 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> Created attachment 173643 [details] [review] 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> Created attachment 173644 [details] [review] Bug 31224: (follow-up) Update instances of metadata->record Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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. Pushed for 24.11! Well done everyone, thank you! 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. (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! Created attachment 174262 [details] [review] 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> Created attachment 174263 [details] [review] 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> 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 (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. Picked 2 follow-up patches ("Move item field..." and "Revert to $metadata->record...") for push to main.
|