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

[jerclarke]

Correct me if I'm wrong, but after some testing it seems that you are able to subscribe a user to a post in the Notifications box even if they don't have edit_others_posts and thus won't be able to "edit" the post and see Editorial Comments etc.

My testing implies that this is at least partially corrected by just not sending notifications to users who wouldn't be able to edit. Is that right?

Either way it seems like a terrible idea to let people subscribe users who won't be able to log in and won't get notified. It's bound to cause confusion even among users that understand what's going on, but more than that it assumes that every user not only understands the roles+capabilities system in WP (i.e. that some users can/can't edit_others_posts, which takes our editors a while to grasp) but also knows what role each other user has (and thus whether they can successfully subscribe someone).

If I'm not wrong above I think it would be much better if Edit Flow protected users from making the mistake of subscribing someone who won't get notified.

One way would be to allow them to do so, but with an obvious warning that the user won't get any emails about the post because they can't edit.

The other way would be to allow the user to be found while searching (to avoid confusion if they are missing) but de-activate the user from being subscribed with some kind of warning/label expressing the fact that they are inactive because they wouldn't have permission to edit the post.

Hope that makes sense! Thanks for considering it :)

[jerclarke]

Adding a quick note that I seem to have found a related bug: If such an author with no edit_others_posts capability is in a GROUP that is subscribed to a post, they DO get the email, but still can't log in.

Very confusing, and seems to imply an unstated rule like Never add users to groups unless they have edit_others_posts capability, which, if it is effectively a constraint should be coded into the system with warnings etc. (and is also a terrible idea generally, since users can change roles and capabilities while they are members of a group, resulting in unpredictable outcomes).

[sboisvert]

@WPprodigy I think we can start working on the notification part that's described in #273 and ties into this.

Adding a quick note that I seem to have found a related bug: If such an author with no edit_others_posts capability is in a GROUP that is subscribed to a post, they DO get the email, but still can't log in.

Seems like pretty bad UX :(

Very confusing, and seems to imply an unstated rule like Never add users to groups unless they have edit_others_posts capability, which, if it is effectively a constraint should be coded into the system with warnings etc.

Seems like a good start, at least until we look into / fix #273

[WPprodigy]

Restricting only those with edit_others_posts caps to be added to groups could break some workflows I would imagine. I'm thinking it would be possible to have groups made up with authors in mind, perhaps with sites that have multi-authorship functionality as well.

Emailing those that cannot view/edit the post is definitely a problem though and I believe I found a good solution but wanted to get some feedback / thoughts first.

1) Notification Validation.

Before notifications are sent out, do a last second check to make sure the user will be able to view/edit the post before sending them the email. It's fairly straightforward and performant to do this for individual users and each user in a group - so nobody will improperly receive these emails and the above bug is resolved.

It's also better to do the edit_post validation this way rather than relying on settings, as this will also help solve edge cases with a user role/authorship being changed but notification settings and/or groupings were not changed, etc.

2) Notification Visibility.

To better show this behavior, and perhaps prepare a bit for the possible feature in the other ticket, it would be good to make it clear to the end user what is happening.

Three main changes here:

1. Users without edit access will have the checkbox disabled and some help text shown beside.
2. Groups that contain some users without edit access will still be selectable, but a notice will be shown to clarify not all users in the group will receive the notification.
3. Added a sentence at the end to the meta box description for some extra clarity on this behavior.

The help text next to the checkboxes doesn't seem ideal from a design standpoint, so definitely open to some ideas around that. I'm thinking it could perhaps show up once the user tries to select the item, or maybe with a tooltip.

And then I'm not sure if the performance hit would be worth it for the groups side help text, as right now the users in each group are never looped over at this point. So it could be better to rely on the top meta box description for now, and perhaps implement the groups side if a button is desired later on with the possible feature to "permit all users in group to edit".

Lastly, do you think it makes sense to show all authors on the left side of the metabox at this point? Since pretty much none of them will be selectable, it could make sense to just leave them out of the list entirely. Though the counter argument would be that it could cause confusion as to why they are missing, and the other (possible) feature would need to add them back anyway.

[WPprodigy]

Also wanted to mention that this sets up the other possible feature quite nicely :), as a good part of the needed functionality would already be implemented if the user_has_cap filter were to be used to conditionally grant some users edit access. Otherwise it would still just be a matter of editing the permissions method that is used to check if a user can receive notifications.

[jerclarke]

@WPprodigy Your solution is precisely what I was hoping for FWIW, thanks for the mockup.

Agree that blanking them out but still showing all users is the right way to go. Having them all there is important so that any user can see any other user and understand why they can/can't add them. Also the search tool is right there, which IMHO is the way to go any time the list is longer than the box.

