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

[jerclarke]

This is a follow-up for #270 (show who is getting notified of comments) which is largely complete and the initial version has been merged to master, so I'm making a new ticket. This issue also directly affects #478 (show the notified list in the email notifications too) since that code uses the same list of users that gets saved into the notification_list comment meta based on the JS-driven metabox UI.

MAIN SYMPTOM: When generating the list labeled "The following will be notified" in the Editorial Comments UI, the person editing the post is included in the list, even though, based on all the logic in this plugin, they are of course not notified.

I noticed this while testing #449 ("Check if a user can edit the post/page before sending a notification.") because I was comparing the list of people in the UI and the total emails sent, and of course, my name was listed in the UI but I wasn't sent an email.

WIDER ISSUE: The list has everyone subscribed, rather than only those who are sent an email.

I didn't notice this problem with the "current user" being listed until now (and presumably others didn't) because it is very natural to see your own user account listed in the UI. Of course as someone editing the post you are "subscribed" in Notifications, and in most ways the list displayed as "The following will be notified" is just a list of all subscribers.

But that's not what it should be. It should be a list of only those people who will, in fact, be notified. And when it's displayed later -- when hovering on the comment and in the email notification -- it should really reflect exactly who got a notification (i.e. which emails were sent out).

The use case I've already brought up, of the "current_user" being listed, is pretty low-priority, since it's unlikely to cause harm and, obviously, whoever wrote the comment "notified themselves" as they wrote and submitted the comment.

The important part is the "subscribed" people who don't get notified for various reasons.

This gets a lot more important when you take into account the conjunction of #265 (original parent ticket) #451 (@WPprodigy 's more specific ticket) and #461 (@WPprodigy 's PR for #451). All three of these relate to how we handle the subscription of users who aren't able to edit a post because they don't have the edit_others_posts capability (i.e. "NO ACCESS").

Essentially, we need to take those people into account in the following ways (this is the stuff in the other tickets):

• Warning users when they subscribe someone with NO ACCESS
• Not sending the notification to someone with NO ACCESS
• A system for granting access to this specific post to users who otherwise have NO ACCESS

Considering all that, I think we can see how there is a very real difference between "Someone subscribed in the notifications box" and "Someone who will be notified".

I propose that on principle, we need to ensure that the "The following will be notified" UI (and it's follow-up uses like the email) should be very precise, and only list those who will be sent an email.

Theoretically, the list should be generated during the actual email sending, in which case it could truly know who was emailed, but this is totally inconvenient, and I think keeping the current JS-based approach makes the most sense.

In this case our mission now is to udpate the JS/HTML of the Notifications box and the Editorial Comments display of who will be notified to take into account the various cases where someone is subscribed but not notified (then to keep this in mind in the future, and ensure the list remains accurate).

---

So does this all make sense @WPprodigy and @sboisvert ?

First step: Remove current user

I think the first step will be to remove the current user from the recipient list. Here's my proposed process that (I'll submit as a patch if you like it):

• In the notifications metabox, add a class to the <li> for the current user: ef-post_following_list-current_user
• In editorial-comments.js -> notifiedMessage check for that class, and don't add the username to the list if they have it

Second step: Remove users with the "[NO ACCESS]" warning

This one impacts the code in #461 which adds a [NO ACCESS] warning on any user who won't be able to log in and edit the post. Such users also won't get an email once the code from #449 is in place.

Since they won't be notified, we should make sure they aren't in the "The Following will be notified" list"

• Make sure there's a clear class labeling users with "[NO ACCESS]" (currently there isn't): ef-post_following_list-no_access
• In editorial-comments.js -> notifiedMessage check for that class, and don't add the username to the list if they have it

Third step: Remove anyone with no email

This is a less common one, but it came up on my test site and it begs for a fix that won't be much extra work. I have an auto-created user from an import (test site after all) that has no email, and thus won't get notified and thus shouldn't be on the list.

Weird situation, but one we should handle IMHO.

• When outputting users in the notifications box, check their email, and if it's not there, add a class to the <li> like the ones described above: ef-post_following_list-no_email
• Similar to the [NO ACCESS] message described above, we should have one for [NO EMAIL] for such users that makes it clear. I think this has it's own inherent value! Great for debugging such users in general!
• As above, update notifiedMessage to ensure that the ef-post_following_list-no_email users don't get marked as being notified

