WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 18 months ago

#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 2 years ago.
43960.2.diff (3.2 KB) - added by birgire 2 years ago.
Screen Shot 2018-05-07 at 9.53.38 AM.png (20.7 KB) - added by desrosj 2 years ago.
43960.3.diff (3.6 KB) - added by pbiron 2 years 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 2 years ago.
43960.5.diff (7.5 KB) - added by birgire 2 years ago.
43960.6.diff (7.4 KB) - added by birgire 23 months ago.
43960.7.diff (7.5 KB) - added by desrosj 23 months ago.
43960-8.diff (7.7 KB) - added by birgire 19 months ago.
43960-9.diff (7.7 KB) - added by birgire 18 months ago.

Download all attachments as: .zip

Change History (53)

@birgire
2 years ago

#1 @birgire
2 years 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 2 years ago by birgire (previous) (diff)

#2 @desrosj
2 years ago

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

#3 @ianbelanger
2 years 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 2 years ago by ianbelanger (previous) (diff)

@birgire
2 years ago

#4 @birgire
2 years ago

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

#5 @ianbelanger
2 years ago

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

#6 @desrosj
2 years 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
2 years 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.


2 years ago

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


2 years ago

#10 @desrosj
2 years 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 2 years ago by desrosj (previous) (diff)

#11 @desrosj
2 years ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#12 @pbiron
2 years 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.


2 years ago

@pbiron
2 years ago

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

#14 @pbiron
2 years ago

  • Keywords needs-refresh removed

#15 @pbiron
2 years 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
2 years ago

#16 @birgire
2 years 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
2 years 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
2 years ago

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


2 years ago

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


2 years ago

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


2 years ago

#21 @desrosj
2 years ago

  • Keywords gdp removed

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


2 years ago

#23 @desrosj
2 years 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.


2 years ago

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


2 years ago

#26 @desrosj
2 years ago

  • Keywords needs-refresh added

Patch is currently failing to apply cleanly. Needs refresh.

@birgire
23 months ago

#27 @birgire
23 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.


23 months ago

#29 @desrosj
23 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
23 months ago

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


22 months ago

#31 @pento
22 months ago

  • Milestone changed from 4.9.9 to 5.0.1

#32 @pento
19 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#33 @pento
19 months ago

  • Milestone changed from 5.0.2 to 5.0.3

#34 @audrasjb
19 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
19 months ago

#35 @birgire
19 months ago

  • Keywords needs-refresh removed

43960-8.diff updates @since to 5.0.3

#36 @desrosj
19 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
18 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.


18 months ago

@birgire
18 months ago

#39 @birgire
18 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
18 months ago

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

#41 @desrosj
18 months ago

  • Keywords needs-refresh removed

#42 @desrosj
18 months ago

  • Version changed from trunk to 4.9.6

#43 @desrosj
18 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.