diff --git a/internal/db/issue_mail.go b/internal/db/issue_mail.go index d529ecdd9..f094cf98a 100644 --- a/internal/db/issue_mail.go +++ b/internal/db/issue_mail.go @@ -8,6 +8,7 @@ import ( "context" "fmt" + "github.com/pkg/errors" "github.com/unknwon/com" log "unknwon.dev/clog/v2" @@ -99,6 +100,8 @@ func NewMailerIssue(issue *Issue) email.Issue { // 1. Repository watchers, users who participated in comments and the assignee. // 2. Users who are not in 1. but get mentioned in current issue/comment. func mailIssueCommentToParticipants(issue *Issue, doer *User, mentions []string) error { + ctx := context.TODO() + if !conf.User.EnableEmailNotification { return nil } @@ -125,7 +128,7 @@ func mailIssueCommentToParticipants(issue *Issue, doer *User, mentions []string) continue } - to, err := Users.GetByID(context.TODO(), watchers[i].UserID) + to, err := Users.GetByID(ctx, watchers[i].UserID) if err != nil { return fmt.Errorf("GetUserByID [%d]: %v", watchers[i].UserID, err) } @@ -156,15 +159,20 @@ func mailIssueCommentToParticipants(issue *Issue, doer *User, mentions []string) // Mail mentioned people and exclude watchers. names = append(names, doer.Name) - tos = make([]string, 0, len(mentions)) // list of user names. + toUsernames := make([]string, 0, len(mentions)) // list of user names. for i := range mentions { if com.IsSliceContainsStr(names, mentions[i]) { continue } - tos = append(tos, mentions[i]) + toUsernames = append(toUsernames, mentions[i]) } - email.SendIssueMentionMail(NewMailerIssue(issue), NewMailerRepo(issue.Repo), NewMailerUser(doer), GetUserEmailsByNames(tos)) + + tos, err = Users.GetMailableEmailsByUsernames(ctx, toUsernames) + if err != nil { + return errors.Wrap(err, "get mailable emails by usernames") + } + email.SendIssueMentionMail(NewMailerIssue(issue), NewMailerRepo(issue.Repo), NewMailerUser(doer), tos) return nil } diff --git a/internal/db/user.go b/internal/db/user.go index 1048a2eb7..17e0f66b9 100644 --- a/internal/db/user.go +++ b/internal/db/user.go @@ -200,36 +200,12 @@ func DeleteInactivateUsers() (err error) { return err } -// GetUserEmailsByNames returns a list of e-mails corresponds to names. -func GetUserEmailsByNames(names []string) []string { - mails := make([]string, 0, len(names)) - for _, name := range names { - u, err := Users.GetByUsername(context.TODO(), name) - if err != nil { - continue - } - if u.IsMailable() { - mails = append(mails, u.Email) - } - } - return mails -} - // UserCommit represents a commit with validation of user. type UserCommit struct { User *User *git.Commit } -// ValidateCommitWithEmail checks if author's e-mail of commit is corresponding to a user. -func ValidateCommitWithEmail(c *git.Commit) *User { - u, err := Users.GetByEmail(context.TODO(), c.Author.Email) - if err != nil { - return nil - } - return u -} - // ValidateCommitsWithEmails checks if authors' e-mails of commits are corresponding to users. func ValidateCommitsWithEmails(oldCommits []*git.Commit) []*UserCommit { emails := make(map[string]*User) diff --git a/internal/db/users.go b/internal/db/users.go index 468bb3acf..7e986a472 100644 --- a/internal/db/users.go +++ b/internal/db/users.go @@ -73,6 +73,10 @@ type UsersStore interface { // GetByKeyID returns the owner of given public key ID. It returns // ErrUserNotExist when not found. GetByKeyID(ctx context.Context, keyID int64) (*User, error) + // GetMailableEmailsByUsernames returns a list of verified primary email + // addresses (where email notifications are sent to) of users with given list of + // usernames. Non-existing usernames are ignored. + GetMailableEmailsByUsernames(ctx context.Context, usernames []string) ([]string, error) // HasForkedRepository returns true if the user has forked given repository. HasForkedRepository(ctx context.Context, userID, repoID int64) bool // IsUsernameUsed returns true if the given username has been used other than @@ -509,6 +513,15 @@ func (db *users) GetByKeyID(ctx context.Context, keyID int64) (*User, error) { return user, nil } +func (db *users) GetMailableEmailsByUsernames(ctx context.Context, usernames []string) ([]string, error) { + emails := make([]string, 0, len(usernames)) + return emails, db.WithContext(ctx). + Model(&User{}). + Select("email"). + Where("lower_name IN (?) AND is_active = ?", usernames, true). + Find(&emails).Error +} + func (db *users) HasForkedRepository(ctx context.Context, userID, repoID int64) bool { var count int64 db.WithContext(ctx).Model(new(Repository)).Where("owner_id = ? AND fork_id = ?", userID, repoID).Count(&count) @@ -816,11 +829,6 @@ func (u *User) IsOrganization() bool { return u.Type == UserTypeOrganization } -// IsMailable returns true if the user is eligible to receive emails. -func (u *User) IsMailable() bool { - return u.IsActive -} - // APIFormat returns the API format of a user. func (u *User) APIFormat() *api.User { return &api.User{ diff --git a/internal/db/users_test.go b/internal/db/users_test.go index c302d2e89..10f30b811 100644 --- a/internal/db/users_test.go +++ b/internal/db/users_test.go @@ -100,6 +100,7 @@ func TestUsers(t *testing.T) { {"GetByID", usersGetByID}, {"GetByUsername", usersGetByUsername}, {"GetByKeyID", usersGetByKeyID}, + {"GetMailableEmailsByUsernames", usersGetMailableEmailsByUsernames}, {"HasForkedRepository", usersHasForkedRepository}, {"IsUsernameUsed", usersIsUsernameUsed}, {"List", usersList}, @@ -582,6 +583,22 @@ func usersGetByKeyID(t *testing.T, db *users) { assert.Equal(t, wantErr, err) } +func usersGetMailableEmailsByUsernames(t *testing.T, db *users) { + ctx := context.Background() + + alice, err := db.Create(ctx, "alice", "alice@exmaple.com", CreateUserOptions{}) + require.NoError(t, err) + bob, err := db.Create(ctx, "bob", "bob@exmaple.com", CreateUserOptions{Activated: true}) + require.NoError(t, err) + _, err = db.Create(ctx, "cindy", "cindy@exmaple.com", CreateUserOptions{Activated: true}) + require.NoError(t, err) + + got, err := db.GetMailableEmailsByUsernames(ctx, []string{alice.Name, bob.Name, "404"}) + require.NoError(t, err) + want := []string{bob.Email} + assert.Equal(t, want, got) +} + func usersHasForkedRepository(t *testing.T, db *users) { ctx := context.Background() diff --git a/internal/route/lfs/mocks_test.go b/internal/route/lfs/mocks_test.go index 960f7108f..9a51b3951 100644 --- a/internal/route/lfs/mocks_test.go +++ b/internal/route/lfs/mocks_test.go @@ -2319,6 +2319,10 @@ type MockUsersStore struct { // GetByUsernameFunc is an instance of a mock function object // controlling the behavior of the method GetByUsername. GetByUsernameFunc *UsersStoreGetByUsernameFunc + // GetMailableEmailsByUsernamesFunc is an instance of a mock function + // object controlling the behavior of the method + // GetMailableEmailsByUsernames. + GetMailableEmailsByUsernamesFunc *UsersStoreGetMailableEmailsByUsernamesFunc // HasForkedRepositoryFunc is an instance of a mock function object // controlling the behavior of the method HasForkedRepository. HasForkedRepositoryFunc *UsersStoreHasForkedRepositoryFunc @@ -2394,6 +2398,11 @@ func NewMockUsersStore() *MockUsersStore { return }, }, + GetMailableEmailsByUsernamesFunc: &UsersStoreGetMailableEmailsByUsernamesFunc{ + defaultHook: func(context.Context, []string) (r0 []string, r1 error) { + return + }, + }, HasForkedRepositoryFunc: &UsersStoreHasForkedRepositoryFunc{ defaultHook: func(context.Context, int64, int64) (r0 bool) { return @@ -2486,6 +2495,11 @@ func NewStrictMockUsersStore() *MockUsersStore { panic("unexpected invocation of MockUsersStore.GetByUsername") }, }, + GetMailableEmailsByUsernamesFunc: &UsersStoreGetMailableEmailsByUsernamesFunc{ + defaultHook: func(context.Context, []string) ([]string, error) { + panic("unexpected invocation of MockUsersStore.GetMailableEmailsByUsernames") + }, + }, HasForkedRepositoryFunc: &UsersStoreHasForkedRepositoryFunc{ defaultHook: func(context.Context, int64, int64) bool { panic("unexpected invocation of MockUsersStore.HasForkedRepository") @@ -2560,6 +2574,9 @@ func NewMockUsersStoreFrom(i db.UsersStore) *MockUsersStore { GetByUsernameFunc: &UsersStoreGetByUsernameFunc{ defaultHook: i.GetByUsername, }, + GetMailableEmailsByUsernamesFunc: &UsersStoreGetMailableEmailsByUsernamesFunc{ + defaultHook: i.GetMailableEmailsByUsernames, + }, HasForkedRepositoryFunc: &UsersStoreHasForkedRepositoryFunc{ defaultHook: i.HasForkedRepository, }, @@ -3561,6 +3578,118 @@ func (c UsersStoreGetByUsernameFuncCall) Results() []interface{} { return []interface{}{c.Result0, c.Result1} } +// UsersStoreGetMailableEmailsByUsernamesFunc describes the behavior when +// the GetMailableEmailsByUsernames method of the parent MockUsersStore +// instance is invoked. +type UsersStoreGetMailableEmailsByUsernamesFunc struct { + defaultHook func(context.Context, []string) ([]string, error) + hooks []func(context.Context, []string) ([]string, error) + history []UsersStoreGetMailableEmailsByUsernamesFuncCall + mutex sync.Mutex +} + +// GetMailableEmailsByUsernames delegates to the next hook function in the +// queue and stores the parameter and result values of this invocation. +func (m *MockUsersStore) GetMailableEmailsByUsernames(v0 context.Context, v1 []string) ([]string, error) { + r0, r1 := m.GetMailableEmailsByUsernamesFunc.nextHook()(v0, v1) + m.GetMailableEmailsByUsernamesFunc.appendCall(UsersStoreGetMailableEmailsByUsernamesFuncCall{v0, v1, r0, r1}) + return r0, r1 +} + +// SetDefaultHook sets function that is called when the +// GetMailableEmailsByUsernames method of the parent MockUsersStore instance +// is invoked and the hook queue is empty. +func (f *UsersStoreGetMailableEmailsByUsernamesFunc) SetDefaultHook(hook func(context.Context, []string) ([]string, error)) { + f.defaultHook = hook +} + +// PushHook adds a function to the end of hook queue. Each invocation of the +// GetMailableEmailsByUsernames method of the parent MockUsersStore instance +// invokes the hook at the front of the queue and discards it. After the +// queue is empty, the default hook function is invoked for any future +// action. +func (f *UsersStoreGetMailableEmailsByUsernamesFunc) PushHook(hook func(context.Context, []string) ([]string, error)) { + f.mutex.Lock() + f.hooks = append(f.hooks, hook) + f.mutex.Unlock() +} + +// SetDefaultReturn calls SetDefaultHook with a function that returns the +// given values. +func (f *UsersStoreGetMailableEmailsByUsernamesFunc) SetDefaultReturn(r0 []string, r1 error) { + f.SetDefaultHook(func(context.Context, []string) ([]string, error) { + return r0, r1 + }) +} + +// PushReturn calls PushHook with a function that returns the given values. +func (f *UsersStoreGetMailableEmailsByUsernamesFunc) PushReturn(r0 []string, r1 error) { + f.PushHook(func(context.Context, []string) ([]string, error) { + return r0, r1 + }) +} + +func (f *UsersStoreGetMailableEmailsByUsernamesFunc) nextHook() func(context.Context, []string) ([]string, error) { + f.mutex.Lock() + defer f.mutex.Unlock() + + if len(f.hooks) == 0 { + return f.defaultHook + } + + hook := f.hooks[0] + f.hooks = f.hooks[1:] + return hook +} + +func (f *UsersStoreGetMailableEmailsByUsernamesFunc) appendCall(r0 UsersStoreGetMailableEmailsByUsernamesFuncCall) { + f.mutex.Lock() + f.history = append(f.history, r0) + f.mutex.Unlock() +} + +// History returns a sequence of +// UsersStoreGetMailableEmailsByUsernamesFuncCall objects describing the +// invocations of this function. +func (f *UsersStoreGetMailableEmailsByUsernamesFunc) History() []UsersStoreGetMailableEmailsByUsernamesFuncCall { + f.mutex.Lock() + history := make([]UsersStoreGetMailableEmailsByUsernamesFuncCall, len(f.history)) + copy(history, f.history) + f.mutex.Unlock() + + return history +} + +// UsersStoreGetMailableEmailsByUsernamesFuncCall is an object that +// describes an invocation of method GetMailableEmailsByUsernames on an +// instance of MockUsersStore. +type UsersStoreGetMailableEmailsByUsernamesFuncCall struct { + // Arg0 is the value of the 1st argument passed to this method + // invocation. + Arg0 context.Context + // Arg1 is the value of the 2nd argument passed to this method + // invocation. + Arg1 []string + // Result0 is the value of the 1st result returned from this method + // invocation. + Result0 []string + // Result1 is the value of the 2nd result returned from this method + // invocation. + Result1 error +} + +// Args returns an interface slice containing the arguments of this +// invocation. +func (c UsersStoreGetMailableEmailsByUsernamesFuncCall) Args() []interface{} { + return []interface{}{c.Arg0, c.Arg1} +} + +// Results returns an interface slice containing the results of this +// invocation. +func (c UsersStoreGetMailableEmailsByUsernamesFuncCall) Results() []interface{} { + return []interface{}{c.Result0, c.Result1} +} + // UsersStoreHasForkedRepositoryFunc describes the behavior when the // HasForkedRepository method of the parent MockUsersStore instance is // invoked. diff --git a/internal/route/repo/commit.go b/internal/route/repo/commit.go index 3e09bd06f..da8c5e48b 100644 --- a/internal/route/repo/commit.go +++ b/internal/route/repo/commit.go @@ -5,6 +5,7 @@ package repo import ( + gocontext "context" "path" "time" @@ -110,6 +111,13 @@ func FileHistory(c *context.Context) { renderCommits(c, c.Repo.TreePath) } +// tryGetUserByEmail returns a non-nil value if the email is corresponding to an +// existing user. +func tryGetUserByEmail(ctx gocontext.Context, email string) *db.User { + user, _ := db.Users.GetByEmail(ctx, email) + return user +} + func Diff(c *context.Context) { c.PageIs("Diff") c.RequireHighlightJS() @@ -156,7 +164,7 @@ func Diff(c *context.Context) { c.Data["IsImageFile"] = commit.IsImageFile c.Data["IsImageFileByIndex"] = commit.IsImageFileByIndex c.Data["Commit"] = commit - c.Data["Author"] = db.ValidateCommitWithEmail(commit) + c.Data["Author"] = tryGetUserByEmail(c.Req.Context(), commit.Author.Email) c.Data["Diff"] = diff c.Data["Parents"] = parents c.Data["DiffNotAvailable"] = diff.NumFiles() == 0 diff --git a/internal/route/repo/view.go b/internal/route/repo/view.go index f4b6a162a..ac5ed3335 100644 --- a/internal/route/repo/view.go +++ b/internal/route/repo/view.go @@ -111,7 +111,7 @@ func renderDirectory(c *context.Context, treeLink string) { } } c.Data["LatestCommit"] = latestCommit - c.Data["LatestCommitUser"] = db.ValidateCommitWithEmail(latestCommit) + c.Data["LatestCommitUser"] = tryGetUserByEmail(c.Req.Context(), latestCommit.Author.Email) if c.Repo.CanEnableEditor() { c.Data["CanAddFile"] = true