Prevent sending emails and notifications to inactive users #2384

daviian · 2017-08-24T16:39:26Z

Targets #2034

Added checks if user is inactive (is_active = false or prohibit_login = true) before creating notifications and sending mails.
Furthermore I added NOT NULL DEFAULT false to prohibit_login field.

Changed unit tests accordingly and added a migration, which I tested for mssql, mysql and postgresql.

lafriks · 2017-08-24T18:19:47Z

models/issue_mail.go

+	if err != nil {
+		return fmt.Errorf("GetUserByID [%d]: %v", issue.PosterID, err)
+	}
+	if issue.PosterID != doer.ID && poster.IsActive == true {


Do not check booleans with == do just simple && poster.IsActive {. Should there be also check for && !poster.ProhibitLogin?

Yes you are right. Will fix that.

lafriks · 2017-08-24T18:23:06Z

models/migrations/v39.go

+)
+
+func addDefaultValueToUserProhibitLogin(x *xorm.Engine) (err error) {
+	// Update user


Do not load data in go code. Just update with condition where prohibid_login is null

I tried but there was an issue with one database.
Was complaining about not finding anything while updating because I had no user with nullin database.

I tested it again with your proposed change.
It is working now, maybe I had a problem somewhere else ;-)

lafriks · 2017-08-24T18:25:55Z

models/user.go

@@ -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:"DEFAULT false"`


Also add notnull here

lunny · 2017-08-25T01:38:55Z

models/repo_watch.go

-	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).


I don't think user who is prohibit_login will be moved from watchers UI.

The method is also used in

gitea/models/notification.go

Line 104 in f61a1d2

watches, err := getWatchers(e, issue.RepoID)

Perhaps I should add a second method or a parameter to add restriction conditionally?

Yes, add additional parameter to function to know in to return all as it was or just active ones

@lunny @lafriks I figured out that the view isn't using this method anyways. The view is using the method from here https://github.com/daviian/gitea/blob/e3f679f39da7985193cec28a7c21fbc16ab2310d/models/repo_watch.go#L67

The method in question is only used by notification and mail

@lunny Any response?

@daviian OK.

ethantkoenig · 2017-08-26T02:16:50Z

models/issue.go

+		And("`comment`.type = ?", CommentTypeComment).
+		And("`user`.is_active = ?", true).
+		And("`user`.prohibit_login = ?", false).
+		Join("LEFT", "user", "`user`.id = `comment`.poster_id").


I'm not sure why we're doing a left join here. If the goal is to include comments that don't have a "valid" poster (e.g. deleted user), I don't think this will work; for those comments the user.is_active column will be NULL, and the user.is_active = true condition won't hold. And if we don't want to include such comments, why not just do an inner join?

Ditto elsewhere

@ethantkoenig good point, I also think it should be changed to INNER join

daviian · 2017-08-28T10:41:36Z

I will implement the requested changes this week, in the best case this evening.

lunny · 2017-09-01T01:56:57Z

@daviian have you fixed all points?

daviian · 2017-09-01T08:03:03Z

@lunny I have changed the joins to INNER as requested by @ethantkoenig and fixed mentioned mistakes from @lafriks
And I still wait for your answer on my comment regarding the impact on UI.

lafriks · 2017-09-03T18:53:34Z

LGTM

bkcsoft · 2017-09-11T02:20:53Z

models/issue.go

@@ -1209,7 +1209,10 @@ func GetParticipantsByIssueID(issueID int64) ([]*User, error) {
 	userIDs := make([]int64, 0, 5)
 	if err := x.Table("comment").Cols("poster_id").
 		Where("issue_id = ?", issueID).


shouldn't this really be?

Where("`comment`.issue_id = ?", issue).

Since issue_id is a unique attribute in this join it is valid sql, but I can add it for the sake of clarity.

bkcsoft · 2017-09-11T02:22:47Z

models/issue_watch.go

@@ -91,6 +91,9 @@ func GetIssueWatchers(issueID int64) ([]*IssueWatch, error) {
 func getIssueWatchers(e Engine, issueID int64) (watches []*IssueWatch, err error) {
 	err = e.
 		Where("issue_id = ?", issueID).


Same here, should have watch.issue_id

As mentioned in the other comment, I can add it for the sake of clarity, but it wouldn't be necessary.

bkcsoft

One minor thing though

bkcsoft · 2017-09-12T15:42:13Z

models/migrations/v40.go

+		_, 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")


_, err = x.Exec(`ALTER TABLE [user] ALTER COLUMN "prohibit_login" BIT NOT NULL`)

Can I ask why?

No you can't :P I think bkcsoft asked that just for readability to not have to use backslashes

So it should be changed in the other statements too?

I would say no, because the other statements contain backticks

codecov-io · 2017-09-13T22:03:33Z

Codecov Report

Merging #2384 into master will decrease coverage by 0.11%.
The diff coverage is 73.68%.

@@            Coverage Diff             @@
##           master    #2384      +/-   ##
==========================================
- Coverage    27.7%   27.59%   -0.12%     
==========================================
  Files          83       83              
  Lines       16914    16928      +14     
==========================================
- Hits         4686     4671      -15     
- Misses      11553    11581      +28     
- Partials      675      676       +1

Impacted Files	Coverage Δ
models/user.go	`18.45% <ø> (ø)`	⬆️
models/issue_mail.go	`5.97% <0%> (-0.38%)`	⬇️
models/issue_watch.go	`84.21% <100%> (+0.87%)`	⬆️
models/issue.go	`24.64% <100%> (+0.22%)`	⬆️
models/repo_watch.go	`68.25% <100%> (+2.15%)`	⬆️
models/notification.go	`58.6% <0%> (-13.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b496e3e...7047786. Read the comment docs.

lafriks · 2017-09-14T04:11:19Z

@bkcsoft need your approval

…ications Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>

daviian · 2017-09-14T21:29:08Z

Rebased with current master

appleboy · 2017-09-15T02:32:06Z

models/issue_mail.go

+	if err != nil {
+		return fmt.Errorf("GetUserByID [%d]: %v", issue.PosterID, err)
+	}
+	if issue.PosterID != doer.ID && poster.IsActive && !poster.ProhibitLogin {


missing testing in this patch?.

@appleboy what do you mean?

bkcsoft · 2017-09-15T14:24:25Z

LGTM

lafriks requested changes Aug 24, 2017

View reviewed changes

lafriks approved these changes Aug 24, 2017

View reviewed changes

lunny reviewed Aug 25, 2017

View reviewed changes

ethantkoenig suggested changes Aug 26, 2017

View reviewed changes

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 28, 2017

daviian force-pushed the inactive-user-receive-mail-fix branch from e3f679f to 4c0d946 Compare August 29, 2017 07:45

lunny added this to the 1.3.0 milestone Aug 30, 2017

lunny added the type/bug label Aug 30, 2017

ethantkoenig approved these changes Sep 1, 2017

View reviewed changes

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 3, 2017

bkcsoft reviewed Sep 11, 2017

View reviewed changes

daviian force-pushed the inactive-user-receive-mail-fix branch 2 times, most recently from c00221a to 4f36800 Compare September 12, 2017 08:14

bkcsoft suggested changes Sep 12, 2017

View reviewed changes

daviian force-pushed the inactive-user-receive-mail-fix branch 2 times, most recently from acc426a to ced11bd Compare September 13, 2017 21:42

bkcsoft approved these changes Sep 14, 2017

View reviewed changes

daviian added 5 commits September 14, 2017 23:21

Filter inactive users before sending emails or creating browser notif…

22537f0

…ications Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>

fix formatting issues

d08001f

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>

included requested changes

76aace8

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>

optimized database queries

9bcaa0d

rebasing new master and add tablenames for clarification in xorm queries

669b5df

remove escaped quotationmarks using backticks

fdc0f6c

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>

daviian force-pushed the inactive-user-receive-mail-fix branch from f0092d9 to fdc0f6c Compare September 14, 2017 21:27

appleboy reviewed Sep 15, 2017

View reviewed changes

Merge branch 'master' into inactive-user-receive-mail-fix

7047786

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 15, 2017

lunny merged commit d766d0c into go-gitea:master Sep 16, 2017

daviian deleted the inactive-user-receive-mail-fix branch September 16, 2017 22:38

lunny mentioned this pull request Sep 22, 2017

Deactivate account #2034

Closed

7 tasks

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent sending emails and notifications to inactive users #2384

Prevent sending emails and notifications to inactive users #2384

daviian commented Aug 24, 2017

lafriks Aug 24, 2017

daviian Aug 24, 2017

lafriks Aug 24, 2017

daviian Aug 24, 2017

daviian Aug 24, 2017

lafriks Aug 24, 2017

lunny Aug 25, 2017

daviian Aug 25, 2017 •

edited

lafriks Aug 25, 2017

daviian Aug 29, 2017

daviian Sep 3, 2017

lunny Sep 4, 2017

ethantkoenig Aug 26, 2017

lafriks Aug 28, 2017

daviian commented Aug 28, 2017

lunny commented Sep 1, 2017

daviian commented Sep 1, 2017 •

edited

lafriks commented Sep 3, 2017

bkcsoft Sep 11, 2017

daviian Sep 11, 2017

bkcsoft Sep 11, 2017

daviian Sep 11, 2017 •

edited

bkcsoft left a comment

bkcsoft Sep 12, 2017

daviian Sep 12, 2017

lafriks Sep 12, 2017

daviian Sep 12, 2017

ethantkoenig Sep 13, 2017

codecov-io commented Sep 13, 2017 •

edited

lafriks commented Sep 14, 2017

daviian commented Sep 14, 2017

appleboy Sep 15, 2017

lafriks Sep 15, 2017

bkcsoft commented Sep 15, 2017

Prevent sending emails and notifications to inactive users #2384

Prevent sending emails and notifications to inactive users #2384

Conversation

daviian commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviian Aug 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviian commented Aug 28, 2017

lunny commented Sep 1, 2017

daviian commented Sep 1, 2017 • edited

lafriks commented Sep 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviian Sep 11, 2017 • edited

Choose a reason for hiding this comment

bkcsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 13, 2017 • edited

Codecov Report

lafriks commented Sep 14, 2017

daviian commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented Sep 15, 2017

daviian Aug 25, 2017 •

edited

daviian commented Sep 1, 2017 •

edited

daviian Sep 11, 2017 •

edited

codecov-io commented Sep 13, 2017 •

edited