MVP implementation that can complete a checkout by elia · Pull Request #156 · solidusio/solidus_stripe

elia · 2022-12-30T15:29:10Z

Summary

Aiming at the simplest implementation that allows completing a checkout.
The mapping between Stripe and Solidus models won't be final and subject to deep changes.
We don't expect anyone grabbing this extension from master given the big warning against that that we have at the top of the readme.

Fixes #152

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

The following are not always needed (~~cross them out~~ if they are not):

I have added automated tests to cover my changes.
I have attached screenshots to demo visual changes.
I have opened a PR to update the guides.
I have updated the README to account for my changes.

Follow the same style we already have for the dummy-app script.

This is only useful for development. It runs again the installer whenever one of the relevant sources changes, and only runs the parts of the installers necessary for an update, which is faster than running everything.

- publishable_key - partial names - factory with spec

kennyadsl

Thank @elia, this is a great start. I left just some questions/comments.

Also, now that we are fresh with some concept, I think this might be a good moment to add some YARD documentation to the main gateway/payment method methods that could help us in the future. Do you think this might have some value?

kennyadsl · 2023-01-20T10:58:18Z


 module SolidusStripe
  class PaymentSource < ::Spree::PaymentSource
+    def gateway_payment_profile_id=(value)


Thoughts on using https://apidock.com/rails/Module/alias_attribute?

kennyadsl · 2023-01-20T11:00:05Z

+      end
+
+      # To create a requires_capture PaymentIntent,
+      # see our guide at: https://stripe.com/docs/payments/capture-later


Is our referring to Stripe here? Maybe we can be more explicit about it.

kennyadsl · 2023-01-20T11:01:48Z

+Set `SOLIDUS_STRIPE_API_KEY` and `SOLIDUS_STRIPE_PUBLISHABLE_KEY` environment variables (e.g. via `direnv`), this
+will trigger the default initializer to create a static preference for SolidusStripe.
+
+Run `bin/dev` to start both the sandbox rail server and the file watcher that will update the sandbox whenever


What if bin/dev also creates the payment method if needed? I think it would be a great UX improvement but don't know if that's easy to do. Thoughts?

Added the static pref that for some reason was left out of the PR and bin/sandbox will auto-create the payment method connecting to the static pref if the same env var is present. 👍

kennyadsl · 2023-01-20T11:03:12Z

    end

+    def create_profile(payment)
+      pi = payment.source.payment_intent


I'd prefer to avoid 2-letter variables. payment_intent is not that long that will create problems IMHO, thoughts?

kennyadsl · 2023-01-20T11:07:23Z

+
+require 'solidus_stripe_spec_helper'
+
+RSpec.describe SolidusStripe::PaymentIntentsController, type: :request do


If I'm not mistaken here, the best practice is to avoid adding the _controller part to the filename and to the main describe name. In general, the file seems too tied to the controller, while requests specs should be more of a full request-cycle spec.

I just checked the RSpec generator as I wasn't sure about what changes I added, this is how it looks like:

require 'rails_helper' RSpec.describe "<%= class_name.pluralize %>", <%= type_metatag(:request) %> do describe "GET /<%= name.underscore.pluralize %>" do it "works! (now write some real specs)" do get <%= index_helper %>_path expect(response).to have_http_status(200) end end end

So the only thing I changed was removing the quotes from around the class name so I could use described_class, happy to change it if there's a different/better way.

Yes, but I think that the class should not be a controller in the request specs. Looking around for examples it seems like it's a more high level concept that a specific class.

kennyadsl · 2023-01-20T11:08:47Z

+>
+  <div data-solidus-stripe-payment-target="paymentElement">
+    <!-- Elements will create form elements here -->
+    <em>Loading...</em>


Good candidate for I18n?

kennyadsl · 2023-01-20T11:10:01Z

+        <dt><%= SolidusStripe::PaymentSource.human_attribute_name(:stripe_payment_intent_id) %>:</dt>
+        <dd><%= link_to(
+          payment.source.stripe_payment_intent_id,
+          "https://dashboard.stripe.com/#{'test/' if payment.payment_method.preferred_test_mode}payments/#{payment.source.stripe_payment_intent_id}",


Thought about moving this URL generation to the payment source model?

It's shorter (and works better with my editor, but please don't tell anyone! :P).

It will be used by the frontend code to generate a client-secret. In order for the routes to be defined on SolidusStripe we need to isolate the namespace on the extension itself.

Only the happy path is taken into account, support for saving profiles and wallet is yet to be added.

This will allow capturing, voiding, and canceling stripe payments.

