WordPress.org

Make WordPress Core

Opened 19 months ago

Last modified 5 months ago

#44081 assigned defect (bug)

Privacy email request failures do not get reported if there are also successful ones

Reported by: desrosj Owned by: desrosj
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-screenshots needs-refresh
Focuses: administration Cc:
PR Number:

Description

When bulk resending emails for export and erasure requests, failures are not properly reported if there are also successful ones.

Example: 3 requests are bulk resent, 2 succeed. The notice when the page reloads is green and says Re-sent 2 request and will not mention the failed request.

When there are mixed results, the notice should be a warning (orange) and say Resent X requests, but X failed to resend., or something similar.

Also, when no requests are resent successfully, the notice is green and says Resent 0 requests. This should also be a warning.

Attachments (8)

Screenshot from 2018-05-15 19_29_23.png (21.8 KB) - added by subrataemfluence 19 months ago.
44081.diff (1.8 KB) - added by javorszky 15 months ago.
Add ability to have different notifications for successful / failed resends.
44081.2.diff (1.8 KB) - added by javorszky 15 months ago.
Change wording for the non-action notice.
44081.3.diff (2.3 KB) - added by javorszky 15 months ago.
Simlifying code per feedback, fix return docblock of _wp_privacy_resend_request
44081.3.2.diff (2.3 KB) - added by javorszky 15 months ago.
Simlifying code per feedback, fix return docblock of _wp_privacy_resend_request
44081.4.diff (2.3 KB) - added by javorszky 15 months ago.
Fix an indentation issue.
44081.5.diff (2.2 KB) - added by javorszky 15 months ago.
Reword messages, remove "no requests to act upon" message.
44081.6-44941.diff (2.5 KB) - added by javorszky 15 months ago.
Same as 44081.5, but also implements a mixed message bar and only displays one bar. 44941 is required to go in before this.

Download all attachments as: .zip

Change History (49)

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


19 months ago

#2 @subrataemfluence
19 months ago

I have one question in this regard. Sorry if I am posting it in the wrong thread.

I added three email addresses on "Erase Personal Data" screen at my local environment. The environment does not send any real email though. So I used "Force Erase Personal Data" link to change Pending status to Completed.

Each of these email addresses on this screen are attached to a user.

When I select and try Resend Email in bulk, I always see Re-sent 0 requests.

Is this because the local system does not send real emails?

#3 @desrosj
19 months ago

@subrataemfluence It may be. What does your local setup look like?

I usually use VVV, which comes with MailCatcher. This prevents emails from sending, but catches them in a UI where you can read the emails that went through.

#4 @desrosj
19 months ago

  • Component changed from General to Privacy

Moving to the new Privacy component.

#5 @desrosj
19 months ago

  • Version set to 4.9.6

Marking privacy bugs as introduced in 4.9.6.

#6 @desrosj
19 months ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

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


17 months ago

#8 @desrosj
17 months ago

  • Keywords gdpr removed

Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.

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


17 months ago

#10 @desrosj
17 months ago

  • Milestone changed from 4.9.8 to 4.9.9

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


16 months ago

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


16 months ago

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


16 months ago

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


16 months ago

#15 @idea15
15 months ago

  • Owner set to javorszky
  • Status changed from new to assigned

@javorszky
15 months ago

Add ability to have different notifications for successful / failed resends.

@javorszky
15 months ago

Change wording for the non-action notice.

#16 @javorszky
15 months ago

  • Keywords has-patch added; needs-patch removed

The diff I just uploaded (44081.2) achieves the following things:

  • if the bulk action was requested with no requests chosen, it will add an error with the text "There were no requests to resend emails for."
  • if the resends were all successful, it will display the successful "Re-sent X requests" green notice.
  • if the resends all failed, it will display a _new_ "Failed to re-send X requests" red notice.
  • if there's a mix of successful and failed resends, it will display both the successful and failed resend notices.

