From ad6107ab88f20de5b94eccf4aaeb4620f9c28cdc Mon Sep 17 00:00:00 2001 From: silverwind Date: Sun, 9 Aug 2026 12:54:31 +0200 Subject: [PATCH] enhance: refine repo watching (#38835) Follow-up to https://github.com/go-gitea/gitea/pull/37571. "Participating and mentions" deleted the watch row, so choosing it dropped you out of the watcher count. It is a watch like the others, so it now keeps a row and simply subscribes to no events. The dashboard feed ignored the per-event options, so a "Custom: issues" watcher still got pull request activity there. It now gates on the same options as mail and notifications. That also closes a gap where pull request reviews bypassed the permission check. Also, address https://github.com/go-gitea/gitea/pull/37571#discussion_r3740487363 and reword a UI text for clarity. --------- Signed-off-by: silverwind Co-authored-by: wxiaoguang --- models/activities/notification_test.go | 2 +- models/repo/watch.go | 40 +++++++++++++++++++-- models/repo/watch_test.go | 11 +++++- options/locale/locale_en-US.json | 3 +- routers/web/repo/watch.go | 27 ++++----------- routers/web/web.go | 2 +- services/context/repo.go | 3 +- services/feed/feed.go | 26 +++++++------- services/feed/feed_test.go | 16 +++++++++ templates/repo/header/watch.tmpl | 46 ++++++++++++++----------- templates/repo/watch_options_modal.tmpl | 19 ++++++---- templates/shared/repo/list.tmpl | 9 +++-- web_src/js/features/common-button.ts | 4 ++- web_src/js/features/repo-watch.ts | 21 ++--------- 14 files changed, 137 insertions(+), 92 deletions(-) diff --git a/models/activities/notification_test.go b/models/activities/notification_test.go index 72e6bc21c4..e1d2ecc9fc 100644 --- a/models/activities/notification_test.go +++ b/models/activities/notification_test.go @@ -54,7 +54,7 @@ func TestCreateOrUpdateIssueNotificationsIgnored(t *testing.T) { // user 4 watches repo 1 and would be notified about issue 1 repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) - assert.NoError(t, repo_model.IgnoreRepo(t.Context(), user, repo)) + assert.NoError(t, repo_model.WatchIgnoreRepo(t.Context(), user, repo)) notified, err := activities_model.CreateOrUpdateIssueNotifications(t.Context(), 1, 0, 2, 0) assert.NoError(t, err) diff --git a/models/repo/watch.go b/models/repo/watch.go index fdbcb74513..2519fe46ab 100644 --- a/models/repo/watch.go +++ b/models/repo/watch.go @@ -74,6 +74,29 @@ func (w *Watch) IsIgnoring() bool { return w.Mode == WatchModeDont } +// IsWatching reports whether the watch counts the user as a watcher of the repository +func (w *Watch) IsWatching() bool { + return IsWatchMode(w.Mode) +} + +// IsWatchingAll reports whether every event is enabled, which is the "all activity" mode +func (w *Watch) IsWatchingAll() bool { + return w.PullRequests && w.Issues && w.Releases +} + +// SelectedMode returns the mode the user picked in the watch menu +func (w *Watch) SelectedMode() string { + switch { + case w.IsIgnoring(): + return "ignore" + case !IsWatchMode(w.Mode), !(w.PullRequests || w.Issues || w.Releases): + return "participate" // also the default while there is no watch row + case w.IsWatchingAll(): + return "all" + } + return "custom" +} + // IsWatchMode Decodes watchability of WatchMode func IsWatchMode(mode WatchMode) bool { return mode != WatchModeNone && mode != WatchModeDont @@ -145,8 +168,8 @@ func WatchRepo(ctx context.Context, doer *user_model.User, repo *Repository, doW return watchRepoMode(ctx, watch, WatchModeNormal) } -// IgnoreRepo mutes the repository, so nothing about it reaches the user. -func IgnoreRepo(ctx context.Context, doer *user_model.User, repo *Repository) error { +// WatchIgnoreRepo mutes the repository (unwatch), so nothing about it reaches the user. +func WatchIgnoreRepo(ctx context.Context, doer *user_model.User, repo *Repository) error { watch, err := GetWatch(ctx, doer.ID, repo.ID) if err != nil { return err @@ -160,6 +183,16 @@ type WatchOptions struct { Releases bool } +// WatchRepoWithOptions starts watching the repository and subscribes to the given events +func WatchRepoWithOptions(ctx context.Context, doer *user_model.User, repo *Repository, opts WatchOptions) error { + return db.WithTx(ctx, func(ctx context.Context) error { + if err := WatchRepo(ctx, doer, repo, true); err != nil { + return err + } + return SetWatchOptions(ctx, doer.ID, repo.ID, opts) + }) +} + // SetWatchOptions updates the per-event options of a watch, callers must run WatchRepo first func SetWatchOptions(ctx context.Context, userID, repoID int64, opts WatchOptions) error { _, err := db.GetEngine(ctx).Where("user_id=? AND repo_id=?", userID, repoID). @@ -187,11 +220,12 @@ func GetUserWatches(ctx context.Context, userID int64, repoIDs []int64) (map[int return watchesByRepo, nil } -// GetWatchers returns all watchers of given repository. +// GetWatchers returns all watchers of given repository, skipping those subscribed to no event. func GetWatchers(ctx context.Context, repoID int64) ([]*Watch, error) { watches := make([]*Watch, 0, 10) return watches, db.GetEngine(ctx).Where("`watch`.repo_id=?", repoID). And("`watch`.mode<>?", WatchModeDont). + And(builder.Or(builder.Eq{"`watch`.pull_requests": true}, builder.Eq{"`watch`.issues": true}, builder.Eq{"`watch`.releases": true})). And("`user`.is_active=?", true). And("`user`.prohibit_login=?", false). Join("INNER", "`user`", "`user`.id = `watch`.user_id"). diff --git a/models/repo/watch_test.go b/models/repo/watch_test.go index ef4ee3403e..a2d394bd84 100644 --- a/models/repo/watch_test.go +++ b/models/repo/watch_test.go @@ -167,5 +167,14 @@ func TestWatchOptions(t *testing.T) { assert.NoError(t, repo_model.WatchRepo(t.Context(), user, repo, true)) watch, err := repo_model.GetWatch(t.Context(), user.ID, repo.ID) assert.NoError(t, err) - assert.True(t, watch.PullRequests && watch.Issues && watch.Releases) + assert.True(t, watch.IsWatchingAll()) +} + +func TestWatchSelectedMode(t *testing.T) { + // a user without a watch row gets the dummy record, whose flags are the column defaults + assert.Equal(t, "participate", (&repo_model.Watch{Mode: repo_model.WatchModeNone, PullRequests: true, Issues: true, Releases: true}).SelectedMode()) + 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, Issues: true}).SelectedMode()) + assert.Equal(t, "all", (&repo_model.Watch{Mode: repo_model.WatchModeAuto, PullRequests: true, Issues: true, Releases: true}).SelectedMode()) } diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index d00ad1ca26..c6725ca18a 100644 --- a/options/locale/locale_en-US.json +++ b/options/locale/locale_en-US.json @@ -1157,7 +1157,6 @@ "repo.watch": "Watch", "repo.watching": "Watching", "repo.ignoring": "Ignoring", - "repo.watch.options.required": "Select at least one event type.", "repo.watch.options.issues": "Issues", "repo.watch.options.pull_requests": "Pull Requests", "repo.watch.options.releases": "Releases", @@ -1168,7 +1167,7 @@ "repo.watch.mode.ignore": "Ignore", "repo.watch.mode.ignore.desc": "Never receive notifications from this repository.", "repo.watch.mode.custom": "Custom", - "repo.watch.mode.custom.desc": "Choose the events you want to receive notifications for.", + "repo.watch.mode.custom.desc": "Select events you want to be notified of, in addition to participating and mentions.", "repo.unstar": "Unstar", "repo.star": "Star", "repo.fork": "Fork", diff --git a/routers/web/repo/watch.go b/routers/web/repo/watch.go index 5e5cce5a12..b2cdf80ecc 100644 --- a/routers/web/repo/watch.go +++ b/routers/web/repo/watch.go @@ -17,29 +17,22 @@ func ActionWatch(ctx *context.Context) { action := ctx.PathParam("action") var err error if action == "ignore" { - err = repo_model.IgnoreRepo(ctx, ctx.Doer, ctx.Repo.Repository) + err = repo_model.WatchIgnoreRepo(ctx, ctx.Doer, ctx.Repo.Repository) } else { - err = repo_model.WatchRepo(ctx, ctx.Doer, ctx.Repo.Repository, action == "watch") + all := action == "watch" // "participate" is a watch that subscribes to no event on its own + err = repo_model.WatchRepoWithOptions(ctx, ctx.Doer, ctx.Repo.Repository, repo_model.WatchOptions{PullRequests: all, Issues: all, Releases: all}) } if err != nil { handleActionError(ctx, err) return } - if action == "watch" { // watching again always restores every event, so "all activity" can undo a custom selection - opts := repo_model.WatchOptions{PullRequests: true, Issues: true, Releases: true} - if err := repo_model.SetWatchOptions(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, opts); err != nil { - ctx.ServerError("SetWatchOptions", err) - return - } - } watch, err := repo_model.GetWatch(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID) if err != nil { ctx.ServerError("GetWatch", err) return } - ctx.Data["Watch"] = watch - ctx.Data["IsWatchingRepo"] = repo_model.IsWatchMode(watch.Mode) + ctx.Data["RepoWatch"] = watch ctx.Data["Repository"], err = repo_model.GetRepositoryByName(ctx, ctx.Repo.Repository.OwnerID, ctx.Repo.Repository.Name) if err != nil { @@ -51,22 +44,14 @@ func ActionWatch(ctx *context.Context) { // ActionWatchOptions watches the repository with a custom selection of events func ActionWatchOptions(ctx *context.Context) { - opts := repo_model.WatchOptions{ + opts := repo_model.WatchOptions{ // clearing every event is allowed, it leaves the participating state PullRequests: ctx.FormBool(string(repo_model.WatchPullRequests)), Issues: ctx.FormBool(string(repo_model.WatchIssues)), Releases: ctx.FormBool(string(repo_model.WatchReleases)), } - if !opts.PullRequests && !opts.Issues && !opts.Releases { - ctx.JSONError(ctx.Tr("repo.watch.options.required")) - return - } - if err := repo_model.WatchRepo(ctx, ctx.Doer, ctx.Repo.Repository, true); err != nil { + if err := repo_model.WatchRepoWithOptions(ctx, ctx.Doer, ctx.Repo.Repository, opts); err != nil { handleActionError(ctx, err) return } - if err := repo_model.SetWatchOptions(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, opts); err != nil { - ctx.ServerError("SetWatchOptions", err) - return - } ctx.JSONRedirect("") } diff --git a/routers/web/web.go b/routers/web/web.go index 34acdd4edb..4e30bd7adc 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1739,7 +1739,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) { m.Get("/watchers", repo.Watchers) m.Get("/search", reqUnitCodeReader, repo.Search) m.Post("/action/{action:star|unstar}", reqSignIn, starsEnabled, repo.ActionStar) - m.Post("/action/{action:watch|unwatch|ignore}", reqSignIn, repo.ActionWatch) + m.Post("/action/{action:watch|participate|ignore}", reqSignIn, repo.ActionWatch) m.Post("/action/watch/options", reqSignIn, repo.ActionWatchOptions) m.Post("/action/{action:accept_transfer|reject_transfer}", reqSignIn, repo.ActionTransfer) }, optSignIn, context.RepoAssignment) diff --git a/services/context/repo.go b/services/context/repo.go index 4345e2f7b0..ecb8e8d74a 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -644,8 +644,7 @@ func repoAssignmentPrepareTemplateData(ctx *Context, data *repoAssignmentPrepare ctx.ServerError("GetWatch", err) return } - ctx.Data["Watch"] = watch - ctx.Data["IsWatchingRepo"] = repo_model.IsWatchMode(watch.Mode) + ctx.Data["RepoWatch"] = watch ctx.Data["IsStaringRepo"] = repo_model.IsStaring(ctx, ctx.Doer.ID, repo.ID) } diff --git a/services/feed/feed.go b/services/feed/feed.go index 92edd38097..d8b7daeb60 100644 --- a/services/feed/feed.go +++ b/services/feed/feed.go @@ -69,20 +69,23 @@ func notifyWatchers(ctx context.Context, act *activities_model.Action, watchers act.UserID = watcher.UserID act.Repo.Units = nil + var allowed bool switch act.OpType { - case activities_model.ActionCommitRepo, activities_model.ActionPushTag, activities_model.ActionDeleteTag, activities_model.ActionPublishRelease, activities_model.ActionDeleteBranch: - if !permCode[i] { - continue - } + case activities_model.ActionCommitRepo, activities_model.ActionPushTag, activities_model.ActionDeleteTag, activities_model.ActionDeleteBranch: + allowed = permCode[i] && watcher.IsWatchingAll() + case activities_model.ActionPublishRelease: + allowed = permCode[i] && watcher.Releases case activities_model.ActionCreateIssue, activities_model.ActionCommentIssue, activities_model.ActionCloseIssue, activities_model.ActionReopenIssue: - if !permIssue[i] { - continue - } - case activities_model.ActionCreatePullRequest, activities_model.ActionCommentPull, activities_model.ActionMergePullRequest, activities_model.ActionClosePullRequest, activities_model.ActionReopenPullRequest, activities_model.ActionAutoMergePullRequest: - if !permPR[i] { - continue - } + allowed = permIssue[i] && watcher.Issues + case activities_model.ActionCreatePullRequest, activities_model.ActionCommentPull, activities_model.ActionMergePullRequest, activities_model.ActionClosePullRequest, + activities_model.ActionReopenPullRequest, activities_model.ActionAutoMergePullRequest, activities_model.ActionApprovePullRequest, + activities_model.ActionRejectPullRequest, activities_model.ActionPullReviewDismissed, activities_model.ActionPullRequestReadyForReview: + allowed = permPR[i] && watcher.PullRequests default: + allowed = watcher.IsWatchingAll() // repository events have no watch option of their own + } + if !allowed { + continue } if err := db.Insert(ctx, act); err != nil { @@ -120,7 +123,6 @@ func NotifyWatchers(ctx context.Context, acts ...*activities_model.Action) error if err != nil { return fmt.Errorf("get watchers: %w", err) } - permCode := make([]bool, len(watchers)) permIssue := make([]bool, len(watchers)) permPR := make([]bool, len(watchers)) diff --git a/services/feed/feed_test.go b/services/feed/feed_test.go index 43cc0ca750..8dc539c14b 100644 --- a/services/feed/feed_test.go +++ b/services/feed/feed_test.go @@ -200,3 +200,19 @@ func TestNotifyWatchers(t *testing.T) { OpType: action.OpType, }) } + +func TestNotifyWatchersRespectsWatchOptions(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + // user 1 watches repo 1 for issues only, user 4 keeps every event + assert.NoError(t, repo_model.SetWatchOptions(t.Context(), 1, 1, repo_model.WatchOptions{Issues: true})) + + assert.NoError(t, NotifyWatchers(t.Context(), + &activities_model.Action{ActUserID: 8, RepoID: 1, OpType: activities_model.ActionCreateIssue}, + &activities_model.Action{ActUserID: 8, RepoID: 1, OpType: activities_model.ActionApprovePullRequest}, + )) + + unittest.AssertExistsAndLoadBean(t, &activities_model.Action{UserID: 1, RepoID: 1, OpType: activities_model.ActionCreateIssue}) + unittest.AssertNotExistsBean(t, &activities_model.Action{UserID: 1, RepoID: 1, OpType: activities_model.ActionApprovePullRequest}) + unittest.AssertExistsAndLoadBean(t, &activities_model.Action{UserID: 4, RepoID: 1, OpType: activities_model.ActionApprovePullRequest}) +} diff --git a/templates/repo/header/watch.tmpl b/templates/repo/header/watch.tmpl index c3b9e52973..6ce3138507 100644 --- a/templates/repo/header/watch.tmpl +++ b/templates/repo/header/watch.tmpl @@ -1,11 +1,12 @@ -{{$isIgnoring := and $.IsSigned $.Watch.IsIgnoring}} -{{$buttonText := ctx.Locale.Tr (Iif $isIgnoring "repo.ignoring" (Iif $.IsWatchingRepo "repo.watching" "repo.watch"))}} +{{$isIgnoring := and $.IsSigned $.RepoWatch.IsIgnoring}} +{{$buttonText := ctx.Locale.Tr (Iif $isIgnoring "repo.ignoring" (Iif $.RepoWatch.IsWatching "repo.watching" "repo.watch"))}} {{$count := CountFmt .Repository.NumWatches}}
{{if $.IsSigned}} - {{$isAll := and $.IsWatchingRepo $.Watch.PullRequests $.Watch.Issues $.Watch.Releases}} - {{$isCustom := and $.IsWatchingRepo (not $isAll)}} - {{$isParticipating := not (or $.IsWatchingRepo $isIgnoring)}} + {{$mode := $.RepoWatch.SelectedMode}} + {{$isAll := eq $mode "all"}} + {{$isCustom := eq $mode "custom"}} + {{$isParticipating := eq $mode "participate"}}
- {{$participating := ctx.Locale.Tr "repo.watch.mode.participating"}} - {{svg "octicon-check" 16 (Iif $isParticipating "" "tw-invisible")}} -
{{$participating}}
{{ctx.Locale.Tr "repo.watch.mode.participating.desc"}}
+
{{$testParticipating}}
{{ctx.Locale.Tr "repo.watch.mode.participating.desc"}}
 - {{$all := ctx.Locale.Tr "repo.watch.mode.all"}} - {{svg "octicon-check" 16 (Iif $isAll "" "tw-invisible")}} -
{{$all}}
{{ctx.Locale.Tr "repo.watch.mode.all.desc"}}
+
{{$textAll}}
{{ctx.Locale.Tr "repo.watch.mode.all.desc"}}
 - {{$ignore := ctx.Locale.Tr "repo.watch.mode.ignore"}} - - {{svg "octicon-check" 16 (Iif $.Watch.IsIgnoring "" "tw-invisible")}} -
{{$ignore}}
{{ctx.Locale.Tr "repo.watch.mode.ignore.desc"}}
+ {{svg "octicon-check" 16 (Iif $isIgnoring "" "tw-invisible")}} +
{{$textIgnore}}
{{ctx.Locale.Tr "repo.watch.mode.ignore.desc"}}
 - {{$custom := ctx.Locale.Tr "repo.watch.mode.custom"}} - {{svg "octicon-check" 16 (Iif $isCustom "" "tw-invisible")}} -
{{$custom}}
{{ctx.Locale.Tr "repo.watch.mode.custom.desc"}}
+
{{$textCustom}}
{{ctx.Locale.Tr "repo.watch.mode.custom.desc"}}
{{else}} diff --git a/templates/repo/watch_options_modal.tmpl b/templates/repo/watch_options_modal.tmpl index a6e09f8e61..92deb8897a 100644 --- a/templates/repo/watch_options_modal.tmpl +++ b/templates/repo/watch_options_modal.tmpl @@ -2,13 +2,20 @@
{{ctx.Locale.Tr "notifications"}}
- {{range $name := StringUtils.Split "issues,pull_requests,releases" ","}} -
-
- -
+
+
+ +
- {{end}} +
+ + +
+
+ + +
+
{{template "base/modal_actions_confirm" dict "ModalButtonTypes" "confirm"}} diff --git a/templates/shared/repo/list.tmpl b/templates/shared/repo/list.tmpl index d574da9be7..f1f0a617dc 100644 --- a/templates/shared/repo/list.tmpl +++ b/templates/shared/repo/list.tmpl @@ -57,9 +57,12 @@ {{$watch := and $.Watches (index $.Watches .ID)}} {{if $watch}} - {{end}} diff --git a/web_src/js/features/common-button.ts b/web_src/js/features/common-button.ts index fd7dc47383..43dc3862d6 100644 --- a/web_src/js/features/common-button.ts +++ b/web_src/js/features/common-button.ts @@ -111,8 +111,10 @@ function onShowModalClick(el: HTMLElement, e: MouseEvent) { if (attrTargetProp) { assignElementProperty(attrTarget, attrTargetProp, attrib.value); + } else if (attrTarget.matches('input[type=checkbox], input[type=radio]')) { + (attrTarget as HTMLInputElement).checked = attrib.value === 'true'; } else if (attrTarget.matches('input, textarea')) { - (attrTarget as HTMLInputElement | HTMLTextAreaElement).value = attrib.value; // FIXME: add more supports like checkbox + (attrTarget as HTMLInputElement | HTMLTextAreaElement).value = attrib.value; } else { attrTarget.textContent = attrib.value; // FIXME: it should be more strict here, only handle div/span/p } diff --git a/web_src/js/features/repo-watch.ts b/web_src/js/features/repo-watch.ts index dba04f49b6..2bb3734635 100644 --- a/web_src/js/features/repo-watch.ts +++ b/web_src/js/features/repo-watch.ts @@ -1,15 +1,10 @@ import {createTippy} from '../modules/tippy.ts'; -import {showFomanticModal} from '../modules/fomantic/modal.ts'; -import {registerGlobalEventFunc, registerGlobalInitFunc} from '../modules/observer.ts'; -import type {Instance} from 'tippy.js'; - -let watchMenuTippy: Instance | null = null; +import {registerGlobalInitFunc} from '../modules/observer.ts'; export function initRepoWatch() { registerGlobalInitFunc('initRepoWatchMenu', (btn: HTMLElement) => { - watchMenuTippy?.destroy(); // a watch action replaces the button, orphaning the old menu const menu = btn.nextElementSibling!; - watchMenuTippy = createTippy(btn, { + const watchMenuTippy = createTippy(btn, { content: menu, theme: 'menu', maxWidth: 350, @@ -18,16 +13,6 @@ export function initRepoWatch() { interactive: true, hideOnClick: true, }); - menu.addEventListener('click', () => watchMenuTippy!.hide()); - }); - - registerGlobalEventFunc('click', 'onRepoWatchOptionsClick', (btn: HTMLElement) => { - const elModal = document.querySelector('#repo-watch-options-modal')!; - const form = elModal.querySelector('form')!; - form.action = btn.getAttribute('data-url')!; - for (const el of form.querySelectorAll('input[type=checkbox]')) { - el.checked = btn.getAttribute(`data-${el.name.replaceAll('_', '-')}`) === 'true'; - } - showFomanticModal(elModal); + menu.addEventListener('click', () => watchMenuTippy.hide()); }); }