https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #1 from David Cook <dcook@prosentient.com.au> --- When I was writing bug 34549, I thought a little bit about how this might be done. Whether it was better to do it by Javascript on the page, an AJAX call, or do it on page submission server side... -- 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=35104 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- As per my comment at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34549#c19 technically the problem we're trying to solve in bug 34549 is preventing the saving of characters that cannot be rendered in XML. Picking up on encoding problems is a lot trickier because bytes can be valid in multiple different encodings. The machine can still output a valid character, so it doesn't realize that the source data is encoded incorrectly. -- 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=35104 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- Created attachment 157625 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157625&action=edit Text file containing control characters -- 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=35104 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |In Discussion -- 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=35104 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157627|application/mbox |text/plain mime type| | Attachment #157627|0 |1 is patch| | -- 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=35104 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #157627|0 |1 is obsolete| | --- Comment #8 from David Cook <dcook@prosentient.com.au> --- Created attachment 157628 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157628&action=edit Bug 35104: Alert when inserting text invalid in XML into bib record This change adds a Javascript based alert when a user tries to save data via the bib record editor that is invalid in XML. Test plan: 0. Apply patch 1. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?frameworkcode= 2. Copy the data from "Text file containing control characters" 3. Paste the data into 245$a and 500$a 4. Click "Save" 5. Note the alerts at the top of the web page and that the record does not save 6. Resolve the problems 7. Click "Save" 8. Note that the record saves -- 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=35104 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This works really nicely, as such I'm signing off... Two little points however.. 1) I wonder if we could/should merge these errors into the existing validation alert rather than having it's own alert box? 2) I did consider whether we should go belt & braces and catch on the server too.. however I think this is a great improvement without that anyway. -- 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=35104 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #12 from David Cook <dcook@prosentient.com.au> --- While I appreciate the sign off, it's not quite ready yet. It doesn't calculate the tab currently for the "Go to field" link. It only works by accident for things like the 245 due to "tabindex" weirdness. -- 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=35104 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- I had an interesting experience that I thought I'd share here since it's relevant. One of my librarians was copying and pasting text from a PDF into Koha. When they did it, it generated a broken MARC record. When I did it, it worked fine. That's when I discovered that Chrome, Firefox, and Edge all treat PDF text differently. Consider the following phrase "like-minded people" which is actually broken against 2 lines on the hyphen in the PDF. If you copy from Chrome, it removes the hyphen so it becomes "likeminded people". If you copy from Firefox, it copies the hyphen and a line break so it becomes something like "like-\nminded people". If you copy from Edge, it mangles the hyphen and turns it into a "Start of text" control character, which will break Koha. -- While we work through our official solution, I came up with a little Javascript function which is run during the "paste" event in the MARC editor. It displays a "confirm" box which contains an explanatory warning, offers a tip on understanding the problem, and then provides an option to try to "fix" the record (by stripping out the bad characters). I'm just running that in their "IntranetUserJS" for now, but I thought I'd share this information. I think having a warning at "save time" is very useful especially for imported records. This "on paste" warning might also be useful though since it's at the time that they're entering the data, so it might be easier for them to notice the problem. -- In any case, it was great to find an example from the wild to test a fix against... -- 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=35104 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize from comment #15)

Wow, that's an impressive find. Man this stuff ends up in fun "exciting" places.

Thanks! I did feel pretty good working that one out! During this process, I've been thinking there might be an alternative to bug 34549 as well. Instead of erasing the invalid characters, surely we could just escape them. So I'm going to use my discovery from https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104#c14 to see if I can do an alternate version of bug 34549 where we escape the bad characters using numeric character references or an entity. So the STX character from that example would be converted to something like  instead of just erased. Of course, it would be good to try it out with a variety of examples and not just this sort of data corruption example. But maybe it's also one of those ones where we can just do our best and see how it evolves over time... -- 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=35104 --- Comment #18 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #17)

I'm going to poke around in this a bit more...

