From c63c00b39b8bd2ed3a69ed044933a9626bfca2c1 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 15 Jan 2024 00:22:06 +0100 Subject: [PATCH] [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. --- models/activities/action.go | 19 +++++++- models/repo/watch.go | 14 ++---- models/repo/watch_test.go | 11 +++-- tests/integration/block_test.go | 44 +++++++++++++++++++ .../TestBlockedNotifications/issue.yml | 16 +++++++ 5 files changed, 87 insertions(+), 17 deletions(-) create mode 100644 tests/integration/fixtures/TestBlockedNotifications/issue.yml diff --git a/models/activities/action.go b/models/activities/action.go index f7aa0af3af..37ef1cc48c 100644 --- a/models/activities/action.go +++ b/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. diff --git a/models/repo/watch.go b/models/repo/watch.go index 6f96254b60..6974d893df 100644 --- a/models/repo/watch.go +++ b/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 -// the doer isn't blocked by one of the watchers. -func GetWatchersExcludeBlocked(ctx context.Context, repoID, doerID int64) ([]*Watch, error) { +// GetWatchers returns all watchers of given repository. +func GetWatchers(ctx context.Context, repoID int64) ([]*Watch, error) { watches := make([]*Watch, 0, 10) - return watches, db.GetEngine(ctx). - 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). + return watches, db.GetEngine(ctx).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) } diff --git a/models/repo/watch_test.go b/models/repo/watch_test.go index c9e9915c92..4dd9234f3b 100644 --- a/models/repo/watch_test.go +++ b/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 and one watcher is blocked, thus minus 2 - assert.Len(t, watches, repo.NumWatches-2) + // One watchers are inactive, thus minus 1 + assert.Len(t, watches, repo.NumWatches-1) 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) } diff --git a/tests/integration/block_test.go b/tests/integration/block_test.go index 4f8249f8cc..96e9d5d3e3 100644 --- a/tests/integration/block_test.go +++ b/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) + }) +} diff --git a/tests/integration/fixtures/TestBlockedNotifications/issue.yml b/tests/integration/fixtures/TestBlockedNotifications/issue.yml new file mode 100644 index 0000000000..9524e60be3 --- /dev/null +++ b/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