Improve: send expiry mail when user has only valid addresses or to an… (

#344)

* Improve: send expiry mail when user has only valid addresses or to any valid when revalidation disabled

* Improve: test partly invalid user with revalidation

* chore: update test comments

* fix: keep behavior without revalidation the same

* chore: update changelog to reflect changes

CHANGELOG.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Changed
+- Sending the account expiry email is done when user has only valid e-mail addresses
+
 ### Fixed
 - User account expiry continues when one or more e-mail addresses are marked for revalidation
 

server/keyshare/tasks/tasks.go

@@ -129,16 +129,16 @@ func (t *taskHandler) sendExpiryEmails(ctx context.Context, id int64, username,
 
 			if err := keyshare.VerifyMXRecord(email); err != nil {
 				if t.revalidateMail {
-					if err = t.db.ExecUserContext(ctx, "UPDATE irma.emails SET revalidate_on = $1 WHERE id = $2",
+					if revErr := t.db.ExecUserContext(ctx, "UPDATE irma.emails SET revalidate_on = $1 WHERE id = $2",
 						time.Now().AddDate(0, 0, 5).Unix(),
-						id); err != nil {
-						t.conf.Logger.WithField("error", err).Error("Could not update email address to set revalidate_on")
-						return err
+						id); revErr != nil {
+						t.conf.Logger.WithField("error", revErr).Error("Could not update email address to set revalidate_on")
+						return revErr
 					}
 				}
 
-				// We wait with further processing until the email address is revalidated
-				// so we can send the expiry mail to all, and only valid, addresses at once
+				// Abort to prevent sending mail. If revalidation is enabled, after the e-mail address is revalidated,
+				// the process will continue so an expiry mail is sent to all, and only valid, addresses at once.
 				return err
 			}
 
@@ -149,7 +149,6 @@ func (t *taskHandler) sendExpiryEmails(ctx context.Context, id int64, username,
 	)
 
 	if err != nil {
-		t.conf.Logger.WithField("error", err).Error("Could not process user's email addresses")
 		return err
 	}
 
@@ -223,13 +222,13 @@ func (t *taskHandler) expireAccounts(ctx context.Context) {
 
 			// Send emails
 			if err := t.sendExpiryEmails(ctx, id, username, lang); err != nil {
-				if err == keyshare.ErrInvalidEmail {
+				if err == keyshare.ErrInvalidEmail || err == keyshare.ErrInvalidEmailDomain {
 					if !t.revalidateMail {
-						// To have the exact same behavior as before email revalidation functionality,
-						// we return nil when the error is ErrInvalidEmail. Additionally we log the error
-						t.conf.Logger.WithField("error", err).Errorf("User decreases processable amount in expireAccounts, id: %d", id)
+						// We can't do anything about an invalid email address or domain
+						// when revalidation is disabled so we log the error and continue iterating
+						t.conf.Logger.WithField("error", err).Errorf("User decreases processable amount in expireAccounts, user_id: %d", id)
 					}
-					return nil
+					return nil // Don't abort the entire task over an email seding issue, only skip this user
 				}
 
 				return err // already logged, just abort

server/keyshare/tasks/tasks_test.go

@@ -278,25 +278,32 @@ func TestEmailRevalidation(t *testing.T) {
 
 	createUser(t, db, 1, "TemporaryInvalid", time.Now().AddDate(-1, -1, -1).Unix(), []string{"[email protected]"})
 	createUser(t, db, 2, "PermanentInvalid", time.Now().AddDate(-1, -1, -1).Unix(), []string{"[email protected]"})
+	createUser(t, db, 3, "PartlyInvalid", time.Now().AddDate(-1, -1, -1).Unix(), []string{"[email protected]", "[email protected]"})
 
 	require.NoError(t, runWithTimeout(th.expireAccounts))
 	require.NoError(t, runWithTimeout(th.revalidateMails))
 
-	// Is revalidate_on set for both email addresses?
-	assert.Equal(t, 2, countRows(t, db, "emails", "revalidate_on IS NOT NULL"))
+	// Is revalidate_on set for all email addresses?
+	assert.Equal(t, 3, countRows(t, db, "emails", "revalidate_on IS NOT NULL"))
+	assert.Equal(t, 0, countRows(t, db, "users", "delete_on IS NOT NULL"))
 
 	// Correct the temporary invalid e-mail address to a valid one, simulating it becoming valid, and 'forward time'
 	_, err = db.Exec("UPDATE irma.emails SET email = $1, revalidate_on = $2 WHERE id = $3", "[email protected]", time.Now().AddDate(0, 0, -1).Unix(), 1)
 	require.NoError(t, err)
 
-	// Forward time for the invalid address
-	_, err = db.Exec("UPDATE irma.emails SET revalidate_on = $1 WHERE id = $2", time.Now().AddDate(0, 0, -1).Unix(), 2)
+	// Forward time for the invalid addresses
+	_, err = db.Exec("UPDATE irma.emails SET revalidate_on = $1 WHERE email LIKE $2", time.Now().AddDate(0, 0, -1).Unix(), "%@permenantlyinvalidaddress.com")
 	require.NoError(t, err)
 
 	// Next revalidation run: the invalid email address should be deleted now
 	require.NoError(t, runWithTimeout(th.revalidateMails))
 
-	assert.Equal(t, 1, countRows(t, db, "emails", ""))
+	assert.Equal(t, 2, countRows(t, db, "emails", ""))
+
+	// Next expireAccounts run: the PartlyInvalid user should be expired since the permanently invalid address is deleted now.
+	require.NoError(t, runWithTimeout(th.expireAccounts))
+	assert.Equal(t, 3, countRows(t, db, "users", ""))
+	assert.Equal(t, 2, countRows(t, db, "users", "delete_on IS NOT NULL"))
 }
 
 func SetupDatabase(t *testing.T) {