enhance: allow custom watches to bypass on-mention email restriction (#39652) (#39837)

Backport #39652 by @Ubayed-Bin-Sufian

Users with "Only Email on Mention" can now still receive email for the
events they picked in a custom repository watch, covering issues, pull
requests and releases. The setting is renamed to "Only Email on Mention
and Custom Watches" to make this explicit, matching GitHub's
"Participating, @mentions and custom".

"All activity" watches don't bypass the setting, because they are also
created automatically by `AUTO_WATCH_NEW_REPOS` for repository creators
and team members. To keep picking every event distinct from "All
activity", the custom watch dialog now stores a new `WatchModeCustom`.
Existing custom watches keep working without a migration.

Addresses: https://github.com/openSUSE/openSUSE-git/issues/183
Ref: https://github.com/go-gitea/gitea/issues/35492
Co-authored-by: Ubayed Bin Sufian <ubayedbinsufian@gmail.com>
Co-authored-by: bircni <bircni@icloud.com>

models/issues/issue_search.go

@@ -477,7 +477,7 @@ func applySubscribedCondition(sess db.Session, subscriberID int64) {
 				Select("repo_id").
 				From("watch").
 				Where(builder.And(builder.Eq{"user_id": subscriberID},
-					builder.In("mode", repo_model.WatchModeNormal, repo_model.WatchModeAuto))),
+					builder.In("mode", repo_model.WatchModeNormal, repo_model.WatchModeAuto, repo_model.WatchModeCustom))),
 			),
 		),
 	)

models/repo/watch.go

@@ -20,9 +20,10 @@ type WatchMode int8
 const (
 	WatchModeNone WatchMode = iota // 0 watch nothing unless mentioned
 
-	WatchModeNormal // 1 proactively watching (all or custom)
+	WatchModeNormal // 1 proactively watching (all, or custom before WatchModeCustom)
 	WatchModeDont   // 2 ignore the repo
 	WatchModeAuto   // 3 automatically watching (from AutoWatchOnChanges)
+	WatchModeCustom // 4 watching the events picked in the custom watch options
 )
 
 // WatchType is the `watch` column gating one kind of notification
