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.

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

Jetpack Post: hide post filters if no items are available #8849

Merged

Conversation

lamosty
Copy link
Contributor

@lamosty lamosty commented Oct 19, 2016

Fixes #8684. Previously, this happened on /posts for a Jetpack site: "(Published/draft/scheduled) all show for a Jetpack site, even when it has none of each type of post". I found out that this was intentional and after asking why, I got this reply from @retrofox:

I’m pretty sure that in that moment posts-count didn’t work for Jetpack plugins
[...] this was more than a year ago

However, it seems like the post counts are still not implemented for Jetpack sites. Is it an issue or do we want to merge this anyways?

Note: In the original issue #8684, @gwwar mentioned that this applies for posts and pages. However, even on non-Jetpack site, /pages always show all the page filters ("Drafts", "Scheduled", etc), even if there are no items available. That's why this PR concerns only posts and not pages.

Testing instructions

  1. With a Jetpack site, navigate to "Blog Posts" in the main (left) sidebar;
  2. Verify that if you have a scheduled or a trashed post, the "Scheduled" or "Trashed" shows in the posts navigation. On the other hand, verify that if you don't have a scheduled or a trashed post, the "Scheduled" or "Trashed" doesn't show in the posts navigation.

@lamosty lamosty added Posts Jetpack [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 19, 2016
@lamosty lamosty added this to the Plans: Maintenance milestone Oct 19, 2016
@lamosty lamosty self-assigned this Oct 19, 2016
@matticbot
Copy link
Contributor

@lamosty lamosty force-pushed the fix/jetpack-hide-post-page-filters-no-items-available branch from b5e2df3 to f241b6e Compare October 19, 2016 10:20
@mtias
Copy link
Member

mtias commented Oct 19, 2016

However, it seems like the post counts are still not implemented for Jetpack sites.

I don't fully understand the situation. The full solution here is to implement post counts, display them, and not render filters if count is zero. When you say post counts are not implemented, do you mean that we don't have the correct data for post counts in jetpack or that we are not displaying it?

@mtias mtias added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 19, 2016
@lamosty
Copy link
Contributor Author

lamosty commented Oct 19, 2016

I don't fully understand the situation. The full solution here is to implement post counts, display them, and not render filters if count is zero.

From the issue title ("Jetpack: Investigate why we display Post/Page filters when there are no items available") and issue description, I got the idea that the task assignee should investigate why we display all the filters even though there are no post items under them and then do the same as on WP.com sites (=> don't show "Trashed" if there are no items under it). There was no mention of adding the counts.

If the task is to add the post counts, display them and not render filters if count is zero (this part is done in this PR), my apologies, will do that.

@lamosty lamosty added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Oct 19, 2016
@mtias
Copy link
Member

mtias commented Oct 19, 2016

@lamosty the thing is that hiding a filter if there are no posts for it relies on the counts data. Imagine a case where you have 100 posts, and post 99 is trashed. You won't know there is a trashed post from the main posts screen until infinite scroll fetches the 100 posts, so you wouldn't know whether you need to display "Trashed" or not. That is why we look at the post counts, because then we know exactly what post statuses you have before fetching any. The problem is that Jetpack had incorrect post counts data when we implemented all this. So the investigation needs to be:

  1. Is Jetpack giving us the right post_counts data now?
  2. If yes, enabled display of post counts and remove filters with no posts in one go.
  3. If no, figure out why, and touch base with Jetpack team to make sure we are syncing this data.

@lamosty lamosty added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 19, 2016
@lamosty lamosty force-pushed the fix/jetpack-hide-post-page-filters-no-items-available branch 2 times, most recently from ee895e0 to bc71d34 Compare October 20, 2016 16:23
@lamosty lamosty added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Oct 20, 2016
@gwwar
Copy link
Contributor

gwwar commented Oct 20, 2016

👍 Tested this and the counts for the most part stay in sync pretty well. I did notice a minor one off on drafts, but I think we can let this go through and keep an eye on if counts can drift.

@gwwar gwwar 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 Oct 20, 2016
@lamosty lamosty force-pushed the fix/jetpack-hide-post-page-filters-no-items-available branch from bc71d34 to fb77443 Compare October 21, 2016 12:02
@lamosty
Copy link
Contributor Author

lamosty commented Oct 21, 2016

Thanks for the review @gwwar!

I tested the drafts counts and it looks pretty solid on my Jetpack testing site:

With 9 drafts:

selection_026

After publishing two of the drafts:

selection_027

@lamosty lamosty merged commit ebab1eb into master Oct 21, 2016
@lamosty lamosty deleted the fix/jetpack-hide-post-page-filters-no-items-available branch October 21, 2016 12:18
@mtias
Copy link
Member

mtias commented Oct 21, 2016

I tested the drafts counts and it looks pretty solid on my Jetpack testing site:

Great!

@lamosty
Copy link
Contributor Author

lamosty commented Oct 21, 2016

Also, I just noticed that draft counts were working even before changes done in this PR, so even if there are errors with them, shouldn't be due to this PR.

@beaulebens
Copy link
Member

I just tried this out on staging on my site and I am seeing claim I have 223 trashed posts. I have 3. Ping me on Slack if you'd like to check it out :)

@mtias
Copy link
Member

mtias commented Oct 24, 2016

Hah, that's what I meant about post counts being a bit messed up. cc @lezama

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants