Support for sorting columns in WP_Privacy_Requests_Table

[birgire]

We should consider adding a support for sorting columns in the WP_Privacy_Requests_Table class.

The requester and requested columns look suitable for sorting.

[birgire]

43960.diff​ is a first pass that:

• Adds support for sorting the requester and requested columns, that are mapped to title and post_date_gmt db cols ordering.
• Uses paged instead of offset query parameter.
• Doesn't override the default search order.
• Sets the default requested order direction as descending.
• Sets the default requester order direction as ascending.
• Sets the screen argument for the instantiation of the table objects, to avoid setting $this->_column_headers directly.

[ianbelanger]

The patch does allow for sorting of requester and requested columns, but it will only sort on the first click, after that the order does not change again.

[birgire]

Thanks @ianbelanger for testing it, I fixed the typos in 43960.2.diff​ ;-)

[ianbelanger]

No problem @birgire, just tested the new patch and all is working fine now.

[desrosj]

This works well. One thing I noticed, though, is that when you sort by date, the default order is correctly reversed but the arrow on the table is incorrect. Incoming screenshot.

[ianbelanger]

@desrosj was that on :hover? It seems to be the default in sortable columns, that on hover it will show the direction that clicking will give you, ASC or DESC. Since this table defaults to DESC, hovering will give you the ASC arrow. See Pages or Posts tables for same behavior, just opposite because they default to Date ASC instead of DESC. Behavior seems normal to me on Chrome 66.0.3359.139

[desrosj]

This needs a refresh, and we just ran out of time for 4.9.6. Punting to 4.9.7.

[desrosj]

Moving all tickets in 4.9.7 to 4.9.8.

[pbiron]

Moving to the Privacy component.

[pbiron]

refreshed so that it will apply cleaned as of 2018-07-10, but no other change

[pbiron]

with 43960.3.diff applied, tests fine for me.

I think the only thing remaining before this can land in 4.9.8 is the possible adding of some @since 4.9.8 tags; but given the particular changes I'm not sure where those should go.

[birgire]

43960.4.diff​

• The default sort is Requested (post_date) DESC. First click on Requested gives post_date ASC. When other field's sorting is active, and we click again on Requested it gives post_date DESC. It also respects the search sorting by relevance.
• Takes the approach from here #32416, regarding the initial order, while a more general approach is being worked on here #32170.
• Adds unit test for the sorting (all passes). We have an abstract class WP_Privacy_Requests_Table, so we use same approach as in Test_WP_Widget_Media using getMockForAbstractClass(). In the tests I had to set the global hook_suffix to handle the screen though, as setting it via mocked arguments input didn't seem to work or using set_current_screen().

[birgire]

43960.5.diff​ uses a mock builder to pass the screen argument to the constructor and reflection property to set the protected post type and request type.

ps: I added @since to the WP_Privacy_Requests_Table::prepare_items() method for the new column sorting support.

[desrosj]

4.9.8 has entered release candidate stages. The changes themselves look good, but the tests are failing locally for me due to some PHP notices.

[desrosj]

Patch is currently failing to apply cleanly. Needs refresh.

[birgire]

@desrosj thanks for testing the previous patch

43960.6.diff​ is a refreshed patch:

• Applies now to the latest trunk.
• Changes @since 4.9.8 to @since 4.9.9.
• Wraps the input arguments with an array in setConstructorArgs( array( $args ) ), instead of using setConstructorArgs( $args ). It looks like I accidentally removed the array when preparing to upload the previous patch in 43960.5.diff​, after running the tests successfully.

Tests run successfully on my install.

[desrosj]

Looks good @birgire! The tests now pass for me and the changes work well in my testing. 43960.7.diff​ has a few minor adjustments.

• Add @param tags for test_columns_should_be_sortable().
• Add @return tag for filter_posts_request().
• Indicate that the first three paramateres returned by the data provider method could also be null in the @return tag.

[audrasjb]

5.0.3 is going to be released in a couple of weeks. We are currently sorting the remaining tickets in the milestone. The ticket is sorted in milestone 5.0.3, but requires a @since refresh and a commit/backport to the associated branch.

[birgire]

43960-8.diff​ updates @since to 5.0.3

[desrosj]

Moving to 5.1 for consideration. This falls outside the 5.0.3 scope.

[garrett-eclipse]

@birgire can you refresh to @since to 5.1 and hopefully it can make it in this version

[birgire]

@garrett-eclipse I bumped the @since to 5.1.0 in 43960-9.diff​.

@desrosj The unit tests run successfully for 43960-9.diff​ on my install. Also tested for the @privacy group.

Here are the manual tests I did on my test install:

• The default sorting is by Requested with post_date DESC (no sorting arrow) (Expected) Request: /wp-admin/tools.php?page=export_personal_data

• First click on Requested gives post_date ASC. (sorting arrow up) (Expected because of the default sort) Request: /wp-admin/tools.php?page=export_personal_data&orderby=requested&order=asc

• Second click on Requested gives post_date DESC. (sorting arrow down) (Expected) Request: /wp-admin/tools.php?page=export_personal_data&orderby=requested&order=desc

• First click on Requester gives post_title ASC. (sorting arrow up) (Expected) Request: /wp-admin/tools.php?page=export_personal_data&orderby=requester&order=asc

• Second click on Requester gives post_title DESC. (sorting arrow down) (Expected) Request: /wp-admin/tools.php?page=export_personal_data&orderby=requester&order=desc

• When Requester sorting is active, and we click on Requested it gives post_date DESC (sorting arrow down) (Expected compared to default) Request: /wp-admin/tools.php?page=export_personal_data&orderby=requested&order=desc

• When Requested sorting is active, and we click on Requester it gives post_title ASC (sorting arrow up) (Expected) Request: /wp-admin/tools.php?page=export_personal_data&orderby=requester&order=asc

• When searching (s=foo) it respects the search-sorting-by-relevance when no arrow shows up (Expected): Request: /wp-admin/tools.php?s=foo&page=export_personal_data&filter-status&orderby&order

• When searching (s=foo): The first click on Requester, sorts by post_title ASC (sorting arrow up) (Expected) Request: /wp-admin/tools.php?s=foo&page=export_personal_data&filter-status&orderby=requester&order=asc

• When searching (s=foo): The next click on Requester, sorts by post_title DESC. (sorting arrow down) (Expected) Request: /wp-admin/tools.php?s=foo&page=export_personal_data&filter-status&orderby=requester&order=desc

• When searching (s=foo): The first click on Requested, sorts by post_date DESC. (sorting arrow down) (Expected) Request: /wp-admin/tools.php?s=foo&page=export_personal_data&filter-status&orderby=requested&order=desc

• When searching (s=foo): The next click on Requested, sorts by post_date ASC. (sorting arrow up) (Expected) Request: /wp-admin/tools.php?s=foo&page=export_personal_data&filter-status&orderby=requested&order=asc

So this looks good to me, where the date field sorting defaults to DESC, but title field sorting defaults to ASC.

[desrosj]

Fixed this in [44628]. I accidentally included the incorrect ticket number.