Bug 41696 - Error handling in additem.pl is too specific and insufficient
Summary: Error handling in additem.pl is too specific and insufficient
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2026-01-23 14:01 UTC by HKS3 Tadeusz Sośnierz
Modified: 2026-02-02 13:40 UTC (History)
2 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 41696: Generalize additem.pl/tt's error handling (5.04 KB, patch)
2026-01-23 14:08 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Bug 41696: Improve additem.pl/tt's error handling (7.25 KB, patch)
2026-01-27 10:02 UTC, HKS3 Tadeusz Sośnierz
Details | Diff | Splinter Review
Bug 41696: Add a test (1.03 KB, patch)
2026-02-02 13:37 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description HKS3 Tadeusz Sośnierz 2026-01-23 14:01:42 UTC
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).
Comment 1 HKS3 Tadeusz Sośnierz 2026-01-23 14:08:13 UTC
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.
Comment 2 HKS3 Tadeusz Sośnierz 2026-01-27 10:02:52 UTC
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
Comment 3 Jonathan Druart 2026-02-02 13:31:52 UTC
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''
Comment 4 Jonathan Druart 2026-02-02 13:34:11 UTC
(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.`
Comment 5 Jonathan Druart 2026-02-02 13:35:45 UTC
(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!
Comment 6 Jonathan Druart 2026-02-02 13:37:43 UTC
Created attachment 192317 [details] [review]
Bug 41696: Add a test

Patch from commit ede59c5
Comment 7 Jonathan Druart 2026-02-02 13:39:01 UTC
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'
Comment 8 Jonathan Druart 2026-02-02 13:40:15 UTC
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.