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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaces Stripe stubs with the real account IDs #11950

Merged

Conversation

filipefurtad0
Copy link
Contributor

@filipefurtad0 filipefurtad0 commented Dec 18, 2023

What? Why?

Related to Epic #11890.
Closes #11949.

Sets up the spec with Stripe credentials, so our code can actually make the real calls to Stripe; Records responses.

Also adds STRIPE_ACCOUNT as sensitive data to VCR setup.

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 self-assigned this Dec 18, 2023
@filipefurtad0 filipefurtad0 changed the title Replaces Stripe stubs with the account and customer IDs Replaces Stripe stubs with the account and client IDs Dec 19, 2023
@filipefurtad0 filipefurtad0 marked this pull request as draft December 19, 2023 16:20
end
end

context "when the Stripe API disconnect succeeds" do
before { Stripe.client_id = client_id }

it "destroys the record" do
# returns status 200
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On this example I'm getting status 401 as well:

        (Bugsnag).notify(#<RuntimeError: StripeDeauthorizeFailure>, {:enterprise_id=>177, :stripe_account=>"acct_1FiqEsKuuB1fWySn"})
           expected: 0 times with any arguments
           received: 1 time with arguments: (#<RuntimeError: StripeDeauthorizeFailure>, {:enterprise_id=>177, :stripe_account=>"acct_1FiqEsKuuB1fWySn"})

Not sure why this is happening. To be continued...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here is that apparently, stripe_account is not connected:

        {
          "error": "invalid_client",
          "error_description": "This application is not connected to stripe account <HIDDEN_ACCOUNT>, or that account does not exist."
        }

So, it does no pass the disconnect test. This is the case, even if use create_account from lib/stripe/account_connector.rb... Maybe I should test this file before proceeding here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, got it now - we can actually create a simple account with a direct API call to Stripe, and then pass it as an argument to our deauthorize_and_destroy method defined here.

If we do this, then the account can be disconnected and Bugsnag is not triggered.

Adds STRIPE_ACCOUNT as sensitive data to VCR setup

Rubocop fixes and re-recording of cassettes

Adds bogus client_id to local test file - for CI to run
@filipefurtad0 filipefurtad0 marked this pull request as ready for review December 21, 2023 11:03
@filipefurtad0 filipefurtad0 changed the title Replaces Stripe stubs with the account and client IDs Replaces Stripe stubs with the real account IDs Dec 21, 2023
Re-records cassettes

Creates a bogus publishable key

We need to feed some value to the ENV variables which are picked up from the local test environment, for the build to run. We could also store them all as environment secrets on our repo, but I don'r think this is necessary, as we only run recorderd API/VCR calls on our build, and never real API calls.
@filipefurtad0 filipefurtad0 added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Dec 21, 2023
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines +29 to +33
expect {
stripe_account.deauthorize_and_destroy
}.to change(
StripeAccount.where(stripe_user_id:), :count
).from(1).to(0)
Copy link
Member

Choose a reason for hiding this comment

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

Excellent!

@mkllnk mkllnk merged commit e15893e into openfoodfoundation:master Dec 21, 2023
52 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/stripe_account.rb
2 participants