Sponsored-by: CCSR ( http://www.ccsr.qc.ca )

The 'cancel hold' checkbox is not taken into account (but it is already the case for the "No, don't check out" button).
The redirect to returns.pl is a little bit unexpected, in my opinion.

I need other qa points of view for this one.

(In reply to Jonathan Druart from comment #3)
> The 'cancel hold' checkbox is not taken into account (but it is already the
> case for the "No, don't check out" button).
> The redirect to returns.pl is a little bit unexpected, in my opinion.
> 
> I need other qa points of view for this one.

I agree, the behavior of this patch is very odd. I don't think the redirect is a very good idea. It feels very much like a hack especially since the user is not told this is what will happen. I would expect it to remain on the circulation screen and open a hold slip popup the same way the "print and confirm" button does on returns.pl. I see no reason why it could not be implemented that way.

(In reply to Kyle M Hall from comment #4)
> I would expect it to remain on
> the circulation screen and open a hold slip popup the same way the "print
> and confirm" button does on returns.pl. I see no reason why it could not be
> implemented that way.

Part of the problem is that the button text isn't clear about what is going to happen, but I agree with Kyle that it could be handled better with regard to the checkout process. It would be great not to interrupt the checkout session.

"Confirm hold and print slip" maybe, triggering the popup? And then circulation.pl returns to a state where it is ready to check out another item to that patron.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.

I've applied against master 3.17.00.029

The test plan was OK as required.

But, when I tried to select "Yes, check out (Y)", the page remained "Loading...".
And no checkout was created for the patron.

So I pass the patch to "Failed QA" status.

I've tried to apply the two patches against master 3.17.00.055

Applying: Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging circ/circulation.pl
Failed to merge in the changes.

So I pass the patch to "Patch doesn't apply" status.

Comment on attachment 34420 [details] [review]
camelot 778 lbo_RM5001_Bordereau / BZ11882 fix

>From d95afe65f1b4939d22ea27af9c531faff3e7ccd3 Mon Sep 17 00:00:00 2001
>From: simith <simith@inlibro.com>
>Date: Mon, 3 Nov 2014 09:06:40 -0500
>Subject: [PATCH] camelot 778 lbo_RM5001_Bordereau / BZ11882 fix
>
>---
> circ/circulation.pl | 18 +++++++-----------
> 1 file changed, 7 insertions(+), 11 deletions(-)
>
>diff --git a/circ/circulation.pl b/circ/circulation.pl
>index dc41405..88a0ccc 100755
>--- a/circ/circulation.pl
>+++ b/circ/circulation.pl
>@@ -380,6 +380,13 @@ if ($barcode) {
>     
>     my ( $od, $issue, $fines ) = GetMemberIssuesAndFines($borrowernumber);
>     $template->param( issuecount => $issue );
>+
>+    if ($question->{RESERVE_WAITING} or $question->{RESERVED}){
>+        $template->param(
>+            reserveborrowernumber => $question->{'resborrowernumber'},
>+            itembiblionumber => $getmessageiteminfo->{'biblionumber'}
>+        );
>+    }
> }
> 
> # reload the borrower info for the sake of reseting the flags.....
>@@ -554,17 +561,6 @@ my $relatives_issues_count =
>   Koha::Database->new()->schema()->resultset('Issue')
>   ->count( { borrowernumber => \@relatives } );
> 
>-(my $returned, my $messages) = AddReturn( $barcode, $branch);
>-if ( $messages->{'ResFound'}) {
>-     my $reserve = $messages->{'ResFound'};
>-    # get biblio description
>-    my $biblio = GetBiblioFromItemNumber($reserve->{'itemnumber'});
>-    $template->param(
>-        reserveborrowernumber => $reserve->{'borrowernumber'},
>-        itembiblionumber => $biblio->{'biblionumber'}
>-    );
>-}
>-
> $template->param(
>     lib_messages_loop => $lib_messages_loop,
>     bor_messages_loop => $bor_messages_loop,
>-- 
>1.9.1

I've tried to apply the 2 attached patches against master 3.19.00.000 

At the first patch :

Applying: camelot 778 lbo_RM5001_Bordereau / BZ11882 fix
fatal: sha1 information is lacking or useless (circ/circulation.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 camelot 778 lbo_RM5001_Bordereau / BZ11882 fix

So I pass the patch to "Patch doesn't apply" status.

[I'm in doubt: perhaps should I have applied only the second attached patch to master?]

Don't use this patch : camelot 778 lbo_RM5001_Bordereau / BZ11882 fix this is a obsolete patch. Have just applyed (last koha version) and worked.

(In reply to simith.doliveira from comment #16)
> Don't use this patch : camelot 778 lbo_RM5001_Bordereau / BZ11882 fix this
> is a obsolete patch. Have just applyed (last koha version) and worked.

(In reply to simith.doliveira from comment #16)
> Don't use this patch : camelot 778 lbo_RM5001_Bordereau / BZ11882 fix this
> is a obsolete patch. 

So I've made that patch obsolete: logged to bugzilla, I selected the "Details" of that patch, and then I "Edit details" at top-right. Then I checked the left-bottom "obsolete" checkbox, and "submit".

When I tried to select "Yes, check out (Y)", the page remained "Loading..." and no checkout was created for the patron.

No item could be checked out, even if not reserved.

On the contrary items can be checked out on master (3.19.00.002).

I pass the patch to "Failed QA" status.

Sorry, I missed check-out failing while testing for Sing-off.

Reason for failing:

The patch calls 
AddReturn( $barcode, $branch)
in circ/circulation.pl  (line 557)

AddReturn() immediately returns the item (and adds an entry about returning it to the Circulation history)

Maybe CheckReserves would be a better way, something like

$tmpitem = GetBiblioItemFromItemNumber(undef, $barcode)
then test return value, call CheckReserves with $tmpitem->{'itemnumber'} and fill the template params as necessary.

The fix for the comment 19 I have already done in 2014-11-03 (to fix the comment 9) but was sent to obsolete by mistake, so I recreated it.

I don't understand the status, to me the second patch needs a signoff.
Simith, could you please provide a decent commit message for it?

Answer needed.

Patchs cherry picked to create a new one to be tested

This seems to work as expected, tested it with next available, item specific, and holds that don't respect holds rules, all seemed fine.  Other buttons seemed to work too.  

Cancel hold checkbox would ideally be moved into/next to the 'Yes Checkout' button as it is otherwise invalid, but that is probab;y beyond scope.

I like this idea, but the text seems odd to me ('Don't check and print slip'), it makes sense if you think about it, but I don't see it making intuitive sense to all of our circ staff.  

That being said, I can't think of a better phrasing so signed off.

Created attachment 36545 [details] [review]
Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item

This patch adds a "Don't Check Out and Print Slip" button to the dialog that appears
        when trying to checkout a reserved item.

Test case :
* Put a hold on an item for User A.
* Try to check out it item with User B.
* The "Please confirm checkout" box should appear. The "Don't Check Out and Print Slip" button should appear in this box.
* Click on the "Don't Check Out and Print Slip" button.
* A print popup will appear

Signed-off-by: Nick <Nick@quechelibrary.org>

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Created attachment 40692 [details] [review]
Bug 11882: (QA followup) fix capitalization errors

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>

Patch pushed to master.

Thanks Simith!

Oops, it seems that my pass qa was an error.
Not a big deal, but looking at the diff, it seems that the code could be improved:
in the template, we have:

IF RESERVED
  display_button
ELSIF RESERVE_WAITING
  display_button

Could be changed with a IF RESERVED or RESERVE_WAITING.

@InLibro: It seems that the behaviour of this new button is not what people expects.
Can you take a look at bug 17798?

Please fix the bug - this feature is not usable at the moment.

Hi Katrin,

Which bug? The one on bug 17798?

To my understanding, this is what happened :

Situation 1 (regular)

- Patron A put a hold on Book A
- Book A is returned. Hold is confirmed.
- Patron B checkout Book A
- Koha says "This book is on hold for Patron A, what do you want to do?"
- The button "Don't checkout and print" allows you to cancel the process and print a slip.
- It keeps the book on hold for Patron A. It's actually already on hold so there should be no reason to update it.

BUT, here comes situation 2 (exception)

- Patron A put a hold on Book A
- Book A is returned. Hold is NOT confirmed (Library wants to confirm the hold later to control when the confirmation email is sent, or library had forgotten, etc.)
- Patron B checkout Book A
- Koha says "This book is on hold for Patron A, what do you want to do?"
- The button "Don't checkout and print" allows you to cancel the process and print a slip.
- The hold is not confirmed and this button doesn’t update it.

You expect it to update the hold and change the status to waiting like if you were checking in the book, right ?

Thanks.

Moving discussion to bug 17798.