Bug 40272 - Add an alert for incorrect (MARC21) fixed-length control fields
Summary: Add an alert for incorrect (MARC21) fixed-length control fields
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks: 41031
  Show dependency treegraph
 
Reported: 2025-06-30 08:58 UTC by Marcel de Rooy
Modified: 2025-10-16 21:21 UTC (History)
2 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This proposal adds an alert when opening the MARC basic editor while a control field (005-008) has an incorrect length.
Version(s) released in:
25.11.00
Circulation function:


Attachments
Bug 40272: Add an alert for bad fixed-length control fields (7.18 KB, patch)
2025-06-30 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40272: Add an alert for bad fixed-length control fields (7.19 KB, patch)
2025-06-30 12:16 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 40272: Add an alert for bad fixed-length control fields (7.26 KB, patch)
2025-09-22 13:58 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40272: Add an alert for bad fixed-length control fields (7.29 KB, patch)
2025-10-16 07:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40272: (QA follow-up) Fix new biblio/empty metadata case (4.24 KB, patch)
2025-10-16 07:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40272: (QA follow-up) Fix new biblio/empty metadata case (4.25 KB, patch)
2025-10-16 07:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40272: (QA follow-up) Only add a biblio test (1.67 KB, patch)
2025-10-16 09:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2025-06-30 08:58:50 UTC
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.
Comment 1 Marcel de Rooy 2025-06-30 09:30:38 UTC
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>
Comment 2 Owen Leonard 2025-06-30 12:16:51 UTC
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>
Comment 3 Martin Renvoize (ashimema) 2025-09-22 13:58:57 UTC
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>
Comment 4 Lucas Gass (lukeg) 2025-10-14 20:57:46 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 5 Lucas Gass (lukeg) 2025-10-15 17:32:37 UTC
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
Comment 6 Lucas Gass (lukeg) 2025-10-15 17:33:55 UTC
I've reverted this one.
Comment 7 Marcel de Rooy 2025-10-16 06:20:58 UTC
(In reply to Lucas Gass (lukeg) from comment #6)
> I've reverted this one.

Taking a look.
Comment 8 Marcel de Rooy 2025-10-16 07:33:34 UTC
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>
Comment 9 Marcel de Rooy 2025-10-16 07:33:36 UTC
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>
Comment 10 Marcel de Rooy 2025-10-16 07:42:07 UTC
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.
Comment 11 Marcel de Rooy 2025-10-16 07:45:28 UTC
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>
Comment 12 Marcel de Rooy 2025-10-16 07:45:47 UTC
Tidied follow-up
Comment 13 Marcel de Rooy 2025-10-16 09:05:07 UTC
Still looking here
Comment 14 Marcel de Rooy 2025-10-16 09:15:13 UTC
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>
Comment 15 Marcel de Rooy 2025-10-16 09:17:17 UTC
(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.
Comment 16 Marcel de Rooy 2025-10-16 09:29:23 UTC
(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.
Comment 17 Lucas Gass (lukeg) 2025-10-16 21:21:54 UTC
Nice work everyone!

Pushed to main for 25.11