WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 8 months ago

Last modified 8 months ago

#40188 closed defect (bug) (fixed)

Filter button should not appear when no comments are available in list

Reported by: swissspidy Owned by: Jim_Panse
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch has-unit-tests
Focuses: ui, administration Cc:

Description

Previously: #37407. See also: #38341. Both tickets contain some screenshots that demonstrate the issue.

It doesn't make sense to be able to filter the comments list table when there are are no (trashed/spam) comments available.

Maybe there are other list tables that might need to be checked as well.

Attachments (14)

40188.patch (657 bytes) - added by Jim_Panse 4 years ago.
Patch for #40188
40188.2.patch (1.1 KB) - added by Jim_Panse 4 years ago.
40188.3.patch (1.1 KB) - added by Jim_Panse 4 years ago.
40188.4.patch (677 bytes) - added by Jim_Panse 4 years ago.
40188.5.patch (1.1 KB) - added by akbarhusen429 4 years ago.
fixes issue
40188.6.patch (549 bytes) - added by akbarhusen429 4 years ago.
fixes issue
40188.diff (1.3 KB) - added by dinhtungdu 4 years ago.
40188.2.diff (1.9 KB) - added by dinhtungdu 4 years ago.
Hide entry table nav when there is no item
40188.7.patch (5.6 KB) - added by birgire 4 years ago.
40188.8.patch (5.4 KB) - added by rebasaurus 9 months ago.
Refresh
Screen Shot 2020-07-17 at 12.41.16 AM.png (59.6 KB) - added by whyisjake 9 months ago.
40188.3.diff (5.7 KB) - added by whyisjake 9 months ago.
40188.4.diff (560 bytes) - added by whyisjake 9 months ago.
40188.5.diff (549 bytes) - added by whyisjake 9 months ago.

Download all attachments as: .zip

Change History (49)

@Jim_Panse
4 years ago

Patch for #40188

#1 @Jim_Panse
4 years ago

Not quite sure if i did it the right way. My first patch, so have some patience with me, please. And guide me into the right direction, if i forgot something.

#2 @swissspidy
4 years ago

@Jim_Panse Thanks for your patch.

Please note that it also hides the filter button after applying a filter. For example, when I have 1 regular comment on my blog and I change the filter from "All comment types" to "Pings", the filter button now is hidden and I can't change the filter again.

#3 @Jim_Panse
4 years ago

  • Keywords has-patch added; needs-patch removed

@swissspidy Attached a new patch file and added a helper function. Please have a look, thank you!

@Jim_Panse
4 years ago

#4 @swissspidy
4 years ago

@Jim_Panse It looks like your patch is reversed (what is red should be green).

A helper method shouldn't be needed though. Why not simply use get_comments( array( 'number' => 1 ) ) in the condition?

#5 @Jim_Panse
4 years ago

Ups, new Patch attached.

I think it makes for a much cleaner code, if i add the helper function instead of calling get_comments directly in the if statement.

@Jim_Panse
4 years ago

#6 @swissspidy
4 years ago

It's really not needed though.

$comments = get_comments( array( 
  'number' => 1, 
) ); 
if( $comments ) {
  return true; 
} else { 
  return false; 
}

is a worse version of

return get_comments( array( 
  'number' => 1, 
) );

This means you can just as well use

if ( 'top' === $which && get_comments( array( 'number' => 1 ) ) ) { … }

which is way cleaner.

#7 @Jim_Panse
4 years ago

You are right, my bad. New Patch attached. :)

@Jim_Panse
4 years ago

#8 @menakas
4 years ago

A thought, though I haven't deep dived into the code to find its feasibility:

Maybe there are other list tables that might need to be checked as well.

@swissspidy concern is valid. This is a feature common to all list tables (WP_List_Table) i.e, all lists should display the Filter option if and only if there is at least one list item.

In fact, in the comments list table, this functionality should reflect in all sub lists - All, Pending, Approved, Spam and Trash. Likewise, in other lists like posts, categories, tags and so on.

With this in mind, a solution at the WP_List_Table class level would be better.

#9 @DrewAPicture
4 years ago

  • Owner set to Jim_Panse
  • Status changed from new to assigned

Assigning ownership to mark the good-first-bug as "claimed".

