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.
Created attachment 28954 [details] [review] Bug 12448 - Ask for confirmation when checking out an item with rental fees 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.
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
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.
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.
(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
No worries, Christoper, it would nice to have nicer formatting for the currencies :) Yes, decimal point is alright.
Created attachment 29123 [details] [review] Bug 12448 - Fixes: confirmation when checking out an item with rental fees Fixes: Default behaviour changed to "Don't ask". Adressed the punctuation issues. Removed the currency symbols in the warning message. Using "GetIssuingCharges()" to calculate the correct charge amount, including applicable discounts.
Created attachment 29124 [details] [review] Bug 12448 - Fixes: confirmation when checking out an item with rental fees Fixes: Default behaviour changed to "Don't ask". Adressed the punctuation issues. Removed the currency symbols in the warning message. Using "GetIssuingCharges()" to calculate the correct charge amount, including applicable discounts.
There is a small typo in sysprefs.sql 'Allow user to confirme when checking out an item with rental fees.' (confirm)
Created attachment 29182 [details] [review] Bug 12448 - Fixed a typo in sysprefs
Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 29311 [details] [review] Bug 12448 - Ask for confirmation when checking out an item with rental fees 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. Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 29312 [details] [review] Bug 12448 - Fixes: confirmation when checking out an item with rental fees Fixes: Default behaviour changed to "Don't ask". Adressed the punctuation issues. Removed the currency symbols in the warning message. Using "GetIssuingCharges()" to calculate the correct charge amount, including applicable discounts. Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 29313 [details] [review] Bug 12448 - Fixed a typo in sysprefs Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
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.
The default value was set to '0' the 'Fixed a typo' patch.
(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
Requesting signoff for the default behaviour.
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.
Created attachment 29361 [details] [review] Bug 12448 - I have set the appropriate default and corrected the typo in updatedatabase.
Tested following Testplan in comment #1 Works as expected. Sing-offs for single patches follow.
Created attachment 29362 [details] [review] [Signed-off] Bug 12448 - Ask for confirmation when checking out an item with rental fees 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. Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Marc Véron <veron@veron.ch> All patches applied. Works as expected. Sign-off on patch #1
Created attachment 29363 [details] [review] [Signed-off] Bug 12448 - Fixes: confirmation when checking out an item with rental fees Fixes: Default behaviour changed to "Don't ask". Adressed the punctuation issues. Removed the currency symbols in the warning message. Using "GetIssuingCharges()" to calculate the correct charge amount, including applicable discounts. Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 29364 [details] [review] Bug 12448 - Fixed a typo in sysprefs Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 29367 [details] [review] [Signed-off] Bug 12448 - Fixed a typo in sysprefs Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 29368 [details] [review] [Signed-off] Bug 12448 - I have set the appropriate default and corrected the typo in updatedatabase. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 29371 [details] [review] Bug 12448 - Ask for confirmation when checking out an item with rental fees 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. Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Marc Véron <veron@veron.ch> All patches applied. Works as expected. Sign-off on patch #1 Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 29372 [details] [review] Bug 12448 - Fixes: confirmation when checking out an item with rental fees Fixes: Default behaviour changed to "Don't ask". Adressed the punctuation issues. Removed the currency symbols in the warning message. Using "GetIssuingCharges()" to calculate the correct charge amount, including applicable discounts. Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 29373 [details] [review] Bug 12448 - Fixed a typo in sysprefs Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org> Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Created attachment 29374 [details] [review] Bug 12448 - I have set the appropriate default and corrected the typo in updatedatabase. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
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 :)
Created attachment 29543 [details] [review] Bug 12448 - Follow-up to Comment 32. In C4/Circulation.pm, 'GetIssuingCharges' is now called after the syspref is checked. The text for the system preference now reads "When checking out an item with rental fees [ask/don't ask] for confirmation. modified: C4/Circulation.pm modified: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
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
Created attachment 30307 [details] [review] Bug 12448 - New patch for master v3.17.00.010 Squashed commit of everything there was before.
Hi Maxime, please remember to assign bugs to yourself, when you are working on them/providing patches, thx!
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.
Created attachment 30515 [details] [review] [Signed-off] Bug 12448 - New patch for master v3.17.00.010 Squashed commit of everything there was before. Signed-off-by: Marc Véron <veron@veron.ch>
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.
I have amended the commit message, added the test plan and sponsored-by line back.
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.
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
Leaving it to Tomas if this is tiny enough for a QA follow-up or requires another sign off.
Patches pushed to master. Thanks!
The updatedb entry refers to bug 11169.
Created attachment 33434 [details] [review] Bug 12448: (RM followup) fix bug number in updatedatabase.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>