Stripe connect #1468

oeoeaio · 2017-02-23T23:38:30Z

Work by @stveep to connect Stripe Accounts to the main instance, and by me to implement a new Stripe Connect based payment gateway.

I figure that if this all works, we can pull it in now, and then pull the rest of it in later...(#1466 and #1456)

enricostano · 2017-06-06T22:35:57Z

app/controllers/checkout_controller.rb

    clear_ship_address
+    binding.pry


enricostano · 2017-06-06T22:38:00Z

app/controllers/checkout_controller.rb

+
+  def construct_saved_card_attributes
+    existing_card_id = params[:order].delete(:existing_card)
+    if existing_card_id.present?


early return maybe? return unless existing_card_id.present? or something like that.

enricostano · 2017-06-06T22:40:50Z

app/controllers/spree/credit_cards_controller.rb

+
+    def set_user
+      @user = spree_current_user
+    end


Why this? spree_current_user should be memoized already, right? just use it.

enricostano · 2017-06-06T22:45:27Z

app/models/spree/gateway/stripe_connect.rb

+      else
+        payment.send(:gateway_error, response.message)
+      end
+    end


This method seems too long to me, I see the opportunity to split it.

It might be a CreateProfile on its own.

enricostano · 2017-06-06T22:48:29Z

app/models/spree/gateway/stripe_connect.rb

+        # Assume the gateway_payment_profile_id is a token generated by StripeJS
+        return token_or_card_id
+      end
+    end


Conditionals with assignments could lead to weird outcomes in ruby. Try to avoid them if possible, furthermore they make the code less readable IMHO.

What's your preferred pattern to check whether something exists and then use it if it does?

enricostano · 2017-06-06T22:49:11Z

app/models/spree/gateway/stripe_connect.rb

+        token = Stripe::Token.create({card: card, customer: customer}, { stripe_account: stripe_account_id})
+        token.id
+      rescue Stripe::InvalidRequestError
+        # Not really sure what to do here....


Maybe at least log it in stdout?

sauloperez · 2017-06-08T06:34:43Z

app/controllers/admin/enterprises_controller.rb

+        if !(spree_current_user.enterprises.include? @enterprise) && !(spree_current_user.admin?)
+          deauthorize_request_for_stripe_id(response_params["stripe_user_id"])
+          redirect_to '/unauthorized' and return
+        end


I think that lines from 123 until here are business logic that would be better off in a factory method of StripeAccount at least, or even on a different object.

As I see it, we could set up error handling here in the controller to deal with these two exceptions:

def stripe_connect_callback stripe_account = StripeAccountFactory.new(params) if stripe_account.save ... end rescue Stripe::Error redirect_to '/unauthorized' and return end

sauloperez · 2017-06-08T06:39:10Z

app/controllers/enterprises_controller.rb

@@ -10,14 +10,14 @@ class EnterprisesController < BaseController
  before_filter :check_stock_levels, only: :shop

  before_filter :clean_permalink, only: :check_permalink
+  before_filter :set_enterprise, only: :relatives


why don't you call the method from the controller action? I believe single-action before_filters have little to no value. It is easier for the reader to see a method call when reading the action than jumping between the before_filter and the action.

sauloperez · 2017-06-08T06:39:37Z

app/controllers/enterprises_controller.rb

@@ -37,8 +37,14 @@ def check_permalink
    end
  end

+
+


sauloperez · 2017-06-08T06:40:26Z

app/controllers/spree/credit_cards_controller.rb

+  class CreditCardsController < BaseController
+
+    before_filter :set_credit_card, only: [:destroy]
+    before_filter :destroy_at_stripe, only: [:destroy]


related to my comment in enterprises_controller.rb

sauloperez · 2017-06-08T06:42:50Z

app/controllers/spree/credit_cards_controller.rb

+    def destroy
+      if @credit_card.destroy
+        flash[:success] = I18n.t(:card_has_been_removed, number: "x-#{@credit_card.last_digits}")
+        redirect_to "/account#/cards"


Using url helpers here would make it future-proof. In case someday we need to change the route we don't have to update this line.

sauloperez · 2017-06-08T06:43:14Z

app/controllers/spree/users_controller_decorator.rb

@@ -1,3 +1,20 @@
 Spree::UsersController.class_eval do
  layout 'darkswarm'
+  before_filter :set_credit_card, only: :show


See my suggestion in enterprises_controller.rb

sauloperez · 2017-06-08T06:49:23Z

app/helpers/admin/stripe_helper.rb

+      JWT.decode(token, Openfoodnetwork::Application.config.secret_token, true, algorithm: 'HS256')[0] # only returns the original payload
+    end
+  end
+end


I stronlgy believe this module should actually be the abstraction the controllers are lacking. For my experience, I believe that payment gateways need to be encapsulated so as not to leak details throughout the whole app. I see that these methods could be independent service objects like Stripe::Authorize, Stripe::Deauthorize, etc.

sauloperez · 2017-06-08T06:49:43Z

app/models/enterprise.rb

@@ -365,6 +366,7 @@ def can_invoice?
    abn.present?
  end

+


sauloperez · 2017-06-08T06:50:40Z

app/models/spree/gateway/stripe_connect.rb

+      else
+        payment.send(:gateway_error, response.message)
+      end
+    end


It might be a CreateProfile on its own.

sauloperez · 2017-06-22T08:08:34Z

Feel free to dismiss my comments. I know I came late to the party and I don't want to block the PR.

oeoeaio · 2017-06-22T08:12:44Z

@sauloperez, no way. I will definitely be looking at them!

stveep · 2017-07-07T07:21:14Z

app/controllers/spree/admin/base_controller_decorator.rb

@@ -34,7 +34,7 @@ def unauthorized
      redirect_to '/unauthorized'
    else
      store_location
-      redirect_to root_path(anchor: "login?after_login=#{ request.env['PATH_INFO'] }")
+      redirect_to root_path(anchor: "login?after_login=#{request.env['PATH_INFO']}")
    end
  end



This reminds me - I had problems with getting signed out of OFN when going through the Stripe Connect process to connect an account. When I was redirected back to OFN I got prompted to sign in, but was not then redirected to the admin page (and the account was not connected on the OFN side as the request was not successful). I only experienced it locally and I'm not sure what the reason for getting signed out was. I remember playing with using some of the other request params here instead (e.g. #fullpath), but was worried I was going to break everything...

stveep@9f3888c

sauloperez · 2017-07-07T07:37:21Z

app/assets/javascripts/darkswarm/controllers/checkout/payment_controller.js.coffee

  angular.extend(this, new FieldsetMixin($scope))
+  defaultCard = [ {id: null, formatted: t("new_credit_card")} ]
+  $scope.savedCreditCards = defaultCard.concat savedCreditCards if savedCreditCards


you can merge these two objects using Object.assign without having to add the conditional, isn't it?

sauloperez · 2017-07-07T07:40:01Z

app/assets/javascripts/darkswarm/services/checkout.js.coffee

@@ -53,6 +59,20 @@ Darkswarm.factory 'Checkout', (CurrentOrder, ShippingMethods, PaymentMethods, $h
            last_name: @order.bill_address.lastname
        }

+      if @paymentMethod()?.method_type == 'stripe'
+        if @secrets.selected_card


I'd rather use an and here

Sorry @sauloperez, I don't follow. You mean instead of the ?. syntax?

sauloperez · 2017-07-07T07:49:34Z

app/controllers/admin/stripe_connect_settings_controller.rb

+    private
+
+    def load_settings
+      klass = Struct.new(:stripe_connect_enabled)


There's no need to create the Struct on every request. You can assign it to a variable like StripeSettings = Struct.new(:stripe_connect_enabled) just next to the before_filter above and use in this line below. As it is, it has an impact on performance.

sauloperez · 2017-07-07T07:54:01Z

app/controllers/checkout_controller.rb

@@ -201,4 +204,29 @@ def redirect_to_paypal_express_form_if_needed
    render json: {path: spree.paypal_express_url(payment_method_id: payment_method.id)}, status: 200
    true
  end
+
+  def construct_saved_card_attributes
+    existing_card_id = params[:order].delete(:existing_card)


I'd rename the key in the sent JSON to existing_card_id for the sake of consistency. It's good to know at a glance whether we're working with an integer or a string

For reference this was written the way it was because existing_card is the key used by future versions of Spree. We are definitely going to have to deal with lots of other stuff between here and there however, so let's use existing_card_id for now.

sauloperez · 2017-07-07T07:55:45Z

app/controllers/checkout_controller.rb

+    return if existing_card_id.blank?
+
+    credit_card = Spree::CreditCard.find(existing_card_id)
+    if credit_card.try(:user_id).blank? || credit_card.user_id != spree_current_user.try(:id)


how come a credit card is not associated to a user? I would rather enforce it with validations so no credit card is added to the system without a user. This way we don't need to check this every time we need to use the credit card.

@sauloperez Users who check out as a guest can still pay via credit card.

sauloperez · 2017-07-07T08:24:12Z

app/models/stripe_account.rb

+class StripeAccount < ActiveRecord::Base
+  belongs_to :enterprise
+  validates :stripe_user_id, :stripe_publishable_key, presence: true
+  validates :enterprise_id, uniqueness: true


we must enforce this constrain at db level as well.

sauloperez · 2017-07-07T08:27:12Z

app/serializers/api/admin/payment_method_serializer.rb

+  end
+end
+
+module Api::Admin::PaymentMethod


I would move this to its own file for consistency. Future devs might expect finding the base serializer in the app/serializers/api/admin/ directory like all the others.

sauloperez · 2017-07-07T08:27:24Z

app/serializers/api/admin/payment_method_serializer.rb

  end

-  def tags
-    object.tag_list.map{ |t| { text: t } }
+  class StripeSerializer < BaseSerializer


here as well

sauloperez · 2017-07-07T08:32:25Z

lib/stripe/account_connector.rb

+    private
+
+    def state
+      OAuth.send(:jwt_decode, params["state"])


Why don't we make jwt_decode public if we need to use it here?

sauloperez · 2017-07-07T08:36:33Z

spec/controllers/admin/enterprises_controller_spec.rb

+
+            it "returns a 500 error" do
+              spree_get :stripe_connect_callback, params
+              response.status.should be 500


please, we should use the current expect(...).to eq() syntax 🙏

Now in: spec/controllers/admin/stripe_accounts_controller_spec.rb

Resolved

enricostano · 2017-07-10T13:06:34Z

app/controllers/admin/stripe_connect_settings_controller.rb

+    end
+
+    def update
+      Spree::Config.set(params[:settings])


Can you please explain and document this?
What do you expect in params[:settings]?
Is Spree::Config.set() really needed? It will get cached by Rails and affect other requests (and specs).

Updating the cached config is exactly what we're trying to achieve here. This controller action is used by super admin users to update the settings the app is using.

Ah OK, could you please specify that in the method documentation? Thanks!

enricostano · 2017-07-10T13:12:57Z

app/controllers/enterprises_controller.rb

@@ -39,6 +39,10 @@ def check_permalink

  private

+  def set_enterprise
+    @enterprise = Enterprise.find(params[:id])


what if the enterprise does not exist? do we need any authorization ability here?

@enricostano I have changed this to Enterprise.find_by_id so that an error isn't raised when a bad id is provided. This method is only used by EnterprisesController#relatives, logic there already deals with the case that no enterprise is found.

No authorisation required here IMO, as this is just describing the existence of relationships in the network. I think this is fine to be public knowledge.

enricostano · 2017-07-10T13:21:34Z

app/controllers/spree/admin/payment_methods_controller_decorator.rb

+      # current payment_method is already a Stripe method
+      def show_stripe?
+        Spree::Config.stripe_connect_enabled || @payment_method.try(:type) == "Spree::Gateway::StripeConnect"
+      end


⚠️ DISCLAIMER: buzz words ahead ⚠️

I think there is room here to apply some "tell don't ask", instead of asking all the time if we support Stripe we should just get that info from some class defined starting up the app (strategy pattern maybe?). In that way you'll not have to remember to check that "if Stripe" in the future avoiding bugs and letting the code be more clear and easier to test.

BTW, this is not blocking of course. Just something that would be nice to start seeing in the codebase.

The value of Spree::Config.stripe_connect_enabled is dynamic (can be changed by super admin), so we can't cache the value on app initialisation. Is that what you mean?

enricostano · 2017-07-10T13:23:10Z

app/controllers/spree/credit_cards_controller.rb

+    end
+
+    def set_credit_card
+      @credit_card = Spree::CreditCard.find(params[:id])


What if the card doesn't exist? Any authorization layer needed?

enricostano · 2017-07-10T13:30:01Z

app/models/spree/gateway/stripe_connect.rb

+
+      # def cancel(response_code)
+      #   provider.void(response_code, {})
+      # end


Can we remove commented out code?

enricostano · 2017-07-10T13:32:22Z

app/models/spree/gateway/stripe_connect.rb

+        options = {
+          email: payment.order.email,
+          login: Stripe.api_key,
+        }.merge! address_for(payment)


Not super important, but if you move this hash population to a separate method you'll have more documentation surface and we'll get prepared for the rubocop rules about methods and classes length I hope we'll agree upon some day 😬

oeoeaio · 2017-07-14T00:43:33Z

app/models/spree/payment_decorator.rb

+      return if source_attributes.nil?
+      if payment_method and payment_method.payment_source_class
+        self.source = payment_method.payment_source_class.new(source_attributes)
+        self.source.payment_method_id = payment_method.id


Hey @stveep, I am a bit worried about this line. I don't think it is going to work for us to link instances Spree::CreditCard to a single Spree::PaymentMethod - this will imply that each card can only be used by a single shop, and we want cards to be used by multiple shops.... Was there a particular reason that we need this association as far as you're aware? I realise this method is coming from Spree, so they must have a reason but I suspect it is more for convenience than actual necessity, because the payment itself stores the payment method, so I don't see why the card needs to store it as well...

Maybe @sauloperez can help here too

Bump @stveep

Make sure that completed payment is for an amount less that the order total

…emerchant

Connector now handles cancellation of the connection wizard by the user more elegantly

…ccount from displaying in the checkout

Log deauthorise failures to Bugsnag

…not found

…r failed

This step is not being responded to anyway, since we are not rending a page for each checkout step It was causing an issue whereby an order in the 'confirm' state was not able to progress through the checkout controller because it was expecting to only redirect to paypal from the 'payment' state. figured it was easiest to just remove the step, seeing as it wasn't being used in any meaningful way. It should be fine to bring the 'confirm' step back in the future if we need it, we will just have to make sure paypal the paypal issue is resolved.

oeoeaio mentioned this pull request Feb 23, 2017

Uk/stripe connect #1353

Closed

oeoeaio force-pushed the stripe-connect branch 2 times, most recently from 9183360 to b58381c Compare May 31, 2017 05:48

enricostano reviewed Jun 6, 2017

View reviewed changes

sauloperez reviewed Jun 8, 2017

View reviewed changes

oeoeaio force-pushed the stripe-connect branch 4 times, most recently from 2329e75 to 7c311ff Compare June 22, 2017 05:22

oeoeaio force-pushed the stripe-connect branch from 7c311ff to 34735d1 Compare June 28, 2017 06:21

oeoeaio force-pushed the stripe-connect branch 2 times, most recently from 305f8fa to 1839a0c Compare July 7, 2017 06:06

stveep reviewed Jul 7, 2017

View reviewed changes

sauloperez reviewed Jul 7, 2017

View reviewed changes

enricostano suggested changes Jul 10, 2017

View reviewed changes

oeoeaio force-pushed the stripe-connect branch 2 times, most recently from 842d937 to 7ce7cab Compare July 12, 2017 08:18

oeoeaio commented Jul 14, 2017

View reviewed changes

oeoeaio force-pushed the stripe-connect branch 2 times, most recently from 919a45a to ccafc8f Compare July 14, 2017 08:35

mkllnk deployed to staging July 14, 2017 09:41 Active

daniellemoorhead assigned oeoeaio Jul 19, 2017

daniellemoorhead added the payment methods label Jul 19, 2017

oeoeaio added 26 commits September 23, 2017 14:58

Use expect syntax instead of should

08c723b

Add documentation to StripeConnectSettings controller

5cba635

Use find_by_id instead of find in EnterpriseController

620721c

Refactor ProfileStorer, call options as a separate method

0e42c1c

Use guard clause

0bbaafb

Extract filters for available PMs into service object

1cd1e9d

Fix rubocop issues

098afc6

Add missing translation referenced from spree_auth_devise

fbda1a2

Make request stubs less brittle by using regex

d2096cf

Fix broken Stripe checkout spec

e14b017

Use lowercased cc_type in CreditCard service

4785f3e

Add basic validation to credit card form on account page

34b109c

Fix flaky OrderSerializer spec

393a4ef

Make sure that completed payment is for an amount less that the order total

Configure StripeElements service to map cc_type to that used by activ…

0f19be4

…emerchant

Ensure cart_spec runs in context where backorders are disabled

7d2deca

Update deauthorize webhook to latest version of API

d3566f9

Refactor Stripe::AccountConnector

665749d

Connector now handles cancellation of the connection wizard by the user more elegantly

Show more helpful payment processing error in checkout

7c82fa3

Expose Stripe token creation error messages in Checkout

ba61f94

Update Bogus Gateway error message in checkout spec

7ba99c0

Prevent Stripe-based payment methods that are not linked to a StripeA…

90007d7

…ccount from displaying in the checkout

Update deauthorize webhook to latest version of API (again)

dd3d205

Allow destruction of StripeAccounts even if deauthorise request fails

db5503d

Log deauthorise failures to Bugsnag

Respond to Stripe webhook with status of 204 if specified account is …

e486dbd

…not found

Set transaction fee adjustments to ineligible if payment is invalid o…

a09a54e

…r failed

oeoeaio force-pushed the stripe-connect branch from 544a847 to 75ec77d Compare September 23, 2017 05:15

RohanM merged commit 75ec77d into openfoodfoundation:master Sep 27, 2017

mkllnk requested a deployment to staging September 27, 2017 01:07 Abandoned

RachL mentioned this pull request Feb 17, 2021

Review if we haven't missed any place where customer balances are used #6908

Closed

Stripe connect #1468

Stripe connect #1468

Conversation

oeoeaio commented Feb 23, 2017 • 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

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

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

sauloperez Jun 8, 2017 • 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

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

sauloperez commented Jun 22, 2017

oeoeaio commented Jun 22, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oeoeaio Sep 11, 2017 • 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

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

oeoeaio Sep 11, 2017 • 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

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

Choose a reason for hiding this comment

oeoeaio commented Feb 23, 2017 •

edited

Loading

sauloperez Jun 8, 2017 •

edited

Loading

oeoeaio Sep 11, 2017 •

edited

Loading

oeoeaio Sep 11, 2017 •

edited

Loading