Make WordPress Core

Opened 9 years ago

Last modified 4 years ago

#33717 assigned enhancement

Send Notification Email When a Comment is Approved From Moderation

Reported by: jeffr0's profile jeffr0 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Comments Keywords: needs-dev-note has-patch
Focuses: Cc:

Description

Currently in WordPress, commenters have no idea their comment is approved unless they visit the page often. When a comment is held for moderation, WordPress should send the commenter an email notification when their comment is approved. I'm using the Comment Approved plugin to add this functionality to WordPress but I really think it should be a core feature.

Attachments (19)

33717.diff (4.4 KB) - added by swissspidy 9 years ago.
33717.2.diff (4.1 KB) - added by swissspidy 9 years ago.
33717.3.diff (4.5 KB) - added by mrahmadawais 9 years ago.
33717.4.diff (4.3 KB) - added by swissspidy 9 years ago.
33717.5.diff (4.2 KB) - added by wonderboymusic 9 years ago.
33717.6.diff (4.3 KB) - added by swissspidy 9 years ago.
33717.7.diff (4.7 KB) - added by swissspidy 9 years ago.
33717.8.diff (4.8 KB) - added by swissspidy 9 years ago.
33717.9.diff (6.3 KB) - added by swissspidy 9 years ago.
33717.10.diff (8.7 KB) - added by swissspidy 9 years ago.
33717.11.diff (17.5 KB) - added by imath 5 years ago.
33717.12.diff (16.4 KB) - added by imath 5 years ago.
337171.14.patch (16.0 KB) - added by imath 4 years ago.
Screenshot 2021-01-10 at 18.09.23.png (108.9 KB) - added by johnbillion 4 years ago.
Screenshot
33717.15.patch (19.9 KB) - added by imath 4 years ago.
33717.13.diff (18.0 KB) - added by johnbillion 4 years ago.
33717.14.diff (13.2 KB) - added by johnbillion 4 years ago.
33717.15.diff (14.7 KB) - added by garrett-eclipse 4 years ago.
Minor CS fix after reviewing @johnbillion's .14 patch. Just indents the contents of the else from lines 41-71 of wp-comments-post.php
33717.16.diff (15.5 KB) - added by johnbillion 4 years ago.

Download all attachments as: .zip

Change History (107)

#1 @rachelbaker
9 years ago

  • Version 4.3 deleted

#2 @knutsp
9 years ago

  • Keywords needs-patch added

Yes!

I don't know how many times the last ten years I have left a comment on a WordPress site and got the message that my comment is held for moderation. Then I leave the site, never knowing my comments would ever be approved.

Last edited 9 years ago by knutsp (previous) (diff)

#3 @atomicjack
9 years ago

+1 this is definitely needed.

#4 @danielbachhuber
9 years ago

This would basically be the best feature in 4.4

#5 @michaelbeil
9 years ago

I would really appreciate being notified of my approved commentary. Seems like a fantastic addition to native comments.

#6 @boonebgorges
9 years ago

  • Keywords good-first-bug added

#7 @rachelbaker
9 years ago

  • Milestone changed from Awaiting Review to Future Release

Sounds like there is plenty of interest here. Anyone interested in submitting a patch to make @jeffr0's dreams come true?

Last edited 9 years ago by rachelbaker (previous) (diff)

#8 @jeffr0
9 years ago

Here's another reason why it should be a core feature. The plugin I use to add this feature to WordPress doesn't work because the Tavern uses Jetpack Comments and not the native comment form. Sigh. I hope that by adding it to core, Jetpack comments wouldn't need to be involved.

http://wptavern.com/help-me-add-comment-approval-notifications-to-wordpress#comment-72576

#9 follow-up: @jeffr0
9 years ago

What are your thoughts on whether a commenter should have to opt-in to receive the notification instead of automatically sending it?

#10 follow-ups: @krogsgard
9 years ago

