Framework: Refactor siteUtils.getDefaultPostFormat to use a selector #13329

tyxla · 2017-04-24T07:58:07Z

This PR removes siteUtils.getDefaultPostFormat and introduces a selector instead. Since this one is used only in one place, this PR also migrates that usage and removes the obsolete siteUtils.getDefaultPostFormat definition.

To test:

Checkout this branch.
Go to http://calypso.localhost:3000/post/:site for a site that has 'standard' default post format and verify it's set properly when creating a post for that site.
Go to http://calypso.localhost:3000/post/:site for a site that has another default post format (for example 'aside' and verify it's set properly when creating a post for that site.
Go to http://calypso.localhost:3000/post/:site for a site with default post format set to '0' and verify it's set properly when creating a post for that site.
Go to http://calypso.localhost:3000/post/:site for a site with no default post format specified and verify it's set properly when creating a post for that site.
Verify the tests of the new selector pass: npm run test-client client/state/selectors/test/get-site-default-post-format.js
Verify the site selectors test still pass: npm run test-client client/state/sites/test/selectors.js

Hint: Want to quickly update your post format? Go to /wp-admin/options.php and change the default_post_format option manually.

matticbot · 2017-04-24T07:58:13Z

Test live: https://calypso.live/?branch=add/site-default-post-format-selector

aduth · 2017-04-24T14:26:15Z

I see this changes the behavior to prefer site settings value for default post format, which had previously been put in place as a workaround to the Jetpack value not always being entirely accurate. I'm not sure if this is still a problem with improved accuracy these days, but figured I'd raise it for context nonetheless:

The settings returned from the endpoint can be assumed to be more accurate than the options returned from /me/sites because the endpoint is processed on the Jetpack site itself.

11836-gh-calypso-pre-oss

gwwar

I tested this one a Jetpack and wpcom site, and it appears to work well. @tyxla maybe keep an eye out if we still need to check for site settings. It did take a bit of time for the Jetpack option to sync, but it was pretty quick, like around 30 secs or so.

tyxla · 2017-04-25T11:51:42Z

@aduth @gwwar that's a good catch! I thought using the .settings is legacy stuff, but guess it isn't.

In a9499a8 I've updated the selector to prioritize the default post format from settings. Also, site settings are queried in both occurrences where default post format is used (in Edit Post and in Writing Settings), so the settings value will actually be used.

tyxla · 2017-04-25T12:06:35Z

I'd appreciate another review of this one when you folks have a minute.

aduth · 2017-04-25T12:31:27Z

client/state/selectors/get-site-default-post-format.js

