[freeside-commits] branch FREESIDE_3_BRANCH updated. 1d8cf0f25e468b871d18962bda022c94d2934f02
Mark Wells
mark at 420.am
Mon Sep 7 23:14:03 PDT 2015
The branch, FREESIDE_3_BRANCH has been updated
via 1d8cf0f25e468b871d18962bda022c94d2934f02 (commit)
via 7ac9115109599df55f55a52c83204f0ac4e0b00c (commit)
from 3220d99fa3ef70ca60cc7218533401da1990f4e0 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 1d8cf0f25e468b871d18962bda022c94d2934f02
Author: Mark Wells <mark at freeside.biz>
Date: Mon Sep 7 17:40:30 2015 -0700
rework discount calculation, #20613, #19173, #19354
diff --git a/FS/FS/part_pkg/discount_Mixin.pm b/FS/FS/part_pkg/discount_Mixin.pm
index abde93f..5de7d8e 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);
commit 7ac9115109599df55f55a52c83204f0ac4e0b00c
Author: Mark Wells <mark at freeside.biz>
Date: Mon Sep 7 17:40:25 2015 -0700
"1 months", eww
diff --git a/FS/FS/discount.pm b/FS/FS/discount.pm
index 1491139..dd69604 100644
--- a/FS/FS/discount.pm
+++ b/FS/FS/discount.pm
@@ -198,7 +198,13 @@ sub description {
( my $months = $self->months ) =~ s/\.0+$//;
$months =~ s/(\.\d*[1-9])0+$/$1/;
- $desc .= " for $months months" if $months;
+ if ($months) {
+ if ($months == 1) {
+ $desc .= " for 1 month";
+ } else {
+ $desc .= " for $months months";
+ }
+ }
$desc .= ', applies to setup' if $self->setup;
-----------------------------------------------------------------------
Summary of changes:
FS/FS/discount.pm | 8 +-
FS/FS/part_pkg/discount_Mixin.pm | 196 ++++++++++++++++++++++----------------
2 files changed, 122 insertions(+), 82 deletions(-)
More information about the freeside-commits
mailing list