Bump activemerchant from 1.123.0 to 1.126.0 #9593
Conversation
![dependabot[bot]](https://avatars.githubusercontent.com/in/29110?s=60&v=4){kind=small}
|
This upgrade introduced #9580. We need to solve that problem before we upgrade again. And to solve that problem, we should probably introduce VCR tests. I'll add some random commit to this so that Dependabot won't close it and open a new PR for newer versions. |
|
Hey @mkllnk I think I may have found a relevant spec: Thinking out loud 🦆 Edit: I mean, we're dealing with :js on the browser level, but upgrading active_merchant should not really influence that I think... |
| @@ -180,6 +180,8 @@ | |||
| end | |||
|
|
|||
| it "allows saving a card and re-using it" do | |||
| # TODO: This case fails in production after the activemerchant upgrade. | |||
| # But this test case still passed. We need to make this spec fail correctly. | |||
There was a problem hiding this comment.
Continuing 🦆
Agree this one is relevant as well - here we're indeed dealing with :js (browser). So puffing-billy would be the way to go to intercept these requests.
I'm thinking though if https://github.com/phillbaker/capybara-mechanize would be suitable here too, in the case we would skip js too 🤔
There was a problem hiding this comment.
I don't see how capybara-mechanize would help here. That seems to be a tool to make actual web requests from specs but we want to deal with the requests sent from our app and record real responses to replay.
filipefurtad0
added
pr-staged-uk
|
I think this change could be the culprit: We don't send this param whe we create a payment intent: It's missing from |
Great research, thank you! That's the big problem with monkey-patching code like this. It looks like this code was originally copied to get a newer version while we were still stuck with old ActiveMerchant: We are well passed v1.98 now and can probably just remove that code from our code base. But reproducing the issue with a spec first would give us more confidence that we are not breaking anything else. |
|
There was an attempt at removing the decorator in #8071 , maybe we can find some useful bits in there |
jibees
force-pushed
the
dependabot/bundler/activemerchant-1.126.0
branch
from
0cc7420 to
1f19aca
Compare
filipefurtad0
force-pushed
the
dependabot/bundler/activemerchant-1.126.0
branch
from
1f19aca to
18a6337
Compare
|
@dependabot-bot recreate |
dependabot
bot
force-pushed
the
dependabot/bundler/activemerchant-1.126.0
branch
from
18a6337 to
10a8929
Compare
|
Should we merge (or test before merging) this one? |
|
I think this needs manual testing - I have not managed to reproduce #9580 with a VCR/automated test yet. I think it might need a code change - but perhaps best to verify if the bug is still triggered with this bump. |
|
Still there, checking out:
triggers: And prevents checkout completion. |
|
Thanks @filipefurtad0 ! |
|
Looks like activemerchant is up-to-date now, so this is no longer needed. |
|
ActiveMerchant was accidentally updated in #9850, which closed this PR. But it's been reverted (d5ae5c9), so I'm re-opening this PR. @filipefurtad0 has re-tested:
It looks like we never managed to catch the original error in a spec, so we are still 'unprotected' from whatever happened before. But we can't reproduce it now, so I don't think we could write a new spec now. So it's probably best to just move on. What do you think @filipefurtad0? Is there any other manual testing to be done before we merge? |
dependabot
bot
force-pushed
the
dependabot/bundler/activemerchant-1.126.0
branch
from
10a8929 to
0bf8dd1
Compare
|
Thank you so much for following up on this @dacook 🙏 I'd have a quick test on this PR again, to check if the checkout problem occurs - just to make sure - and post back here. |
|
I was still able to reproduce the error on #9580 (checking out while saving a new card like x-3184) on
Why do we observe this now, and not from staging the RSpec bump #9850? I don't know. For some reason, some PR's I've staged yesterday did not reflect all changes in master (see this example). This can be due to:
I'm not sure. In any case, the error was triggered now on staging-UK, so, I think we're still not ready for this bump. |
|
I'm glad you double-checked, thanks @filipefurtad0 ! Great work on documenting the findings. Looks like we need to be careful with this one. |
dependabot
bot
force-pushed
the
dependabot/bundler/activemerchant-1.126.0
branch
2 times, most recently
from
824c4a1 to
c0cd824
Compare
dependabot
bot
force-pushed
the
dependabot/bundler/activemerchant-1.126.0
branch
from
c0cd824 to
4802f49
Compare
dependabot
bot
force-pushed
the
dependabot/bundler/activemerchant-1.126.0
branch
from
4802f49 to
add7f03
Compare
|
@dependabot rebase |
Bumps [activemerchant](https://github.com/activemerchant/active_merchant) from 1.123.0 to 1.126.0. - [Release notes](https://github.com/activemerchant/active_merchant/releases) - [Changelog](https://github.com/activemerchant/active_merchant/blob/master/CHANGELOG) - [Commits](activemerchant/active_merchant@v1.123.0...v1.126.0) --- updated-dependencies: - dependency-name: activemerchant dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
dependabot
bot
force-pushed
the
dependabot/bundler/activemerchant-1.126.0
branch
from
add7f03 to
632d701
Compare
|
Marked |
|
Superseded by #10801. |
Bumps activemerchant from 1.123.0 to 1.126.0.
Release notes
Sourced from activemerchant's releases.
... (truncated)
Changelog
Sourced from activemerchant's changelog.
... (truncated)
Commits
f7ca1f9Airwallex: Add support fororiginal_transaction_ida9d5b3aRelease v1.126.0 (#4402)eda06d0Add Cartes Bancaires bin ranges (#4398)f5ba1dbAirwallex: Add 3DS Global Support987ac8bMultiple Gateways: Resolvewhen/casebug9d22fb2NMI: Update post URLcb45b38Priority: addsettledandvoidedto list of successful response statusesfdec498Conekta: Fix remote test15379d2Priority: Updateverifymethod signature2f3db70Rapyd: Updatetypeoption topm_typeYou can trigger a rebase of this PR by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)