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

Corrects setup for setup in credit card cloner spec #12127

Conversation

filipefurtad0
Copy link
Contributor

@filipefurtad0 filipefurtad0 commented Feb 6, 2024

We should not need additional hard coded keys other than the API key and the CLIENT_ID;

What? Why?

A PR has been merged with additional hard coded STRIPE_CUSTOMER and STRIPE_ACCOUNT, which seem incorrect to me:

  • customers should be created per demand; if customer are deleted at some point, then re-recording the spec will fail, as the customer_id will be invalid
  • we don't need to state stripe_account in this test; connected accounts should be as well created by demand.

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 force-pushed the corrects_setup_in_credit_card_cloner_spec branch 2 times, most recently from 075cee7 to 65961d0 Compare February 6, 2024 18:54
@filipefurtad0 filipefurtad0 added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Feb 6, 2024
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.

Great, but you are missing the api key now. I needed this to get it working:

  before {
    Stripe.api_key = ENV.fetch('STRIPE_SECRET_TEST_API_KEY', nil)
  }

@filipefurtad0
Copy link
Contributor Author

filipefurtad0 commented Feb 7, 2024

Great, but you are missing the api key now. I needed this to get it working:

Yes, absolutely. I've mixed it up with another PR (#12104, which is not yet merged and might make stating Stripe.api_key on each spec obsolete; we're not there just yet); I've undone the removal of the key statement, and squashed the commit.

We should not need additional hard coded keys other than the API key and the CLIENT_ID; this PR removes hard coded customer ID - creates one instead

Undoes Stripe.api_key deletion

This was a mixup with an ongoing PR in which we remove the need to call api_key in each individual spec.
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.

Great. That works for me now.

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.

Great, so the test is more self-contained now 👍

@dacook dacook merged commit c4ea343 into openfoodfoundation:master Feb 7, 2024
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.

None yet

3 participants