Bug 11651

The text plan is OK until the 3 step.
Holds queue is not updating by running $KOHAPATH/misc/cronjobs/holds/build_holds_queue.pl
We can see the 2 holds in patron's information page but not in the hold queue (view_holdsqueue.pl) 

But we see the 2 new buttons but not the items ("No items found").

Ok until step 7.
When we try to print, a new window is open (strange ?)

with '404 error' at this URL : 

http://pro.test2.biblibre.com/cgi-bin/koha/circ/hold-pull-print.pl?reserve_id=1424&reserve_id=1425

Patch tested with a sandbox, by Aurelie Fichot <aurelie.fichot@iepg.fr>

It works !

8) The Print botton open a new window just for printing the holds, with all biblio and item info.
9) click show printed to display printed holds : OK
10) click hide printed, now holds queue shuld be empty : OK
11) disable printSlipFromHoldsQueue go to holds queue and check if holds
are again visible : OK

Comment on attachment 26262 [details] [review]
Bug 11651: Add possibility to print holds from holds queue

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

Please take a look at the following.
Marked as Failed QA.

::: C4/HoldsQueue.pm
@@ +47,5 @@
>          &TransportCostMatrix
>          &UpdateTransportCostMatrix
> +
> +        &MarkHoldPrinted
> +        &GetItemNumberFromTmpHold

New unit tests should be provided for these 2 new subroutines.

@@ +136,5 @@
> +    if (C4::Context->preference('printSlipFromHoldsQueue')){
> +        $query .= $branchlimit ? " AND" : " WHERE";
> +        $query .= " tmp_holdsqueue.print_status = ?";
> +        push @bind_params, $printstatus;
> +    }

New unit tests should be provided for this subroutine change.

::: C4/Reserves.pm
@@ +2255,5 @@
> +            'biblio'      => $reserve->{biblionumber},
> +            'items'       => $itemnumber,
> +        },
> +    );
> +}

At least one unit test should be provided for this new subroutine.

::: circ/hold-pull-print.pl
@@ +12,5 @@
> +# A PARTICULAR PURPOSE.  See the GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License along
> +# with Koha; if not, write to the Free Software Foundation, Inc.,
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

License should be GPLv3.

@@ +15,5 @@
> +# with Koha; if not, write to the Free Software Foundation, Inc.,
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +use strict;
> +#use warnings; #FIXME - Bug 2505

replace use strict/warnings with use Modern::Perl.

::: circ/view_holdsqueue.pl
@@ +76,4 @@
>  $template->param(
>       branchloop => GetBranchesLoop(C4::Context->userenv->{'branch'}),
>     itemtypeloop => \@itemtypesloop,
> +   printenable => C4::Context->preference('printSlipFromHoldsQueue'),

Same as previously.

::: installer/data/mysql/updatedatabase.pl
@@ +8071,5 @@
> +    $dbh->do("ALTER TABLE old_reserves ADD print_status BOOLEAN NOT NULL DEFAULT '0'");
> +    $dbh->do("ALTER TABLE tmp_holdsqueue ADD print_status BOOLEAN NOT NULL DEFAULT '0'");
> +    $dbh->do("ALTER TABLE tmp_holdsqueue ADD reserve_id INT ( 11 ) FIRST");
> +    $dbh->do("INSERT INTO systempreferences (variable,value,options,explanation,type) VALUES('printSlipFromHoldsQueue', '0', 'NULL', 'If enabled reserve slips can be printed from Holds Queue screen,', 'YesNo');");
> +    print "Upgrade to $DBversion done \n";

you forgot the SetVersion call here.

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/view_holdsqueue.tt
@@ +44,5 @@
> +        for (var i=0; i < document.hold_print.reserve_id.length; i++) {
> +            document.hold_print.reserve_id[i].checked=false;
> +        }
> +    }
> +}

you can use [un]checkCheckboxes functions provided by jquery.checkboxes.min.js to do that.

