Supporters may make one off payments towards a project #98

zspencer · 2018-01-04T02:00:17Z

Resolves #64

zspencer · 2018-01-04T18:10:25Z

spec/factories/registered_user_factory.rb

-  factory :registered_user do
-    email "user-#{SecureRandom.uuid}@example.com"
+  factory :registered_user, aliases: [:user] do
+    email { "user-#{SecureRandom.uuid}@example.com" }


TURNS OUT if you don't pass a block this gets evaluated at runtime, so using the factory multiple times doesn't work. I was very confused for about 30m on that.

zspencer · 2018-01-04T18:11:46Z

features/step_definitions/stripe_steps.rb

+
+Then("a stripe customer was created for me") do
+  expect(app.current_user.stripe_customer_id).not_to be_nil
+  expect(app.stripe_api.created_customers.last).to include(tok: "fake-elements-token",


I'm intending to wrap the Stripe Ruby Gem so that we can have a "fake client" that stores the the past transactions with it ala Hexagonal application architecture.

zspencer · 2018-01-04T18:21:33Z

db/schema.rb

    t.index ["email"], name: "index_registered_users_on_email", unique: true
    t.index ["reset_password_token"], name: "index_registered_users_on_reset_password_token", unique: true
  end

  create_table "stripe_connections", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
    t.uuid "owner_id"
    t.string "stripe_account_id"
-    t.string "business_name"
-    t.string "display_name"
-    t.string "statement_descriptor"


I don't know why this was removed? Probably because I rebased wrong or something?

zspencer · 2018-01-07T19:16:33Z

app/views/projects/_summary.html.erb

@@ -3,6 +3,7 @@
  <h<%= heading_level %> class="title"><%= project.title %></h<%= heading_level%>>
  <p class="summary"><%= project.summary %></p>
  <div class="actions">
+    <%= link_to(t('contributing_to_project.begin'), new_project_contribution_path(project)) %>


I will make this prettier 🔜

zspencer · 2018-01-07T19:20:21Z

features/support/fake_stripe.rb

+
+  def charges
+    @charges ||= []
+  end


@bhaibel - This is the kind of seam I'm talking about re: making a tiny wrapper that acts as a persistent spy that we can stub/etc without requiring us to spy explicitly; while still giving us the flexibility to stub with particular behaviors when we want to. Basically a pass-through adapter that in our test code shovels data. Ideally, we could pull this kind of thing out into a stripe-spec gem similar to https://github.com/email-spec/email-spec

Yeah, this is the kind of thing I really don't like, it feels like a sink for the kind of thing that's the illusion of productivity -- I like the pass-through, it makes it better, and given that this is a lesser-of-evils situation maybe it makes it better enough, but I'm still pretty frowny-face about this tbh.

Lemme poke at how much of a pain it is to do test data cleanup via the Stripe API -- I feel like if we freshly create users on a per-test-run-basis rather than have fixture users, that gets around my concerns re too many secrets needed to run tests. It'll be slow but, eh, these are already feature tests.

Let's hit the real stripe API then. I've had the literal opposite experience that you've had though, where using the pattern of explicitly stubbing interactions with external APIs in feature tests took a test suite that was full of brittleness and errors which degraded trust to one that could be relied on to actually give useful feedback.

That said, having a "contract/smoke test" where we actually go through the UI/API without any stubs is a really useful/good thing to have; especially if it can be fired against staging.

Can we put the stuff that actually hits real APIs in a different build that the features? That way the feature tests can stay reasonably independent and then the smoke/contract tests can be ran pre-promotion from staging to production?

zspencer · 2018-01-08T03:10:19Z

app/models/contribution.rb

+    transaction do
+      save
+      return false unless persisted?
+      self.charge = payment_processor.charge(amount: amount)


I want to start persisting the raw results of a charge into the database, perhaps as JSON?

All of this feels really dangerous to me. I don't like the false sense of safety that transactional stuff in distributed systems provides -- there's a rework of the data model necessary here. Will throw myself at it. Alternately we can just not persist contributions yet -- that's what I was going for with having Contribution be ActiveModel at first, so that we could punt on the shape of that

We need to be storing the charge ID somewhere so if stripe throws a fit later we can trace it back. That doesn't mean it needs to be in the same model as the contribution, heck it could just be in logs for all we really care ATM, but losing access to the charge ID will be a giant pain in the ass later.

That doesn't mean we need to be doing it in a "database transaction" per-se, I was trying to get ahead of a thing you were concerned about earlier; which is why I put this in a real database transaction.

One way we could slice this is to "take contributions on faith" initially, then fill in the stripe payment processor bits. Happy to see what slices you come up with.

zspencer · 2018-01-08T03:50:40Z

app/models/contribution.rb

+      return false unless persisted?
+      self.charge = payment_processor.charge(amount: amount)
+      self.amount_cents = charge.amount
+      charge.captured?


We probably should fire a rollback if the charge wasn't captured or paid?

bhaibel

This PR is big and only gonna get bigger. I'm going to sink some time into extracting a smaller story from it so that we can get the parts we have consensus on merged instead of turning this into even more of a lumbering everything-blocking beast.

bhaibel · 2018-01-08T15:21:50Z

app/lib/nourish/stripe.rb

+    end
+
+    class << self
+      attr_writer :instance


Is there a reason we can't use Ruby's baked in Singleton stuff here? Feels like clearer intent.

Not really, I had some bad experiences with ruby singleton before and have shied away from it since, but it doesn't really matter to me.

bhaibel · 2018-01-08T15:26:23Z

app/models/contribution.rb

+    transaction do
+      save
+      return false unless persisted?
+      self.charge = payment_processor.charge(amount: amount)


All of this feels really dangerous to me. I don't like the false sense of safety that transactional stuff in distributed systems provides -- there's a rework of the data model necessary here. Will throw myself at it. Alternately we can just not persist contributions yet -- that's what I was going for with having Contribution be ActiveModel at first, so that we could punt on the shape of that

bhaibel · 2018-01-08T15:33:30Z

features/support/fake_stripe.rb

+
+  def charges
+    @charges ||= []
+  end


Yeah, this is the kind of thing I really don't like, it feels like a sink for the kind of thing that's the illusion of productivity -- I like the pass-through, it makes it better, and given that this is a lesser-of-evils situation maybe it makes it better enough, but I'm still pretty frowny-face about this tbh.

Lemme poke at how much of a pain it is to do test data cleanup via the Stripe API -- I feel like if we freshly create users on a per-test-run-basis rather than have fixture users, that gets around my concerns re too many secrets needed to run tests. It'll be slow but, eh, these are already feature tests.

zspencer temporarily deployed to nourish-party-staging-pr-98 January 4, 2018 02:00 Inactive

zspencer force-pushed the zs-supporter-may-make-one-off-payment-to-a-project branch from ab0f17d to 0f832c7 Compare January 4, 2018 17:50

zspencer temporarily deployed to nourish-party-staging-pr-98 January 4, 2018 17:50 Inactive

zspencer commented Jan 4, 2018

View reviewed changes

zspencer temporarily deployed to nourish-party-staging-pr-98 January 5, 2018 23:57 Inactive

zspencer temporarily deployed to nourish-party-staging-pr-98 January 6, 2018 02:56 Inactive

zspencer force-pushed the zs-supporter-may-make-one-off-payment-to-a-project branch from 244242b to 0cbc746 Compare January 7, 2018 17:43

zspencer temporarily deployed to nourish-party-staging-pr-98 January 7, 2018 17:43 Inactive

bhaibel force-pushed the zs-supporter-may-make-one-off-payment-to-a-project branch from 0cbc746 to 244242b Compare January 7, 2018 18:00

zspencer temporarily deployed to nourish-party-staging-pr-98 January 7, 2018 18:00 Inactive

zspencer force-pushed the zs-supporter-may-make-one-off-payment-to-a-project branch from 244242b to f1bd1d9 Compare January 7, 2018 18:15

zspencer temporarily deployed to nourish-party-staging-pr-98 January 7, 2018 18:16 Inactive

zspencer temporarily deployed to nourish-party-staging-pr-98 January 7, 2018 19:07 Inactive

zspencer temporarily deployed to nourish-party-staging-pr-98 January 7, 2018 19:13 Inactive

zspencer commented Jan 7, 2018

View reviewed changes

zspencer temporarily deployed to nourish-party-staging-pr-98 January 7, 2018 22:57 Inactive

zspencer temporarily deployed to nourish-party-staging-pr-98 January 8, 2018 02:50 Inactive

zspencer temporarily deployed to nourish-party-staging-pr-98 January 8, 2018 03:08 Inactive

zspencer commented Jan 8, 2018

View reviewed changes

zspencer temporarily deployed to nourish-party-staging-pr-98 January 8, 2018 03:25 Inactive

zspencer commented Jan 8, 2018

View reviewed changes

zspencer temporarily deployed to nourish-party-staging-pr-98 January 8, 2018 03:52 Inactive

zspencer deployed to nourish-party-staging-pr-98 January 8, 2018 03:52 View deployment

bhaibel suggested changes Jan 8, 2018

View reviewed changes

bhaibel mentioned this pull request Jan 8, 2018

Projects display whether they can take contributions #107

Merged

Write feature tests for one off project payments

07e48fb

zspencer added 7 commits January 14, 2018 02:40

User may store their stripe customer id in the database

7065394

Contributions are sent to stripe and may be inspected by feature tests

524f497

Refactor contribution amounts to feel more like money-rails

9c727d0

A record of contributions is persisted

33bb5cc

Show users a history of their contributions

ce52ff1

Refactor to use the self << class syntax

146a216

Fix test failure caused by merge conflict resolution

0ebc978

zspencer force-pushed the zs-supporter-may-make-one-off-payment-to-a-project branch from 82cd294 to 0ebc978 Compare January 14, 2018 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supporters may make one off payments towards a project #98

Supporters may make one off payments towards a project #98

zspencer commented Jan 4, 2018 •

edited

Loading

zspencer Jan 4, 2018 •

edited

Loading

zspencer Jan 4, 2018

zspencer Jan 4, 2018

zspencer Jan 7, 2018

zspencer Jan 7, 2018

bhaibel Jan 8, 2018

zspencer Jan 8, 2018

zspencer Jan 8, 2018 •

edited

Loading

bhaibel Jan 8, 2018

zspencer Jan 8, 2018

zspencer Jan 8, 2018

zspencer Jan 8, 2018

bhaibel left a comment

bhaibel Jan 8, 2018

zspencer Jan 8, 2018

bhaibel Jan 8, 2018

bhaibel Jan 8, 2018

Supporters may make one off payments towards a project #98

Are you sure you want to change the base?

Supporters may make one off payments towards a project #98

Conversation

zspencer commented Jan 4, 2018 • edited Loading

zspencer Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zspencer Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhaibel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zspencer commented Jan 4, 2018 •

edited

Loading

zspencer Jan 4, 2018 •

edited

Loading

zspencer Jan 8, 2018 •

edited

Loading