Current notification functions are pluggable, and that's (I hear) not a go anymore. So if you keep the same naming convention, you'd probably utilize wp_notify_commentauthor inside of wp-includes/comment-functions.php that would operate similarly to wp_notify_postauthor.

wp_notify_postauthor is triggered from wp_set_comment_status for newly approved comments. You could call wp_notify_commentauthor in the same way.

Unfortunately that method would likely add a lot of duplicate code from the other comment notification emails. It'd be nice to have a more generic notification function.

#11 in reply to: ↑ 10 @kasparsd
9 years ago

Replying to krogsgard:

wp_notify_postauthor is triggered from wp_set_comment_status for newly approved comments. You could call wp_notify_commentauthor in the same way.

There is also the transition_comment_status action.

I think the "Comment Approved" plugin is a good of example of how it could be achieved.

#12 in reply to: ↑ 10 @kraftbj
9 years ago

Replying to krogsgard:

Unfortunately that method would likely add a lot of duplicate code from the other comment notification emails. It'd be nice to have a more generic notification function.

Yeah, we could split out a wp_notify for all of the functional bits, then use wp_notify_postauthor, wp_notify_moderator, wp_notify_commentauthor (for example) as a wrapper of that function.

Replying to jeffr0:

What are your thoughts on whether a commenter should have to opt-in to receive the notification instead of automatically sending it?

Personally, automatic that can be disabled with a filter. When enabled, the "Your comment awaits moderation." changes to "Your comment awaits moderation. You will be notified if it is approved."

Personally, I'm of the mindset that we should not (at least by default) notify comment authors that are deleted/spammed, etc.

#13 @jeffr0
9 years ago

<blockquote>Personally, I'm of the mindset that we should not (at least by default) notify comment authors that are deleted/spammed, etc.</blockquote>

I agree. The notifications should be strictly limited to pending comments. If they are sent to spam or trashed, no notification is sent. It wouldn't make sense.

#14 @ubernaut
9 years ago

how would notifications work with twitter/fb commenters?

#15 @idogenealogy
9 years ago

That would be a great feature to add to core.

#16 @mrahmadawais
9 years ago

This seems to be a good feature. +1 @jeffr0.

I think we can hook it inside wp_transition_comment_status

#17 @swissspidy
9 years ago

  • Owner set to swissspidy
  • Status changed from new to assigned

@swissspidy
9 years ago

@swissspidy
9 years ago

#18 follow-up: @swissspidy
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

What 33717.2.diff does:

  • Adds a new function called wp_notify_commenter which is hooked to transition_comment_status
  • Includes two new filters:
    • comment_approval_notification_text
    • comment_approval_notification_subject
  • Changes the Your comment is awaiting moderation. text to Your comment is awaiting moderation. You will receive an email when it gets approved.

The email looks as follows:

Howdy Jane Doe,

Your comment on the post "Hello World" got approved.

View comment: http://src.wordpress-develop.dev/2015/09/06/hello-world/#comment-16

Changes to the first patch:

  • Email is only sent when comment gets approved, not when it got trashed etc.
  • No comment_notification_notify_commenter filter. Functionality can be easily disabled using remove_action( 'transition_comment_status', 'wp_notify_commenter' )

#19 @swissspidy
9 years ago

  • Milestone changed from Future Release to 4.4

@mrahmadawais
9 years ago

#20 in reply to: ↑ 18 @mrahmadawais
9 years ago

Replying to swissspidy:

Added a check for if in case The comment was left by the author & The author moderated a comment on their own post

BTW we can use a better action i.e. comment_unapproved_to_approved action is defined in function wp_transition_comment_status 1286 comment-functions.php.

What do you think?

Last edited 9 years ago by mrahmadawais (previous) (diff)

@swissspidy
9 years ago

#21 follow-up: @swissspidy
9 years ago

Replying to mrahmadawais:

Added a check for if in case The comment was left by the author & The author moderated a comment on their own post

