-
Notifications
You must be signed in to change notification settings - Fork 207
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
Removing Giropay due deprecation #3229
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good and it tests well, just the one test mentions sofort
(which was deprecated a while back) instead of the intended alipay
.
And also, the usual conflicts with the readme and changelog.
None of them are critical, so I'm approving this... feel free to merge this after fixing those
client/settings/general-settings-section/__tests__/general-settings-section.test.js
Outdated
Show resolved
Hide resolved
client/settings/general-settings-section/__tests__/general-settings-section.test.js
Outdated
Show resolved
Hide resolved
client/settings/general-settings-section/__tests__/general-settings-section.test.js
Outdated
Show resolved
Hide resolved
client/settings/general-settings-section/__tests__/general-settings-section.test.js
Outdated
Show resolved
Hide resolved
client/settings/general-settings-section/__tests__/general-settings-section.test.js
Outdated
Show resolved
Hide resolved
client/settings/general-settings-section/__tests__/general-settings-section.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes here @wjrosa !
Things work as specified in the PR description. Giropay isn't displayed:
✅ On the shortcode checkout page
✅ On the block checkout page
✅ On the Stripe settings page
Both when the Legacy checkout experience is enabled and disabled.
I have a few non-blocking questions.
❓ Why not remove the Giropay-related settings from the database on upgrade, instead of filtering it out later in several places? I left a comment below related to this.
❓ Besides the Giropay gateway files, we have other references to Giropay in the code. Are these intentionally kept?
❓ Why not remove the Giropay gateways?
Just questions. There may be scenarios I'm missing, and we could address anything that's needed separately as this behaves as expected. Happy to see this merged.
client/settings/general-settings-section/__tests__/general-settings-section.test.js
Outdated
Show resolved
Hide resolved
client/settings/general-settings-section/__tests__/general-settings-section.test.js
Outdated
Show resolved
Hide resolved
tests/phpunit/admin/test-class-wc-rest-stripe-settings-controller.php
Outdated
Show resolved
Hide resolved
@@ -25,6 +25,7 @@ const api = new WCStripeAPI( | |||
const upeMethods = getPaymentMethodsConstants(); | |||
Object.entries( getBlocksConfiguration()?.paymentMethodsConfig ) | |||
.filter( ( [ upeName ] ) => upeName !== 'link' ) | |||
.filter( ( [ upeName ] ) => upeName !== 'giropay' ) // Skip giropay as it was deprecated by Jun, 30th 2024. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're filtering out giropay in several places right before we display it. How about removing it earlier, closer to the source we use everywhere else?
Like when retrieving the available payment methods that are then passed to JS...
Or earlier when retrieving the enabled payment methods.
Or removing its options from the database directly on upgrade.
Not requesting changes for this, just curious about the approach. Maybe I'm missing a scenario that's better handled this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just felt safe removing it that way because we need to keep the method for refunds. But I think your suggestions make sense. I will merge this as is and open another PR to improve it for the next release if that sounds ok!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just felt safe removing it that way because we need to keep the method for refunds.
Keeping it if it's still in use makes sense. But are refunds working for Giropay with these changes?
Testing over here, I only see the Manual refund option in a Giropay order when checking out these changes.
If I go back to the 8.4.0 tag, the option to refund via Giropay does show up
Glad to learn if there's a use for it that I'm missing. And glad to keep the Gateway class in that case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks for checking! I will work on this separately on the next days
…tings-section.test.js Co-authored-by: Diego Curbelo <[email protected]>
…tings-section.test.js Co-authored-by: Diego Curbelo <[email protected]>
…tings-section.test.js Co-authored-by: Diego Curbelo <[email protected]>
…tings-section.test.js Co-authored-by: Diego Curbelo <[email protected]>
…tings-section.test.js Co-authored-by: Diego Curbelo <[email protected]>
…tings-section.test.js Co-authored-by: Diego Curbelo <[email protected]>
Fixes #3223
Changes proposed in this Pull Request:
Giropay is going away as of Jun 30th, 2024. This PR removes it as a payment option in settings and checkout (block and shortcode versions) for legacy and new checkout experiences.
Testing instructions
tweak/remove-giropay
)npm install
,npm run build:webpack
andnpm run up
wp-admin/admin.php?page=wc-settings&tab=checkout§ion=stripe&panel=methods
)changelog.txt
andreadme.txt
(or does not apply)Post merge