Fix/top posts widget by leogermani · Pull Request #15109 · Automattic/jetpack

leogermani · 2020-03-24T22:15:16Z

Fixes #15104,
See #2369
Addresses 4794-jpop-issues, 4988-jpop-issues

Changes proposed in this Pull Request:

This PR improves the way the Top Posts & Pages Widget fetches posts to be displayed.

How it worked before this PR:

Widget makes a request to the wp.com API to fetch the top posts (of all post types)
Since it makes a request with summarize=1, it receives up to 500 posts in the response
It filters out only the number of posts the widget is configured to be displayed
From this few posts, it filters out only posts of the post types configured to be displayed
If it results in zero posts, it falls back fetching only one post or page, no matter what the post types configured in the widget are

The problem here is that if there were 10 posts in the top 10 positions, and you queried for pages, you would get no results.

This PR proposes two changes:

It filters out the posts by post type at the same time it's iterating over all the posts returned by the API, till it reaches the number of posts configured in the widget.
If no posts are found, it respects the post type and number of posts configurations

Limitations: This PR does not completely solve the problem. There is no way to query the wp.com API for post types filtering for a specific post type. So the same problem still persists, the only difference is that it now looks in all 500 retrieved posts, and not only on the first 10.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

It fixes an existing feature

Testing instructions:

It's very difficult to test the edge cases that this PR fixes, but I'll give some basic testing instructions and describe the edge case afterward.

Run these tests both on a wp.com site and on a JP site.

On an existing site with some stats.
2. in Jetpack settings, activate Make extra widgets available for use on your site including subscription forms and Twitter streams
3. Go to Appearance > Widgets and add two "Top Posts & Pages" Widget
4. Configure one of the widgets to display only posts, and the other one to display only pages
5. Visit the site both logged in and logged out and check if the widgets are working as expected

On a brand new site

Follow the same instructions, 1 to 4
Visit the site, logged in, and see the message "There are no posts to display"
Visit the site, logged out, and see that the posts displayed in both widget respect the configured post type and count.

The Edge case

The Edge case is a site that has many posts of a certain post type and, when you add a widget to display posts of another post type, this widget would not find any post.

Considering a site that has many posts of the post type post as its top rankings posts, and few pages with few visits

Follow the same instructions 1 to 4
Check the site front end, bot logged in and logged out. Check that the widget that should display pages finds the most viewed pages.

Proposed changelog entry for your changes:

Fixes Top Posts & Pages Widget handling with different post types

Consider all the posts retrieved from the wp.com API when filtering by post type

When no posts are found in wp.com API Fallback to local posts but respecting post type and count

matticbot · 2020-03-24T22:15:22Z

Caution: This PR has changes that must be merged to WordPress.com
Hello leogermani! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D40778-code before merging this PR. Thank you!

jetpackbot · 2020-03-24T22:17:59Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 97d3618

sergeymitr · 2020-03-27T15:36:58Z

Hi @leogermani.
For some reason the "Top Posts & Pages" widget doesn't display any posts or pages when I'm logged in as an admin:

Everything works correctly when I'm viewing the website as a guest though.
Let me know if you need help debugging it.

kraftbj

Very minor things. (and the updated commit may retrigger Travis)

kraftbj · 2020-03-27T16:52:24Z

modules/widgets/top-posts.php

+	 * @param array $types The post types that should be returned.
+	 * @return array
+	 */
+	public function get_posts( $post_ids, $count, $types ) {


Should we set $types = array( 'post', 'page' ) to make it an optional parameter?

we could. I didn't because this variable is treated here:

jetpack/modules/widgets/top-posts.php

Line 244 in 218e6fb

$types = isset( $instance['types'] ) ? (array) $instance['types'] : array( 'post', 'page' );

But no harm doing that

I'm reconsidering this.

This method is not called directly by the main widget method, it is passed through get_by_likes, get_by_views and get_fallback_posts. So this would require a chain of optional and useless arguments...

Both $count and $types are present in the widgets $instance, so a nice refactor would be to use it instead. Not sure if we should do it now... Also, these methods could/should be private (or live somewhere else in jetpack codebase)

I'm always afraid with backward compatibility when I consider refactoring things, but this is a widget and the code, I beleive, is self contained and shouldn't be called from outside...

What do you think? quick refactor?

I'm okay covering that in a later PR. Could you either open an issue (if you don't have bandwidth) or open a new PR for it?

kraftbj · 2020-03-27T16:53:41Z

modules/widgets/top-posts.php

