Social: Refresh store on Publicize module toggle via 1 call #34142

gmjuhasz · 2023-11-15T16:20:15Z

This change updates how we refresh our options on the Publicize module change, and will use the common api fetch, so we can decrease the number of API calls.

Proposed changes:

Removed the separated refresh from the toggle components 07cf749
Added a common refresh into the admin page and to Jetpack Dashboard. 61facfa and 1902f5d

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

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:

Turn SIG and auto conversion on.
Turn off the main Publicize module.
Refresh the page
Turn on the module, the child toggles should reflect their actual state
There should be only 1 call to the API

github-actions · 2023-11-15T16:23:42Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
🔴 Add changelog entries to affected projects

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.

🔴 Action required: Please add missing changelog entries for the following projects: projects/js-packages/publicize-components, projects/plugins/jetpack, projects/plugins/social

Use the Jetpack CLI tool to generate changelog entries by running the following command: jetpack changelog add.
Guidelines: /docs/writing-a-good-changelog-entry.md

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.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

WordPress.com Simple releases happen daily.
WoA releases happen weekly.
Releases to self-hosted sites happen monthly. The next release is scheduled for December 5, 2023 (scheduled code freeze on November 28, 2023).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Social plugin:

Next scheduled release: December 5, 2023.
Scheduled code freeze: November 27, 2023.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

github-actions · 2023-11-15T16:23:57Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the update/social-refresh-options-together branch.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack update/social-refresh-options-together

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

pablinos

Looks good. I need to give it a proper test, but I've left a few comments

pablinos · 2023-11-15T19:31:12Z

projects/plugins/social/src/js/components/admin-page/index.jsx

+	useEffect( () => {
+		if (
+			isModuleEnabled &&
+			isUpdatingJetpackSettings &&


Should this be:

Suggested change

isUpdatingJetpackSettings &&

! isUpdatingJetpackSettings &&

I think it makes sense to update the store while the main module is turning on, this way it will be refreshed already when the toggles appear, reducing the page jank

pablinos · 2023-11-15T19:34:53Z

projects/plugins/social/src/js/components/admin-page/index.jsx

+		) {
+			refreshJetpackSocialSettings();
+		}
+	} );


This hasn't got a dependency array, which will mean that it runs on every render. I think putting it a useEffect means that it runs after the render rather than before. Is that what's intended?

This useEffect may also be unnecessary.

#34142 (comment)

pablinos · 2023-11-15T19:36:41Z

projects/plugins/social/src/js/components/admin-page/index.jsx

