|
Description
Nick Clemens (kidclamp)
2023-10-18 17:33:39 UTC
There is a test to catch that t/db_dependent/selenium/regressions.t Created attachment 157394 [details] [review] Bug 35099: Defensive coding for bad MARC21 records This patch modernises the get_marc_volumes method to match the form of get_marc_components. The original code was submitted around a similar time but didn't get the modernisations introduced since. We remove the original localised caching as well as putting an eval around the search_simple_compat call to prevent crashes on bad marc records found in the database. Created attachment 157398 [details] [review] Bug 35099: Defensive coding for bad MARC21 records This patch modernises the get_marc_volumes method to match the form of get_marc_components. The original code was submitted around a similar time but didn't get the modernisations introduced since. We remove the original localised caching as well as putting an eval around the search_simple_compat call to prevent crashes on bad marc records found in the database. Created attachment 157423 [details] [review] Bug 35099: Prevent crash from invalid marc in the parent record When getting the query for components, we need to parse the MARC::Record. If that fails, we simply should not attempt the component search as the user should see a warning about the degraded MARC To test: prove -v t/db_dependent/selenium/regressions.t It fails apply patch, restart_all prove -v t/db_dependent/selenium/regressions.t It passes Created attachment 157430 [details] [review] Bug 35099: Defensive coding for bad MARC21 records This patch modernises the get_marc_volumes method to match the form of get_marc_components. The original code was submitted around a similar time but didn't get the modernisations introduced since. We remove the original localised caching as well as putting an eval around the search_simple_compat call to prevent crashes on bad marc records found in the database. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 157431 [details] [review] Bug 35099: Prevent crash from invalid marc in the parent record When getting the query for components, we need to parse the MARC::Record. If that fails, we simply should not attempt the component search as the user should see a warning about the degraded MARC To test: prove -v t/db_dependent/selenium/regressions.t It fails apply patch, restart_all prove -v t/db_dependent/selenium/regressions.t It passes Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 157432 [details] [review] Bug 35099: Improve consistency This patch inproves the consistency between get_components_query and get_get_volumes_query methods so they both check for MARC21 and check that the record is valid prior to building the query. We also add a check for !$invalid_marc_record in the staff client details view to match the check for the equivilent compenents check. We need both as the OPAC doesn't do an early check for invalid records in the controller. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 1. tests are missing
2. 739 return [] if ( C4::Context->preference('marcflavour') ne 'MARC21' );
Why that? Where is it advertised?
3.
781 my $marc;
782 eval { $marc = $self->metadata->record; };
783 return unless $marc;
I disagree with that, and I thought we agreed on it.
We must assume the MARC is good, and deal with invalid record on the detail page (and edition now) only.
The comment is also wrong, you could call this method from somewhere else, and we just ignore the error, which we should (almost) never do.
4. Finally, isn't this change enough?
-my $show_volumes = @{ $biblio->get_marc_volumes(1) } ? 1 : 0;
+my $show_volumes = ( !$invalid_marc_record && @{ $biblio->get_marc_volumes(1) } ) ? 1 : 0;
I just tried to make it all more consistent with the existing methods that accomplish the same. The OPAC doesn't do early checks and thus currently explodes I suspect without this patchset.. looks likely only staff was covered to me. I worked in a fix quickly for release.. I confirm that the following change makes the test pass and 369 is accessible with a nice red warning.
-my $show_volumes = @{ $biblio->get_marc_volumes(1) } ? 1 : 0;
+my $show_volumes = ( !$invalid_marc_record && @{ $biblio->get_marc_volumes(1) } ) ? 1 : 0;
Created attachment 157510 [details] [review] Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML This patch fixes the original issue. If you have other concerns you should open new bug reports and provide tests. Created attachment 157513 [details] [review] Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Trivial fix, passing QA - there are larger issues around this, but this gets things working Created attachment 157515 [details] [review] Bug 35099: (bug 26314 follow-up) Fix detail view for records with invalid MARCXML Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> lol We clashed.. but were doing the same thing :) Pushed to master for 23.11. Nice work everyone, thanks! Depends on Bug 26314 not in 23.05.x |