Bug 12448 - Ask for confirmation when checking out an item with rental fees
Summary: Ask for confirmation when checking out an item with rental fees
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Maxime Beaulieu
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 12776
  Show dependency treegraph
 
Reported: 2014-06-18 15:52 UTC by Maxime Beaulieu
Modified: 2015-06-04 23:33 UTC (History)
11 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 12448 - Ask for confirmation when checking out an item with rental fees (6.18 KB, patch)
2014-06-18 18:58 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 12448 - Fixes: confirmation when checking out an item with rental fees (37.45 KB, patch)
2014-06-20 19:05 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 12448 - Fixes: confirmation when checking out an item with rental fees (3.65 KB, patch)
2014-06-20 19:22 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 12448 - Fixed a typo in sysprefs (1.48 KB, patch)
2014-06-23 20:49 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 12448 - Ask for confirmation when checking out an item with rental fees (6.25 KB, patch)
2014-06-26 22:57 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12448 - Fixes: confirmation when checking out an item with rental fees (3.71 KB, patch)
2014-06-26 22:57 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12448 - Fixed a typo in sysprefs (1.54 KB, patch)
2014-06-26 22:57 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12448 - I have set the appropriate default and corrected the typo in updatedatabase. (1.25 KB, patch)
2014-06-30 12:09 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
[Signed-off] Bug 12448 - Ask for confirmation when checking out an item with rental fees (6.48 KB, patch)
2014-06-30 12:39 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 12448 - Fixes: confirmation when checking out an item with rental fees (3.88 KB, patch)
2014-06-30 12:45 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12448 - Fixed a typo in sysprefs (1.67 KB, patch)
2014-06-30 12:48 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 12448 - Fixed a typo in sysprefs (1.68 KB, patch)
2014-06-30 13:04 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 12448 - I have set the appropriate default and corrected the typo in updatedatabase. (1.39 KB, patch)
2014-06-30 13:07 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 12448 - Ask for confirmation when checking out an item with rental fees (6.50 KB, patch)
2014-06-30 14:21 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12448 - Fixes: confirmation when checking out an item with rental fees (3.90 KB, patch)
2014-06-30 14:21 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12448 - Fixed a typo in sysprefs (1.73 KB, patch)
2014-06-30 14:21 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12448 - I have set the appropriate default and corrected the typo in updatedatabase. (1.44 KB, patch)
2014-06-30 14:21 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12448 - Follow-up to Comment 32. (2.75 KB, patch)
2014-07-07 12:47 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 12448 - New patch for master v3.17.00.010 (5.19 KB, patch)
2014-07-29 20:33 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
[Signed-off] Bug 12448 - New patch for master v3.17.00.010 (5.36 KB, patch)
2014-08-03 11:34 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 12448: Ask for confirmation when checking out an item with rental fees (6.27 KB, patch)
2014-08-15 19:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12448: Follow-up: Only ask confirmation when rental charge > 0 (1.05 KB, patch)
2014-08-15 19:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12448: (RM followup) fix bug number in updatedatabase.pl (1.11 KB, patch)
2014-11-10 15:56 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 Maxime Beaulieu 2014-06-18 15:52:50 UTC
Prompts the user for confirmation when checking out an item with rental fees.
This patch adds a confirmation box between the toolbar and the checkout form on the checkout page when checking out an item with rental fees.

A system preference is added to manage the patch's behaviour.
The preference's value can either be set to "ask" or "don't ask" and is located under:
Circulation preferences > Checkout Policy > RentalFeesCheckoutConfirmation 

Sponsor: Ville de Victoriaville, QC

Confirmation box contents:

"Please confirm checkout"
"-Rental charge for this item: n"
[Yes, checkout (Y)] [No, Don't Check Out (N)]

Test case A: Confirm checkout
1) Go to checkout user "X"'s checkout page.
2) Enter barcode for an item with rental fees.
3) Click the "Check out" button.
4) Confirmation box appears.
5) Click on the "Yes" button.
6) Item is added to checkout list.
7) Fees are added to the patron's account.

Test case B: Decline checkout
1) Go to checkout user "X"'s checkout page.
2) Enter barcode for an item with rental fees.
3) Click the "Check out" button.
4) Confirmation box appears.
5) Click the "No" button.
6) Checkout page goes back to its initial state.
7) Patron has no item checked out and no fees to pay.
Comment 1 Maxime Beaulieu 2014-06-18 18:58:02 UTC Comment hidden (obsolete)
Comment 2 Christopher Brannon 2014-06-19 22:48:02 UTC
Maxime,
   Functionally, the patch works as described.  A few issues need to be addressed.

First, to maintain consistent behavior, I think the default for the RentalFeesCheckoutConfirmation preference should be Don't Ask.

Next, below is the message that popped up:

Please confirm checkout

    The patron has a debt of 3.00
    Rental charge for this item : 5,00 $


Need the currency symbol ($) in front of what the patron already owes.

Add period at the end of the first sentence.