@akbarhusen429
4 years ago

fixes issue

@akbarhusen429
4 years ago

fixes issue

This ticket was mentioned in Slack in #core by akbarhusen. View the logs.


4 years ago

#11 @swissspidy
4 years ago

  • Keywords needs-patch added; has-patch removed

@akbarhusen429 Have you tested this? Because there's no has_comments() method in WP_Comments_List_Table(). That means your code doesn't work.

There was such a method in 40188.2.patch, but as per prior comments it was not really needed.

Also see another comment above:

This is a feature common to all list tables (WP_List_Table) i.e, all lists should display the Filter option if and only if there is at least one list item.
With this in mind, a solution at the WP_List_Table class level would be better.

@dinhtungdu
4 years ago

#12 @dinhtungdu
4 years ago

  • Keywords has-patch added; needs-patch removed

I think we really encounter the problem with Comment type ( Comment and Ping) as @swissspidy said before

Please note that it also hides the filter button after applying a filter. For example, when I have 1 regular comment on my blog and I change the filter from "All comment types" to "Pings", the filter button now is hidden and I can't change the filter again.

Why a user need to see a "Ping" option when there isn't any pingback or trackback. We should check the number of comment or ping before display the filter option for it respectively.
Others like post, user, plugin doesn't have this problem.
From here, we can hide the extra_tablenav which contains Filter dropdown when there isn't any item.

Last edited 4 years ago by dinhtungdu (previous) (diff)

@dinhtungdu
4 years ago

Hide entry table nav when there is no item

This ticket was mentioned in Slack in #core by dinhtungdu. View the logs.


4 years ago

@birgire
4 years ago

#14 @birgire
4 years ago

  • Keywords has-unit-tests added