Thanks, good catch. However I noticed the following things with your patch:

  • Indentation is wrong throughout the function.
  • $author isn't used anywhere
  • $notify_author isn't defined anywhere, which would lead to an error.

33717.4.diff accommodates for that. Plus it fixes the one multi-line comment.

BTW we can use a better action i.e. comment_unapproved_to_approved action is defined in function wp_transition_comment_status 1286 comment-functions.php.

I'd say the transition_comment_status hook can be more easily found and debugged as the dynamic comment_{$old_status}_to_{$new_status} hook.
Also, does this work when the comment was previously trashed / mistakenly marked as spam but then approved?

#22 in reply to: ↑ 21 @mrahmadawais
9 years ago

Replying to swissspidy:

Replying to mrahmadawais:

Added a check for if in case The comment was left by the author & The author moderated a comment on their own post

Thanks, good catch. However I noticed the following things with your patch:

  • Indentation is wrong throughout the function.
  • $author isn't used anywhere
  • $notify_author isn't defined anywhere, which would lead to an error.

Thanks for the fix. I had actually completed writing a function inside pluggable.php when you uploaded the patch. I think I just copied this check and forgot about the two variables, should have submitted it after second cup of coffee.

33717.4.diff accommodates for that. Plus it fixes the one multi-line comment.

BTW we can use a better action i.e. comment_unapproved_to_approved action is defined in function wp_transition_comment_status 1286 comment-functions.php.

I'd say the transition_comment_status hook can be more easily found and debugged as the dynamic comment_{$old_status}_to_{$new_status} hook.
Also, does this work when the comment was previously trashed / mistakenly marked as spam but then approved?

I haven't tested that, but in theory, it should work whenever a comment goes from unapproved to approved status.

#23 @wonderboymusic
9 years ago

  • Keywords needs-refresh added; good-first-bug needs-testing removed

This looks great, but we need a more robust way of knowing where the comment is transitioning from. Right now, an email is getting sent from Trash Undo, which is not good. We should aim to do something that results in the email being sent as few times as is possible.

#24 @jeffr0
9 years ago

I agree with Scott, the email should only be sent when a comment is approved from the moderation queue and not under any other circumstances.

@swissspidy
9 years ago

#25 follow-ups: @swissspidy
9 years ago

  • Keywords needs-refresh removed

As suggested initially by @mrahmadawais, 33717.6.diff uses the comment_unapproved_to_approved hook to only send a notification when the comment status changes from unapproved to approved.

#26 in reply to: ↑ 25 @mrahmadawais
9 years ago

Replying to swissspidy:

As suggested initially by @mrahmadawais, 33717.6.diff uses the comment_unapproved_to_approved hook to only send a notification when the comment status changes from unapproved to approved.

Yup, all good now. I have had actually tried this hook before. Working fine at my end now.


Last edited 9 years ago by mrahmadawais (previous) (diff)

#27 in reply to: ↑ 25 @jdgrimes
9 years ago

  • Keywords needs-refresh added

Replying to swissspidy:

As suggested initially by @mrahmadawais, 33717.6.diff uses the comment_unapproved_to_approved hook to only send a notification when the comment status changes from unapproved to approved.

This will still cause an email to be sent multiple times if the comment is approved and then unapproved and then approved again. As a result, the email will also be sent for old comments if the comment status is toggled in this way.

I would have suggested setting a meta flag on the comment once the email was sent, and then checking for that to prevent multiple emails. However, I think maybe we need to find a more robust way to determine whether an approval is the initial approval, or else the emails would still be fired off for old comments that were received before this code existed, if the comment status was toggled (possibly that is just an edge case, though).

#28 @knutsp
9 years ago

Also not send if comments are closed on the post. Toggling the status for old comments is not a problem, or too much of an edge case for core. Let plugins (a filter) handle that.

#29 @helen
9 years ago

What happens with custom comment statuses if using that hook? :)

#30 @dshanske
9 years ago

I just realized that #33735 is related to this in that what I am proposing re duplication and more flexibility can be resolved while adding this new notification.