@@ -79,7 +80,7 @@ func (w *Watch) IsWatching() bool {
 
 // IsWatchingAll reports whether every event is enabled, which is the "all activity" mode
 func (w *Watch) IsWatchingAll() bool {
-	return w.IncludePullRequests && w.IncludeIssues && w.IncludeReleases
+	return w.Mode != WatchModeCustom && w.IncludePullRequests && w.IncludeIssues && w.IncludeReleases
 }
 
 func (w *Watch) IsWatchingAny() bool {
@@ -166,6 +167,9 @@ func WatchRepoAuto(ctx context.Context, doer *user_model.User, repo *Repository,
 	if user_model.IsUserBlockedBy(ctx, doer, repo.OwnerID) {
 		return user_model.ErrBlockedUser
 	}
+	if watch.Mode == WatchModeCustom { // keep the events the user picked
+		return nil
+	}
 
 	return watchRepoByMode(ctx, watch, WatchModeNormal)
 }
@@ -189,7 +193,7 @@ func WatchRepoWithOptions(ctx context.Context, doer *user_model.User, repo *Repo
 		if err != nil {
 			return err
 		}
-		if opts.Mode == WatchModeNormal {
+		if opts.Mode == WatchModeNormal || opts.Mode == WatchModeCustom {
 			_, err = db.GetEngine(ctx).Where("user_id=? AND repo_id=?", doer.ID, repo.ID).
 				Cols("include_pull_requests", "include_issues", "include_releases").
 				Update(&Watch{IncludePullRequests: opts.WatchPullRequests, IncludeIssues: opts.WatchIssues, IncludeReleases: opts.WatchReleases})
@@ -247,6 +251,16 @@ func GetRepoIgnorersIDs(ctx context.Context, repoID int64) ([]int64, error) {
 // but avoids joining with `user` for performance reasons
 // User permissions must be verified elsewhere if required
 func GetRepoWatchersIDs(ctx context.Context, repoID int64, watchType WatchType) ([]int64, error) {
+	return getRepoWatchersIDs(ctx, repoID, watchType, false)
+}
+
+// GetRepoCustomWatchersIDs is GetRepoWatchersIDs limited to custom watches, which unlike
+// watching all activity are never set up automatically (e.g. by AutoWatchNewRepos)
+func GetRepoCustomWatchersIDs(ctx context.Context, repoID int64, watchType WatchType) ([]int64, error) {
+	return getRepoWatchersIDs(ctx, repoID, watchType, true)
+}
+
+func getRepoWatchersIDs(ctx context.Context, repoID int64, watchType WatchType, customOnly bool) ([]int64, error) {
 	ids := make([]int64, 0, 64)
 	var watchColName string
 	switch watchType {
@@ -259,12 +273,19 @@ func GetRepoWatchersIDs(ctx context.Context, repoID int64, watchType WatchType)
 	default:
 		panic("invalid WatchType")
 	}
-	return ids, db.GetEngine(ctx).Table("watch").
+	sess := db.GetEngine(ctx).Table("watch").
 		Where("watch.repo_id=?", repoID).
 		And("watch.mode<>?", WatchModeDont).
-		And(builder.Eq{watchColName: true}).
-		Select("user_id").
-		Find(&ids)
+		And(builder.Eq{watchColName: true})
+	if customOnly { // matches SelectedMode() == "custom"
+		sess = sess.And(builder.Or(
+			builder.Eq{"watch.mode": WatchModeCustom},
+			builder.Eq{"watch.include_pull_requests": false},
+			builder.Eq{"watch.include_issues": false},
+			builder.Eq{"watch.include_releases": false},
+		))
+	}
+	return ids, sess.Select("user_id").Find(&ids)
 }
 
 // GetRepoWatchers returns range of users watching given repository.

models/repo/watch_test.go

@@ -145,7 +145,7 @@ func TestWatchOptions(t *testing.T) {
 	// repo 1 is watched by users 1, 4, 9 and 11, all with every event enabled
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
 	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
-	assert.NoError(t, repo_model.WatchRepoWithOptions(t.Context(), user, repo, repo_model.WatchOptions{Mode: repo_model.WatchModeNormal, WatchPullRequests: true}))
+	assert.NoError(t, repo_model.WatchRepoWithOptions(t.Context(), user, repo, repo_model.WatchOptions{Mode: repo_model.WatchModeNormal, WatchPullRequests: true})) // custom watches were stored like this before WatchModeCustom
 
 	for watchType, expected := range map[repo_model.WatchType][]int64{
 		repo_model.WatchPullRequests: {1, 4, 9, 11},
@@ -157,6 +157,20 @@ func TestWatchOptions(t *testing.T) {
 		assert.ElementsMatch(t, expected, ids, watchType)
 	}
 
+	// a custom watch picking every event differs from all activity, which can be set up automatically
+	customUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+	assert.NoError(t, repo_model.WatchRepoWithOptions(t.Context(), customUser, repo, repo_model.WatchOptions{Mode: repo_model.WatchModeCustom, WatchPullRequests: true, WatchIssues: true, WatchReleases: true}))
+	assert.NoError(t, repo_model.WatchRepoAuto(t.Context(), customUser, repo, true)) // must keep the custom watch
+	for watchType, expected := range map[repo_model.WatchType][]int64{
+		repo_model.WatchPullRequests: {1, 4},
+		repo_model.WatchIssues:       {4},
+		repo_model.WatchReleases:     {4},
+	} {
+		ids, err := repo_model.GetRepoCustomWatchersIDs(t.Context(), repo.ID, watchType)
+		assert.NoError(t, err)
+		assert.ElementsMatch(t, expected, ids, watchType)
+	}
+
 	// the options of one user must not show up for another
 	watches, err := repo_model.GetUserWatches(t.Context(), 4, []int64{repo.ID})
 	assert.NoError(t, err)
@@ -176,5 +190,6 @@ func TestWatchSelectedMode(t *testing.T) {
 	assert.Equal(t, "participate", (&repo_model.Watch{Mode: repo_model.WatchModeNormal}).SelectedMode())
 	assert.Equal(t, "ignore", (&repo_model.Watch{Mode: repo_model.WatchModeDont}).SelectedMode())
 	assert.Equal(t, "custom", (&repo_model.Watch{Mode: repo_model.WatchModeNormal, IncludeIssues: true}).SelectedMode())
+	assert.Equal(t, "custom", (&repo_model.Watch{Mode: repo_model.WatchModeCustom, IncludePullRequests: true, IncludeIssues: true, IncludeReleases: true}).SelectedMode())
 	assert.Equal(t, "all", (&repo_model.Watch{Mode: repo_model.WatchModeAuto, IncludePullRequests: true, IncludeIssues: true, IncludeReleases: true}).SelectedMode())
 }

options/locale/locale_en-US.json

@@ -930,7 +930,7 @@
   "settings.delete_account_title": "Delete User Account",
   "settings.delete_account_desc": "Are you sure you want to permanently delete this user account?",
   "settings.email_notifications.enable": "Enable Email Notifications",
-  "settings.email_notifications.onmention": "Only Email on Mention",
+  "settings.email_notifications.onmention_and_custom": "Only Email on Mention and Custom Watches",
   "settings.email_notifications.disable": "Disable Email Notifications",
   "settings.email_notifications.submit": "Set Email Preference",
   "settings.email_notifications.andyourown": "And Your Own Notifications",

routers/web/repo/watch.go

@@ -49,7 +49,7 @@ func ActionWatch(ctx *context.Context) {
 // ActionWatchOptions watches the repository with a custom selection of events
 func ActionWatchOptions(ctx *context.Context) {
 	opts := repo_model.WatchOptions{ // clearing every event is allowed, it leaves the participating state
-		Mode:              repo_model.WatchModeNormal,
+		Mode:              repo_model.WatchModeCustom,
 		WatchPullRequests: ctx.FormBool("pull_requests"),
 		WatchIssues:       ctx.FormBool("issues"),
 		WatchReleases:     ctx.FormBool("releases"),

services/mailer/mail_comment.go

@@ -54,7 +54,7 @@ func MailMentionsComment(ctx context.Context, pr *issues_model.PullRequest, c *i
 			ActionType: activities_model.ActionCommentPull,
 			Content:    c.Content,
 			Comment:    c,
-		}, mentions, visited, true); err != nil {
+		}, mentions, visited, true, nil); err != nil {
 		log.Error("mailIssueCommentBatch: %v", err)
 	}
 	return nil

services/mailer/mail_issue.go

@@ -66,6 +66,7 @@ func mailIssueCommentToParticipants(ctx context.Context, comment *mailComment, m
 
 	// =========== Repo watchers ===========
 	// Make repo watchers last, since it's likely the list with the most users
+	var customWatchers container.Set[int64]
 	if !(comment.Issue.IsPull && comment.Issue.PullRequest.IsWorkInProgress(ctx) && comment.ActionType != activities_model.ActionCreatePullRequest) {
 		watchType := util.Iif(comment.Issue.IsPull, repo_model.WatchPullRequests, repo_model.WatchIssues)
 		ids, err = repo_model.GetRepoWatchersIDs(ctx, comment.Issue.RepoID, watchType)
@@ -73,6 +74,12 @@ func mailIssueCommentToParticipants(ctx context.Context, comment *mailComment, m
 			return fmt.Errorf("GetRepoWatchersIDs(%d): %w", comment.Issue.RepoID, err)
 		}
 		unfiltered = append(ids, unfiltered...)
+
+		ids, err = repo_model.GetRepoCustomWatchersIDs(ctx, comment.Issue.RepoID, watchType)
+		if err != nil {
+			return fmt.Errorf("GetRepoCustomWatchersIDs(%d): %w", comment.Issue.RepoID, err)
+		}
+		customWatchers = container.SetOf(ids...)
 	}
 
 	visited := make(container.Set[int64], len(unfiltered)+len(mentions)+1)
@@ -90,7 +97,7 @@ func mailIssueCommentToParticipants(ctx context.Context, comment *mailComment, m
 	}
 
 	// =========== Mentions ===========
-	if err = mailIssueCommentBatch(ctx, comment, mentions, visited, true); err != nil {
+	if err = mailIssueCommentBatch(ctx, comment, mentions, visited, true, nil); err != nil {
 		return fmt.Errorf("mailIssueCommentBatch() mentions: %w", err)
 	}
 
@@ -101,18 +108,18 @@ func mailIssueCommentToParticipants(ctx context.Context, comment *mailComment, m
 	}
 	visited.AddMultiple(ids...)
 
-	unfilteredUsers, err := user_model.GetMailableUsersByIDs(ctx, unfiltered, false)
+	unfilteredUsers, err := user_model.GetMailableUsersByIDs(ctx, unfiltered, len(customWatchers) > 0)
 	if err != nil {
 		return err
 	}
-	if err = mailIssueCommentBatch(ctx, comment, unfilteredUsers, visited, false); err != nil {
+	if err = mailIssueCommentBatch(ctx, comment, unfilteredUsers, visited, false, customWatchers); err != nil {
 		return fmt.Errorf("mailIssueCommentBatch(): %w", err)
 	}
 
 	return nil
 }
 
-func mailIssueCommentBatch(ctx context.Context, comment *mailComment, 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, customWatchers container.Set[int64]) error {
 	checkUnit := unit.TypeIssues
 	if comment.Issue.IsPull {
 		checkUnit = unit.TypePullRequests
@@ -124,10 +131,10 @@ func mailIssueCommentBatch(ctx context.Context, comment *mailComment, users []*u
 			continue
 		}
 		// At this point we exclude:
-		// user that don't have all mails enabled or users only get mail on mention and this is one ...
+		// user that don't have all mails enabled or users only get mail on mention or custom watch and this is neither ...
 		if !(user.EmailNotificationsPreference == user_model.EmailNotificationsEnabled ||
 			user.EmailNotificationsPreference == user_model.EmailNotificationsAndYourOwn ||
-			fromMention && user.EmailNotificationsPreference == user_model.EmailNotificationsOnMention) {
+			user.EmailNotificationsPreference == user_model.EmailNotificationsOnMention && (fromMention || customWatchers.Contains(user.ID))) {
 			continue
 		}
 

services/mailer/mail_release.go

@@ -14,6 +14,7 @@ import (
 	repo_model "gitea.dev/models/repo"
 	"gitea.dev/models/unit"
 	user_model "gitea.dev/models/user"
+	"gitea.dev/modules/container"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/markup/markdown"
 	"gitea.dev/modules/setting"
@@ -41,7 +42,14 @@ func MailNewRelease(ctx context.Context, rel *repo_model.Release) {
 		return
 	}
 
-	recipients, err := user_model.GetMailableUsersByIDs(ctx, watcherIDList, false)
+	customWatcherIDs, err := repo_model.GetRepoCustomWatchersIDs(ctx, rel.RepoID, repo_model.WatchReleases)
+	if err != nil {
+		log.Error("GetRepoCustomWatchersIDs(%d): %v", rel.RepoID, err)
+		return
+	}
+	customWatchers := container.SetOf(customWatcherIDs...)
+
+	recipients, err := user_model.GetMailableUsersByIDs(ctx, watcherIDList, len(customWatchers) > 0)
 	if err != nil {
 		log.Error("user_model.GetMailableUsersByIDs: %v", err)
 		return
@@ -52,9 +60,11 @@ func MailNewRelease(ctx context.Context, rel *repo_model.Release) {
 		return
 	}
 
-	// delete publisher or any users with no permission
+	// delete publisher, on-mention users without a custom watch or any users with no permission
 	recipients = slices.DeleteFunc(recipients, func(u *user_model.User) bool {
-		return u.ID == rel.PublisherID || !access_model.CheckRepoUnitUser(ctx, rel.Repo, u, unit.TypeReleases)
+		return u.ID == rel.PublisherID ||
+			u.EmailNotificationsPreference == user_model.EmailNotificationsOnMention && !customWatchers.Contains(u.ID) ||
+			!access_model.CheckRepoUnitUser(ctx, rel.Repo, u, unit.TypeReleases)
 	})
 
 	langMap := make(map[string][]*user_model.User)

services/mailer/mail_release_test.go

@@ -61,3 +61,29 @@ func TestMailNewReleaseFiltersUnauthorizedWatchers(t *testing.T) {
 	assert.Equal(t, admin.EmailTo(), sent[0].To)
 	assert.NotEqual(t, unauthorized.EmailTo(), sent[0].To)
 }
+
+func TestMailNewReleaseOnMentionCustomWatch(t *testing.T) {
+	assert.NoError(t, unittest.PrepareTestDatabase())
+	defer test.MockVariableValue(&setting.MailService, &setting.Mailer{FromEmail: "noreply@example.com"})()
+	defer mockMailTemplates(string(tplNewReleaseMail), "{{.Subject}}", "<p>{{.Release.TagName}}</p>")()
+
+	// users 4 and 9 only get mail on mention and watch repo 1 with all activity
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+	customWatcher := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+	allWatcher := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 9})
+	require.NoError(t, repo_model.WatchRepoWithOptions(t.Context(), customWatcher, repo, repo_model.WatchOptions{Mode: repo_model.WatchModeCustom, WatchReleases: true}))
+
+	rel := unittest.AssertExistsAndLoadBean(t, &repo_model.Release{ID: 1, RepoID: repo.ID})
+	rel.Publisher = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: rel.PublisherID})
+
+	var recipients []string
+	defer test.MockVariableValue(&SendAsync, func(msgs ...*sender_service.Message) {
+		for _, msg := range msgs {
+			recipients = append(recipients, msg.To)
+		}
+	})()
+
+	MailNewRelease(t.Context(), rel)
+	assert.Contains(t, recipients, customWatcher.EmailTo())
+	assert.NotContains(t, recipients, allWatcher.EmailTo())
+}

services/mailer/mail_test.go

@@ -167,6 +167,28 @@ func TestMailMentionsComment(t *testing.T) {
 	assert.Equal(t, 3, mails)
 }
 
+func TestMailParticipantsOnMentionCustomWatch(t *testing.T) {
+	doer, repo, issue, comment := prepareMailerTest(t)
+	comment.Poster = doer
+	defer mockMailTemplates("mail/repo/issue/comment", subjectTpl, bodyTpl)()
+
+	// users 4 and 9 only get mail on mention and watch repo 1 with all activity
+	customWatcher := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+	allWatcher := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 9})
+	require.NoError(t, repo_model.WatchRepoWithOptions(t.Context(), customWatcher, repo, repo_model.WatchOptions{Mode: repo_model.WatchModeCustom, WatchPullRequests: true, WatchIssues: true, WatchReleases: true}))
+
+	var recipients []string
+	defer test.MockVariableValue(&SendAsync, func(msgs ...*sender_service.Message) {
+		for _, msg := range msgs {
+			recipients = append(recipients, msg.To)
+		}
+	})()
+
+	require.NoError(t, MailParticipantsComment(t.Context(), comment, activities_model.ActionCommentIssue, issue, nil))
+	assert.Contains(t, recipients, customWatcher.Email)
+	assert.NotContains(t, recipients, allWatcher.Email)
+}
+
 func TestMailsSkipBots(t *testing.T) {
 	doer, repo, issue, comment := prepareMailerTest(t)
 	comment.Poster = doer

templates/user/settings/notifications.tmpl

@@ -16,7 +16,7 @@
 								<div class="menu">
 									<div data-value="enabled" class="{{if eq .EmailNotificationsPreference "enabled"}}active selected {{end}}item">{{ctx.Locale.Tr "settings.email_notifications.enable"}}</div>
 									<div data-value="andyourown" class="{{if eq .EmailNotificationsPreference "andyourown"}}active selected {{end}}item">{{ctx.Locale.Tr "settings.email_notifications.andyourown"}}</div>
-									<div data-value="onmention" class="{{if eq .EmailNotificationsPreference "onmention"}}active selected {{end}}item">{{ctx.Locale.Tr "settings.email_notifications.onmention"}}</div>
+									<div data-value="onmention" class="{{if eq .EmailNotificationsPreference "onmention"}}active selected {{end}}item">{{ctx.Locale.Tr "settings.email_notifications.onmention_and_custom"}}</div>
 									<div data-value="disabled" class="{{if eq .EmailNotificationsPreference "disabled"}}active selected {{end}}item">{{ctx.Locale.Tr "settings.email_notifications.disable"}}</div>
 								</div>
 							</div>