WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 2 weeks ago

Last modified 9 days ago

#43913 closed defect (bug) (fixed)

On sending the personal data export email, the request should be marked COMPLETED

Reported by: allendav Owned by: iandunn
Milestone: 4.9.6 Priority: normal
Severity: normal Version: trunk
Component: Privacy Keywords: gdpr fixed-major
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Unfortunately it is not - instead the confirmed timestamp is updated instead - this is because a change in changeset [43008] incorrectly changed the _wp_privacy_completed_request function to update the CONFIRMATION timestamp and post status to CONFIRMED.

_wp_privacy_completed_request should be updated to touch the _wp_user_request_completed_timestamp and set the post status to request-completed

@desrosj - another 4.9.6 buglet to pick up

Attachments (3)

43913.diff (924 bytes) - added by allendav 4 weeks ago.
Use completed meta, not confirmed
43913.2.diff (4.0 KB) - added by birgire 3 weeks ago.
43913.3.diff (3.6 KB) - added by birgire 3 weeks ago.

Download all attachments as: .zip

Change History (18)

@allendav
4 weeks ago

Use completed meta, not confirmed

This ticket was mentioned in Slack in #gdpr-compliance by allendav. View the logs.


4 weeks ago

#2 @desrosj
4 weeks ago

  • Milestone changed from Awaiting Review to 4.9.6

#3 @desrosj
4 weeks ago

  • Keywords has-patch needs-testing needs-unit-tests added; needs-patch removed

#4 @SergeyBiryukov
3 weeks ago

  • Description modified (diff)

#5 @coreymckrill
3 weeks ago

The patch is working for me on data export requests. The request is marked as complete after either downloading the data or sending the email (though this is not evident until the screen is refreshed).

Note, though, that it does not fix the same issue in data erasure requests, as described in #43922.

This ticket was mentioned in Slack in #gdpr-compliance by coreymckrill. View the logs.


3 weeks ago

#7 @xkon
3 weeks ago

#43972 was marked as a duplicate.

@birgire
3 weeks ago

#8 @birgire
3 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests removed

The 43913.2.diff patch:

  • Adds tests.
  • Fixes a typo in the docblock (succes -> success).
  • Adjusts the description of the docblock (current timestamp instead of datetime).
  • Adjusts the docblock according to the Coding Standard.

ps: In general I'm not sure if we should allow a negative ID, by using absint(). That seems like an invalid input?


Also just want to mention that _wp_user_request_completed_timestamp(), without an input argument, triggers a PHP error/warning, as expected.

Here's an example:

function some_function( $arg ) {
}

some_function();

In PHP 7.2:

<b>Fatal error</b>:  Uncaught ArgumentCountError: Too few arguments to function some_function(), 0 passed in [...][...] on line 5 and exactly 1 expected in [...][...]:2
Stack trace:
#0 [...][...](5): some_function()
#1 {main}
  thrown in <b>[...][...]</b> on line <b>2</b><br />

In PHP 5.6:

<b>Warning</b>:  Missing argument 1 for some_function(), called in [...][...] on line 5 and defined in <b>[...][...]</b> on line <b>2</b><br />
<br />
<b>Warning</b>:  Missing argument 1 for some_function(), called in [...][...] on line 8 and defined in <b>[...][...]</b> on line <b>2</b><br />

This can be fixed with e.g.

function some_function( $arg = null ) {
}

I couldn't find what's preferred in general in core, but both cases are used.

The _wp_user_request_completed_timestamp() is an "private" function, only for core internal use, so in this case it will not matter much, but I'm more thinking about this in core in general :)

Last edited 3 weeks ago by birgire (previous) (diff)

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


3 weeks ago

#10 @allendav
3 weeks ago

  • Keywords needs-refresh added

@birgire - the patch almost applies cleanly - the sql query no longer needs that trailing whitespace cleaned up

@birgire
3 weeks ago

#11 @birgire
3 weeks ago

  • Keywords needs-refresh removed

@allendav thanks, 43913.3.diff is a refreshed patch.

#12 @iandunn
2 weeks ago

  • Owner set to iandunn
  • Resolution set to fixed
  • Status changed from new to closed

In 43183:

Privacy: Mark processed requests as completed instead of confirmed.

r43008 refactored the request flow to make several improvements, but accidentally marked completed requests as confirmed. This commit restores the intended statuses, so that the data and corresponding UI reflect reality.

Props allendav, birgire.
Fixes #43913.

#13 @iandunn
2 weeks ago

  • Keywords fixed-major added; has-patch needs-testing has-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.9 backport.

#14 @SergeyBiryukov
2 weeks ago

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

In 43187:

Privacy: Mark processed requests as completed instead of confirmed.

r43008 refactored the request flow to make several improvements, but accidentally marked completed requests as confirmed. This commit restores the intended statuses, so that the data and corresponding UI reflect reality.

Props allendav, birgire.
Merges [43183] to the 4.9 branch.
Fixes #43913.

#15 @desrosj
9 days ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.