Specifically since parts of all comment notification can come from a common source.

Last edited 9 years ago by dshanske (previous) (diff)

@swissspidy
9 years ago

#31 follow-up: @swissspidy
9 years ago

  • Keywords needs-refresh removed

We should aim to do something that results in the email being sent as few times as is possible.

33717.7.diff sets a flag using comment meta to prevent sending emails more than once when the status changes back and forth.

Also not send if comments are closed on the post.

I disagree here. If comments are closed and a comment gets approved, it will still be shown on the website and the user should be notified about that.

What happens with custom comment statuses if using that hook?

In my opinion, when someone adds new comment statuses, chances are that sending an email is not desired. If it is, the comment_{$old_status}_to_{$new_status} filter can easily be used to add this.

#32 in reply to: ↑ 31 ; follow-up: @obenland
9 years ago

Notify a comment author when his comment gets approved. - Let's use a gender-neutral their.
We usually don't add an empty line between @param and @return (unfortunately).
The meta key _wp_notification_sent seems a bit generic.
I also wonder if we should accept both a comment ID and a WP_Comment object to be more in line with wp_notify_postauthor().

@swissspidy
9 years ago

#33 in reply to: ↑ 32 @swissspidy
9 years ago

Replying to obenland:

Notify a comment author when his comment gets approved. - Let's use a gender-neutral their.

Done in the latest patch.

We usually don't add an empty line between @param and @return (unfortunately).

I know, but my IDE unfortunately always adds an empty line, even though I configured all sniffs :/

The meta key _wp_notification_sent seems a bit generic.

Renamed it to _wp_commenter_notification_sent.

I also wonder if we should accept both a comment ID and a WP_Comment object to be more in line with wp_notify_postauthor().


Good idea. That way the function can also be hooked to other actions where only an ID is passed. Added now.

#34 @quicoto
9 years ago

+1 For this feature.

#35 in reply to: ↑ 9 @Monika
9 years ago

Replying to jeffr0:

What are your thoughts on whether a commenter should have to opt-in to receive the notification instead of automatically sending it?

The data privacy act at Austria and Germany doesn't allow any automatically e-mail without double opt in.
So we need the possibility to ask the user or to disable this automatically notification or a way to inform the user before he commented.

#36 @rmccue
9 years ago

