New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant transaction in customers.create #341

Merged
merged 1 commit into from Sep 12, 2017

Conversation

Projects
None yet
3 participants
@lukeburden
Contributor

lukeburden commented Jun 16, 2017

The use of transaction.atomic() wrapping the Customer.objects.create call is redundant as there is only a single query which will either succeed or fail. If a customer already exists, an IntegrityError will still be raised.

@lukeburden

This comment has been minimized.

Show comment
Hide comment
@lukeburden

lukeburden Jun 16, 2017

Contributor

Some context for this one, @paltman. I've been getting issues when using customer.create whereby the webhook customer.source.created hits my servers before the Customer record has been committed to the database. This results in an IntegrityError trying to write the Source to the database, as customer_id cannot be null.

It seems unlikely that the create Customer query would be uncommitted for a long enough period due transaction.atomic() such that the webhook arrives first, and yet I'm seeing this occur a few times per day.

Any thoughts? I'd rather not preemptively create customers with Stripe.

Contributor

lukeburden commented Jun 16, 2017

Some context for this one, @paltman. I've been getting issues when using customer.create whereby the webhook customer.source.created hits my servers before the Customer record has been committed to the database. This results in an IntegrityError trying to write the Source to the database, as customer_id cannot be null.

It seems unlikely that the create Customer query would be uncommitted for a long enough period due transaction.atomic() such that the webhook arrives first, and yet I'm seeing this occur a few times per day.

Any thoughts? I'd rather not preemptively create customers with Stripe.

@paltman

This comment has been minimized.

Show comment
Hide comment
@paltman

paltman Aug 25, 2017

Member

I'm trying to recall why I even had the transaction there in the first place.

Member

paltman commented Aug 25, 2017

I'm trying to recall why I even had the transaction there in the first place.

@paltman

This comment has been minimized.

Show comment
Hide comment
@paltman

paltman Aug 25, 2017

Member

I'm good to merge this. Can you fix the lint issue first though, @lukeburden?

Member

paltman commented Aug 25, 2017

I'm good to merge this. Can you fix the lint issue first though, @lukeburden?

@lukeburden

This comment has been minimized.

Show comment
Hide comment
@lukeburden

lukeburden Sep 12, 2017

Contributor

So, I hit the reason why you added the transaction in the first place. Nasty, one that I've also hit in various projects.

I've done some minor refactoring of the customer.create function in order to avoid needing to potentially trigger and handle an IntegrityError at all. Let me know what you think!

Contributor

lukeburden commented Sep 12, 2017

So, I hit the reason why you added the transaction in the first place. Nasty, one that I've also hit in various projects.

I've done some minor refactoring of the customer.create function in order to avoid needing to potentially trigger and handle an IntegrityError at all. Let me know what you think!

@paltman paltman merged commit f7bc7c9 into pinax:master Sep 12, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@paltman

This comment has been minimized.

Show comment
Hide comment
@paltman

paltman Sep 12, 2017

Member

Thanks!

Member

paltman commented Sep 12, 2017

Thanks!

if plan and charge_immediately:
invoices.create_and_pay(cus)
else:
# remove this extra customer as it is not needed
stripe.Customer.retrieve(stripe_customer["id"]).delete()

This comment has been minimized.

@blueyed

blueyed Sep 12, 2017

Contributor

Wouldn't it be better to create it only in case there is no local customer already in the first pace?

@blueyed

blueyed Sep 12, 2017

Contributor

Wouldn't it be better to create it only in case there is no local customer already in the first pace?

This comment has been minimized.

@lukeburden

lukeburden Sep 12, 2017

Contributor

Yes, that would make sense. Go for it!

@lukeburden

lukeburden Sep 12, 2017

Contributor

Yes, that would make sense. Go for it!

This comment has been minimized.

@blueyed

blueyed Sep 14, 2017

Contributor

Cool, see #364.

@blueyed

blueyed Sep 14, 2017

Contributor

Cool, see #364.

This comment has been minimized.

@blueyed

blueyed Sep 25, 2017

Contributor

Just for reference: finally rejected in #364.

@blueyed

blueyed Sep 25, 2017

Contributor

Just for reference: finally rejected in #364.

blueyed added a commit to blueyed/pinax-stripe that referenced this pull request Sep 14, 2017

@paltman paltman added this to the Samwise milestone Oct 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment