Bug 11651 - Add possibility to print holds from holds queue
Summary: Add possibility to print holds from holds queue
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Rafal Kopaczka
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-31 14:52 UTC by Rafal Kopaczka
Modified: 2016-12-01 21:58 UTC (History)
9 users (show)

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


Attachments
Bug-11651-Add-possibility-to-print-holds-from-holds-.patch (18.09 KB, patch)
2014-01-31 14:56 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
Bug-11651-Add-possibility-to-print-holds-from-holds (18.13 KB, patch)
2014-03-13 10:07 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
0001-Bug-11651-Add-possibility-to-print-holds-from-holds (20.70 KB, patch)
2014-03-13 10:47 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
Bug 11651: Add possibility to print holds from holds queue (20.71 KB, patch)
2014-03-13 11:41 UTC, Biblibre Sandboxes
Details | Diff | Splinter 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. (11.46 KB, patch)
2014-05-30 14:11 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
Bug 11651: Add possibility to print holds from holds queue (20.98 KB, patch)
2014-10-10 09:57 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
Bug 11651: (QA Follow-up) Add possibility to print holds from holds queue. (15.36 KB, patch)
2014-10-10 09:57 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
Bug 11651: Add possibility to print holds from holds queue (20.80 KB, patch)
2014-10-24 16:27 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
Bug 11651: (QA Follow-up) Add possibility to print holds from holds queue. (16.53 KB, patch)
2014-10-24 16:28 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
Bug 11651: Add possibility to print holds from holds queue (20.94 KB, patch)
2015-04-17 14:51 UTC, Rafal Kopaczka
Details | Diff | Splinter Review
Bug 11651: (QA Follow-up) Add possibility to print holds from holds queue. (17.70 KB, patch)
2015-04-17 14:51 UTC, Rafal Kopaczka
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Rafal Kopaczka 2014-01-31 14:52:33 UTC
I've made a patch that allow print holds from holds queue page. In this way we manage holds in our library, maybe someone else also would like it.
It's managed by preferences, when disabled nothing is changed (default) when enabled there is possibility to print holds separately. 
There are some changes in database schema. I've add print_status column to reserves, old_reserves and tmp_holdsqueue to distinguish between printed and not printed holds. And reserve_id in tmp_holdsqueue to easily gets info about particular reserve.
Comment 1 Rafal Kopaczka 2014-01-31 14:56:39 UTC Comment hidden (obsolete)
Comment 2 Aurélie Fichot 2014-03-12 13:56:52 UTC
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").
Comment 3 Rafal Kopaczka 2014-03-13 10:07:54 UTC Comment hidden (obsolete)
Comment 4 Aurélie Fichot 2014-03-13 10:34:06 UTC
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
Comment 5 Rafal Kopaczka 2014-03-13 10:47:33 UTC Comment hidden (obsolete)
Comment 6 Biblibre Sandboxes 2014-03-13 11:41:03 UTC
Patch tested with a sandbox, by Aurelie Fichot <aurelie.fichot@iepg.fr>
Comment 7 Biblibre Sandboxes 2014-03-13 11:41:22 UTC Comment hidden (obsolete)
Comment 8 Aurélie Fichot 2014-03-13 11:41:42 UTC
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 9 Jonathan Druart 2014-04-11 11:20:27 UTC
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>.
Comment 10 Rafal Kopaczka 2014-05-30 14:11:02 UTC Comment hidden (obsolete)
Comment 11 Rafal Kopaczka 2014-05-30 14:11:50 UTC
(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.
Comment 12 Rafal Kopaczka 2014-10-10 09:57:03 UTC Comment hidden (obsolete)
Comment 13 Rafal Kopaczka 2014-10-10 09:57:40 UTC Comment hidden (obsolete)
Comment 14 Rafal Kopaczka 2014-10-10 09:58:44 UTC
Rebased, against master. Also few minor changes.
Comment 15 Owen Leonard 2014-10-24 14:37:25 UTC
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.
Comment 16 Rafal Kopaczka 2014-10-24 15:26:35 UTC
(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!
Comment 17 Owen Leonard 2014-10-24 16:04:08 UTC
(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.
Comment 18 Rafal Kopaczka 2014-10-24 16:27:46 UTC Comment hidden (obsolete)
Comment 19 Rafal Kopaczka 2014-10-24 16:28:21 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2015-04-17 13:44:48 UTC
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"
Comment 21 Rafal Kopaczka 2015-04-17 14:51:19 UTC
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>
Comment 22 Rafal Kopaczka 2015-04-17 14:51:45 UTC
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.
Comment 23 Rafal Kopaczka 2015-04-17 14:54:04 UTC
(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.
Comment 24 Aleisha Amohia 2015-12-23 23:06:56 UTC
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