satellite/console: add cutoff to email reminders

There are multiple entries in the users table with the same email
address. This is because in the past users were able to register
multiple times if the email was not verified. This is no longer
the case. If a user tries to register with an unverified email
already in the DB, we send a verification email instead of
creating another entry. However, since these old entries in the
table with duplicate emails were never cleaned up, the email
reminder chore will send out email verification reminders to them.
A single person will get one separate email per entry in the DB
with their email and where status = 0.

Since the multiple entries with the same email problem was solved
a while ago, just add a constraint to GetUnverifiedNeedingReminder
to only select users created after a cutoff. Once the DB is
migrated to remove the duplicate emails, we can remove the cutoff.

github issue: https://github.com/storj/storj/issues/4853

Change-Id: I07d77d43109bcacc8909df61d4fb49165a99527c

satellite/console/emailreminders/chore.go

@@ -69,7 +69,12 @@ func (chore *Chore) Run(ctx context.Context) (err error) {
 		defer mon.Task()(&ctx)(&err)
 
 		now := time.Now()
-		users, err := chore.usersDB.GetUnverifiedNeedingReminder(ctx, now.Add(-chore.config.FirstVerificationReminder), now.Add(-chore.config.SecondVerificationReminder))
+
+		// cutoff to avoid emailing users multiple times due to email duplicates in the DB.
+		// TODO: remove cutoff once duplicates are removed.
+		cutoff := now.Add(30 * (-24 * time.Hour))
+
+		users, err := chore.usersDB.GetUnverifiedNeedingReminder(ctx, now.Add(-chore.config.FirstVerificationReminder), now.Add(-chore.config.SecondVerificationReminder), cutoff)
 		if err != nil {
 			chore.log.Error("error getting users in need of reminder", zap.Error(err))
 			return nil

satellite/console/users.go

@@ -21,7 +21,7 @@ type Users interface {
 	// Get is a method for querying user from the database by id.
 	Get(ctx context.Context, id uuid.UUID) (*User, error)
 	// GetUnverifiedNeedingReminder gets unverified users needing a reminder to verify their email.
-	GetUnverifiedNeedingReminder(ctx context.Context, firstReminder, secondReminder time.Time) ([]*User, error)
+	GetUnverifiedNeedingReminder(ctx context.Context, firstReminder, secondReminder, cutoff time.Time) ([]*User, error)
 	// UpdateVerificationReminders increments verification_reminders.
 	UpdateVerificationReminders(ctx context.Context, id uuid.UUID) error
 	// GetByEmailWithUnverified is a method for querying users by email from the database.

satellite/console/users_test.go

@@ -380,7 +380,7 @@ func TestGetUnverifiedNeedingReminder(t *testing.T) {
 			// Since we have no control over `created_at` (it's autoinserted) we will instead pass in a future time
 			// as the `now` argument to `GetUnverifiedNeedingReminder`
 			futureTime := now.Add(time.Duration(i*24) * time.Hour)
-			needReminder, err := db.GetUnverifiedNeedingReminder(ctx, futureTime.Add(-config.FirstVerificationReminder), futureTime.Add(-config.SecondVerificationReminder))
+			needReminder, err := db.GetUnverifiedNeedingReminder(ctx, futureTime.Add(-config.FirstVerificationReminder), futureTime.Add(-config.SecondVerificationReminder), now.Add(-time.Hour))
 			require.NoError(t, err)
 
 			// These are the conditions in the SQL query which selects users needing reminder

satellite/satellitedb/userfromdbx_test.go

@@ -0,0 +1,40 @@
+// Copyright (C) 2019 Storj Labs, Inc.
+// See LICENSE for copying information.
+
+package satellitedb
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+
+	"storj.io/storj/satellite/satellitedb/dbx"
+)
+
+func TestUserFromDbx(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("can't create dbo from nil dbx model", func(t *testing.T) {
+		user, err := userFromDBX(ctx, nil)
+		assert.Nil(t, user)
+		assert.Error(t, err)
+	})
+
+	t.Run("can't create dbo from dbx model with invalid ID", func(t *testing.T) {
+		dbxUser := dbx.User{
+			Id:           []byte("qweqwe"),
+			FullName:     "Very long full name",
+			ShortName:    nil,
+			Email:        "some@mail.test",
+			PasswordHash: []byte("ihqerfgnu238723huagsd"),
+			CreatedAt:    time.Now(),
+		}
+
+		user, err := userFromDBX(ctx, &dbxUser)
+
+		assert.Nil(t, user)
+		assert.Error(t, err)
+	})
+}

satellite/satellitedb/users.go

@@ -77,18 +77,19 @@ func (users *users) GetByEmail(ctx context.Context, email string) (_ *console.Us
 }
 
 // GetUnverifiedNeedingReminder returns users in need of a reminder to verify their email.
-func (users *users) GetUnverifiedNeedingReminder(ctx context.Context, firstReminder, secondReminder time.Time) (usersNeedingReminder []*console.User, err error) {
+func (users *users) GetUnverifiedNeedingReminder(ctx context.Context, firstReminder, secondReminder, cutoff time.Time) (usersNeedingReminder []*console.User, err error) {
 	defer mon.Task()(&ctx)(&err)
 
 	rows, err := users.db.Query(ctx, `
 		SELECT id, email, full_name, short_name
 		FROM users
 		WHERE status = 0
+			AND created_at > $3
 			AND (
 				(verification_reminders = 0 AND created_at < $1)
 				OR (verification_reminders = 1 AND created_at < $2)
 			)
-	`, firstReminder, secondReminder)
+	`, firstReminder, secondReminder, cutoff)
 	if err != nil {
 		return nil, err
 	}

satellite/satellitedb/users_test.go

@@ -1,40 +1,56 @@
 // Copyright (C) 2019 Storj Labs, Inc.
 // See LICENSE for copying information.
 
-package satellitedb
+package satellitedb_test
 
 import (
-	"context"
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
-	"storj.io/storj/satellite/satellitedb/dbx"
+	"storj.io/common/testcontext"
+	"storj.io/common/testrand"
+	"storj.io/storj/satellite"
+	"storj.io/storj/satellite/console"
+	"storj.io/storj/satellite/satellitedb/satellitedbtest"
 )
 
-func TestUserFromDbx(t *testing.T) {
-	ctx := context.Background()
+func TestGetUnverifiedNeedingReminderCutoff(t *testing.T) {
+	satellitedbtest.Run(t, func(ctx *testcontext.Context, t *testing.T, db satellite.DB) {
+		users := db.Console().Users()
 
-	t.Run("can't create dbo from nil dbx model", func(t *testing.T) {
-		user, err := userFromDBX(ctx, nil)
-		assert.Nil(t, user)
-		assert.Error(t, err)
+		id := testrand.UUID()
+		_, err := users.Insert(ctx, &console.User{
+			ID:           id,
+			FullName:     "test",
+			Email:        "userone@mail.test",
+			PasswordHash: []byte("testpassword"),
 		})
+		require.NoError(t, err)
 
-	t.Run("can't create dbo from dbx model with invalid ID", func(t *testing.T) {
-		dbxUser := dbx.User{
-			Id:           []byte("qweqwe"),
-			FullName:     "Very long full name",
-			ShortName:    nil,
-			Email:        "some@mail.test",
-			PasswordHash: []byte("ihqerfgnu238723huagsd"),
-			CreatedAt:    time.Now(),
-		}
+		u, err := users.Get(ctx, id)
+		require.NoError(t, err)
+		require.Equal(t, console.UserStatus(0), u.Status)
 
-		user, err := userFromDBX(ctx, &dbxUser)
+		now := time.Now()
+		reminders := now.Add(time.Hour)
 
-		assert.Nil(t, user)
-		assert.Error(t, err)
+		// to get a reminder, created_at needs be after cutoff.
+		// since we don't have control over created_at, make cutoff in the future to test that
+		// user doesn't get a reminder.
+		cutoff := now.Add(time.Hour)
+
+		needingReminder, err := users.GetUnverifiedNeedingReminder(ctx, reminders, reminders, cutoff)
+		require.NoError(t, err)
+		require.Len(t, needingReminder, 0)
+
+		// change cutoff so user created_at is after it.
+		// user should get a reminder.
+		cutoff = now.Add(-time.Hour)
+
+		needingReminder, err = users.GetUnverifiedNeedingReminder(ctx, now, now, cutoff)
+		require.NoError(t, err)
+		require.Len(t, needingReminder, 1)
 	})
 }