-	function get_posts( $post_ids, $count ) {
+	/**
+	 * Get posts from an array of IDs
+	 *


Could we add a @since 8.4.0 Added $types?

Done in 97d3618

leogermani · 2020-03-27T17:45:16Z

Hi @leogermani.
For some reason the "Top Posts & Pages" widget doesn't display any posts or pages when I'm logged in as an admin:

That's expected with a brand new site.

When you have an anonymous visitor, it falls back and make a simple query for posts (the second fix on this PR). Before this PR. it would display only one post, no matter how many you configured to be displayed in the widget, and also ignore the post type.

matticbot · 2020-03-27T18:59:55Z

leogermani, Your synced wpcom patch D40778-code has been updated.

kraftbj · 2020-03-27T19:32:52Z

r204992-wpcom

kraftbj · 2020-03-27T20:53:36Z

D41022-code // r204996-wpcom for the clean-up of the defaults.

leogermani · 2020-03-27T21:25:24Z

modules/widgets/top-posts.php

 	 * Get posts from an array of IDs
 	 *
-	 * @since 8.4.0 Added $count and $types parameters
+	 * @since 8.4.0 Added $types parameters


ah yes, the parameter existed, but it was ignored

* Initial changelog entry * Changelog: add #14904 * Changelog: add #14910 * Changelog: add #14913 * Changelog: add #14916 * Changelog: add #14922 * Changelog: add #14924 * Changelog: add #14925 * Changelog: add #14928 * Changelog: add #14840 * Changelog: add #14841 * Changelog: add #14842 * Changelog: add #14826 * Changelog: add #14835 * Changelog: add #14859 * Changelog: add #14884 * Changelog: add #14888 * Changelog: add #14817 * Changelog: add #14814 * Changelog: add #14819 * Changelog;: add #14797 * Changelog: add #14798 * Changelog: add #14802 * Changelog: add #13676 * Changelog: add #13744 * Changelog: add #13777 * Changelog: add #14446 * Changelog: add #14739 * Changelog: add #14770 * Changelog: add #14784 * Changelog: add #14897 * Changelog: add #14898 * Changelog: add #14968 * Changelog: add #14985 * Changelog: add #15044 * Changelog: add #15052 * Update to remove Podcast since it remains in Beta * Changelog: add #14803 * Changelog: add #15028 * Changelog: add #15065 * Changelog:add #14886 * Changelog: add #15118 * Changelog: add #14990 * Changelog: add #14528 * Changelog: add #15120 * Changelog: add #15126 * Changelog: add #15049 * Chanegelog: add #14852 * Changelog: add #15090 * Changelog: add #15138 * Changelog: add #15124 * Changelog:add #15055 * Changelog: add #15017 * Changelog: add #15109 * Changelog: add #15145 * Changelog:add #15096 * Changelog:add #15153 * Changelog: add #15133 * Changelog: add #14960 * Changelog: add #15127 * Changelog: add #15056 * Copy current changelog to changelog archive. * Clarify changelog description

$types was added in 8.4, but we did not add a default value when it's not provided. See #15109 for more details.

…am. (#15395) $types was added in 8.4, but we did not add a default value when it's not provided. See #15109 for more details.

leogermani added 2 commits March 24, 2020 18:27

Top Posts - fix filter by post type #15104

c5d402a

Consider all the posts retrieved from the wp.com API when filtering by post type

Top posts wdgt: fallback respect settings #15104

218e6fb

When no posts are found in wp.com API Fallback to local posts but respecting post type and count

leogermani added [Feature] Extra Sidebar Widgets [Status] Needs Review This PR is ready for review. labels Mar 24, 2020

leogermani requested a review from a team March 24, 2020 22:15

matticbot added the Touches WP.com Files label Mar 24, 2020

kraftbj assigned leogermani Mar 24, 2020

jeherve added the Bug When a feature is broken and / or not performing as intended label Mar 25, 2020

jeherve added this to the 8.4 milestone Mar 25, 2020

kraftbj requested changes Mar 27, 2020

View reviewed changes

add changelog to methods (refs #15109)

97d3618

kraftbj previously approved these changes Mar 27, 2020

View reviewed changes

kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 27, 2020

Adds default for new third parameter

ebe06ee

kraftbj dismissed their stale review via ebe06ee March 27, 2020 20:43

kraftbj added 2 commits March 27, 2020 15:45

Set default args for get_fallback_posts

3a73e7f

Fix bad paste into docblock

6556aa7

kraftbj approved these changes Mar 27, 2020

View reviewed changes

kraftbj merged commit 3eb619e into master Mar 27, 2020

kraftbj deleted the fix/top-posts-widget branch March 27, 2020 20:57

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 27, 2020

leogermani commented Mar 27, 2020

View reviewed changes

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Mar 31, 2020

jeherve added a commit that referenced this pull request Mar 31, 2020

Changelog: add #15109

8b9708d

jeherve added a commit that referenced this pull request Apr 9, 2020

Top Posts Widget: provide defaults for newly added param.

6a830e9

$types was added in 8.4, but we did not add a default value when it's not provided. See #15109 for more details.

This was referenced Apr 9, 2020

Top Posts Widget: provide defaults for newly added param. #15395

Merged

Top Posts & Pages widget: applies filter post type choice filter after calculating total posts to display #2369

Closed

kraftbj pushed a commit that referenced this pull request Apr 9, 2020

Top Posts Widget: provide defaults for newly added param. (#15395)

feeadaf

$types was added in 8.4, but we did not add a default value when it's not provided. See #15109 for more details.

kraftbj pushed a commit that referenced this pull request Apr 9, 2020

[not verified] Top Posts Widget: provide defaults for newly added par…

2b405de

…am. (#15395) $types was added in 8.4, but we did not add a default value when it's not provided. See #15109 for more details.

Conversation

leogermani commented Mar 24, 2020 • edited by jeherve Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

How it worked before this PR:

This PR proposes two changes:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

matticbot commented Mar 24, 2020

Uh oh!

jetpackbot commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergeymitr commented Mar 27, 2020

Uh oh!

kraftbj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraftbj Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

leogermani Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

leogermani Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraftbj Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

kraftbj Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

leogermani Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

leogermani Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

leogermani commented Mar 27, 2020

Uh oh!

matticbot commented Mar 27, 2020

Uh oh!

kraftbj commented Mar 27, 2020

Uh oh!

kraftbj commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leogermani Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

leogermani commented Mar 24, 2020 •

edited by jeherve

Loading

jetpackbot commented Mar 24, 2020 •

edited

Loading

kraftbj left a comment •

edited

Loading

leogermani Mar 27, 2020 •

edited

Loading

kraftbj commented Mar 27, 2020 •

edited

Loading