Change comma to decimal in the rental charge.

Move currency symbol on charge to the front of price.  ($5.00)

Remove space before colon.

Ideally, the message should be formatted to look like this:

Please confirm checkout

    The patron has a debt of $3.00.
    Rental charge for this item: $5.00

Christopher
Comment 3 Katrin Fischer 2014-06-19 22:51:46 UTC
I am sorry, but please don't include a currency symbol at all.

Not everyone is using $. What we ideally would need is a new template toolkit plugin to format amounts correctly and put the currency symbols in the right spot. But that would be a bit outside the scope of this bug and so being consistent with the rest of Koha would be ok.
Comment 4 Katrin Fischer 2014-06-19 22:56:46 UTC
Looking at the code I spotted another problem:

my $sth = $dbh->prepare("SELECT rentalcharge FROM itemtypes WHERE itemtype = ?");

is used to get the rentalcharge - but this doesn't take into account the circulation conditions, where you can define a discount for the rental that will influence how much the borrower has to pay.
Comment 5 Christopher Brannon 2014-06-19 23:04:57 UTC
(In reply to Katrin Fischer from comment #3)
> I am sorry, but please don't include a currency symbol at all.
> 
> Not everyone is using $. What we ideally would need is a new template
> toolkit plugin to format amounts correctly and put the currency symbols in
> the right spot. But that would be a bit outside the scope of this bug and so
> being consistent with the rest of Koha would be ok.

Sorry Katrin, I hadn't noticed that behavior in Koha.  Aside from that, the other formatting should still be adjusted, correct?

Christopher
Comment 6 Katrin Fischer 2014-06-19 23:09:56 UTC
No worries, Christoper, it would nice to have nicer formatting for the currencies :) Yes, decimal point is alright.
Comment 7 Maxime Beaulieu 2014-06-20 19:05:02 UTC Comment hidden (obsolete)
Comment 8 Maxime Beaulieu 2014-06-20 19:22:10 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2014-06-23 07:04:23 UTC
There is a small typo in sysprefs.sql
'Allow user to confirme when checking out an item with rental fees.'
(confirm)
Comment 10 Maxime Beaulieu 2014-06-23 20:49:53 UTC Comment hidden (obsolete)
Comment 11 Biblibre Sandboxes 2014-06-26 22:56:44 UTC
Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>
Comment 12 Biblibre Sandboxes 2014-06-26 22:57:19 UTC Comment hidden (obsolete)
Comment 13 Biblibre Sandboxes 2014-06-26 22:57:23 UTC Comment hidden (obsolete)
Comment 14 Biblibre Sandboxes 2014-06-26 22:57:26 UTC Comment hidden (obsolete)
Comment 15 Christopher Brannon 2014-06-26 23:08:35 UTC
Please double check the default setting of RentalFeesCheckoutConfirmation.  I signed off on the feature, but could not confirm if it was my setup.  It does not look like it is defaulting.

Sorry, forgot to check on last testing.
Comment 16 Maxime Beaulieu 2014-06-27 18:05:33 UTC
The default value was set to '0' the 'Fixed a typo' patch.
Comment 17 Christopher Brannon 2014-06-27 18:10:19 UTC
(In reply to Maxime Beaulieu from comment #16)
> The default value was set to '0' the 'Fixed a typo' patch.

I would ask that someone else confirm that the preference is defaulting to "Don't Ask" before we move forward.  As I said, I can't confirm that it works.

Christopher
Comment 18 Maxime Beaulieu 2014-06-27 18:34:15 UTC
Requesting signoff for the default behaviour.
Comment 19 Marc Véron 2014-06-28 09:21:59 UTC
I tested. After updating the database, the value of 'RentalFeesCheckoutConfirmation' was set to 1.

Reason: The default value of the system preference is set to '0' in sysprefs.sql, but not in updatedatabase.pl.

Same for the typo.
Comment 20 Maxime Beaulieu 2014-06-30 12:09:39 UTC Comment hidden (obsolete)
Comment 21 Marc Véron 2014-06-30 12:33:17 UTC
Tested following Testplan in comment #1
Works as expected.
Sing-offs for single patches follow.
Comment 22 Marc Véron 2014-06-30 12:39:05 UTC Comment hidden (obsolete)
Comment 23 Marc Véron 2014-06-30 12:45:55 UTC Comment hidden (obsolete)
Comment 24 Marc Véron 2014-06-30 12:48:48 UTC Comment hidden (obsolete)
Comment 25 Marc Véron 2014-06-30 13:04:51 UTC Comment hidden (obsolete)
Comment 26 Marc Véron 2014-06-30 13:07:28 UTC Comment hidden (obsolete)
Comment 27 Biblibre Sandboxes 2014-06-30 14:20:43 UTC
Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>
Comment 28 Biblibre Sandboxes 2014-06-30 14:21:25 UTC Comment hidden (obsolete)
Comment 29 Biblibre Sandboxes 2014-06-30 14:21:28 UTC Comment hidden (obsolete)
Comment 30 Biblibre Sandboxes 2014-06-30 14:21:31 UTC Comment hidden (obsolete)
Comment 31 Biblibre Sandboxes 2014-06-30 14:21:34 UTC Comment hidden (obsolete)
Comment 32 Katrin Fischer 2014-07-06 17:53:32 UTC
I feel like it would be better to check for the preference first, so the database is only queried for the rental fee, if it's needed. What do you think?
This is the code from the squashed patches: 

+    #
+    # CHECK IF THERE IS RENTAL CHARGES. RENTAL MUST BE CONFIRMED BY THE BORROWER
+    #
+    my ($rentalCharge) = GetIssuingCharges( $item->{'itemnumber'}, $borrower->{'borrowernumber'} );
+    my $rentalConfirmation = C4::Context->preference("RentalFeesCheckoutConfirmation");
+
+    if ( $rentalConfirmation && $rentalCharge != 0 )
+    {
+        $rentalCharge = sprintf("%.02f", $rentalCharge);
+        $needsconfirmation{RENTALCHARGE} = $rentalCharge;
+    }

For the pref description I think maybe it should be:
ask/don't ask 'for confirmation' instead of 'a confirmation'. 
But as I am not a native speaker myself, you might want to check that with one :)
Comment 33 Maxime Beaulieu 2014-07-07 12:47:28 UTC Comment hidden (obsolete)
Comment 34 Marc Véron 2014-07-26 17:27:56 UTC
Sorry, it doesn't apply on current master (3.17.00.010)

