[MODERATION] Refactor excluding watchers mechanism (squash)

This solves two bugs. One bug is that due to the JOIN with the
`forgejo_blocked_users` table, duplicated users were generated if a user
had more than one user blocked, this lead to receiving more than one
entry in the actions table. The other bug is that if a user blocked more
than one user, it would still receive a action entry by a
blocked user, because the SQL query would not exclude the other
duplicated users that was generated by the JOIN.

The new solution is somewhat non-optimal in my eyes, but it's better
than rewriting the query to become a potential perfomance blocker (usage
of WHERE IN, which cannot be rewritten to a JOIN). It simply removes the
watchers after it was retrieved by the SQL query.

(cherry picked from commit c63c00b39b)

models/activities/action.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"net/url"
 	"path"
+	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -21,6 +22,7 @@ import (
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/base"
+	"code.gitea.io/gitea/modules/container"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
@@ -591,10 +593,25 @@ func NotifyWatchers(ctx context.Context, actions ...*Action) error {
 
 		if repoChanged {
 			// Add feeds for user self and all watchers.
-			watchers, err = repo_model.GetWatchersExcludeBlocked(ctx, act.RepoID, act.ActUserID)
+			watchers, err = repo_model.GetWatchers(ctx, act.RepoID)
 			if err != nil {
 				return fmt.Errorf("get watchers: %w", err)
 			}
+
+			// Be aware that optimizing this correctly into the `GetWatchers` SQL
+			// query is for most cases less performant than doing this.
+			blockedDoerUserIDs, err := user_model.ListBlockedByUsersID(ctx, act.ActUserID)
+			if err != nil {
+				return fmt.Errorf("user_model.ListBlockedByUsersID: %w", err)
+			}
+
+			if len(blockedDoerUserIDs) > 0 {
+				excludeWatcherIDs := make(container.Set[int64], len(blockedDoerUserIDs))
+				excludeWatcherIDs.AddMultiple(blockedDoerUserIDs...)
+				watchers = slices.DeleteFunc(watchers, func(v *repo_model.Watch) bool {
+					return excludeWatcherIDs.Contains(v.UserID)
+				})
+			}
 		}
 
 		// Add feed for actioner.

models/repo/watch.go

@@ -10,8 +10,6 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
-
-	"xorm.io/builder"
 )
 
 // WatchMode specifies what kind of watch the user has on a repository
@@ -131,18 +129,14 @@ func WatchRepo(ctx context.Context, userID, repoID int64, doWatch bool) (err err
 	return err
 }
 
-// GetWatchersExcludeBlocked returns all watchers of given repository, whereby
+// GetWatchers returns all watchers of given repository.
-// the doer isn't blocked by one of the watchers.
+func GetWatchers(ctx context.Context, repoID int64) ([]*Watch, error) {
-func GetWatchersExcludeBlocked(ctx context.Context, repoID, doerID int64) ([]*Watch, error) {
 	watches := make([]*Watch, 0, 10)
-	return watches, db.GetEngine(ctx).
+	return watches, db.GetEngine(ctx).Where("`watch`.repo_id=?", repoID).
-		Join("INNER", "`user`", "`user`.id = `watch`.user_id").
-		Join("LEFT", "forgejo_blocked_user", "forgejo_blocked_user.user_id = `watch`.user_id").
-		Where("`watch`.repo_id=?", repoID).
 		And("`watch`.mode<>?", WatchModeDont).
 		And("`user`.is_active=?", true).
 		And("`user`.prohibit_login=?", false).
-		And(builder.Or(builder.IsNull{"`forgejo_blocked_user`.block_id"}, builder.Neq{"`forgejo_blocked_user`.block_id": doerID})).
+		Join("INNER", "`user`", "`user`.id = `watch`.user_id").
 		Find(&watches)
 }
 

models/repo/watch_test.go

@@ -26,20 +26,19 @@ func TestIsWatching(t *testing.T) {
 	assert.False(t, repo_model.IsWatching(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID))
 }
 
-func TestGetWatchersExcludeBlocked(t *testing.T) {
+func TestGetWatchers(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
-	watches, err := repo_model.GetWatchersExcludeBlocked(db.DefaultContext, repo.ID, 1)
+	watches, err := repo_model.GetWatchers(db.DefaultContext, repo.ID)
 	assert.NoError(t, err)
-
+	// One watchers are inactive, thus minus 1
-	// One watchers are inactive and one watcher is blocked, thus minus 2
+	assert.Len(t, watches, repo.NumWatches-1)
-	assert.Len(t, watches, repo.NumWatches-2)
 	for _, watch := range watches {
 		assert.EqualValues(t, repo.ID, watch.RepoID)
 	}
 
-	watches, err = repo_model.GetWatchersExcludeBlocked(db.DefaultContext, unittest.NonexistentID, 1)
+	watches, err = repo_model.GetWatchers(db.DefaultContext, unittest.NonexistentID)
 	assert.NoError(t, err)
 	assert.Len(t, watches, 0)
 }

tests/integration/block_test.go

@@ -11,6 +11,8 @@ import (
 	"strconv"
 	"testing"
 
+	"code.gitea.io/gitea/models/activities"
+	"code.gitea.io/gitea/models/db"
 	issue_model "code.gitea.io/gitea/models/issues"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
@@ -390,3 +392,45 @@ func TestBlockActions(t *testing.T) {
 		)
 	})
 }
+
+func TestBlockedNotification(t *testing.T) {
+	defer tests.AddFixtures("tests/integration/fixtures/TestBlockedNotifications")()
+	defer tests.PrepareTestEnv(t)()
+
+	doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+	normalUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+	blockedUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 10})
+	issue := unittest.AssertExistsAndLoadBean(t, &issue_model.Issue{ID: 1000})
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID})
+	issueURL := fmt.Sprintf("%s/issues/%d", repo.FullName(), issue.Index)
+	notificationBean := &activities.Notification{UserID: doer.ID, RepoID: repo.ID, IssueID: issue.ID}
+
+	assert.False(t, user_model.IsBlocked(db.DefaultContext, doer.ID, normalUser.ID))
+	BlockUser(t, doer, blockedUser)
+
+	mentionDoer := func(t *testing.T, session *TestSession) {
+		t.Helper()
+
+		req := NewRequestWithValues(t, "POST", issueURL+"/comments", map[string]string{
+			"_csrf":   GetCSRF(t, session, issueURL),
+			"content": "I'm annoying. Pinging @" + doer.Name,
+		})
+		session.MakeRequest(t, req, http.StatusOK)
+	}
+
+	t.Run("Blocks notification of blocked user", func(t *testing.T) {
+		session := loginUser(t, blockedUser.Name)
+
+		unittest.AssertNotExistsBean(t, notificationBean)
+		mentionDoer(t, session)
+		unittest.AssertNotExistsBean(t, notificationBean)
+	})
+
+	t.Run("Do not block notifications of normal user", func(t *testing.T) {
+		session := loginUser(t, normalUser.Name)
+
+		unittest.AssertNotExistsBean(t, notificationBean)
+		mentionDoer(t, session)
+		unittest.AssertExistsAndLoadBean(t, notificationBean)
+	})
+}

tests/integration/fixtures/TestBlockedNotifications/issue.yml

@@ -0,0 +1,16 @@
+-
+  id: 1000
+  repo_id: 4
+  index: 1000
+  poster_id: 10
+  original_author_id: 0
+  name: issue for moderation
+  content: Hello there!
+  milestone_id: 0
+  priority: 0
+  is_closed: false
+  is_pull: false
+  num_comments: 0
+  created_unix: 1705939088
+  updated_unix: 1705939088
+  is_locked: false