Comments: Moderation Emails Overrides #8144

rodrigoi · 2017-11-10T16:05:07Z

Depends on:

Automattic/wp-calypso#19635

Changes proposed in this Pull Request:

Overrides the pluggable functions wp_notify_postauthor and wp_notify_moderator to point the user to WordPress.com
The code has been taken verbatim from pluggable.php on Core v4.8.3 and only the moderation links have been modified.
Should only work on active and connected instances of Jetpack.
Should only work when all the users to be notified are connected to WordPress.com

Testing instructions:

Enable edit_links_calypso_redirect option by adding true to this line:

if ( JetpackOptions::get_option( 'edit_links_calypso_redirect', true ) && ! is_admin() ) {...}

To test moderation

Set Comment must be manually approved on Settings > Discussion
Verify that the admin user is connected to WordPress.com.
Comment on any post
The moderation email notifications links should point to WordPress.com and should include the correct site slug and action.

To test notifications

Disable Set Comment must be manually approved on Settings > Discussion
Verify that the author of the post to comment is connected to WordPress.com.
Comment on a post.
The email notifications links should point to WordPress.com and should include the correct site slug and action.

jeherve

Will this be activated for all Jetpack users? Should we check if Jetpack is connected to WordPress.com before to make those changes?
Should we check if the user has linked their account to WordPress.com? Indeed, it could be that the site is connected to WordPress.com, but the post author has not linked their account to WordPress.com, and thus is not able to access the site in WordPress.com.

jeherve · 2017-11-13T08:42:07Z

moderation-emails-override.php

+	 * @param array $emails     An array of email addresses to receive a comment notification.
+	 * @param int   $comment_id The comment ID.
+	 */
+	$emails = apply_filters( 'comment_notification_recipients', $emails, $comment->comment_ID );


Since those are core hooks, you do not need to include the whole docblock. Something like this would do the trick:
/** This filter is documented in core/src/wp-includes/pluggable.php */

jeherve · 2017-11-13T08:42:16Z

moderation-emails-override.php

+	 *                         Default false.
+	 * @param int  $comment_id The comment ID.
+	 */
+	$notify_author = apply_filters( 'comment_notification_notify_author', false, $comment->comment_ID );


Same as above.

jeherve · 2017-11-13T08:42:32Z

moderation-emails-override.php

+	 * @param string $notify_message The comment notification email text.
+	 * @param int    $comment_id     Comment ID.
+	 */
+	$notify_message = apply_filters( 'comment_notification_text', $notify_message, $comment->comment_ID );


Same as above.

jeherve · 2017-11-13T08:42:37Z

moderation-emails-override.php

+	 * @param string $subject    The comment notification email subject.
+	 * @param int    $comment_id Comment ID.
+	 */
+	$subject = apply_filters( 'comment_notification_subject', $subject, $comment->comment_ID );


Same as above.

jeherve · 2017-11-13T08:42:41Z

moderation-emails-override.php

+	 * @param string $message_headers Headers for the comment notification email.
+	 * @param int    $comment_id      Comment ID.
+	 */
+	$message_headers = apply_filters( 'comment_notification_headers', $message_headers, $comment->comment_ID );


Same as above.

kwight · 2017-11-14T18:07:33Z

Works great!

Can we somehow make sure that we don't clobber an existing user's own versions of these pluggable functions? If one exists already in a plugin, it should get to be active, regardless of loading sequences.

rodrigoi · 2017-11-14T18:18:35Z

@jeherve I've added a few checks so that it only works when Jetpack is connected and when all the users to be notified are connected to WordPress.com.
Any other check I may be missing?

rodrigoi · 2017-11-14T18:23:17Z

Can we somehow make sure that we don't clobber an existing user's own versions of these pluggable functions? If one exists already in a plugin, it should get to be active, regardless of loading sequences.

we already check with function_exists('wp_notify_postauthor'), but plugins loading sequences are very much in play here. I don't know how to check for that. Any suggestions?

gwwar · 2017-11-27T18:56:41Z

Let's make sure that we only override this when folks request to Calypso-ify wp-admin/site links. See related:

#8069
#7732

