[bugfix] Rework notifs to use min_id for paging up (#1734)

3 files changed, 61 insertions, 20 deletions

diff --git a/internal/db/bundb/notification.go b/internal/db/bundb/notification.go
index 1cc286f44..f2ff60b9a 100644
--- a/internal/db/bundb/notification.go
+++ b/internal/db/bundb/notification.go
@@ -23,6 +23,7 @@ import (
 
 	"github.com/superseriousbusiness/gotosocial/internal/db"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+	"github.com/superseriousbusiness/gotosocial/internal/id"
 	"github.com/superseriousbusiness/gotosocial/internal/log"
 	"github.com/superseriousbusiness/gotosocial/internal/state"
 	"github.com/uptrace/bun"
@@ -73,53 +74,93 @@ func (n *notificationDB) GetNotification(
 	}, notificationType, targetAccountID, originAccountID, statusID)
 }
 
-func (n *notificationDB) GetAccountNotifications(ctx context.Context, accountID string, excludeTypes []string, limit int, maxID string, sinceID string) ([]*gtsmodel.Notification, db.Error) {
+func (n *notificationDB) GetAccountNotifications(
+	ctx context.Context,
+	accountID string,
+	maxID string,
+	sinceID string,
+	minID string,
+	limit int,
+	excludeTypes []string,
+) ([]*gtsmodel.Notification, db.Error) {
 	// Ensure reasonable
 	if limit < 0 {
 		limit = 0
 	}
 
-	// Make a guess for slice size
-	notifIDs := make([]string, 0, limit)
+	// Make educated guess for slice size
+	var (
+		notifIDs    = make([]string, 0, limit)
+		frontToBack = true
+	)
 
 	q := n.conn.
 		NewSelect().
 		TableExpr("? AS ?", bun.Ident("notifications"), bun.Ident("notification")).
 		Column("notification.id")
 
-	if maxID != "" {
-		q = q.Where("? < ?", bun.Ident("notification.id"), maxID)
+	if maxID == "" {
+		maxID = id.Highest
 	}
 
+	// Return only notifs LOWER (ie., older) than maxID.
+	q = q.Where("? < ?", bun.Ident("notification.id"), maxID)
+
 	if sinceID != "" {
+		// Return only notifs HIGHER (ie., newer) than sinceID.
 		q = q.Where("? > ?", bun.Ident("notification.id"), sinceID)
 	}
 
+	if minID != "" {
+		// Return only notifs HIGHER (ie., newer) than minID.
+		q = q.Where("? > ?", bun.Ident("notification.id"), minID)
+
+		frontToBack = false // page up
+	}
+
 	for _, excludeType := range excludeTypes {
+		// Filter out unwanted notif types.
 		q = q.Where("? != ?", bun.Ident("notification.notification_type"), excludeType)
 	}
 
-	q = q.
-		Where("? = ?", bun.Ident("notification.target_account_id"), accountID).
-		Order("notification.id DESC")
+	// Return only notifs for this account.
+	q = q.Where("? = ?", bun.Ident("notification.target_account_id"), accountID)
 
-	if limit != 0 {
+	if limit > 0 {
 		q = q.Limit(limit)
 	}
 
+	if frontToBack {
+		// Page down.
+		q = q.Order("notification.id DESC")
+	} else {
+		// Page up.
+		q = q.Order("notification.id ASC")
+	}
+
 	if err := q.Scan(ctx, &notifIDs); err != nil {
 		return nil, n.conn.ProcessError(err)
 	}
 
-	notifs := make([]*gtsmodel.Notification, 0, limit)
+	if len(notifIDs) == 0 {
+		return nil, nil
+	}
+
+	// If we're paging up, we still want notifications
+	// to be sorted by ID desc, so reverse ids slice.
+	// https://zchee.github.io/golang-wiki/SliceTricks/#reversing
+	if !frontToBack {
+		for l, r := 0, len(notifIDs)-1; l < r; l, r = l+1, r-1 {
+			notifIDs[l], notifIDs[r] = notifIDs[r], notifIDs[l]
+		}
+	}
 
-	// now we have the IDs, select the notifs one by one
-	// reason for this is that for each notif, we can instead get it from our cache if it's cached
+	notifs := make([]*gtsmodel.Notification, 0, len(notifIDs))
 	for _, id := range notifIDs {
 		// Attempt fetch from DB
 		notif, err := n.GetNotificationByID(ctx, id)
 		if err != nil {
-			log.Errorf(ctx, "error getting notification %q: %v", id, err)
+			log.Errorf(ctx, "error fetching notification %q: %v", id, err)
 			continue
 		}
 
diff --git a/internal/db/bundb/notification_test.go b/internal/db/bundb/notification_test.go
index bdee911b3..8bbea78ad 100644
--- a/internal/db/bundb/notification_test.go
+++ b/internal/db/bundb/notification_test.go
@@ -89,7 +89,7 @@ func (suite *NotificationTestSuite) TestGetAccountNotificationsWithSpam() {
 	suite.spamNotifs()
 	testAccount := suite.testAccounts["local_account_1"]
 	before := time.Now()
-	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, []string{}, 20, id.Highest, id.Lowest)
+	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, id.Highest, id.Lowest, "", 20, nil)
 	suite.NoError(err)
 	timeTaken := time.Since(before)
 	fmt.Printf("\n\n\n withSpam: got %d notifications in %s\n\n\n", len(notifications), timeTaken)
@@ -103,7 +103,7 @@ func (suite *NotificationTestSuite) TestGetAccountNotificationsWithSpam() {
 func (suite *NotificationTestSuite) TestGetAccountNotificationsWithoutSpam() {
 	testAccount := suite.testAccounts["local_account_1"]
 	before := time.Now()
-	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, []string{}, 20, id.Highest, id.Lowest)
+	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, id.Highest, id.Lowest, "", 20, nil)
 	suite.NoError(err)
 	timeTaken := time.Since(before)
 	fmt.Printf("\n\n\n withoutSpam: got %d notifications in %s\n\n\n", len(notifications), timeTaken)
@@ -120,9 +120,9 @@ func (suite *NotificationTestSuite) TestDeleteNotificationsWithSpam() {
 	err := suite.db.DeleteNotifications(context.Background(), nil, testAccount.ID, "")
 	suite.NoError(err)
 
-	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, []string{}, 20, id.Highest, id.Lowest)
+	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, id.Highest, id.Lowest, "", 20, nil)
 	suite.NoError(err)
-	suite.NotNil(notifications)
+	suite.Nil(notifications)
 	suite.Empty(notifications)
 }
 
@@ -132,9 +132,9 @@ func (suite *NotificationTestSuite) TestDeleteNotificationsWithTwoAccounts() {
 	err := suite.db.DeleteNotifications(context.Background(), nil, testAccount.ID, "")
 	suite.NoError(err)
 
-	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, []string{}, 20, id.Highest, id.Lowest)
+	notifications, err := suite.db.GetAccountNotifications(context.Background(), testAccount.ID, id.Highest, id.Lowest, "", 20, nil)
 	suite.NoError(err)
-	suite.NotNil(notifications)
+	suite.Nil(notifications)
 	suite.Empty(notifications)
 
 	notif := []*gtsmodel.Notification{}
diff --git a/internal/db/notification.go b/internal/db/notification.go
index c4860093a..c17cf3d93 100644
--- a/internal/db/notification.go
+++ b/internal/db/notification.go
@@ -28,7 +28,7 @@ type Notification interface {
 	// GetNotifications returns a slice of notifications that pertain to the given accountID.
 	//
 	// Returned notifications will be ordered ID descending (ie., highest/newest to lowest/oldest).
-	GetAccountNotifications(ctx context.Context, accountID string, excludeTypes []string, limit int, maxID string, sinceID string) ([]*gtsmodel.Notification, Error)
+	GetAccountNotifications(ctx context.Context, accountID string, maxID string, sinceID string, minID string, limit int, excludeTypes []string) ([]*gtsmodel.Notification, Error)
 
 	// GetNotification returns one notification according to its id.
 	GetNotificationByID(ctx context.Context, id string) (*gtsmodel.Notification, Error)