WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 5 months ago

#40188 assigned defect (bug)

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

Reported by: swissspidy Owned by: Jim_Panse
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Comments Keywords: good-first-bug 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 (9)

40188.patch (657 bytes) - added by Jim_Panse 11 months ago.
Patch for #40188
40188.2.patch (1.1 KB) - added by Jim_Panse 11 months ago.
40188.3.patch (1.1 KB) - added by Jim_Panse 11 months ago.
40188.4.patch (677 bytes) - added by Jim_Panse 11 months ago.
40188.5.patch (1.1 KB) - added by akbarhusen429 8 months ago.
fixes issue
40188.6.patch (549 bytes) - added by akbarhusen429 8 months ago.
fixes issue
40188.diff (1.3 KB) - added by dinhtungdu 8 months ago.
40188.2.diff (1.9 KB) - added by dinhtungdu 8 months ago.
Hide entry table nav when there is no item
40188.7.patch (5.6 KB) - added by birgire 5 months ago.

Download all attachments as: .zip

Change History (23)

@Jim_Panse
11 months ago

Patch for #40188

#1 @Jim_Panse
11 months 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
11 months 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
11 months 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
11 months ago

#4 @swissspidy
11 months 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
11 months 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
11 months ago

#6 @swissspidy
11 months 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
11 months ago

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

@Jim_Panse
11 months ago

#8 @menakas
9 months 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
8 months ago

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

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

@akbarhusen429
8 months ago

fixes issue

@akbarhusen429
8 months ago

fixes issue

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


8 months ago

#11 @swissspidy
8 months 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
8 months ago

#12 @dinhtungdu
8 months 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 8 months ago by dinhtungdu (previous) (diff)

@dinhtungdu
8 months ago

Hide entry table nav when there is no item

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


8 months ago

@birgire
5 months ago

#14 @birgire
5 months 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 5 months ago by birgire (previous) (diff)
Note: See TracTickets for help on using tickets.