mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2024-12-23 07:43:18 -05:00
[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 c63c00b39b
)
This commit is contained in:
parent
f6cf951b5d
commit
3d3790ef4c
5 changed files with 78 additions and 36 deletions
|
@ -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.
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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())
|
||||
|
||||
|
|
|
@ -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)
|
||||
})
|
||||
}
|
||||
|
|
|
@ -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
|
Loading…
Reference in a new issue