Maintain the same Stripe Customer across multiple cards #77

brchristian · 2020-07-24T22:26:35Z

The initial commit creates a (currently failing) spec to test whether multiple cards used by the same Solidus user produce the same Stripe customer (i.e., the same gateway_customer_profile_id).

This is the beginning of progress to address #26. The following commits resolve the issue.

brchristian · 2020-07-25T05:34:12Z

I've created a fix that addresses #26, and the specs should now be green.

It's a bit inelegant, but I am hoping we can refactor anyway once we move away from ActiveMerchant.

This should solve #26.

brchristian · 2020-07-26T19:49:12Z

Specs are passing for both V2 and V3 with Elements! 🎉

I have never used V3 with Intents before and am unfamiliar with how it works, so I am not sure what to do there. @spaghetticode what do you think?

brchristian · 2020-07-29T16:17:11Z

Now that #78 has been merged, we can make these new specs a bit cleaner / more concise. I have rebased to current master and added a new commit 39fbb69 to use the fill_in_card helper.

spaghetticode

@brchristian thank you so much for this PR! 💳 🎉

The approach looks good to me, I think it just needs some polishing before merging. In general, I noticed that there are some commits that can be squashed, as they modify stuff that was introduced earlier with this PR and IMO they have no real reason to be separated. Then there's some code duplication in the specs that would be great if could be avoided somehow.

Also, it would be great if you could review the commit messages. Generally speaking we tend to prefer commits with a body description and capitalized title. This is a great reference for writing good commit messages, it really helped me in the past: https://chris.beams.io/posts/git-commit/

spec/features/stripe_customer_spec.rb

Gemfile

app/models/spree/payment_method/stripe_credit_card.rb

spaghetticode · 2020-08-07T09:13:05Z

app/models/spree/payment_method/stripe_credit_card.rb

@@ -130,8 +136,8 @@ def create_profile(payment)
          else
            payment.source.update!(
              cc_type: payment.source.cc_type,
-              gateway_customer_profile_id: response.params['id'],
-              gateway_payment_profile_id: response.params['default_source'] || response.params['default_card']
+              gateway_customer_profile_id: options[:customer] ? response.params['customer'] : response.params['id'],


I guess the ternary can become: response.params['customer'] || response.params['id']. Currently response.params['customer'] is nil when not reusing a customer, but we can make it a little more future proof with response.params['customer'].presence, so it will work properly also if we start receiving empty-string values.

In general I would agree with you, but I think in context with the line that follows, it's a bit more complicated. As you can see, both ternaries use the same setup with options[:customer] ? .

So I think there's an argument for keeping the logic parallel between the two, even if the first one could be made shorter.

spec/features/stripe_customer_spec.rb

brchristian · 2020-08-07T20:04:53Z

@spaghetticode Thanks for the code review!

I squashed into two commits (specs + change) and made all of your suggested changes with two exceptions where I've followed up and which you can see above. LMK on those last two and then we should be just about there!

brchristian · 2020-08-08T00:26:12Z

Update: I returned to the specs and was able to dedupe 172 lines by following your suggestion. :)

That just leaves the last question about stripe_credit_card.rb. My personal opinion is to leave it as is, for the reasons I explained on the diff, but if you think it’s better to make that change, LMK and I defer to you.

igorbp · 2020-10-14T15:36:39Z

Hi, I just want to raise a question here :

It's possible to delete a customer in the stripe dashboard, and I couldn't find a way to recreate it with the same ID. When it happens it will break this code. I'm not sure if this should be considered or not.

@brchristian @spaghetticode

spaghetticode · 2020-10-14T18:55:05Z

@igorbp thanks for raising this question 💯

I don't know if it's very common for admins to delete accounts on the stripe dashboards, but I think it's something that we should consider fixing although not necessarily in the first implementation of this feature.

igorbp · 2020-10-14T19:49:09Z

@spaghetticode I agree with you.

Also, I'm aware of #81 and #82. But I think we could move this forward before start the change to the official stripe gem.

brchristian · 2020-10-14T20:59:37Z

Hi @igorbp I second @spaghetticode in saying this is a very good point. Though we might want to visit that in a future PR.

@spaghetticode I agree with @igorbp that it might make sense to merge this before we look at either #81 or #82 which introduce additional changes. Ideally I would like to add some tests as well, because there is certain behavior that differs between Active Merchant and the Stripe Gem – for instance, whether to return nil or throw an exception when card data is bad – which is not part of current specs (or exists in Active Merchant specs but should probably go here as well, especially if we move away from Active Merchant). But I think all of that we can deal with later when thinking about #81 and/or #82.

