From 1813f9f4ff4d48ad6bf76d70c01edd67c5a4bfa4 Mon Sep 17 00:00:00 2001 From: Mark Wells Date: Mon, 7 Sep 2015 17:40:30 -0700 Subject: [PATCH] rework discount calculation, #20613, #19173, #19354 --- FS/FS/part_pkg/discount_Mixin.pm | 196 +++++++++++++++++++++++---------------- 1 file changed, 115 insertions(+), 81 deletions(-) diff --git a/FS/FS/part_pkg/discount_Mixin.pm b/FS/FS/part_pkg/discount_Mixin.pm index abde93f8f..5de7d8ea5 100644 --- a/FS/FS/part_pkg/discount_Mixin.pm +++ b/FS/FS/part_pkg/discount_Mixin.pm @@ -28,11 +28,15 @@ sub calc_recur { =head METHODS -=item calc_discount +=item calc_discount CUST_PKG, SDATE, DETAILS_ARRAYREF, PARAM_HASHREF -Takes all the arguments of calc_recur. Calculates and returns the amount -by which to reduce the recurring fee; also increments months used on the -discount. +Takes all the arguments of calc_recur. Calculates and returns the amount +by which to reduce the charge; also increments months used on the discount. + +If there is a setup fee, this will be called once with 'setup_charge' => the +setup fee amount (and should return the discount to be applied to the setup +charge, if any), and again without it (for the recurring fee discount). +PARAM_HASHREF carries over between the two invocations. =cut @@ -40,9 +44,6 @@ sub calc_discount { my($self, $cust_pkg, $sdate, $details, $param ) = @_; my $conf = new FS::Conf; - my $br_permonth = $self->base_recur_permonth($cust_pkg, $sdate); - $br_permonth += $param->{'override_charges'} if $param->{'override_charges'}; - my $br = $self->base_recur($cust_pkg, $sdate); $br += $param->{'override_charges'} * ($cust_pkg->part_pkg->freq || 0) if $param->{'override_charges'}; @@ -83,52 +84,125 @@ sub calc_discount { my $discount_left; my $discount = $cust_pkg_discount->discount; #UI enforces one or the other (for now? probably for good) + # $chg_months: the number of months we are charging recur for + # $months: $chg_months or the months left on the discount, whchever is less + + my $chg_months = $cust_pkg->part_pkg->freq || 1; + if ( defined($param->{'months'}) ) { # then override + $chg_months = $param->{'months'}; + } + + my $months = $chg_months; + if ( $discount->months ) { + $months = min( $chg_months, + $discount->months - $cust_pkg_discount->months_used ); + } + + # $amount is now the (estimated) discount amount on the recurring charge. + # if it's a percent discount, that's base recur * percentage. + my $amount = 0; - $amount += $discount->amount - if $cust_pkg->pkgpart == $param->{'real_pkgpart'}; - $amount += sprintf('%.2f', $discount->percent * $br_permonth / 100 ); # FIXME: should this use $br / $freq to avoid rounding errors? - my $chg_months = defined($param->{'months'}) ? - $param->{'months'} : - $cust_pkg->part_pkg->freq; - - my $months = $discount->months - ? min( $chg_months, - $discount->months - $cust_pkg_discount->months_used ) - : $chg_months; if (defined $param->{'setup_charge'}) { + + # we are calculating the setup discount. + # if this discount doesn't apply to setup fees, skip it. + # if it's a percent discount, set $amount = percent * setup_charge. + # if it's a flat amount discount for one month: + # - if the discount amount > setup_charge, then set it to setup_charge, + # and set 'discount_left_recur' to the difference. + # - otherwise set it to just the discount amount. + # if it's a flat amount discount for other than one month: + # - skip the discount. unsure, leaving it alone for now. + next unless $discount->setup; + $months = 0; # never count a setup discount as a month of discount + # (the recur discount in the same month should do it) + if ( $discount->percent > 0 ) { - $amount = sprintf('%.2f', $discount->percent * $param->{'setup_charge'} / 100 ); - $months = 1; - } elsif ( $discount->amount > 0 && $discount->months == 1) { - $discount_left = $param->{'setup_charge'} - $discount->amount; - $amount = $param->{'setup_charge'} if $discount_left < 0; - $amount = $discount->amount if $discount_left >= 0; - $months = 1; - + $amount = $discount->percent * $param->{'setup_charge'} / 100; + } elsif ( $discount->amount > 0 && ($discount->months || 0) == 1) { + # apply the discount amount, up to a maximum of the setup charge + $amount = min($discount->amount, $param->{'setup_charge'}); + $discount_left = sprintf('%.2f', $discount->amount - $amount); # transfer remainder of discount, if any, to recur - $param->{'discount_left_recur'}{$discount->discountnum} = - 0 - $discount_left if $discount_left < 0; + $param->{'discount_left_recur'}{$discount->discountnum} = $discount_left; } else { + # I guess we don't allow multiple-month flat amount discounts to + # apply to setup? next; } - } elsif ( defined $param->{'discount_left_recur'}{$discount->discountnum} - && $param->{'discount_left_recur'}{$discount->discountnum} > 0 - ) { - # use up transferred remainder of discount from setup + + } else { + + # we are calculating a recurring fee discount. estimate the recurring + # fee: + # XXX it would be more accurate for calc_recur to just _tell us_ what + # it's going to charge + + my $recur_charge = $br * ($cust_pkg->quantity || 1) * $chg_months / $self->freq; + # round this, because the real recur charge is rounded + $recur_charge = sprintf('%.2f', $recur_charge); + + # if it's a percentage discount, calculate it based on that estimate. + # otherwise use the flat amount. + + if ( $discount->percent > 0 ) { + $amount = $recur_charge * $discount->percent / 100; + } elsif ( $discount->amount > 0 + and $cust_pkg->pkgpart == $param->{'real_pkgpart'} ) { + $amount = $discount->amount * $months; + } + + if ( exists $param->{'discount_left_recur'}{$discount->discountnum} ) { + # there is a discount_left_recur entry for this discountnum, so this + # is the second (recur) pass on the discount. use up transferred + # remainder of discount from setup. + # + # note that discount_left_recur can now be zero. $amount = $param->{'discount_left_recur'}{$discount->discountnum}; $param->{'discount_left_recur'}{$discount->discountnum} = 0; - $months = 1; - } elsif ( $discount->setup - && $discount->months == 1 - && $discount->amount > 0 - ) { - next; - } + $months = 1; # XXX really? not $chg_months? + } + #elsif ( $discount->setup + # && ($discount->months || 0) == 1 + # && $discount->amount > 0 + # ) { + # next; + # + # RT #11512: bugfix to prevent applying flat discount to both setup + # and recur. The original implementation ignored discount_left_recur + # if it was zero, so if the setup fee used up the entire flat + # discount, the recurring charge would get to use the entire flat + # discount also. This bugfix was a kludge. Instead, we now allow + # discount_left_recur to be zero in that case, and then the available + # recur discount is zero. + #} + + # transfer remainder of discount, if any, to setup + # this is used when the recur phase wants to add a setup fee + # (prorate_defer_bill): the "discount_left_setup" amount will + # be subtracted in _make_lines. + if ( $discount->setup && $discount->amount > 0 + && ($discount->months || 0) != 1 + ) + { + # $amount is no longer permonth at this point! correct. very good. + $discount_left = $amount - $recur_charge; # backward, as above + if ( $discount_left > 0 ) { + $amount = $recur_charge; + $param->{'discount_left_setup'}{$discount->discountnum} = + 0 - $discount_left; + } + } - if ( ! defined $param->{'setup_charge'} ) { + # cap the discount amount at the recur charge + $amount = min($amount, $recur_charge); + + # if this is the base pkgpart, schedule increment_months_used to run at + # the end of billing. (addon packages haven't been calculated yet, so + # don't let the discount expire during the billing process. RT#17045.) if ( $cust_pkg->pkgpart == $param->{'real_pkgpart'} ) { push @{ $param->{precommit_hooks} }, sub { my $error = $cust_pkg_discount->increment_months_used($months); @@ -136,62 +210,22 @@ sub calc_discount { }; } - $amount = min($amount * $months, $br); } $amount = sprintf('%.2f', $amount + 0.00000001 ); #so 1.005 rounds to 1.01 next unless $amount > 0; - # transfer remainder of discount, if any, to setup - if ( $discount->setup && $discount->amount > 0 - && (!$discount->months || $discount->months != 1) - && !defined $param->{'setup_charge'} - ) - { - $discount_left = $br_permonth - $amount; # FIXME: $amount is no longer permonth at this point! - if ( $discount_left < 0 ) { - $amount = $br_permonth; # FIXME: seems like this should *= $months - $param->{'discount_left_setup'}{$discount->discountnum} = - 0 - $discount_left; - } - } - #record details in cust_bill_pkg_discount my $cust_bill_pkg_discount = new FS::cust_bill_pkg_discount { 'pkgdiscountnum' => $cust_pkg_discount->pkgdiscountnum, 'amount' => $amount, 'months' => $months, + # XXX should have a 'setuprecur' }; push @{ $param->{'discounts'} }, $cust_bill_pkg_discount; $tot_discount += $amount; - #add details on discount to invoice - # no longer! this is now done during rendering based on the existence - # of the cust_bill_pkg_discount record - # - #my $money_char = $conf->config('money_char') || '$'; - #$months = sprintf('%.2f', $months) if $months =~ /\./; - - #my $d = 'Includes '; - #my $format; - - #if ( $months eq '1' ) { - # $d .= "discount of $money_char$amount"; - # $d .= " each" if $cust_pkg->quantity > 1; - # $format = 'Undiscounted amount: %s%.2f'; - #} else { - # $d .= 'setup ' if defined $param->{'setup_charge'}; - # $d .= 'discount of '. $discount->description_short; - # $d .= " for $months months" - # unless defined $param->{'setup_charge'}; - # $d .= ": $money_char$amount" if $discount->percent; - # $format = 'Undiscounted monthly amount: %s%.2f'; - #} - - #push @$details, $d; - #push @$details, sprintf( $format, $money_char, $br_permonth ); - } sprintf('%.2f', $tot_discount); -- 2.11.0