This proposal adds an alert when opening the MARC basic editor while a control field (005-008) has an incorrect length. Note that some historical bugs may have caused 008 fields to be shorter than 40 positions.
Created attachment 183640 [details] [review] Bug 40272: Add an alert for bad fixed-length control fields Development wise it is much simpler to do this check in perl when opening the editor than getting this logic into the template with javascript. Furthermore, older data may be corrupted, but we trust the current plugins to not insert such fields anymore. Test plan: Run t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC/MARC21.t Open an existing record in the basic editor. Shorten 008 without plugin. Save and open the record again. Do you get the alert? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 183641 [details] [review] Bug 40272: Add an alert for bad fixed-length control fields Development wise it is much simpler to do this check in perl when opening the editor than getting this logic into the template with javascript. Furthermore, older data may be corrupted, but we trust the current plugins to not insert such fields anymore. Test plan: Run t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC/MARC21.t Open an existing record in the basic editor. Shorten 008 without plugin. Save and open the record again. Do you get the alert? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 186710 [details] [review] Bug 40272: Add an alert for bad fixed-length control fields Development wise it is much simpler to do this check in perl when opening the editor than getting this logic into the template with javascript. Furthermore, older data may be corrupted, but we trust the current plugins to not insert such fields anymore. Test plan: Run t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC/MARC21.t Open an existing record in the basic editor. Shorten 008 without plugin. Save and open the record again. Do you get the alert? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Nice work everyone! Pushed to main for 25.11
This isn't ready for inclusion. With this patchset applied I cannot create a bib with the basic editor: Can't call method "field" on an undefined value at /kohadevbox/koha/Koha/Biblio/Metadata/Extractor/MARC/MARC21.pm line 133
I've reverted this one.
(In reply to Lucas Gass (lukeg) from comment #6) > I've reverted this one. Taking a look.
Created attachment 187959 [details] [review] Bug 40272: Add an alert for bad fixed-length control fields Development wise it is much simpler to do this check in perl when opening the editor than getting this logic into the template with javascript. Furthermore, older data may be corrupted, but we trust the current plugins to not insert such fields anymore. Test plan: Run t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC/MARC21.t Open an existing record in the basic editor. Shorten 008 without plugin. Save and open the record again. Do you get the alert? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 187960 [details] [review] Bug 40272: (QA follow-up) Fix new biblio/empty metadata case Test plan: Add new biblio record in basic editor. Run t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC/MARC21.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Hi Lucas, Good catch. The new biblio case should surely have been included in the original test plan.. I did a bit more than adding the strictly needed "if $biblio"-check in the addbiblio script on the follow-up. I also added a check too in the extractor code on having a MARC::Record object. Actually this should imo be in Extractor::MARC->metadata. But that should enlarge the scope of this report too much for now. Will open a new report for that. And added both cases in unit tests: metadata is just a string, metadata is an empty MARC::Record object.
Created attachment 187961 [details] [review] Bug 40272: (QA follow-up) Fix new biblio/empty metadata case Test plan: Add new biblio record in basic editor. Run t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC/MARC21.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tidied follow-up
Still looking here
Created attachment 187978 [details] [review] Bug 40272: (QA follow-up) Only add a biblio test And move the comment where it belongs. More changes on 41031. Test plan: Add a new biblio record with basic editor. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #10) > Hi Lucas, > > Good catch. The new biblio case should surely have been included in the > original test plan.. > > I did a bit more than adding the strictly needed "if $biblio"-check in the > addbiblio script on the follow-up. > I also added a check too in the extractor code on having a MARC::Record > object. Actually this should imo be in Extractor::MARC->metadata. But that > should enlarge the scope of this report too much for now. Will open a new > report for that. > And added both cases in unit tests: metadata is just a string, metadata is > an empty MARC::Record object. Sorry for the noise here. But I finally decided not to include more changes but do a quick fix here. And add more changes on the new report. Adjusting the check_fixed_length too.
(In reply to Marcel de Rooy from comment #15) > And add more changes on the new report. Adjusting the check_fixed_length too. => Adjusting the check_fixed_length tests too.