Bug 20661

Summary: Implement blocking errors for circulation scripts
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, katrin.fischer, m.de.rooy, martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20351
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20927
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 15184, 20351, 21125, 21221    
Attachments: Bug 20661: Shortcut scripts if a blocking error appeared
Bug 20661: Shortcut circ scripts if a blocking error appeared
Bug 20661: Shortcut circ scripts if a blocking error appeared
Bug 20661: (QA follow-up) Add POD for output_and_exit

Description Jonathan Druart 2018-04-25 20:08:18 UTC

    
Comment 1 Jonathan Druart 2018-04-25 20:08:57 UTC
Created attachment 74858 [details] [review]
Bug 20661: Shortcut scripts if a blocking error appeared
Comment 2 Jonathan Druart 2018-04-25 20:09:46 UTC
I do not have time right now to implement this, but it seems something we really need and should apply to scripts with mandatory parameters.
Comment 3 Jonathan Druart 2018-04-25 20:10:21 UTC
I think it's quite complete for circ/* (and already done for members/*).
Comment 4 Katrin Fischer 2018-04-29 22:34:20 UTC
Hi Jonathan, can you explain this a bit more and add a test plan?
Comment 5 Jonathan Druart 2018-05-08 17:33:27 UTC
The idea of output_and_exit_if_error (added by bug 18403) is to make sure parameters are valid before executing the script.
If not (old or broken URLs), we shortcut everything coming next to display a generic error ("object does not exist", "you do not have permission to do that", etc.)

This bug report fixes the scripts under circ/*, other bug reports will follow to take care of other modules.

Test plan:
Hit the script under the circ directory with an invalid biblionumber parameter and confirm you get an error instead of the normal view with empty values.

The goal is not to be exhaustive during the first iteration, but at least to fix the most common views.
Comment 6 Mark Tompsett 2018-05-09 00:03:09 UTC
Comment on attachment 74858 [details] [review]
Bug 20661: Shortcut scripts if a blocking error appeared

Review of attachment 74858 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/bookcount.tt
@@ +12,5 @@
>  
> +<div id="breadcrumbs">
> +    <a href="/cgi-bin/koha/mainpage.pl">Home</a>
> +    &rsaquo; <a href="/cgi-bin/koha/circ/circulation-home.pl">Circulation</a>
> +    [% UNLESS blocking_error %]&rsaquo; Circulation statistics for [% title |html %][% END %]

Why add this unless, when line 26 still displays the title?
Comment 7 Jonathan Druart 2018-05-09 16:01:37 UTC
Created attachment 75209 [details] [review]
Bug 20661: Shortcut circ scripts if a blocking error appeared
Comment 8 Jonathan Druart 2018-05-09 16:02:17 UTC
(In reply to M. Tompsett from comment #6)
> Comment on attachment 74858 [details] [review] [review]
> Bug 20661: Shortcut scripts if a blocking error appeared
> 
> Review of attachment 74858 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/bookcount.tt
> @@ +12,5 @@
> >  
> > +<div id="breadcrumbs">
> > +    <a href="/cgi-bin/koha/mainpage.pl">Home</a>
> > +    &rsaquo; <a href="/cgi-bin/koha/circ/circulation-home.pl">Circulation</a>
> > +    [% UNLESS blocking_error %]&rsaquo; Circulation statistics for [% title |html %][% END %]
> 
> Why add this unless, when line 26 still displays the title?

It did not, title was an empty string.
But it indeed makes sense to swap these 2 lines.
Comment 9 Marcel de Rooy 2018-07-27 06:49:00 UTC
Created attachment 77296 [details] [review]
Bug 20661: Shortcut circ scripts if a blocking error appeared

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
No test plan found ;)
But tested bookcount and request-article.
Comment 10 Marcel de Rooy 2018-07-27 06:49:04 UTC
Created attachment 77297 [details] [review]
Bug 20661: (QA follow-up) Add POD for output_and_exit

Resolve:
 FAIL   C4/Output.pm
   FAIL   pod
                *** ERROR: Apparent command =cut not preceded by blank line  in file C4/Output.pm
   FAIL   pod coverage
                POD is missing for output_and_exit

Test plan:
Run QA tools again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2018-07-27 06:50:18 UTC
Combining SO=QA in same run. Trivial changes.
Comment 12 Nick Clemens (kidclamp) 2018-08-10 12:56:46 UTC
Awesome work all!

Pushed to master for 18.11
Comment 13 Martin Renvoize 2018-08-10 14:00:15 UTC
Enhancement, will not be backported to 18.05.x series.
Comment 14 Martin Renvoize 2018-08-13 08:24:49 UTC
After some discussion, I've decided to treat this as a bugfix for 18.05.x branch rather than an enhancement.

Pushed to 18.05.x for 18.05.03
Comment 15 Fridolin Somers 2018-09-21 12:36:01 UTC
I prefer not push to 17.11.x