Just to note on the wording, "Your comment got approved" sounds a bit strange (though I can't remember the technical reason linguistically). "Your comment has been approved" would sound a bit better here. Even better if we can add a touch of pizzazz along the lines of "howdy" and keep it a bit more informal.

Last edited 9 years ago by rmccue (previous) (diff)

@swissspidy
9 years ago

#37 @swissspidy
9 years ago

  • Keywords has-unit-tests added

33717.9.diff makes it easier to remove the the functionality.

If you remove the action, e.g. using remove_action( 'comment_unapproved_to_approved', 'wp_new_comment_notify_commenter' ), the "You will receive an email when it gets approved." message won't be displayed.

The patch also fixes translator comments in the email and adds a unit test.

Oh, and yes, "Your comment has been approved" is definitely better than "Your comment got approved".

#38 @wonderboymusic
9 years ago

  • Milestone changed from 4.4 to Future Release

This ticket was mentioned in Slack in #core by helen. View the logs.


9 years ago

@swissspidy
9 years ago

#40 @swissspidy
9 years ago

  • Milestone changed from Future Release to 4.5

33717.10.diff is a new version of the patch that applies cleanly against trunk, as I'd really like to see this in 4.5.

Differences to the previous patch:

  • New function name: wp_new_comment_notify_comment_author
  • Some refined wording, e.g. do not use "Howdy %s," when comment author name is empty
  • Do not send email when comment author email is empty
  • A bunch of new unit tests to test for every possible scenario

This ticket was mentioned in Slack in #core by rachelbaker. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


9 years ago

#43 @swissspidy
9 years ago

  • Milestone changed from 4.5 to Future Release

#44 @voldemortensen
9 years ago

  • Milestone changed from Future Release to 4.6

Patch still applies cleanly.

#45 @rachelbaker
9 years ago

@swissspidy How are you feeling about this?

#46 @swissspidy
9 years ago

@rachelbaker I'd still love to get this into core!

There was a lack of feedback/interest during the last cycle, but if the consensus is "let's do it" now, by all means let's do it :)

#47 follow-up: @rachelbaker
9 years ago

@swissspidy I agree this would be a useful feature, and you have captured my interest.

I am concerned that emails to comments authors will be triggered when a moderator accidentally "approves" - and then realizing their mistake - quickly undoes the approval action. I don't know how often a scenario like that occurs to users other than me.

This ticket was mentioned in Slack in #core by rachelbaker. View the logs.


9 years ago

#49 in reply to: ↑ 47 @jeffr0
9 years ago

Replying to rachelbaker:

@swissspidy I agree this would be a useful feature, and you have captured my interest.

I am concerned that emails to comments authors will be triggered when a moderator accidentally "approves" - and then realizing their mistake - quickly undoes the approval action. I don't know how often a scenario like that occurs to users other than me.

This is a valid concern and something I've done a few times. Perhaps a delay of 5 minutes or so could be added to the action to give people more than enough time to correct a mistake without sending the email.

#50 @swissspidy
9 years ago

Perhaps a delay of 5 minutes or so could be added to the action to give people more than enough time to correct a mistake without sending the email.

That sounds like we'd need to leverage the cron API for this, similar to how scheduling posts works. Now that I think about it, it could be a great use case for a potential notifications API.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


9 years ago

#52 follow-up: @chriscct7
9 years ago

  • Milestone changed from 4.6 to Future Release

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


8 years ago

#54 in reply to: ↑ 52 @raizen84
6 years ago

Replying to chriscct7:

Hi any news with this? I also believe is very important functionality. Many thnaks

#55 @imath
5 years ago

Hi,

I agree it's an interesting feature and thanks a lot to everyone who contributed to this ticket (in particular @swissspidy). I believe it would be safer to ask the user if he wishes to be informed once his comment is approved. It could use a checkbox the same way there's one for the GDPR cookie consent. We could use a comment meta to make user's choice persist as long as the email is not sent yet. I'll try to refresh the patch in this direction.

@imath
5 years ago

#56 @imath
5 years ago

Now there is the GDPR, I suggest to have a different approach about this feature : request the user to opt-in to be notified when his comment has been approved.

Here is the suggested process :

1) Once the comment has been submitted, the user is redirected to his comment where he can read the "awaiting moderation" info + a form to inform he wishes to be notified when the comment has been approved.

Screenshot: https://cldup.com/vAda-rbD00.png

2) Once the user activate the checkbox and validate the form, WordPress saves a comment meta to store this wish and the user is redirected to his comment again with a message to inform him he will receive the notification when the comment has been approved.

Screenshot: https://cldup.com/tBuPu0Cd9E.png

3) The site administator is informed a user has opted in to be notified (to avoid using the cron).

Dashbord Screenshot: https://cldup.com/xpCVsMuGsN.png
Comments Administration Screen Screenshot: https://cldup.com/xDq5TQsHqX.png

4) When the site administrator approves the comment, the notification is sent and the comment meta is deleted.

The process is included into the 33717.11.diff patch

#57 @garrett-eclipse
5 years ago

  • Keywords needs-testing added

Thanks @imath I appreciate you re-instilling some life into this ticket, I too feel it would be a great feature.

Some comments on your outlined approach;

  1. I would prefer to see the opt-in on the comment form itself and not introduce an additional step that's easily overlooked by users.
  1. Verbiage of the checkbox can be improved by using 'is' over 'will be';

