Skip to content

remove context from mail struct #33811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 6, 2025
Merged

remove context from mail struct #33811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d061c2
remove context from mail struct
TheFox0x7 Mar 5, 2025
2b642aa
add a very basic mail test
TheFox0x7 Mar 6, 2025
f3b51f0
Merge branch 'main' into remove-mail-context
lunny Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions services/mailer/mail_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@ func MailParticipantsComment(ctx context.Context, c *issues_model.Comment, opTyp
if c.Type == issues_model.CommentTypePullRequestPush {
content = ""
}
if err := mailIssueCommentToParticipants(
&mailCommentContext{
Context: ctx,
if err := mailIssueCommentToParticipants(ctx,
&mailComment{
Issue: issue,
Doer: c.Poster,
ActionType: opType,
Expand All @@ -48,9 +47,8 @@ func MailMentionsComment(ctx context.Context, pr *issues_model.PullRequest, c *i

visited := make(container.Set[int64], len(mentions)+1)
visited.Add(c.Poster.ID)
if err = mailIssueCommentBatch(
&mailCommentContext{
Context: ctx,
if err = mailIssueCommentBatch(ctx,
&mailComment{
Issue: pr.Issue,
Doer: c.Poster,
ActionType: activities_model.ActionCommentPull,
Expand Down
56 changes: 27 additions & 29 deletions services/mailer/mail_issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,88 +24,88 @@ const MailBatchSize = 100 // batch size used in mailIssueCommentBatch
// This function sends two list of emails:
// 1. Repository watchers (except for WIP pull requests) and users who are participated in comments.
// 2. Users who are not in 1. but get mentioned in current issue/comment.
func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*user_model.User) error {
func mailIssueCommentToParticipants(ctx context.Context, comment *mailComment, mentions []*user_model.User) error {
// Required by the mail composer; make sure to load these before calling the async function
if err := ctx.Issue.LoadRepo(ctx); err != nil {
if err := comment.Issue.LoadRepo(ctx); err != nil {
return fmt.Errorf("LoadRepo: %w", err)
}
if err := ctx.Issue.LoadPoster(ctx); err != nil {
if err := comment.Issue.LoadPoster(ctx); err != nil {
return fmt.Errorf("LoadPoster: %w", err)
}
if err := ctx.Issue.LoadPullRequest(ctx); err != nil {
if err := comment.Issue.LoadPullRequest(ctx); err != nil {
return fmt.Errorf("LoadPullRequest: %w", err)
}

// Enough room to avoid reallocations
unfiltered := make([]int64, 1, 64)

// =========== Original poster ===========
unfiltered[0] = ctx.Issue.PosterID
unfiltered[0] = comment.Issue.PosterID

// =========== Assignees ===========
ids, err := issues_model.GetAssigneeIDsByIssue(ctx, ctx.Issue.ID)
ids, err := issues_model.GetAssigneeIDsByIssue(ctx, comment.Issue.ID)
if err != nil {
return fmt.Errorf("GetAssigneeIDsByIssue(%d): %w", ctx.Issue.ID, err)
return fmt.Errorf("GetAssigneeIDsByIssue(%d): %w", comment.Issue.ID, err)
}
unfiltered = append(unfiltered, ids...)

// =========== Participants (i.e. commenters, reviewers) ===========
ids, err = issues_model.GetParticipantsIDsByIssueID(ctx, ctx.Issue.ID)
ids, err = issues_model.GetParticipantsIDsByIssueID(ctx, comment.Issue.ID)
if err != nil {
return fmt.Errorf("GetParticipantsIDsByIssueID(%d): %w", ctx.Issue.ID, err)
return fmt.Errorf("GetParticipantsIDsByIssueID(%d): %w", comment.Issue.ID, err)
}
unfiltered = append(unfiltered, ids...)

// =========== Issue watchers ===========
ids, err = issues_model.GetIssueWatchersIDs(ctx, ctx.Issue.ID, true)
ids, err = issues_model.GetIssueWatchersIDs(ctx, comment.Issue.ID, true)
if err != nil {
return fmt.Errorf("GetIssueWatchersIDs(%d): %w", ctx.Issue.ID, err)
return fmt.Errorf("GetIssueWatchersIDs(%d): %w", comment.Issue.ID, err)
}
unfiltered = append(unfiltered, ids...)

// =========== Repo watchers ===========
// Make repo watchers last, since it's likely the list with the most users
if !(ctx.Issue.IsPull && ctx.Issue.PullRequest.IsWorkInProgress(ctx) && ctx.ActionType != activities_model.ActionCreatePullRequest) {
ids, err = repo_model.GetRepoWatchersIDs(ctx, ctx.Issue.RepoID)
if !(comment.Issue.IsPull && comment.Issue.PullRequest.IsWorkInProgress(ctx) && comment.ActionType != activities_model.ActionCreatePullRequest) {
ids, err = repo_model.GetRepoWatchersIDs(ctx, comment.Issue.RepoID)
if err != nil {
return fmt.Errorf("GetRepoWatchersIDs(%d): %w", ctx.Issue.RepoID, err)
return fmt.Errorf("GetRepoWatchersIDs(%d): %w", comment.Issue.RepoID, err)
}
unfiltered = append(ids, unfiltered...)
}

visited := make(container.Set[int64], len(unfiltered)+len(mentions)+1)

// Avoid mailing the doer
if ctx.Doer.EmailNotificationsPreference != user_model.EmailNotificationsAndYourOwn && !ctx.ForceDoerNotification {
visited.Add(ctx.Doer.ID)
if comment.Doer.EmailNotificationsPreference != user_model.EmailNotificationsAndYourOwn && !comment.ForceDoerNotification {
visited.Add(comment.Doer.ID)
}

// =========== Mentions ===========
if err = mailIssueCommentBatch(ctx, mentions, visited, true); err != nil {
if err = mailIssueCommentBatch(ctx, comment, mentions, visited, true); err != nil {
return fmt.Errorf("mailIssueCommentBatch() mentions: %w", err)
}

// Avoid mailing explicit unwatched
ids, err = issues_model.GetIssueWatchersIDs(ctx, ctx.Issue.ID, false)
ids, err = issues_model.GetIssueWatchersIDs(ctx, comment.Issue.ID, false)
if err != nil {
return fmt.Errorf("GetIssueWatchersIDs(%d): %w", ctx.Issue.ID, err)
return fmt.Errorf("GetIssueWatchersIDs(%d): %w", comment.Issue.ID, err)
}
visited.AddMultiple(ids...)

unfilteredUsers, err := user_model.GetMailableUsersByIDs(ctx, unfiltered, false)
if err != nil {
return err
}
if err = mailIssueCommentBatch(ctx, unfilteredUsers, visited, false); err != nil {
if err = mailIssueCommentBatch(ctx, comment, unfilteredUsers, visited, false); err != nil {
return fmt.Errorf("mailIssueCommentBatch(): %w", err)
}

return nil
}

func mailIssueCommentBatch(ctx *mailCommentContext, users []*user_model.User, visited container.Set[int64], fromMention bool) error {
func mailIssueCommentBatch(ctx context.Context, comment *mailComment, users []*user_model.User, visited container.Set[int64], fromMention bool) error {
checkUnit := unit.TypeIssues
if ctx.Issue.IsPull {
if comment.Issue.IsPull {
checkUnit = unit.TypePullRequests
}

Expand All @@ -129,7 +129,7 @@ func mailIssueCommentBatch(ctx *mailCommentContext, users []*user_model.User, vi
}

// test if this user is allowed to see the issue/pull
if !access_model.CheckRepoUnitUser(ctx, ctx.Issue.Repo, user, checkUnit) {
if !access_model.CheckRepoUnitUser(ctx, comment.Issue.Repo, user, checkUnit) {
continue
}

Expand All @@ -141,7 +141,7 @@ func mailIssueCommentBatch(ctx *mailCommentContext, users []*user_model.User, vi
// working backwards from the last (possibly) incomplete batch. If len(receivers) can be 0 this
// starting condition will need to be changed slightly
for i := ((len(receivers) - 1) / MailBatchSize) * MailBatchSize; i >= 0; i -= MailBatchSize {
msgs, err := composeIssueCommentMessages(ctx, lang, receivers[i:], fromMention, "issue comments")
msgs, err := composeIssueCommentMessages(ctx, comment, lang, receivers[i:], fromMention, "issue comments")
if err != nil {
return err
}
Expand All @@ -168,9 +168,8 @@ func MailParticipants(ctx context.Context, issue *issues_model.Issue, doer *user
content = ""
}
forceDoerNotification := opType == activities_model.ActionAutoMergePullRequest
if err := mailIssueCommentToParticipants(
&mailCommentContext{
Context: ctx,
if err := mailIssueCommentToParticipants(ctx,
&mailComment{
Issue: issue,
Doer: doer,
ActionType: opType,
Expand Down Expand Up @@ -205,8 +204,7 @@ func SendIssueAssignedMail(ctx context.Context, issue *issues_model.Issue, doer
}

for lang, tos := range langMap {
msgs, err := composeIssueCommentMessages(&mailCommentContext{
Context: ctx,
msgs, err := composeIssueCommentMessages(ctx, &mailComment{
Issue: issue,
Doer: doer,
ActionType: activities_model.ActionType(0),
Expand Down
65 changes: 32 additions & 33 deletions services/mailer/mail_issue_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@ func fallbackMailSubject(issue *issues_model.Issue) string {
return fmt.Sprintf("[%s] %s (#%d)", issue.Repo.FullName(), issue.Title, issue.Index)
}

type mailCommentContext struct {
context.Context
type mailComment struct {
Issue *issues_model.Issue
Doer *user_model.User
ActionType activities_model.ActionType
Expand All @@ -43,7 +42,7 @@ type mailCommentContext struct {
ForceDoerNotification bool
}

func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipients []*user_model.User, fromMention bool, info string) ([]*sender_service.Message, error) {
func composeIssueCommentMessages(ctx context.Context, comment *mailComment, lang string, recipients []*user_model.User, fromMention bool, info string) ([]*sender_service.Message, error) {
var (
subject string
link string
Expand All @@ -54,44 +53,44 @@ func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipient
)

commentType := issues_model.CommentTypeComment
if ctx.Comment != nil {
commentType = ctx.Comment.Type
link = ctx.Issue.HTMLURL() + "#" + ctx.Comment.HashTag()
if comment.Comment != nil {
commentType = comment.Comment.Type
link = comment.Issue.HTMLURL() + "#" + comment.Comment.HashTag()
} else {
link = ctx.Issue.HTMLURL()
link = comment.Issue.HTMLURL()
}

reviewType := issues_model.ReviewTypeComment
if ctx.Comment != nil && ctx.Comment.Review != nil {
reviewType = ctx.Comment.Review.Type
if comment.Comment != nil && comment.Comment.Review != nil {
reviewType = comment.Comment.Review.Type
}

// This is the body of the new issue or comment, not the mail body
rctx := renderhelper.NewRenderContextRepoComment(ctx.Context, ctx.Issue.Repo).WithUseAbsoluteLink(true)
body, err := markdown.RenderString(rctx, ctx.Content)
rctx := renderhelper.NewRenderContextRepoComment(ctx, comment.Issue.Repo).WithUseAbsoluteLink(true)
body, err := markdown.RenderString(rctx, comment.Content)
if err != nil {
return nil, err
}

if setting.MailService.EmbedAttachmentImages {
attEmbedder := newMailAttachmentBase64Embedder(ctx.Doer, ctx.Issue.Repo, maxEmailBodySize)
attEmbedder := newMailAttachmentBase64Embedder(comment.Doer, comment.Issue.Repo, maxEmailBodySize)
bodyAfterEmbedding, err := attEmbedder.Base64InlineImages(ctx, body)
if err != nil {
log.Error("Failed to embed images in mail body: %v", err)
} else {
body = bodyAfterEmbedding
}
}
actType, actName, tplName := actionToTemplate(ctx.Issue, ctx.ActionType, commentType, reviewType)
actType, actName, tplName := actionToTemplate(comment.Issue, comment.ActionType, commentType, reviewType)

if actName != "new" {
prefix = "Re: "
}
fallback = prefix + fallbackMailSubject(ctx.Issue)
fallback = prefix + fallbackMailSubject(comment.Issue)

if ctx.Comment != nil && ctx.Comment.Review != nil {
if comment.Comment != nil && comment.Comment.Review != nil {
reviewComments = make([]*issues_model.Comment, 0, 10)
for _, lines := range ctx.Comment.Review.CodeComments {
for _, lines := range comment.Comment.Review.CodeComments {
for _, comments := range lines {
reviewComments = append(reviewComments, comments...)
}
Expand All @@ -104,12 +103,12 @@ func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipient
"FallbackSubject": fallback,
"Body": body,
"Link": link,
"Issue": ctx.Issue,
"Comment": ctx.Comment,
"IsPull": ctx.Issue.IsPull,
"User": ctx.Issue.Repo.MustOwner(ctx),
"Repo": ctx.Issue.Repo.FullName(),
"Doer": ctx.Doer,
"Issue": comment.Issue,
"Comment": comment.Comment,
"IsPull": comment.Issue.IsPull,
"User": comment.Issue.Repo.MustOwner(ctx),
"Repo": comment.Issue.Repo.FullName(),
"Doer": comment.Doer,
"IsMention": fromMention,
"SubjectPrefix": prefix,
"ActionType": actType,
Expand Down Expand Up @@ -140,22 +139,22 @@ func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipient
}

// Make sure to compose independent messages to avoid leaking user emails
msgID := generateMessageIDForIssue(ctx.Issue, ctx.Comment, ctx.ActionType)
reference := generateMessageIDForIssue(ctx.Issue, nil, activities_model.ActionType(0))
msgID := generateMessageIDForIssue(comment.Issue, comment.Comment, comment.ActionType)
reference := generateMessageIDForIssue(comment.Issue, nil, activities_model.ActionType(0))

var replyPayload []byte
if ctx.Comment != nil {
if ctx.Comment.Type.HasMailReplySupport() {
replyPayload, err = incoming_payload.CreateReferencePayload(ctx.Comment)
if comment.Comment != nil {
if comment.Comment.Type.HasMailReplySupport() {
replyPayload, err = incoming_payload.CreateReferencePayload(comment.Comment)
}
} else {
replyPayload, err = incoming_payload.CreateReferencePayload(ctx.Issue)
replyPayload, err = incoming_payload.CreateReferencePayload(comment.Issue)
}
if err != nil {
return nil, err
}

unsubscribePayload, err := incoming_payload.CreateReferencePayload(ctx.Issue)
unsubscribePayload, err := incoming_payload.CreateReferencePayload(comment.Issue)
if err != nil {
return nil, err
}
Expand All @@ -164,7 +163,7 @@ func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipient
for _, recipient := range recipients {
msg := sender_service.NewMessageFrom(
recipient.Email,
fromDisplayName(ctx.Doer),
fromDisplayName(comment.Doer),
setting.MailService.FromEmail,
subject,
mailBody.String(),
Expand All @@ -175,7 +174,7 @@ func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipient
msg.SetHeader("In-Reply-To", reference)

references := []string{reference}
listUnsubscribe := []string{"<" + ctx.Issue.HTMLURL() + ">"}
listUnsubscribe := []string{"<" + comment.Issue.HTMLURL() + ">"}

if setting.IncomingEmail.Enabled {
if replyPayload != nil {
Expand Down Expand Up @@ -203,7 +202,7 @@ func composeIssueCommentMessages(ctx *mailCommentContext, lang string, recipient
msg.SetHeader("References", references...)
msg.SetHeader("List-Unsubscribe", listUnsubscribe...)

for key, value := range generateAdditionalHeaders(ctx, actType, recipient) {
for key, value := range generateAdditionalHeaders(comment, actType, recipient) {
msg.SetHeader(key, value)
}

Expand Down Expand Up @@ -303,7 +302,7 @@ func generateMessageIDForIssue(issue *issues_model.Issue, comment *issues_model.
return fmt.Sprintf("<%s/%s/%d%s@%s>", issue.Repo.FullName(), path, issue.Index, extra, setting.Domain)
}

func generateAdditionalHeaders(ctx *mailCommentContext, reason string, recipient *user_model.User) map[string]string {
func generateAdditionalHeaders(ctx *mailComment, reason string, recipient *user_model.User) map[string]string {
repo := ctx.Issue.Repo

return map[string]string{
Expand Down
Loading