Fix Rspec warnings and the broken Stripe intent API specs that fixing them revealed

[deivid-rodriguez]

What? Why?

This is a pretty deep rabbit hole.

It all started trying to fix the RSpec warnings that show up when running bundle exec rspec spec/lib/stripe/payment_intent_validator_spec.rb:

(...)

Stripe::PaymentIntentValidator
  #call
    as a guest
      when payment intent is valid
        valid non-3D credit cards are correctly handled
          behaves like payments intents
            from Visa
WARNING: Using `expect { }.not_to raise_error(SpecificErrorClass)` risks false positives, since literally any other error would cause the expectation to pass, including those raised by Ruby (e.g. `NoMethodError`, `NameError` and `ArgumentError`), meaning the code you are intending to test may not even get reached. Instead consider using `expect { }.not_to raise_error` or `expect { }.to raise_error(DifferentSpecificErrorClass)`. This message can be suppressed by setting: `RSpec::Expectations.configuration.on_potential_false_positives = :nothing`. Called from /Users/deivid/code/openfoodfoundation/openfoodnetwork/spec/lib/stripe/payment_intent_validator_spec.rb:53:in `block (7 levels) in <main>'.
              returns payment intent id and does not raise
              captures the payment
          behaves like payments intents
            from Visa (debit)
WARNING: Using `expect { }.not_to raise_error(SpecificErrorClass)` risks false positives, since literally any other error would cause the expectation to pass, including those raised by Ruby (e.g. `NoMethodError`, `NameError` and `ArgumentError`), meaning the code you are intending to test may not even get reached. Instead consider using `expect { }.not_to raise_error` or `expect { }.to raise_error(DifferentSpecificErrorClass)`. This message can be suppressed by setting: `RSpec::Expectations.configuration.on_potential_false_positives = :nothing`. Called from /Users/deivid/code/openfoodfoundation/openfoodnetwork/spec/lib/stripe/payment_intent_validator_spec.rb:53:in `block (7 levels) in <main>'.
              returns payment intent id and does not raise
              captures the payment
          behaves like payments intents
            from Mastercard
WARNING: Using `expect { }.not_to raise_error(SpecificErrorClass)` risks false positives, since literally any other error would cause the expectation to pass, including those raised by Ruby (e.g. `NoMethodError`, `NameError` and `ArgumentError`), meaning the code you are intending to test may not even get reached. Instead consider using `expect { }.not_to raise_error` or `expect { }.to raise_error(DifferentSpecificErrorClass)`. This message can be suppressed by setting: `RSpec::Expectations.configuration.on_potential_false_positives = :nothing`. Called from /Users/deivid/code/openfoodfoundation/openfoodnetwork/spec/lib/stripe/payment_intent_validator_spec.rb:53:in `block (7 levels) in <main>'.
              returns payment intent id and does not raise
(...)

The warning makes a lot of sense and I think the best solution is to completely remove the not_to raise_error(SpecificErrorClass) expectation: if an error happens, the spec should always fail.

Removing those expectations surfaced that the spec was failing a the very beginning when setup up the DB objects needed for it, so the expectations were not even running.

This PR fixes the successive errors I run into until getting the specs back to green without warnings. The different fixes are explained in the different commit messages.

What should we test?

I'd like a second opinion that the specs rewritten to not use raw credit card data still test what they are supposed to test.

Release notes

• Technical changes only

[filipefurtad0]

Awesome work.
Thank you for fixing this @deivid-rodriguez 👏 👏 👏

[deivid-rodriguez]

No problem @filipefurtad0 😄

Currently the commit history for this PR is a bit verbose because in each commit I was trying to highlight the successive errors I was getting. It has the gotcha that not every commit passes tests, which could affect bisecting the code base if ever needed, for example. I'm happy to squash commits a bit so that all of them pass tests if needed.

Also, the failing "linters" job is getting addressed through other reviewdog related PRs that I have opened 👍.

[filipefurtad0]

commit history for this PR is a bit verbose because in each commit I was trying to highlight the successive errors I was getting

I find that's totally justified and helpful here, it makes it clearer to review, so no need to squash on my side - but let's see what others think :-)

[dacook]

Great work, thank you for going down into the rabbit hole. The result is better specs, hooray! 🎉
I just have a couple of suggestions to rename a variable and add a comment.

[dacook]

Nice!

[dacook]

As this relates to payments, I'll await one more team member's review

[deivid-rodriguez]

Thank you @dacook! If I could get some opinions on #13643 and #13645 and there's will to move those forward, it'd be nice to introduce them first and then rebase this one to prove that the linters check gets fixed. That said, it starts to be a pretty deep prerequisite chain, because we've already set this PR as a dependency of #13493 😅