kwight · 2017-12-05T23:01:55Z

we already check with function_exists('wp_notify_postauthor'), but plugins loading sequences are very much in play here. I don't know how to check for that. Any suggestions?

My thought at the time was to hook the check and the require to something quite late, something after which a plugin would normally hook itself, like add_actions( 'plugins_loaded', 'function_name', 99 );. Or later even, on init or wp_loaded.

gwwar · 2017-12-20T21:15:47Z

Do folks recall if edit_links_calypso_redirect is visible as an option yet? This is currently set to true for new AT transfers.

gwwar · 2017-12-20T21:48:21Z

class.jetpack.php

@@ -640,6 +640,10 @@ private function __construct() {
 		// We should make sure to only do this for front end links.


Probably should update this comment

gwwar · 2017-12-20T21:55:42Z

@rodrigoi which version would you like to target? 5.7 has a code freeze on the 26th and releases on Jan 2nd. Stick with 5.7 or move to 5.8?

gwwar · 2017-12-20T22:11:13Z

This one was testing pretty well for me. @jeherve Would it be possible for folks to take another look?

vindl

Left a couple of comments regarding coding standards. There seems to be a lot more places where proper spacing is missing, but I didn't want to leave too many comments. This could benefit from another pass to correct them before merging.

Otherwise this looks fine to me.

vindl · 2017-12-26T10:15:06Z

moderation-emails-override.php

@@ -0,0 +1,328 @@
+<?php
+
+if ( ! function_exists('wp_notify_postauthor') && Jetpack::is_active() ) :


Missing spaces before/after parens.

vindl · 2017-12-26T10:20:15Z

moderation-emails-override.php

+
+	$switched_locale = switch_to_locale( get_locale() );
+
+	$comment_author_domain = @gethostbyaddr($comment->comment_author_IP);


Missing spaces here too.

vindl · 2017-12-26T10:20:39Z

moderation-emails-override.php

+
+	// The blogname option is escaped with esc_html on the way into the database in sanitize_option
+	// we want to reverse this for the plain text arena of emails.
+	$blogname = wp_specialchars_decode(get_option('blogname'), ENT_QUOTES);


More spaces. :)

vindl · 2017-12-26T10:47:40Z

Do folks recall if edit_links_calypso_redirect is visible as an option yet? This is currently set to true for new AT transfers.

If this is referring to front end facing setting, afaik it's still not available.

we already check with function_exists('wp_notify_postauthor'), but plugins loading sequences are very much in play here. I don't know how to check for that. Any suggestions?

Apart from what @kwight already suggested, I don't think there is much we can do. There is an option for checking for conflicting plugins in addition to function_exists, but I think that would be an overkill here.

oskosk · 2017-12-26T20:23:09Z

I'm punting this PR for the Jetpack 5.8 release for the following reasons.

The code here is in need of a rebase.
The feedback of reviewers has not been addressed.
There are several issues regarding coding style.
I'm about to release Jetpack Beta 5.7 today.

oskosk · 2018-01-05T15:40:29Z

I'm sorry @rodrigoi . Forgot to reply , but there was no beta-2 (nor people around to help re-test :P )

rodrigoi · 2018-01-08T13:31:37Z

@oskosk no problem. @Automattic/jetpack Any chance we can get this merged? Whos commandeering the 5.8 release?

oskosk · 2018-01-08T14:57:52Z

@rodrigoi Sorry. I totally missed the first ping. There wasn't any beta-2 anyways. We'll get this in for 5.8 don't worry.

rodrigoi · 2018-01-24T15:47:30Z

@oskosk any chance we can get this merged?

… links to WordPress.com

… and if all emails have WordPress.com accounts

…calypso_redirect option is set

zinigor

Tested the code, it works fine, except I fixed indentation and moved the override file to the library folder.

One thing I noticed though: the connected users that we send emails to are checked by their email. But it can happen that a user uses one email on their Jetpack site, and another email on WordPress.com. I think the next step for this would be to actually check the user, not the email.

rodrigoi · 2018-01-25T15:30:55Z

thanks @zinigor!! I've run the code thru phpcs/phpbfs to fix the formatting, so I'll triple check next time.