The TransformHtmlToMarc doesn't seem to affect it... If I do $record->as_formatted then I see: likeminded If I do $record->as_xml then I see: &#2; Looking at https://metacpan.org/dist/MARC-File-XML/source/lib/MARC/File/XML.pm#L378 there is an escape function that escapes ampersands and angle brackets. In theory, maybe MARC::File::XML should escape any invalid characters using character references or remove them since they're invalid. But MARC::File::XML's escaping means it's impossible for us to pre-escape any invalid characters. It feels like MARC::File::XML is essentially holding us hostage. We need to clean our input data (in whatever format) before it reaches MARC::File::XML, which seems a bit silly, since it's the XML format which has the restrictions... That being said... the XML 1.0 spec is pretty forgiving. After review, it's really just excluding *some* ASCII control characters, Unicode surrogates, U+FFFE, and U+FFFF. That's a really small number of characters and none of them are printable characters. -- 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=35104 --- Comment #20 from David Cook <dcook@prosentient.com.au> --- We've got an interesting assortment of fixes at the moment... If you already have a record with invalid characters, you can open it using bug 34014 which will scrub your record clean within the editor, but it will also show you the parser errors, so you could go and repair your record, in theory. With bug 34549, you can try to save invalid characters, and it will silently wipe them out. With bug 35104, I've been looking at a client-side warning, but it doesn't tell you exactly which characters are a problem. Maybe we'd be better off re-using the mechanism from bug 34014 here in bug 35104. That would remove the need for a client-side check, it would be more consistent, and then we could undo bug 34549... -- 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=35104 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |In Discussion --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I think we could perhaps go a level lower and put this exception into Koha::Biblio::Metadata->store maybe instead? We'll also need to account for the exception at various places with a try/catch block as detailed in the commit message.. but I wanted to start getting feedback on the proposed approach before committing to continuing down this path. -- 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=35104 --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 159621 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159621&action=edit Bug 35104: Initial attempt at catching the failure on new records Whilst I can see the warning output from store, I can't seem to actually catch any exceptions.. confused -- 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=35104 --- Comment #26 from David Cook <dcook@prosentient.com.au> --- I've applied the last 2 patches and restarted Koha... Bug 35104: [Alternative] Throw exception on store of invalid marcxml Bug 35104: Initial attempt at catching the failure on new records And then I added the text from "Text file containing control characters" into a test record -- I'm getting an error trace page. It looks like you made a little mistake when catching the exception and it's causing you to rethrow it. I'll fix that up in a sec... -- However, with the fix, we're still having a problem rendering the page. But I'll let folk take it from there... -- 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=35104 --- Comment #28 from David Cook <dcook@prosentient.com.au> --- I have to run and I'm away tomorrow morning, but hopefully this gets you a bit further. -- 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=35104 --- Comment #30 from David Cook <dcook@prosentient.com.au> --- (In reply to Martin Renvoize from comment #29)

Awesome.. I knew it was something silly when catching the exception!

That gives me enough to get going again :), thanks David

Yay teamwork! :D -- 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=35104 --- Comment #32 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 159811 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159811&action=edit Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. -- 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=35104 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #159621|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=35104 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |34551 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34551 [Bug 34551] Don't allow Z39.50 import if MARC can't be converted to MARCXML -- 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=35104 --- Comment #33 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 159812 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159812&action=edit Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. -- 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=35104 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #159810|0 |1 is obsolete| | -- 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=35104 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@ptfs-europe |ity.org |.com -- 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=35104 --- Comment #36 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #35)

IMO this is too low level, we should not call MARC::Record::new_from_xml everytime we store.

It will work ofc but what about perf?

I think that we should validate each time we store. However... maybe we could use XML::LibXML directly instead of MARC::Record to reduce some overhead. I don't know if it makes an actual difference in terms of perf though. And the nice thing about using MARC::Record is we know if it will break on subsequent usage. Perhaps more importantly, these patches don't add error handling for every instance of Koha::Biblio::Metadata->store(). We might be breaking Staged MARC imports here and not realizing it. So maybe we should add a Koha::Biblio::Metadata->validate() and just call it from the controller script for now?

