Check if a user can edit the post/page before sending a notification. by WPprodigy · Pull Request #449 · Automattic/edit-flow

WPprodigy · 2018-04-04T01:10:59Z

Part of #265

The functionality is all implemented in the first commit, the other two are cleanup for it's surroundings.

Some things to note:

It's possible that the user_can_be_notified method will need to be abstracted and put into the EF_Module parent class for part 2 of the above issue, but that can be done if/when necessary.

And then it seems calling both edit_post or edit_page conditionally would be redundant according to the core method: https://github.com/WordPress/WordPress/blob/master/wp-includes/capabilities.php#L127-L128. Would be nice to have a second pair of eyes to confirm this, but my testing showed that this was indeed the case. Some more info on what I found here #265 (comment)

Steps for testing

Add multiple authors and editors to a user group.
Using the current master branch, select this user group to be notified on a draft post.
Post an editorial comment.
Look at the email logs or just error_log the $recipients variable at the end of _get_notification_recipients.
Note that all authors/editors that were signed up received this email.
Switch to this branch and post another editorial comment.
Note that only editors were sent the email, along with the post author if they were selected.

Can repeat and test with individual user notifications as well instead or alongside groups.

You shouldn’t receive a notification email unless you are able to view/edit the post.

$authors is never used, so declaring it then merging in an empty array isn’t needed.

General cleanup for the _get_notification_recipients method to align with WP coding standards. Mostly all spaces, braces, and periods. The one outlier is the removal of the first declaration of $recipients which is not needed.

sboisvert

A few questions :)

sboisvert · 2018-05-01T18:30:03Z

 		$post_id = $post->ID;
-		if( !$post_id ) return;
-
-		$authors = array();


Why are we removing these from here?

$authors was never used. Was just set to an empty array, and then array_merge'd later on. The missing $post_id conditional is still present, just unfolded per WP coding standards.

sboisvert · 2018-05-01T18:30:20Z

-
+		}
+
 		$usergroup_users = array();


Maybe we can bump this up to the top of the function like the others?

Move $usergroup_users = array(); to the top? I removed all of the top empty variable declarations. One is longer needed, and the others are declared in a more localized place.

sboisvert · 2018-05-01T18:37:37Z

 					$usergroup_user = get_user_by( 'id', $user_id );
