See bug 29697 and bug 23846 Koha currently catches an exception if the view is degraded in order to allow the staff to know there is a problem with the record. However, if the staff user tries to edit the record, we simply throw the exception and there is no way to recover. In other places we use StripNonXmlCharacters to get past encoding issues. We should do the same in the record editor.
Created attachment 152360 [details] [review] Bug 34014: Allow staff to edit degraded records This patch adds an eval to the call for the record form metadata If there is an error we catch it, display it, and attempt to strip bad characters from the record To test: 1 - In koha testing docker attempt to view record 369 in the staff interface 2 - You see a warning about degraded view 3 - Attempt to edit the record 4 - Internal server error 5 - Apply patch, restart all 6 - Reload the editor 7 - Record successfully loads! 8 - You have a warning about encoding issues! 9 - Save the record 10 - No more warning! 11 - Edit again, no more warning!
This largely works - the logs don't capture the error though. Should we have a: $biblio->metadata->record({ attempt_cleaning => 1 }); or something?
Works unless you EnableAdvancedCatalogingEditor, which just hangs at "Loading, please wait..." without having caught the 500 from /cgi-bin/koha/svc/bib/369
(In reply to Nick Clemens from comment #2) > This largely works - the logs don't capture the error though. You could just add a warn when catching the error? > Should we have a: > $biblio->metadata->record({ attempt_cleaning => 1 }); > > or something? Not sure I follow, although I'm opposed to things like "maybe_" and "attempt_" as they are harder to test and imply that the function being performed can have different behaviours besides just doing that one function.
I like where this is going though. It is annoying when I have to go into the database to fix broken records...
This is a significant problem - we get at least a few tickets a day from libraries with encoding issues on imported and legacy records. These encoding issues also cause OAI harvesting to fail. Libraries really need to be able to fix them.
+1 to Liz Rea. This also stops processes like authority linking and, at times, even an import, if/when the matching record is still viewable in staff.
Our catalogers are encountering this issue repetitively. We have no recourse except a ticket to ByWater to fix the specific records. Then it happens again on different records. We would greatly appreciate a fix.
Nick, I would remove the changes in ModBiblio, and keep the explicit update in the controller. Also I think we should have a selenium test. We already have 'Bibliographic record detail page must not explode even with invalid metadata' in regressions.t.
(In reply to Jonathan Druart from comment #9) > Nick, I would remove the changes in ModBiblio, and keep the explicit update > in the controller. > > Also I think we should have a selenium test. We already have 'Bibliographic > record detail page must not explode even with invalid metadata' in > regressions.t. The ModBiblio changes are needed to prevent an explosion in logging - without them we die on recording the modification
(In reply to Nick Clemens from comment #10) > (In reply to Jonathan Druart from comment #9) > > Nick, I would remove the changes in ModBiblio, and keep the explicit update > > in the controller. > > > > Also I think we should have a selenium test. We already have 'Bibliographic > > record detail page must not explode even with invalid metadata' in > > regressions.t. > > The ModBiblio changes are needed to prevent an explosion in logging - > without them we die on recording the modification Got it, agreed. Thanks for your quick answer.
Note that bib record deletions are also impacted by this issue and this patch.
(In reply to Nick Clemens from comment #10) > The ModBiblio changes are needed to prevent an explosion in logging - > without them we die on recording the modification In the context of touch_all_biblios.pl, I suppose we'll still bust on the line 'my $record = $biblio->metadata->record;' which is probably good for now...
Hmm after reviewing this one again... perhaps a combination of this and bug 34549 make sense...
Bug 34549 came about because I had a library that was able to import and save MARC data which couldn't be parsed as MARCXML. We could warn or fail earlier. In their case, they had the encoding wrong on their Z39.50 server config. As soon as I fixed that, the problem went away. So perhaps we should put more thought into treating the root cause and not just the symptom...
So where are we at with this one? "Needs Signoff"? "In discussion"?
Created attachment 157274 [details] [review] Bug 34014: Allow staff to edit degraded records This patch adds an eval to the call for the record form metadata If there is an error we catch it, display it, and attempt to strip bad characters from the record To test: 1 - In koha testing docker attempt to view record 369 in the staff interface 2 - You see a warning about degraded view 3 - Attempt to edit the record 4 - Internal server error 5 - Apply patch, restart all 6 - Reload the editor 7 - Record successfully loads! 8 - You have a warning about encoding issues! 9 - Save the record 10 - No more warning! 11 - Edit again, no more warning!
Created attachment 157275 [details] [review] Bug 34014: Add support for advanced cataloging editor To test: 1 - Enable the advanced cataloging editor 2 - Edit record 369 in the advanced cataloging editor 3 - On load, get a notice that there were issues and to check logs 4 - Save record 5 - Close and reopen in editor 6 - Confirm warning is gone
Created attachment 157276 [details] [review] Bug 34014: Unit test
Created attachment 157277 [details] [review] Bug 34014: (follow-up) Tidy
(In reply to David Cook from comment #16) > So where are we at with this one? "Needs Signoff"? "In discussion"? I updated to sue the code from bug 33270 - needs signoff
Created attachment 157280 [details] [review] Bug 34014: (follow-up) No need to parse old record before updating If the old record doesn't parse correctly it will cause a failure - the check is just to ensure we are editing an existing biblio, just getting the metadata object should confirm it exists
Tested both methods - in the basic editor and the advanced editor, all is working as expected.
Created attachment 157401 [details] [review] Bug 34014: Allow staff to edit degraded records This patch adds an eval to the call for the record form metadata If there is an error we catch it, display it, and attempt to strip bad characters from the record To test: 1 - In koha testing docker attempt to view record 369 in the staff interface 2 - You see a warning about degraded view 3 - Attempt to edit the record 4 - Internal server error 5 - Apply patch, restart all 6 - Reload the editor 7 - Record successfully loads! 8 - You have a warning about encoding issues! 9 - Save the record 10 - No more warning! 11 - Edit again, no more warning! Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157402 [details] [review] Bug 34014: Add support for advanced cataloging editor To test: 1 - Enable the advanced cataloging editor 2 - Edit record 369 in the advanced cataloging editor 3 - On load, get a notice that there were issues and to check logs 4 - Save record 5 - Close and reopen in editor 6 - Confirm warning is gone Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157403 [details] [review] Bug 34014: Unit test Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157404 [details] [review] Bug 34014: (follow-up) Tidy Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 157405 [details] [review] Bug 34014: (follow-up) No need to parse old record before updating If the old record doesn't parse correctly it will cause a failure - the check is just to ensure we are editing an existing biblio, just getting the metadata object should confirm it exists Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This is a great improvement, all working as designed and I'm really happy to see this. QA scripts are happy and tests are all passing. Passing QA
Pushed to master for 23.11. Nice work everyone, thanks!
Could this please be backported to 23.05 and 22.11?
"It is alive!", dead records will be reborn Frankenstein's style :D Great fix, thanks all.
Pushed to 23.05.x for 23.05.05
Dependency missing from 23.05 and 22.11 - not backporting to 22.11.x
This has caused errors in 23.05.05 in the Advanced editor - we're seeing records throw an 'invalid metadata' error that were fine to open in the editor previously.
(In reply to Aleisha Amohia from comment #35) > This has caused errors in 23.05.05 in the Advanced editor - we're seeing > records throw an 'invalid metadata' error that were fine to open in the > editor previously. Have you tried 23.05.06? I think bug 35181 might have fixed that
(In reply to David Cook from comment #36) > (In reply to Aleisha Amohia from comment #35) > > This has caused errors in 23.05.05 in the Advanced editor - we're seeing > > records throw an 'invalid metadata' error that were fine to open in the > > editor previously. > > Have you tried 23.05.06? I think bug 35181 might have fixed that You're totally right, I'm really failing to search for bugs recently! That's the solve, thank you :)
Ouf ;)