WordPress.org

Make WordPress Core

#43960 closed defect (bug) (fixed)

Support for sorting columns in WP_Privacy_Requests_Table

Reported by: birgire Owned by: desrosj
Milestone: 5.1 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-unit-tests
Focuses: administration Cc:

Description

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.

Attachments (10)

43960.diff (3.3 KB) - added by birgire 22 months ago.
43960.2.diff (3.2 KB) - added by birgire 22 months ago.
Screen Shot 2018-05-07 at 9.53.38 AM.png (20.7 KB) - added by desrosj 22 months ago.
43960.3.diff (3.6 KB) - added by pbiron 20 months ago.
refreshed so that it will apply cleaned as of 2018-07-10, but no other change
43960.4.diff (6.7 KB) - added by birgire 20 months ago.
43960.5.diff (7.5 KB) - added by birgire 20 months ago.
43960.6.diff (7.4 KB) - added by birgire 18 months ago.
43960.7.diff (7.5 KB) - added by desrosj 18 months ago.
43960-8.diff (7.7 KB) - added by birgire 14 months ago.
43960-9.diff (7.7 KB) - added by birgire 13 months ago.

Download all attachments as: .zip

Change History (53)

@birgire
22 months ago

#1 @birgire
22 months ago

  • Keywords has-patch added

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.
Last edited 22 months ago by birgire (previous) (diff)

#2 @desrosj
22 months ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 4.9.6

#3 @ianbelanger
22 months ago

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.

Last edited 22 months ago by ianbelanger (previous) (diff)

@birgire
22 months ago

#4 @birgire
22 months ago

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

#5 @ianbelanger
22 months ago

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

#6 @desrosj
22 months ago

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.

#7 @ianbelanger
22 months ago

@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

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


22 months ago

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


22 months ago

#10 @desrosj
22 months ago

  • Keywords needs-refresh added
  • Milestone changed from 4.9.6 to 4.9.7

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

Last edited 22 months ago by desrosj (previous) (diff)

#11 @desrosj
21 months ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#12 @pbiron
20 months ago

  • Component changed from General to Privacy

Moving to the Privacy component.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


20 months ago

@pbiron
20 months ago

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

#14 @pbiron
20 months ago

  • Keywords needs-refresh removed

#15 @pbiron
20 months ago

  • Keywords 2nd-opinion added; needs-testing removed

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
20 months ago

#16 @birgire
20 months ago

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().

#17 @birgire
20 months ago

  • Keywords has-unit-tests added; 2nd-opinion removed

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.

@birgire
20 months ago

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


20 months ago

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


19 months ago

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


19 months ago

#21 @desrosj
19 months ago

  • Keywords gdp removed

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


19 months ago

#23 @desrosj
19 months ago

  • Milestone changed from 4.9.8 to 4.9.9

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.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


19 months ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


18 months ago

#26 @desrosj
18 months ago

  • Keywords needs-refresh added

Patch is currently failing to apply cleanly. Needs refresh.

@birgire
18 months ago

#27 @birgire
18 months ago

  • Keywords needs-refresh removed

@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.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


18 months ago

#29 @desrosj
18 months ago

  • Keywords commit added

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.

@desrosj
18 months ago

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


17 months ago

#31 @pento
17 months ago

  • Milestone changed from 4.9.9 to 5.0.1

#32 @pento
15 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#33 @pento
14 months ago

  • Milestone changed from 5.0.2 to 5.0.3

#34 @audrasjb
14 months ago

  • Keywords needs-refresh added

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
14 months ago

#35 @birgire
14 months ago

  • Keywords needs-refresh removed

43960-8.diff updates @since to 5.0.3

#36 @desrosj
14 months ago

  • Milestone changed from 5.0.3 to 5.1

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

#37 @garrett-eclipse
14 months ago

  • Keywords needs-refresh added

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

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


13 months ago

@birgire
13 months ago

#39 @birgire
13 months ago

@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.

#40 @desrosj
13 months ago

  • Owner set to desrosj
  • Status changed from new to reviewing

#41 @desrosj
13 months ago

  • Keywords needs-refresh removed

#42 @desrosj
13 months ago

  • Version changed from trunk to 4.9.6

#43 @desrosj
13 months ago

  • Keywords commit removed
  • Resolution set to fixed
  • Status changed from reviewing to closed

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

Note: See TracTickets for help on using tickets.