Display who was notified for an editorial comment

[WPprodigy]

Part of #270. This PR started in #271, and continued with some edits here.

The functionality is the same as described in 271 above, with a few smaller changes:

• Only save comment meta if needed (don't save empty messages).
• Localize all text strings.
• Add filter so this feature can be disabled.

To test:

1. Enable notifications & editorial comments features.
2. Add people and/or groups to be notified.
3. Leave editorial comments.

Screenshots can be found in #271, as there weren't many changes to the display (aside from colors and text aligning).

[sboisvert]

The code looks good to me. I think we need to have a few folks test it to see if we have any problems before merging it. We'll probably want to push this to a minor release focused on features. cc @rinatkhaziev and @philipjohn for their thoughts

[sboisvert]

@jerclarke If you get a chance to test this on Global Voices and have feedback on it that'd be greatly appreciated as well.

[rinatkhaziev]

LGTM, merging into 0.8.3

[rinatkhaziev]

I have accidentally merged this PR, even though I meant this to go into 0.9. I have reverted that merge, but this PR now shows as merged.

[rinatkhaziev]

Ok, no way back then, this goes into 0.8.3 :)

[sboisvert]

https://youtu.be/hooKVstzbz0?t=84

[jerclarke]

FYI: Just tested this and it's working as expected. Very nice! My users will find this extremely useful.

I'm inclined to try and make the display a little jazzier, especially on the hover text after submitting, but what's there now is simple and wonderful. I'm in no rush to mess with it :)

I'm realizing that I should use the same output as the post-submit text from this ticket as the generator for #478 will be doing that now

[jerclarke]

For reference: I used the comment meta field to solve #478, so these should now be considered inextricably linked.

[jerclarke]

Update: Found a big problem that I didn't think to test previously

All previous comments left before this update show the "No users or groups were notified." message

Is this intentional? Obviously it's not true, and could be very misleading. IMHO better to show nothing, or else a message about how it's unknown.

The problem is that how the comment meta is set up now, there's no way to distinguish between a comment where this is true (a user submits a comment to a post where they are the only subscriber) and one where we can't know (it was before the comment meta was added).

From editorial-comments.php -> maybe_output_comment_meta():

$notification = get_comment_meta( $comment_id, 'notification_list', true );

if ( empty( $notification ) ) {
	$message = esc_html__( 'No users or groups were notified.', 'edit-flow' );
} else {
	$message = '<strong>'. esc_html__( 'Notified', 'edit-flow' ) . ':</strong> ' . esc_html( $notification );
}

Can anyone think of a good way around this?

Always save comment meta, even if it's empty

One would be to always save the comment meta, even if it's empty, and then only show the "no users or groups" message if there's an empty comment meta. This has the obvious downside of many empty rows in the database, but the upside that we do end up with reliable data. The fact that no one was notified is itself a piece of data, it's absence has a different meaning from being able to assert that no one was notified.

Only show a message when someone was notified and we recorded it

Another solution would be to remove the "No users or groups were notified" message entirely, and have nothing when there's no comment meta.

The downside of that is the use case where someone has sent a comment only to themselves, and thus there's no message, and they might be confused that hovering has no effect.

Sidenote: Copying the string directly from the JS display before sending comment might have side effects

In tracking down where exactly the comment meta string gets generated, I realized it's a bit brittle.

When sending the data along with the comment to admin ajax, the 'notified' string is this:

post.notification = jQuery('#ef-reply-notifier').val();

So it's just copying it right out of the js output. This doesn't necessarily cause any specific bugs, but it could have some potential strange effects when filtering etc. is taken into account. I mention it just for future reference.