In the meantime, however, anything else that I can do to help get this over the finish line?

kennyadsl · 2020-10-15T07:39:18Z

Gemfile

@@ -27,6 +27,7 @@ group :development, :test do
  gem "pry-rails"
  gem "ffaker"
  gem "rails-controller-testing"
+  gem "stripe"


Why do we need to introduce this development dependency? Is it just for testing purposes?

@kennyadsl Exactly, I used the Stripe gem in the specs as a source for "ground-truth" customer and payment info.

spaghetticode

@brchristian thanks for addressing the issues ❤️

I think the change is good, I left some more comments on the code.

One more thing, I would expand the commit message for 5d43f as it's currently not explaining the reason of the change. A good description may help people debugging and understanding what's going on in case of unexpected behavior.

spaghetticode · 2020-10-23T09:43:41Z

app/models/spree/payment_method/stripe_credit_card.rb

@@ -119,6 +119,14 @@ def create_profile(payment)
          creditcard = source
        end

+        user_stripe_payment_sources = payment.source.user&.wallet&.wallet_payment_sources&.select do |wps|
+          wps.payment_source.payment_method.type == 'Spree::PaymentMethod::StripeCreditCard'


We can avoid hardcoding the class name here by replacing 'Spree::PaymentMethod::StripeCreditCard' with self.class.name

Great idea! I’ve made the change and force-pushed.

spec/features/stripe_customer_spec.rb

spaghetticode · 2020-10-23T15:54:21Z

spec/features/stripe_customer_spec.rb

@@ -0,0 +1,176 @@
+# frozen_string_literal: true
+


The first 100 lines here are still almost identical to the ones in stripe_checkout.spec. I think that all this duplication may become an issue in the future, as changes in one file will need to be ported to the other, for example if something changes on Solidus FE flow.

Good point. I’ve moved this to a checkout_helper, into a method called initialize_checkout. This deduplicates almost all of the code that was shared between stripe_checkout_spec and stripe_customer_spec.

brchristian · 2020-10-27T15:44:47Z

@spaghetticode I've gone ahead and followed all of your suggestions! I've...

Updated self.class.name in place of the hardcoded string
Deduplicated the spec code into a checkout_helper in the testing_support folder as in Refactor spec with fill_in_card helper #78
Expanded the commit message (which now has the hash 74f5abb).
Rebased to the current master
Force-pushed

...and CI is green!

spaghetticode · 2020-11-06T13:43:45Z

lib/solidus_stripe/testing_support/checkout_helper.rb

@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+module SolidusCheckoutHelper


I think it would be better to have the module name match the filename. Also, since we're in the Solidus Stripe gem, I would rather name the module StripeCheckoutHelper or something similar, what do you think?

@spaghetticode Sounds good. I've renamed to StripeCheckoutHelper and stripe_checkout_helper.rb and have force-pushed the latest!

brchristian · 2021-01-15T21:54:35Z

@spaghetticode What else is needed to push this over the finish line?

spaghetticode · 2021-01-22T14:59:46Z

@brchristian sorry for the late answer. I think the changes look good, but the CI is failing. The failure is not related to your changes, but I think it's best to wait for the green. I'll keep an eye on the fix (either in Solidus or Solidus Support) and ping you back if any rebase/change is needed, thanks!

brchristian · 2021-02-19T22:17:12Z

@spaghetticode Now that #99 has been merged, the specs are looking good except for one issue, that only seems to come up in Solidus v2.10 and seems unrelated to these changes – I also notice it in the failing specs for #92, for example.

Is this just a fluke, or is there more work to be done on master to get that passing? LMK if I can help.

spaghetticode · 2021-05-05T14:27:57Z

@brchristian can you please rebase this branch with master? The postgres build is now failing because of Solidus gem incompatibilities that are solved in master, so hopefully after that it should be passing again. Thanks 🙏

These specs address the need in solidusio#26 for maintaining a stable Stripe customer object across multiple credit cards. Currently V2 and V3 Elements are supported, with V3 Intents functionality remaining as a TODO for a later PR.

Avoid creating a new Stripe customer during checkout in both V2 and V3 Elements by checking to see whether this Solidus user has a previous Stripe payment source. Support for V3 Intents will require further work, and will be included in a subsequent PR.

stale · 2022-11-11T10:52:46Z

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.

kennyadsl · 2023-07-12T14:39:34Z

I think we can close this one. It is covered in the new stripe version (v5). Thanks anyway!

brchristian mentioned this pull request Jul 24, 2020

Reuse Stripe profiles when customer adds a new credit card #26

Open

