fix post_charge() arguments:

* updated Kavorka signature to remove non-functional argument types * removed Net::Stripe::Customer and HashRef for customer, as neither form was being serialized correctly for passing to the API call * removed Net::Stripe::Card and Net::Stripe::Token for card, as neither form was being serialized correctly for passing to the API call * added in-method validation and unit tests for the different combinations of the allowed argument types * updated and reorganized failing unit tests * closes <#94> and <#137>
lukec · Jan 18, 2020 · fd8e79e · fd8e79e
1 parent b3ffb02
commit fd8e79e
Show file tree

Hide file tree

Showing 4 changed files with 226 additions and 31 deletions.
diff --git a/lib/Net/Stripe.pm b/lib/Net/Stripe.pm
@@ -8,6 +8,7 @@ use HTTP::Request::Common qw/GET POST DELETE/;
 use MIME::Base64 qw/encode_base64/;
 use URI::Escape qw/uri_escape/;
 use JSON qw/decode_json/;
+use Net::Stripe::TypeConstraints;
 use Net::Stripe::Token;
 use Net::Stripe::Invoiceitem;
 use Net::Stripe::Invoice;
@@ -88,6 +89,19 @@ which is the format that C<Net::Stripe::List> expects. This makes the SDK
 compatible with the Stripe API back to the earliest documented API version
 <https://stripe.com/docs/upgrades#2011-06-21>.
 
+=item fix post_charge() arguments
+
+Some argument types for `customer` and `card` were non-functional in the
+current code and have been removed from the Kavorka method signature. We
+removed `Net::Stripe::Customer` and `HashRef` for `customer` and we removed
+`Net::Stripe::Card` and `Net::Stripe::Token` for `card`, as none of these
+forms were being serialized correctly for passing to the API call. We must
+retain `Net::Stripe::Card` for the `card` attribute in `Net::Stripe::Charge`
+because the `card` value returned from the API call is objectified into
+a card object. We have also added TypeConstraints for the string arguments
+passed and added in-method validation to ensure that the passed argument
+values make sense in the context of one another.
+
 =back
 
 =method new PARAMHASH
