Bug 8454 - Holds to Pull : Show pull list on load. Otherwise, it looks like the pull list is empty.
Summary: Holds to Pull : Show pull list on load. Otherwise, it looks like the pull lis...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 18079
  Show dependency treegraph
 
Reported: 2012-07-17 01:26 UTC by David Cook
Modified: 2017-02-08 08:49 UTC (History)
3 users (show)

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


Attachments
Bug 8454 - Holds to Pull : Show pull list on load. Otherwise, it looks like the pull list is empty. (1.27 KB, patch)
2012-07-17 01:35 UTC, David Cook
Details | Diff | Splinter Review
Bug 8454 - Holds to Pull : Show pull list on load. Otherwise, it looks like the pull list is empty. (1.29 KB, patch)
2012-08-09 14:17 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8454 - Show pull list on load. Otherwise, it looks like the pull list is empty - Amended (1.46 KB, patch)
2012-09-12 21:33 UTC, David Cook
Details | Diff | Splinter Review
Bug 8454 - Holds to Pull : Show pull list on load. Otherwise, it looks like the pull list is empty. (1.47 KB, patch)
2012-09-13 07:47 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 David Cook 2012-07-17 01:26:04 UTC
Currently, when you click "Holds to pull" from the Circulation menu, the only thing that you see is a white screen and a side navigation bar that says "Refine Results".

Since there appear to be no results to refine, the holds to pull list appears completely empty. Staff have to know to click "Submit" on the "Refine Results" form to get anything.

I propose that we have the "run_report" flag set to ON for the initial load. The report uses the default 2 days mentioned in the "Refine Results" form, so there is continuity across the board.
Comment 1 David Cook 2012-07-17 01:35:45 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2012-08-09 14:17:11 UTC
Patch tested with a sandbox, by Delaye Stephane <stephane.delaye@biblibre.com>
Comment 3 Paul Poulain 2012-08-09 14:17:28 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-08-10 13:29:20 UTC
QA Comments:

Your patch is the same as
  if ( 1 ){}

I think a better way to do this is : 
  my $run_report = ( not defined $input->param('run_report') ) ? 1 : $input->param('run_report');

If someone wants to load this page without running the report it is possible.
Comment 5 David Cook 2012-08-30 03:21:53 UTC
I figured the "if ( 1 ){}" approach would be the easiest, as it meant the fewest changes to the code, but I'm certainly up for alternative strategies :).

I'm not sure that I understand your conditional here though. Doesn't that say that $run_report will be 1, if $input->param('run_report') isn't set/defined? Otherwise, if it is defined...then it will just be the param that's passed, which amounts to 1 anyway? 

In either case, it seems inevitable that the page will load the report upon page load, which I'm proposing is a good idea. 

However, in that event, how would it be possible for someone to load this page without running the report, as you suggest? 

(In reply to comment #4)
> QA Comments:
> 
> Your patch is the same as
>   if ( 1 ){}
> 
> I think a better way to do this is : 
>   my $run_report = ( not defined $input->param('run_report') ) ? 1 :
> $input->param('run_report');
> 
> If someone wants to load this page without running the report it is possible.
Comment 6 Jonathan Druart 2012-08-30 07:50:44 UTC
(In reply to comment #5)
> I figured the "if ( 1 ){}" approach would be the easiest, as it meant the
> fewest changes to the code, but I'm certainly up for alternative strategies
> :).
> 
> I'm not sure that I understand your conditional here though. Doesn't that
> say that $run_report will be 1, if $input->param('run_report') isn't
> set/defined? Otherwise, if it is defined...then it will just be the param
> that's passed, which amounts to 1 anyway? 
> 
> In either case, it seems inevitable that the page will load the report upon
> page load, which I'm proposing is a good idea. 
> 
> However, in that event, how would it be possible for someone to load this
> page without running the report, as you suggest? 

Hi David,
Your patch does not allow to open the page without launching the report.
For me it is not logical to have a cgi param (run_report) and to not check it. And "the fewest changes to the code" is not a good reason for me if it is not consistent :)

If, on the master code, you replace 
   my $run_report=$input->param('run_report');
with
   my $run_report = ( not defined $input->param('run_report') ) ? 1 : $input->param('run_report');

the page can be displayed without the report if you call it with run_report=0. In all others cases, the report will be displayed (even if the param is not passed). I think it is a good behaviour.
Comment 7 David Cook 2012-09-12 21:33:22 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2012-09-13 07:47:56 UTC
Created attachment 12182 [details] [review]
Bug 8454 - Holds to Pull : Show pull list on load. Otherwise, it looks like the pull list is empty.

Currently, when you click "Holds to pull" from the Circulation menu, the only thing that you see is a white screen and a side navigation bar that says "Refine Results".

Since there appear to be no results to refine, the holds to pull list appears completely empty. Staff have to know to click "Submit" on the "Refine Results" form to get anything.

I propose that we have the "run_report" flag set to ON for the initial load. The report uses the default 2 days mentioned in the "Refine Results" form, so there is continuity across the board.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 9 Paul Poulain 2012-10-03 13:56:52 UTC
QA comment:
 * very tiny patch, no side effect risk, just a default value added
 * koha-qa.pl OK
Comment 10 Paul Poulain 2012-10-03 13:58:22 UTC
Patch pushed to master