[Stripe, VCR] Updates credit_card_remover_spec.rb

[filipefurtad0]

Closes #12001, as I think:

./spec/lib/stripe/credit_card_remover_spec.rb requires ./lib/stripe/credit_card_clone_destroyer.rb to work.

Not too sure my approach is correct:

• maybe we need to create clones, before deleting cards?
• while I feel we replace the previously existing test assertions, could we actually verify cards are deleted? I can't seem to find this in the VCR calls, when running our code, I only find the HTTP method delete called over customers... not sure.

Still, I think this provides value and perhaps some food for thought.

What? Why?

• Closes test files: lib/stripe/* #12001.

• Updates credit_card_remover_spec.rb, by replacing remove stubs with real HTTP/vcr calls.

• Addresses a loose end, from a previous PR, here.

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

[mkllnk]

Great. This is good basic coverage with real interaction. We don't need to cover everything. We can add more specs if we find bugs somewhere.

[dacook]

It's great to see more and more real-world tests, good work!

I have a question to discuss/consider, which doesn't have to be addressed immediately.

± But I think we should update the example description, as suggested below:

[dacook]

Setting a system config, without later un-setting it makes me uncomfortable (it means the test leaves the system in a different state to when it started).
It looks like you developed the with_stripe_setup helper to do this safely, so I would suggest using that.

But... I'm not sure that's even necessary. Do you know why we have a different environment variable for the test api_key? Why don't we just set the test key in STRIPE_INSTANCE_SECRET_KEY for test environments? This will be loaded automatically (like it is on production environments).

[filipefurtad0]

Nice catch. Thanks for this comment.

I've changed the ./config/initializers/stripe.rb on the last commit to take the test API key for test and CI environments. I think this PR would need to be staged and tested, so we're sure the key is still taken for staging and production environments - right? What do you think @dacook?

[dacook]

Thanks, that simplifies things, and clarifies my suggestion a little bit.

My suggestion was to go even further, to remove the variable STRIPE_SECRET_TEST_API_KEY altogether.
We can then add STRIPE_INSTANCE_SECRET_KEY to .env.test.local, and add the secret test key there.

Then I think we wouldn't need to change ./config/initializers/stripe.rb at all, meaning we don't need any manual testing.

[rioug]

Nice one @filipefurtad0 !
I added a few suggestions, see my comments.

[rioug]

Same as above

[filipefurtad0]

Hum. I believe I must have screwed up my git history: this should not be here, in fact it is possible to delete the customer within a before block, and then run the test... I'll try to replace this stub as well. Moving back to in dev.

[filipefurtad0]

while I feel we replace the previously existing test assertions, could we actually verify cards are deleted? I can't seem to find this in the VCR calls, when running our code, I only find the HTTP method delete called over customers... not sure.

Hum. I think I see why we're not really deleting cards, if we call this separately, we get:

pry(#<RSpec::ExampleGroups::StripeCreditCardRemover::Remove::StripeCustomerExists::AndIsNotDeleted>)> Stripe::CreditCardCloneDestroyer.new.destroy_clones(credit_card)
=> []

So, there are no clones, because none were created. So the tests are only catching customer deletion. Maybe this is ok for now, but the descriptions on the it blocks clearly state otherwise.

Still, I guess the PR brings improvements, and reviews have been addressed. I'd move to code review (and open an issue to explicitly address card deletion if we opt for merging this one as is).

[dacook]

Great job, I agree that this is a good improvement and we can proceed with other changes for deleting cloned cards in a new PR.

As you suggested, it would be a good idea to manually test, because we're updating the stipe initialiser.
In that case, it might be worth considering my comment, which removes the need for testing. I'll move to in dev in case you'd like to try that, but if not (or it doesn't work), then this is fine as it is.

[filipefurtad0]

it might be worth considering my comment, which removes the need for testing

Apologies @dacook , I did not read your comment correctly before:

My suggestion was to go even further, to remove the variable STRIPE_SECRET_TEST_API_KEY altogether.
We can then add STRIPE_INSTANCE_SECRET_KEY to .env.test.local, and add the secret test key there.

The changes on stripe.rb initializer were undone on the last commit. In addition to these changes, we need to change .env.test as this file is used when the tests run in the CI. I think we're good, green build now.

[dacook]

Great, except we need to re-record the cassettes to be sure.
I did this, and found that we need to update the vcr config to ensure the key is filtered out.
I did that, but a couple of specs failed 😢

1. This might be because I was missing STRIPE_CUSTOMER in my config. Can you please check if that is in bitwarden? I also don't have STRIPE_ACCOUNT, should I?

     Stripe::InvalidRequestError:
       No such customer: 'bogus_customer'
     # ./spec/lib/stripe/credit_card_cloner_spec.rb:63:in `block (4 levels) in <module:Stripe>'

2. At first glance, I'm not sure about this one, do you know?

       expected: "requires_capture"
            got: "requires_action"

       (compared using ==)
     Shared Example Group: "payments intents" called from ./spec/lib/stripe/payment_intent_validator_spec.rb:90
     # ./spec/lib/stripe/payment_intent_validator_spec.rb:60:in `block (6 levels) in <main>'

[filipefurtad0]

This might be because I was missing STRIPE_CUSTOMER in my config. Can you please check if that is in bitwarden? I also don't have STRIPE_ACCOUNT, should I?

Yes... I believe this is the case too. The reason for this is that I've mistakenly hard-coded these two on a previous PR, and am proposing the correct approach here.

Maybe we should wait till #12127 is merged, and then rebase this branch? I've taken the liberty to mark this PR as blocked.

2. At first glance, I'm not sure about this one, do you know?

Edited

This relates to the payment state on Stripe (not OFN), I'm not entirely sure why we're getting this, but I think it relates to point 1.; I'd suggest to address it first, and the re-assess point 2.

I've investigated this a bit further, and am not convinced points 1. and 2. are related. At least I don't see it now. I've opened a PR to make the build green again (when re-recording requests).

[sigmundpetersen]

expected: "requires_capture"
got: "requires_action"

This is also discussed here #12113 (comment) for reference

[filipefurtad0]

Alright, quite a journey on this one 😅
Rebased, fixed conflicts, re-recorded cassettes and pinged for re-review.

PS: in case reviewers wish to re-record the tapes -> we need to rename the api key in .env.test.local from STRIPE_SECRET_TEST_API_KEY=... to STRIPE_INSTANCE_SECRET_KEY=... just like it is done for .env.test.

[dacook]

Hooray, great work pulling it all together!