Added campaign_name field to Donation model #335

olasitarska · 2015-01-21T19:06:44Z

No description provided.

jezdez · 2015-01-21T19:47:28Z

fundraising/views.py

        if fixed_amount:
            try:
-                initial = {'amount': Decimal(fixed_amount)}
+                initial = {'amount': Decimal(fixed_amount), 'campaign': campaign}


Would you mind reformatting that to multiple lines?

jezdez · 2015-01-21T19:50:26Z

LGTM

Of course if we want to store more info about campaigns than just the name maybe it'd be better to have a separate campaign data model and have an optional foreign key in the donation model. That allows us to have in theory easy metadata storage and is future proof. E.g. imagine having a campaign page with just the heart for that campaign. In fact you could consider this fundraiser the first campaign we could use this system for.

timgraham · 2015-01-21T19:50:50Z

fundraising/forms.py

@@ -160,6 +161,11 @@ class PaymentForm(forms.Form):
        ),
    )



no newline between fields (you can remove the one above expires please)

timgraham · 2015-01-21T19:53:24Z

The complexity of code without tests is starting to make me nervous. Would you like to write some? :tests: <-- where is a good icon when I need it?

jezdez · 2015-01-21T20:01:41Z

There is is: 🐐

jezdez · 2015-01-21T20:02:32Z

Reference (et al): http://www.obeythetestinggoat.com/

jezdez · 2015-01-21T20:17:02Z

fundraising/views.py

                )
                return redirect(donation)
        else:
            if 'amount' in form.errors:
                show_amount = True
    else:
-        fixed_amount = request.GET.get('amount') or None


Please don't do that, it will break with empty GET parameters.

olasitarska · 2015-01-21T22:59:02Z

I pushed some tests here.. the submitting donation form test doesn't work yet, mocking is not really working as it should (maybe I used wrong url?). I am too tired to work on this longer today though. Not sure about tomorrow yet. Sorry!

jezdez · 2015-01-22T11:14:44Z

fundraising/tests.py

