Send notification emails when contributors are inactive #206

iandunn · 2022-07-28T22:54:59Z

Fixes most of #27, but doesn't send an email to the sponsoring company. That can be a future iteration while @angelasjin is working on the details of that.

This sends the email to folks that haven't logged in in 3 months. Once #119 and #189 are done, we can update it to look at 5ftF contributions specifically.

It needs polishing, but I think it's close to a state that I'm happy with, and close enough to get some review while I finish the polishing.

I initially tried using more complex SQL queries to reduce the number of rows that would be fetched, and the amount of PHP code needed, but that ended up being more trouble than it was worth.

See #205 for a discussion about the email copy.

tellyworth · 2022-08-05T03:49:55Z

plugins/wporg-5ftf/includes/email.php

@@ -22,7 +23,7 @@
 *
 * @return bool
 */
-function send_email( $to, $subject, $message, $pledge_id ) {
+function send_email( $to, $subject, $message, $pledge_id = false ) {
 	$headers = array(
 		'From: WordPress - Five for the Future <donotreply@wordpress.org>',


Some mixed messages here - we ask people to reply but then have donotreply as the From address. I realise the Reply-To will work, but some users might not.

That's a good point. I was thinking it was used in order to match the Return-Path, so that spam filters don't think we're forging From. I just checked though, and the Return-Path is bounce@wordpress.org.

We could change it to that. It technically has the same meaning, but most folks won't be familiar with the term, so it's not as bad.

I'm leery of changing the From to getinvolved@ though, because of those spam filters.

I set it to bounce@ in 7cad4bd, but I'm open to other ideas if you think there's a better way.

tellyworth · 2022-08-05T03:56:41Z

plugins/wporg-5ftf/includes/contributor.php

+	$inactivity_threshold = strtotime( INACTIVITY_THRESHOLD_MONTHS . ' months ago' );
+
+	foreach ( $contributors as $index => $contributor ) {
+		if ( $contributor['last_logged_in'] > $inactivity_threshold ) {


Does this need some logic to handle a missing/invalid last_logged_in value?

It should always exist at this point, it'll be 0 if nothing exists in the db.

five-for-the-future/plugins/wporg-5ftf/includes/contributor.php

Line 742 in 8d6ea36

$full_user['last_logged_in'] = intval( strtotime( $full_user['last_logged_in'] ?? '' ) ); // Convert `false` to `0`.

tellyworth · 2022-08-05T03:59:14Z

plugins/wporg-5ftf/includes/contributor.php

+ * Notify an inactive contributor.
+ */
+function notify_inactive_contributor( array $contributor ) : void {
+	if ( ! Email\send_contributor_inactive_email( $contributor ) ) {


Could there be a situation where sending fails repeatedly? If it's a permanent error like an invalid email address we'd effectively be silently retrying forever, if I'm reading correctly.

🤔 that's true.

It looks like account registration and editing both validate the address with Core's is_email(). That's not RFC-compliant, but that might actually help us in this case, since IIRC there are lots of "valid" addresses that aren't routable. There's probably some edge cases though.

wp_mail() also uses is_email() to validate. So, I think an address will always either:

be blocked by wp_mail() and therefore not trigger any spam filters that hurt our IP reputation; or

be sent to the address's mail server, and therefore get an updated 5ftf_last_inactivity_email

In the case of 1, though, that could maybe eventually cause some users to never get the email. If enough edge cases clumped together inside a batch group, then they could get stuck at the front of it, causing the others at the end of the group to always be ignored.

five-for-the-future/plugins/wporg-5ftf/includes/contributor.php

Lines 596 to 599 in 8d6ea36

// Limit to 25 emails per cron run, to avoid triggering spam filters.

if ( count( $contributors ) > 25 ) {

$contributors = array_slice( $contributors, 0, 25 );

}

Adding a shuffle() before slicing would avoid that.

I updated 041abcf to include shuffle()

tellyworth · 2022-08-05T04:03:46Z

plugins/wporg-5ftf/includes/contributor.php

+	$field_names = array_flip( XProfile\FIELD_IDS );
+
+	foreach ( $user_xprofiles as $user ) {
+		$user->user_id = absint( $user->user_id );


absint( $user->user_id ) makes me wonder what edge case this is intended for, and whether there's a better way to handle it.

IIRC it's just because the DB returns it as a string, but in other places we need to compare it to an int. We have to use strict comparison because the linter will complain if we don't. It also just makes me feel more comfortable, knowing that it's what I expect.

tellyworth · 2022-08-05T04:07:49Z

plugins/wporg-5ftf/includes/email.php

+		return trim( $team );
+	}, $contributor['teams_names'] );
+
+	$message = sprintf( "


Should there be a last-chance check to make sure all the variables are set? Just to guard against silly errors like "you pledged 0 hours" and other things that are likely to wind up in a screenshot on social media :)

🤔

$name falls back to user_nicename, which should be guaranteed to exist

$contributor['hours_per_week'] and team_names have checks to make sure they're valid

five-for-the-future/plugins/wporg-5ftf/includes/contributor.php

Lines 650 to 651 in 8d6ea36

$user->hours_per_week = absint( $user->hours_per_week ?? 0 );

$user->team_names = (array) $user->team_names ?? array();

five-for-the-future/plugins/wporg-5ftf/includes/contributor.php

Lines 666 to 669 in 8d6ea36

if ( $xprofile->hours_per_week <= 0 || empty( $xprofile->team_names ) ) {

unset( $xprofiles[ $index ] );

continue;

}

It feels a bit WET/overkill to duplicate that in the email function, but I can see the concern.

Sometimes we do want folks to reply to messages, and the `donotreply` address contradicts that. `bounce@` has a similar meaning, but is less known. `bounce@` has the advantage of matching the `Return-Path`, which may reduce the chance of a message being flagged as spam for forging the `From` header. See #206 (comment)

Sometimes we do want folks to reply to messages, and the `donotreply` address contradicts that. `bounce@` has a similar meaning, but is less known. `bounce@` has the advantage of matching the `Return-Path`, which may reduce the chance of a message being flagged as spam for forging the `From` header. See WordPress/five-for-the-future#206 (comment)

iandunn requested a review from tellyworth July 28, 2022 22:54

iandunn mentioned this pull request Jul 28, 2022

Outreach: emails to send to individuals who are dormant/inactive/etc #205

Open

3 tasks

iandunn force-pushed the inactivity-cron branch 3 times, most recently from 2f260eb to 9bffc73 Compare August 1, 2022 23:52

iandunn mentioned this pull request Aug 2, 2022

Expand inactivity notifications to more teams #207

Open

5 tasks

iandunn force-pushed the inactivity-cron branch from 9bffc73 to 7505baa Compare August 2, 2022 17:48

Add stub for natural_language_join().

2044c2c

iandunn force-pushed the inactivity-cron branch from 7505baa to 8d6ea36 Compare August 3, 2022 23:23

tellyworth approved these changes Aug 5, 2022

View reviewed changes

iandunn force-pushed the inactivity-cron branch from 8d6ea36 to 7cad4bd Compare August 5, 2022 21:04

iandunn added 3 commits August 5, 2022 14:51

Add email that is sent to inactive contributors.

d09ed70

Add cron job to send emails to inactive contributors.

d5448bc

iandunn force-pushed the inactivity-cron branch from 7cad4bd to 67db5a0 Compare August 5, 2022 21:51

iandunn marked this pull request as ready for review August 10, 2022 17:24

iandunn merged commit 4dc03ad into production Aug 10, 2022

iandunn mentioned this pull request Aug 10, 2022

Regularly email contributors to prompt for updates #27

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send notification emails when contributors are inactive #206

Send notification emails when contributors are inactive #206

iandunn commented Jul 28, 2022

tellyworth Aug 5, 2022

iandunn Aug 5, 2022

iandunn Aug 5, 2022

tellyworth Aug 5, 2022

iandunn Aug 5, 2022

tellyworth Aug 5, 2022

iandunn Aug 5, 2022 •

edited

Loading

iandunn Aug 5, 2022

tellyworth Aug 5, 2022

iandunn Aug 5, 2022

tellyworth Aug 5, 2022

iandunn Aug 5, 2022 •

edited

Loading

	// Limit to 25 emails per cron run, to avoid triggering spam filters.
	if ( count( $contributors ) > 25 ) {
	$contributors = array_slice( $contributors, 0, 25 );
	}

	$user->hours_per_week = absint( $user->hours_per_week ?? 0 );
	$user->team_names = (array) $user->team_names ?? array();

	if ( $xprofile->hours_per_week <= 0 \|\| empty( $xprofile->team_names ) ) {
	unset( $xprofiles[ $index ] );
	continue;
	}

Send notification emails when contributors are inactive #206

Send notification emails when contributors are inactive #206

Conversation

iandunn commented Jul 28, 2022

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

iandunn Aug 5, 2022 • 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

Choose a reason for hiding this comment

iandunn Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

iandunn Aug 5, 2022 •

edited

Loading

iandunn Aug 5, 2022 •

edited

Loading