pinax / pinax-stripe-light

a payments Django app for Stripe
MIT License
686 stars 285 forks source link

sync_payment_source_from_stripe_data is broken #383

Open blueyed opened 7 years ago

blueyed commented 7 years ago

Issue Summary

I am seeing KeyErrors for 'active' via

  File "/usr/lib/python3.6/site-packages/pinax/stripe/actions/events.py", line 30, in add_event
    webhook.process()
  File "/usr/lib/python3.6/site-packages/pinax/stripe/webhooks.py", line 91, in process
    self.process_webhook()
  File "/usr/lib/python3.6/site-packages/pinax/stripe/webhooks.py", line 313, in process_webhook
    customers.sync_customer(self.event.customer, self.event.customer.stripe_customer)
  File "/usr/lib/python3.6/site-packages/pinax/stripe/actions/customers.py", line 162, in sync_customer
    sources.sync_payment_source_from_stripe_data(customer, source)
  File "/usr/lib/python3.6/site-packages/pinax/stripe/actions/sources.py", line 120, in sync_payment_source_from_stripe_data
    return sync_bitcoin(customer, source)
  File "/usr/lib/python3.6/site-packages/pinax/stripe/actions/sources.py", line 86, in sync_bitcoin
    active=source["active"],
  File "/usr/lib/python3.6/site-packages/stripe/resource.py", line 184, in __getitem__
    raise err
  File "/usr/lib/python3.6/site-packages/stripe/resource.py", line 173, in __getitem__
    return super(StripeObject, self).__getitem__(k)
KeyError: 'active'

This is because sync_payment_source_from_stripe_data considers this to be a bitcoin source (via https://github.com/pinax/pinax-stripe/blob/787ea7e17ff32a4b87152655fa6dfea85de755ec/pinax/stripe/actions/sources.py#L117), but the check for id starts with "card" should probably rather look at the type:

As far as I know this is an example testing card provided by Stripe itself:

`sources['data']` is:

```json
    {
      "amount": null,
      "card": {
        "address_line1_check": null,
        "address_zip_check": null,
        "brand": "Visa",
        "country": "US",
        "cvc_check": "unchecked",
        "dynamic_last4": null,
        "exp_month": 12,
        "exp_year": 2019,
        "fingerprint": "XXX",
        "funding": "credit",
        "last4": "4242",
        "three_d_secure": "optional",
        "tokenization_method": null
      },
      "client_secret": "src_client_secret_XXX",
      "created": 1507215076,
      "currency": null,
      "customer": "cus_XXX",
      "flow": "none",
      "id": "src_XXX",
      "livemode": false,
      "metadata": {},
      "object": "source",
      "owner": {
        "address": null,
        "email": null,
        "name": null,
        "phone": null,
        "verified_address": null,
        "verified_email": null,
        "verified_name": null,
        "verified_phone": null
      },
      "statement_descriptor": null,
      "status": "chargeable",
      "type": "card",
      "usage": "reusable"
    }
  ],

I think sync_bitcoin should only be used if the type is "bitcoin". This is not included in the list of values for "type" however (https://stripe.com/docs/api/python#source_object-type), but used in the example.

The event (for reference), the customer's sources are being fetched during processing:

{
   "request" : {
      "idempotency_key" : null,
      "id" : "req_UWfKtJifYzMRkH"
   },
   "data" : {
      "object" : {
         "customer" : "cus_XXX",
         "object" : "subscription",
         "livemode" : false,
         "items" : {
            "has_more" : false,
            "total_count" : 1,
            "url" : "/v1/subscription_items?subscription=sub_XXX",
            "data" : [
               {
                  "id" : "si_XXX",
                  "metadata" : {},
                  "plan" : {
                     "amount" : 20000,
                     "interval_count" : 1,
                     "livemode" : false,
                     "object" : "plan",
                     "created" : 1503996123,
                     "currency" : "eur",
                     "interval" : "week",
                     "name" : "Weekly plan",
                     "trial_period_days" : null,
                     "metadata" : {},
                     "id" : "XXX",
                     "statement_descriptor" : null
                  },
                  "created" : 1507215085,
                  "quantity" : 0,
                  "object" : "subscription_item"
               }
            ],
            "object" : "list"
         },
         "created" : 1507215084,
         "billing" : "charge_automatically",
         "plan" : {
            "created" : 1503996123,
            "currency" : "eur",
            "object" : "plan",
            "livemode" : false,
            "amount" : 20000,
            "interval_count" : 1,
            "metadata" : {},
            "statement_descriptor" : null,
            "id" : "XXX",
            "interval" : "week",
            "trial_period_days" : null,
            "name" : "Weekly plan"
         },
         "current_period_end" : 1507819884,
         "current_period_start" : 1507215084,
         "metadata" : {},
         "discount" : null,
         "ended_at" : null,
         "application_fee_percent" : null,
         "tax_percent" : null,
         "cancel_at_period_end" : false,
         "trial_start" : null,
         "canceled_at" : null,
         "trial_end" : null,
         "quantity" : 0,
         "status" : "active",
         "start" : 1507215084,
         "id" : "sub_XXX"
      }
   },
   "livemode" : false,
   "object" : "event",
   "created" : 1507215084,
   "type" : "customer.subscription.created",
   "pending_webhooks" : 1,
   "id" : "evt_1B9ah6EWnIuOzoofHM1ni06S",
   "api_version" : "2017-08-15"
}

That might be related to the API version being used then maybe, since I've not found other issues regarding this. We're using the latest/current API version (2017-08-15).

blueyed commented 7 years ago

@paltman Any input on this? You've added the tests/fixtures in 7c8fbae4 (where I still fail to see where this would match, i.e. "bitcoin_receiver"

sync_payment_source_from_stripe_data itself was added in 3aca8ae.

blueyed commented 7 years ago

The iOS SDK has some fixtures, e.g. for card sources (https://github.com/stripe/stripe-ios/blob/master/Tests/Tests/CardSource.json). The actual card object seems to be something different (from the cards endpoint, not what cu["sources"] would give you (https://github.com/stripe/stripe-ios/blob/master/Tests/Tests/Card.json). And this would be the actual bitcoin source: https://github.com/stripe/stripe-ios/blob/master/Tests/Tests/BitcoinSource.json.

blueyed commented 7 years ago

@paltman @lukeburden Don't you see a problem with this in your webhooks? I've created https://github.com/pinax/pinax-stripe/pull/407 for now.

blueyed commented 7 years ago

While it is better by now I would not consider this to be fixed. There are remaining questions, and it is likely still broken with "bitcoin_receiver", and maybe missing other use cases even.