[Bug 43025] New: Show metadata repair warnings in the cataloguing edit form
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Bug ID: 43025 Summary: Show metadata repair warnings in the cataloguing edit form Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Cataloging Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org CC: m.de.rooy@rijksmuseum.nl Depends on: 35104 Target Milestone: --- Bug 35104 makes Koha::Biblio::Metadata->store() recover non-XML characters and empty datafields automatically (recording what was fixed in biblio_metadata_errors), or throw Koha::Exceptions::Metadata::Invalid when a record's MARCXML cannot be saved at all. This patch surfaces that in the place cataloguers actually hit it: the cataloguing edit form. This patch: - Catches Koha::Exceptions::Metadata::Invalid on save in cataloguing/addbiblio.pl and shows a clear error instead of a raw exception, keeping the form populated so nothing is lost. - Shows a "Record saved with modifications" warning when a save succeeded but required automatic repair. - Lists any pre-existing, unresolved metadata errors when opening a record for editing, each with a quick-link to the affected field (where the field survived the repair) and a "Mark resolved" action. - Adds DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id} to resolve (delete) a metadata error row, used by the "Mark resolved" action. Depends on bug 35104. Test plan: 1. Apply this patch on top of bug 35104. 2. Catalogue a new record (or edit an existing one) and add a control character (e.g. paste text containing a Unit Separator, 0x1F) into a subfield value. Save the record. 3. Confirm the record saves successfully and a "Record saved with modifications" warning banner appears describing what was automatically fixed. 4. Re-open the record for editing. Confirm a "Data quality warnings" banner lists the fault, with a "Go to field" link and a "Mark resolved" button. 5. Click "Go to field" - confirm it switches to the correct tab and scrolls to the affected field. 6. Click "Mark resolved" - confirm the warning disappears immediately and does not reappear after reloading the page. 7. Save a record that cannot be repaired at all (e.g. temporarily force Koha::Biblio::Metadata::store to throw Koha::Exceptions::Metadata::Invalid, or use a MARC::File::XML error you can reproduce) - confirm a clear error message is shown and the form is redisplayed with your data intact, rather than a raw exception or a 500 error. 8. prove t/db_dependent/api/v1/biblios_metadata_errors.t Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 [Bug 35104] We should warn when attempting to save MARC records that contain characters invalid in XML -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 201701 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201701&action=edit Bug 43025: Add REST endpoint to resolve a biblio metadata error Adds DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id} to mark a recorded metadata fault (see bug 35104) as resolved. Deletion is scoped through the biblio's own metadata_errors relation, so an error_id belonging to a different biblio correctly 404s rather than being deletable cross-biblio. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 201702 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201702&action=edit Bug 43025: Show metadata repair warnings and existing errors in the cataloguing edit form Surfaces bug 35104's exception/recovery behaviour where cataloguers actually encounter it - saving a record: - If the record is unrecoverable, AddBiblio/ModBiblio's Koha::Exceptions::Metadata::Invalid is caught and shown instead of a generic error, with the form re-displayed so nothing is lost. - If the record was recoverable but modified, the save proceeds and a warning banner explains what was automatically fixed (via the new warnings option on AddBiblio/ModBiblio). - Any pre-existing (not yet resolved) errors on the record are listed when opening it for editing, each with a quick-link to the affected field (where the field survived the repair) and a "Mark resolved" action calling the new REST DELETE endpoint. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #201701|0 |1 is obsolete| | Attachment #201702|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 201727 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201727&action=edit Bug 43025: Add REST endpoint to resolve a biblio metadata error Adds DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id} to mark a recorded metadata fault (see bug 35104) as resolved. Deletion is scoped through the biblio's own metadata_errors relation, so an error_id belonging to a different biblio correctly 404s rather than being deletable cross-biblio. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 201728 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201728&action=edit Bug 43025: Show metadata repair warnings and existing errors in the cataloguing edit form Surfaces bug 35104's exception/recovery behaviour where cataloguers actually encounter it - saving a record: - If the record is unrecoverable, AddBiblio/ModBiblio's Koha::Exceptions::Metadata::Invalid is caught and shown instead of a generic error, with the form re-displayed so nothing is lost. - If the record was recoverable but modified, the save proceeds and a warning banner explains what was automatically fixed (via the new warnings option on AddBiblio/ModBiblio). - Any pre-existing (not yet resolved) errors on the record are listed when opening it for editing, each with a quick-link to the affected field (where the field survived the repair) and a "Mark resolved" action calling the new REST DELETE endpoint. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Status|Signed Off |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off Comma delimited| |OpenFifth list of Sponsors| | Sponsorship status|--- |Sponsored -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #201727|0 |1 is obsolete| | Attachment #201728|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203269 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203269&action=edit Bug 43025: Add REST endpoint to resolve a biblio metadata error Adds DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id} to mark a recorded metadata fault (see bug 35104) as resolved. Deletion is scoped through the biblio's own metadata_errors relation, so an error_id belonging to a different biblio correctly 404s rather than being deletable cross-biblio. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 203270 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203270&action=edit Bug 43025: Show metadata repair warnings and existing errors in the cataloguing edit form Surfaces bug 35104's exception/recovery behaviour where cataloguers actually encounter it - saving a record: - If the record is unrecoverable, AddBiblio/ModBiblio's Koha::Exceptions::Metadata::Invalid is caught and shown instead of a generic error, with the form re-displayed so nothing is lost. - If the record was recoverable but modified, the save proceeds and a warning banner explains what was automatically fixed (via the new warnings option on AddBiblio/ModBiblio). - Any pre-existing (not yet resolved) errors on the record are listed when opening it for editing, each with a quick-link to the affected field (where the field survived the repair) and a "Mark resolved" action calling the new REST DELETE endpoint. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #7 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- Created attachment 203656 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=203656&action=edit Bug 43025: Handle encoding errors when viewing and loading records This patch updates the pre-existing handling of errors to use the new recovery routine and display all of the errors correctly formatted Ultimately - fix_marcxml should probably contain the code from store that generates the errors into the DB table, but this a large tree and we should polish once it is in The other patches here are useful, however, I can't actually manage to enter invalid characters in either editor -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #8 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Hi Nick, Thanks for digging into this and leaving a patch — genuinely useful QA, and I agree with the underlying idea (using repair_marcxml() for a proper per-fault breakdown instead of the older record_strip_nonxml + raw exception dump on load). I didn't apply it as-is though, for a few reasons: - There's a typo in the template: nonxml_stripped.char_od should be char_ord — that's the actual key _find_nonxml_chars() returns, so that field would have rendered blank. - repair_marcxml() returns an empty list when it can't identify a known-recoverable fault (i.e. the record is broken in some way other than stray non-XML characters or empty datafields). In that case the patch left $record undef with nothing downstream to catch it. I kept the original record_strip_nonxml fallback for that case, so a genuinely unrecoverable record still degrades gracefully instead of crashing the page. - The patch touched both cataloguing/addbiblio.pl (this bug) and catalogue/detail.pl (bug 43026's territory), so I split it across the two bugs it actually belongs to. I've reworked and applied both halves with those fixes, and credited your patch in the commit messages (Based-on-patch-by:). On your broader point — moving the error-recording itself into a shared routine so a plain load/view also persists to biblio_metadata_errors, not just a save — I agree that's worth doing, but think it deserves its own bug: it means a GET request writing to the DB (locking/concurrency implications on a hot page) and touching Koha::Biblio::Metadata itself rather than any of these follow-up bugs. Happy to file that separately, or if you'd rather pick it up yourself, go for it. Will get updated patches up on this bug and 43026 shortly for another look whenever you have time. Thanks again, -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #203269|0 |1 is obsolete| | Attachment #203270|0 |1 is obsolete| | Attachment #203656|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204733 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204733&action=edit Bug 43025: Add REST endpoint to resolve a biblio metadata error Adds DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id} to mark a recorded metadata fault (see bug 35104) as resolved. Deletion is scoped through the biblio's own metadata_errors relation, so an error_id belonging to a different biblio correctly 404s rather than being deletable cross-biblio. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #10 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204734 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204734&action=edit Bug 43025: Show metadata repair warnings and existing errors in the cataloguing edit form Surfaces bug 35104's exception/recovery behaviour where cataloguers actually encounter it - saving a record: - If the record is unrecoverable, AddBiblio/ModBiblio's Koha::Exceptions::Metadata::Invalid is caught and shown instead of a generic error, with the form re-displayed so nothing is lost. - If the record was recoverable but modified, the save proceeds and a warning banner explains what was automatically fixed (via the new warnings option on AddBiblio/ModBiblio). - Any pre-existing (not yet resolved) errors on the record are listed when opening it for editing, each with a quick-link to the affected field (where the field survived the repair) and a "Mark resolved" action calling the new REST DELETE endpoint. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Sponsored-by: OpenFifth -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204735 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204735&action=edit Bug 43025: Use repair_marcxml to show structured errors when loading an already-corrupt record The cataloguing edit form load path (unrelated to bug 35104 itself) fell back to record_strip_nonxml + a raw exception dump when a record was already undecodable before it could be opened for editing. That gave a much less useful message than the per-fault breakdown this bug already shows for faults introduced/fixed during a fresh save. Try the same repair_marcxml() used on save first; only fall back to the old record_strip_nonxml + raw exception display if repair_marcxml cannot identify a known-recoverable fault (i.e. the record is genuinely unrecoverable by this mechanism). Based-on-patch-by: Nick Clemens <nick@bywatersolutions.com> Sponsored-by: OpenFifth -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43025 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org