Note on email address visibility: Currently all users listed in this box have their emails visible, which is no bueno. In WP, email addresses of other users are not "known information" by other "authors" (anyone without edit_users), but EF totally leaks everyones email to everyone else via the Notifications metabox. There's already ticket #264 for this, but not patch yet (I solved it with CSS, which sucks).

Either way, the point is that IMHO for future-proofing, we should assume that email addresses are NOT visible in that box, which makes a [NO EMAIL] label much more important. If we could always assume that emails are visible, then the lack of visible email on a user is a subtle form of a [NO EMAIL] warning

---

Hope that all makes sense, and sorry for not noticing this and proposing a fix months ago (before it got committed to master). Tracking down the interesections of all these changes is a headache!

[jerclarke]

@rinatkhaziev Hi, could you take a look at this one and let me know if it is coherent and acceptable as a plan? IMHO it belongs with the initial implementation of the "who will be/was notified" feature.

[jerclarke]

@sboisvert @rinatkhaziev Just checking in on whether this makes sense to you.

[rinatkhaziev]

@jerclarke Thanks for the detailed follow-up.

TBH I'm not a huge fan of how notifications/comments work now. Storing data in DOM like it is right now is brittle (btw, thanks for that commit 4cf139b).

I just pushed a commit that hides the current user from the list of people to be notified,

it should really reflect exactly who got a notification (i.e. which emails were sent out).

A successful email send doesn't mean it'll reach the recipient. I'm not saying it's not possible, but it seems to me we'd have to introduce more complexity for very little benefit.

As for No Access badge, wouldn't it make more sense, to just not show any users who won't be able to edit the post in the notification list?

[jerclarke]

https://trello.com/c/msBDJ4rf/469-geo-mashup-update-and-get-api-message-to-go-away

Sorry that you're in the position of taking this over mid-development, but that's a big question with a lot of text in these tickets about it.

Changing the list of people to only show those who can access is a big, complicated change, that affects a lot of things, unlike what I'm proposing here, which just addresses a specific problem with the features that are already there.

One thing is that it's hard to check who will be able to edit the post for every user, on a site with thousands of users (like ours) it could be a crashing level of analysis to do either in PHP or in MySQL.

Another is that even though there is currently no built-in way to "grant users access to a post" when they don't have edit_others_posts, there are already plugin-based ways to do so (and in fact I've got that working on our site, and it's mission-critical for us). Not showing users who can be granted access would totally break my filter-based solution to this, AND it would get in the way of a future plan to add the feature into core in a format where someone editing a post can consciously grant access to the post to a specific user.

See #273 which is about this.

Also #265 which is about how groups interact with this

---

So all that said, I think I stand by my original proposal in this ticket to fix the current system, even though the fix is brittle, just like the current code that controls the "notified list" is brittle.

Adding a warning for "no email" and backing up the "NO ACCESS" flag by not showing those users in the "notified list" is a direct improvement of the current system, and we can always review them later when/if the other related features come into existence.

[rinatkhaziev]

@jerclarke Ok I see your point.

First step
Fixed in 76adf5e

Second step: Remove users with the "[NO ACCESS]" warning
Both #461 and #449 are merged.

Third step: Remove anyone with no email

Do you want to take a swing at this? Personally, I'd probably still skip users with not email in PHP around:

Edit-Flow/common/php/class-module.php

Lines 472 to 487 in 76adf5e

	<?php foreach( $users as $user ) : ?>
	<?php $checked = ( in_array($user->ID, $selected) ) ? 'checked="checked"' : ''; ?>
	<li>
	<label for="<?php echo esc_attr( $input_id .'-'. $user->ID ) ?>">
	<div class="ef-user-subscribe-actions">
	<?php do_action( 'ef_user_subscribe_actions', $user->ID, $checked ) ?>
	<input type="checkbox" id="<?php echo esc_attr( $input_id .'-'. $user->ID ) ?>" name="<?php echo esc_attr( $input_id ) ?>[]" value="<?php echo esc_attr( $user->ID ); ?>" <?php echo $checked; ?> />
	</div>
	<span class="ef-user_displayname"><?php echo esc_html( $user->display_name ); ?></span>
	<span class="ef-user_useremail"><?php echo esc_html( $user->user_email ); ?></span>
	</label>
	</li>
	<?php endforeach; ?>
	</ul>
	<?php endif; ?>