@@ -90,10 +102,10 @@ const Admin = () => {
 						<InstagramNotice onUpgrade={ onUpgradeToggle } />
 						<SocialModuleToggle />
 						{ ! isUpdatingJetpackSettings && isModuleEnabled && isAutoConversionAvailable && (
-							<AutoConversionToggle shouldRefresh />
+							<AutoConversionToggle />


Should we still render it but disable it if the state is updating? It might mean that there's less page jank. I don't know if there's a busy/loading state that we can use with the toggles, but that would be great.

There is no page jank, the store has an isUpdating flag for each which disables the toggle while updating. Is that what you ask about?

Not quite, I was thinking that there's an ! isUpdatingJetpackSettings check, so if isUpdatingJetpackSettings we won't rending the toggle at all.

Oh I see what you mean. It's working this way on Jetpack Settings that's what I mimicked, but I can make that be there even with Publicize off if you think that's better.

pablinos · 2023-11-15T22:05:08Z

projects/js-packages/publicize-components/src/components/refresh-jetpack-settings/index.js

+ * @param {object} props.children - The children to render.
+ * @returns { object } The refreshJetpackSocialSettings function.
+ */
+export default function RefreshJetpackSocialSettingsWrapper( { shouldRefresh, children } ) {


I wonder if this file should be in a folder named refresh-social-settings

pablinos · 2023-11-15T22:15:31Z

projects/plugins/jetpack/_inc/client/sharing/publicize.jsx

-							{ shouldShowChildElements && hasSocialImageGenerator && (
-								<SocialImageGeneratorSection />
-							) }
+							<RefreshJetpackSocialSettingsWrapper


Are we using this component here so we can encapsulate the useEffect because this is a class component?

That's probably fine, but it might be worth checking that we don't want to do something like handle it with a lifecycle method in here.

useEffect is not even needed for this and thus a lifecycle method may be a good place for it.

Yeah, and also useDispatch. I added the useEffect to fix an issue earlier where the inner component was updated while the parent was changing, but it seems that's already fixed and it might not be needed. I think we still need the component to be able to dispatch the store function, or we need to wrap publicize.jsx into another HOC component

projects/js-packages/publicize-components/src/components/refresh-jetpack-settings/index.js

pablinos

Looks good. Couple of thoughts, as always! Sorry...

projects/plugins/social/src/js/components/admin-page/index.jsx

manzoorwanijk

It looks good now.

* Add the new settings object + tests * Update initial states + previous usages * Update UI + store functions * changelog * Fix versions * Add control for fetching both options together * Social: Remove deprecated feature files (#34114) * Remove unneeded files * changelog * Social: Refresh store on Publicize module toggle via 1 call (#34142) * 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 * Rename structure * Fixup versions * Fix template issue, and make sure it's migrated * fix tests * Test fix * Fix auto-conversion migration logic + add test * Add auto-conversion sync option * changelog * Update versions * Fix versions

gmjuhasz added 3 commits November 15, 2023 17:15

Remove previous refresh functionality

07cf749

Add wrapper to refresh settings in class component on Jetpack Settings

61facfa

Refresh on social admin page on module toggle change

1902f5d

gmjuhasz added the [Status] Needs Team Review label Nov 15, 2023

gmjuhasz requested a review from a team November 15, 2023 16:20

gmjuhasz self-assigned this Nov 15, 2023

gmjuhasz mentioned this pull request Nov 15, 2023

Social: Refactor feature option storing #34113

Merged

3 tasks

github-actions bot added [JS Package] Publicize Components [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Social Issues about the Jetpack Social plugin Admin Page React-powered dashboard under the Jetpack menu RNA labels Nov 15, 2023

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 15, 2023

pablinos reviewed Nov 15, 2023

View reviewed changes

manzoorwanijk reviewed Nov 16, 2023

View reviewed changes

projects/js-packages/publicize-components/src/components/refresh-jetpack-settings/index.js Outdated Show resolved Hide resolved

Remove useEffect

b441f37

gmjuhasz requested review from pablinos and manzoorwanijk November 16, 2023 09:23

gmjuhasz added 2 commits November 16, 2023 12:16

Run useEffect only once on module update

3e98588

Show toggles instantly on change + only refresh ones on page lifetime

fa0dac4

pablinos reviewed Nov 17, 2023

View reviewed changes

projects/plugins/social/src/js/components/admin-page/index.jsx Outdated Show resolved Hide resolved

projects/plugins/social/src/js/components/admin-page/index.jsx Outdated Show resolved Hide resolved

projects/plugins/social/src/js/components/admin-page/index.jsx Show resolved Hide resolved

Update ref logic

4b5f559

gmjuhasz requested a review from pablinos November 20, 2023 09:47

manzoorwanijk approved these changes Nov 20, 2023

View reviewed changes

gmjuhasz merged commit 2b51295 into refactor/social-register-feature-settings Nov 20, 2023
51 of 52 checks passed

gmjuhasz deleted the update/social-refresh-options-together branch November 20, 2023 10:59

github-actions bot removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs Team Review labels Nov 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Social: Refresh store on Publicize module toggle via 1 call #34142

Social: Refresh store on Publicize module toggle via 1 call #34142

gmjuhasz commented Nov 15, 2023 •

edited

Loading

github-actions bot commented Nov 15, 2023

github-actions bot commented Nov 15, 2023 •

edited

Loading

pablinos left a comment

pablinos Nov 15, 2023

gmjuhasz Nov 16, 2023

pablinos Nov 15, 2023

manzoorwanijk Nov 16, 2023

gmjuhasz Nov 16, 2023

pablinos Nov 15, 2023

gmjuhasz Nov 16, 2023

pablinos Nov 16, 2023

gmjuhasz Nov 16, 2023

pablinos Nov 15, 2023

pablinos Nov 15, 2023

manzoorwanijk Nov 16, 2023

gmjuhasz Nov 16, 2023 •

edited

Loading

pablinos left a comment

manzoorwanijk left a comment

Social: Refresh store on Publicize module toggle via 1 call #34142

Social: Refresh store on Publicize module toggle via 1 call #34142

Conversation

gmjuhasz commented Nov 15, 2023 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Nov 15, 2023

github-actions bot commented Nov 15, 2023 • edited Loading

pablinos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmjuhasz Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

pablinos left a comment

Choose a reason for hiding this comment

manzoorwanijk left a comment

Choose a reason for hiding this comment

gmjuhasz commented Nov 15, 2023 •

edited

Loading

github-actions bot commented Nov 15, 2023 •

edited

Loading

gmjuhasz Nov 16, 2023 •

edited

Loading