brchristian changed the title ~~add Stripe Customer spec~~ Maintain the same Stripe Customer across multiple cards Jul 25, 2020

brchristian force-pushed the customer_spec branch from f1b4331 to 39fbb69 Compare July 29, 2020 16:15

spaghetticode reviewed Aug 7, 2020

View reviewed changes

brchristian force-pushed the customer_spec branch 2 times, most recently from 337451a to 8eb6d6f Compare August 7, 2020 20:01

brchristian force-pushed the customer_spec branch 2 times, most recently from 6ca923d to 5d43fed Compare August 8, 2020 00:15

This was referenced Aug 15, 2020

Refactor create_profile to use Stripe gem #81

Open

Sync Solidus Users and Stripe Customers #82

Open

kennyadsl reviewed Oct 15, 2020

View reviewed changes

spaghetticode reviewed Oct 23, 2020

View reviewed changes

brchristian force-pushed the customer_spec branch 2 times, most recently from 1f10984 to 59f152b Compare October 27, 2020 04:35

spaghetticode reviewed Nov 6, 2020

View reviewed changes

brchristian force-pushed the customer_spec branch from 59f152b to 0fe8990 Compare November 6, 2020 17:14

brchristian force-pushed the customer_spec branch from 0fe8990 to 78b5e66 Compare January 25, 2021 21:46

brchristian force-pushed the customer_spec branch from 78b5e66 to 6ea3fa1 Compare February 17, 2021 03:54

aldesantis requested review from spaghetticode and kennyadsl May 5, 2021 09:51

brchristian added 3 commits May 5, 2021 08:53

Add specs for Stripe customer accounts

3870668

These specs address the need in solidusio#26 for maintaining a stable Stripe customer object across multiple credit cards. Currently V2 and V3 Elements are supported, with V3 Intents functionality remaining as a TODO for a later PR.

Dedupe shared code in checkout specs

be15193

brchristian force-pushed the customer_spec branch from 6ea3fa1 to be15193 Compare May 5, 2021 15:54

stale bot added the stale label Nov 11, 2022

gsmendoza removed the stale label Nov 11, 2022

elia added the v4.x Issues and PRs targeting the classic frontend (before v5) label Nov 29, 2022

elia changed the base branch from master to v4 December 20, 2022 10:25

kennyadsl closed this Jul 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintain the same Stripe Customer across multiple cards #77

Maintain the same Stripe Customer across multiple cards #77

brchristian commented Jul 24, 2020 •

edited

Loading

brchristian commented Jul 25, 2020 •

edited

Loading

brchristian commented Jul 26, 2020

brchristian commented Jul 29, 2020

spaghetticode left a comment

spaghetticode Aug 7, 2020

brchristian Aug 7, 2020

brchristian commented Aug 7, 2020

brchristian commented Aug 8, 2020

igorbp commented Oct 14, 2020

spaghetticode commented Oct 14, 2020

igorbp commented Oct 14, 2020

brchristian commented Oct 14, 2020

kennyadsl Oct 15, 2020

brchristian Oct 15, 2020

spaghetticode left a comment

spaghetticode Oct 23, 2020

brchristian Oct 27, 2020

spaghetticode Oct 23, 2020

brchristian Oct 27, 2020

brchristian commented Oct 27, 2020

spaghetticode Nov 6, 2020

brchristian Nov 6, 2020

brchristian commented Jan 15, 2021

spaghetticode commented Jan 22, 2021

brchristian commented Feb 19, 2021

spaghetticode commented May 5, 2021

stale bot commented Nov 11, 2022

kennyadsl commented Jul 12, 2023

		@@ -0,0 +1,57 @@
		# frozen_string_literal: true

		module SolidusCheckoutHelper

Maintain the same Stripe Customer across multiple cards #77

Maintain the same Stripe Customer across multiple cards #77

Conversation

brchristian commented Jul 24, 2020 • edited Loading

brchristian commented Jul 25, 2020 • edited Loading

brchristian commented Jul 26, 2020

brchristian commented Jul 29, 2020

spaghetticode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brchristian commented Aug 7, 2020

brchristian commented Aug 8, 2020

igorbp commented Oct 14, 2020

spaghetticode commented Oct 14, 2020

igorbp commented Oct 14, 2020

brchristian commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spaghetticode 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

brchristian commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brchristian commented Jan 15, 2021

spaghetticode commented Jan 22, 2021

brchristian commented Feb 19, 2021

spaghetticode commented May 5, 2021

stale bot commented Nov 11, 2022

kennyadsl commented Jul 12, 2023

brchristian commented Jul 24, 2020 •

edited

Loading

brchristian commented Jul 25, 2020 •

edited

Loading