As for leaking emails, it's a good idea to fix that. Maybe obfuscate them a bit?
E.g. instead of [email protected] com display r***t@*****le.com. Should be an easy change.

I definitely want to release 0.9 during the next week (Jan 7 to Jan 11, 2019).

[rinatkhaziev]

@jerclarke FYI we're planning to release 0.9 this Thursday, if you want the third step to get merged in 0.9 now's your chance :) If you don't have time, that's ok, we'll ship that functionality in the next patch release.

[WPprodigy]

Just wanted to mention that user's with no email address are technically filtered out here: https://github.com/Automattic/Edit-Flow/blob/master/modules/notifications/notifications.php#L844. So while they will appear "checked", it won't try to send an email.

[jerclarke]

Hey sorry @rinatkhaziev I haven't had time yet this week. I will try to do it tomorrow morning ASAP.

@WPprodigy Good to know that it's filtered out. Having them not be listed too just makes sense.

[jerclarke]

Okay, testing current master I discovered a major issue with how you hid the current user: 76adf5e#commitcomment-31885120

Current master breaks the ability for people to untag themselves from notifications on a post, which is worse than anything we're talking about in this ticket. If nothing else 76adf5e needs to be reverted.

Edit: Sorry, 76adf5e shouldn't be reverted, because it also has the fixes for groups (would be better to separate commits like that in the future). Instead we need a new commit that disables the ef_users_select_form_get_users_args filter (and probably removes ef_users_select_form_get_users_args method entirely)

[jerclarke]

I think the approach for the Notifications metabox should be:

All users are displayed

• For backwards compatibility/consistency with how it has worked
• For forwards/plugin compatibility with tools that enable us to grant access to users who might not otherwise have it

Users that are non-functional are labeled as such

• So you can always find the user you're thinking of (rather than being confused they are missing) but see that there is a problem
• This warning should be filterable/disable-able by plugins, e.g. if the plugin auto-grants permission to tagged users the way my setup works.

Users that are non-functional can still be tagged

• Matches existing behavior and thus is backward-forward compatible
• Simplifies the potential workflows and makes sense in case e.g. they are promoted to a new user role after tagging

Users that are non-functional do not show in "Notified List"

• Thus the "Notified List" serves as a second warning that such users aren't going to be sent email.

[rinatkhaziev]

@jerclarke I reverted the query part in 04f2ddf

[jerclarke]

Great. I've got code ready to create a PR that will show you how I handle the "No Access" use case as a start. Will send soon then work on a proposal for how to do similar for current-user and no-email

[jerclarke]

There you go. I think that's the cleanest solution that doesn't change much and solves that particular problem.

Please take a look and let me know if there's something wrong with it.

I think to make the other two work, it will likely be better to generalize the system a bit, maybe rename some of the *_no_access parts to be *_errors or something like that.

If nothing else, the part where we check the class while generating the "notified list" can easily be updated to just check for several different classes, so as long as they are all simple span.XYZ that part is easy. It's getting the various spans added there in a clean and sensible way that I need to figure out.

[jerclarke]

Just updated my feature branch:

Second commit generalizes the warning badge system and adds support for "No Email" badge.

• Evaluates users for having an email or not during AJAX
• Returns a list of users with no email
• Checks that list when generating display
• Checks for the email class before adding subscribers to notified list

[jerclarke]

Alright all done. Sorry it's coming in late but I think in the end #492 solves all the problems discussed in a pretty simple and effective way:

• Users with no email are marked with a span, and never added to the Notification List
• Users who won't haver permission to edit are marked with a span, and never added to Notification List
• The current user's checkbox is marked with a class, and won't get added to Notification List

[rinatkhaziev]

Thanks, @jerclarke, I'll test it out and merge tonight.

[jerclarke]

Merged! Thanks! I'll close this and we can make new tickets if there's bugs or improvements for whats there.

Yatta 🙌🏻