Sync Solidus Users and Stripe Customers #82
Conversation
brchristian
force-pushed
the
stripe_gem_users
branch
from
efdd711
to
c28c1c2
Compare
|
|
||
| class AddStripeCustomerIdToUsers < SolidusSupport::Migration[5.1] | ||
| def up | ||
| add_column :spree_users, :stripe_customer_id, :string, unique: true |
There was a problem hiding this comment.
There's a concern regarding this migration. On custom authentications, the table may not be :spree_users (or Spree::User model). This migration would need to find the correct user class through Spree.user_class.
There was a problem hiding this comment.
Good point! I changed :spree_users to Spree.user_class.table_name.to_sym.
| add_column :spree_users, :stripe_customer_id, :string, unique: true | ||
|
|
||
| Spree::User.includes(bill_address: :country, ship_address: :country).find_each do |u| | ||
| user_stripe_payment_sources = user&.wallet&.wallet_payment_sources&.select do |wps| |
There was a problem hiding this comment.
I think user here is meant to be u or probably the other way around, isn't it?
There was a problem hiding this comment.
Good catch, thank you! I changed it to u.
There was a problem hiding this comment.
Hi @brchristian. This looks like a good start toward supporting the official stripe gem :)
I've added a few comments regarding the migration.
brchristian
force-pushed
the
stripe_gem_users
branch
from
c28c1c2
to
eb5f9c2
Compare
|
Thanks @igorbp! I appreciate the careful look. I've made both of those suggested changes and have force-pushed this PR. |
There was a problem hiding this comment.
Hey @brchristian. Thanks for considering my comments last time. :)
I made a few more suggestions that you might want to take a look at. I would be glad to hear your opinion about it.
|
|
||
| def self.prepended(base) | ||
| base.after_create :create_stripe_customer | ||
| base.after_update :update_stripe_customer |
There was a problem hiding this comment.
I think we could create/update the stripe_customer only during a payment done through stripe. Also, with those callbacks, every change in the 'User' will trigger an update for the stripe_customer which in some cases won't be necessary.
| # frozen_string_literal: true | ||
|
|
||
| module Spree | ||
| module UserDecorator |
There was a problem hiding this comment.
What do you think about having a dedicated class to deal with the StripeCustomer management(creation, update, delete, etc)? I don't think the User class should have knowledge about the Stripe integration, even through a decorator.
|
|
||
| def create_stripe_customer | ||
| stripe_customer = Stripe::Customer.create(self.stripe_params) | ||
| update_column(:stripe_customer_id, stripe_customer.id) |
There was a problem hiding this comment.
Is there a reason for using update_column instead of the normal update? I don't think we should skip validations and callbacks here.
| @@ -0,0 +1 @@ | |||
| Stripe.api_key = Spree::PaymentMethod::StripeCreditCard.last&.preferences&.dig(:secret_key) | |||
There was a problem hiding this comment.
The Stripe secret_key can also be defined in the database, which means it could be changed in the runtime. In that case, this would still have the old key. It would be better to define this during the runtime. This is also a good indication that we might need a class to handle the integration.
| def up | ||
| add_column Spree.user_class.table_name.to_sym, :stripe_customer_id, :string, unique: true | ||
|
|
||
| Spree::User.includes(bill_address: :country, ship_address: :country).find_each do |u| |
There was a problem hiding this comment.
This part could be in a different migration. I believe with the custom user class situation, some users would want to deal with it themselves, and would be easier to skip a migration instead of comment this one.
Solves solidusio#26. Begins to solve solidusio#74.
brchristian
force-pushed
the
stripe_gem_users
branch
from
eb5f9c2
to
32472ab
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It might be closed if no further activity occurs. Thank you for your contributions. |
Solves #26.
Begins the solution of #74.
NOTE: This PR is an alternative (IMO a cleaner and superior one) to both #77 and #81.
The idea is that the
gateway_customer_idreally represents a mapping betweenSpree::UsersandStripe::Customers. As Stripe puts it: "You should create a Customer object when your customer creates an account with your business." This achieves that goal, withSpree::UsersandStripe::Customernow linked.This allows us to create a very clean implementation of preserving the Stripe::Customer across multiple Solidus purchases (with both the same and different cards), across V2, Elements, and Intents. I believe it is an even better and more principled/elegant solution to #26 than either #77 and #81.
This also begins the process of shifting the
solidus_stripeextension from its dependency on ActiveMerchant onto the official Stripe gem, which when complete will be I think a terrific simplification as well as giving us greater power and flexibility in how Solidus stores can take the fullest advantage of Stripe.