I want to be notified by email when my comment is approved.

  1. The comment cookie consent checkbox functionality can be updated and leveraged to save the users notification preference. If the user opts in to the cookies a new cookie comment_author_notification_{HASH} can be placed to store their notification preference. If this cookie is set and true then the notification checkbox would default to checked. If the cookie isn't present or is false the notification checkbox would be unchecked.

All the best

#58 @garrett-eclipse
5 years ago

  1. Similar to the comment consent checkbox it would be nice to support a hook and an option. Below is the code from the comment cookie consent;

if ( has_action( 'set_comment_cookies', 'wp_set_comment_cookies' ) && get_option( 'show_comments_cookies_opt_in' ) ) {
A similar setup to this will allow admins to have better control of the funcitonality.
From the hook the checkbox is disabled like this - remove_action( 'set_comment_cookies', 'wp_set_comment_cookies' );
As to the option it would be provided as a checkbox on the Discussion Settings similar to the cookies opt-in;
Show comments cookies opt-in checkbox, allowing comment author cookies to be set

@imath
5 years ago

#59 @imath
5 years ago

  • Keywords needs-unit-tests added; has-unit-tests removed

Hi @garrett-eclipse,

Thanks for your feedback :)

About 1) I thought about adding the checkbox to the comment form at the beginning. But I've changed my mind for 2 reasons :

  • I know some plugins are adding checkboxes there. For instance to be notified of new replies, so I felt adding too much checkboxes there was complexifying the form.
  • You're only sure the comment will be held for moderation if the site administrator has chosen to moderate manually each comment. If he chose the "Comment author must have a previously approved comment" option, then it's becoming more complex and requires an AJAX request to check if the user has not previously commented.

I believe we should try to keep it as simple as possible and I think our options are :

  1. notify any user having their comment approved (previous patches),
  2. use a new form once the comment has been saved as pending to leave the choice to the comment author to be notified or not (my preference),
  3. do nothing :)

About 2) Thanks a lot! I've fixed it in 33717.12.diff

About 3) Using a cookie would only avoid 1 click for the 2nd option, I'm not sure it saves the comment author so much time as he would need to submit the form anyway. But if we think it's absolutely required, then why not ;)

About 4) I agree we need to make sure the notification opt-in form is not output if the site owner removed the hook that triggers the notification. In other words, using remove_action( 'comment_unapproved_to_approved', 'wp_new_comment_notify_comment_author' ); is disabling the feature. I think if we can avoid a new option into the Discussion settings, it's always better. 33717.12.diff has been improved to handle this case.

Last edited 5 years ago by imath (previous) (diff)

This ticket was mentioned in Slack in #core by imath. View the logs.


5 years ago

#61 @imath
4 years ago

  • Milestone changed from Future Release to 5.7
  • Owner changed from swissspidy to imath

#62 @johnbillion
4 years ago

  • Keywords needs-dev-note added

I like the fact that this whole piece of functionality, including the checkbox and the email itself, is linked to one action.

@imath Is there any protection here against sending multiple emails if a comment is approved, unapproved, and approved a second time? I think in that case the email should only ever be sent once.


This will need a dedicated dev note. tl;dr a plugin that provides this functionality itself and wishes to disable core's functionality (to avoid duplicate emails) needs to call remove_action( 'comment_unapproved_to_approved', 'wp_new_comment_notify_comment_author' ).

#63 @imath
4 years ago

  • Keywords needs-refresh added; has-patch removed

Hi @johnbillion

Thanks a lot for your feedback.

As the notification opt-in is checked before notifying the comment author and deleted once the notification was sent, I believe it provides us from sending more than one email per comment.

I'm going to refresh the patch and work on adding unit tests to be sure about this :)

@imath
4 years ago

#64 @imath
4 years ago

  • Keywords has-patch added; needs-testing needs-unit-tests needs-refresh removed

Hi,

337171.14.patch​ is refreshed to latest trunk (December 13, 2020). It adds a unit test to check the email is sent and only once: see the PHPUnit test test_wp_new_comment_notify_comment_author_once_only() (cc @johnbillion).