-					if ( $usergroup_user && is_user_member_of_blog( $user_id ) )
+					if ( $this->user_can_be_notified( $usergroup_user, $post_id ) ) {
 						$usergroup_users[] = $usergroup_user->user_email;


What's the difference between $usergroup_users and $usergroup->user_ids ?
Perhaps we could rename $usergroup_users to something that's clearer, $notified_group_users, $usergroup_to_notify or something along those lines? (This might just be personal preference on my part :) )

looking at the code below, $usergroup_recipients seems like it could work.

$usergroup_users is an array of all emails belonging to users that are connected to a group that is following a post. $usergroup->user_ids are the user IDs belonging to a specific user group. There can be multiple user groups connected to a post.

Could definitely use some more specific naming, will make some readability changes.

sboisvert · 2018-05-01T18:40:26Z

+		// Process the recipients for this email to be sent.
+		foreach(  $recipients as $key => $user_email ) {
+			// Get rid of empty email entries.
+			if ( empty( $recipients[ $key ] ) ) {


When / Why would this happen?

Hmm, good question. This check was legacy code that I left there, but re-reading looks like a possible typo. Should be checking if the email is empty, because it is technically possible to have a user without an email address.

Instead of the conditional, could just wrap array_filter() around the array manipulations above which will simply strip away any empty values when used without a callback function.

When array_filter is used without a callback, it will remove any array items that evalulate to false, namely empty strings and NULLs.

david-binda

In general the code looks good to me, good work on cleaning it up!

I just have flagged some further possible improvements inline. Feel free to hit me with any questions!

david-binda · 2018-05-02T15:46:59Z

+			$can_be_notified = $user->has_cap( 'edit_post', $post_id );
+		}
+
+		return apply_filters( 'ef_notification_user_can_be_notified', $can_be_notified, $user, $post_id );


There is a catch with the $user as it might be boolean false under certain circumstances ( get_user_by call in _get_notification_recipients might return false ). Thus, I would propose to document the filter via PHPDoc notifying the users about that possible issue.

david-binda · 2018-05-02T15:51:27Z

+			$can_be_notified = $user->has_cap( 'edit_post', $post_id );
+		}
+
+		return apply_filters( 'ef_notification_user_can_be_notified', $can_be_notified, $user, $post_id );


Shall we cast the filtered value to bool in order to make sure that boolean is always returned from this method? Per PHPDoc comment?

david-binda · 2018-05-02T15:57:01Z

-				unset( $recipients[$key] );
+
+		$user_recipients = $this->get_following_users( $post_id, 'user_email' );
+		foreach( (array) $user_recipients as $key => $user ) {


Is the typecasting to array really necessary here? Looks like https://github.com/Automattic/Edit-Flow/blob/6fc7cb193c74a48581bdb0c5287ed35c7acbe763/modules/notifications/notifications.php#L956,L1000 is always returning an array.

It might be worth digging into why it has been added to the original code in the first place. Or whether it was in there from the very beginning.

I would be adding it in this PR, so you are right - it's not needed.

sboisvert · 2018-05-03T22:13:04Z

This Looks good. I don't think we want to merge it into master yet but it's ready to go into a feature branch.

WPprodigy · 2018-05-03T22:37:37Z

Agreed. I personally think #451 needs to be ready before this goes into a release.

As without that piece, an editor could be mislead into thinking certain people are getting notifications when they are not.

rinatkhaziev · 2018-05-03T22:47:28Z

@sboisvert do we want this to go into 0.8.3 or 0.9?

rinatkhaziev · 2018-05-03T23:00:01Z

Per Slack - marking this for 0.9

user_can_be_notified was already defined in `451-notification-visibility` branch PR Automattic#449

jerclarke · 2018-11-09T22:16:44Z

Tested this and it seems to work, aside from conflicts with the other patches as noted in the referenced links above.

Would love if we could pick an official branch for all these changes and start merging them in. They should all be launched together in 0.9 and they have a lot of interactions that need to be addressed in a merged codebase (since they tend to interact with the same few files).

rinatkhaziev · 2018-12-19T04:16:58Z

@WPprodigy thanks for the work, @jerclarke thanks for the testing. Merging that in to go 0.9 :)

WPprodigy added 3 commits April 3, 2018 19:24

Implement caps check before sending notifications

d7edaa9

You shouldn’t receive a notification email unless you are able to view/edit the post.

Remove unused and empty author array

51ca666

$authors is never used, so declaring it then merging in an empty array isn’t needed.

Coding standards and cleanup

8b9c2a9

General cleanup for the _get_notification_recipients method to align with WP coding standards. Mostly all spaces, braces, and periods. The one outlier is the removal of the first declaration of $recipients which is not needed.

This was referenced Apr 5, 2018

Make it clear which users can/can't receive post notifications #451

Closed

Shouldn't be able to subscribe someone to notifications if they can't edit post. #265

Closed

sboisvert reviewed May 1, 2018

View reviewed changes

WPprodigy added 2 commits May 1, 2018 17:35

Remove empty email addresses from recipients list

116ca33

When array_filter is used without a callback, it will remove any array items that evalulate to false, namely empty strings and NULLs.

Rename variables for clarity

98c3461

david-binda reviewed May 2, 2018

View reviewed changes

WPprodigy added 2 commits May 2, 2018 22:48

Return array or string always

5080c0e

Add doc blocks for filters, and some typecasting

5ebc65f

rinatkhaziev added this to the 0.9 milestone May 3, 2018

jerclarke mentioned this pull request Nov 8, 2018

Add a "No Access" badge to subscribers that will not be able to receive notifications #461

Merged

jerclarke added a commit to jerclarke/Edit-Flow that referenced this pull request Nov 8, 2018

notifications.php - remove duplicate copy of user_can_be_notified

77b70dd

user_can_be_notified was already defined in `451-notification-visibility` branch PR Automattic#449

jerclarke mentioned this pull request Nov 9, 2018

Editorial comments "Notified" list should only include people who get email (not current_user) #483

Closed

rinatkhaziev merged commit bd49802 into master Dec 19, 2018

GaryJones deleted the fix/265-notification-validation branch December 9, 2019 15:01

Conversation

WPprodigy commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Some things to note:

Steps for testing

Uh oh!

sboisvert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WPprodigy May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WPprodigy May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-binda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sboisvert commented May 3, 2018

Uh oh!

WPprodigy commented May 3, 2018

Uh oh!

rinatkhaziev commented May 3, 2018

Uh oh!

rinatkhaziev commented May 3, 2018

Uh oh!

jerclarke commented Nov 9, 2018

Uh oh!

rinatkhaziev commented Dec 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WPprodigy commented Apr 4, 2018 •

edited

Loading

WPprodigy May 1, 2018 •

edited

Loading

WPprodigy May 1, 2018 •

edited

Loading