Social: Refactor feature option storing #34113
Conversation
github-actions
bot
added
[JS Package] Publicize Components
[Package] Publicize
[Plugin] Jetpack
|
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 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Migration plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
| return apiFetch( { | ||
| path: '/jetpack/v4/social-image-generator/settings', | ||
| path: '/wp/v2/settings', | ||
| method: 'POST', | ||
| data: action.settings, | ||
| data: { | ||
| jetpack_social_image_generator_settings: action.settings, | ||
| }, | ||
| } ); | ||
| }, | ||
| [ FETCH_AUTO_CONVERSION_SETTINGS ]: function () { | ||
| return apiFetch( { path: '/jetpack/v4/auto-conversion/settings' } ); | ||
| return apiFetch( { | ||
| path: '/wp/v2/settings?_fields=jetpack_social_autoconvert_images', | ||
| } ); | ||
| }, | ||
| [ UPDATE_AUTO_CONVERSION_SETTINGS ]: function ( action ) { | ||
| return apiFetch( { | ||
| path: '/jetpack/v4/auto-conversion/settings', | ||
| path: '/wp/v2/settings', | ||
| method: 'POST', | ||
| data: action.settings, |
There was a problem hiding this comment.
Why do we need multiple API calls? Can we make a single API call to get all the settings at once?
There was a problem hiding this comment.
We can add the settings fields separated by comma
/wp/v2/settings?_fields=jetpack_social_image_generator_settings,jetpack_social_autoconvert_images
There was a problem hiding this comment.
Yes, to fetch the settings we can just have one call.
There was a problem hiding this comment.
Yeah, I'll do that.
* Remove unneeded files * changelog
* Remove previous refresh functionality * Add wrapper to refresh settings in class component on Jetpack Settings * Refresh on social admin page on module toggle change * Remove useEffect * Run useEffect only once on module update * Show toggles instantly on change + only refresh ones on page lifetime * Update ref logic
github-actions
bot
added
the
Admin Page
github-actions
bot
added
[Plugin] Protect
gmjuhasz
force-pushed
the
refactor/social-register-feature-settings
branch
from
def6ef2 to
b7c2568
Compare
gmjuhasz
removed
[Package] Sync
[Plugin] Backup
github-actions
bot
added
[Plugin] Backup
| updatedSettings.jetpack_social_image_generator_settings | ||
| ); | ||
| return true; | ||
| } catch ( e ) { | ||
| return false; | ||
| } finally { | ||
| yield setUpdatingAutoConversionSettingsDone(); | ||
| yield setUpdatingSocialImageGeneratorSettingsDone(); | ||
| } |
There was a problem hiding this comment.
TIL that finally block is executed before the return statement inside try/catch.
| ), | ||
| ), | ||
| ), | ||
| 'type' => 'boolean', |
There was a problem hiding this comment.
I think something might be wrong here, this type should be object. Do you think that's how it ended up being the empty string? I think maybe something has been cast as a boolean and it's evaluated to false.
This change refactors how we store our options for SIG and auto conversion. We moved to use the register_setting function, this way we need no Rest controllers.
It's easier to review this following the commits.
FEATURE BRANCH OTHER PRs:
Proposed changes:
Other information:
Jetpack product discussion
https://github.com/orgs/Automattic/projects/733/views/2?pane=issue&itemId=44049666
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
This probably needs a lot of testing to see that everything is working. You can run the tests on the backend with
composer run test-phpfrom thepublicizefolder.To test the flow you can do the following:
Have both Jetpack and Jetpack Social enabled. Open up The Jetpack Settings, and go to the sharing section. On another tab open up the Social admin page.
Have the conversion on, then create a new post. The default state of SIG in the editor should match the state of the toggle in the admin page.
Clear the dismissed notices with
jetpack docker wp option delete jetpack_social_dismissed_noticesOpen up a new post with the conversion on, then upload an invalid image that can be converted. You should see the notice that the image will be converted.