active_merchant
active_merchant copied to clipboard
Pin: Update 3DS to include new parameters
Pin: Update 3DS secure to include new parameters
We have found we cannot use 3DS secure with Pin as the parameters do not match those documented by Pin. This closes #4719.
Test Summary Local: 5476 tests, 77236 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Unit: 44 tests, 145 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed Remote: 21 tests, 65 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
Nice work @hudakh, changes look good to me and Unit/Remote tests pass :+1:
Still needs workflow approval and review from an AM maintainer.
Nice work @hudakh, changes look good to me and Unit/Remote tests pass +1
Still needs workflow approval and review from an AM maintainer.
Thanks @AMHOL. I've cleaned up the commits and PR description in preparation for further review. Not sure if I need to assign to anyone.
I haven't done in the past, normally someone just comes along and picks it up.
@madpilot can this be marked as ready for review please? I am not sure how to proceed further to get this merged.
Is there not somewhere else to do this? My last PR on this repo was 11 years ago. I'm not the best person to be approving anything.
Is there not somewhere else to do this? My last PR on this repo was 11 years ago. I'm not the best person to be approving anything.
No worries. Your name was the only name listed for Pin under the contributors list.. I thought perhaps that would give you some super powers I do not have.
@jessiagee Can you please assist here? I see you were the reviewer of the last Pin-related merge request. I am not sure of next steps, but I feel this won't progress if I don't prod someone.
following this too. trying to pin @rachelkirk as well who might have looked at previous Pin-related PRs before.
Hey Rachel, would you be able to help or direct to someone who could help with progressing this PR?
@hudakh if this is blocking you, you can pull the gem from your fork using the following in your Gemfile:
gem 'activemerchant', git: '[email protected]:hudakh/active_merchant.git', branch: '1-update-pin-gateway'
# or
gem 'activemerchant', git: '[email protected]:hudakh/active_merchant.git', ref: 'c716f13a9c18db7bb4550f175f42f981555af079'
Otherwise you'll have to wait for the next activemerchant release after this PR is merged.
Hello! Could you please elaborate on how you will need the change in AM (i.e. do you need a new release or just a merge to master
)? Additionally can you provide more details on why you are making this change?
@aenand
@hudakh would need a merge/release in order to use this feature on the mainline.
The change updates the Pin gateway in order to add support for passing three_d_secure
params to the charges
endpoint as documented here, whilst retaining support for the old style 3D Secure passthrough options in order to maintain support for existing integrations.
There are some rubocop linter failures. Please run bundle exec rubocop -a
to autocorrect them!
There are some rubocop linter failures. Please run
bundle exec rubocop -a
to autocorrect them!
I'm not familiar with running rubocop in different ruby versions, so I corrected the issues manually.
To provide a cleaner slate for the maintenance of the library, this PR/Issue is being labeled stale after 60 days without activity. It will be closed in 14 days unless you comment with an update regarding its applicability to the current build. Thank you!
Pending review.
Sorry I lost this 🤦🏽 Looks good to me. Please squash the commits and add a changelog!
@hudakh are you available to do this?
Hey, sorry I am on leave until February and don't have access to the repo until then. Happy for you to take over.
On Thu, 30 Nov 2023, 9:16 pm Andy Holland, @.***> wrote:
Sorry I lost this 🤦🏽 Looks good to me. Please squash the commits and add a changelog!
@hudakh https://github.com/hudakh are you available to do this?
— Reply to this email directly, view it on GitHub https://github.com/activemerchant/active_merchant/pull/4720#issuecomment-1833514342, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEM3DKB7MHRI3ZCMMSEMQATYHBPYNAVCNFSM6AAAAAAVLSVQCKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZTGUYTIMZUGI . You are receiving this because you were mentioned.Message ID: @.***>
Hey, sorry I am on leave until February and don't have access to the repo until then. Happy for you to take over. … On Thu, 30 Nov 2023, 9:16 pm Andy Holland, @.> wrote: Sorry I lost this 🤦🏽 Looks good to me. Please squash the commits and add a changelog! @hudakh https://github.com/hudakh are you available to do this? — Reply to this email directly, view it on GitHub <#4720 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEM3DKB7MHRI3ZCMMSEMQATYHBPYNAVCNFSM6AAAAAAVLSVQCKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZTGUYTIMZUGI . You are receiving this because you were mentioned.Message ID: @.>
Nice, happy to do that @hudakh if you invite me as a collaborator on https://github.com/hudakh/active_merchant/tree/1-update-pin-gateway, otherwise I will need to open a separate PR for it.
Done @aenand
Awesome, thank you so much!
On Thu, 30 Nov 2023, 11:07 pm Andy Holland, @.***> wrote:
Done @aenand https://github.com/aenand
— Reply to this email directly, view it on GitHub https://github.com/activemerchant/active_merchant/pull/4720#issuecomment-1833698562, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEM3DKHZ66HIYFLTZS3MFRLYHB4XDAVCNFSM6AAAAAAVLSVQCKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZTGY4TQNJWGI . You are receiving this because you were mentioned.Message ID: @.***>
To provide a cleaner slate for the maintenance of the library, this PR/Issue is being labeled stale after 60 days without activity. It will be closed in 14 days unless you comment with an update regarding its applicability to the current build. Thank you!
Rebased
@aenand any update on this?
@AMHOL i'm so sorry i missed this. it's saying there are merge conflicts, can you check those?
@aenand I am not seeing any merge conflict issues?
@aenand is anything further needed to get this merged?