Code Refactor of IssueWatch related things (#10401)
* refactor * optimize * remove Iretating function LoadWatchUsers do not load Users into IW object and it is used only in api ... so move this logic * remove unessesary * Apply suggestions from code review Thx Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com> * make Tests more robust * fix rebase * restart CI * CI no dont hit sqlites deadlock Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>
This commit is contained in:
parent
e5944a9521
commit
084a2b0026
8 changed files with 66 additions and 80 deletions
|
@ -207,11 +207,14 @@ func TestCreateOrUpdateRepoFileForCreate(t *testing.T) {
|
|||
|
||||
commitID, _ := gitRepo.GetBranchCommitID(opts.NewBranch)
|
||||
expectedFileResponse := getExpectedFileResponseForRepofilesCreate(commitID)
|
||||
assert.EqualValues(t, expectedFileResponse.Content, fileResponse.Content)
|
||||
assert.EqualValues(t, expectedFileResponse.Commit.SHA, fileResponse.Commit.SHA)
|
||||
assert.EqualValues(t, expectedFileResponse.Commit.HTMLURL, fileResponse.Commit.HTMLURL)
|
||||
assert.EqualValues(t, expectedFileResponse.Commit.Author.Email, fileResponse.Commit.Author.Email)
|
||||
assert.EqualValues(t, expectedFileResponse.Commit.Author.Name, fileResponse.Commit.Author.Name)
|
||||
assert.NotNil(t, expectedFileResponse)
|
||||
if expectedFileResponse != nil {
|
||||
assert.EqualValues(t, expectedFileResponse.Content, fileResponse.Content)
|
||||
assert.EqualValues(t, expectedFileResponse.Commit.SHA, fileResponse.Commit.SHA)
|
||||
assert.EqualValues(t, expectedFileResponse.Commit.HTMLURL, fileResponse.Commit.HTMLURL)
|
||||
assert.EqualValues(t, expectedFileResponse.Commit.Author.Email, fileResponse.Commit.Author.Email)
|
||||
assert.EqualValues(t, expectedFileResponse.Commit.Author.Name, fileResponse.Commit.Author.Name)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
@ -68,10 +68,14 @@ func getIssueWatch(e Engine, userID, issueID int64) (iw *IssueWatch, exists bool
|
|||
// but avoids joining with `user` for performance reasons
|
||||
// User permissions must be verified elsewhere if required
|
||||
func GetIssueWatchersIDs(issueID int64) ([]int64, error) {
|
||||
return getIssueWatchersIDs(x, issueID, true)
|
||||
}
|
||||
|
||||
func getIssueWatchersIDs(e Engine, issueID int64, watching bool) ([]int64, error) {
|
||||
ids := make([]int64, 0, 64)
|
||||
return ids, x.Table("issue_watch").
|
||||
return ids, e.Table("issue_watch").
|
||||
Where("issue_id=?", issueID).
|
||||
And("is_watching = ?", true).
|
||||
And("is_watching = ?", watching).
|
||||
Select("user_id").
|
||||
Find(&ids)
|
||||
}
|
||||
|
@ -99,39 +103,9 @@ func getIssueWatchers(e Engine, issueID int64, listOptions ListOptions) (IssueWa
|
|||
}
|
||||
|
||||
func removeIssueWatchersByRepoID(e Engine, userID int64, repoID int64) error {
|
||||
iw := &IssueWatch{
|
||||
IsWatching: false,
|
||||
}
|
||||
_, err := e.
|
||||
Join("INNER", "issue", "`issue`.id = `issue_watch`.issue_id AND `issue`.repo_id = ?", repoID).
|
||||
Cols("is_watching", "updated_unix").
|
||||
Where("`issue_watch`.user_id = ?", userID).
|
||||
Update(iw)
|
||||
Delete(new(IssueWatch))
|
||||
return err
|
||||
}
|
||||
|
||||
// LoadWatchUsers return watching users
|
||||
func (iwl IssueWatchList) LoadWatchUsers() (users UserList, err error) {
|
||||
return iwl.loadWatchUsers(x)
|
||||
}
|
||||
|
||||
func (iwl IssueWatchList) loadWatchUsers(e Engine) (users UserList, err error) {
|
||||
if len(iwl) == 0 {
|
||||
return []*User{}, nil
|
||||
}
|
||||
|
||||
var userIDs = make([]int64, 0, len(iwl))
|
||||
for _, iw := range iwl {
|
||||
if iw.IsWatching {
|
||||
userIDs = append(userIDs, iw.UserID)
|
||||
}
|
||||
}
|
||||
|
||||
if len(userIDs) == 0 {
|
||||
return []*User{}, nil
|
||||
}
|
||||
|
||||
err = e.In("id", userIDs).Find(&users)
|
||||
|
||||
return
|
||||
}
|
||||
|
|
|
@ -133,55 +133,42 @@ func CreateOrUpdateIssueNotifications(issueID, commentID int64, notificationAuth
|
|||
}
|
||||
|
||||
func createOrUpdateIssueNotifications(e Engine, issueID, commentID int64, notificationAuthorID int64) error {
|
||||
issueWatches, err := getIssueWatchers(e, issueID, ListOptions{})
|
||||
// init
|
||||
toNotify := make(map[int64]struct{}, 32)
|
||||
notifications, err := getNotificationsByIssueID(e, issueID)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
issue, err := getIssueByID(e, issueID)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
watches, err := getWatchers(e, issue.RepoID)
|
||||
issueWatches, err := getIssueWatchersIDs(e, issueID, true)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
for _, id := range issueWatches {
|
||||
toNotify[id] = struct{}{}
|
||||
}
|
||||
|
||||
notifications, err := getNotificationsByIssueID(e, issueID)
|
||||
repoWatches, err := getRepoWatchersIDs(e, issue.RepoID)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
alreadyNotified := make(map[int64]struct{}, len(issueWatches)+len(watches))
|
||||
|
||||
notifyUser := func(userID int64) error {
|
||||
// do not send notification for the own issuer/commenter
|
||||
if userID == notificationAuthorID {
|
||||
return nil
|
||||
}
|
||||
|
||||
if _, ok := alreadyNotified[userID]; ok {
|
||||
return nil
|
||||
}
|
||||
alreadyNotified[userID] = struct{}{}
|
||||
|
||||
if notificationExists(notifications, issue.ID, userID) {
|
||||
return updateIssueNotification(e, userID, issue.ID, commentID, notificationAuthorID)
|
||||
}
|
||||
return createIssueNotification(e, userID, issue, commentID, notificationAuthorID)
|
||||
for _, id := range repoWatches {
|
||||
toNotify[id] = struct{}{}
|
||||
}
|
||||
|
||||
for _, issueWatch := range issueWatches {
|
||||
// ignore if user unwatched the issue
|
||||
if !issueWatch.IsWatching {
|
||||
alreadyNotified[issueWatch.UserID] = struct{}{}
|
||||
continue
|
||||
}
|
||||
|
||||
if err := notifyUser(issueWatch.UserID); err != nil {
|
||||
return err
|
||||
}
|
||||
// dont notify user who cause notification
|
||||
delete(toNotify, notificationAuthorID)
|
||||
// explicit unwatch on issue
|
||||
issueUnWatches, err := getIssueWatchersIDs(e, issueID, false)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
for _, id := range issueUnWatches {
|
||||
delete(toNotify, id)
|
||||
}
|
||||
|
||||
err = issue.loadRepo(e)
|
||||
|
@ -189,16 +176,23 @@ func createOrUpdateIssueNotifications(e Engine, issueID, commentID int64, notifi
|
|||
return err
|
||||
}
|
||||
|
||||
for _, watch := range watches {
|
||||
// notify
|
||||
for userID := range toNotify {
|
||||
issue.Repo.Units = nil
|
||||
if issue.IsPull && !issue.Repo.checkUnitUser(e, watch.UserID, false, UnitTypePullRequests) {
|
||||
if issue.IsPull && !issue.Repo.checkUnitUser(e, userID, false, UnitTypePullRequests) {
|
||||
continue
|
||||
}
|
||||
if !issue.IsPull && !issue.Repo.checkUnitUser(e, watch.UserID, false, UnitTypeIssues) {
|
||||
if !issue.IsPull && !issue.Repo.checkUnitUser(e, userID, false, UnitTypeIssues) {
|
||||
continue
|
||||
}
|
||||
|
||||
if err := notifyUser(watch.UserID); err != nil {
|
||||
if notificationExists(notifications, issue.ID, userID) {
|
||||
if err = updateIssueNotification(e, userID, issue.ID, commentID, notificationAuthorID); err != nil {
|
||||
return err
|
||||
}
|
||||
continue
|
||||
}
|
||||
if err = createIssueNotification(e, userID, issue, commentID, notificationAuthorID); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
|
|
@ -144,8 +144,12 @@ func GetWatchers(repoID int64) ([]*Watch, error) {
|
|||
// but avoids joining with `user` for performance reasons
|
||||
// User permissions must be verified elsewhere if required
|
||||
func GetRepoWatchersIDs(repoID int64) ([]int64, error) {
|
||||
return getRepoWatchersIDs(x, repoID)
|
||||
}
|
||||
|
||||
func getRepoWatchersIDs(e Engine, repoID int64) ([]int64, error) {
|
||||
ids := make([]int64, 0, 64)
|
||||
return ids, x.Table("watch").
|
||||
return ids, e.Table("watch").
|
||||
Where("watch.repo_id=?", repoID).
|
||||
And("watch.mode<>?", RepoWatchModeDont).
|
||||
Select("user_id").
|
||||
|
|
|
@ -1409,7 +1409,7 @@ func GetUserNamesByIDs(ids []int64) ([]string, error) {
|
|||
}
|
||||
|
||||
// GetUsersByIDs returns all resolved users from a list of Ids.
|
||||
func GetUsersByIDs(ids []int64) ([]*User, error) {
|
||||
func GetUsersByIDs(ids []int64) (UserList, error) {
|
||||
ous := make([]*User, 0, len(ids))
|
||||
if len(ids) == 0 {
|
||||
return ous, nil
|
||||
|
|
|
@ -48,6 +48,9 @@ type Branch struct {
|
|||
|
||||
// GetHEADBranch returns corresponding branch of HEAD.
|
||||
func (repo *Repository) GetHEADBranch() (*Branch, error) {
|
||||
if repo == nil {
|
||||
return nil, fmt.Errorf("nil repo")
|
||||
}
|
||||
stdout, err := NewCommand("symbolic-ref", "HEAD").RunInDir(repo.Path)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
|
|
|
@ -58,8 +58,11 @@ func LoadRepoCommit(t *testing.T, ctx *context.Context) {
|
|||
defer gitRepo.Close()
|
||||
branch, err := gitRepo.GetHEADBranch()
|
||||
assert.NoError(t, err)
|
||||
ctx.Repo.Commit, err = gitRepo.GetBranchCommit(branch.Name)
|
||||
assert.NoError(t, err)
|
||||
assert.NotNil(t, branch)
|
||||
if branch != nil {
|
||||
ctx.Repo.Commit, err = gitRepo.GetBranchCommit(branch.Name)
|
||||
assert.NoError(t, err)
|
||||
}
|
||||
}
|
||||
|
||||
// LoadUser load a user into a test context.
|
||||
|
|
|
@ -190,9 +190,14 @@ func GetIssueSubscribers(ctx *context.APIContext) {
|
|||
return
|
||||
}
|
||||
|
||||
users, err := iwl.LoadWatchUsers()
|
||||
var userIDs = make([]int64, 0, len(iwl))
|
||||
for _, iw := range iwl {
|
||||
userIDs = append(userIDs, iw.UserID)
|
||||
}
|
||||
|
||||
users, err := models.GetUsersByIDs(userIDs)
|
||||
if err != nil {
|
||||
ctx.Error(http.StatusInternalServerError, "LoadWatchUsers", err)
|
||||
ctx.Error(http.StatusInternalServerError, "GetUsersByIDs", err)
|
||||
return
|
||||
}
|
||||
|
||||
|
|
Reference in a new issue