Bug 34014 - There is no way to fix records with broken MARCXML
Summary: There is no way to fix records with broken MARCXML
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P3 major with 31 votes (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 23846 29697 35099
Blocks: 35119 35181
  Show dependency treegraph
 
Reported: 2023-06-14 19:05 UTC by Nick Clemens (kidclamp)
Modified: 2023-11-30 17:19 UTC (History)
18 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.05


Attachments
Bug 34014: Allow staff to edit degraded records (3.91 KB, patch)
2023-06-14 19:22 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34014: Allow staff to edit degraded records (3.62 KB, patch)
2023-10-17 18:14 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34014: Add support for advanced cataloging editor (2.76 KB, patch)
2023-10-17 18:14 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34014: Unit test (2.04 KB, patch)
2023-10-17 18:14 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34014: (follow-up) Tidy (2.94 KB, patch)
2023-10-17 18:14 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34014: (follow-up) No need to parse old record before updating (1.03 KB, patch)
2023-10-17 19:17 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34014: Allow staff to edit degraded records (3.80 KB, patch)
2023-10-19 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34014: Add support for advanced cataloging editor (2.94 KB, patch)
2023-10-19 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34014: Unit test (2.21 KB, patch)
2023-10-19 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34014: (follow-up) Tidy (3.12 KB, patch)
2023-10-19 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34014: (follow-up) No need to parse old record before updating (1.21 KB, patch)
2023-10-19 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-06-14 19:05:04 UTC
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.
Comment 1 Nick Clemens (kidclamp) 2023-06-14 19:22:49 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens (kidclamp) 2023-06-14 19:24:55 UTC
This largely works - the logs don't capture the error though.

Should we have a:
$biblio->metadata->record({ attempt_cleaning => 1 });

or something?
Comment 3 Phil Ringnalda 2023-06-14 22:47:24 UTC
Works unless you EnableAdvancedCatalogingEditor, which just hangs at "Loading, please wait..." without having caught the 500 from /cgi-bin/koha/svc/bib/369
Comment 4 David Cook 2023-06-14 23:16:07 UTC
(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.
Comment 5 David Cook 2023-06-14 23:16:29 UTC
I like where this is going though. It is annoying when I have to go into the database to fix broken records...
Comment 6 Liz Rea 2023-07-28 13:44:26 UTC
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.
Comment 7 Enica Davis 2023-07-28 19:34:02 UTC
+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.
Comment 8 vstever 2023-07-28 19:41:44 UTC
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.
Comment 9 Jonathan Druart 2023-08-01 06:57:47 UTC
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.
Comment 10 Nick Clemens (kidclamp) 2023-08-01 16:17:46 UTC
(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
Comment 11 Jonathan Druart 2023-08-01 18:17:09 UTC
(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.
Comment 12 David Cook 2023-08-15 04:14:36 UTC
Note that bib record deletions are also impacted by this issue and this patch.
Comment 13 David Cook 2023-08-15 04:18:22 UTC
(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...
Comment 14 David Cook 2023-08-17 04:36:02 UTC
Hmm after reviewing this one again... perhaps a combination of this and bug 34549 make sense...
Comment 15 David Cook 2023-08-17 04:40:45 UTC
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...
Comment 16 David Cook 2023-09-26 05:04:36 UTC
So where are we at with this one? "Needs Signoff"? "In discussion"?
Comment 17 Nick Clemens (kidclamp) 2023-10-17 18:14:33 UTC
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!
Comment 18 Nick Clemens (kidclamp) 2023-10-17 18:14:36 UTC
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
Comment 19 Nick Clemens (kidclamp) 2023-10-17 18:14:39 UTC
Created attachment 157276 [details] [review]
Bug 34014: Unit test
Comment 20 Nick Clemens (kidclamp) 2023-10-17 18:14:42 UTC
Created attachment 157277 [details] [review]
Bug 34014: (follow-up) Tidy
Comment 21 Nick Clemens (kidclamp) 2023-10-17 18:15:24 UTC
(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
Comment 22 Nick Clemens (kidclamp) 2023-10-17 19:17:49 UTC
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
Comment 23 Kelly McElligott 2023-10-17 19:28:56 UTC
Tested both methods  - in the basic editor and the advanced editor, all is working as expected.
Comment 24 Martin Renvoize 2023-10-19 09:33:05 UTC
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>
Comment 25 Martin Renvoize 2023-10-19 09:33:09 UTC
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>
Comment 26 Martin Renvoize 2023-10-19 09:33:12 UTC
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>
Comment 27 Martin Renvoize 2023-10-19 09:33:16 UTC
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>
Comment 28 Martin Renvoize 2023-10-19 09:33:19 UTC
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>
Comment 29 Martin Renvoize 2023-10-19 09:34:12 UTC
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
Comment 30 Tomás Cohen Arazi 2023-10-20 14:03:55 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 31 Laura O'Neil 2023-10-23 12:37:43 UTC
Could this please be backported to 23.05 and 22.11?
Comment 32 Fridolin Somers 2023-10-25 22:02:34 UTC
"It is alive!", dead records will be reborn Frankenstein's style :D
Great fix, thanks all.
Comment 33 Fridolin Somers 2023-10-25 22:02:53 UTC
Pushed to 23.05.x for 23.05.05
Comment 34 Matt Blenkinsop 2023-11-13 14:07:54 UTC
Dependency missing from 23.05 and 22.11 - not backporting to 22.11.x
Comment 35 Aleisha Amohia 2023-11-23 01:57:29 UTC
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.
Comment 36 David Cook 2023-11-23 02:02:35 UTC
(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
Comment 37 Aleisha Amohia 2023-11-23 02:57:44 UTC
(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 :)
Comment 38 Fridolin Somers 2023-11-23 07:21:24 UTC
Ouf ;)