FWIW our site has thousands of users that show in that box, so whatever the solution is, it can't be something that breaks when there's lots of users in the box. I feel like doing a current_user_can for everyone on the site might not work as a result. This might imply the best solution is one that shows an error after ticking a user (in the AJAX result), rather than in the initial list. Not a great UX compared to what you've mocked up, but still a huge improvement over what's there currently.

Also wanted to mention that this sets up the other possible feature quite nicely :), as a good part of the needed functionality would already be implemented if the user_has_cap filter were to be used to conditionally grant some users edit access.

In terms of setting this up to help with #273 I guess the outcome would be that if you had the needed cap (grant_edit_post or something like that, which I could then filter to be a clone of edit_users to match my request in #273) then the AJAX would succeed and you'd see a message saying they were granted access, which is different from the default message. Since the default message is just the checkmark, maybe it could be a little tag saying "[ACCESS GRANTED]" or something. Not sure what the best UX for that is, but making it clear would be useful.

[jerclarke]

In terms of the groups system, I'm not really inclined towards the kind of labeling you mention. Would rather this distinction be in the groups admin screen, so if you look at the list of people in the group, you see warnings on anyone without edit_others_posts

Also worth considering how this interacts with pages, and make sure everything works on all post types.

[sboisvert]

Thanks for the feedback @jerclarke!

FWIW our site has thousands of users that show in that box, so whatever the solution is, it can't be something that breaks when there's lots of users in the box

This is an interesting use case. I feel like showing all users in that drop down could get messy and would hit a point where it wouldn't scale well (Either PHP runs out of memory from getting 100k authors or the browser has problems with it). I was wondering if people do scroll thru the thousand users.
Do they use the search feature?
I could see a hybrid model where we show all authors if the count is less then X where X is filterable with a default of let's say 20 [totally arbitrary number] and then if you want more you need to search. Maybe in the UI we'd put a note at the bottom of the list that mentions that you need to search to get more results. This should allow us to have the inline disabled button. That being said, I might be overthinking it and Ajax is the way to go.

In terms of the groups system, I'm not really inclined towards the kind of labeling you mention. Would rather this distinction be in the groups admin screen, so if you look at the list of people in the group, you see warnings on anyone without edit_others_posts

Would also scale much better.

@WPprodigy I really like what you've done with the mockup. I think we should give it a shot. Maybe let's implement the first step 'Notification Validation' independently of the second step to keep things a bit cleaner.

I like @jerclarke's suggestion of being able to then piggy back on the ajax check to add the functionality to add folks to get access. So maybe that answers my previous comments on top there about searching and limiting the list.

[sboisvert]

@WPprodigy When we knock out the first part let's scope out part 2 again based on the feedback. I'm also going to ask @mattoperry @philipjohn for input

[jerclarke]

This is an interesting use case. I feel like showing all users in that drop down could get messy and would hit a point where it wouldn't scale well (Either PHP runs out of memory from getting 100k authors or the browser has problems with it).

In terms of just listing all the users, this already comes up as part of the default Author metabox, which just lists everyone all the time and doesn't even have a search feature. On GV the de facto answer is "use the browser search" either by actually CMD|CTRL+F searching or by clicking the Author* select then starting to type their display name.

Either way, PHP fetches every single one and loops through them just like in the EF Notifications box. The EF box does have more formatting obviously, but it's fairly comparable IMHO. GV is already pushing it with thousands of authors in a single site and AFAICT it isn't a huge problem at least with our current highly-optimized php7+nginx+http2 etc. setup.

I was wondering if people do scroll thru the thousand users. Do they use the search feature?

On GV people definitely do, or at least I do!

[WPprodigy]

Sounds good, I'll push out the 'Notification Validation' part shortly and can then look a bit more into stage two of the above.

Also worth considering how this interacts with pages, and make sure everything works on all post types.

Looked into this a bit, and by default the author role isn't able to edit pages no matter what (even if they are set as the author). So perhaps not much to do in this area. Can still pass through the permissions check though in case this were to be customized.

https://github.com/WordPress/WordPress/blob/master/wp-includes/capabilities.php#L174

More info:

You can see above where core's map_meta_cap ends up if you do $author_user->has_cap( 'edit_page', $page_id ); on a draft page in which $author_user is the page author. This maps it over to the edit_pages capability, otherwise if published, it is then mapped to edit_published_pages. So seems there is not much to do in this area as either way a typical author role will not have permissions for this.

Also interesting is that it looks like edit_page is redundant since you can just call edit_post to cover both bases of posts and pages.

I feel like showing all users in that drop down could get messy and would hit a point where it wouldn't scale well

Perhaps out of scope for this issue (maybe a new ticket would be good for user listing performance/improvements), but ajax loading the user list after a certain point definitely seems like a good performance gain (even though WP would still be doing the same thing). Users that are currently subscribed/checked should also always be shown in this scenario and at the top of the list.