What if I do want invalid marcxml? :D

:P -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Baptiste <baptiste.bayche@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Thanks for the follow-up here Baptiste.. it's great to see someone is still interested in this. I'll continue to add such catches elsewhere as it seems people do actually want to help pursue it. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Status|Signed Off |Patch doesn't apply --- Comment #40 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Getting sha1 errors. Can you post a branch on github? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #159812|0 |1 is obsolete| | --- Comment #42 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 173145 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=173145&action=edit Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: baptiste <baptiste.bayche@inlibro.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #44 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Rebased.. I started working on other cases but ran out of time for this.. there's a lot of calls to ModBiblio in Koha that would benefit from try/catch treatment after this and propagating the errors to screen or gracefully skipping over them. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- URL| |https://github.com/PTFS-Eur | |ope/koha/tree/bug_35104 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #173145|0 |1 is obsolete| | --- Comment #45 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 176820 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176820&action=edit Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: baptiste <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |baptiste.wojtkowski@biblibr | |e.com --- Comment #47 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Isn't there risk to generate internal server error outside of the addbiblio.pl ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176820|0 |1 is obsolete| | --- Comment #49 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176850 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176850&action=edit Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #51 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 176852 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=176852&action=edit Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #53 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- So.. we'll have other failing tests.. t/db_dependant/Biblio.t for example.. this is because we're actually testing that putting bad characters into the database should succeed and with this patchset it doesn't.. given that's the idea of this patchset.. to prevent bad data ever making it's way into the database I'm not sure how to proceed. We can continue to add tests for bad data to ensure we capture it and deal with it when reading data out (should someone decided to put it in directly using database queries).. but should we continue to allow ModBiblio to "press on regardless".. personally I think not. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=38933 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisette@bywatersolutions.co | |m -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |41927 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41927 [Bug 41927] bulkmarcimport stops if encounters bad record -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176851|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #176860|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=35104 --- Comment #56 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193986 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193986&action=edit Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. Signed-off-by: Baptiste Bayche <baptiste.bayche@inlibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #58 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193988 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193988&action=edit Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #60 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193990 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193990&action=edit Bug 35104: Persist nonxml_stripped on biblio_metadata, show on detail pages Add a nonxml_stripped column to biblio_metadata so that the automatic stripping of non-XML characters during store() survives beyond the current request. The column is set to the XML parse error message when stripping was needed, and cleared to NULL when the record is saved cleanly, so the warning disappears once the cataloguer has reviewed and re-saved the record. Show a warning alert on the biblio detail page and MARC detail page whenever nonxml_stripped is set, with a direct link to the edit form. Fix Koha::Import::Record::get_marc_record() to fall back to StripNonXmlChars when the raw staged MARCXML cannot be parsed, so that the import preview (showmarc.pl?importid=...) no longer returns a 404 for records that contain non-XML characters. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #62 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193992 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193992&action=edit Bug 35104: Update Biblio.t to reflect graceful non-XML stripping behaviour The ModBiblio subtest was written for the old exception-throwing behaviour where invalid MARC would emit a Perl warn() and the error would appear in the action log. With the graceful stripping introduced by this bug: - StripNonXmlChars recovers the record silently (no Perl warn()) - The stripping event is reported via the warnings arrayref in $options - nonxml_stripped is persisted on biblio_metadata - ModBiblio still succeeds and logs the modification to action logs Update the three assertions accordingly. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193985|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193987|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193989|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193991|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=35104 --- Comment #63 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193993 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193993&action=edit Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. 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=35104 --- Comment #65 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193995 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193995&action=edit Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. 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=35104 --- Comment #67 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193997 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193997&action=edit Bug 35104: Strip non-XML characters gracefully and notify user Instead of always throwing when MARCXML contains invalid characters, attempt recovery via StripNonXmlChars first. If stripping produces a parseable record, save the clean version and set stripped_nonxml on the object; only throw Koha::Exceptions::Metadata::Invalid when the record is truly unrecoverable. Propagate the strip notification through ModBiblioMarc -> ModBiblio / AddBiblio via an optional warnings arrayref in $options. Entry points (addbiblio.pl, merge.pl) collect warnings and display a prominent alert asking the user to review the saved record. Update unit tests: replace the embedded-ESC 'Invalid MARCXML handling' subtest with a chr(31)-based 'MARCXML with strippable non-XML characters' subtest and add a stripped_nonxml=undef assertion for clean records. 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=35104 --- Comment #69 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 193999 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=193999&action=edit Bug 35104: Warn and log when non-XML characters are stripped in bulkmarcimport Pass a per-record warnings arrayref through the ModBiblio/AddBiblio options so that any nonxml_stripped events are captured and reported via print STDERR and via printlog() to the import log file. 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=35104 --- Comment #71 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194001 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194001&action=edit Bug 35104: DBIC Scheme Update Patch from commit 7762ba6 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193994|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193996|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #193998|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194000|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=35104 --- Comment #72 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194004 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194004&action=edit Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. 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=35104 --- Comment #74 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194006 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194006&action=edit Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. 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=35104 --- Comment #76 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194008 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194008&action=edit Bug 35104: Strip non-XML characters gracefully and notify user Instead of always throwing when MARCXML contains invalid characters, attempt recovery via StripNonXmlChars first. If stripping produces a parseable record, save the clean version and set stripped_nonxml on the object; only throw Koha::Exceptions::Metadata::Invalid when the record is truly unrecoverable. Propagate the strip notification through ModBiblioMarc -> ModBiblio / AddBiblio via an optional warnings arrayref in $options. Entry points (addbiblio.pl, merge.pl) collect warnings and display a prominent alert asking the user to review the saved record. Update unit tests: replace the embedded-ESC 'Invalid MARCXML handling' subtest with a chr(31)-based 'MARCXML with strippable non-XML characters' subtest and add a stripped_nonxml=undef assertion for clean records. 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=35104 --- Comment #78 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194010 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194010&action=edit Bug 35104: Warn and log when non-XML characters are stripped in bulkmarcimport Pass a per-record warnings arrayref through the ModBiblio/AddBiblio options so that any nonxml_stripped events are captured and reported via print STDERR and via printlog() to the import log file. 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=35104 --- Comment #80 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194012 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194012&action=edit Bug 35104: DBIC Scheme Update 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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- URL|https://github.com/PTFS-Eur | |ope/koha/tree/bug_35104 | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bernard.scaife@openfifth.co | |.uk --- Comment #83 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- *** Bug 41927 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #85 from David Cook <dcook@prosentient.com.au> --- In cataloguing/addbiblio.pl why is "elsif ($encode_error)" at the bottom? Wouldn't it make more sense to put that near the top of the if/elsif and that way you could avoid adding all the "!$encode_error" condition to the other elsif statements? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194004|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194006|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194008|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194010|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194012|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=35104 --- Comment #87 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194152 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194152&action=edit Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. 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=35104 --- Comment #89 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194154 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194154&action=edit Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. 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=35104 --- Comment #91 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194156 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194156&action=edit Bug 35104: Strip non-XML characters gracefully and notify user Instead of always throwing when MARCXML contains invalid characters, attempt recovery via StripNonXmlChars first. If stripping produces a parseable record, save the clean version and set stripped_nonxml on the object; only throw Koha::Exceptions::Metadata::Invalid when the record is truly unrecoverable. Propagate the strip notification through ModBiblioMarc -> ModBiblio / AddBiblio via an optional warnings arrayref in $options. Entry points (addbiblio.pl, merge.pl) collect warnings and display a prominent alert asking the user to review the saved record. Update unit tests: replace the embedded-ESC 'Invalid MARCXML handling' subtest with a chr(31)-based 'MARCXML with strippable non-XML characters' subtest and add a stripped_nonxml=undef assertion for clean records. 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=35104 --- Comment #93 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194158 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194158&action=edit Bug 35104: Warn and log when non-XML characters are stripped in bulkmarcimport Pass a per-record warnings arrayref through the ModBiblio/AddBiblio options so that any nonxml_stripped events are captured and reported via print STDERR and via printlog() to the import log file. 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=35104 --- Comment #95 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194160 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194160&action=edit Bug 35104: DBIC Scheme Update 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=35104 --- Comment #97 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194162 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194162&action=edit Bug 35104: Automated Schema Update Patch from commit 0ca3840 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #99 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194164 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194164&action=edit Bug 35104: Replace nonxml_stripped column with biblio_metadata_errors table Instead of storing a single nonxml_stripped flag directly on biblio_metadata, introduce a normalised biblio_metadata_errors table that can hold multiple typed error rows per metadata record. - Add biblio_metadata_errors table (id, metadata_id FK, error_type, message, created_on) with CASCADE delete from biblio_metadata - Remove the nonxml_stripped column from biblio_metadata - Add Koha::Biblio::Metadata::Error / Errors object classes - Refactor Koha::Biblio::Metadata::store() to write/clear a nonxml_stripped row in biblio_metadata_errors rather than the old column - Add metadata_errors() accessor on Koha::Biblio::Metadata - Update C4::Biblio, catalogue/detail.pl, catalogue/MARCdetail.pl and tests to query biblio_metadata_errors instead of the old column 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=35104 --- Comment #101 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194166 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194166&action=edit Bug 35104: Record each stripped character as its own error with field and position context Previously a single error row was written with a deduplicated list of affected field references. This was an improvement over the raw XML parser error (which only reported the first bad character), but still lost the per-character detail the parser provided. Replace _nonxml_affected_fields() with _find_nonxml_chars(), which scans every datafield subfield and control field in the original MARC XML and returns one hashref per bad-character occurrence, carrying: - field: human-readable field reference (e.g. "245$a") - char_ord: decimal ordinal of the illegal character - position: 1-based character offset within the field value store() now writes one biblio_metadata_errors row per occurrence with a message in the form: 245$a: invalid char value 31 (U+001F) at position 10 This combines field/subfield identification (introduced in the previous commit) with the character value and in-field location that the XML parser previously reported only for the first bad character. The logger warn retains the original XML parser error string alongside the deduplicated field list, so operators still see the full technical detail. A fallback to storing the raw parser error is kept for the unlikely case where the regex scan finds no matches despite stripping being needed. catalogue/detail.pl and catalogue/MARCdetail.pl now join all per-character messages with newlines so the existing <pre> alert block shows all errors. 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=35104 --- Comment #103 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194168 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194168&action=edit Bug 35104: Use Unicode Control Pictures in nonxml_stripped context snippets Instead of a [U+XXXX] marker, the context snippet now shows a visible glyph in place of the stripped character: C0 controls (U+0000-U+001F) -> Unicode Control Pictures (U+2400-U+241F) DEL (U+007F) -> U+2421 SYMBOL FOR DELETE (?) other non-XML characters -> U+FFFD REPLACEMENT CHARACTER (?) The first line of the error message is unchanged, continuing to identify the character by its decimal value and U+XXXX code: 336$a: invalid char value 8 (U+0008) at position 12 t $xtac arr?\ $btxt V $2rdac ontent ^ -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #105 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194170 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194170&action=edit Bug 35104: Remove silent pre-emptive StripNonXmlChars call from TransformHtmlToMarc The call was only applied to the first subfield of a newly-created MARC field (the elsif branch), not to subsequent subfields added to an existing field, making the stripping incomplete and inconsistent. It was also never imported, causing an "Undefined subroutine" crash on any save via the MARC editor. Non-XML character stripping is now handled entirely by Koha::Biblio::Metadata::store(), which operates on the complete MARC XML, records each stripped character as a biblio_metadata_errors row, and logs a warning - giving cataloguers and administrators full visibility. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #107 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194172 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194172&action=edit Bug 35104: Add 'Mark resolved' button for nonxml_stripped errors in edit form Adds a REST DELETE endpoint and UI button so cataloguers can explicitly dismiss nonxml_stripped data-quality warnings without needing to re-save the record. - DELETE /api/v1/biblios/{biblio_id}/metadata/errors/{error_id} deletes the specific error row; requires edit_catalogue permission. - Koha::REST::V1::Biblios::MetadataErrors controller handles the request, verifying the error belongs to the given biblio before deleting. - The edit form's warning banner now shows a 'Mark resolved' button next to each error. Clicking it fires an AJAX DELETE; on success the row is removed from the DOM and the entire banner is hidden when no errors remain. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194152|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194154|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194156|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194158|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194160|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194162|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194164|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194166|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194168|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194170|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194172|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=35104 --- Comment #109 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194214 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194214&action=edit Bug 35104: Catch the failure on new and edited records We now catch the error and display it to the end user so they can fix the issues before attempting to save again. 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=35104 --- Comment #111 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194216 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194216&action=edit Bug 35104: Add unit tests for exception on store We add a new set of unit tests for the exception we introduce here, and we also update the existing tests to directly manipulate data in the database rather then using AddBiblio which will now fail due to the introduced exception on attempting to add bad data. 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=35104 --- Comment #113 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194218 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194218&action=edit Bug 35104: Automated Schema Update Patch from commit 0f43be8 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 --- Comment #115 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194220 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194220&action=edit Bug 35104: Add Koha::Biblio::Metadata::Error(s) object classes Introduce Koha::Biblio::Metadata::Error and Koha::Biblio::Metadata::Errors as the Koha object layer over the new biblio_metadata_errors table. 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=35104 --- Comment #117 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194222 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194222&action=edit Bug 35104: Strip non-XML characters gracefully and notify user on save Wire the new Metadata::store graceful stripping through every path that saves a bibliographic record: * C4::Biblio::ModBiblioMarc collects stripping warnings via an optional warnings arrayref and pushes a 'nonxml_stripped' entry (using the new stripped_on_last_store() flag so pre-existing errors are not mistaken for a fresh stripping event). TransformHtmlToMarc no longer pre-empts the store-level check by stripping silently on its own. * cataloguing/addbiblio.pl handles the NONXML_STRIPPED warning: when stripping occurred the record is shown back in the editor together with a prominent "Record saved with modifications" notice so the cataloguer can review the result before continuing. * cataloguing/merge.pl propagates the same pattern for merge operations. * Koha::Import::Record::process() catches the Metadata::Invalid exception that is still thrown for truly unrecoverable MARCXML. * misc/migration_tools/bulkmarcimport.pl logs a warning when stripping occurs during a bulk import. 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=35104 --- Comment #119 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194224 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194224&action=edit Bug 35104: Show nonxml_stripped errors in edit form with quick-links to fields When a biblio being edited has existing nonxml_stripped error rows in biblio_metadata_errors, display them in a 'Data quality warnings' alert banner above the MARC editor. Each error entry includes a 'Go to field' quick-link that switches to the correct tab and scrolls to the first occurrence of the affected subfield so the cataloguer can inspect and correct the data. 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=35104 --- Comment #121 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194226 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194226&action=edit Bug 35104: Update and extend tests to reflect graceful non-XML stripping * t/db_dependent/Koha/Biblio/Metadata.t: replace tests that expected an exception on bad MARCXML with tests that verify graceful stripping behaviour, per-field error recording, context snippet format, the stripped_on_last_store flag, and preservation of existing error rows on a clean re-save. * t/db_dependent/Biblio.t: remove tests that relied on the exception path and update assertions to match the graceful-stripping model. * t/db_dependent/Exporter/Record.t: bypass store() validation for a test record that intentionally contains bad data so the export-path tests remain independent of the stripping logic. 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=35104 --- Comment #122 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 194227 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=194227&action=edit Rogue record to trigger errors -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #124 from David Nind <david@davidnind.com> --- Some initial testing. Maybe we should ask some cataloguers for same sample records that should cause issues? First attempted test -------------------- I attempted to test, but getting this error in several catalog areas in the staff interface after importing the sample record: The method Koha::Biblio::Metadata->metadata_errors is not covered by tests! Trace begun at /kohadevbox/koha/Koha/Object.pm line 1042 Areas include: - Accessing a record from search results - Trying to directly access a record - View and edit actions for manage stated MARC records This only happens after importing the sample record. I did a reset_all, then updatedatabase, dbic, yarn build, and a restart_all, and didn't get this error. Imported record --------------- Viewing the imported record in the staff interface: Record was automatically modified Non-XML characters were stripped from this record when it was last saved. Please review and re-save the record to confirm the data is correct. 336$a: invalid char value 8 (U+0008) at position 12 t $xtac arr␈\ $btxt V $2rdac ontent ^ Resolved error and updated other MARC issues, can view and edit fine. Testing notes ------------- Testing notes so far (using KTD): 1. Apply the patch 2. Run the database update: updatedatabase 3. Run DBIC: dbic (not sure if this is required) 4. Test importing the sample record using Cataloging > Stage record for import and Manage staged record 5. Run tests prove t/db_dependent/Koha/Biblio/Metadata.t t/db_dependent/Biblio.t t/db_dependent/Exporter/Record.t 6. Bonus tests: - Merge a record - Import record from the command line - Z39.50 import -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #126 from Lisette Scheer <lisette@bywatersolutions.com> --- I didn't have the same problem as David. David, did you restart_all? Everything worked fine for me except the qa tool: [SKIP] Koha/Biblio/Metadata/Error.pm SKIP pod_coverage No POD exists [FAIL] t/db_dependent/Koha/Biblio/Metadata.t FAIL forbidden_patterns forbidden pattern: Use https:// instead of http:// for URLs (line 58) [FAIL] t/db_dependent/api/v1/biblios_metadata_errors.t FAIL file_permissions File must have the exec flag -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194213|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194215|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194217|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194219|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194221|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194223|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=35104 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #194225|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=35104 --- Comment #128 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 195433 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195433&action=edit Bug 35104: Throw exception on store of invalid marcxml This patch adds an exception Koha::Biblio::Metadata->store to prevent erroneous characters in marcxml being stored into the database. We also remove the cleanup of such characters from TransformHtmlToMarc as introduced by bug 34549. This should result in there being no way, outside of direct database manipulation, of getting forbidden characters into our marcxml store. We'll need to work through the codebase to catch the exception thrown to give the end user the oportunities to act on the error and fix the data at source. 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=35104 --- Comment #130 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 195435 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195435&action=edit Bug 35104: Catch the failure on merging records We now catch the error and display it to the end user so they can fix the issues before attempting to merge again. 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=35104 --- Comment #132 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 195437 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195437&action=edit Bug 35104: Add biblio_metadata_errors table The preceding signed-off commits (69a6b1c09f7..bfb14bb949e) established an approach where Metadata::store throws a Koha::Exceptions::Metadata::Invalid exception when non-XML characters are encountered, requiring every caller to catch and handle the exception explicitly. Building on that foundation, we change direction here: instead of throwing, Metadata::store will strip non-XML characters gracefully on save and persist structured error details in a new biblio_metadata_errors table. This will give cataloguers a clear view of which fields were affected, with context snippets pointing to the exact character positions, and allows them to review and confirm or correct the data before marking the errors as resolved. The new biblio_metadata_errors table supports multiple per-field error records per metadata row. 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=35104 --- Comment #134 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 195439 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195439&action=edit Bug 35104: Add koha_object_class to BiblioMetadataError schema result Map the new BiblioMetadataError DBIx::Class result to its Koha object classes so that Koha::Biblio::Metadata->metadata_errors returns a proper Koha::Biblio::Metadata::Errors resultset. Sponsored-by: OpenFifth -- You are receiving this mail because: You are watching all bug changes.