-
Notifications
You must be signed in to change notification settings - Fork 19
Add hotfix that prevents the cheque payment method from being auto-enabled. #33
Conversation
ryelle
left a comment
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 good, worked in testing. Just one comment about the textdomain - if using a different textdomain for hotfixes is intentional, it should be documented somewhere.
|
|
||
| function wc_api_dev_cheque_defaults( $settings ) { | ||
| $settings['enabled']['default'] = 'no'; | ||
| $settings['description']['default'] = __( 'Pay for this order by check.', 'wc-api-dev' ); |
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.
Do you mean for the textdomain to be wc-api-dev, rather than woocommerce?
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.
@ryelle So that's a good question..
For the other strings in the plugin, the strings are in WooCommerce Core already. These strings are new so they would have to be translated separately. They also won't ever be merged back to WooCommerce core like the API strings.
I think we will be breaking these types of changes out of the API plugin very shortly (see the latest thread on our P2: p90Yrv-1K).. so those would have a separate text-domain anyway, and a separate sub-project here, so they could be translated prior to launching to more users on .com. So in that case, I think it makes sense to use a different text-domain for these changes. I can document that in the README. Does that sound good?
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.
OK — I thought it would be something like that, but without a note I wasn't sure. Documenting in the readme sounds good 👍
See Automattic/wp-calypso#16658 and Automattic/wp-calypso#16663 for some history.
By default WC auto-enables the cheque payment method. We don't want this auto-enabled for Store on WP.com. Luckily, we are provided a filter where we can pass defaults, which are used if settings are not stored for the site (i.e. user hasn't configured the payment method). It also cleans up the default text.
It also adds a constant for easily disabling all these WP.com specific hotfixes.
To Test:
wc-api-devcopy.wc-api-dev.php'sinitusing thewp-adminplugin editor and added adelete_option( 'woocommerce_cheque_settings' );. I did a page load, and then removed this line.chequepayment method is not displayed.http://calypso.localhost:3000/store/settings/:siteand enable thechequepayment method. Under setup, you should see the defaults.chequepayment method is displayed.phpunitand make sure all tests pass (just to be safe).