In 40188.7.patch

  • we introduce a new comment_status_dropdown() method of WP_Comments_List_Table, to make it more readable, in a similar way as the categories_dropdown() method was introduced in WP_Posts_List_Table.
  • the select option's content is escaped, just as in the walker_pagedropdown().
  • we test if the filter admin_comment_types_dropdown returns an empty array, to make sure that no empty <select> dropdown is displayed.
  • only display a status in the dropdown with a get_comments( array( 'number' => 1, 'type' => $type ) check, as previously suggested.

I like the idea to remove the table nav for no comments in WP_List_Table, like:

protected function display_tablenav( $which ) {
    if( ! $this->has_items() ) {
        return;
    }
    ...

but it sounded out of scope according to the current ticket title, so I did a smaller step instead, in WP_Comments_List_Table only, similar to the approach in #37407

Added a test, based on the ones in #38341

  • test_filter_button_should_not_be_shown_if_there_are_no_comments()


and then two more:

  • test_filter_button_should_be_shown_if_there_are_comments()
  • test_filter_comment_status_dropdown_should_be_shown_if_there_are_comments()

It was a real headache to make the last two work as expected, because after lot of digging I found out that other tests where modifying the $_REQUEST global and that was being picket up here by our WP_Comments_List_Table instance. That meant the offset ended up as 120 and no comments where returned ;-)

So I will create a new ticket to fix these older tests, so currently our tests are blocked by that.

Update: See ticket #42044 addressing that.

Last edited 4 years ago by birgire (previous) (diff)

This ticket was mentioned in Slack in #core by sergey. View the logs.


11 months ago

#16 @SergeyBiryukov
11 months ago

  • Milestone changed from Awaiting Review to 5.5

#17 @davidbaumwald
9 months ago

  • Keywords needs-refresh needs-dev-note added

The latest patch no longer applies cleanly to trunk, so marking this for a refresh. Also, with the new function, this deserves a call-out on the Misc Dev Note.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


9 months ago

@rebasaurus
9 months ago

Refresh

#19 @rebasaurus
9 months ago

  • Keywords needs-refresh removed

#20 @whyisjake
9 months ago

There is a pretty big forehead on the page if the filter buttons are missing. Should we add some CSS to make it look a little nicer?

I might be over thinking this.

@whyisjake
9 months ago

#21 @whyisjake
9 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 48521:

Comments: Don't show the filter/pagination actions if there are no comments to list.

It doesn't make sense to be able to filter the comments list table when there are are no (trashed/spam) comments available.

Fixes #40188.
Props swissspidy, Jim_Panse, menakas, akbarhusen429, dinhtungdu, birgire, SergeyBiryukov, davidbaumwald, rebasaurus, whyisjake.

#22 @whyisjake
9 months ago

Missed pressing submit on the comment, but 40188.3.diff is adding a check to prevent the top actions nav from being shown. This gets rid of the big forehead on the page.

#23 @SergeyBiryukov
9 months ago

if ( ! empty( $output ) && $this->has_items() )

It's not quite clear why this check doesn't use the $has_items static variable declared above, while the next check does use the variable:

if ( ( 'spam' === $comment_status || 'trash' === $comment_status ) && current_user_can( 'moderate_comments' ) && $has_items ) {

If that's intentional, it should be documented.

It's also worth noting that class-wp-posts-list-table.php and class-wp-ms-sites-list-table.php don't check $this->has_items(), just ! empty( $output ). Would be nice to bring some consistency here.

#24 @desrosj
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening this to discuss/address the points raised in comment:23.

#25 @desrosj
9 months ago

  • Keywords needs-dev-note removed

I'm also going to exclude this from the miscellaneous developer note. Even though there is a new method being introduced, there is not much here for developers to be aware of.

@whyisjake
9 months ago

#26 @whyisjake
9 months ago

@SergeyBiryukov 40188.5.diff attached should make the conditionals the same.

It's also worth noting that class-wp-posts-list-table.php and class-wp-ms-sites-list-table.php don't check $this->has_items(), just ! empty( $output ). Would be nice to bring some consistency here.

Moving this over to #50815 so that it isn't blocked against the 5.5 milestone.

Last edited 9 months ago by whyisjake (previous) (diff)

@whyisjake
9 months ago

#27 @whyisjake
8 months ago

  • Keywords commit dev-feedback added; good-first-bug removed

If we can get a review here, we can address this little bit of cleanup in 5.5 RC2 today.

This ticket was mentioned in Slack in #core by whyisjake. View the logs.


8 months ago

#29 @SergeyBiryukov
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48722:

Comments: Use the existing static variable instead of calling ::has_items() again in WP_Comments_List_Table::extra_tablenav().

Additionally, removed unnecessary esc_html() on the Filter button label. Core translations are considered safe, and this label is not escaped in any other instance.

Props whyisjake, SergeyBiryukov.
Fixes #40188.

#30 follow-up: @SergeyBiryukov
8 months ago

  • Keywords dev-reviewed added; dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for the 5.5 branch.

#31 @SergeyBiryukov
8 months ago

[48722] caused unit test failures:

There were 2 failures:

1) Tests_Admin_includesListTable::test_filter_button_should_be_shown_if_there_are_comments
Failed asserting that '<div class="alignleft actions"></div>' contains "id="post-query-submit"".

/var/www/tests/phpunit/tests/admin/includesListTable.php:316

2) Tests_Admin_includesListTable::test_filter_comment_status_dropdown_should_be_shown_if_there_are_comments
Failed asserting that '<div class="alignleft actions"></div>' contains "id="filter-by-comment-type"".

/var/www/tests/phpunit/tests/admin/includesListTable.php:338

Reverting for now, let's clean this up in #50815.

#32 @SergeyBiryukov
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48724:

Comments: Restore the ::has_items() call in WP_Comments_List_Table::extra_tablenav() for now, until unit tests are updated accordingly.

Follow-up to [48722].

Fixes #40188. See #50815.

#33 @SergeyBiryukov
8 months ago

  • Keywords commit dev-reviewed removed

#34 in reply to: ↑ 30 @SergeyBiryukov
8 months ago

Replying to SergeyBiryukov:

Reopening for the 5.5 branch.

For reference, [48722] and [48724] are in trunk only and should not be backported to the 5.5 branch.

#35 @SergeyBiryukov
8 months ago

In 48741:

Comments: Remove a few more unnecessary instances of esc_html() in WP_Comments_List_Table::comment_status_dropdown().

Core translations are considered safe, and these labels are not escaped in any other instances.

Follow-up to [48521], [48722], [48724].

See #40188, #50815.

Note: See TracTickets for help on using tickets.