Here are some updated screenshots:

  1. The form to subscribe to the notification (once a comment has been posted and held for moderation)

https://cldup.com/bk_H7Ap3La.png

  1. The subscription confirmation

https://cldup.com/Wkhps21OCz.png

  1. The warning about the fact a comment author requested to be notified
  1. Into the dashboard Widget

https://cldup.com/7IfL0eBSFQ.png

  1. Into the comments list table

https://cldup.com/zVYHGvnxzd.png

  1. The email received by the comment author

https://cldup.com/f0ky8A2pEy.png

This ticket was mentioned in Slack in #core-comments by imath. View the logs.


4 years ago

#66 @ubernaut
4 years ago

awesome! :)

This ticket was mentioned in Slack in #core by imath. View the logs.


4 years ago

#68 @longman2020
4 years ago

Hi,

This feature is really needed.
Please add it to the core.

Thanks.

#69 @johnbillion
4 years ago

  • Owner changed from imath to johnbillion
  • Status changed from assigned to reviewing

#70 @johnbillion
4 years ago

@imath Thanks for working on this. Overall the functionality looks good. Some feedback and questions from me:

  • Themes that call wp_list_comments() with a custom comment walker class (affects Twenty Twenty, Twenty Nineteen) or a custom callback (affects Twenty Eleven, Twenty Ten) don't benefit from this new functionality, meaning there's no way to opt in to a notification on the front end with such a theme in use. Is there a way we can do one of the following?
    • Inject the checkbox and message using another method so that all themes benefit from it
    • Or implement a simple method (eg. calling do_action()) for themes to use in a backwards-compatible manner
  • On the Comments admin screen and in the Recent Comments section of the dashboard the "{author} has opted in to receive a notification on comment’s approval" message is shown, but this message isn't shown either when editing the comment or when clicking through to approve the comment from an email notification. I'm not entirely convinced the message is needed, but if it is then it should be shown in these locations too or not shown at all.
  • In wp_new_comment_notify_comment_author(), the post author is prevented from receiving a notification. Is there a reason for this? They are able to opt in to receiving one.
  • There are several instances of 0 === (int) $comment->comment_approved. These should be changed to '0' === $comment->comment_approved to avoid unnecessary type juggling.
  • A test is needed to cover the situation where a user does not opt in to a notification and their comment gets approved, to assert that no email is sent.
  • The comment_approval_notification_text and comment_approval_notification_subject filters in wp_new_comment_notify_comment_author() should be replaced with one filter that filters the whole array of data for the email. See the new_site_email as an example.

#71 @johnbillion
4 years ago

Another observation, possibly related to the first point in my comment above. When an unapproved comment is displayed there is moderation-related text both above and below its content. This means if the comment text is long then the "I want to be notified..." checkbox is nowhere near the "Your comment is awaiting moderation" message. It would be good to place them together. Screenshot above.

#72 @imath
4 years ago

  • Keywords needs-refresh added; has-patch removed

Hi @johnbillion thanks a lot for your review. I'm working on improving the patch with your recommandations asap.

@imath
4 years ago

#73 @imath
4 years ago

  • Keywords has-patch added; needs-refresh removed

Hi @johnbillion

I've applied your recommandations in 33717.15.patch​ :)

  • The form is now included into themes extending the Walker_Comment class.
  • I've added the warning message to the edit comment screen, when necessary.
  • I've removed the post author check, but I wasn't able to see the issue there. If I comment on my own post having the Author role it's immediately approved.
  • I've changed the strict checks.
  • I've added the test for the case you described.
  • I've edited filters the way you suggested.
  • I've moved the form and the confirmation over the comment text.

Thanks again for your review.

This ticket was mentioned in Slack in #core by imath. View the logs.


4 years ago

@johnbillion
4 years ago

#75 follow-up: @johnbillion
4 years ago