+ * @return {?String}           The default post format of that site
+ */
+export default function getSiteDefaultPostFormat( state, siteId ) {
+	const site = getRawSite( state, siteId );


Why do we need to call getRawSite at all? Wouldn't getSiteOption and getSiteSettings safely fall back to an empty value if the site data isn't available?

That would make sense only if we decide to not return the standard as the default post format if there's no other post format specified yet, but site is already known. I've expanded that in my comment below.

aduth · 2017-04-25T12:33:09Z

client/state/selectors/get-site-default-post-format.js

+		defaultPostFormat = getSiteOption( state, siteId, 'default_post_format' );
+	}
+
+	if ( ! defaultPostFormat || defaultPostFormat === '0' ) {


To the above point, two things:

Settings and site data are separately retrieved from the API, so it's not enough to check just that the raw site data is available

I'm not sure if we want to assume a "standard" format in the case that we don't know what the site or site settings are; typically we'd return null from a selector in those cases to indicate a "can't know" state.

Settings and site data are separately retrieved from the API, so it's not enough to check just that the raw site data is available

Yes, but since we're now migrating away from sites-list, the settings will not be available in the site object. And that is why we're falling back to the setting in the site object. Also, since we're using the Redux site in these cases .settings is not there anyway, so we're only improving the current solution. I'm open to any better alternatives.

I'm not sure if we want to assume a "standard" format in the case that we don't know what the site or site settings are; typically we'd return null from a selector in those cases to indicate a "can't know" state.

For me, the getSiteDefaultPostFormat selector is a more specific one, because I'd expect it to return a default value that we can actually use. In the WordPress core, empty string is saved for the post format of a post when "standard" is selected, and if you've never touched post formats in your installation, the "Standard" is the default post format when inserting a new post. So, it would make sense for me that we'd reach the "can't know" state only if we don't have information about the site yet. But if we have information about the site, it would make sense to return the standard post format as the default post format if there's no other default post format specified. In other words, standard is the "default" default post format in WordPress, not null.

Yes, but since we're now migrating away from sites-list, the settings will not be available in the site object.

Right, but there's a hypothetical situation where we have the site's settings received from GET /sites/%s/settings but not the GET /sites/%s data, in which case I'd expect the data from the prior to be returned. But with the implementation here, it'll never reach that point because the site hasn't yet been received. Or at least based on the intent of the selector, I'd expect this to be the flow of logic (even if in practice this would never occur).

But if we have information about the site, it would make sense to return the standard post

I agree with this. I think I was confused by my internal desire to remove the if ( ! site ) { condition at the top of the function, but with it, your reasoning makes sense. Generally, I just want to be sure that we don't return standard if the site data hasn't been received (since, for all we know, the default format is actually aside).

This is a great catch @aduth, and a perfectly valid scenario. I've updated the logic of the selector to reflect that case too. Here is how it will work now:

If site is not fetched AND settings are not fetched, will return null.

If site is fetched, but settings are not, use the value from the site.

If site is not fetched but settings are, use the value from settings.

If both site and settings are fetched, use the value from settings.

If the format is "" or "0", default it to 'standard'.

Otherwise, it'll return the actual fetched format.

I've also expanded the unit tests to cover all of these situations. Feel free to have another look @aduth.

Here is how it will work now:

This makes perfect sense, thanks for illustrating 👍

gwwar · 2017-04-27T19:09:06Z

@aduth did we need to add another check to see if all data is available? Otherwise I think this is good to 🚢

aduth · 2017-04-28T13:27:18Z

client/state/site-settings/selectors.js

@@ -44,7 +44,7 @@ export function getSiteSettingsSaveRequestStatus( state, siteId ) {
 * @return {Object}        Site settings
 */
 export function getSiteSettings( state, siteId ) {
-	return get( state.siteSettings.items, [ siteId ], null );
+	return get( state, [ 'siteSettings', 'items', siteId ], null );


Was this changed due to tests? Based on reducer shape of state we should always be able to assume that items exists. I think we'll want to update tests instead.

Good call, will update them instead.

Updated in aed545c and removed the commit that altered getSiteSettings. Also, updated some more tests that used the computed site to contain siteSettings.items in the state in 0e5c017.

aduth · 2017-04-28T13:27:39Z

client/state/selectors/get-site-default-post-format.js

+		defaultPostFormat = getSiteOption( state, siteId, 'default_post_format' );
+	}
+
+	if ( ! defaultPostFormat || defaultPostFormat === '0' ) {


Here is how it will work now:

This makes perfect sense, thanks for illustrating 👍

aduth · 2017-04-28T13:29:45Z

did we need to add another check to see if all data is available?

@gwwar Can you clarify what you mean by this?

aduth · 2017-04-28T13:33:17Z

This does work well in my testing 👍

gwwar · 2017-04-28T17:31:18Z

@gwwar Can you clarify what you mean by this?

I might have misread earlier, but it sounded like we might have needed to wait for 2 data sources to populate before knowing what the correct default is.

aduth · 2017-04-28T17:45:30Z

I might have misread earlier, but it sounded like we might have needed to wait for 2 data sources to populate before knowing what the correct default is.

Ah. There is two sources, yes, but we should be able to use one or the other, with a preference toward the site settings. Or at least, that's how the previous behavior had been implemented.

…Format

tyxla added [Feature] Post/Page Editor The editor for editing posts and pages. Framework State [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Janitorial labels Apr 24, 2017

tyxla self-assigned this Apr 24, 2017

tyxla requested review from gwwar and aduth April 24, 2017 07:58

matticbot added the [Size] L Large sized issue label Apr 24, 2017

gwwar approved these changes Apr 24, 2017

View reviewed changes

matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Apr 25, 2017

matticbot added [Size] L Large sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels Apr 25, 2017

aduth reviewed Apr 25, 2017

View reviewed changes

matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Apr 28, 2017

tyxla mentioned this pull request Apr 28, 2017

Exploration: Top-down removal of SitesList, starting from Layout #13094

Merged

10 tasks

aduth reviewed Apr 28, 2017

View reviewed changes

tyxla force-pushed the add/site-default-post-format-selector branch from 19b6185 to 81734d1 Compare May 1, 2017 10:57

tyxla added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 2, 2017

tyxla added 10 commits May 2, 2017 14:41

State: Introduce getSiteDefaultPostFormat selector

58eb215

State: Use default post format selector in computeSiteOptions

bd61c8c

Editor: Use selector for default post format

a60f3c3

Framework: Remove siteUtils.getDefaultPostFormat

d99f80f

State: Prioritize settings when retrieving default post format

a94ed3f

State: Cleanup getSiteDefaultPostFormat tests

b7ccdd3

State: Handle fetched settings and missing site in getSiteDefaultPost…

d8c60bb

…Format

State: Update getSiteDefaultPostFormat JSdoc

ef722d3

State: Add siteSettings to getSiteDefaultPostFormat tests state

9a42be0

State: Fix some more failing tests

7c99cf2

tyxla force-pushed the add/site-default-post-format-selector branch from 0e5c017 to 7c99cf2 Compare May 2, 2017 11:42

tyxla merged commit 1333fe9 into master May 2, 2017

tyxla deleted the add/site-default-post-format-selector branch May 2, 2017 11:50

tyxla removed the [Status] Ready to Merge label May 2, 2017

This was referenced May 30, 2021

[Snyk] Fix for 1 vulnerabilities ajesse11x/wp-calypso#52

Open

[Snyk] Security upgrade @wordpress/editor from 9.0.11 to 9.1.0 awaisab172/wp-calypso#79

Open

This was referenced Oct 24, 2021

[Snyk] Security upgrade @wordpress/editor from 9.0.11 to 9.1.0 awaisab172/wp-calypso#101

Open

[Snyk] Fix for 1 vulnerabilities ajesse11x/wp-calypso#65

Open

snyk-bot mentioned this pull request Nov 4, 2021

[Snyk] Fix for 1 vulnerabilities ajesse11x/wp-calypso#66

Open

ajesse11x mentioned this pull request Oct 20, 2023

[Snyk] Fix for 2 vulnerabilities ajesse11x/wp-calypso#93

Open

awaisab172 mentioned this pull request Oct 20, 2023

[Snyk] Security upgrade @wordpress/editor from 9.0.11 to 9.1.0 awaisab172/wp-calypso#162

Open

ajesse11x mentioned this pull request Nov 16, 2023

[Snyk] Fix for 2 vulnerabilities ajesse11x/wp-calypso#95

Open

awaisab172 mentioned this pull request Nov 17, 2023

[Snyk] Security upgrade @wordpress/editor from 9.0.11 to 9.1.0 awaisab172/wp-calypso#167

Open

awaisab172 mentioned this pull request Mar 27, 2024

[Snyk] Security upgrade @wordpress/editor from 9.0.11 to 9.1.0 awaisab172/wp-calypso#188

Open

ajesse11x mentioned this pull request Mar 28, 2024

[Snyk] Fix for 2 vulnerabilities ajesse11x/wp-calypso#102

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Refactor siteUtils.getDefaultPostFormat to use a selector #13329

Framework: Refactor siteUtils.getDefaultPostFormat to use a selector #13329

tyxla commented Apr 24, 2017 •

edited

Loading

matticbot commented Apr 24, 2017

aduth commented Apr 24, 2017

gwwar left a comment

tyxla commented Apr 25, 2017

tyxla commented Apr 25, 2017

aduth Apr 25, 2017

tyxla Apr 27, 2017 •

edited

Loading

aduth Apr 25, 2017

tyxla Apr 27, 2017

aduth Apr 27, 2017

tyxla Apr 28, 2017

aduth Apr 28, 2017

gwwar commented Apr 27, 2017

aduth Apr 28, 2017

tyxla May 1, 2017

tyxla May 1, 2017 •

edited

Loading

aduth Apr 28, 2017

aduth commented Apr 28, 2017

aduth commented Apr 28, 2017

gwwar commented Apr 28, 2017

aduth commented Apr 28, 2017

Framework: Refactor siteUtils.getDefaultPostFormat to use a selector #13329

Framework: Refactor siteUtils.getDefaultPostFormat to use a selector #13329

Conversation

tyxla commented Apr 24, 2017 • edited Loading

matticbot commented Apr 24, 2017

aduth commented Apr 24, 2017

gwwar left a comment

Choose a reason for hiding this comment

tyxla commented Apr 25, 2017

tyxla commented Apr 25, 2017

Choose a reason for hiding this comment

tyxla Apr 27, 2017 • edited Loading

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

gwwar commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Apr 28, 2017

aduth commented Apr 28, 2017

gwwar commented Apr 28, 2017

aduth commented Apr 28, 2017

tyxla commented Apr 24, 2017 •

edited

Loading

tyxla Apr 27, 2017 •

edited

Loading

tyxla May 1, 2017 •

edited

Loading