additem.tt template "humanizes" every error it knows about, and ignores all others. This problem is noted in additem.pl itself (https://gitlab.com/koha-community/Koha/-/blob/166604f9f3938307de6ea5f2530b61ba1cb3b492/cataloguing/additem.pl#L638). The way it currently works is that any unhandled failure when adding/editing items fails quietly on the backend, but returns a 200 and shows a successful page to the user (a background job handling the same, but batched edit would fail visibly). The code and the template should be changed so that any caught exception leaves *some* trace in the interface and clearly indicates that an action has failed. (Patch incoming).
Created attachment 191940 [details] [review] Bug 41696: Generalize additem.pl/tt's error handling This prevents us from losing track of errors and failing quietly.
Created attachment 192092 [details] [review] Bug 41696: Improve additem.pl/tt's error handling This prevents us from losing track of errors and failing quietly. Also gets rid of hardcoded error messages for Koha::Object exceptions, defering to full_message which provides more details. Sponsored-by: Universitätsbibliothek der Andrássy Universität
1. Missing filter [FAIL] koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt FAIL filters missing_filter at line 115 ( <strong>Error</strong>: [% error %].) 2. You need to keep the HTML tag around the error, or the translator script will add TT tag to the translations and it will be messy. 3. t/db_dependent/Koha/ApiKey.t and t/db_dependent/Koha/Object.t are failing, but I am expecting more 4. I've added a test to ApiKey and the result is a bit unexpected: # Failed test at t/db_dependent/Koha/ApiKey.t line 118. # got: 'Invalid parameter passed, patron_id= does not exist' # expected: 'Duplicate ID for key 'patron_id''
(In reply to Jonathan Druart from comment #3) > 4. I've added a test to ApiKey and the result is a bit unexpected: > > # Failed test at t/db_dependent/Koha/ApiKey.t line 118. > # got: 'Invalid parameter passed, patron_id= does not exist' > # expected: 'Duplicate ID for key 'patron_id'' Ha, it's actually the message added by your patch that is not accurate. 81 $msg = sprintf( "Invalid parameter passed, %s=%s does not exist", $self->broken_fk, $self->value ); Note that the test also raises a warning on this line: `Use of uninitialized value in sprintf at /kohadevbox/koha/Koha/Exceptions/Object.pm line 81.`
(In reply to Jonathan Druart from comment #4) > (In reply to Jonathan Druart from comment #3) > > 4. I've added a test to ApiKey and the result is a bit unexpected: > > > > # Failed test at t/db_dependent/Koha/ApiKey.t line 118. > > # got: 'Invalid parameter passed, patron_id= does not exist' > > # expected: 'Duplicate ID for key 'patron_id'' > > Ha, it's actually the message added by your patch that is not accurate. > > 81 $msg = sprintf( "Invalid parameter passed, %s=%s does not > exist", $self->broken_fk, $self->value ); > > Note that the test also raises a warning on this line: > `Use of uninitialized value in sprintf at > /kohadevbox/koha/Koha/Exceptions/Object.pm line 81.` Sorry, wrong exception, I was looking at the other one and mixed both!
Created attachment 192317 [details] [review] Bug 41696: Add a test Patch from commit ede59c5
Adjusted test, still failing # Failed test at t/db_dependent/Koha/ApiKey.t line 118. # got: 'Invalid parameter passed, patron_id= does not exist' # expected: 'Invalid parameter passed, patron_id=162 does not exist'
I do think the change is correct, and that we should have more description in our exceptions (i.e. more with full_message). However the patch is still not ready for inclusion.