Created attachment 74858 [details] [review] Bug 20661: Shortcut scripts if a blocking error appeared
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.
I think it's quite complete for circ/* (and already done for members/*).
Hi Jonathan, can you explain this a bit more and add a test plan?
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 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> > + › <a href="/cgi-bin/koha/circ/circulation-home.pl">Circulation</a> > + [% UNLESS blocking_error %]› Circulation statistics for [% title |html %][% END %] Why add this unless, when line 26 still displays the title?
Created attachment 75209 [details] [review] Bug 20661: Shortcut circ scripts if a blocking error appeared
(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> > > + › <a href="/cgi-bin/koha/circ/circulation-home.pl">Circulation</a> > > + [% UNLESS blocking_error %]› 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.
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.
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>
Combining SO=QA in same run. Trivial changes.
Awesome work all! Pushed to master for 18.11
Enhancement, will not be backported to 18.05.x series.
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
I prefer not push to 17.11.x