Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaces Stripe Stubs with VCR calls #11924

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

filipefurtad0
Copy link
Contributor

@filipefurtad0 filipefurtad0 commented Dec 11, 2023

Replaces stubs on VCR cassettes. We do this here for the file ./spec/models/spree/gateway/stripe_sca_spec.rb.

What? Why?

Instead of asserting on our DB we can call Stripe::PaymentIntent.retrieve to actually be sure that the Stripe payment changes status, by running our code 馃帀 -> rookie optimism :-)

Edit: we can we'll just do the above and assert on the ActiveMerchant response...

What should we test?

  • green build

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@filipefurtad0 filipefurtad0 added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Dec 11, 2023
@filipefurtad0 filipefurtad0 self-assigned this Dec 11, 2023
Adds new VCR cassettes

Keeps assertion on ActiverMerchant response

Re-records cassettes
@filipefurtad0
Copy link
Contributor Author

filipefurtad0 commented Dec 11, 2023

I have to admit I don't know how to deal with the describe "#external_payment_url" do group... I'd leave as it is for now, unless anyone has a hint on how to proceed?

Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for working through this is making our tests check the real-world responses! 馃挭

I have a couple of queries, and a suggestion for tidying up.

spec/models/spree/gateway/stripe_sca_spec.rb Outdated Show resolved Hide resolved
spec/models/spree/gateway/stripe_sca_spec.rb Outdated Show resolved Hide resolved
spec/models/spree/gateway/stripe_sca_spec.rb Outdated Show resolved Hide resolved
spec/models/spree/gateway/stripe_sca_spec.rb Show resolved Hide resolved
Fixes typo on capture_amount declaration
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code is good now, but I'll also request a review from Maikel, as he has been discussing this with you.

@dacook dacook requested a review from mkllnk December 12, 2023 22:29
@mkllnk mkllnk merged commit 55ff376 into openfoodfoundation:master Dec 13, 2023
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Integration/API tests] app/models/spree/gateway/stripe_sca.rb
3 participants