Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

HasCharges collect LineItems; charges are items

This is a big commit, but it seemed best to keep it all together,
rather than try too hard to break it into non-test-breaking pieces.

* HasCharges means the object has a set of LineItems
* charges are a kind of line item
* counts_toward_total is gone; is_abandoned is used directly

This clears up a lot of otherwise confusing code where a LineItem did
the Charge role, but $line_item->is_charge was false.  Now there is no
abstract Charge, only InvoiceCharge and JournalCharge, each of which is
a kind of line item.

All line items count toward the total, unless abandonable and abandoned.
Line items can also be zero-amount, or even negative, now, though, so we
can add notices and discount items to invoices.

The CouponCreator code has been removed.  It was also removed on another
branch, so I've removed it here rather than updating it to the new
LineItem code.
  • Loading branch information...
commit 21c657b33f908f6822c49447f04b9bf3bbe7b672 1 parent cbe33e0
@rjbs rjbs authored
View
53 lib/Moonpig/Role/ChargeLike.pm
@@ -1,53 +0,0 @@
-package Moonpig::Role::ChargeLike;
-# ABSTRACT: something that can go in an invoice or a journal
-use Moose::Role;
-
-use MooseX::Types::Moose qw(Str);
-use Moonpig;
-use Moonpig::Types qw(Millicents TagSet);
-
-use Moonpig::Behavior::Packable;
-
-use namespace::autoclean;
-
-requires 'check_amount';
-requires 'counts_toward_total';
-requires 'is_charge'; # Is it a real charge, or something like an explanatory line item?
-
-with ('Moonpig::Role::HasTagset' => {});
-
-has description => (
- is => 'ro',
- isa => Str,
- required => 1,
-);
-
-has amount => (
- is => 'ro',
- isa => Millicents,
- coerce => 1,
- required => 1,
- trigger => sub {
- my ($self, $newval) = @_;
- $self->check_amount($newval);
- },
-);
-
-has date => (
- is => 'ro',
- isa => 'DateTime',
- default => sub { Moonpig->env->now() },
-);
-
-PARTIAL_PACK {
- my ($self) = @_;
-
- return {
- description => $self->description,
- amount => $self->amount,
- date => $self->date,
- tags => [ $self->taglist ],
- };
-};
-
-1;
View
1  lib/Moonpig/Role/Consumer.pm
@@ -515,6 +515,7 @@ sub charge_current_journal {
$args->{extra_tags} ||= [];
$args->{tags} ||= [ @{$self->journal_charge_tags}, @extra_tags ];
$args->{date} ||= Moonpig->env->now;
+ $args->{consumer} = $self;
$self->apply_coupons_to_charge_args($args); # Could modify amount, desc., etc.
return $self->ledger->current_journal->charge($args);
View
25 lib/Moonpig/Role/HasCharges.pm
@@ -10,7 +10,7 @@ use Moonpig::Types;
use Moonpig::Util qw(class sumof);
use Moose::Util::TypeConstraints;
use MooseX::Types::Moose qw(ArrayRef);
-use Moonpig::Types qw(Time);
+use Moonpig::Types qw(LineItem Time);
parameter charge_role => (
isa => enum([qw(InvoiceCharge JournalCharge)]),
@@ -22,24 +22,25 @@ role {
requires 'accepts_charge';
- # This is a misnomer, since it might not yield only charges, but anything chargelike.
+ # This is a misnomer, since it might not yield only charges, but any line
+ # item.
# We did not want to have to modify the existing database. mjd 2012-07-12
has charges => (
is => 'ro',
- isa => ArrayRef[ "Moonpig::Types::ChargeLike" ],
+ isa => ArrayRef[ LineItem ],
init_arg => undef,
default => sub { [] },
traits => [ 'Array' ],
handles => {
all_items => 'elements',
has_items => 'count',
- _add_charge => 'push',
+ _add_item => 'push',
_add_item => 'push',
},

Now you have two _add_items. I had put in _add_item as a synonym for _add_charge.
I still think that add_charge reads better in some cases, but I think it's not better enough to justify the unnecessary multiplication of entities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
);
method all_charges => sub {
- return grep $_->is_charge, $_[0]->all_items;
+ return $_[0]->all_items; # any reason at all to make this filter?
};

I'm surprised if we don't need it, but perhaps we don't need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
method has_charges => sub {
@@ -47,8 +48,16 @@ role {
};
method total_amount => sub {
- my @charges = grep $_->counts_toward_total, $_[0]->all_charges;
- sumof { $_->amount } @charges;
+ sumof { $_->amount } $_[0]->unabandoned_items;
+ };
+
+ method unabandoned_items => sub {
+ my @items = grep {
+ ! $_->does('Moonpig::Role::LineItem::Abandonable')
+ || ! $_->is_abandoned
+ } $_[0]->all_items;

I suggest using ! ( $_->does(…) && $_->is_abandoned ). It makes clearer that the does test is just to establish validity for the following is_abandoned test, which is the real point of the condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ return @items;
};
method _objectify_charge => sub {
@@ -68,7 +77,7 @@ role {
Moonpig::X->throw("bad charge type")
unless $self->accepts_charge($charge);
- $self->_add_charge($charge);
+ $self->_add_item($charge);
return $charge;
};
View
6 lib/Moonpig/Role/Invoice.pm
@@ -129,7 +129,7 @@ sub abandon_with_replacement {
# XXX This discards non-charge items. Is that correct? mjd 2012-07-11
for my $charge (grep ! $_->is_abandoned, $self->all_charges) {
- $new_invoice->_add_charge($charge);
+ $new_invoice->_add_item($charge);
}
$self->abandoned_in_favor_of($new_invoice->guid)
@@ -140,7 +140,7 @@ sub abandon_with_replacement {
return $new_invoice;
}
-sub add_line_item { $_[0]->_add_charge($_[1]) }
+sub add_line_item { $_[0]->_add_item($_[1]) }
sub abandon_without_replacement { $_[0]->abandon_with_replacement(undef) }
@@ -164,7 +164,7 @@ sub _pay_charges {
my ($self, $event) = @_;
# Include non-charge items, and charges that are not abandoned
- my @items = grep { ! ($_->is_charge && $_->is_abandoned) } $self->all_items;
+ my @items = $self->unabandoned_items;
my $collection = $self->ledger->consumer_collection;
my @guids = uniq map { $_->owner_guid } @items;
View
24 lib/Moonpig/Role/InvoiceCharge.pm
@@ -3,10 +3,10 @@ use Moose::Role;
# ABSTRACT: a charge placed on an invoice
with(
- 'Moonpig::Role::ChargeLike',
- 'Moonpig::Role::ConsumerComponent',
+ 'Moonpig::Role::LineItem',
+ 'Moonpig::Role::LineItem::Abandonable',
+ 'Moonpig::Role::LineItem::RequiresPositiveAmount',
'Moonpig::Role::HandlesEvents',
- 'Moonpig::Role::ChargeLike::RequiresPositiveAmount',
);
use namespace::autoclean;
@@ -15,23 +15,6 @@ use Moonpig::Behavior::Packable;
use Moonpig::Types qw(Time);
use MooseX::SetOnce;
-has abandoned_at => (
- is => 'ro',
- isa => Time,
- predicate => 'is_abandoned',
- writer => '__set_abandoned_at',
- traits => [ qw(SetOnce) ],
-);
-
-sub counts_toward_total { ! $_[0]->is_abandoned }
-sub is_charge { 1 }
-
-sub mark_abandoned {
- my ($self) = @_;
- Moonpig::X->throw("can't abandon an executed charge") if $self->is_executed;
- $self->__set_abandoned_at( Moonpig->env->now );
-}
-
has executed_at => (
is => 'ro',
isa => Time,
@@ -54,7 +37,6 @@ PARTIAL_PACK {
return {
owner_guid => $self->owner_guid,
executed_at => $self->executed_at,
- abandoned_at => $self->abandoned_at,
};
};
View
43 lib/Moonpig/Role/InvoiceCharge/CouponCreator.pm
@@ -1,43 +0,0 @@
-package Moonpig::Role::InvoiceCharge::CouponCreator;
-# ABSTRACT: a charge that, when paid, should have a coupon created
-use Moose::Role;
-
-with(
- 'Moonpig::Role::InvoiceCharge::Active',
-);
-
-use Moonpig;
-use Moonpig::Logger '$Logger';
-use Moonpig::Types qw(Factory GUID Ledger);
-use Moonpig::Util qw(class);
-use MooseX::Types::Moose qw(HashRef);
-
-use namespace::autoclean;
-
-has coupon_class => (
- is => 'ro',
- isa => Factory,
- required => 1,
-);
-
-has coupon_args => (
- is => 'ro',
- isa => HashRef,
- default => sub { {} },
-);
-
-sub when_paid {
- # Create the coupon
- my ($self, $event) = @_;
-
- my $coupon;
- Moonpig->env->storage->do_rw(
- sub {
- $coupon = $self->ledger->add_coupon($self->coupon_class, $self->coupon_args);
- $Logger->log([ 'created coupon %s in ledger %s', $coupon->ident, $self->ledger->ident ]);
- Moonpig->env->storage->save_ledger($self->ledger);
- });
- return $coupon;
-}
-
-1;
View
3  lib/Moonpig/Role/Journal.pm
@@ -32,7 +32,7 @@ sub charge {
my ($self, $args) = @_;
{ my $FAIL = "";
- for my $reqd (qw(from to amount desc tags)) {
+ for my $reqd (qw(from to amount desc tags consumer)) {
$FAIL .= __PACKAGE__ . "::charge missing required '$reqd' argument"
unless $args->{$reqd};
}
@@ -49,6 +49,7 @@ sub charge {
});
my $charge = $self->charge_factory->new({
+ consumer => $args->{consumer},
description => $args->{desc},
amount => $args->{amount},
date => $args->{when} || Moonpig->env->now(),
View
7 lib/Moonpig/Role/JournalCharge.pm
@@ -2,13 +2,10 @@ package Moonpig::Role::JournalCharge;
use Moose::Role;
# ABSTRACT: a charge placed on an journal
with(
- 'Moonpig::Role::ChargeLike',
- 'Moonpig::Role::ChargeLike::RequiresPositiveAmount',
+ 'Moonpig::Role::LineItem',
+ 'Moonpig::Role::LineItem::RequiresPositiveAmount',
);
use namespace::autoclean;
-sub counts_toward_total { 1 }
-sub is_charge { 1 }
-
1;
View
59 lib/Moonpig/Role/LineItem.pm
@@ -1,13 +1,54 @@
package Moonpig::Role::LineItem;
-# ABSTRACT: a non-charge line item on an invoice
+# ABSTRACT: something that can go in an invoice or a journal
use Moose::Role;
-with ('Moonpig::Role::ChargeLike',
- 'Moonpig::Role::ConsumerComponent',
- 'Moonpig::Role::HandlesEvents',
- );
-
-sub check_amount { 1 }
-sub counts_toward_total { 0 }
-sub is_charge { 0 }
+
+use MooseX::Types::Moose qw(Str);
+use Moonpig;
+use Moonpig::Types qw(Millicents TagSet);
+
+use Moonpig::Behavior::Packable;
+
+use namespace::autoclean;
+
+requires 'check_amount';
+
+with(
+ 'Moonpig::Role::HasTagset' => {},
+ 'Moonpig::Role::ConsumerComponent',
+);
+
+has description => (
+ is => 'ro',
+ isa => Str,
+ required => 1,
+);
+
+has amount => (
+ is => 'ro',
+ isa => Millicents,
+ coerce => 1,
+ required => 1,
+ trigger => sub {
+ my ($self, $newval) = @_;
+ $self->check_amount($newval);
+ },
+);
+
+has date => (
+ is => 'ro',
+ isa => 'DateTime',
+ default => sub { Moonpig->env->now() },
+);
+
+PARTIAL_PACK {
+ my ($self) = @_;
+
+ return {
+ description => $self->description,
+ amount => $self->amount,
+ date => $self->date,
+ tags => [ $self->taglist ],
+ };
+};
1;
View
36 lib/Moonpig/Role/LineItem/Abandonable.pm
@@ -0,0 +1,36 @@
+package Moonpig::Role::LineItem::Abandonable;
+use Moose::Role;
+# ABSTRACT: a charge placed on an invoice

fix ABSTRACT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+with(
+ 'Moonpig::Role::LineItem',
+);
+
+use namespace::autoclean;
+use Moonpig::Behavior::Packable;
+use Moonpig::Types qw(Time);
+use MooseX::SetOnce;
+
+has abandoned_at => (
+ is => 'ro',
+ isa => Time,
+ predicate => 'is_abandoned',
+ writer => '__set_abandoned_at',
+ traits => [ qw(SetOnce) ],
+);
+
+sub mark_abandoned {
+ my ($self) = @_;
+ Moonpig::X->throw("can't abandon an executed charge") if $self->is_executed;
+ $self->__set_abandoned_at( Moonpig->env->now );
+}
+
+PARTIAL_PACK {
+ my ($self) = @_;
+
+ return {
+ abandoned_at => $self->abandoned_at,

Perhaps $self->is_abandoned ? ( abandoned_at => $self->abandoned_at ) : () ?

@rjbs Owner
rjbs added a note

Could do, but this is just duplicating what we had before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ };
+};
+
+1;
View
7 lib/Moonpig/Role/ChargeLike/Active.pm → lib/Moonpig/Role/LineItem/Active.pm
@@ -1,8 +1,9 @@
-package Moonpig::Role::ChargeLike::Active;
+package Moonpig::Role::LineItem::Active;
use Moose::Role;
-# ABSTRACT: a charge that does something extra when paid
+# ABSTRACT: a line item that does something extra when paid
with(
- 'Moonpig::Role::ChargeLike',
+ 'Moonpig::Role::LineItem',
+ 'Moonpig::Role::HandlesEvents',
);
use namespace::autoclean;
View
17 lib/Moonpig/Role/LineItem/Discount.pm
@@ -0,0 +1,17 @@
+package Moonpig::Role::LineItem::Discount;
+use Moose::Role;
+# ABSTRACT: a charge that requires that its amount is negative
+use Moonpig::Types qw(NegativeMillicents);
+
+use namespace::autoclean;
+
+with(
+ 'Moonpig::Role::LineItem',
+);
+
+sub check_amount {
+ my ($self, $amount) = @_;
+ NegativeMillicents->assert_valid($amount);
+}
+
+1;
View
12 lib/Moonpig/Role/LineItem/Note.pm
@@ -0,0 +1,12 @@
+package Moonpig::Role::LineItem::Note;
+use Moose::Role;
+# ABSTRACT: a charge that requires that its amount is zero
+
+with(
+ 'Moonpig::Role::LineItem',
+ 'Moonpig::Role::LineItem::RequiresZeroAmount',
+);
+
+use namespace::autoclean;
+
+1;
View
3  lib/Moonpig/Role/LineItem/PsyncB5G1Magic.pm
@@ -6,7 +6,8 @@ use Moonpig::Types qw(PositiveMillicents);
use Moose::Role;
use Moonpig::Behavior::Packable;
with ('Moonpig::Role::LineItem',
- 'Moonpig::Role::ChargeLike::Active',
+ 'Moonpig::Role::LineItem::Active',
+ 'Moonpig::Role::LineItem::RequiresZeroAmount',
);
has adjustment_amount => (
View
2  ...Role/ChargeLike/RequiresPositiveAmount.pm → ...g/Role/LineItem/RequiresPositiveAmount.pm
@@ -1,4 +1,4 @@
-package Moonpig::Role::ChargeLike::RequiresPositiveAmount;
+package Moonpig::Role::LineItem::RequiresPositiveAmount;
use Moose::Role;
# ABSTRACT: a charge that requires that its amount is positive
use Moonpig::Types qw(PositiveMillicents);
View
13 lib/Moonpig/Role/LineItem/RequiresZeroAmount.pm
@@ -0,0 +1,13 @@
+package Moonpig::Role::LineItem::RequiresZeroAmount;
+use Moose::Role;
+# ABSTRACT: a charge that requires that its amount is zero
+use Moonpig::Types qw(ZeroMillicents);
+
+use namespace::autoclean;
+
+sub check_amount {
+ my ($self, $amount) = @_;
+ ZeroMillicents->assert_valid($amount);
+}
+
+1;
View
11 lib/Moonpig/Types.pm
@@ -3,11 +3,12 @@ package Moonpig::Types;
use MooseX::Types -declare => [ qw(
EmailAddresses
Ledger Consumer
- Millicents PositiveMillicents NonNegativeMillicents
+ Millicents PositiveMillicents NegativeMillicents NonNegativeMillicents
+ ZeroMillicents
Credit
- ChargeLike
+ LineItem
Invoice
Journal
@@ -59,7 +60,7 @@ role_type Ledger, { role => 'Moonpig::Role::Ledger' };
role_type Consumer, { role => 'Moonpig::Role::Consumer' };
-role_type ChargeLike, { role => 'Moonpig::Role::ChargeLike' };
+role_type LineItem, { role => 'Moonpig::Role::LineItem' };
role_type Invoice, { role => 'Moonpig::Role::Invoice' };
@@ -69,11 +70,15 @@ subtype PositiveInt, as Int, where { $_ > 0 };
subtype Millicents, as Int;
subtype PositiveMillicents, as Millicents, where { $_ > 0 };
+subtype NegativeMillicents, as Millicents, where { $_ < 0 };
subtype NonNegativeMillicents, as Millicents, where { $_ >= 0 };
+subtype ZeroMillicents, as Millicents, where { $_ == 0 };
coerce Millicents, from Num, via { int };
coerce PositiveMillicents, from Num, via { int };
coerce NonNegativeMillicents, from Num, via { int };
+coerce NegativeMillicents, from Num, via { int };
+coerce ZeroMillicents, from Num, via { int };
role_type Credit, { role => 'Moonpig::Role::Credit' };
View
3  lib/Moonpig/Util.pm
@@ -97,7 +97,8 @@ sub dollars {
my ($dollars) = @_;
my $millicents = $dollars * 100 * 1000;
- return int ($millicents + 0.5);
+ $millicents += 0.5 if $millicents > 0;
+ return int $millicents;
}

Seems like this should be something like return int($millicents + 0.5 * sgn($millicents)), where

    sub sgn { $_[0] > 0 ? 1 : $_[0] == 0 ? 0 : -1 }

Or we could do Moonpig::Util::round_to_integer that always rounds to the nearest integer, even when the argument is negative, since that is what we are really after.

Sadly, POSIX does not provide such a function. The GNU C library calls it trunc.

@rjbs Owner
rjbs added a note

Yeah, I wanted to implement the sign-based thing when I did this, but my brain said, "you are not in the right mindset to get this right," so I just pressed on. I'll use sprintf %f, which rounds correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
# returns unrounded fractional dollars
View
51 t/invoice/lineitem.t
@@ -30,27 +30,38 @@ test 'zero amounts' => sub {
}, undef, "$method with zero amount");
}
- ok(class("LineItem")->new({ amount => dollars(0),
- description => "lineitem zero",
- consumer => $c,
- }),
- "zero-amount line item");
-
- ok(class("LineItem")->new({ amount => dollars(-1),
- description => "lineitem zero",
- consumer => $c,
- }),
- "negative-amount line item");
-
- my $line_item = class("LineItem")->new({ amount => dollars(1),
- description => "lineitem",
- consumer => $c,
- });
-
- $ledger->current_invoice->add_charge($line_item);
+ my $note = class("LineItem::Note")->new({
+ amount => dollars(0),
+ description => "lineitem zero",
+ consumer => $c,
+ });
+
+ ok($note, "zero-amount line item");
+
+ my $discount = class("LineItem::Discount")->new({
+ amount => -100000, # using dollars(-1) failed!!
+ description => "lineitem zero",
+ consumer => $c,
+ });
+
+ ok($discount, "negative-amount line item");
+
+ my $charge = class("InvoiceCharge")->new({
+ amount => dollars(1),
+ description => "lineitem",
+ consumer => $c,
+ });
+
+ $ledger->current_invoice->add_charge($note);
+ $ledger->current_invoice->add_charge($discount);
+ $ledger->current_invoice->add_charge($charge);
+
my @all_items = $ledger->current_invoice->all_items;
- is(@all_items, 1, "added line item to current invoice");
- is($ledger->current_invoice->total_amount, dollars(0), "Line item doesn't count");
+ is(@all_items, 3, "added 3 line items to current invoice");
+
+ my @unab_items = $ledger->current_invoice->unabandoned_items;
+ is(@unab_items, 3, "...none is abandoned");
+ is($ledger->current_invoice->total_amount, dollars(0), "the total is 0");
});

I would feel more comfortable if the expected total here were something other than zero. It's too easy to get a false positive with a test for zero.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
};
View
8 t/invoice/psync/b5g1.t
@@ -98,7 +98,7 @@ test 'build quote' => sub {
$c->_maybe_send_psync_quote();
is(my ($q) = $ledger->quotes, 1, "now one quote");
is(my @ch = $q->all_items, 6, "it has six items");
- is(my ($special) = grep($_->does("Moonpig::Role::ChargeLike::Active"), @ch), 1,
+ is(my ($special) = grep($_->does("Moonpig::Role::LineItem::Active"), @ch), 1,
"one special item");
ok($special->does("Moonpig::Role::LineItem::PsyncB5G1Magic"),
"special item does the right role");
@@ -115,7 +115,7 @@ test 'adjustment execution' => sub {
$_->total_charge_amount(dollars(120)) for @chain;
$c->_maybe_send_psync_quote();
is(my ($q) = $ledger->quotes, 1, "now one quote");
- is(my ($special) = grep($_->does("Moonpig::Role::ChargeLike::Active"),
+ is(my ($special) = grep($_->does("Moonpig::Role::LineItem::Active"),
$q->all_items),
1,
"special item does the right role");
@@ -141,7 +141,7 @@ test 'adjustment amounts' => sub {
$c->_maybe_send_psync_quote();
if ($days < 50) {
is(my ($q) = $ledger->quotes, 1, "now one quote");
- is(my ($special) = grep($_->does("Moonpig::Role::ChargeLike::Active"),
+ is(my ($special) = grep($_->does("Moonpig::Role::LineItem::Active"),
$q->all_items),
1,
"special item does the right role");
@@ -183,7 +183,7 @@ test long_chain => sub {
$a->_maybe_send_psync_quote();
is(my ($q) = $ledger->quotes, 1, "now one quote");
is(my (@it) = $q->all_items, 10, "it has 10 items");
- is(my ($special) = grep($_->does("Moonpig::Role::ChargeLike::Active"), @it),
+ is(my ($special) = grep($_->does("Moonpig::Role::LineItem::Active"), @it),
1,
"found special item");
is($special->adjustment_amount, dollars(20), "adjustment amount is \$20");
View
42 t/lib/Role/Consumer/CouponCreator.pm
@@ -1,42 +0,0 @@
-package t::lib::Role::Consumer::CouponCreator;
-use Moose::Role;
-
-use Moonpig;
-use Moonpig::Logger '$Logger';
-use Moonpig::Trait::Copy;
-use Moonpig::Types qw(Factory);
-use Moonpig::Util qw(class);
-use MooseX::Types::Moose qw(HashRef);
-
-use namespace::autoclean;
-
-has coupon_class => (
- is => 'ro',
- isa => Factory,
- required => 1,
- traits => [ qw(Copy) ],
-);
-
-has coupon_args => (
- is => 'ro',
- isa => HashRef,
- default => sub { {} },
- traits => [ qw(Copy) ],
-);
-
-after _invoice => sub {
- my ($self) = @_;
- $self->ledger->current_invoice->add_charge(
- class("InvoiceCharge::CouponCreator")->new({
- consumer => $self,
- coupon_class => $self->coupon_class,
- coupon_args => $self->coupon_args,
- description => "Pseudocharge to trigger coupon creation on behalf of consumer for " .
- $self->xid,
- amount => 1,
- tags => [ $self->xid ],
- }));
-};
-
-
-1;
Please sign in to comment.
Something went wrong with that request. Please try again.