Bug 20661 - Implement blocking errors for circulation scripts
Summary: Implement blocking errors for circulation scripts
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 15184 20351 21125 21221
  Show dependency treegraph
 
Reported: 2018-04-25 20:08 UTC by Jonathan Druart
Modified: 2019-10-14 19:56 UTC (History)
5 users (show)

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


Attachments
Bug 20661: Shortcut scripts if a blocking error appeared (6.57 KB, patch)
2018-04-25 20:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20661: Shortcut circ scripts if a blocking error appeared (6.70 KB, patch)
2018-05-09 16:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20661: Shortcut circ scripts if a blocking error appeared (6.82 KB, patch)
2018-07-27 06:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20661: (QA follow-up) Add POD for output_and_exit (1.30 KB, patch)
2018-07-27 06:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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