solidusio / solidus_stripe

💳 Integrate Solidus with Stripe
https://stripe.com
BSD 3-Clause "New" or "Revised" License
36 stars 62 forks source link

Fix `solidus_gateway` to `solidus_stripe` preference migration (v4) #297

Closed benjaminwil closed 1 year ago

benjaminwil commented 1 year ago

Summary

This pull request resolves #273 for v4 of the solidus_stripe extension.

It edits a migration. The migration as written did not not successfully migrate payment method preferences that were created with solidus_gateway to preferences for use with solidus_stripe.

Checklist

The following are mandatory for all PRs:

benjaminwil commented 1 year ago

I'm requesting that we push this as v4.4.1 seeing as it's unlikely we'll do another minor version of v4.

benjaminwil commented 1 year ago

I'm unsure whether CI can pass for this branch?

elia commented 1 year ago

@benjaminwil we need to drop testing solidus master in this branch, after that I think it should pass

benjaminwil commented 1 year ago

@elia Got it working ✨

I ended up just copying the v5 CircleCI configuration and removing the the documentation generation step.

kennyadsl commented 1 year ago

@benjaminwil for some reason specs are not part of the checks now, I can only see the linter passing. Can you please double-check?

benjaminwil commented 1 year ago

After a small amount of suffering I was able to fix the CircleCI workflows.

Unfortunately, tests seem to be failing against Solidus 3.4. I think it's reasonable for v4 not to support Solidus v3.4, so I could add a commit to deal with that if y'all think that's acceptable.

benjaminwil commented 1 year ago

Added another commit to drop support for ~> 3.3. If this is not acceptable I'm happy to drop it!

kennyadsl commented 1 year ago

@benjaminwil thanks again! What was the problem with Solidus v3.4, I thought SolidusStripe v4 should work with that version, can you help me understand?

benjaminwil commented 1 year ago

@kennyadsl On second look, I think it should work. It's just that CI is not passing because of how some tests were written, it seems:

Failures:

  1) Spree::PaymentMethod::StripeCreditCard capture with payment class gets correct amount
     Failure/Error: payment.capture!
       #<Double "success_response"> received unexpected message :message with (no args)
     # ./spec/models/spree/payment_method/stripe_credit_card_spec.rb:272:in `block (3 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # Psych::DisallowedClass:
     #   Tried to dump unspecified class: RSpec::Mocks::Double
     #   ./spec/models/spree/payment_method/stripe_credit_card_spec.rb:272:in `block (3 levels) in <top (required)>'

  2) SolidusStripe::CreateIntentsPaymentService#call when there are previous pending payments when one of them is a Payment Intent invalidates it
     Failure/Error: current_order.payments.pending.where(payment_method: stripe).each(&:void_transaction!)
       #<Double (anonymous)> received unexpected message :message with (no args)
     # ./app/models/solidus_stripe/create_intents_payment_service.rb:37:in `invalidate_previous_payment_intents_payments'
     # ./app/models/solidus_stripe/create_intents_payment_service.rb:14:in `call'
     # ./spec/models/solidus_stripe/create_intents_payment_service_spec.rb:61:in `block (3 levels) in <top (required)>'
     # ./spec/models/solidus_stripe/create_intents_payment_service_spec.rb:116:in `block (6 levels) in <top (required)>'
     # ./spec/models/solidus_stripe/create_intents_payment_service_spec.rb:116:in `block (5 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # Psych::DisallowedClass:
     #   Tried to dump unspecified class: RSpec::Mocks::Double
     #   ./app/models/solidus_stripe/create_intents_payment_service.rb:37:in `invalidate_previous_payment_intents_payments'

Finished in 2 minutes 58.3 seconds (files took 4.98 seconds to load)
72 examples, 2 failures

Failed examples:

rspec ./spec/models/spree/payment_method/stripe_credit_card_spec.rb:275 # Spree::PaymentMethod::StripeCreditCard capture with payment class gets correct amount
rspec ./spec/models/solidus_stripe/create_intents_payment_service_spec.rb:115 # SolidusStripe::CreateIntentsPaymentService#call when there are previous pending payments when one of them is a Payment Intent invalidates it

Randomized with seed 9825

Coverage report generated for RSpec to /home/circleci/project/coverage. 339 / 360 LOC (94.17%) covered.
Stopped processing SimpleCov as a previous error not related to SimpleCov has been detected
/usr/local/bin/ruby -I/home/circleci/project/vendor/bundle/v3.4/ruby/3.0.0/gems/rspec-core-3.12.2/lib:/home/circleci/project/vendor/bundle/v3.4/ruby/3.0.0/gems/rspec-support-3.12.0/lib /home/circleci/project/vendor/bundle/v3.4/ruby/3.0.0/gems/rspec-core-3.12.2/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb --format progress --format RspecJunitFormatter --out test-results/solidus-v3.4/results.xml failed

Exited with code exit status 1

Edit: I believe I've resolved this now.

benjaminwil commented 1 year ago

I don't have permission to re-run CI, but when I ran the feature tests locally I saw the same error.

I don't know what would have changed to make this start happening, and I haven't tracked down any known issues like this yet:

      25.1) Failure/Error: visit spree.root_path

            ArgumentError:
              wrong number of arguments (given 2, expected 0..1)
            # ./spec/features/stripe_checkout_spec.rb:26:in `block (2 levels) in <top (required)>'

      25.2) Failure/Error: self.session_name, self.specified_session = previous_session_info.values_at(:session_name, :specified_session)

            NoMethodError:
              undefined method `values_at' for nil:NilClass
            # /Users/bw/Dev/solidus_stripe/vendor/bundle/ruby/3.0.0/gems/capybara-3.39.0/lib/capybara.rb:378:in `ensure in using_session'
            # /Users/bw/Dev/solidus_stripe/vendor/bundle/ruby/3.0.0/gems/capybara-3.39.0/lib/capybara.rb:379:in `using_session'
            # /Users/bw/Dev/solidus_stripe/vendor/bundle/ruby/3.0.0/gems/capybara-screenshot-1.0.26/lib/capybara-screenshot/capybara.rb:7:in `using_session'
            # /Users/bw/Dev/solidus_stripe/vendor/bundle/ruby/3.0.0/gems/capybara-screenshot-1.0.26/lib/capybara-screenshot/rspec.rb:55:in `after_failed_example'
            # /Users/bw/Dev/solidus_stripe/vendor/bundle/ruby/3.0.0/gems/capybara-screenshot-1.0.26/lib/capybara-screenshot/rspec.rb:105:in `block (2 levels) in <top (required)>'
kennyadsl commented 1 year ago

Ah, one small thing: maybe we should change the Don't support Solidus newer than v3.4 commit message and description now?

benjaminwil commented 1 year ago

@kennyadsl I think the commit message and description are correct: it used to say newer than v3.3, but since I resolved the failing tests I think v3.4 would be the last minor version supported. Is that true?

kennyadsl commented 1 year ago

@benjaminwil oh, sorry, I misread them. We are good!

benjaminwil commented 1 year ago

Looks like another release sneaked in since I last thought about this.

Going to abandon this, as we've already spent too much time on this and it probably doesn't really matter to anyone. Hopefully the existence of this pull request is documentation enough for anyone that runs into this issue shrug.