Make auto create a payment method in development if the same env var is available.

kennyadsl · 2023-01-23T08:25:05Z

@elia I think you missed the YARD documentation part of my review comment, I can't find any reply to that and nothing changed in the code proposed. 👓

elia · 2023-01-23T17:53:46Z

@elia I think you missed the YARD documentation part of my review comment, I can't find any reply to that and nothing changed in the code proposed. 👓

Thanks for calling that out, I indeed missed the comment.

It's probably early for adding documentation but I did it anyway mostly to have an idea of what it looks like, many gateway methods are still incomplete and will change considerably, but the current situation should be covered accurately.

kennyadsl

Thanks Elia, I'm sure that even if it's going to change, this Yard documentation won't be regretted.

elia self-assigned this Dec 30, 2022

elia linked an issue Dec 30, 2022 that may be closed by this pull request

Implement the standard gateway interfaces using the Stripe gem #152

Closed

6 tasks

elia force-pushed the elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem branch 2 times, most recently from 028c385 to 0da82df Compare January 9, 2023 18:46

elia changed the title ~~Implement the standard gateway interfaces using the stripe gem~~ MVP implementation that can complete a checkout Jan 11, 2023

elia force-pushed the elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem branch from 0da82df to 4a69d77 Compare January 11, 2023 18:45

elia added 3 commits January 12, 2023 14:13

Cleanup the sandbox script

f47df41

Follow the same style we already have for the dummy-app script.

Add a --watch option to the install generator

6a7d661

This is only useful for development. It runs again the installer whenever one of the relevant sources changes, and only runs the parts of the installers necessary for an update, which is faster than running everything.

Add a --sync option to the installer that will just copy the files

410e775

elia force-pushed the elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem branch 3 times, most recently from 5a83713 to 8c07acf Compare January 13, 2023 10:02

Add a comment about why we have stlib gems in the gemfile

5bf23e4

elia force-pushed the elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem branch 2 times, most recently from b95c06e to 6813058 Compare January 16, 2023 12:40

elia added 2 commits January 16, 2023 13:41

Add a bin/dev script that will start both the server and the watcher

28aa006

Add the payment method basic info

e0f0cc1

- publishable_key - partial names - factory with spec

elia force-pushed the elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem branch from 6813058 to 92cc121 Compare January 19, 2023 12:36

elia marked this pull request as ready for review January 19, 2023 12:49

elia force-pushed the elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem branch from 92cc121 to a8bf830 Compare January 19, 2023 12:49

kennyadsl reviewed Jan 20, 2023

View reviewed changes

elia added 9 commits January 20, 2023 17:39

Attach the stripe payment source to a payment intent id

e2a55fd

Add a basic gateway implementation

467f235

Reimplement bin/rspec in ruby

3022fbf

It's shorter (and works better with my editor, but please don't tell anyone! :P).

Add a payment intent creation endpoint

6c8d8d7

It will be used by the frontend code to generate a client-secret. In order for the routes to be defined on SolidusStripe we need to isolate the namespace on the extension itself.

Set a table-name prefix for the whole extension

cbc782b

Fix the Spree namespace reference in the generator

d63c96b

Add a simple checkout flow with specs

8bfc6da

Only the happy path is taken into account, support for saving profiles and wallet is yet to be added.

Add an admin partial for payments

592ec83

This will allow capturing, voiding, and canceling stripe payments.

Adjust a few rubocop rules to our specific needs

fd5c326

elia added 2 commits January 20, 2023 17:44

Setup static payment method prefs if the ENV key is available

e206a15

Make auto create a payment method in development if the same env var is available.

Move the dashboard link generation to PaymentSource

d6a7c23

elia force-pushed the elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem branch from a8bf830 to d6a7c23 Compare January 20, 2023 16:52

elia requested a review from kennyadsl January 20, 2023 16:52

Document the gateway with YARD

9bf87a4

kennyadsl approved these changes Jan 23, 2023

View reviewed changes

elia merged commit 7340550 into master Jan 24, 2023

elia deleted the elia/152-implement-the-standard-gateway-interfaces-using-the-stripe-gem branch January 24, 2023 11:09

elia added this to the v5 milestone Feb 22, 2023


		require 'solidus_stripe_spec_helper'

		RSpec.describe SolidusStripe::PaymentIntentsController, type: :request do

Uh oh!

Uh oh!

Conversation

elia commented Dec 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kennyadsl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kennyadsl commented Jan 23, 2023

Uh oh!

elia commented Jan 23, 2023

Uh oh!

kennyadsl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

elia commented Dec 30, 2022 •

edited

Loading