WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 19 months ago

Last modified 19 months 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: 5.1
Component: Privacy Keywords: gdpr fixed-major
Focuses: Cc:
PR Number:

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 19 months ago.
Use completed meta, not confirmed
43913.2.diff (4.0 KB) - added by birgire 19 months ago.
43913.3.diff (3.6 KB) - added by birgire 19 months ago.

Download all attachments as: .zip

Change History (18)

@allendav
19 months ago

Use completed meta, not confirmed

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


19 months ago

#2 @desrosj
19 months ago

  • Milestone changed from Awaiting Review to 4.9.6

#3 @desrosj
19 months ago

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

#4 @SergeyBiryukov
19 months ago

  • Description modified (diff)

#5 @coreymckrill
19 months 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.


19 months ago

#7 @xkon
19 months ago

#43972 was marked as a duplicate.

@birgire
19 months ago

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

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


19 months ago

#10 @allendav
19 months ago

  • Keywords needs-refresh added

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

@birgire
19 months ago

#11 @birgire
19 months ago

  • Keywords needs-refresh removed

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

#12 @iandunn
19 months 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
19 months 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
19 months 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
19 months ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.