Newsletters: fix subscription options translations and persistent - #50909
Newsletters: fix subscription options translations and persistent#50909dlind1 wants to merge 8 commits into
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
There was a problem hiding this comment.
Pull request overview
This PR updates Jetpack’s WP.com JSON API site settings endpoint behavior for subscription_options so defaults are translated in the current user’s locale and unchanged defaults are not persisted (preventing locale “freezing” in the DB).
Changes:
- Read
subscription_optionsunder the current user locale when building the settings response. - On POST, compare incoming
subscription_optionsagainst the current value/defaults and persist only truly changed sub-keys. - Add/adjust PHPUnit coverage for the new persistence/no-op behavior and falsy
hide_free_tierhandling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| projects/plugins/jetpack/tests/php/json-api/WPCOM_JSON_API_Site_Settings_V1_4_Endpoint_Test.php | Adds POST regression tests ensuring unchanged defaults aren’t persisted and only edited sub-keys are stored. |
| projects/plugins/jetpack/json-endpoints/class.wpcom-json-api-site-settings-endpoint.php | Switches read path to user-locale retrieval and changes POST logic to ignore unchanged default values and store only diffs. |
| projects/plugins/jetpack/changelog/fix-newsletter-subscription-options-persistence | Adds a Jetpack changelog entry describing the user-facing bugfix. |
Comments suppressed due to low confidence (2)
projects/plugins/jetpack/tests/php/json-api/WPCOM_JSON_API_Site_Settings_V1_4_Endpoint_Test.php:286
- This test installs a
default_option_subscription_optionsfilter via an anonymous closure but never removes it, which can affect later tests in the same run. Clean it up in afinallyblock to avoid cross-test contamination.
public function test_post_subscription_options_ignores_unchanged_defaults() {
$defaults = $this->register_subscription_options_default();
$setting = wp_json_encode( array( 'subscription_options' => $defaults ), JSON_UNESCAPED_SLASHES );
$response = $this->make_post_request( $setting );
projects/plugins/jetpack/tests/php/json-api/WPCOM_JSON_API_Site_Settings_V1_4_Endpoint_Test.php:303
- Like the other new subscription_options tests, this leaves the
default_option_subscription_optionstest filter installed after the test completes (and it can stack if called repeatedly). Add cleanup in afinallyblock so later tests aren’t influenced by the stubbed defaults.
public function test_post_subscription_options_persists_only_changed_sub_keys() {
$defaults = $this->register_subscription_options_default();
$setting = wp_json_encode(
array( 'subscription_options' => array_merge( $defaults, array( 'welcome' => 'My own welcome' ) ) ),
JSON_UNESCAPED_SLASHES
);
$response = $this->make_post_request( $setting );
Follow-up: Atomic / self-hosted sites still get empty defaultsThis fix only covers the WPCOM JSON API endpoint ( On v4, Knock-on effect on save: the settings UI rebuilds and posts the whole bag from those empty GET values, and v4's So the "translate the defaults / don't persist unchanged defaults" goal isn't met on Atomic. Suggest extending the same read-in-user-locale + change-detection handling to the v4 endpoint (likely by dropping the empty-string schema default so the fallback fires). |
… the welcome message default
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
projects/plugins/jetpack/tests/php/json-api/WPCOM_JSON_API_Site_Settings_V1_4_Endpoint_Test.php:248
register_subscription_options_default()adds a globaldefault_option_subscription_optionsfilter but never removes it. Because filters are global in the PHP process, this can leak into later tests and changeget_option( 'subscription_options' )behavior unexpectedly. Consider storing the callback so it can be removed intear_down()(or removing it at the end of each test).
add_filter(
'default_option_subscription_options',
static function ( $default_value, $option, $passed_default ) use ( $defaults ) {
return $passed_default ? $default_value : $defaults;
},
10,
3
);
projects/plugins/jetpack/json-endpoints/class.wpcom-json-api-site-settings-endpoint.php:665
get_subscription_options_in_user_locale()repeats the defaults+stored merge block twice (including calling thedefault_option_subscription_optionsfilter andget_option()twice). This is redundant work and can introduce unexpected side effects if the filter is not idempotent; it should only run once.
$subscription_options = array_merge( $default_subscription_options, $stored_subscription_options );
if ( $switched_locale ) {
restore_previous_locale();
}
Fixes I18N-289
Proposed changes
Does this pull request change what data or activity we track or use?
Testing instructions