Closed melinath closed 8 years ago
I think this new organizational structure is good. It all seems to work from this end, tests pass, etc.
My only comments are about new test of partial refunds, which is not really related to this PR, so I don't think it should stop us from merging this in. But just so it's out there: I kind of think those all of those test_stripe.py
tests need to be reworked, including this one, to reduce the amount of boilerplate and increase the clarity of what's being tested (among other things). Having this one more test there is fine, but I think it would be wise not to add a ton more to that file without fixing it up a little, first.
Also, when I go through the process of charging a card, I get an error here: https://github.com/littleweaver/django-brambling/blob/payment-refactor/brambling/forms/orders.py#L408
AttributeError: card
Where the Charge object looks like this:
<Charge charge id=ch_18I3yJKpBVRiPjhpfsGwtxWR at 0x7fb21c3153c8> JSON: {
"amount": 200,
"amount_refunded": 0,
"application_fee": "fee_8ZCNIOjZLD1W0s",
"balance_transaction": {
"amount": 200,
"available_on": 1465430400,
"created": 1464905243,
"currency": "usd",
"description": null,
"fee": 41,
"fee_details": [
{
"amount": 5,
"application": "ca_4chEaXTzoWf7wlHdukG9gYj2n4ivgvpR",
"currency": "usd",
"description": "Dancerfly application fee",
"type": "application_fee"
},
{
"amount": 36,
"application": null,
"currency": "usd",
"description": "Stripe processing fees",
"type": "stripe_fee"
}
],
"id": "txn_18I3yJKpBVRiPjhp1WvG63Kk",
"net": 159,
"object": "balance_transaction",
"source": "ch_18I3yJKpBVRiPjhpfsGwtxWR",
"sourced_transfers": {
"data": [],
"has_more": false,
"object": "list",
"total_count": 0,
"url": "/v1/transfers?source_transaction=ch_18I3yJKpBVRiPjhpfsGwtxWR"
},
"status": "pending",
"type": "charge"
},
"captured": true,
"created": 1464905243,
"currency": "usd",
"customer": null,
"description": null,
"destination": null,
"dispute": null,
"failure_code": null,
"failure_message": null,
"fraud_details": {},
"id": "ch_18I3yJKpBVRiPjhpfsGwtxWR",
"invoice": null,
"livemode": false,
"metadata": {
"event": "Ghoul's Ball",
"order": "ZgY3ecN9"
},
"object": "charge",
"order": null,
"paid": true,
"receipt_email": null,
"receipt_number": null,
"refunded": false,
"refunds": {
"data": [],
"has_more": false,
"object": "list",
"total_count": 0,
"url": "/v1/charges/ch_18I3yJKpBVRiPjhpfsGwtxWR/refunds"
},
"shipping": null,
"source": {
"address_city": null,
"address_country": null,
"address_line1": null,
"address_line1_check": null,
"address_line2": null,
"address_state": null,
"address_zip": null,
"address_zip_check": null,
"brand": "Visa",
"country": "US",
"customer": null,
"cvc_check": "pass",
"dynamic_last4": null,
"exp_month": 11,
"exp_year": 2019,
"fingerprint": "3NY6ijzfyaiXuX7w",
"funding": "credit",
"id": "card_18I3yIKpBVRiPjhpc8BZiEp4",
"last4": "4242",
"metadata": {},
"name": null,
"object": "card",
"tokenization_method": null
},
"source_transfer": null,
"statement_descriptor": null,
"status": "succeeded"
}
I'm not really 100% happy with the way the payment form knows things about stripe that it shouldn't... long-term I might want us to move these forms into payment.stripe.forms
and refactor them somewhat. But for now I think just updating the test should be fine.
Now getting:
Request req_8ZZnp03pyIkOym: Can only apply an application_fee when the request is made on behalf of another account (using an OAuth key, the Stripe-Account header, or the destination parameter).
Okay, that error was just because the test event I was using was hooked up to LW's test account. Basically you can't charge yourself an application fee.
Went through and manually tested the code paths related to stripe & dwolla payment. I'm confident that this is all working as expected, combined w/ @chigby's previous review :-)
This branch consolidates most payment-related functionality under
brambling.payment
, which makes testing and updating apis easier (since we know more or less what parts of the system "know" about the inner workings of the remote API. This is still something we should keep working towards, but... this should do for now.Relatedly, this PR also updates the stripe API and adds a test proving that partial refunds aren't currently "broken".
This is a step towards #380. I'm hoping to get this reviewed by @chigby or @harrislapiroff soon so that I can have two relatively simple PRs rather than one mega-PR.