From 66a621c54133679d17e04f6c57f238fcfcc014d4 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 21 Feb 2020 14:46:45 +0100 Subject: [PATCH 1/9] refactor --- models/issue_watch.go | 10 +++---- models/notification.go | 64 ++++++++++++++++++------------------------ models/repo_watch.go | 6 +++- 3 files changed, 38 insertions(+), 42 deletions(-) diff --git a/models/issue_watch.go b/models/issue_watch.go index c4732d784e181..3073b0a8879ae 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -68,10 +68,13 @@ 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"). Where("issue_id=?", issueID). - And("is_watching = ?", true). + And("is_watching = ?", watching). Select("user_id"). Find(&ids) } @@ -99,14 +102,11 @@ 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 } diff --git a/models/notification.go b/models/notification.go index e7217a6e04793..34d2214c4b0a7 100644 --- a/models/notification.go +++ b/models/notification.go @@ -133,55 +133,40 @@ 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{}, 50) + 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 - } + // 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 +174,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 } } diff --git a/models/repo_watch.go b/models/repo_watch.go index a9d56eff03d3e..11cfa88918447 100644 --- a/models/repo_watch.go +++ b/models/repo_watch.go @@ -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"). From cef8ef84a6f4952d8b376f0de74e0425f507b918 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 21 Feb 2020 14:53:54 +0100 Subject: [PATCH 2/9] optimize --- models/notification.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/models/notification.go b/models/notification.go index 34d2214c4b0a7..c52d6c557a5d6 100644 --- a/models/notification.go +++ b/models/notification.go @@ -134,7 +134,7 @@ func CreateOrUpdateIssueNotifications(issueID, commentID int64, notificationAuth func createOrUpdateIssueNotifications(e Engine, issueID, commentID int64, notificationAuthorID int64) error { // init - toNotify := make(map[int64]struct{}, 50) + toNotify := make(map[int64]struct{}, 32) notifications, err := getNotificationsByIssueID(e, issueID) if err != nil { return err @@ -160,6 +160,8 @@ func createOrUpdateIssueNotifications(e Engine, issueID, commentID int64, notifi toNotify[id] = struct{}{} } + // dont notify user who cause notification + delete(toNotify, notificationAuthorID) // explicit unwatch on issue issueUnWatches, err := getIssueWatchersIDs(e, issueID, false) if err != nil { From b0db0883c644e35e1142167b18b5667bcd861585 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 23 Feb 2020 00:51:34 +0100 Subject: [PATCH 3/9] remove Iretating function LoadWatchUsers do not load Users into IW object and it is used only in api ... so move this logic --- models/issue_watch.go | 26 ----------------------- models/user.go | 2 +- routers/api/v1/repo/issue_subscription.go | 9 ++++++-- 3 files changed, 8 insertions(+), 29 deletions(-) diff --git a/models/issue_watch.go b/models/issue_watch.go index 3073b0a8879ae..9588d24cc70ac 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -109,29 +109,3 @@ func removeIssueWatchersByRepoID(e Engine, userID int64, repoID int64) error { 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 -} diff --git a/models/user.go b/models/user.go index bf59c1240bc7b..8be15ba6df8dd 100644 --- a/models/user.go +++ b/models/user.go @@ -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 diff --git a/routers/api/v1/repo/issue_subscription.go b/routers/api/v1/repo/issue_subscription.go index 274da966fda8a..0406edd207844 100644 --- a/routers/api/v1/repo/issue_subscription.go +++ b/routers/api/v1/repo/issue_subscription.go @@ -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 } From a904f4b2056f64c0159801e658e7084f9ad09c3f Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 23 Feb 2020 03:46:17 +0100 Subject: [PATCH 4/9] remove unessesary --- models/issue_watch.go | 1 - 1 file changed, 1 deletion(-) diff --git a/models/issue_watch.go b/models/issue_watch.go index 9588d24cc70ac..d2e14d772061b 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -104,7 +104,6 @@ func getIssueWatchers(e Engine, issueID int64, listOptions ListOptions) (IssueWa func removeIssueWatchersByRepoID(e Engine, userID int64, repoID int64) error { _, 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). Delete(new(IssueWatch)) return err From b7d08fed8a0cb9e7bf81fca1bd17c6235cfe2dce Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 24 Feb 2020 03:09:35 +0100 Subject: [PATCH 5/9] Apply suggestions from code review Thx Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com> --- models/issue_watch.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/issue_watch.go b/models/issue_watch.go index d2e14d772061b..539acca6cb4ec 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -70,6 +70,7 @@ func getIssueWatch(e Engine, userID, issueID int64) (iw *IssueWatch, exists bool 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"). From 99867f3505d9e00dd40da7b471b6b0a1382fd370 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 24 Feb 2020 20:11:39 +0100 Subject: [PATCH 6/9] make Tests more robust --- integrations/repofiles_update_test.go | 13 ++++++++----- modules/git/repo_branch.go | 3 +++ modules/test/context_tests.go | 7 +++++-- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/integrations/repofiles_update_test.go b/integrations/repofiles_update_test.go index a7beec49553b3..c422483bf8754 100644 --- a/integrations/repofiles_update_test.go +++ b/integrations/repofiles_update_test.go @@ -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) + } }) } diff --git a/modules/git/repo_branch.go b/modules/git/repo_branch.go index e79bab76a6f30..3d0e6497edcf0 100644 --- a/modules/git/repo_branch.go +++ b/modules/git/repo_branch.go @@ -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 diff --git a/modules/test/context_tests.go b/modules/test/context_tests.go index cf9c5fbc548ad..f9f0ec5d42c93 100644 --- a/modules/test/context_tests.go +++ b/modules/test/context_tests.go @@ -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. From 029bfe24914df68b92fe9fd95ebe543117b5560b Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 25 Feb 2020 12:14:12 +0100 Subject: [PATCH 7/9] fix rebase --- models/issue_watch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issue_watch.go b/models/issue_watch.go index 539acca6cb4ec..9046e4d2f7518 100644 --- a/models/issue_watch.go +++ b/models/issue_watch.go @@ -73,7 +73,7 @@ func GetIssueWatchersIDs(issueID int64) ([]int64, error) { 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 = ?", watching). Select("user_id"). From ec51e5d44bb7523fbb1902f2b0537393ad00a703 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 25 Feb 2020 12:54:31 +0100 Subject: [PATCH 8/9] restart CI From 22e636c91421554f82f683028ab11eaa55492ce8 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 25 Feb 2020 13:27:31 +0100 Subject: [PATCH 9/9] CI no dont hit sqlites deadlock