+
+        response = self.client.post(reverse('fundraising:donate'), {
+            'amount': 100,
+            'number': '4242424242424242',


The number, cvc and expires data is not submitted to our form view, the form actually only submits the stripe_token that is set by our JavaScript. Card data never hits our servers :) So simply set a stripe_token to something and it should be valid.

olasitarska · 2015-01-22T17:51:08Z

I fixed all the things you commented on, thanks!

I am still not able to mock Stripe requests despite mocking all of the urls. Looking at this, there is definitely more magic involved than just trying to mock the request with request_mock, I think.

olasitarska · 2015-01-22T23:24:36Z

Uh finally! For some reason mocking API calls via setUpClass didn't work at all. It only worked when I added the mocking decorator directly on the test. Anyway, then together with @bmispelon we've decided that mocking API calls isn't really a good idea, because Stripe wanted us to return a very particular response. So these last tests I added are patching Stripe methods and it works.

Is this all ok?

* stripe calls in form.make_donation method instead of donate view * tests of donate view * tests of make_donation * logging if creation of stripe Customer failed

asendecka · 2015-01-23T01:03:57Z

Meanwhile I and @oinopion took a look and refactor donate method a little bit. Previous state was pretty hard to test. We moved stripe & creating of a donation to form method. We also added logging, since there was only raise there. For now we simply use Bad Request. I think it should be addressed when solving #342 (maybe displaying form again?), but it's better than 500 anyway.

jezdez · 2015-01-23T08:08:24Z

@asendecka The reason why I fell back to just catching StripeError was simply time constraints on the implementation (I did that in one afternoon after dabbling with the various Django apps that didn't do what we want), but I know there are more than just this one exception. A bad request is only better for the user, but letting the exception bubble up allowed us to see what happened in Sentry, which we wouldn't with the 400. Logging doesn't help as much here since we don't have a sensible logging setup.

Check out CardError (https://github.com/stripe/stripe-python/blob/846ec9080a7f3cd55113871803eb9d7c29e869d7/stripe/error.py#L29-L36) that has a status code attached with the reason why the card was declined: https://stripe.com/docs/api/python#errors

There is also a pretty awesome example for how to catch all the other exceptions in Python below that list errors in the Stripe docs, under "Handling errors".

try:
  # Use Stripe's bindings...
  pass
except stripe.error.CardError, e:
  # Since it's a decline, stripe.error.CardError will be caught
  body = e.json_body
  err  = body['error']

  print "Status is: %s" % e.http_status
  print "Type is: %s" % err['type']
  print "Code is: %s" % err['code']
  # param is '' in this case
  print "Param is: %s" % err['param']
  print "Message is: %s" % err['message']
except stripe.error.InvalidRequestError, e:
  # Invalid parameters were supplied to Stripe's API
  pass
except stripe.error.AuthenticationError, e:
  # Authentication with Stripe's API failed
  # (maybe you changed API keys recently)
  pass
except stripe.error.APIConnectionError, e:
  # Network communication with Stripe failed
  pass
except stripe.error.StripeError, e:
  # Display a very generic error to the user, and maybe send
  # yourself an email
  pass
except Exception, e:
  # Something else happened, completely unrelated to Stripe
  pass

I think in case a CardError is raised we should reraise it as a ValidationError with saying something like "We're sorry but we had problems charging your card: %s" % errror_code. InvalidRequestError and AuthenticationError should raise a ImproperlyConfigured exception (with a 500 so it shows up in Sentry) since that's an error on our side, APIConnectionError probably should just redisplay the form and ask the user to try again. And then the raw StripeError and Exception should just be raised to bubble up in Sentry.

Now that you've already started the refactor, let's merge #342 into this and refactor it completely.

olasitarska · 2015-01-23T09:09:41Z

Are going to be able to merge this small feature this pull request was about before this campaign ends?

jezdez · 2015-01-24T22:07:47Z

Merged in 9e9d176.

jezdez reviewed Jan 21, 2015
View reviewed changes

timgraham reviewed Jan 21, 2015
View reviewed changes

jezdez reviewed Jan 21, 2015
View reviewed changes

jezdez reviewed Jan 22, 2015
View reviewed changes

olasitarska force-pushed the fundraising-campaign-link branch from 9fd9703 to 1fbcbf0 Compare January 22, 2015 17:18

olasitarska added 3 commits January 22, 2015 23:22

Added campaign_name field to Donation model

78f5687

Campaign review fixes

1f816ab

Changed campaign_name field from null=True to blank=True

a56d276

olasitarska added 7 commits January 22, 2015 23:22

Added "or None" for fixed amount back

b7f30f3

Added tests making sure that donation forms are rendered properly.

bde1941

Submitting donation form test (work in progress)

8ef13f3

Fixed minor things in tests

5bf8acd

Mocking more Stripe api calls

14296ce

Added tests for empty campaign

9665b10

Added tests mocking Stripe methods

fe54e1b

olasitarska force-pushed the fundraising-campaign-link branch from a459d4f to fe54e1b Compare January 22, 2015 23:22

Refactoring of donate view. Tests added.

210cf69

* stripe calls in form.make_donation method instead of donate view * tests of donate view * tests of make_donation * logging if creation of stripe Customer failed

jezdez mentioned this pull request Jan 23, 2015

Improve donation error handling #342

Closed

jezdez assigned asendecka Jan 23, 2015

Removed logging and returned to raising 500 on Stripe API error

57fe44b

jezdez closed this Jan 24, 2015

jezdez deleted the fundraising-campaign-link branch January 25, 2015 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added campaign_name field to Donation model #335

Added campaign_name field to Donation model #335

olasitarska commented Jan 21, 2015

jezdez Jan 21, 2015

jezdez commented Jan 21, 2015

timgraham Jan 21, 2015

timgraham commented Jan 21, 2015

jezdez commented Jan 21, 2015

jezdez commented Jan 21, 2015

jezdez Jan 21, 2015

olasitarska commented Jan 21, 2015

jezdez Jan 22, 2015

olasitarska commented Jan 22, 2015

olasitarska commented Jan 22, 2015

asendecka commented Jan 23, 2015

jezdez commented Jan 23, 2015

olasitarska commented Jan 23, 2015

jezdez commented Jan 24, 2015

Added campaign_name field to Donation model #335

Added campaign_name field to Donation model #335

Conversation

olasitarska commented Jan 21, 2015

jezdez Jan 21, 2015

Choose a reason for hiding this comment

jezdez commented Jan 21, 2015

timgraham Jan 21, 2015

Choose a reason for hiding this comment

timgraham commented Jan 21, 2015

jezdez commented Jan 21, 2015

jezdez commented Jan 21, 2015

jezdez Jan 21, 2015

Choose a reason for hiding this comment

olasitarska commented Jan 21, 2015

jezdez Jan 22, 2015

Choose a reason for hiding this comment

olasitarska commented Jan 22, 2015

olasitarska commented Jan 22, 2015

asendecka commented Jan 23, 2015

jezdez commented Jan 23, 2015

olasitarska commented Jan 23, 2015

jezdez commented Jan 24, 2015