## How to test:

  • apply patch
  • have at least one request under Tools -> Erase Personal Data
  • select "Resend requests" or "Delete" from the drop down menu without selecting any of the requests. This should summon the "There were no requests to act on" text.
  • you might need to edit the `wp-includes/admin/user.php and set both $count and $failure_count to a positive integer after the foreach loop
  • Do a resend emails bulk action again (make sure you have mailcatcher / mailhog / mailtrap set up, or selecting your own email address)
  • that should summon both notices with the appropriate numbers.

https://d3vv6lp55qjaqc.cloudfront.net/items/2S0d1t1u1U0M073T1Q0r/Screen%20Shot%202018-09-08%20at%2012.38.47.png

https://d3vv6lp55qjaqc.cloudfront.net/items/0d142k0o2t3x2e1I4240/Screen%20Shot%202018-09-08%20at%2012.40.05.png

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


15 months ago

#18 follow-up: @birgire
15 months ago

Thanks for the patch @javorszky

In 44081.2.diff I was wondering if we might simplify:

if ( $resend && is_wp_error( $resend ) ) {
	$failure_count++;
}

with

if ( is_wp_error( $resend ) ) {
	$failure_count++;
}

as it looks like $resend is either true or WP_Error.

Another minor suggestion is to skip the else-part:

if ( empty( $request_ids ) ) {
	add_settings_error(
		'bulk_action',
		'bulk_action',
		__( 'There were no requests to act on.' )
	);
	return;
}

check_admin_referer( 'bulk-privacy_requests' );

instead of

if ( $request_ids ) {
	check_admin_referer( 'bulk-privacy_requests' );
} else {
	add_settings_error(
		'bulk_action',
		'bulk_action',
		__( 'There were no requests to act on.' )
	);
	return;
}


ps: It looks like _wp_privacy_resend_request() might need adjustments for it's inline docs, as it says it can return false, but it doesn't look like it.

#19 in reply to: ↑ 18 @javorszky
15 months ago

  • Keywords needs-refresh added; has-patch removed

Thanks for the review @birgire ! I'll get these fixed up, along with the inline doc change for _wp_privacy_resend_request() later today.

@javorszky
15 months ago

Simlifying code per feedback, fix return docblock of _wp_privacy_resend_request

@javorszky
15 months ago

Simlifying code per feedback, fix return docblock of _wp_privacy_resend_request

#20 @javorszky
15 months ago

  • Keywords has-patch has-screenshots added; needs-refresh removed

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


15 months ago

#22 @desrosj
15 months ago

  • Keywords ui-feedback added

For context, it seems a few of the points @birgire pointed out stem from the changes in [43568], which ensures wp_send_user_request() returns a WP_Error instead of the result of the wp_mail() call.

I took a look at other screens in core with WP_List_Tables to see how they handle instances where the user does not select items to perform an action on. Posts, taxonomies, users, and comments all refresh the page and do not show a notice. The plugins screen, however, prevents a reload and displays an error notice with JavaScript. I wonder if we can move the scenario where nothing is selected to a new ticket so that all WP_List_Table pages can benefit.

The patch itself looks good and works well for me. There are a few minor alignment issues that can be fixed later. @javorszky was there a reason why you went with two notices (green and red) instead of one warning notice (orange)? I am flagging for some UI feedback, but I think that will prevent admin notice overload.

Also, I think that we should account for failures when attempting to delete requests. wp_delete_post() will return WP_Errors when deleting is not successful.

@joshuawold can you weigh in with your recommendation how best to implement these notices?

#23 @javorszky
15 months ago

was there a reason why you went with two notices (green and red) instead of one warning notice (orange)?

Yup. This is the relevant docblock for the add_settings_error function:

 * @param string $type    Optional. Message type, controls HTML class. Accepts 'error' or 'updated'.
 *                        Default 'error'.
 */
function add_settings_error( $setting, $code, $message, $type = 'error' ) {
        global $wp_settings_errors;

        $wp_settings_errors[] = array(
                'setting' => $setting,
                'code'    => $code,
                'message' => $message,
                'type'    => $type,
        );
}

I took the liberty of believing the inline documentation instead of going hunting for the actual declared css classes that are available to me.

I'll fix up the spacing issue.

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


15 months ago

@javorszky
15 months ago

Fix an indentation issue.

#25 @javorszky
15 months ago

Indentation issue has been fixed.

Yesterday's office hours briefly touched on this. The end result was that two different notices are better to avoid people only reading the first half of a proposed notice that would contain both info.

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


15 months ago

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


15 months ago

#28 follow-up: @JoshuaWold
15 months ago

Following up on this ticket, my preference from a “keep things as clean as possible” perspective would be to have failed and successful notices incorporated into the same notice. If we were to do that it would probably look something like this:

“Attempted to re-send requests: 5 successful, 11 failed.” With an orange bar.

However, given the feedback in previous comments about folks missing the failed notices, I’m inclined to agree that we should keep the success and failed notices separate, at least for now. My only feedback then would be to update the wording slightly. Right now it’s a bit hard to tell if a message was a success based purely on the wording (imagine a situation with a screen reader or someone that was hard of sight, where color is harder to differentiate).

My suggestion then is to make the following changes:

  1. Change “Re-sent 5 requests” to “Successfully re-sent 5 requests.
  2. Change “There were no requests to act on” to “There were no requests selected to act on.” As a followup I’m wondering if we could just keep people from ever seeing this notice by not not allowing the previous action to happen?

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


15 months ago

#30 @desrosj
15 months ago

Related: #44941.

@javorszky
15 months ago

Reword messages, remove "no requests to act upon" message.

@javorszky
15 months ago

Same as 44081.5, but also implements a mixed message bar and only displays one bar. 44941 is required to go in before this.

#31 @javorszky
15 months ago

Uploaded two new files.

Both take into account @JoshuaWold 's feedback on wording. I've also removed the *"There were no requests to act on"* message as it did not achieve anything really.

A variant is also up there, in case 44941 goes in.

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


15 months ago

#33 in reply to: ↑ 28 @garrett-eclipse
15 months ago

  • Keywords needs-refresh added; has-patch removed

Thanks @javorszky this is some great progress.

First I just wanted to flag to discussion here that the bulk resend action was proposed to be removed in #44673. Discussion on that item may determine the fate of this functionality so your input is appreciated.

Taking a look at existing core conventions a few notes.

Replying to JoshuaWold:

  1. Change “There were no requests to act on” to “There were no requests selected to act on.” As a followup I’m wondering if we could just keep people from ever seeing this notice by not not allowing the previous action to happen?

The existing response on Bulk Actions when no items are selected is to have no response. This is the same for js and php so if js no action is taken and if php reload occurs.
*That being said I did like the idea that a response should be given on these actions with no items are selected so opened #45006 and provided an alternate concept of simply disabling the button when no items selected
If the 'There were no requests to act on.' notice stays, then the return there should be a WP_Error instead of just return;
NOTE: This message seems to have been removed in 44081.5.diff so may be a moot point here. But the return; looks to be kept in 44081.6-44941.diff so would need to be updated to return a WP_Error as the phpdoc indicates.

Also should the bulk action be consistent with the single action in it's use of $result instead of $resend;
https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/user.php#L654

Replying to JoshuaWold:

  1. Change “Re-sent 5 requests” to “Successfully re-sent 5 requests.”

As to verbiage we should indicate it's a 'confirmation request' and the count should be first with the action success or failure last as follows;
with other notices in core.
'X confirmation request(s) resent successfully.'
'X confirmation requests(s) failed to resend.'
*Note: resend/resent are without a hyphen
This is also the format with the other privacy notices placing the adverb last;
https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/user.php#L667
https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/user.php#L745

One thing to note about the initial 'Send Request' notice (username_or_email_for_privacy_request) is the use of 'initiated' verbiage instead of 'sent'/'send'. But in the single retry notice (privacy_action_email_retry) it's 'sent again'. For these I'm referring to the above links.

Also the check_admin_referer found in core seems to be within each action of the switch/if rather than preceding it;
https://github.com/WordPress/WordPress/blob/6fd8080e7ee7599b36d4528f72a8ced612130b8c/wp-admin/link.php#L35
https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/update.php#L28
*They seem to have separate referers per action.

And to wrap up I would go with the separate notices presented in 44081.5.diff for code simplicity, ease of translation and the distinction allows the failed to be denoted as an error to draw attention to itself.

All the best

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


15 months ago

#35 @pento
14 months ago

  • Milestone changed from 4.9.9 to Future Release

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


14 months ago

#37 @desrosj
13 months ago

  • Owner changed from javorszky to desrosj

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


6 months ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


6 months ago

#40 @kjellr
6 months ago

  • Keywords ui-feedback removed

👋 We discussed this briefly in this week's #design triage in slack:

https://wordpress.slack.com/archives/C02S78ZAL/p1559580286033400

It looks like @JoshuaWold provided some great feedback above, so we'll remove the ui-feedback label for now. Feel free to re-add if more feedback is required!

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


5 months ago

Note: See TracTickets for help on using tickets.