Applying: Bug 12448 - Ask for confirmation when checking out an item with rental fees
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
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 12448 - Ask for confirmation when checking out an item with rental fees
Comment 35 Maxime Beaulieu 2014-07-29 20:33:54 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2014-07-30 05:28:01 UTC
Hi Maxime, please remember to assign bugs to yourself, when you are working on them/providing patches, thx!
Comment 37 Katrin Fischer 2014-08-03 10:22:14 UTC
Hi Maxime, we try to avoid squashing patches, when it would mean to lose sign-off lines or a mix up of authorship of the code. The code in your squashed patch looks perfectly fine, but now we are missing the sign-off lines from Christopher and Marc. A follow-up would have worked better in this case.

I am switching back to Needs Signoff for now - if Marc or Christopher are ok with it, I can also add back their lines, but didn't want to add them to changed code without their permission.
Comment 38 Marc Véron 2014-08-03 11:34:12 UTC Comment hidden (obsolete)
Comment 39 Katrin Fischer 2014-08-15 19:08:02 UTC
Created attachment 30862 [details] [review]
[PASSED QA] Bug 12448: Ask for confirmation when checking out an item with rental fees

Sponsored-by: Ville de Victoriaville, QC

Confirmation box contents:

"Please confirm checkout"
"-Rental charge for this item: n"
[Yes, check out (Y)] [No, Don't Check Out (N)]

Test case A: Confirm checkout
1) Go to checkout user "X"'s checkout page.
2) Enter barcode for an item with rental fees.
3) Click the "Check out" button.
4) Confirmation box appears.
5) Click on the "Yes" button.
6) Item is added to checkout list.
7) Fees are added to the patron's account.

Test case B: Decline checkout
1) Go to checkout user "X"'s checkout page.
2) Enter barcode for an item with rental fees.
3) Click the "Check out" button.
4) Confirmation box appears.
5) Click the "No" button.
6) Checkout page goes back to its initial state.
7) Patron has no item checked out and no fees to pay.

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
With the system preference RentalFeesCheckoutConfirmation
set to "don't ask" there is no change in behaviour.
Comment 40 Katrin Fischer 2014-08-15 19:09:25 UTC
I have amended the commit message, added the test plan and sponsored-by line back.
Comment 41 Katrin Fischer 2014-08-15 19:27:35 UTC
I am sorry, but I found a problem: checking out an item with 0.00 also requires confirmation now. That should not be the case.
Comment 42 Katrin Fischer 2014-08-15 19:39:25 UTC
Created attachment 30864 [details] [review]
Bug 12448: Follow-up: Only ask confirmation when rental charge > 0

To test:
- Check out an item with no rental charge
- Verify no confirmation message appears
- Check out an item with rental charge
- Verify rental charge shows
Comment 43 Katrin Fischer 2014-08-15 19:40:06 UTC
Leaving it to Tomas if this is tiny enough for a QA follow-up or requires another sign off.
Comment 44 Tomás Cohen Arazi 2014-11-07 14:46:47 UTC
Patches pushed to master.

Thanks!
Comment 45 Jonathan Druart 2014-11-10 11:50:08 UTC
The updatedb entry refers to bug 11169.
Comment 46 Tomás Cohen Arazi 2014-11-10 15:56:49 UTC
Created attachment 33434 [details] [review]
Bug 12448: (RM followup) fix bug number in updatedatabase.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>