@@ +93,2 @@
>      [% IF ( itemsloop ) %]
> +        <form id="hold_print" name="hold_print" method="get" action="/cgi-bin/koha/circ/hold-pull-print.pl" target="_blank">

why do you use target="_blank" here?

@@ +142,5 @@
>          </tr>
>      [% END %]</tbody>
>      </table>
> +        [% IF ( printenable ) %]
> +        </form>

This close tag is in a IF but not the <form>.

(In reply to Jonathan Druart from comment #9)
> Comment on attachment 26262 [details] [review] [review]
> Bug 11651: Add possibility to print holds from holds queue
> 
> Review of attachment 26262 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Please take a look at the following.
> Marked as Failed QA.
> > +        &MarkHoldPrinted
> > +        &GetItemNumberFromTmpHold
> 
> New unit tests should be provided for these 2 new subroutines.
> @@ +136,5 @@
> > +    if (C4::Context->preference('printSlipFromHoldsQueue')){
> > +        $query .= $branchli
> 
> New unit tests should be provided for this subroutine change.
> 
> ::: C4/Reserves.pm
> @@ +2255,5 @@
> > +            'biblio'      => $reserve->{biblionumber},
> > +            'items'       => $itemnumber,
> At least one unit test should be provided for this new subroutine.

Fixed. But please, take a look to this test. I'm new in writing unit tests. So any feedback may be helpful. 

 
> ::: circ/hold-pull-print.pl
> License should be GPLv3.
> > +use strict;
> > +#use warnings; #FIXME - Bug 2505
> 
> replace use strict/warnings with use Modern::Perl.
> 
Fixed.

> ::: circ/view_holdsqueue.pl
> @@ +76,4 @@
> >  $template->param(
> >       branchloop => GetBranchesLoop(C4::Context->userenv->{'branch'}),
> >     itemtypeloop => \@itemtypesloop,
> > +   printenable => C4::Context->preference('printSlipFromHoldsQueue'),
> 
> Same as previously.
I don't get it, what is wrong here? Not unit test I think?

> 
> ::: installer/data/mysql/updatedatabase.pl

> > +    print "Upgrade to $DBversion done \n";
> 
> you forgot the SetVersion call here.

Fixed, also rebased to master.

> you can use [un]checkCheckboxes functions provided by
> jquery.checkboxes.min.js to do that.
Fixed.
> 
> @@ +93,2 @@
> >      [% IF ( itemsloop ) %]
> > +        <form id="hold_print" name="hold_print" method="get" action="/cgi-bin/koha/circ/hold-pull-print.pl" target="_blank">
> 
> why do you use target="_blank" here?
Print slip must be open in new window it is in form to print, same as all printable reports in Koha (Reserve slip/Borrower sumary etc.). Why here? It works, at least for me :)

> @@ +142,5 @@
> >          [% IF ( printenable ) %]
> > +        </form>
> 
> This close tag is in a IF but not the <form>.

Fixed.

Rebased, against master. Also few minor changes.

This isn't working for me. When I select 4 titles in the holds queue to print, the resulting printout says:

Reserve not foundReserve not foundReserve not foundReserve not found

Looks like reserve_id isn't getting passed to the template?

Note that the term "hold" should be used instead of "reserve." We only use "reserve" in the context of Course Reserves.

Looks like that text is coming directly from the .pl file. English strings should be handled in a way which is translatable, preferably by putting them in the template. printslip.tt appears to be capable of handling it already?

Some other suggestions:

- Leave the header cell in the checkbox column empty. It doesn't
  need the word "Check."
- Exclude the checkbox column from being sorted by the table sorter.
- Add a warning to the user if they click the "Print checked holds"
  without checking anything.

(In reply to Owen Leonard from comment #15)
> This isn't working for me. When I select 4 titles in the holds queue to
> print, the resulting printout says:
> 
> Reserve not foundReserve not foundReserve not foundReserve not found
> 
> Looks like reserve_id isn't getting passed to the template?

It says, that "reserve not found", so some id must be passed to script, have you updated database and re-run build_holds_queue.pl after that? 
Printing from holds queue require reserve_id in tmp_holdsqueue, which is populated by build holds queue after patch. If this didn't work let me know. 

> 
> Note that the term "hold" should be used instead of "reserve." We only use
> "reserve" in the context of Course Reserves.
> 
> Looks like that text is coming directly from the .pl file. English strings
> should be handled in a way which is translatable, preferably by putting them
> in the template. printslip.tt appears to be capable of handling it already?

printslip.tt only partial can handle this case, since it print "hold not found", only if slip variable is undef. In case of printing many holds only few may be not found, while other will define slip. So hold not found info would not show.
I'll try to figure out some error handling in print for this case.

> 
> Some other suggestions:
> 
> - Leave the header cell in the checkbox column empty. It doesn't
>   need the word "Check."
> - Exclude the checkbox column from being sorted by the table sorter.
> - Add a warning to the user if they click the "Print checked holds"
>   without checking anything.

Will do. 

Thanks for test!

(In reply to Rafal Kopaczka from comment #16)
> It says, that "reserve not found", so some id must be passed to script, have
> you updated database and re-run build_holds_queue.pl after that? 

I must not have rebuilt the holds queue. Sorry about that. I can confirm now that the selected holds are printed and that they are subsequently hidden from the holds queue report.

Three issues:

1) The layout on this appears to be a bit wonky. I think it needs to use the toolbar markup used in many other places in Koha.

2) The show/hide printed holds button doesn't do what it says. It switches to showing only printed holds or non-printed holds. The language should be "View printed holds" and "View un-printed holds" or some such thing.

3) "Print checked holds" should be "Print selected holds" or maybe just "Print selected"

Created attachment 38079 [details] [review]
Bug 11651: Add possibility to print holds from holds queue

This patch adds possibility to print holds slip, directly from holds
queue page.
Added preference to enable or disable. When enabled in holds queue are 2
new buttons and checkboxes to select holds to print. First button prints
selected holds on RESERVESLIP form. Second is to switch between
display printed or not printed holds. In this mode isn't possible
to display printed and non printed together.

Test plan:
1) Apply patch
2) Update database
3) Make 2 new holds, 1 on specific copy, another on next available
4) Update holds queue by running $KOHAPATH/misc/cronjobs/holds/build_holds_queue.pl
5) Check holds queue there should be 2 new holds from step 3
6) Enable printSlipFromHoldsQueue in preferences-> circulation-> interface
7) Go to holds after submit branch there should be 2 new buttons and
checkboxes in table.
8) Print both holds and check if on slip are all biblio and item info.
9) click show printed to display printed holds
10) click hide printed, now holds queue shuld be empty
11) disable printSlipFromHoldsQueue go to holds queue and check if holds
are again visible.

Signed-off-by: Aurelie Fichot <aurelie.fichot@iepg.fr>

Created attachment 38080 [details] [review]
Bug 11651: (QA Follow-up) Add possibility to print holds from holds queue.

Added unit tests, for new and modified subroutines.
Fixed minor qa suggestions.
Koha-qa.pl - pass.

(In reply to Kyle M Hall from comment #20)
> Three issues:
> 
> 1) The layout on this appears to be a bit wonky. I think it needs to use the
> toolbar markup used in many other places in Koha.

Added toolbar, looks much better :) 

> 2) The show/hide printed holds button doesn't do what it says. It switches
> to showing only printed holds or non-printed holds. The language should be
> "View printed holds" and "View un-printed holds" or some such thing.
> 3) "Print checked holds" should be "Print selected holds" or maybe just
> "Print selected"

Fixed. 
Also rebased to current master.

Applying: Bug 11651: Add possibility to print holds from holds queue
fatal: sha1 information is lacking or useless (C4/HoldsQueue.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 11651: Add possibility to print holds from holds queue