Improve notification (#8835)

* Improve notifications

* batch load user

* Update notification only when read

* Fix reorder

* fix lint

* fix test

* fix lint

* make function meaningful

* fix comment

diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index 2e289b6f734e1a78d85bf965f2b4ae44eba38f76..e5bfc2b881eaeb0c550705909f100aa8201ef6cd 100644 (file)
--- 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 (file)
index 0000000..60b8fb4
--- /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 5b6ce597d15b8909b052584cee2285fd9034d68f..c8203754f90dd3dc55624abdec0c37d8a82d19d5 100644 (file)
--- 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 01c960c9299abe66ed7c818eda54b6716293968c..728be7182ca8c2640648340060aa36db001feb7d 100644 (file)
--- 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 22089158f5b72c464f48c1fe985501cc76baf578..bfe497f866d460c80400612b18f0dc0d214eea24 100644 (file)
--- 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 8c23d76fa9e02c45d3315166065e4464b5f9b2c6..cd6617a23321455a8aa71cd51dbf39f5d6f06f5b 100644 (file)
--- 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 3c30bbe3056e5c091b82133559ae892f65f9f18a..5b34604fb746bd6ad78e9f9593289af3ae0a1b02 100644 (file)
--- a/templates/user/notification/notification.tmpl
+++ b/templates/user/notification/notification.tmpl
@@ -34,11 +34,11 @@
                                <table class="ui unstackable striped very compact small selectable table">
                                        <tbody>
                                                {{range $notification := .Notifications}}
-                                                       {{$issue := $notification.GetIssue}}
-                                                       {{$repo := $notification.GetRepo}}
+                                                       {{$issue := $notification.Issue}}
+                                                       {{$repo := $notification.Repository}}
                                                        {{$repoOwner := $repo.MustOwner}}
 
-                                                       <tr data-href="{{AppSubUrl}}/{{$repoOwner.Name}}/{{$repo.Name}}/issues/{{$issue.Index}}">
+                                                       <tr data-href="{{$notification.HTMLURL}}">
                                                                <td class="collapsing">
                                                                        {{if eq $notification.Status 3}}
                                                                                <i class="blue octicon octicon-pin"></i>
@@ -61,7 +61,7 @@
                                                                        {{end}}
                                                                </td>
                                                                <td class="eleven wide">
-                                                                       <a class="item" href="{{AppSubUrl}}/{{$repoOwner.Name}}/{{$repo.Name}}/issues/{{$issue.Index}}">
+                                                                       <a class="item" href="{{$notification.HTMLURL}}">
                                                                                #{{$issue.Index}} - {{$issue.Title}}
                                                                        </a>
                                                                </td>