Using the email is intentional. We don't want to send inaccessible WP.com links to a non WP.com linked email address. If the target email is Jetpack only, we want the users to go to wp-admin.

zinigor · 2018-01-26T12:46:49Z

We don't want to send inaccessible WP.com links to a non WP.com linked email address. If the target email is Jetpack only, we want the users to go to wp-admin.

What I mean is that an email mismatch doesn't mean that the user is not connected to WordPress.com. There can be legitimate cases when a user is logged in to WordPress.com, has connected their Jetpack user, and will have no problems accessing Calypso links, but the emails don't match.

rodrigoi · 2018-01-26T18:52:56Z

@zinigor I'll set up a test case for that scenario as soon as possible, but I'd like to keep it on a new PR because this has been open for a long time 😄

* Changelog 5.8: create base for changelog. * Update 5.8 release post link * fix 5.8 release date * Updates to plugin description * Changelog: add #8499 * Changelog: add #8506 * Changelog: add #8509 * Changelog: add #8516 * Changelog: add #8517 * Changelog: add #8523 * Changelog: add #8547 * Changelog: add #8496 * Changelog: add #8584 * Changelog: add #8595 * Changelog: add #8445 * Changelog: add #8431 * Changelog: add #8284 * Changelog: add #8270 * Changelog: add #8124 * Changelog: add #8581 * Changelog: add #8463 * Changelog: add #8568 (#8646) * Updates to testing list and changelog * Changelog: add #8443 * Changelog: add #8459 * Changelog: add #8469 * Changelog: add #8464 * Changelog: add #8478 and #8479 * Changelog: add #8483 * Changelog: add #8488 * Changelog: add #8513 * Changelog: add #8555 * Changelog: add #8565 * Changelog: add #8601 * Changelog: add #8612 * Changelog: add first pass at Search items. * Changelog: add more info to help test Search. * Changelog: add #8144 * Changelog: add #8313 * Changelog: add #8419 * Changelog: add #8465 * Changelog: add #8515 * Changelog: add #8587 * Changelog: add #8591 * Changelog: add #8659 * Changelog: add #8661 * Changelog: add #8671 * Changelog: add 5.7.1 to archived changelog too. * Reverted changes to readme, removed entry about backups.

