Bug 11882 - Add a new button to the checkout confirmation dialog when checking out an on hold item
Summary: Add a new button to the checkout confirmation dialog when checking out an on ...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: simith.doliveira
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 17798
  Show dependency treegraph
 
Reported: 2014-02-28 22:57 UTC by Frédérick Capovilla
Modified: 2017-06-28 21:40 UTC (History)
14 users (show)

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


Attachments
Add a "Don't Check Out and Print Slip" button to the dialog that appears when trying to checkout a reserved item. (1.61 KB, patch)
2014-02-28 22:57 UTC, Frédérick Capovilla
Details | Diff | Splinter Review
[Signed-off] Add a "Don't Check Out and Print Slip" button to the dialog that appears when trying to checkout a reserved item. (2.34 KB, patch)
2014-03-04 15:32 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item (4.34 KB, patch)
2014-09-03 13:00 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item (4.12 KB, patch)
2014-09-05 13:50 UTC, simith.doliveira
Details | Diff | Splinter Review
Add a new button to the checkout confirmation dialog when checking out a reserved item (1.57 KB, patch)
2014-11-03 14:15 UTC, simith.doliveira
Details | Diff | Splinter Review
camelot 778 lbo_RM5001_Bordereau / BZ11882 fix (1.57 KB, patch)
2014-12-15 13:40 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item (3.71 KB, patch)
2014-12-15 13:48 UTC, simith.doliveira
Details | Diff | Splinter Review
[Signed-off] Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item (3.88 KB, text/plain)
2014-12-20 16:49 UTC, Marc Véron
Details
Fix comment 9 and 19 (1.60 KB, text/plain)
2015-01-15 19:31 UTC, simith.doliveira
Details
Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item (3.66 KB, patch)
2015-02-26 18:25 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item (3.71 KB, patch)
2015-02-26 19:22 UTC, Nick Clemens
Details | Diff | Splinter Review
[Signed-off] Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item (3.89 KB, patch)
2015-02-26 19:40 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 11882 - Add a new button to the checkout confirmation dialog when checking out a reserved item (3.75 KB, patch)
2015-03-05 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11882: (QA followup) fix capitalization errors (2.23 KB, patch)
2015-06-26 18:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédérick Capovilla 2014-02-28 22:57:04 UTC Comment hidden (obsolete)
Comment 1 Frédérick Capovilla 2014-02-28 22:57:39 UTC
Sponsored-by: CCSR ( http://www.ccsr.qc.ca )
Comment 2 Marc Véron 2014-03-04 15:32:13 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-03-20 12:06:51 UTC
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.
Comment 4 Kyle M Hall 2014-04-11 14:54:12 UTC
(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.
Comment 5 Owen Leonard 2014-04-11 15:26:10 UTC
(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.
Comment 6 simith.doliveira 2014-09-03 13:00:08 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2014-09-05 13:14:04 UTC
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.
Comment 8 simith.doliveira 2014-09-05 13:50:33 UTC Comment hidden (obsolete)
Comment 9 Paola Rossi 2014-10-21 15:36:07 UTC
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.
Comment 10 simith.doliveira 2014-11-03 14:15:08 UTC Comment hidden (obsolete)
Comment 11 Paola Rossi 2014-11-17 16:44:48 UTC
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 12 simith.doliveira 2014-12-15 13:40:07 UTC Comment hidden (obsolete)
Comment 13 simith.doliveira 2014-12-15 13:45:44 UTC
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
Comment 14 simith.doliveira 2014-12-15 13:48:39 UTC Comment hidden (obsolete)
Comment 15 Paola Rossi 2014-12-16 11:03:41 UTC
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?]
Comment 16 simith.doliveira 2014-12-16 18:48:48 UTC
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.
Comment 17 Paola Rossi 2014-12-19 16:18:02 UTC
(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".
Comment 18 Marc Véron 2014-12-20 16:49:43 UTC Comment hidden (obsolete)
Comment 19 Paola Rossi 2014-12-22 15:25:28 UTC
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.
Comment 20 Marc Véron 2014-12-22 18:23:55 UTC
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.
Comment 21 simith.doliveira 2015-01-15 19:31:44 UTC Comment hidden (obsolete)
Comment 22 simith.doliveira 2015-01-15 19:45:34 UTC
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.
Comment 23 Jonathan Druart 2015-01-16 09:59:20 UTC
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?
Comment 24 Jonathan Druart 2015-02-26 14:39:26 UTC
Answer needed.
Comment 25 simith.doliveira 2015-02-26 18:25:34 UTC Comment hidden (obsolete)
Comment 26 simith.doliveira 2015-02-26 18:27:54 UTC
Patchs cherry picked to create a new one to be tested
Comment 27 Nick Clemens 2015-02-26 19:22:16 UTC Comment hidden (obsolete)
Comment 28 Nick Clemens 2015-02-26 19:36:11 UTC
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.
Comment 29 Marc Véron 2015-02-26 19:40:08 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2015-03-05 09:57:12 UTC
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>
Comment 31 Tomás Cohen Arazi 2015-06-26 18:12:53 UTC
Created attachment 40692 [details] [review]
Bug 11882: (QA followup) fix capitalization errors

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 32 Tomás Cohen Arazi 2015-06-26 18:14:03 UTC
Patch pushed to master.

Thanks Simith!
Comment 33 Jonathan Druart 2015-07-02 09:31:50 UTC
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.
Comment 34 Jonathan Druart 2017-01-10 09:29:03 UTC
@InLibro: It seems that the behaviour of this new button is not what people expects.
Can you take a look at bug 17798?
Comment 35 Katrin Fischer 2017-05-14 21:50:39 UTC
Please fix the bug - this feature is not usable at the moment.
Comment 36 Francois Charbonnier 2017-06-28 14:17:22 UTC
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.
Comment 37 Katrin Fischer 2017-06-28 21:40:28 UTC
Moving discussion to bug 17798.