]> source.dussan.org Git - gitea.git/commitdiff
Prevent sending emails and notifications to inactive users (#2384)
authorDavid Schneiderbauer <daviian@users.noreply.github.com>
Sat, 16 Sep 2017 00:18:25 +0000 (02:18 +0200)
committerLunny Xiao <xiaolunwen@gmail.com>
Sat, 16 Sep 2017 00:18:25 +0000 (08:18 +0800)
* Filter inactive users before sending emails or creating browser notifications

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
* fix formatting issues

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
* included requested changes

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
* optimized database queries

* rebasing new master and add tablenames for clarification in xorm queries

* remove escaped quotationmarks using backticks

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
13 files changed:
models/fixtures/repository.yml
models/fixtures/watch.yml
models/issue.go
models/issue_mail.go
models/issue_test.go
models/issue_watch.go
models/issue_watch_test.go
models/migrations/migrations.go
models/migrations/v41.go [new file with mode: 0644]
models/notification_test.go
models/repo_watch.go
models/repo_watch_test.go
models/user.go

index 3409ba8113f6463347ca3f31353f46fb88c42682..51812d358553b7d72058a8383564745186ed4ddf 100644 (file)
@@ -9,7 +9,7 @@
   num_pulls: 2
   num_closed_pulls: 0
   num_milestones: 2
-  num_watches: 2
+  num_watches: 3
 
 -
   id: 2
index cd6efb102ec8b2c4bd7c8b3bab19aef42a202257..2ff888b7c78ffa92c41af63c08208abe37d326b1 100644 (file)
@@ -7,3 +7,8 @@
   id: 2
   user_id: 4
   repo_id: 1
+
+-
+  id: 3
+  user_id: 10
+  repo_id: 1
index 2b71e0776d13be66a1fcbcd199fa52ac47f0c3dd..9d081471a74a57912d8c678e1c69bf6f4c7ece70 100644 (file)
@@ -1204,8 +1204,11 @@ func GetParticipantsByIssueID(issueID int64) ([]*User, error) {
 func getParticipantsByIssueID(e Engine, issueID int64) ([]*User, error) {
        userIDs := make([]int64, 0, 5)
        if err := e.Table("comment").Cols("poster_id").
-               Where("issue_id = ?", issueID).
-               And("type = ?", CommentTypeComment).
+               Where("`comment`.issue_id = ?", issueID).
+               And("`comment`.type = ?", CommentTypeComment).
+               And("`user`.is_active = ?", true).
+               And("`user`.prohibit_login = ?", false).
+               Join("INNER", "user", "`user`.id = `comment`.poster_id").
                Distinct("poster_id").
                Find(&userIDs); err != nil {
                return nil, fmt.Errorf("get poster IDs: %v", err)
index 9e604a50f1b9eddacfed6d4a4fcaf21b4a2885d0..74ef6605548627c2475da7b76dd89386c565a24c 100644 (file)
@@ -36,9 +36,13 @@ func mailIssueCommentToParticipants(e Engine, issue *Issue, doer *User, comment
                return fmt.Errorf("getParticipantsByIssueID [issue_id: %d]: %v", issue.ID, err)
        }
 
-       // In case the issue poster is not watching the repository,
+       // In case the issue poster is not watching the repository and is active,
        // even if we have duplicated in watchers, can be safely filtered out.
-       if issue.PosterID != doer.ID {
+       poster, err := GetUserByID(issue.PosterID)
+       if err != nil {
+               return fmt.Errorf("GetUserByID [%d]: %v", issue.PosterID, err)
+       }
+       if issue.PosterID != doer.ID && poster.IsActive && !poster.ProhibitLogin {
                participants = append(participants, issue.Poster)
        }
 
index 8a8ccce459d50d313f48cfbe4e9251be34cc5dda..21df146b58aef6590f3d94e7488a8a604f1c0b03 100644 (file)
@@ -80,7 +80,8 @@ func TestGetParticipantsByIssueID(t *testing.T) {
        // User 1 is issue1 poster (see fixtures/issue.yml)
        // User 2 only labeled issue1 (see fixtures/comment.yml)
        // Users 3 and 5 made actual comments (see fixtures/comment.yml)
-       checkParticipants(1, []int{3, 5})
+       // User 3 is inactive, thus not active participant
+       checkParticipants(1, []int{5})
 }
 
 func TestIssue_AddLabel(t *testing.T) {
index 37511787e5da3e745094dd579d9ea3db92a716e7..994ae5a97f115a08d67300e823244fb047a831e4 100644 (file)
@@ -90,7 +90,10 @@ func GetIssueWatchers(issueID int64) ([]*IssueWatch, error) {
 
 func getIssueWatchers(e Engine, issueID int64) (watches []*IssueWatch, err error) {
        err = e.
-               Where("issue_id = ?", issueID).
+               Where("`issue_watch`.issue_id = ?", issueID).
+               And("`user`.is_active = ?", true).
+               And("`user`.prohibit_login = ?", false).
+               Join("INNER", "user", "`user`.id = `issue_watch`.user_id").
                Find(&watches)
        return
 }
index d8b456c3ae0c4f93e5cb77479401423bb0d324a7..a4819b22240316008ca0b981a6733708814fd39a 100644 (file)
@@ -43,6 +43,11 @@ func TestGetIssueWatchers(t *testing.T) {
 
        iws, err := GetIssueWatchers(1)
        assert.NoError(t, err)
+       // Watcher is inactive, thus 0
+       assert.Equal(t, 0, len(iws))
+
+       iws, err = GetIssueWatchers(2)
+       assert.NoError(t, err)
        assert.Equal(t, 1, len(iws))
 
        iws, err = GetIssueWatchers(5)
index e7542954d7687677bbbfd01627b7326f8b835388..c36f6fabb79f4af0958d94e9ac8b11bdba4c507d 100644 (file)
@@ -130,6 +130,8 @@ var migrations = []Migration{
        NewMigration("adds time tracking and stopwatches", addTimetracking),
        // v40 -> v41
        NewMigration("migrate protected branch struct", migrateProtectedBranchStruct),
+       // v41 -> v42
+       NewMigration("add default value to user prohibit_login", addDefaultValueToUserProhibitLogin),
 }
 
 // Migrate database to current version
diff --git a/models/migrations/v41.go b/models/migrations/v41.go
new file mode 100644 (file)
index 0000000..89763c3
--- /dev/null
@@ -0,0 +1,42 @@
+// Copyright 2017 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 (
+       "fmt"
+
+       "code.gitea.io/gitea/models"
+
+       "github.com/go-xorm/xorm"
+)
+
+func addDefaultValueToUserProhibitLogin(x *xorm.Engine) (err error) {
+       user := &models.User{
+               ProhibitLogin: false,
+       }
+
+       if _, err := x.Where("`prohibit_login` IS NULL").Cols("prohibit_login").Update(user); err != nil {
+               return err
+       }
+
+       dialect := x.Dialect().DriverName()
+
+       switch dialect {
+       case "mysql":
+               _, err = x.Exec("ALTER TABLE user MODIFY `prohibit_login` tinyint(1) NOT NULL DEFAULT 0")
+       case "postgres":
+               _, err = x.Exec("ALTER TABLE \"user\" ALTER COLUMN `prohibit_login` SET NOT NULL, ALTER COLUMN `prohibit_login` SET DEFAULT false")
+       case "mssql":
+               // xorm already set DEFAULT 0 for data type BIT in mssql
+               _, err = x.Exec(`ALTER TABLE [user] ALTER COLUMN "prohibit_login" BIT NOT NULL`)
+       case "sqlite3":
+       }
+
+       if err != nil {
+               return fmt.Errorf("Error changing user prohibit_login column definition: %v", err)
+       }
+
+       return err
+}
index 2166afa2a62308db540d427ea081b8ad495ba82e..c1b7d50529833fda35c5abe56fefc6a88979b7b8 100644 (file)
@@ -16,9 +16,8 @@ func TestCreateOrUpdateIssueNotifications(t *testing.T) {
 
        assert.NoError(t, CreateOrUpdateIssueNotifications(issue, 2))
 
-       notf := AssertExistsAndLoadBean(t, &Notification{UserID: 1, IssueID: issue.ID}).(*Notification)
-       assert.Equal(t, NotificationStatusUnread, notf.Status)
-       notf = AssertExistsAndLoadBean(t, &Notification{UserID: 4, IssueID: issue.ID}).(*Notification)
+       // Two watchers are inactive, thus only notification for user 10 is created
+       notf := AssertExistsAndLoadBean(t, &Notification{UserID: 10, IssueID: issue.ID}).(*Notification)
        assert.Equal(t, NotificationStatusUnread, notf.Status)
        CheckConsistencyFor(t, &Issue{ID: issue.ID})
 }
index c3baab9d2f8b35c527ba7f862d65ec74f1d428c4..cf9dba900af6460b1d0c5b78d48861cf34c3b4a3 100644 (file)
@@ -51,7 +51,11 @@ func WatchRepo(userID, repoID int64, watch bool) (err error) {
 
 func getWatchers(e Engine, repoID int64) ([]*Watch, error) {
        watches := make([]*Watch, 0, 10)
-       return watches, e.Find(&watches, &Watch{RepoID: repoID})
+       return watches, e.Where("`watch`.repo_id=?", repoID).
+               And("`user`.is_active=?", true).
+               And("`user`.prohibit_login=?", false).
+               Join("INNER", "user", "`user`.id = `watch`.user_id").
+               Find(&watches)
 }
 
 // GetWatchers returns all watchers of given repository.
index a1543566c153da51587f6235d2682951a7b02eea..1277b156c5c2a429dbd40060f9dc2aa0635429ab 100644 (file)
@@ -40,7 +40,8 @@ func TestGetWatchers(t *testing.T) {
        repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
        watches, err := GetWatchers(repo.ID)
        assert.NoError(t, err)
-       assert.Len(t, watches, repo.NumWatches)
+       // Two watchers are inactive, thus minus 2
+       assert.Len(t, watches, repo.NumWatches-2)
        for _, watch := range watches {
                assert.EqualValues(t, repo.ID, watch.RepoID)
        }
@@ -77,21 +78,16 @@ func TestNotifyWatchers(t *testing.T) {
        }
        assert.NoError(t, NotifyWatchers(action))
 
+       // Two watchers are inactive, thus action is only created for user 8, 10
        AssertExistsAndLoadBean(t, &Action{
                ActUserID: action.ActUserID,
-               UserID:    1,
-               RepoID:    action.RepoID,
-               OpType:    action.OpType,
-       })
-       AssertExistsAndLoadBean(t, &Action{
-               ActUserID: action.ActUserID,
-               UserID:    4,
+               UserID:    8,
                RepoID:    action.RepoID,
                OpType:    action.OpType,
        })
        AssertExistsAndLoadBean(t, &Action{
                ActUserID: action.ActUserID,
-               UserID:    8,
+               UserID:    10,
                RepoID:    action.RepoID,
                OpType:    action.OpType,
        })
index f6d70510133df9d993eb935537399dc0b758639a..9adc5bd4e1e9746cdd17d52537bb23466b874eb6 100644 (file)
@@ -111,7 +111,7 @@ type User struct {
        AllowGitHook            bool
        AllowImportLocal        bool // Allow migrate repository by local path
        AllowCreateOrganization bool `xorm:"DEFAULT true"`
-       ProhibitLogin           bool
+       ProhibitLogin           bool `xorm:"NOT NULL DEFAULT false"`
 
        // Avatar
        Avatar          string `xorm:"VARCHAR(2048) NOT NULL"`