rodrigoi added [Feature] Comments [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 10, 2017

rodrigoi added this to the 5.6 milestone Nov 10, 2017

rodrigoi self-assigned this Nov 10, 2017

rodrigoi requested a review from a team November 10, 2017 16:05

rodrigoi requested a review from a team as a code owner November 10, 2017 16:05

jeherve previously requested changes Nov 13, 2017

View reviewed changes

jeherve added General [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Feature] Comments labels Nov 13, 2017

rodrigoi force-pushed the add/comments/moderation-email-overrides branch from 78aa2e1 to b22555c Compare November 14, 2017 02:46

rodrigoi removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 14, 2017

rodrigoi modified the milestones: 5.6, 5.7 Nov 27, 2017

jeherve added [Feature] WPCOM API and removed [Feature] WPCOM API labels Nov 27, 2017

rodrigoi mentioned this pull request Dec 5, 2017

Comments: Moderation Hotlinks Automattic/wp-calypso#19774

Closed

6 tasks

rodrigoi force-pushed the add/comments/moderation-email-overrides branch from 87dea84 to 0876f11 Compare December 7, 2017 19:48

gwwar reviewed Dec 20, 2017

View reviewed changes

vindl approved these changes Dec 26, 2017

View reviewed changes

rodrigoi removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Dec 27, 2017

rodrigoi force-pushed the add/comments/moderation-email-overrides branch from 5e34e12 to cebf37b Compare January 8, 2018 13:32

rodrigoi force-pushed the add/comments/moderation-email-overrides branch from cebf37b to de3dc76 Compare January 15, 2018 12:13

Rodrigo Iloro added 5 commits January 24, 2018 17:11

Comments: adds overrides to the moderation emails to point moderation…

587fe25

… links to WordPress.com

Comments: adds check for active and connected Jetpack

66bc99e

Comments: overrides emails only when Jetpack is active and connected,…

e09540f

… and if all emails have WordPress.com accounts

Comments: override pluggable notifications functions when edit_links_…

0c5494a

…calypso_redirect option is set

Comments: phpcs/phpbfs style checks on moderation email overrides file

f0fd596

rodrigoi force-pushed the add/comments/moderation-email-overrides branch from de3dc76 to f0fd596 Compare January 24, 2018 20:11

zinigor added 2 commits January 25, 2018 11:32

Fixed indentation.

df169f8

Moved the override file into the library folder.

fc70e8c

zinigor force-pushed the add/comments/moderation-email-overrides branch from 4c62f15 to fc70e8c Compare January 25, 2018 08:58

zinigor approved these changes Jan 25, 2018

View reviewed changes

zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 25, 2018

dereksmart merged commit a72f60f into master Jan 30, 2018

matticbot added the [Status] Needs Changelog label Jan 30, 2018

dereksmart deleted the add/comments/moderation-email-overrides branch January 30, 2018 16:35

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Jan 30, 2018

jeherve added a commit that referenced this pull request Jan 30, 2018

Changelog: add #8144

f37fca3

kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments: Moderation Emails Overrides #8144

Comments: Moderation Emails Overrides #8144

rodrigoi commented Nov 10, 2017 •

edited

Loading

jeherve left a comment

jeherve Nov 13, 2017

jeherve Nov 13, 2017

jeherve Nov 13, 2017

jeherve Nov 13, 2017

jeherve Nov 13, 2017

kwight commented Nov 14, 2017

rodrigoi commented Nov 14, 2017

rodrigoi commented Nov 14, 2017

gwwar commented Nov 27, 2017

kwight commented Dec 5, 2017 •

edited

Loading

gwwar commented Dec 20, 2017 •

edited

Loading

gwwar Dec 20, 2017

gwwar commented Dec 20, 2017

gwwar commented Dec 20, 2017

vindl left a comment •

edited

Loading

vindl Dec 26, 2017

vindl Dec 26, 2017

vindl Dec 26, 2017

vindl commented Dec 26, 2017

oskosk commented Dec 26, 2017

oskosk commented Jan 5, 2018

rodrigoi commented Jan 8, 2018

oskosk commented Jan 8, 2018

rodrigoi commented Jan 24, 2018

zinigor left a comment

rodrigoi commented Jan 25, 2018

zinigor commented Jan 26, 2018

rodrigoi commented Jan 26, 2018

		@@ -640,6 +640,10 @@ private function __construct() {
		// We should make sure to only do this for front end links.

		@@ -0,0 +1,328 @@
		<?php

		if ( ! function_exists('wp_notify_postauthor') && Jetpack::is_active() ) :


		$switched_locale = switch_to_locale( get_locale() );

		$comment_author_domain = @gethostbyaddr($comment->comment_author_IP);

Comments: Moderation Emails Overrides #8144

Comments: Moderation Emails Overrides #8144

Conversation

rodrigoi commented Nov 10, 2017 • edited Loading

Changes proposed in this Pull Request:

Testing instructions:

jeherve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwight commented Nov 14, 2017

rodrigoi commented Nov 14, 2017

rodrigoi commented Nov 14, 2017

gwwar commented Nov 27, 2017

kwight commented Dec 5, 2017 • edited Loading

gwwar commented Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

gwwar commented Dec 20, 2017

gwwar commented Dec 20, 2017

vindl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vindl commented Dec 26, 2017

oskosk commented Dec 26, 2017

oskosk commented Jan 5, 2018

rodrigoi commented Jan 8, 2018

oskosk commented Jan 8, 2018

rodrigoi commented Jan 24, 2018

zinigor left a comment

Choose a reason for hiding this comment

rodrigoi commented Jan 25, 2018

zinigor commented Jan 26, 2018

rodrigoi commented Jan 26, 2018

rodrigoi commented Nov 10, 2017 •

edited

Loading

kwight commented Dec 5, 2017 •

edited

Loading

gwwar commented Dec 20, 2017 •

edited

Loading

vindl left a comment •

edited

Loading