Browse Source

Prevent sending emails and notifications to inactive users (#2384)

* 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>
David Schneiderbauer 1 year ago
parent
commit
d766d0c4e0

+ 1 - 1
models/fixtures/repository.yml

@@ -9,7 +9,7 @@
9 9
   num_pulls: 2
10 10
   num_closed_pulls: 0
11 11
   num_milestones: 2
12
-  num_watches: 2
12
+  num_watches: 3
13 13
 
14 14
 -
15 15
   id: 2

+ 5 - 0
models/fixtures/watch.yml

@@ -7,3 +7,8 @@
7 7
   id: 2
8 8
   user_id: 4
9 9
   repo_id: 1
10
+
11
+-
12
+  id: 3
13
+  user_id: 10
14
+  repo_id: 1

+ 5 - 2
models/issue.go

@@ -1204,8 +1204,11 @@ func GetParticipantsByIssueID(issueID int64) ([]*User, error) {
1204 1204
 func getParticipantsByIssueID(e Engine, issueID int64) ([]*User, error) {
1205 1205
 	userIDs := make([]int64, 0, 5)
1206 1206
 	if err := e.Table("comment").Cols("poster_id").
1207
-		Where("issue_id = ?", issueID).
1208
-		And("type = ?", CommentTypeComment).
1207
+		Where("`comment`.issue_id = ?", issueID).
1208
+		And("`comment`.type = ?", CommentTypeComment).
1209
+		And("`user`.is_active = ?", true).
1210
+		And("`user`.prohibit_login = ?", false).
1211
+		Join("INNER", "user", "`user`.id = `comment`.poster_id").
1209 1212
 		Distinct("poster_id").
1210 1213
 		Find(&userIDs); err != nil {
1211 1214
 		return nil, fmt.Errorf("get poster IDs: %v", err)

+ 6 - 2
models/issue_mail.go

@@ -36,9 +36,13 @@ func mailIssueCommentToParticipants(e Engine, issue *Issue, doer *User, comment
36 36
 		return fmt.Errorf("getParticipantsByIssueID [issue_id: %d]: %v", issue.ID, err)
37 37
 	}
38 38
 
39
-	// In case the issue poster is not watching the repository,
39
+	// In case the issue poster is not watching the repository and is active,
40 40
 	// even if we have duplicated in watchers, can be safely filtered out.
41
-	if issue.PosterID != doer.ID {
41
+	poster, err := GetUserByID(issue.PosterID)
42
+	if err != nil {
43
+		return fmt.Errorf("GetUserByID [%d]: %v", issue.PosterID, err)
44
+	}
45
+	if issue.PosterID != doer.ID && poster.IsActive && !poster.ProhibitLogin {
42 46
 		participants = append(participants, issue.Poster)
43 47
 	}
44 48
 

+ 2 - 1
models/issue_test.go

@@ -80,7 +80,8 @@ func TestGetParticipantsByIssueID(t *testing.T) {
80 80
 	// User 1 is issue1 poster (see fixtures/issue.yml)
81 81
 	// User 2 only labeled issue1 (see fixtures/comment.yml)
82 82
 	// Users 3 and 5 made actual comments (see fixtures/comment.yml)
83
-	checkParticipants(1, []int{3, 5})
83
+	// User 3 is inactive, thus not active participant
84
+	checkParticipants(1, []int{5})
84 85
 }
85 86
 
86 87
 func TestIssue_AddLabel(t *testing.T) {

+ 4 - 1
models/issue_watch.go

@@ -90,7 +90,10 @@ func GetIssueWatchers(issueID int64) ([]*IssueWatch, error) {
90 90
 
91 91
 func getIssueWatchers(e Engine, issueID int64) (watches []*IssueWatch, err error) {
92 92
 	err = e.
93
-		Where("issue_id = ?", issueID).
93
+		Where("`issue_watch`.issue_id = ?", issueID).
94
+		And("`user`.is_active = ?", true).
95
+		And("`user`.prohibit_login = ?", false).
96
+		Join("INNER", "user", "`user`.id = `issue_watch`.user_id").
94 97
 		Find(&watches)
95 98
 	return
96 99
 }

+ 5 - 0
models/issue_watch_test.go

@@ -43,6 +43,11 @@ func TestGetIssueWatchers(t *testing.T) {
43 43
 
44 44
 	iws, err := GetIssueWatchers(1)
45 45
 	assert.NoError(t, err)
46
+	// Watcher is inactive, thus 0
47
+	assert.Equal(t, 0, len(iws))
48
+
49
+	iws, err = GetIssueWatchers(2)
50
+	assert.NoError(t, err)
46 51
 	assert.Equal(t, 1, len(iws))
47 52
 
48 53
 	iws, err = GetIssueWatchers(5)

+ 2 - 0
models/migrations/migrations.go

@@ -130,6 +130,8 @@ var migrations = []Migration{
130 130
 	NewMigration("adds time tracking and stopwatches", addTimetracking),
131 131
 	// v40 -> v41
132 132
 	NewMigration("migrate protected branch struct", migrateProtectedBranchStruct),
133
+	// v41 -> v42
134
+	NewMigration("add default value to user prohibit_login", addDefaultValueToUserProhibitLogin),
133 135
 }
134 136
 
135 137
 // Migrate database to current version

+ 42 - 0
models/migrations/v41.go

@@ -0,0 +1,42 @@
1
+// Copyright 2017 The Gitea Authors. All rights reserved.
2
+// Use of this source code is governed by a MIT-style
3
+// license that can be found in the LICENSE file.
4
+
5
+package migrations
6
+
7
+import (
8
+	"fmt"
9
+
10
+	"code.gitea.io/gitea/models"
11
+
12
+	"github.com/go-xorm/xorm"
13
+)
14
+
15
+func addDefaultValueToUserProhibitLogin(x *xorm.Engine) (err error) {
16
+	user := &models.User{
17
+		ProhibitLogin: false,
18
+	}
19
+
20
+	if _, err := x.Where("`prohibit_login` IS NULL").Cols("prohibit_login").Update(user); err != nil {
21
+		return err
22
+	}
23
+
24
+	dialect := x.Dialect().DriverName()
25
+
26
+	switch dialect {
27
+	case "mysql":
28
+		_, err = x.Exec("ALTER TABLE user MODIFY `prohibit_login` tinyint(1) NOT NULL DEFAULT 0")
29
+	case "postgres":
30
+		_, err = x.Exec("ALTER TABLE \"user\" ALTER COLUMN `prohibit_login` SET NOT NULL, ALTER COLUMN `prohibit_login` SET DEFAULT false")
31
+	case "mssql":
32
+		// xorm already set DEFAULT 0 for data type BIT in mssql
33
+		_, err = x.Exec(`ALTER TABLE [user] ALTER COLUMN "prohibit_login" BIT NOT NULL`)
34
+	case "sqlite3":
35
+	}
36
+
37
+	if err != nil {
38
+		return fmt.Errorf("Error changing user prohibit_login column definition: %v", err)
39
+	}
40
+
41
+	return err
42
+}

+ 2 - 3
models/notification_test.go

@@ -16,9 +16,8 @@ func TestCreateOrUpdateIssueNotifications(t *testing.T) {
16 16
 
17 17
 	assert.NoError(t, CreateOrUpdateIssueNotifications(issue, 2))
18 18
 
19
-	notf := AssertExistsAndLoadBean(t, &Notification{UserID: 1, IssueID: issue.ID}).(*Notification)
20
-	assert.Equal(t, NotificationStatusUnread, notf.Status)
21
-	notf = AssertExistsAndLoadBean(t, &Notification{UserID: 4, IssueID: issue.ID}).(*Notification)
19
+	// Two watchers are inactive, thus only notification for user 10 is created
20
+	notf := AssertExistsAndLoadBean(t, &Notification{UserID: 10, IssueID: issue.ID}).(*Notification)
22 21
 	assert.Equal(t, NotificationStatusUnread, notf.Status)
23 22
 	CheckConsistencyFor(t, &Issue{ID: issue.ID})
24 23
 }

+ 5 - 1
models/repo_watch.go

@@ -51,7 +51,11 @@ func WatchRepo(userID, repoID int64, watch bool) (err error) {
51 51
 
52 52
 func getWatchers(e Engine, repoID int64) ([]*Watch, error) {
53 53
 	watches := make([]*Watch, 0, 10)
54
-	return watches, e.Find(&watches, &Watch{RepoID: repoID})
54
+	return watches, e.Where("`watch`.repo_id=?", repoID).
55
+		And("`user`.is_active=?", true).
56
+		And("`user`.prohibit_login=?", false).
57
+		Join("INNER", "user", "`user`.id = `watch`.user_id").
58
+		Find(&watches)
55 59
 }
56 60
 
57 61
 // GetWatchers returns all watchers of given repository.

+ 5 - 9
models/repo_watch_test.go

@@ -40,7 +40,8 @@ func TestGetWatchers(t *testing.T) {
40 40
 	repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
41 41
 	watches, err := GetWatchers(repo.ID)
42 42
 	assert.NoError(t, err)
43
-	assert.Len(t, watches, repo.NumWatches)
43
+	// Two watchers are inactive, thus minus 2
44
+	assert.Len(t, watches, repo.NumWatches-2)
44 45
 	for _, watch := range watches {
45 46
 		assert.EqualValues(t, repo.ID, watch.RepoID)
46 47
 	}
@@ -77,21 +78,16 @@ func TestNotifyWatchers(t *testing.T) {
77 78
 	}
78 79
 	assert.NoError(t, NotifyWatchers(action))
79 80
 
81
+	// Two watchers are inactive, thus action is only created for user 8, 10
80 82
 	AssertExistsAndLoadBean(t, &Action{
81 83
 		ActUserID: action.ActUserID,
82
-		UserID:    1,
83
-		RepoID:    action.RepoID,
84
-		OpType:    action.OpType,
85
-	})
86
-	AssertExistsAndLoadBean(t, &Action{
87
-		ActUserID: action.ActUserID,
88
-		UserID:    4,
84
+		UserID:    8,
89 85
 		RepoID:    action.RepoID,
90 86
 		OpType:    action.OpType,
91 87
 	})
92 88
 	AssertExistsAndLoadBean(t, &Action{
93 89
 		ActUserID: action.ActUserID,
94
-		UserID:    8,
90
+		UserID:    10,
95 91
 		RepoID:    action.RepoID,
96 92
 		OpType:    action.OpType,
97 93
 	})

+ 1 - 1
models/user.go

@@ -111,7 +111,7 @@ type User struct {
111 111
 	AllowGitHook            bool
112 112
 	AllowImportLocal        bool // Allow migrate repository by local path
113 113
 	AllowCreateOrganization bool `xorm:"DEFAULT true"`
114
-	ProhibitLogin           bool
114
+	ProhibitLogin           bool `xorm:"NOT NULL DEFAULT false"`
115 115
 
116 116
 	// Avatar
117 117
 	Avatar          string `xorm:"VARCHAR(2048) NOT NULL"`