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

[desrosj]

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.

[subrataemfluence]

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?

[desrosj]

@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.

[desrosj]

Moving to the new Privacy component.

[desrosj]

Marking privacy bugs as introduced in 4.9.6.

[desrosj]

Moving all tickets in 4.9.7 to 4.9.8.

[desrosj]

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

[javorszky]

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

[javorszky]

Change wording for the non-action notice.

[javorszky]

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.

[birgire]

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.

[javorszky]

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]

Simlifying code per feedback, fix return docblock of _wp_privacy_resend_request

[javorszky]

Simlifying code per feedback, fix return docblock of _wp_privacy_resend_request

[desrosj]

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?

[javorszky]

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.

[javorszky]

Fix an indentation issue.

[javorszky]

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.

[JoshuaWold]

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?

[desrosj]

Related: #44941.

[javorszky]

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

[javorszky]

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

[javorszky]

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.

[garrett-eclipse]

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:

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?

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

[kjellr]

👋 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!

[garrett-eclipse]

Refresh for 5.6

[garrett-eclipse]

Confirmation resend messaging.

[garrett-eclipse]

Request deletion messaging

[garrett-eclipse]

Thanks for all the work on this so far everyone. I've refreshed this in 44081.7.diff to see if we can address this in 5.6.

I've gone with the approach of two notices to show what is successful separate from the failures. As well as improved the verbiage as previously discussed.

Please test and we'll move this along. It will need a refresh once #44266 is committed.

[hellofromTonya]

📣 Call for testers 📣

This ticket is ready for commit once it gets some testing. Calling on our testers to help us get this over the finish line to land in 5.6 Beta 1 (which is next week, ie 20th).

[hellofromTonya]

Testing resend on 44081.8 patch. Works as expected.

[hellofromTonya]

Testing delete on 44081.8 patch. Works as expected.

[hellofromTonya]

Refreshed to latest wordpress-develop master branch. Added guard clause to unwrap code for improved readability.

[hellofromTonya]

• Refreshed 44081.7.diff patch to the current wordpress-develop master branch.
• Tested both the resend and delete requests admin notices. Worked as expected.

Removing needs-testing and adding commit.

@garrett-eclipse can you confirm this is ready to go?

[garrett-eclipse]

Awesome thanks @hellofromTonya looks good to me.

[SergeyBiryukov]

This was fixed in [49331].

[helen]

In 49333:

Privacy: Show failures before successes for all bulk actions.

Props garrett-eclipse.
See #44081, [49331].