This is looking really good. A few more points:

  • I've uploaded another patch, 33717.13.diff, mostly with formatting tweaks and improvements to the language used.
  • Unmoderated comments are only visible for one minute via the moderation hash (ref). I think this needs to be extended in order to allow users a longer window to opt in to the moderation notification and still be able to see their pending comment. If you're on a slow connection, you're using assistive technology, or you're a slow reader or slow decision maker, you can easily take longer than 60 seconds to submit this form. I can open a follow-up ticket for this.
  • I don't see much (any?) benefit to the messages displayed that say "{author} has opted in to receive a notification on comment’s approval". Does anyone feel strongly about these? I think they add quite a lot of visual noise but are of no concern to the user who is moderating comments. Thoughts?

#76 in reply to: ↑ 75 @kraftbj
4 years ago

Replying to johnbillion:

  • I don't see much (any?) benefit to the messages displayed that say "{author} has opted in to receive a notification on comment’s approval". Does anyone feel strongly about these? I think they add quite a lot of visual noise but are of no concern to the user who is moderating comments. Thoughts?

I agree. As a default, I don't think information needs to be visible and would defer to a plugin to add that if it is critical for some reason.

To give another example, there is no visible indicator on a comment when that commenter has subscribed to follow-up comments when using the Jetpack plugin and, afaik/can tell, it has never been requested.

#77 @imath
4 years ago

Hi @johnbillion Thanks a lot for your improvements 👍

The notice message was an attempt to reply to this comment, but I don’t feel strongly about it ☺️. I’m fine with removing it.

About the 60s delay to opt in for the notice, it might be safer to double this time, I agree.

@johnbillion
4 years ago

#78 @johnbillion
4 years ago

  • Type changed from feature request to enhancement

33717.14.diff slims down the patch by removing those messages. I'm now reviewing the changes to the walker.

@garrett-eclipse
4 years ago

Minor CS fix after reviewing @johnbillion's .14 patch. Just indents the contents of the else from lines 41-71 of wp-comments-post.php

@johnbillion
4 years ago

#79 @johnbillion
4 years ago

  • Keywords commit added

33717.16.diff is one last tweak to this.

  • Removes the $skip_backcompat parameter from the the comment_approval_notification_form() as its value is always true. A custom walker would not need to call this method without disabling the backcompat output otherwise you get the form twice.
  • Renames the $backcompat property so it only deals with this specific form output instead of being an array. If we end up introducing other features like this that also need backcompat it's a bit easier to see when they were introduced if they're in individual properties.

#80 @johnbillion
4 years ago

@garrett-eclipse Thanks for the reminder, I had un-indented it to make the changes easier to view locally.

#81 @johnbillion
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 50109:

Comments: Introduce a method for commenters to opt-in to receiving an email notification when their moderated comment gets approved.

The opt-in form is shown after the comment is submitted and held for moderation.

Sorry this took five years.

Props jeffr0, swissspidy, mrahmadawais, wonderboymusic, jdgrimes, obenland, Monika, imath, garrett-eclipse, johnbillion

Fixes #33717

#82 @SergeyBiryukov
4 years ago

In 50112:

I18N: Correct placeholders in translator comments in wp_new_comment_notify_comment_author().

Follow-up to [42827], [45932], [50109].

See #33717.

#83 @johnbillion
4 years ago

In 50113:

Comments: Fix a coding standards issue introduced in [50109].

See #33717

#84 @johnbillion
4 years ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#85 @johnbillion
4 years ago

In 50375:

Comments: Revert the introduction of the opt-in comment approval notification feature.

This reverts the following commits: [50113], [50112], [50109].

See #33717

#86 @johnbillion
4 years ago

  • Milestone changed from 5.7 to Future Release

Following advice from @chanthaboune this feature has been reverted and won't be part of 5.7. Moving to Future Release for now.

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


4 years ago

#88 @johnbillion
4 years ago

  • Owner johnbillion deleted
  • Status changed from reopened to assigned
Note: See TracTickets for help on using tickets.