WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 16 months ago

Last modified 15 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:

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

Download all attachments as: .zip

Change History (18)

@allendav
16 months ago

Use completed meta, not confirmed

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


16 months ago

#2 @desrosj
16 months ago

  • Milestone changed from Awaiting Review to 4.9.6

#3 @desrosj
16 months ago

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

#4 @SergeyBiryukov
16 months ago

  • Description modified (diff)

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


16 months ago

#7 @xkon
16 months ago

#43972 was marked as a duplicate.

@birgire
16 months ago

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

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


16 months ago

#10 @allendav
16 months ago

  • Keywords needs-refresh added

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

@birgire
16 months ago

#11 @birgire
16 months ago

  • Keywords needs-refresh removed

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

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

  • Component changed from General to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.