@@ -215,15 +229,40 @@ Returns a list of L<Net::Stripe::Charge> objects.
 Charges: {
     method post_charge(Int :$amount,
                        Str :$currency,
-                       Net::Stripe::Customer|HashRef|Str :$customer?,
-                       Net::Stripe::Card|Net::Stripe::Token|Str|HashRef :$card?,
+                       StripeCustomerId :$customer?,
+                       StripeTokenId|StripeCardId|HashRef :$card?,
                        Str :$description?,
                        HashRef :$metadata?,
                        Bool :$capture?,
                        Str :$statement_descriptor?,
                        Int :$application_fee?,
                        Str :$receipt_email?
                      ) {
+
+        if ( defined( $card ) ) {
+            my $card_id_type = Moose::Util::TypeConstraints::find_type_constraint( 'StripeCardId' );
+            if ( defined( $customer ) && ! $card_id_type->check( $card ) ) {
+                die Net::Stripe::Error->new(
+                    type => "post_charge error",
+                    message => sprintf(
+                        "Invalid value '%s' passed for parameter 'card'. Charges for an existing customer can only accept a card id.",
+                        $card,
+                    ),
+                );
+            }
+
+            my $token_id_type = Moose::Util::TypeConstraints::find_type_constraint( 'StripeTokenId' );
+            if ( ! defined( $customer ) && ! $token_id_type->check( $card ) && ref( $card ) ne 'HASH' ) {
+                die Net::Stripe::Error->new(
+                    type => "post_charge error",
+                    message => sprintf(
+                        "Invalid value '%s' passed for parameter 'card'. Charges without an existing customer can only accept a token id or card hashref.",
+                        $card,
+                    ),
+                );
+            }
+        }
+
         my $charge = Net::Stripe::Charge->new(amount => $amount,
                                               currency => $currency,
                                               customer => $customer,

diff --git a/lib/Net/Stripe/Charge.pm b/lib/Net/Stripe/Charge.pm
@@ -10,8 +10,8 @@ has 'id'                  => (is => 'ro', isa => 'Maybe[Str]');
 has 'created'             => (is => 'ro', isa => 'Maybe[Int]');
 has 'amount'              => (is => 'ro', isa => 'Maybe[Int]', required => 1);
 has 'currency'            => (is => 'ro', isa => 'Maybe[Str]', required => 1);
-has 'customer'            => (is => 'ro', isa => 'Maybe[Str]');
-has 'card'                => (is => 'ro', isa => 'Maybe[Net::Stripe::Token|Net::Stripe::Card|Str]');
+has 'customer'            => (is => 'ro', isa => 'Maybe[StripeCustomerId]');
+has 'card'                => (is => 'ro', isa => 'Maybe[Net::Stripe::Card|StripeTokenId|StripeCardId]');
 has 'description'         => (is => 'ro', isa => 'Maybe[Str]');
 has 'livemode'            => (is => 'ro', isa => 'Maybe[Bool|Object]');
 has 'paid'                => (is => 'ro', isa => 'Maybe[Bool|Object]');

diff --git a/lib/Net/Stripe/TypeConstraints.pm b/lib/Net/Stripe/TypeConstraints.pm
@@ -0,0 +1,35 @@
+package Net::Stripe::TypeConstraints;
+
+use strict;
+use Moose::Util::TypeConstraints qw/subtype as where message/;
+
+# ABSTRACT: Custom Moose TypeConstraints for Net::Stripe object attributes and parameters
+
+subtype 'StripeTokenId',
+    as 'Str',
+    where {
+        /^tok_.+/
+    },
+    message {
+        sprintf( "Value '%s' must be a token id string of the form tok_.+", $_ );
+    };
+
+subtype 'StripeCardId',
+    as 'Str',
+    where {
+        /^card_.+/
+    },
+    message {
+        sprintf( "Value '%s' must be a card id string of the form card_.+", $_ );
+    };
+
+subtype 'StripeCustomerId',
+    as 'Str',
+    where {
+        /^cus_.+/
+    },
+    message {
+        sprintf( "Value '%s' must be a customer id string of the form cus_.+", $_ );
+    };
+
+1;
diff --git a/t/live.t b/t/live.t
@@ -267,7 +267,7 @@ Charges: {
         is $card->name, $fake_card->{name}, 'retrieve the card name';
     }
 
-    Post_charge_using_token: {
+    Post_charge_using_token_id: {
         my $token = $stripe->post_token( card => $fake_card );
         my $charge = $stripe->post_charge(
             amount => 100,
@@ -277,48 +277,171 @@ Charges: {
         isa_ok $charge, 'Net::Stripe::Charge';
         ok $charge->paid, 'charge was paid';
         is $charge->status, 'paid', 'charge status is paid';
+        is $charge->card->id, $token->card->id, 'charge card id matches';
     }
 
-    Rainy_day: {
-        # swallow the expected warning rather than have it print out durring tests.
-        close STDERR;
-        open(STDERR, ">", "/dev/null");
+    Post_charge_using_card_id: {
+        my $token = $stripe->post_token( card => $fake_card );
+        eval {
+             $stripe->post_charge(
+                amount => 100,
+                currency => 'usd',
+                card => $token->card->id,
+            );
+        };
+        if ($@) {
+            my $e = $@;
+            isa_ok $e, 'Net::Stripe::Error', 'error raised is an object';
+            is $e->type, 'post_charge error', 'error type';
+            like $e->message, qr/^Invalid value 'card_.+' passed for parameter 'card'\. Charges without an existing customer can only accept a token id or card hashref\.$/, 'error message';
+        } else {
+            fail 'post charge with card id';
+        }
+    }
+
+    Post_charge_using_card_hash: {
+        my $charge = $stripe->post_charge(
+            amount => 100,
+            currency => 'usd',
+            card => $fake_card,
+        );
+        isa_ok $charge, 'Net::Stripe::Charge';
+        ok $charge->paid, 'charge was paid';
+        is $charge->status, 'paid', 'charge status is paid';
+        is $charge->card->last4, substr( $fake_card->{number}, -4 ), 'charge card last4 matches';
+    }
+
+    Post_charge_for_customer_id_with_attached_card: {
+        my $customer = $stripe->post_customer(
+            card => $fake_card,
+        );
+        my $charge = $stripe->post_charge(
+            amount => 100,
+            currency => 'usd',
+            customer => $customer->id,
+        );
+        isa_ok $charge, 'Net::Stripe::Charge';
+        ok $charge->paid, 'charge was paid';
+        is $charge->status, 'paid', 'charge status is paid';
+        is $charge->card->id, $customer->default_card, 'charged default card';
+    }
 
-        throws_ok {
+    Post_charge_for_customer_id_without_attached_card: {
+        my $customer = $stripe->post_customer();
+        eval {
+            # swallow the expected warning rather than have it print out during tests.
+            local $SIG{__WARN__} = sub {};
             $stripe->post_charge(
-                amount => 3300,
+                amount => 100,
                 currency => 'usd',
-                description => 'Wikileaks donation',
+                customer => $customer->id,
+            );
+        };
+        if ($@) {
+            my $e = $@;
+            isa_ok $e, 'Net::Stripe::Error', 'error raised is an object';
+            is $e->type, 'card_error', 'error type';
+            is $e->message, 'Cannot charge a customer that has no active card', 'error message';
+        } else {
+            fail 'post charge for customer with token id';
+        }
+    }
+
+    Post_charge_for_customer_id_using_token_id: {
+        my $token = $stripe->post_token( card => $fake_card );
+        my $customer = $stripe->post_customer();
+        eval {
+            $stripe->post_charge(
+                amount => 100,
+                currency => 'usd',
+                customer => $customer->id,
+                card => $token->id,
+            );
+        };
+        if ($@) {
+            my $e = $@;
+            isa_ok $e, 'Net::Stripe::Error', 'error raised is an object';
+            is $e->type, 'post_charge error', 'error type';
+            like $e->message, qr/^Invalid value 'tok_.+' passed for parameter 'card'\. Charges for an existing customer can only accept a card id\.$/, 'error message';
+        } else {
+            fail 'post charge for customer with token id';
+        }
+    }
+
+    Post_charge_for_customer_id_using_card_id: {
+        # customer may have multiple cards. allow ability to select a specific
+        # card for a given charge.
+        my $customer = $stripe->post_customer();
+        my $card = $stripe->post_card(
+            customer => $customer,
+            card => $fake_card,
+        );
+        for ( 1..3 ) {
+            my $other_card = $stripe->post_card(
+                customer => $customer,
+                card => $fake_card,
             );
-        } qr/invalid_request_error/, 'missing card and customer';
+            isnt $card->id, $other_card->id, 'different card id';
+        }
+        my $charge = $stripe->post_charge(
+            amount => 100,
+            currency => 'usd',
+            customer => $customer->id,
+            card => $card->id,
+        );
+        isa_ok $charge, 'Net::Stripe::Charge';
+        ok $charge->paid, 'charge was paid';
+        is $charge->status, 'paid', 'charge status is paid';
+        is $charge->card->id, $card->id, 'charge card id matches';
+    }
+
+    Post_charge_for_customer_id_using_card_hash: {
+        my $customer = $stripe->post_customer();
+        eval {
+            $stripe->post_charge(
+                amount => 100,
+                currency => 'usd',
+                customer => $customer->id,
+                card => $fake_card,
+            );
+        };
+        if ($@) {
+            my $e = $@;
+            isa_ok $e, 'Net::Stripe::Error', 'error raised is an object';
+            is $e->type, 'post_charge error', 'error type';
+            like $e->message, qr/^Invalid value 'HASH\(0x[0-9a-f]+\)' passed for parameter 'card'. Charges for an existing customer can only accept a card id\.$/, 'error message';
+        } else {
+            fail 'post charge for customer with card hashref';
+        }
+    }
 
-        throws_ok {
+    Rainy_day: {
+        # swallow the expected warning rather than have it print out during tests.
+        local $SIG{__WARN__} = sub {};
+        # Test a charge with no source or customer
+        eval {
             $stripe->post_charge(
                 amount => 3300,
                 currency => 'usd',
                 description => 'Wikileaks donation',
-                customer => 'fake-customer-id',
-                card => {
-                    number => '4242-4242-4242-4242',
-                    exp_month => $future->month,
-                    exp_year  => $future->year,
-                    cvc => 123,
-                    name => 'Anonymous',
-                },
             );
-        } qr/invalid_request_error/, 'missing card and customer';
+
+        };
+        if ($@) {
+            my $e = $@;
+            isa_ok $e, 'Net::Stripe::Error', 'error raised is an object';
+            is $e->type, 'invalid_request_error', 'error type';
+            is $e->message, 'Must provide source or customer.', 'error message';
+        } else {
+            fail 'missing card and customer';
+        }
 
         # Test an invalid currency
         eval {
             $stripe->post_charge(
                 amount => 3300,
                 currency => 'zzz',
-                card => {
-                    number => '4242-4242-4242-4242',
-                    exp_month => $future->month,
-                    exp_year  => $future->year,
-                    cvc => 123,
-                },
+                card => $fake_card,
             );
         };
         if ($@) {
@@ -330,8 +453,6 @@ Charges: {
         } else {
             fail 'report invalid currency';
         }
-        close STDERR;
-        open(STDERR, ">&", STDOUT);
     }
 
     Charge_with_receipt_email: {