diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 2e289b6f734..e5bfc2b881e 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -270,6 +270,8 @@ var migrations = []Migration{ NewMigration("add column `mode` to table watch", addModeColumnToWatch), // v107 -> v108 NewMigration("Add template options to repository", addTemplateToRepo), + // v108 -> v109 + NewMigration("Add comment_id on table notification", addCommentIDOnNotification), } // Migrate database to current version diff --git a/models/migrations/v108.go b/models/migrations/v108.go new file mode 100644 index 00000000000..60b8fb47aec --- /dev/null +++ b/models/migrations/v108.go @@ -0,0 +1,18 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "xorm.io/xorm" +) + +func addCommentIDOnNotification(x *xorm.Engine) error { + type Notification struct { + ID int64 `xorm:"pk autoincr"` + CommentID int64 + } + + return x.Sync2(new(Notification)) +} diff --git a/models/notification.go b/models/notification.go index 5b6ce597d15..c8203754f90 100644 --- a/models/notification.go +++ b/models/notification.go @@ -44,8 +44,10 @@ type Notification struct { Status NotificationStatus `xorm:"SMALLINT INDEX NOT NULL"` Source NotificationSource `xorm:"SMALLINT INDEX NOT NULL"` - IssueID int64 `xorm:"INDEX NOT NULL"` - CommitID string `xorm:"INDEX"` + IssueID int64 `xorm:"INDEX NOT NULL"` + CommitID string `xorm:"INDEX"` + CommentID int64 + Comment *Comment `xorm:"-"` UpdatedBy int64 `xorm:"INDEX NOT NULL"` @@ -58,22 +60,27 @@ type Notification struct { // CreateOrUpdateIssueNotifications creates an issue notification // for each watcher, or updates it if already exists -func CreateOrUpdateIssueNotifications(issue *Issue, notificationAuthorID int64) error { +func CreateOrUpdateIssueNotifications(issueID, commentID int64, notificationAuthorID int64) error { sess := x.NewSession() defer sess.Close() if err := sess.Begin(); err != nil { return err } - if err := createOrUpdateIssueNotifications(sess, issue, notificationAuthorID); err != nil { + if err := createOrUpdateIssueNotifications(sess, issueID, commentID, notificationAuthorID); err != nil { return err } return sess.Commit() } -func createOrUpdateIssueNotifications(e Engine, issue *Issue, notificationAuthorID int64) error { - issueWatches, err := getIssueWatchers(e, issue.ID) +func createOrUpdateIssueNotifications(e Engine, issueID, commentID int64, notificationAuthorID int64) error { + issueWatches, err := getIssueWatchers(e, issueID) + if err != nil { + return err + } + + issue, err := getIssueByID(e, issueID) if err != nil { return err } @@ -83,7 +90,7 @@ func createOrUpdateIssueNotifications(e Engine, issue *Issue, notificationAuthor return err } - notifications, err := getNotificationsByIssueID(e, issue.ID) + notifications, err := getNotificationsByIssueID(e, issueID) if err != nil { return err } @@ -102,9 +109,9 @@ func createOrUpdateIssueNotifications(e Engine, issue *Issue, notificationAuthor alreadyNotified[userID] = struct{}{} if notificationExists(notifications, issue.ID, userID) { - return updateIssueNotification(e, userID, issue.ID, notificationAuthorID) + return updateIssueNotification(e, userID, issue.ID, commentID, notificationAuthorID) } - return createIssueNotification(e, userID, issue, notificationAuthorID) + return createIssueNotification(e, userID, issue, commentID, notificationAuthorID) } for _, issueWatch := range issueWatches { @@ -157,12 +164,13 @@ func notificationExists(notifications []*Notification, issueID, userID int64) bo return false } -func createIssueNotification(e Engine, userID int64, issue *Issue, updatedByID int64) error { +func createIssueNotification(e Engine, userID int64, issue *Issue, commentID, updatedByID int64) error { notification := &Notification{ UserID: userID, RepoID: issue.RepoID, Status: NotificationStatusUnread, IssueID: issue.ID, + CommentID: commentID, UpdatedBy: updatedByID, } @@ -176,16 +184,25 @@ func createIssueNotification(e Engine, userID int64, issue *Issue, updatedByID i return err } -func updateIssueNotification(e Engine, userID, issueID, updatedByID int64) error { +func updateIssueNotification(e Engine, userID, issueID, commentID, updatedByID int64) error { notification, err := getIssueNotification(e, userID, issueID) if err != nil { return err } - notification.Status = NotificationStatusUnread - notification.UpdatedBy = updatedByID + // NOTICE: Only update comment id when the before notification on this issue is read, otherwise you may miss some old comments. + // But we need update update_by so that the notification will be reorder + var cols []string + if notification.Status == NotificationStatusRead { + notification.Status = NotificationStatusUnread + notification.CommentID = commentID + cols = []string{"status", "update_by", "comment_id"} + } else { + notification.UpdatedBy = updatedByID + cols = []string{"update_by"} + } - _, err = e.ID(notification.ID).Update(notification) + _, err = e.ID(notification.ID).Cols(cols...).Update(notification) return err } @@ -199,7 +216,7 @@ func getIssueNotification(e Engine, userID, issueID int64) (*Notification, error } // NotificationsForUser returns notifications for a given user and status -func NotificationsForUser(user *User, statuses []NotificationStatus, page, perPage int) ([]*Notification, error) { +func NotificationsForUser(user *User, statuses []NotificationStatus, page, perPage int) (NotificationList, error) { return notificationsForUser(x, user, statuses, page, perPage) } @@ -239,6 +256,204 @@ func (n *Notification) GetIssue() (*Issue, error) { return n.Issue, err } +// HTMLURL formats a URL-string to the notification +func (n *Notification) HTMLURL() string { + if n.Comment != nil { + return n.Comment.HTMLURL() + } + return n.Issue.HTMLURL() +} + +// NotificationList contains a list of notifications +type NotificationList []*Notification + +func (nl NotificationList) getPendingRepoIDs() []int64 { + var ids = make(map[int64]struct{}, len(nl)) + for _, notification := range nl { + if notification.Repository != nil { + continue + } + if _, ok := ids[notification.RepoID]; !ok { + ids[notification.RepoID] = struct{}{} + } + } + return keysInt64(ids) +} + +// LoadRepos loads repositories from database +func (nl NotificationList) LoadRepos() (RepositoryList, error) { + if len(nl) == 0 { + return RepositoryList{}, nil + } + + var repoIDs = nl.getPendingRepoIDs() + var repos = make(map[int64]*Repository, len(repoIDs)) + var left = len(repoIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + rows, err := x. + In("id", repoIDs[:limit]). + Rows(new(Repository)) + if err != nil { + return nil, err + } + + for rows.Next() { + var repo Repository + err = rows.Scan(&repo) + if err != nil { + rows.Close() + return nil, err + } + + repos[repo.ID] = &repo + } + _ = rows.Close() + + left -= limit + repoIDs = repoIDs[limit:] + } + + var reposList = make(RepositoryList, 0, len(repoIDs)) + for _, notification := range nl { + if notification.Repository == nil { + notification.Repository = repos[notification.RepoID] + } + var found bool + for _, r := range reposList { + if r.ID == notification.Repository.ID { + found = true + break + } + } + if !found { + reposList = append(reposList, notification.Repository) + } + } + return reposList, nil +} + +func (nl NotificationList) getPendingIssueIDs() []int64 { + var ids = make(map[int64]struct{}, len(nl)) + for _, notification := range nl { + if notification.Issue != nil { + continue + } + if _, ok := ids[notification.IssueID]; !ok { + ids[notification.IssueID] = struct{}{} + } + } + return keysInt64(ids) +} + +// LoadIssues loads issues from database +func (nl NotificationList) LoadIssues() error { + if len(nl) == 0 { + return nil + } + + var issueIDs = nl.getPendingIssueIDs() + var issues = make(map[int64]*Issue, len(issueIDs)) + var left = len(issueIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + rows, err := x. + In("id", issueIDs[:limit]). + Rows(new(Issue)) + if err != nil { + return err + } + + for rows.Next() { + var issue Issue + err = rows.Scan(&issue) + if err != nil { + rows.Close() + return err + } + + issues[issue.ID] = &issue + } + _ = rows.Close() + + left -= limit + issueIDs = issueIDs[limit:] + } + + for _, notification := range nl { + if notification.Issue == nil { + notification.Issue = issues[notification.IssueID] + notification.Issue.Repo = notification.Repository + } + } + return nil +} + +func (nl NotificationList) getPendingCommentIDs() []int64 { + var ids = make(map[int64]struct{}, len(nl)) + for _, notification := range nl { + if notification.CommentID == 0 || notification.Comment != nil { + continue + } + if _, ok := ids[notification.CommentID]; !ok { + ids[notification.CommentID] = struct{}{} + } + } + return keysInt64(ids) +} + +// LoadComments loads comments from database +func (nl NotificationList) LoadComments() error { + if len(nl) == 0 { + return nil + } + + var commentIDs = nl.getPendingCommentIDs() + var comments = make(map[int64]*Comment, len(commentIDs)) + var left = len(commentIDs) + for left > 0 { + var limit = defaultMaxInSize + if left < limit { + limit = left + } + rows, err := x. + In("id", commentIDs[:limit]). + Rows(new(Comment)) + if err != nil { + return err + } + + for rows.Next() { + var comment Comment + err = rows.Scan(&comment) + if err != nil { + rows.Close() + return err + } + + comments[comment.ID] = &comment + } + _ = rows.Close() + + left -= limit + commentIDs = commentIDs[limit:] + } + + for _, notification := range nl { + if notification.CommentID > 0 && notification.Comment == nil { + notification.Comment = comments[notification.CommentID] + notification.Comment.Issue = notification.Issue + } + } + return nil +} + // GetNotificationCount returns the notification count for user func GetNotificationCount(user *User, status NotificationStatus) (int64, error) { return getNotificationCount(x, user, status) diff --git a/models/notification_test.go b/models/notification_test.go index 01c960c9299..728be7182ca 100644 --- a/models/notification_test.go +++ b/models/notification_test.go @@ -14,7 +14,7 @@ func TestCreateOrUpdateIssueNotifications(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) issue := AssertExistsAndLoadBean(t, &Issue{ID: 1}).(*Issue) - assert.NoError(t, CreateOrUpdateIssueNotifications(issue, 2)) + assert.NoError(t, CreateOrUpdateIssueNotifications(issue.ID, 0, 2)) // User 9 is inactive, thus notifications for user 1 and 4 are created notf := AssertExistsAndLoadBean(t, &Notification{UserID: 1, IssueID: issue.ID}).(*Notification) diff --git a/modules/notification/ui/ui.go b/modules/notification/ui/ui.go index 22089158f5b..bfe497f866d 100644 --- a/modules/notification/ui/ui.go +++ b/modules/notification/ui/ui.go @@ -18,7 +18,8 @@ type ( } issueNotificationOpts struct { - issue *models.Issue + issueID int64 + commentID int64 notificationAuthorID int64 } ) @@ -36,7 +37,7 @@ func NewNotifier() base.Notifier { func (ns *notificationService) Run() { for opts := range ns.issueQueue { - if err := models.CreateOrUpdateIssueNotifications(opts.issue, opts.notificationAuthorID); err != nil { + if err := models.CreateOrUpdateIssueNotifications(opts.issueID, opts.commentID, opts.notificationAuthorID); err != nil { log.Error("Was unable to create issue notification: %v", err) } } @@ -44,43 +45,51 @@ func (ns *notificationService) Run() { func (ns *notificationService) NotifyCreateIssueComment(doer *models.User, repo *models.Repository, issue *models.Issue, comment *models.Comment) { - ns.issueQueue <- issueNotificationOpts{ - issue, - doer.ID, + var opts = issueNotificationOpts{ + issueID: issue.ID, + notificationAuthorID: doer.ID, + } + if comment != nil { + opts.commentID = comment.ID } + ns.issueQueue <- opts } func (ns *notificationService) NotifyNewIssue(issue *models.Issue) { ns.issueQueue <- issueNotificationOpts{ - issue, - issue.Poster.ID, + issueID: issue.ID, + notificationAuthorID: issue.Poster.ID, } } func (ns *notificationService) NotifyIssueChangeStatus(doer *models.User, issue *models.Issue, isClosed bool) { ns.issueQueue <- issueNotificationOpts{ - issue, - doer.ID, + issueID: issue.ID, + notificationAuthorID: doer.ID, } } func (ns *notificationService) NotifyMergePullRequest(pr *models.PullRequest, doer *models.User, gitRepo *git.Repository) { ns.issueQueue <- issueNotificationOpts{ - pr.Issue, - doer.ID, + issueID: pr.Issue.ID, + notificationAuthorID: doer.ID, } } func (ns *notificationService) NotifyNewPullRequest(pr *models.PullRequest) { ns.issueQueue <- issueNotificationOpts{ - pr.Issue, - pr.Issue.PosterID, + issueID: pr.Issue.ID, + notificationAuthorID: pr.Issue.PosterID, } } func (ns *notificationService) NotifyPullRequestReview(pr *models.PullRequest, r *models.Review, c *models.Comment) { - ns.issueQueue <- issueNotificationOpts{ - pr.Issue, - r.Reviewer.ID, + var opts = issueNotificationOpts{ + issueID: pr.Issue.ID, + notificationAuthorID: r.Reviewer.ID, + } + if c != nil { + opts.commentID = c.ID } + ns.issueQueue <- opts } diff --git a/routers/user/notification.go b/routers/user/notification.go index 8c23d76fa9e..cd6617a2332 100644 --- a/routers/user/notification.go +++ b/routers/user/notification.go @@ -68,6 +68,25 @@ func Notifications(c *context.Context) { return } + repos, err := notifications.LoadRepos() + if err != nil { + c.ServerError("LoadRepos", err) + return + } + if err := repos.LoadAttributes(); err != nil { + c.ServerError("LoadAttributes", err) + return + } + + if err := notifications.LoadIssues(); err != nil { + c.ServerError("LoadIssues", err) + return + } + if err := notifications.LoadComments(); err != nil { + c.ServerError("LoadComments", err) + return + } + total, err := models.GetNotificationCount(c.User, status) if err != nil { c.ServerError("ErrGetNotificationCount", err) diff --git a/templates/user/notification/notification.tmpl b/templates/user/notification/notification.tmpl index 3c30bbe3056..5b34604fb74 100644 --- a/templates/user/notification/notification.tmpl +++ b/templates/user/notification/notification.tmpl @@ -34,11 +34,11 @@ {{range $notification := .Notifications}} - {{$issue := $notification.GetIssue}} - {{$repo := $notification.GetRepo}} + {{$issue := $notification.Issue}} + {{$repo := $notification.Repository}} {{$repoOwner := $repo.MustOwner}} - +
{{if eq $notification.Status 3}} @@ -61,7 +61,7 @@ {{end}} - + #{{$issue.Index}} - {{$issue.Title}}