Work to be done: 1) Add new column for daily rental fee to itemtypes 2) Add new syspref to control if daily rental fee includes holidays 3) Add code to C4::Circulation::AddIssue that will check the item's type to determine if it has a daily rental fee a) If it does, the rental fee will be calculated in full as "checkout length" times "daily rental fee", less holidays if syspref is set to ignore them 4) Add unit tests
Created attachment 76065 [details] [review] Bug 20912: Update database
Created attachment 76066 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days and item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00
Created attachment 76067 [details] [review] Bug 20912: Update schema file
Created attachment 76068 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days and item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00
Created attachment 76069 [details] [review] Bug 20912: Update schema file
Tested - works fine for me! Signed off - Martha Fuerst
Created attachment 77302 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org>
Created attachment 77303 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days and item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org>
Created attachment 77304 [details] [review] Bug 20912: Update schema file Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org>
Patch doesn't apply - please rebase! (might need some more XSS filters)
Created attachment 81141 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org>
Created attachment 81142 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days and item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org>
Created attachment 81143 [details] [review] Bug 20912: Update schema file
Starting here with QA script and code review: 1) QA Script FAIL t/db_dependent/Circulation.t OK critic OK forbidden patterns OK git manipulation OK pod FAIL spelling calulated ==> calculated OK valid FAIL t/db_dependent/Koha/ItemTypes.t OK critic OK forbidden patterns OK git manipulation OK pod FAIL spelling calulated ==> calculated OK valid 2) Code review 2.1) <span class="hint">This fee is charged a checkout/renewal time for each day between the checkout/renewal date and due date.</span> ==> charged at... 2.2) Why the check on notforloan? What if the notforloan is overriden? + <td> + [% UNLESS ( itemtype.notforloan ) %] + [% itemtype.rental_charge_daily | $Price %] + [% END %] + </td> 3) Tests Circulation.t needs to be updated to run 120 tests (not 119) There is a new warn in the other tests: t/db_dependent/Koha/ItemTypes.t .. 23/25 Use of uninitialized value in string eq at /home/vagrant/kohaclone/Koha/ItemType.pm line 43.
Created attachment 81450 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos
Fixed 1) and the first part of 3). Still needs some love :)
Created attachment 81771 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org>
Created attachment 81772 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days and item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org>
Created attachment 81773 [details] [review] Bug 20912: Update schema file
Created attachment 81774 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos
Created attachment 81775 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test
Created attachment 81778 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings
I won't be able to finish here today - would be really happy if someone else from the QA team could have a look.
I'd like to see some tests for various combinations of process_fee, rentalcharge and rental_charge_daily before passing this. Looking at the code there seems to be a little disparity between charging the amount returned by GetIssuingCharges followed by a second charge calculated using rental_charge_daily.. sometimes it looks like a combined charge, sometimes it looks like two charges and sometimes two charges with the second change also being a combined charge. Hence the asking for a few more tests ;) Also.. i think I'd prefer that the calculation was folded into GetIssuingCharges myself.. it seems somewhat fraught with peril to be duplicating this code.
I have a few remarks: my ($rentalCharge) = GetIssuingCharges( $item->{'itemnumber'}, $patron->borrowernumber ); + my $itemtype = Koha::ItemTypes->find( $item->{itype} ); # GetItem sets effective itemtype + $rentalCharge += $itemtype->calc_rental_charge_daily( { from => dt_from_string(), to => $duedate } ); This doesn't look right, shouldn't it be either a fixed charge or the daily calculated one but not both?? If it was two different charges, we might better have separate offsets for them, right? I would remove any fee calculation from Koha::ItemType (same for using userenv). It should only return the 'daily' charge and let some other class handle the calculation. I've been thinking of my $fee = Koha::Fee->new({ library => $library, patron => $patron, item => $item, datetime_due => $dt }); This is of course out of the scope of this bug, but with that in mind, I concur on Martin's proposal of having GetIssuingCharges do it on this bug. The only concern I have is offsets.
(In reply to Tomás Cohen Arazi from comment #25) > I have a few remarks: > > my ($rentalCharge) = GetIssuingCharges( $item->{'itemnumber'}, > $patron->borrowernumber ); > + my $itemtype = Koha::ItemTypes->find( $item->{itype} ); # GetItem > sets effective itemtype > + $rentalCharge += $itemtype->calc_rental_charge_daily( { from => > dt_from_string(), to => $duedate } ); > > This doesn't look right, shouldn't it be either a fixed charge or the daily > calculated one but not both?? If it was two different charges, we might > better have separate offsets for them, right? Yes, a library should be allowed to have a flat fee plus a daily fee if they wish. This patch set already ensures they will have separate offsets. > I would remove any fee calculation from Koha::ItemType (same for using > userenv). It should only return the 'daily' charge and let some other class > handle the calculation. That's fine by me! > I've been thinking of > my $fee = Koha::Fee->new({ library => $library, patron => $patron, item => > $item, datetime_due => $dt }); > > This is of course out of the scope of this bug, but with that in mind, I > concur on Martin's proposal of having GetIssuingCharges do it on this bug. > The only concern I have is offsets. Integrating this into GetIssuingCharges seems like a step backwards. What if I created Koha::Fees and moved the method to it? What I imagine is basically my @fees = Koha::Fee->new({ library => $library, patron => $patron, item => > $item, datetime_due => $dt })->get_fees, where the return is a list of Koha::Fee objects. To limit the scope creep, I will only move calc_rental_charge_daily into Fees.pm, and leave the breaking up and moving of the rest of GetIssuingCharges as a later exercise. What do you think?
(In reply to Kyle M Hall from comment #26) > (In reply to Tomás Cohen Arazi from comment #25) > > I have a few remarks: > > > > my ($rentalCharge) = GetIssuingCharges( $item->{'itemnumber'}, > > $patron->borrowernumber ); > > + my $itemtype = Koha::ItemTypes->find( $item->{itype} ); # GetItem > > sets effective itemtype > > + $rentalCharge += $itemtype->calc_rental_charge_daily( { from => > > dt_from_string(), to => $duedate } ); > > > > This doesn't look right, shouldn't it be either a fixed charge or the daily > > calculated one but not both?? If it was two different charges, we might > > better have separate offsets for them, right? > > Yes, a library should be allowed to have a flat fee plus a daily fee if they > wish. This patch set already ensures they will have separate offsets. > > > > I would remove any fee calculation from Koha::ItemType (same for using > > userenv). It should only return the 'daily' charge and let some other class > > handle the calculation. > > That's fine by me! > > > I've been thinking of > > my $fee = Koha::Fee->new({ library => $library, patron => $patron, item => > > $item, datetime_due => $dt }); > > > > This is of course out of the scope of this bug, but with that in mind, I > > concur on Martin's proposal of having GetIssuingCharges do it on this bug. > > The only concern I have is offsets. > > Integrating this into GetIssuingCharges seems like a step backwards. What if > I created Koha::Fees and moved the method to it? What I imagine is basically > > my @fees = Koha::Fee->new({ library => $library, patron => $patron, item => > > $item, datetime_due => $dt })->get_fees, where the return is a list of Koha::Fee objects. To limit the scope creep, I will only move calc_rental_charge_daily into Fees.pm, and leave the breaking up and moving of the rest of GetIssuingCharges as a later exercise. > > What do you think? I agree.
Created attachment 82195 [details] [review] Bug 20912: Move calculation to Koha::Fees
Created attachment 82196 [details] [review] Bug 20912: Move calculation to Koha::Fees
Created attachment 84412 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org>
Created attachment 84413 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days and item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org>
Created attachment 84414 [details] [review] Bug 20912: Update schema file
Created attachment 84415 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos
Created attachment 84416 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test
Created attachment 84417 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings
Created attachment 84418 [details] [review] Bug 20912: Move calculation to Koha::Fees
Created attachment 84419 [details] [review] Bug 20912 (QA Follow-up): Fix up Koha::Fees
Created attachment 84420 [details] [review] Bug 20912 (QA Follow-up): Fix up unit tests
Created attachment 84570 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84571 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days an item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84572 [details] [review] Bug 20912: Update schema file Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84573 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84574 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84575 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84576 [details] [review] Bug 20912: Move calculation to Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84577 [details] [review] Bug 20912: (QA follow-up): Fix up Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84578 [details] [review] Bug 20912: (QA follow-up): Fix up unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84579 [details] [review] Bug 20912: (QA follow-up) Move Fees to Charges:: In preparation for the introduction of Koha::Charges::Fines I have moved this ::Fees class into the Koha::Charges:: namespace Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84580 [details] [review] Bug 20912: (QA follow-up) Test for distinct accountlines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84581 [details] [review] Bug 20912: (QA follow-up) Make DB Update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84582 [details] [review] Bug 20912: (follow-up) Improve test coverage Increase test coverage for CanBookBeIssued and fix a introduced during the refactoring to Koha::Fees. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84583 [details] [review] Bug 20912: (follow-up) Add hourly support It seemed strange to only add support for Daily loans in the feature when Koha also support loan periods in hours. This patch adds parallel functionaliy for hourly loan periods. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84584 [details] [review] Bug 20912: (QA follow-up) Fix TestBuilder Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84585 [details] [review] Bug 20912: (QA follow-up) Fix some test failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I have given this a thorough testing and added a few qa followups to fix some failing test cases and improve test coverage. I also felt it important to add support for hourly loan periods and as such, I've added a pair of follow-ups to add this. As such, although I'd PQA on all work preceding my own, I've asked Tomas and Kyle to cast their eyes over the followup as a final QA signoff.
Created attachment 84610 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84611 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days an item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 84612 [details] [review] Bug 20912: Update schema file Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84613 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84614 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84615 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84616 [details] [review] Bug 20912: Move calculation to Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84617 [details] [review] Bug 20912: (QA follow-up): Fix up Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84618 [details] [review] Bug 20912: (QA follow-up): Fix up unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84619 [details] [review] Bug 20912: (QA follow-up) Move Fees to Charges:: In preparation for the introduction of Koha::Charges::Fines I have moved this ::Fees class into the Koha::Charges:: namespace Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84620 [details] [review] Bug 20912: (QA follow-up) Test for distinct accountlines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84621 [details] [review] Bug 20912: (QA follow-up) Make DB Update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84622 [details] [review] Bug 20912: (follow-up) Improve test coverage Increase test coverage for CanBookBeIssued and fix a introduced during the refactoring to Koha::Fees. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84623 [details] [review] Bug 20912: (follow-up) Add hourly support It seemed strange to only add support for Daily loans in the feature when Koha also support loan periods in hours. This patch adds parallel functionaliy for hourly loan periods. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84624 [details] [review] Bug 20912: (QA follow-up) Fix TestBuilder Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84625 [details] [review] Bug 20912: (QA follow-up) Fix some test failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to Martin Renvoize from comment #55) > I have given this a thorough testing and added a few qa followups to fix > some failing test cases and improve test coverage. > > I also felt it important to add support for hourly loan periods and as such, > I've added a pair of follow-ups to add this. As such, although I'd PQA on > all work preceding my own, I've asked Tomas and Kyle to cast their eyes over > the followup as a final QA signoff. Are we waiting for Tomas or should this be moved on?
Created attachment 84707 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84708 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days an item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84709 [details] [review] Bug 20912: Update schema file Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84710 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84711 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84712 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84713 [details] [review] Bug 20912: Move calculation to Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84714 [details] [review] Bug 20912: (QA follow-up): Fix up Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84715 [details] [review] Bug 20912: (QA follow-up): Fix up unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84716 [details] [review] Bug 20912: (QA follow-up) Move Fees to Charges:: In preparation for the introduction of Koha::Charges::Fines I have moved this ::Fees class into the Koha::Charges:: namespace Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84717 [details] [review] Bug 20912: (QA follow-up) Test for distinct accountlines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84718 [details] [review] Bug 20912: (QA follow-up) Make DB Update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84719 [details] [review] Bug 20912: (follow-up) Improve test coverage Increase test coverage for CanBookBeIssued and fix a introduced during the refactoring to Koha::Fees. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84720 [details] [review] Bug 20912: (follow-up) Add hourly support It seemed strange to only add support for Daily loans in the feature when Koha also support loan periods in hours. This patch adds parallel functionaliy for hourly loan periods. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84721 [details] [review] Bug 20912: (QA follow-up) Fix TestBuilder Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84722 [details] [review] Bug 20912: (QA follow-up) Fix some test failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 84723 [details] [review] Bug 20912: (QA follow-up) POD fix Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 84724 [details] [review] Bug 20912: (QA follow-up) The default rule already has renewalsallowed=0 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Kyle implemented the things we suggested on QA, and Martin and I discussed the changes he made. We are all on the same page about this.
(In reply to Tomás Cohen Arazi from comment #91) > Kyle implemented the things we suggested on QA, and Martin and I discussed > the changes he made. We are all on the same page about this. Great, thx!
Created attachment 85291 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85292 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days an item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85293 [details] [review] Bug 20912: Update schema file Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85294 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85295 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85296 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85297 [details] [review] Bug 20912: Move calculation to Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85298 [details] [review] Bug 20912: (QA follow-up): Fix up Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85299 [details] [review] Bug 20912: (QA follow-up): Fix up unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85300 [details] [review] Bug 20912: (QA follow-up) Move Fees to Charges:: In preparation for the introduction of Koha::Charges::Fines I have moved this ::Fees class into the Koha::Charges:: namespace Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85301 [details] [review] Bug 20912: (QA follow-up) Test for distinct accountlines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85302 [details] [review] Bug 20912: (QA follow-up) Make DB Update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85303 [details] [review] Bug 20912: (follow-up) Improve test coverage Increase test coverage for CanBookBeIssued and fix a introduced during the refactoring to Koha::Fees. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85304 [details] [review] Bug 20912: (follow-up) Add hourly support It seemed strange to only add support for Daily loans in the feature when Koha also support loan periods in hours. This patch adds parallel functionaliy for hourly loan periods. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85305 [details] [review] Bug 20912: (QA follow-up) Fix TestBuilder Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 85306 [details] [review] Bug 20912: (QA follow-up) Fix some test failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 85307 [details] [review] Bug 20912: (QA follow-up) POD fix Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85308 [details] [review] Bug 20912: (QA follow-up) The default rule already has renewalsallowed=0 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Not strictly a blocker for 14825, but I believe it will cause a requirement to rebase or at least revisit the affected routine for the add_devit/add_credit conversion tree so I'm adding it there to aid my own tracking.
Couple things: 1 - Early renewals: The patron is charged for the full checkout duration on each renewal - I think they should only be charged for the additional days? or at least only for the renewal period? - if renewal period base is 'current day' and the checkout is not extended by the renewal I think they should not be charged, or charged only the renewal period - if the renewal period base is 'old due date' then they are charged for only added days 2 - The OPAC patron summary lists any general rental fees, but does not list the periodic rental fees - ideally I think the patron would have to confirm acceptance of charges before completing the transaction, but warning ahead of time is needed at a minimum - I can file a new bug for confirmation of renewing with rental charges
Created attachment 85316 [details] [review] Bug 20912: (follow-up) Prevent double charging on renewal This patch updates the tests the ensure we do not double charge for renewals that take place before the original due date and fixes the corresponding loging in C4::Circulation::AddRenewal. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Followup added to fix the first issue Nick found.. can't believe that got through so many QA eyes.. it was even incorrectly tested for in the tests, highlighting the issue. I'm not sure I fully understand your second comment though Nick... is that something that can go in a second bug with a clear description of what you think should happen?
Created attachment 85543 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85544 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days an item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85545 [details] [review] Bug 20912: Update schema file Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85546 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85547 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85548 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85549 [details] [review] Bug 20912: Move calculation to Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85550 [details] [review] Bug 20912: (QA follow-up): Fix up Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85551 [details] [review] Bug 20912: (QA follow-up): Fix up unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85552 [details] [review] Bug 20912: (QA follow-up) Move Fees to Charges:: In preparation for the introduction of Koha::Charges::Fines I have moved this ::Fees class into the Koha::Charges:: namespace Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85553 [details] [review] Bug 20912: (QA follow-up) Test for distinct accountlines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85554 [details] [review] Bug 20912: (QA follow-up) Make DB Update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85555 [details] [review] Bug 20912: (follow-up) Improve test coverage Increase test coverage for CanBookBeIssued and fix a introduced during the refactoring to Koha::Fees. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85556 [details] [review] Bug 20912: (follow-up) Add hourly support It seemed strange to only add support for Daily loans in the feature when Koha also support loan periods in hours. This patch adds parallel functionaliy for hourly loan periods. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85557 [details] [review] Bug 20912: (QA follow-up) Fix TestBuilder Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 85558 [details] [review] Bug 20912: (QA follow-up) Fix some test failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 85559 [details] [review] Bug 20912: (QA follow-up) POD fix Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85560 [details] [review] Bug 20912: (QA follow-up) The default rule already has renewalsallowed=0 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85561 [details] [review] Bug 20912: (follow-up) Prevent double charging on renewal This patch updates the tests the ensure we do not double charge for renewals that take place before the original due date and fixes the corresponding loging in C4::Circulation::AddRenewal. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Minor rebase to fit it on top of the add_debits accounts bug tree.
(In reply to Nick Clemens from comment #112) > 2 - The OPAC patron summary lists any general rental fees, but does not list > the periodic rental fees - ideally I think the patron would have to confirm > acceptance of charges before completing the transaction, but warning ahead > of time is needed at a minimum - I can file a new bug for confirmation of > renewing with rental charges OK, I understand now.. I'll mark it back to failed qa.. hopefully one of us will get to it some time soon. In short, I think we need to add Koha::Charges::Fees into opac/opac-user.pl to add in predicted renewal fees for a period based fee so that they display in the checkouts table where the user can opt to renew items.
(In reply to Martin Renvoize from comment #135) > (In reply to Nick Clemens from comment #112) > > 2 - The OPAC patron summary lists any general rental fees, but does not list > > the periodic rental fees - ideally I think the patron would have to confirm > > acceptance of charges before completing the transaction, but warning ahead > > of time is needed at a minimum - I can file a new bug for confirmation of > > renewing with rental charges > > OK, I understand now.. I'll mark it back to failed qa.. hopefully one of us > will get to it some time soon. > > In short, I think we need to add Koha::Charges::Fees into opac/opac-user.pl > to add in predicted renewal fees for a period based fee so that they display > in the checkouts table where the user can opt to renew items. Does the existing rental charge do this?
Yup, nick even showed me a screenshot 😉. It's not obvious though to be fair, it's just a note within renew field of the table. It's says "Fee for item type 'Book': 1.00" - https://snag.gy/MWntYk.jpg
(In reply to Martin Renvoize from comment #137) > Yup, nick even showed me a screenshot 😉. It's not obvious though to be > fair, it's just a note within renew field of the table. It's says "Fee for > item type 'Book': 1.00" - https://snag.gy/MWntYk.jpg I think this was added not too long ago, which might explain it not being well known: Bug 13796 - Alert in OPAC when renewing an item with a rental charge (signed by Kyle :) ) I agree that information/confirmation is needed before charging from a user's point of view, but I think also probably legally in some cases. I remember discussions here about labelling buttons for ordering clearly. Better to be safe.
Created attachment 85628 [details] [review] Bug 20912: (QA follow-up) Warn patron of daily and hourly renewal fees
Created attachment 85631 [details] [review] Bug 20912: (QA follow-up) Warn patron of daily and hourly renewal fees
Created attachment 86140 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86141 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days an item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86142 [details] [review] Bug 20912: Update schema file Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86143 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86144 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86145 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86146 [details] [review] Bug 20912: Move calculation to Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86147 [details] [review] Bug 20912: (QA follow-up): Fix up Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86148 [details] [review] Bug 20912: (QA follow-up): Fix up unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86149 [details] [review] Bug 20912: (QA follow-up) Move Fees to Charges:: In preparation for the introduction of Koha::Charges::Fines I have moved this ::Fees class into the Koha::Charges:: namespace Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86151 [details] [review] Bug 20912: (QA follow-up) Test for distinct accountlines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86152 [details] [review] Bug 20912: (QA follow-up) Make DB Update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86153 [details] [review] Bug 20912: (follow-up) Improve test coverage Increase test coverage for CanBookBeIssued and fix a introduced during the refactoring to Koha::Fees. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86154 [details] [review] Bug 20912: (follow-up) Add hourly support It seemed strange to only add support for Daily loans in the feature when Koha also support loan periods in hours. This patch adds parallel functionaliy for hourly loan periods. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86155 [details] [review] Bug 20912: (QA follow-up) Fix TestBuilder Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86156 [details] [review] Bug 20912: (QA follow-up) Fix some test failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86157 [details] [review] Bug 20912: (QA follow-up) POD fix Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86158 [details] [review] Bug 20912: (QA follow-up) The default rule already has renewalsallowed=0 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86159 [details] [review] Bug 20912: (follow-up) Prevent double charging on renewal This patch updates the tests the ensure we do not double charge for renewals that take place before the original due date and fixes the corresponding loging in C4::Circulation::AddRenewal. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86160 [details] [review] Bug 20912: (QA follow-up) Warn patron of daily and hourly renewal fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86244 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86245 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days an item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86246 [details] [review] Bug 20912: Update schema file Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86247 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86248 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86249 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86250 [details] [review] Bug 20912: Move calculation to Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86251 [details] [review] Bug 20912: (QA follow-up): Fix up Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86252 [details] [review] Bug 20912: (QA follow-up): Fix up unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86253 [details] [review] Bug 20912: (QA follow-up) Move Fees to Charges:: In preparation for the introduction of Koha::Charges::Fines I have moved this ::Fees class into the Koha::Charges:: namespace Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86254 [details] [review] Bug 20912: (QA follow-up) Test for distinct accountlines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86255 [details] [review] Bug 20912: (QA follow-up) Make DB Update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86256 [details] [review] Bug 20912: (follow-up) Improve test coverage Increase test coverage for CanBookBeIssued and fix a introduced during the refactoring to Koha::Fees. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86257 [details] [review] Bug 20912: (follow-up) Add hourly support It seemed strange to only add support for Daily loans in the feature when Koha also support loan periods in hours. This patch adds parallel functionaliy for hourly loan periods. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86258 [details] [review] Bug 20912: (QA follow-up) Fix TestBuilder Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86259 [details] [review] Bug 20912: (QA follow-up) Fix some test failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86260 [details] [review] Bug 20912: (QA follow-up) POD fix Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86261 [details] [review] Bug 20912: (QA follow-up) The default rule already has renewalsallowed=0 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86262 [details] [review] Bug 20912: (follow-up) Prevent double charging on renewal This patch updates the tests the ensure we do not double charge for renewals that take place before the original due date and fixes the corresponding loging in C4::Circulation::AddRenewal. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86263 [details] [review] Bug 20912: (QA follow-up) Warn patron of daily and hourly renewal fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86264 [details] [review] Bug 20912: (QA follow-up) Rebase error corrections The automatic rebase after bug 21206 required a helping hand. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Fresh rebase, should all be working again now.
Created attachment 86282 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86283 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days an item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86284 [details] [review] Bug 20912: Update schema file Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86285 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86286 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86287 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86288 [details] [review] Bug 20912: Move calculation to Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86289 [details] [review] Bug 20912: (QA follow-up): Fix up Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86290 [details] [review] Bug 20912: (QA follow-up): Fix up unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86291 [details] [review] Bug 20912: (QA follow-up) Move Fees to Charges:: In preparation for the introduction of Koha::Charges::Fines I have moved this ::Fees class into the Koha::Charges:: namespace Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86292 [details] [review] Bug 20912: (QA follow-up) Test for distinct accountlines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86293 [details] [review] Bug 20912: (QA follow-up) Make DB Update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86294 [details] [review] Bug 20912: (follow-up) Improve test coverage Increase test coverage for CanBookBeIssued and fix a introduced during the refactoring to Koha::Fees. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86295 [details] [review] Bug 20912: (follow-up) Add hourly support It seemed strange to only add support for Daily loans in the feature when Koha also support loan periods in hours. This patch adds parallel functionaliy for hourly loan periods. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86296 [details] [review] Bug 20912: (QA follow-up) Fix TestBuilder Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86297 [details] [review] Bug 20912: (QA follow-up) Fix some test failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86298 [details] [review] Bug 20912: (QA follow-up) POD fix Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86299 [details] [review] Bug 20912: (QA follow-up) The default rule already has renewalsallowed=0 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86300 [details] [review] Bug 20912: (follow-up) Prevent double charging on renewal This patch updates the tests the ensure we do not double charge for renewals that take place before the original due date and fixes the corresponding loging in C4::Circulation::AddRenewal. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86301 [details] [review] Bug 20912: (QA follow-up) Warn patron of daily and hourly renewal fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86302 [details] [review] Bug 20912: (QA follow-up) Rebase error corrections The automatic rebase after bug 21206 required a helping hand. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86306 [details] [review] Bug 20912: Update database Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86307 [details] [review] Bug 20912: Rental Fees based on Time Period Some libraries would like to be able to charge a rental fee based on the number of days an item will be checked out, as opposed to the flat fee currently offered by Koha. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Edit an itemtype, add a daily rental fee of 1.00 4) Check an item of that itemtype out for 7 days 5) Verify the patron now has rental fee of 7.00 Signed-off-by: Matha Fuerst <mfuerst@hmcpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86308 [details] [review] Bug 20912: Update schema file Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86309 [details] [review] Bug 20912: (QA follow-up) Fix number of tests and correct typos Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86310 [details] [review] Bug 20912: (QA follow-up) Set daily charge to 0 for item type not being used for test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86311 [details] [review] Bug 20912: (QA follow-up) Make unit tests reliable and get rid of perl warnings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86312 [details] [review] Bug 20912: Move calculation to Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86313 [details] [review] Bug 20912: (QA follow-up): Fix up Koha::Fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86314 [details] [review] Bug 20912: (QA follow-up): Fix up unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86315 [details] [review] Bug 20912: (QA follow-up) Move Fees to Charges:: In preparation for the introduction of Koha::Charges::Fines I have moved this ::Fees class into the Koha::Charges:: namespace Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86316 [details] [review] Bug 20912: (QA follow-up) Test for distinct accountlines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86317 [details] [review] Bug 20912: (QA follow-up) Make DB Update idempotent Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86318 [details] [review] Bug 20912: (follow-up) Improve test coverage Increase test coverage for CanBookBeIssued and fix a introduced during the refactoring to Koha::Fees. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86319 [details] [review] Bug 20912: (follow-up) Add hourly support It seemed strange to only add support for Daily loans in the feature when Koha also support loan periods in hours. This patch adds parallel functionaliy for hourly loan periods. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86320 [details] [review] Bug 20912: (QA follow-up) Fix TestBuilder Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86321 [details] [review] Bug 20912: (QA follow-up) Fix some test failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86322 [details] [review] Bug 20912: (QA follow-up) POD fix Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86323 [details] [review] Bug 20912: (QA follow-up) The default rule already has renewalsallowed=0 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 86324 [details] [review] Bug 20912: (follow-up) Prevent double charging on renewal This patch updates the tests the ensure we do not double charge for renewals that take place before the original due date and fixes the corresponding loging in C4::Circulation::AddRenewal. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86325 [details] [review] Bug 20912: (QA follow-up) Warn patron of daily and hourly renewal fees Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86326 [details] [review] Bug 20912: (QA follow-up) Rebase error corrections The automatic rebase after bug 21206 required a helping hand. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86327 [details] [review] Bug 20912: (QA follow-up) Add on_editing to $Price filter Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Awesome work all! Pushed to master for 19.05
Enhancement will not be backported to 18.11.x series.
I'm afraid that this patch broke AddRenewal which now calculates wrong duedate because of: - $datedue = CalcDateDue($datedue, $itemtype, _GetCircControlBranch($item_unblessed, $patron_unblessed), $patron_unblessed, 'is a renewal'); + $datedue = CalcDateDue($datedue, $itemtype, $circ_library, $patron_unblessed, 'is a renewal'); This will pass $circ_library object instead of branch name so $duedate will be calculated wrongly.
Dobrica, long time no see.. nice to have you back. Very annoying this wasn't caught by a test.. I'll open it in a new bug as I think it'll only add to the confusion adding followups here.
Created attachment 87363 [details] [review] Bug 20912: (QA follow-up) Fix for missed signature failure We missed a change in calling parameters passed to CalcDateDue in AddRenewal during the initial QAing. This patch corrects the call and adds a test to catch regressions. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87413 [details] [review] Bug 20912: (QA follow-up) Fix for missed signature failure We missed a change in calling parameters passed to CalcDateDue in AddRenewal during the initial QAing. This patch corrects the call and adds a test to catch regressions. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 87703 [details] [review] Bug 20912: (QA follow-up) Fix for missed signature failure
Thanks for patch, I signed-off on it.
(In reply to Dobrica Pavlinusic from comment #232) > Created attachment 87703 [details] [review] [review] > Bug 20912: (QA follow-up) Fix for missed signature failure Awesome work all! follow-up pushed to master for 19.05