From a0a88a289d76dce8d16bdbd639c97e9ba03d7af2 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sat, 5 Oct 2019 00:26:35 -0300 Subject: [PATCH 01/10] Draft for ResolveMentionsByVisibility() --- models/issue.go | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/models/issue.go b/models/issue.go index 9590bc04ff627..9f88ade1b8cf0 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1909,3 +1909,52 @@ func (issue *Issue) updateClosedNum(e Engine) (err error) { } return } + +// ResolveMentionsByVisibility returns the users mentioned in an issue, removing those that +// don't have access to reading it. OrgUsers are returned separately for flexibility. +func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, mentions []string) (users[]*User, orgUsers[]*OrgUser, err error) { + if len(mentions) == 0 { + return + } + if err = issue.loadRepo(ctx.e); err != nil { + return + } + for i := range mentions { + mentions[i] = strings.ToLower(mentions[i]) + } + unchecked := make([]*User, 0, len(mentions)) + + if err := ctx.e.In("lower_name", mentions).Asc("lower_name").Find(&users); err != nil { + return nil, nil, fmt.Errorf("find mentioned users: %v", err) + } + + users = make([]*User, 0, len(mentions)) + for _, user := range unchecked { + if !user.IsOrganization() { + // Normal users must have read access to the referencing issue + perm, err := getUserRepoPermission(ctx.e, issue.Repo, user) + if err != nil { + return nil, nil, fmt.Errorf("getUserRepoPermission [%d]: %v", user.ID, err) + } + if !perm.CanReadIssuesOrPulls(issue.IsPull) { + continue + } + users = append(users, user) + } else if user.NumMembers > 0 { + // To mention a whole organization the doer must belong to the owners team + if isOwner, err := isOrganizationOwner(ctx.e, user.ID, doer.ID); err != nil { + return nil, nil, fmt.Errorf("isOrganizationOwner [%d]: %v", user.ID, err) + } else if !isOwner { + continue + } + morgUsers, err := getOrgUsersByOrgID(ctx.e, user.ID) + if err != nil { + return nil, nil, fmt.Errorf("GetOrgUsersByOrgID [%d]: %v", user.ID, err) + } + + orgUsers = append(orgUsers, morgUsers...) + } + } + + return +} \ No newline at end of file From 82aee51b9369dc9daebf671e1e00fe3cb399c58c Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sat, 5 Oct 2019 11:23:46 -0300 Subject: [PATCH 02/10] Correct typo --- models/org_team.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/org_team.go b/models/org_team.go index fc5d5834ef603..9170ea2c2af88 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -314,7 +314,7 @@ func (t *Team) UnitEnabled(tp UnitType) bool { func (t *Team) unitEnabled(e Engine, tp UnitType) bool { if err := t.getUnits(e); err != nil { - log.Warn("Error loading repository (ID: %d) units: %s", t.ID, err.Error()) + log.Warn("Error loading team (ID: %d) units: %s", t.ID, err.Error()) } for _, unit := range t.Units { From f1df07dd34b851312b087d7d57b76c26f18dee88 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sat, 5 Oct 2019 11:24:52 -0300 Subject: [PATCH 03/10] Resolve teams instead of orgs for mentions --- models/issue.go | 128 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 99 insertions(+), 29 deletions(-) diff --git a/models/issue.go b/models/issue.go index 9f88ade1b8cf0..c82e38e3d48af 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1911,50 +1911,120 @@ func (issue *Issue) updateClosedNum(e Engine) (err error) { } // ResolveMentionsByVisibility returns the users mentioned in an issue, removing those that -// don't have access to reading it. OrgUsers are returned separately for flexibility. -func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, mentions []string) (users[]*User, orgUsers[]*OrgUser, err error) { +// don't have access to reading it. Teams are expanded into their users, but organizations are ignored. +func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, mentions []string) (users[]*User, err error) { if len(mentions) == 0 { return } if err = issue.loadRepo(ctx.e); err != nil { return } - for i := range mentions { - mentions[i] = strings.ToLower(mentions[i]) + resolved := make(map[string]bool,len(mentions)) + for _, name := range mentions { + name := strings.ToLower(name) + if _, ok := resolved[name]; ok { + continue + } + resolved[name] = false } - unchecked := make([]*User, 0, len(mentions)) - if err := ctx.e.In("lower_name", mentions).Asc("lower_name").Find(&users); err != nil { - return nil, nil, fmt.Errorf("find mentioned users: %v", err) + if err := issue.Repo.getOwner(ctx.e); err != nil { + return nil, err } - users = make([]*User, 0, len(mentions)) - for _, user := range unchecked { - if !user.IsOrganization() { - // Normal users must have read access to the referencing issue - perm, err := getUserRepoPermission(ctx.e, issue.Repo, user) - if err != nil { - return nil, nil, fmt.Errorf("getUserRepoPermission [%d]: %v", user.ID, err) + names := make([]string,0,len(resolved)) + for name, _ := range resolved { + names = append(names, name) + } + + if issue.Repo.Owner.IsOrganization() { + + // Since there can be users with names that match the name of a team, + // if the team exists and can read the issue, the team takes precedence. + teams := make([]*Team,0,len(names)) + if err := ctx.e. + Join("INNER", "team_repo", "team_repo.team_id = team.id"). + Where("team_repo.repo_id=?", issue.Repo.ID). + In("team.lower_name", names). + Find(&teams); + err != nil { + return nil, fmt.Errorf("find mentioned teams: %v", err) + } + if len(teams) != 0 { + checked := make([]*Team,0,len(teams)) + unittype := UnitTypeIssues + if issue.IsPull { + unittype = UnitTypePullRequests } - if !perm.CanReadIssuesOrPulls(issue.IsPull) { - continue + for _, team := range teams { + if team.Authorize >= AccessModeOwner { + checked = append(checked, team) + resolved[team.LowerName] = true + continue + } + has, err := ctx.e.Get(&TeamUnit{OrgID: issue.Repo.Owner.ID, TeamID: team.ID, Type: unittype}) + if err != nil { + return nil, fmt.Errorf("get team units (%d): %v", team.ID, err) + } + if has { + checked = append(checked, team) + resolved[team.LowerName] = true + } } - users = append(users, user) - } else if user.NumMembers > 0 { - // To mention a whole organization the doer must belong to the owners team - if isOwner, err := isOrganizationOwner(ctx.e, user.ID, doer.ID); err != nil { - return nil, nil, fmt.Errorf("isOrganizationOwner [%d]: %v", user.ID, err) - } else if !isOwner { - continue + if len(checked) != 0 { + ids := make([]int64,len(checked)) + for i, _ := range checked { + ids[i] = checked[i].ID + } + if err := ctx.e. + Join("INNER", "team_user", "team_user.team_id = `user`.id"). + Where("`user`.prohibit_login", false). + And("`user`.is_active", true). + In("`team_user`.team_id", ids). + Distinct(). + Find(users); err != nil { + return nil, fmt.Errorf("get teams users: %v", err) + } + for _, user := range users { + resolved[user.LowerName] = true + } } - morgUsers, err := getOrgUsersByOrgID(ctx.e, user.ID) - if err != nil { - return nil, nil, fmt.Errorf("GetOrgUsersByOrgID [%d]: %v", user.ID, err) + } + + // Remove names already in the list + names = make([]string,0,len(resolved)) + for name, already := range resolved { + if !already { + names = append(names, name) } - - orgUsers = append(orgUsers, morgUsers...) } } + unchecked := make([]*User,0,len(names)) + if err := ctx.e. + Where("`user`.prohibit_login", false). + And("`user`.is_active", true). + In("`user`.lower_name", names). + Find(&unchecked); err != nil { + return nil, fmt.Errorf("find mentioned users: %v", err) + } + + for _, user := range unchecked { + if _, already := resolved[user.LowerName]; already || user.IsOrganization() { + continue + } + // Normal users must have read access to the referencing issue + perm, err := getUserRepoPermission(ctx.e, issue.Repo, user) + if err != nil { + return nil, fmt.Errorf("getUserRepoPermission [%d]: %v", user.ID, err) + } + if !perm.CanReadIssuesOrPulls(issue.IsPull) { + continue + } + users = append(users, user) + resolved[user.LowerName] = true + } + return -} \ No newline at end of file +} + From 7db41790d32bcc3d5832edfb5231be571f5db110 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sat, 5 Oct 2019 14:36:19 -0300 Subject: [PATCH 04/10] Create test for ResolveMentionsByVisibility --- models/issue_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/models/issue_test.go b/models/issue_test.go index 9cd9ff0ad98a8..04309eb4e9daa 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -366,3 +366,22 @@ func TestIssue_InsertIssue(t *testing.T) { testInsertIssue(t, "my issue1", "special issue's comments?") testInsertIssue(t, `my issue2, this is my son's love \n \r \ `, "special issue's '' comments?") } + +func TestIssue_ResolveMentions(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + testSuccess := func(repoID, doerID int64, mentions []string, expected []int64) { + issue := &Issue{RepoID: repoID} + doer := AssertExistsAndLoadBean(t, &User{ID: doerID}).(*User) + resolved, err := issue.ResolveMentionsByVisibility(DefaultDBContext(), doer, mentions) + assert.NoError(t, err) + ids := make([]int64, len(resolved)) + for i, user := range resolved { + ids[i] = user.ID + } + sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] }) + assert.EqualValues(t, expected, ids) + } + + testSuccess(1, 1, []string{"user5"}, []int64{5}) +} \ No newline at end of file From 4edc9766c2398440b75af78fc5f76a7712e57de3 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sat, 5 Oct 2019 14:44:14 -0300 Subject: [PATCH 05/10] Fix check for individual users and doer --- models/issue.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/models/issue.go b/models/issue.go index c82e38e3d48af..100fac0ecc44b 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1925,7 +1925,7 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti if _, ok := resolved[name]; ok { continue } - resolved[name] = false + resolved[name] = (name == doer.LowerName) } if err := issue.Repo.getOwner(ctx.e); err != nil { @@ -1933,12 +1933,13 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti } names := make([]string,0,len(resolved)) - for name, _ := range resolved { - names = append(names, name) + for name, already := range resolved { + if !already { + names = append(names, name) + } } if issue.Repo.Owner.IsOrganization() { - // Since there can be users with names that match the name of a team, // if the team exists and can read the issue, the team takes precedence. teams := make([]*Team,0,len(names)) @@ -1978,8 +1979,8 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti } if err := ctx.e. Join("INNER", "team_user", "team_user.team_id = `user`.id"). - Where("`user`.prohibit_login", false). - And("`user`.is_active", true). + Where("`user`.is_active = ?", true). + And("`user`.prohibit_login = ?", false). In("`team_user`.team_id", ids). Distinct(). Find(users); err != nil { @@ -1998,19 +1999,21 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti names = append(names, name) } } + if len(names) == 0 { + return + } } unchecked := make([]*User,0,len(names)) if err := ctx.e. - Where("`user`.prohibit_login", false). - And("`user`.is_active", true). + Where("`user`.is_active = ?", true). + And("`user`.prohibit_login = ?", false). In("`user`.lower_name", names). Find(&unchecked); err != nil { return nil, fmt.Errorf("find mentioned users: %v", err) } - for _, user := range unchecked { - if _, already := resolved[user.LowerName]; already || user.IsOrganization() { + if already := resolved[user.LowerName]; already || user.IsOrganization() { continue } // Normal users must have read access to the referencing issue From f5936eb28c6f7bf629cb009c4ffbf3e4c1cbceb9 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sun, 6 Oct 2019 12:52:11 -0300 Subject: [PATCH 06/10] Test and fix team mentions --- models/issue.go | 20 +++++++++++++------- models/issue_test.go | 23 ++++++++++++++++++----- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/models/issue.go b/models/issue.go index 100fac0ecc44b..270f06aec30d9 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1977,17 +1977,23 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti for i, _ := range checked { ids[i] = checked[i].ID } + unchecked := make([]*User,0,20) if err := ctx.e. - Join("INNER", "team_user", "team_user.team_id = `user`.id"). - Where("`user`.is_active = ?", true). - And("`user`.prohibit_login = ?", false). + Join("INNER", "team_user", "team_user.uid = `user`.id"). In("`team_user`.team_id", ids). - Distinct(). - Find(users); err != nil { + And("`user`.is_active = ?", true). + And("`user`.prohibit_login = ?", false). + Find(&unchecked); err != nil { return nil, fmt.Errorf("get teams users: %v", err) } - for _, user := range users { - resolved[user.LowerName] = true + if len(unchecked) > 0 { + users = make([]*User,0,len(unchecked)) + for _, user := range unchecked { + if already, ok := resolved[user.LowerName]; !ok || !already { + users = append(users, user) + resolved[user.LowerName] = true + } + } } } } diff --git a/models/issue_test.go b/models/issue_test.go index 04309eb4e9daa..6203195e50aec 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -370,10 +370,12 @@ func TestIssue_InsertIssue(t *testing.T) { func TestIssue_ResolveMentions(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) - testSuccess := func(repoID, doerID int64, mentions []string, expected []int64) { - issue := &Issue{RepoID: repoID} - doer := AssertExistsAndLoadBean(t, &User{ID: doerID}).(*User) - resolved, err := issue.ResolveMentionsByVisibility(DefaultDBContext(), doer, mentions) + testSuccess := func(owner, repo, doer string, mentions []string, expected []int64) { + o := AssertExistsAndLoadBean(t, &User{LowerName: owner}).(*User) + r := AssertExistsAndLoadBean(t, &Repository{OwnerID: o.ID, LowerName: repo}).(*Repository) + issue := &Issue{RepoID: r.ID} + d := AssertExistsAndLoadBean(t, &User{LowerName: doer}).(*User) + resolved, err := issue.ResolveMentionsByVisibility(DefaultDBContext(), d, mentions) assert.NoError(t, err) ids := make([]int64, len(resolved)) for i, user := range resolved { @@ -383,5 +385,16 @@ func TestIssue_ResolveMentions(t *testing.T) { assert.EqualValues(t, expected, ids) } - testSuccess(1, 1, []string{"user5"}, []int64{5}) + // Public repo, existing user + testSuccess("user2", "repo1", "user1", []string{"user5"}, []int64{5}) + // Public repo, non-existing user + testSuccess("user2", "repo1", "user1", []string{"nonexisting"}, []int64{}) + // Public repo, doer + testSuccess("user2", "repo1", "user1", []string{"user1"}, []int64{}) + // Private repo, team member + testSuccess("user17", "big_test_private_4", "user20", []string{"user2"}, []int64{2}) + // Private repo, not a team member + testSuccess("user17", "big_test_private_4", "user20", []string{"user5"}, []int64{}) + // Private repo, whole team + testSuccess("user17", "big_test_private_4", "user15", []string{"owners"}, []int64{15,18}) } \ No newline at end of file From 02cc0c00f459edd90596d025bd7c3f2b4e93c89f Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sun, 6 Oct 2019 14:59:54 -0300 Subject: [PATCH 07/10] Run all mentions through visibility filter --- models/issue.go | 72 ++++++++++----------------------- models/issue_test.go | 4 +- services/mailer/mail_comment.go | 10 +++-- services/mailer/mail_issue.go | 10 +++-- 4 files changed, 37 insertions(+), 59 deletions(-) diff --git a/models/issue.go b/models/issue.go index 270f06aec30d9..eff5f0b3b29b5 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1477,46 +1477,18 @@ func getParticipantsByIssueID(e Engine, issueID int64) ([]*User, error) { return users, e.In("id", userIDs).Find(&users) } -// UpdateIssueMentions extracts mentioned people from content and -// updates issue-user relations for them. -func UpdateIssueMentions(ctx DBContext, issueID int64, mentions []string) error { +// UpdateIssueMentions updates issue-user relations for mentioned users. +func UpdateIssueMentions(ctx DBContext, issueID int64, mentions []*User) error { if len(mentions) == 0 { return nil } - - for i := range mentions { - mentions[i] = strings.ToLower(mentions[i]) - } - users := make([]*User, 0, len(mentions)) - - if err := ctx.e.In("lower_name", mentions).Asc("lower_name").Find(&users); err != nil { - return fmt.Errorf("find mentioned users: %v", err) + ids := make([]int64, len(mentions)) + for i, u := range mentions { + ids[i] = u.ID } - - ids := make([]int64, 0, len(mentions)) - for _, user := range users { - ids = append(ids, user.ID) - if !user.IsOrganization() || user.NumMembers == 0 { - continue - } - - memberIDs := make([]int64, 0, user.NumMembers) - orgUsers, err := getOrgUsersByOrgID(ctx.e, user.ID) - if err != nil { - return fmt.Errorf("GetOrgUsersByOrgID [%d]: %v", user.ID, err) - } - - for _, orgUser := range orgUsers { - memberIDs = append(memberIDs, orgUser.ID) - } - - ids = append(ids, memberIDs...) - } - if err := UpdateIssueUsersByMentions(ctx, issueID, ids); err != nil { return fmt.Errorf("UpdateIssueUsersByMentions: %v", err) } - return nil } @@ -1912,14 +1884,14 @@ func (issue *Issue) updateClosedNum(e Engine) (err error) { // ResolveMentionsByVisibility returns the users mentioned in an issue, removing those that // don't have access to reading it. Teams are expanded into their users, but organizations are ignored. -func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, mentions []string) (users[]*User, err error) { +func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, mentions []string) (users []*User, err error) { if len(mentions) == 0 { return } if err = issue.loadRepo(ctx.e); err != nil { return } - resolved := make(map[string]bool,len(mentions)) + resolved := make(map[string]bool, len(mentions)) for _, name := range mentions { name := strings.ToLower(name) if _, ok := resolved[name]; ok { @@ -1932,7 +1904,7 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti return nil, err } - names := make([]string,0,len(resolved)) + names := make([]string, 0, len(resolved)) for name, already := range resolved { if !already { names = append(names, name) @@ -1942,17 +1914,16 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti if issue.Repo.Owner.IsOrganization() { // Since there can be users with names that match the name of a team, // if the team exists and can read the issue, the team takes precedence. - teams := make([]*Team,0,len(names)) + teams := make([]*Team, 0, len(names)) if err := ctx.e. Join("INNER", "team_repo", "team_repo.team_id = team.id"). Where("team_repo.repo_id=?", issue.Repo.ID). In("team.lower_name", names). - Find(&teams); - err != nil { + Find(&teams); err != nil { return nil, fmt.Errorf("find mentioned teams: %v", err) } if len(teams) != 0 { - checked := make([]*Team,0,len(teams)) + checked := make([]*Team, 0, len(teams)) unittype := UnitTypeIssues if issue.IsPull { unittype = UnitTypePullRequests @@ -1973,11 +1944,11 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti } } if len(checked) != 0 { - ids := make([]int64,len(checked)) - for i, _ := range checked { + ids := make([]int64, len(checked)) + for i := range checked { ids[i] = checked[i].ID } - unchecked := make([]*User,0,20) + unchecked := make([]*User, 0, 20) if err := ctx.e. Join("INNER", "team_user", "team_user.uid = `user`.id"). In("`team_user`.team_id", ids). @@ -1987,7 +1958,7 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti return nil, fmt.Errorf("get teams users: %v", err) } if len(unchecked) > 0 { - users = make([]*User,0,len(unchecked)) + users = make([]*User, 0, len(unchecked)) for _, user := range unchecked { if already, ok := resolved[user.LowerName]; !ok || !already { users = append(users, user) @@ -1999,7 +1970,7 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti } // Remove names already in the list - names = make([]string,0,len(resolved)) + names = make([]string, 0, len(resolved)) for name, already := range resolved { if !already { names = append(names, name) @@ -2010,12 +1981,12 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti } } - unchecked := make([]*User,0,len(names)) + unchecked := make([]*User, 0, len(names)) if err := ctx.e. - Where("`user`.is_active = ?", true). - And("`user`.prohibit_login = ?", false). - In("`user`.lower_name", names). - Find(&unchecked); err != nil { + Where("`user`.is_active = ?", true). + And("`user`.prohibit_login = ?", false). + In("`user`.lower_name", names). + Find(&unchecked); err != nil { return nil, fmt.Errorf("find mentioned users: %v", err) } for _, user := range unchecked { @@ -2036,4 +2007,3 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti return } - diff --git a/models/issue_test.go b/models/issue_test.go index 6203195e50aec..7909eaf79eff9 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -396,5 +396,5 @@ func TestIssue_ResolveMentions(t *testing.T) { // Private repo, not a team member testSuccess("user17", "big_test_private_4", "user20", []string{"user5"}, []int64{}) // Private repo, whole team - testSuccess("user17", "big_test_private_4", "user15", []string{"owners"}, []int64{15,18}) -} \ No newline at end of file + testSuccess("user17", "big_test_private_4", "user15", []string{"owners"}, []int64{15, 18}) +} diff --git a/services/mailer/mail_comment.go b/services/mailer/mail_comment.go index cb477f887b5cc..b3784d74e3e59 100644 --- a/services/mailer/mail_comment.go +++ b/services/mailer/mail_comment.go @@ -19,11 +19,15 @@ func MailParticipantsComment(c *models.Comment, opType models.ActionType, issue } func mailParticipantsComment(ctx models.DBContext, c *models.Comment, opType models.ActionType, issue *models.Issue) (err error) { - mentions := markup.FindAllMentions(c.Content) - if err = models.UpdateIssueMentions(ctx, c.IssueID, mentions); err != nil { + rawMentions := markup.FindAllMentions(c.Content) + userMentions, err := issue.ResolveMentionsByVisibility(ctx, c.Poster, rawMentions) + if err = models.UpdateIssueMentions(ctx, c.IssueID, userMentions); err != nil { return fmt.Errorf("UpdateIssueMentions [%d]: %v", c.IssueID, err) } - + mentions := make([]string, len(userMentions)) + for i, u := range userMentions { + mentions[i] = u.LowerName + } if len(c.Content) > 0 { if err = mailIssueCommentToParticipants(issue, c.Poster, c.Content, c, mentions); err != nil { log.Error("mailIssueCommentToParticipants: %v", err) diff --git a/services/mailer/mail_issue.go b/services/mailer/mail_issue.go index 92d2c5a8795f8..7a14c718458f1 100644 --- a/services/mailer/mail_issue.go +++ b/services/mailer/mail_issue.go @@ -123,11 +123,15 @@ func MailParticipants(issue *models.Issue, doer *models.User, opType models.Acti } func mailParticipants(ctx models.DBContext, issue *models.Issue, doer *models.User, opType models.ActionType) (err error) { - mentions := markup.FindAllMentions(issue.Content) - - if err = models.UpdateIssueMentions(ctx, issue.ID, mentions); err != nil { + rawMentions := markup.FindAllMentions(issue.Content) + userMentions, err := issue.ResolveMentionsByVisibility(ctx, doer, rawMentions) + if err = models.UpdateIssueMentions(ctx, issue.ID, userMentions); err != nil { return fmt.Errorf("UpdateIssueMentions [%d]: %v", issue.ID, err) } + mentions := make([]string, len(userMentions)) + for i, u := range userMentions { + mentions[i] = u.LowerName + } if len(issue.Content) > 0 { if err = mailIssueCommentToParticipants(issue, doer, issue.Content, nil, mentions); err != nil { From 89f5f2939313ad0bf3be4a1e2286da41ace9d58f Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sun, 6 Oct 2019 15:43:37 -0300 Subject: [PATCH 08/10] Fix error check --- services/mailer/mail_comment.go | 3 +++ services/mailer/mail_issue.go | 3 +++ 2 files changed, 6 insertions(+) diff --git a/services/mailer/mail_comment.go b/services/mailer/mail_comment.go index b3784d74e3e59..f64db04fffa9d 100644 --- a/services/mailer/mail_comment.go +++ b/services/mailer/mail_comment.go @@ -21,6 +21,9 @@ func MailParticipantsComment(c *models.Comment, opType models.ActionType, issue func mailParticipantsComment(ctx models.DBContext, c *models.Comment, opType models.ActionType, issue *models.Issue) (err error) { rawMentions := markup.FindAllMentions(c.Content) userMentions, err := issue.ResolveMentionsByVisibility(ctx, c.Poster, rawMentions) + if err != nil { + return fmt.Errorf("ResolveMentionsByVisibility [%d]: %v", c.IssueID, err) + } if err = models.UpdateIssueMentions(ctx, c.IssueID, userMentions); err != nil { return fmt.Errorf("UpdateIssueMentions [%d]: %v", c.IssueID, err) } diff --git a/services/mailer/mail_issue.go b/services/mailer/mail_issue.go index 7a14c718458f1..da0249d595dab 100644 --- a/services/mailer/mail_issue.go +++ b/services/mailer/mail_issue.go @@ -125,6 +125,9 @@ func MailParticipants(issue *models.Issue, doer *models.User, opType models.Acti func mailParticipants(ctx models.DBContext, issue *models.Issue, doer *models.User, opType models.ActionType) (err error) { rawMentions := markup.FindAllMentions(issue.Content) userMentions, err := issue.ResolveMentionsByVisibility(ctx, doer, rawMentions) + if err != nil { + return fmt.Errorf("ResolveMentionsByVisibility [%d]: %v", issue.ID, err) + } if err = models.UpdateIssueMentions(ctx, issue.ID, userMentions); err != nil { return fmt.Errorf("UpdateIssueMentions [%d]: %v", issue.ID, err) } From ca3206a907746fdef00a0bf0cc2b13c28940225b Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Wed, 9 Oct 2019 14:40:50 -0300 Subject: [PATCH 09/10] Simplify code, fix doer included in teams --- models/issue.go | 27 +++++++++++---------------- models/issue_test.go | 2 +- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/models/issue.go b/models/issue.go index eff5f0b3b29b5..959802dd8f756 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1891,26 +1891,22 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti if err = issue.loadRepo(ctx.e); err != nil { return } - resolved := make(map[string]bool, len(mentions)) + resolved := make(map[string]bool, 20) + names := make([]string, 0, 20) + resolved[doer.LowerName] = true for _, name := range mentions { name := strings.ToLower(name) if _, ok := resolved[name]; ok { continue } - resolved[name] = (name == doer.LowerName) + resolved[name] = false + names = append(names, name) } if err := issue.Repo.getOwner(ctx.e); err != nil { return nil, err } - names := make([]string, 0, len(resolved)) - for name, already := range resolved { - if !already { - names = append(names, name) - } - } - if issue.Repo.Owner.IsOrganization() { // Since there can be users with names that match the name of a team, // if the team exists and can read the issue, the team takes precedence. @@ -1948,18 +1944,18 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti for i := range checked { ids[i] = checked[i].ID } - unchecked := make([]*User, 0, 20) + teamusers := make([]*User, 0, 20) if err := ctx.e. Join("INNER", "team_user", "team_user.uid = `user`.id"). In("`team_user`.team_id", ids). And("`user`.is_active = ?", true). And("`user`.prohibit_login = ?", false). - Find(&unchecked); err != nil { + Find(&teamusers); err != nil { return nil, fmt.Errorf("get teams users: %v", err) } - if len(unchecked) > 0 { - users = make([]*User, 0, len(unchecked)) - for _, user := range unchecked { + if len(teamusers) > 0 { + users = make([]*User, 0, len(teamusers)) + for _, user := range teamusers { if already, ok := resolved[user.LowerName]; !ok || !already { users = append(users, user) resolved[user.LowerName] = true @@ -1969,7 +1965,7 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti } } - // Remove names already in the list + // Remove names already in the list to avoid querying the database if pending names remain names = make([]string, 0, len(resolved)) for name, already := range resolved { if !already { @@ -2002,7 +1998,6 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti continue } users = append(users, user) - resolved[user.LowerName] = true } return diff --git a/models/issue_test.go b/models/issue_test.go index 7909eaf79eff9..5b039bc1d5fff 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -396,5 +396,5 @@ func TestIssue_ResolveMentions(t *testing.T) { // Private repo, not a team member testSuccess("user17", "big_test_private_4", "user20", []string{"user5"}, []int64{}) // Private repo, whole team - testSuccess("user17", "big_test_private_4", "user15", []string{"owners"}, []int64{15, 18}) + testSuccess("user17", "big_test_private_4", "user15", []string{"owners"}, []int64{18}) } From 8b2a714e911e32cf462827b1c43bced415956b06 Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Thu, 10 Oct 2019 12:12:35 -0300 Subject: [PATCH 10/10] Simplify team id list build --- models/issue.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/models/issue.go b/models/issue.go index a8031f1f0817c..f8fa1377a8fb2 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1919,14 +1919,14 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti return nil, fmt.Errorf("find mentioned teams: %v", err) } if len(teams) != 0 { - checked := make([]*Team, 0, len(teams)) + checked := make([]int64, 0, len(teams)) unittype := UnitTypeIssues if issue.IsPull { unittype = UnitTypePullRequests } for _, team := range teams { if team.Authorize >= AccessModeOwner { - checked = append(checked, team) + checked = append(checked, team.ID) resolved[team.LowerName] = true continue } @@ -1935,19 +1935,15 @@ func (issue *Issue) ResolveMentionsByVisibility(ctx DBContext, doer *User, menti return nil, fmt.Errorf("get team units (%d): %v", team.ID, err) } if has { - checked = append(checked, team) + checked = append(checked, team.ID) resolved[team.LowerName] = true } } if len(checked) != 0 { - ids := make([]int64, len(checked)) - for i := range checked { - ids[i] = checked[i].ID - } teamusers := make([]*User, 0, 20) if err := ctx.e. Join("INNER", "team_user", "team_user.uid = `user`.id"). - In("`team_user`.team_id", ids). + In("`team_user`.team_id", checked). And("`user`.is_active = ?", true). And("`user`.prohibit_login = ?", false). Find(&teamusers); err != nil {