-
Notifications
You must be signed in to change notification settings - Fork 438
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
WIP: Add support for Stripe Payment Element #409
base: main
Are you sure you want to change the base?
Conversation
def self.prepended(base) | ||
# Added the 'intent' state to allow payment gateway to handle creation of payment intent. | ||
# Overridden here for now, but assume this would be better sitting in core. | ||
base.state_machine initial: :checkout do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we need this additional state related to intent, or if we could just save a Payment
object for StripeElementsGateway only if the payment was successfully created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if that needs to happen, this would probably be better of in core. This will be a breaking change though for other payment gateways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafalcymerys Yes, I was unsure if there was a way to avoid adding the extra 'intent' payment state. In the end I felt like it was significant enough to warrant a separate state as it may apply to other payment gateways, and seemed to make sense as a way to call the Stripe Gateway. If you were to remove the 'intent' state, how would you suggest calling the Stripe Gateway to create the payment intent prior to save? Would it be some sort of before_action related to payment? And would it go via the spree_gateway?
@@ -4,7 +4,7 @@ class Engine < Rails::Engine | |||
|
|||
config.autoload_paths += %W(#{config.root}/lib) | |||
|
|||
initializer "spree.gateway.payment_methods", :after => "spree.register.payment_methods" do |app| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, good point
@@ -30,6 +30,7 @@ Gem::Specification.new do |s| | |||
|
|||
s.add_dependency 'spree_core', '>= 3.7.0' | |||
s.add_dependency 'spree_extension' | |||
s.add_dependency 'stripe' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, so that's not already included?
I would suggest locking this dependency to a particular version - Gemfile.lock will be created in projects using this gem, and they may then resolve to an incompatible version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I only added this to make manipulating the object coming back from Stripe via webhook easier. ActiveMerchant handles the rest of the Stripe <--> Spree logic, so was not previously included. Happy to add the current version number once we've got the other points 'locked down' (pun intended).
@rafalcymerys @damianlegawiec
I have been working on integrating Stripe payment element with Spree Vue (see vuestorefront-community/spree#257). Here is where I am at with the spree side. Would be great to get your feedback on my current approach, and any suggestions.
Some key points:
TO DO: