From 3d3790ef4c6cdbcbe0cf7ec80627596f44701977 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) Backport of #2143 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 c63c00b39b8bd2ed3a69ed044933a9626bfca2c1) --- models/activities/action.go | 19 +++++++- models/repo/watch.go | 17 ------- models/repo/watch_test.go | 18 -------- tests/integration/block_test.go | 44 +++++++++++++++++++ .../TestBlockedNotifications/issue.yml | 16 +++++++ 5 files changed, 78 insertions(+), 36 deletions(-) create mode 100644 tests/integration/fixtures/TestBlockedNotifications/issue.yml diff --git a/models/activities/action.go b/models/activities/action.go index fe1642641c..65bed1b583 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 ce07bfc0ab..e415d0e394 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 @@ -144,21 +142,6 @@ func GetWatchers(ctx context.Context, repoID int64) ([]*Watch, error) { Find(&watches) } -// 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) { - 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). - 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})). - Find(&watches) -} - // GetRepoWatchersIDs returns IDs of watchers for a given repo ID // but avoids joining with `user` for performance reasons // User permissions must be verified elsewhere if required diff --git a/models/repo/watch_test.go b/models/repo/watch_test.go index 8b6ac450f2..ebe1b75a5d 100644 --- a/models/repo/watch_test.go +++ b/models/repo/watch_test.go @@ -43,24 +43,6 @@ func TestGetWatchers(t *testing.T) { assert.Len(t, watches, 0) } -func TestGetWatchersExcludeBlocked(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) - assert.NoError(t, err) - - // One watchers are inactive and one watcher is blocked, thus minus 2 - 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) - assert.NoError(t, err) - assert.Len(t, watches, 0) -} - func TestRepository_GetWatchers(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) diff --git a/tests/integration/block_test.go b/tests/integration/block_test.go index 70e3fc08a5..75722a835d 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" @@ -387,3 +389,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