Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Widgets - Top Posts: use the WP.com REST API endpoint #6335

Merged
merged 1 commit into from
Feb 10, 2017
Merged

Widgets - Top Posts: use the WP.com REST API endpoint #6335

merged 1 commit into from
Feb 10, 2017

Conversation

eliorivero
Copy link
Contributor

@eliorivero eliorivero commented Feb 8, 2017 •
edited
Loading

Fixes #2196

Changes proposed in this Pull Request:

  • stop fetching a CSV with posts stats and use the /sites/%site%/stats/top-posts endpoint in wpcom.

Testing instructions:

  • add the widget, it should correctly display the top posts

Proposed changelog entry for your changes:

Widgets: the Top Posts widget now uses an endpoint from WP.com REST API which allows better performance and is more flexible for future updates.

@eliorivero eliorivero added [Feature] Extra Sidebar Widgets [Status] Needs Review This PR is ready for review. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Feb 8, 2017
@eliorivero eliorivero self-assigned this Feb 8, 2017
@eliorivero eliorivero requested a review from jeherve February 8, 2017 20:41
@eliorivero eliorivero mentioned this pull request Feb 8, 2017
Closed
jeherve
jeherve reviewed Feb 8, 2017
View reviewed changes
modules/widgets/top-posts.php Outdated
@@ -488,12 +488,14 @@ function get_by_views( $count, $args ) {
$days = 2;
}

$post_view_posts = stats_get_csv( 'postviews', array( 'days' => absint( $days ), 'limit' => 11 ) );
if ( ! $post_view_posts ) {
$post_view_posts = stats_get_from_restapi( array(), 'top-posts?max=10&summarize=1&num=' . absint( $days ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Did you add the query parameters to the sub-endpoint URL instead of the arguments array to bypass #5860?

Also, is there a reason why you went with 10 instead of 11? in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, if I added them as array in that first argument they were filtered out so I added them as query parameters.

There's no explanation about the 11 in the commit that introduced it and the widget says it will returns 10 at most so it makes sense to set it at 10. Please let me now if I'm missing something here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that commit message wasn't very helpful.

The widget can indeed be set to return 10 posts maximum. However, among those 10 posts you'll often find the home page, and that home page is not displayed in the Top Posts Widget.

That means that if you do a query for 10 posts, you may end up with only 9 unique posts that can be used in the widget.

To avoid displaying only 9 posts when the site owner asked for 10 posts in the widget settings, we decided to query for one more than the maximum (11). We then make sure to respect the specified $count by adding that limit to the array of posts that we build later on.

I hope this clarifies things a bit. I'll add a link to this comment in the original commit, just in case this conversation comes up again in the future :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! I've verified it works as you described so I set the number to 11 as before.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Feb 8, 2017
@eliorivero eliorivero force-pushed the update/use-top-posts-endpoint branch from 06429f6 to 422b0ee Compare February 8, 2017 20:58
@eliorivero eliorivero added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 8, 2017
@jeherve jeherve added [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Feb 8, 2017
@eliorivero eliorivero force-pushed the update/use-top-posts-endpoint branch from 422b0ee to d0ba8ac Compare February 9, 2017 01:32
@eliorivero eliorivero added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 9, 2017
@thingalon
Copy link
Member

Looks good to me, appears to work well. :)

@thingalon thingalon 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 Feb 9, 2017
@samhotchkiss samhotchkiss merged commit 1affa2d into master Feb 10, 2017
@samhotchkiss samhotchkiss deleted the update/use-top-posts-endpoint branch February 10, 2017 04:55
@samhotchkiss samhotchkiss removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 10, 2017
jeherve added a commit that referenced this pull request Feb 21, 2017
@jeherve
Changelog: add #6335
ce92d54
dereksmart pushed a commit that referenced this pull request Feb 28, 2017
@jeherve @dereksmart
Jetpack 4.7: Changelog (#6331)
a9adf86 
* Changelog: update stable tag and move changelog to changelog.txt

Also remove old releases from readme.txt to keep the changelog tab short.

* Changelog: add #5883

Also update the filter's docblock to match new version.

* Changelog: add #5938

* Changelog: add #6298

* Changelog: add #3405

* Changelog: add #5941

* Changelog: add #6239

* Changelog: add #6281

* Changelog: add #6303

* Changelog: add #6018

* Changelog: add #6300

* Changelog: add #6296

* Changelog: add #6130

* Changelog: add #6292

* Readme: remove extra "on".

* Changelog: add #6307

* Changelog: add #3297

* Changelog: add #6275

* Changelog: add #6321

* Changelog: add #6297

* Readme: update the support forum link anchor.

Anchor changed when WordPress.org forums were updated to bbPress 2

* Readme: update list of a12s, it wasn't up to date anymore!

* Changelog: add #6338

* Changelog: add #6337

* Changelog: add #6335

* Changelog: add #6333

* Testing List: first version of the 4.7 testing list.

* Changelog: add #6332

* Changelog: add #6325

* Changelog: add #6326

* Changelog: add #6339

* Changelog: add #6342

* Changelog: add #6343

* Changelog: add #6346

* Changelog: add #6347

* Changelog: add #6279

* Changelog: add #6306

* Changelog: add #6312

* Changelog: add #6316

* Changelog: add #6171

* Changelog: add #6317

* Changelog: add #6246

* Changelog: add #6263

* Changelog: add #4220

* Changelog: add #5888

* Changelog: add #3406

* Changelog: add #3637

* Changelog: add #6320

* Changelog: add #5992

* Changelog: add #6322

* Changelog: add #6324

* Changelog: add #6352

* Changelog: add #6355

* Changelog: add #6360

* Changelog: add #6362

* Changelog: add #6369, #6382

* Changelog: add #6370

* Changelog: add #6375

* Changelog: add #6383

* Changelog: add #6384

* Changelog: add #6386

* Changelog: add #6395

* Changelog: add #6403

* Changelog: add #6406

* Changelog: add #6418

* Changelog: add #6419

* Changelog: add #6434

* Changelog: add #6446

* Changelog: add #6006

* Changelog: add #6096

* Changelog: add #6399

* Changelog: fix typo.

@see #6331 (comment)

* Changelog: add #6440

* Changelog: add #6443

* Changelog: add #6445

* Changelog: add #6463

* Changelog: add #6468

* Changelog: add #6471

* Changelog: add #6474

* Changelog: add #6480

* Changelog: add #6497

* Changelog: add #6499

* Changelog: add #6514

* Changelog: add #6267

* Changelog: add #5940

* Changelog: add #6492

* Changelog: add #5281

* Changelog: add #6327

* Changelog: add #6451

* Changelog: add #6525

* Changelog: add #6530
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeherve jeherve jeherve left review comments

Assignees

@eliorivero eliorivero

Labels
[Feature] Extra Sidebar Widgets [Focus] Performance Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eliorivero @thingalon @jeherve @samhotchkiss @matticbot