diff --git a/models/activities/action.go b/models/activities/action.go index ad924b7667..9a033718e7 100644 --- a/models/activities/action.go +++ b/models/activities/action.go @@ -131,6 +131,68 @@ func (at ActionType) String() string { } } +func (at ActionType) WatchSelection() repo_model.WatchSelection { + // WatchAllSelection generally means that there is no granular enough setting for this. + switch at { + case ActionCreateRepo: + return repo_model.WatchAllSelection + case ActionRenameRepo: + return repo_model.WatchAllSelection + case ActionStarRepo: + return repo_model.WatchAllSelection + case ActionWatchRepo: + return repo_model.WatchAllSelection + case ActionCommitRepo: + return repo_model.WatchAllSelection + case ActionCreateIssue: + return repo_model.WatchSelection{Issues: true, PullRequests: false, Releases: false} + case ActionCreatePullRequest: + return repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false} + case ActionTransferRepo: + return repo_model.WatchAllSelection + case ActionPushTag: + return repo_model.WatchAllSelection + case ActionCommentIssue: + return repo_model.WatchSelection{Issues: true, PullRequests: false, Releases: false} + case ActionMergePullRequest: + return repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false} + case ActionCloseIssue: + return repo_model.WatchSelection{Issues: true, PullRequests: false, Releases: false} + case ActionReopenIssue: + return repo_model.WatchSelection{Issues: true, PullRequests: false, Releases: false} + case ActionClosePullRequest: + return repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false} + case ActionReopenPullRequest: + return repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false} + case ActionDeleteTag: + return repo_model.WatchAllSelection + case ActionDeleteBranch: + return repo_model.WatchAllSelection + case ActionMirrorSyncPush: + return repo_model.WatchAllSelection + case ActionMirrorSyncCreate: + return repo_model.WatchAllSelection + case ActionMirrorSyncDelete: + return repo_model.WatchAllSelection + case ActionApprovePullRequest: + return repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false} + case ActionRejectPullRequest: + return repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false} + case ActionCommentPull: + return repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false} + case ActionPublishRelease: + return repo_model.WatchSelection{Issues: false, PullRequests: false, Releases: true} + case ActionPullReviewDismissed: + return repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false} + case ActionPullRequestReadyForReview: + return repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false} + case ActionAutoMergePullRequest: + return repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false} + default: + return repo_model.WatchAllSelection + } +} + func (at ActionType) InActions(actions ...string) bool { return slices.Contains(actions, at.String()) } @@ -636,7 +698,7 @@ func NotifyWatchers(ctx context.Context, actions ...*Action) ([]Action, error) { if repoChanged { // Add feeds for user self and all watchers. - watchers, err = repo_model.GetWatchers(ctx, act.RepoID) + watchers, err = repo_model.GetSelectWatchers(ctx, act.RepoID, act.GetOpType().WatchSelection()) if err != nil { return nil, fmt.Errorf("get watchers: %w", err) } diff --git a/models/activities/action_test.go b/models/activities/action_test.go index 592fdf71e3..bde4665a2f 100644 --- a/models/activities/action_test.go +++ b/models/activities/action_test.go @@ -224,6 +224,32 @@ func TestNotifyWatchers(t *testing.T) { }) } +func TestNotifySelectWatchers(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + action := &activities_model.Action{ + ActUserID: 8, + RepoID: 5, + OpType: activities_model.ActionPullReviewDismissed, + } + _, err := activities_model.NotifyWatchers(db.DefaultContext, action) + require.NoError(t, err) + + unittest.AssertExistsAndLoadBean(t, &activities_model.Action{ + ActUserID: action.ActUserID, + UserID: 5, + RepoID: action.RepoID, + OpType: action.OpType, + }) + // Notice that user 9 isn't here because they don't watch PRs on this repo. + unittest.AssertNotExistsBean(t, &activities_model.Action{ + ActUserID: action.ActUserID, + UserID: 4, + RepoID: action.RepoID, + OpType: action.OpType, + }) +} + func TestGetFeedsCorrupted(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) diff --git a/models/activities/notification_list.go b/models/activities/notification_list.go index 3f3a48eaa5..2bce5082e8 100644 --- a/models/activities/notification_list.go +++ b/models/activities/notification_list.go @@ -108,7 +108,7 @@ func createOrUpdateIssueNotifications(ctx context.Context, issueID, commentID, n } toNotify.AddMultiple(issueWatches...) if !issue.IsPull || !issues_model.HasWorkInProgressPrefix(issue.Title) { - repoWatches, err := repo_model.GetRepoWatchersIDs(ctx, issue.RepoID) + repoWatches, err := repo_model.GetSelectWatcherIDs(ctx, issue.RepoID, repo_model.WatchSelection{Issues: !issue.IsPull, PullRequests: issue.IsPull, Releases: false}) if err != nil { return err } diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index cb88bd3acd..99aace43ed 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -63,7 +63,7 @@ lower_name: repo3 name: repo3 default_branch: master - num_watches: 0 + num_watches: 1 num_stars: 0 num_forks: 0 num_milestones: 0 @@ -93,7 +93,7 @@ lower_name: repo4 name: repo4 default_branch: master - num_watches: 0 + num_watches: 1 num_stars: 1 num_forks: 0 num_milestones: 0 @@ -121,7 +121,7 @@ lower_name: repo5 name: repo5 default_branch: master - num_watches: 0 + num_watches: 2 num_stars: 0 num_forks: 0 num_milestones: 0 diff --git a/models/fixtures/watch.yml b/models/fixtures/watch.yml index c6c9726cc8..e12712ed2e 100644 --- a/models/fixtures/watch.yml +++ b/models/fixtures/watch.yml @@ -2,34 +2,88 @@ id: 1 user_id: 1 repo_id: 1 - mode: 1 # normal + source: false # explicit + watch_selection_issues: true + watch_selection_pull_requests: true + watch_selection_releases: true - id: 2 user_id: 4 repo_id: 1 - mode: 1 # normal + source: false # explicit + watch_selection_issues: true + watch_selection_pull_requests: true + watch_selection_releases: true - id: 3 user_id: 9 repo_id: 1 - mode: 1 # normal + source: false # explicit + watch_selection_issues: true + watch_selection_pull_requests: true + watch_selection_releases: true - id: 4 user_id: 8 repo_id: 1 - mode: 2 # don't watch + source: false # explicit + watch_selection_issues: false + watch_selection_pull_requests: false + watch_selection_releases: false - id: 5 user_id: 11 repo_id: 1 - mode: 3 # auto + source: true # automatic + watch_selection_issues: true + watch_selection_pull_requests: true + watch_selection_releases: true - id: 6 user_id: 4 repo_id: 2 - mode: 1 # normal + source: false # explicit + watch_selection_issues: true + watch_selection_pull_requests: true + watch_selection_releases: true + +- + id: 7 + user_id: 4 + repo_id: 3 + source: false # explicit + watch_selection_issues: true + watch_selection_pull_requests: false + watch_selection_releases: false + +- + id: 8 + user_id: 4 + repo_id: 4 + source: false # explicit + watch_selection_issues: false + watch_selection_pull_requests: true + watch_selection_releases: false + +- + id: 9 + user_id: 4 + repo_id: 5 + source: false # explicit + watch_selection_issues: false + watch_selection_pull_requests: false + watch_selection_releases: true + +- + id: 10 + user_id: 5 + repo_id: 5 + source: false # explicit + watch_selection_issues: false + watch_selection_pull_requests: true + watch_selection_releases: true diff --git a/models/forgejo_migrations/README.md b/models/forgejo_migrations/README.md index 693c093cb4..c5ee2f507d 100644 --- a/models/forgejo_migrations/README.md +++ b/models/forgejo_migrations/README.md @@ -75,7 +75,7 @@ func myMigrationFunction(x *xorm.Engine) error { // add migration logic here // // to prevent `make watch` from recording this migration as done when it - // isn't authored yet, returh an error until the implementation is done + // isn't authored yet, return an error until the implementation is done return errors.New("not implemented yet") } ``` diff --git a/models/forgejo_migrations/v16e_add-granular-watch.go b/models/forgejo_migrations/v16e_add-granular-watch.go new file mode 100644 index 0000000000..48f1d045ba --- /dev/null +++ b/models/forgejo_migrations/v16e_add-granular-watch.go @@ -0,0 +1,113 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package forgejo_migrations + +import ( + "code.forgejo.org/xorm/xorm" +) + +type WatchSource bool + +const ( + // WatchSourceExplicit means the user explicitly chose to watch certain things (or none or all) of this repo. + // It means that setting.Service.AutoWatchOnChanges doesn't have an effect on this user for this repo; they explicitly made their choice after all. + // This mode replaces the old WatchModeDont and WatchModeNormal states. + WatchSourceExplicit WatchSource = false + // WatchSourceAutomatic means the user didn't explicitly select whether to watch this repo or not. + // Instead, the user either doesn't watch the repo because they didn't ever click the watch/unwatch button. + // Or they do watch the repo but only because the user pushed to the repo and setting.Service.AutoWatchOnChanges is true. + // When there is no record in the db this is the same as WatchSourceAutomatic combined with all watch selections turned off (i.e., not watching anything). + // This used to be WatchModeNone. + // When in this mode the watch selection is never fully deselected. + // Otherwise there'd be some automatic method to unwatch a repo; which does not exist. + // This mode replaces the old WatchModeAuto and WatchModeNone states. + WatchSourceAutomatic WatchSource = true + + // There may not be more modes than the above two. + // I intend this to be a single bit. +) + +func init() { + registerMigration(&Migration{ + Description: "Add granular watch settings to repos.", + Upgrade: addGranularWatchColumnsAndDropModeColumn, + }) +} + +func addGranularWatchColumnsAndDropModeColumn(x *xorm.Engine) error { + type Watch struct { + Source WatchSource `xorm:"BOOL DEFAULT TRUE"` + WatchSelectionIssues bool `xorm:"BOOL DEFAULT TRUE"` + WatchSelectionPullRequests bool `xorm:"BOOL DEFAULT TRUE"` + WatchSelectionReleases bool `xorm:"BOOL DEFAULT TRUE"` + } + + _, err := x.SyncWithOptions(xorm.SyncOptions{IgnoreDropIndices: true}, new(Watch)) + if err != nil { + return err + } + + // copy of old code // + type WatchMode uint8 + const ( + // WatchModeNone don't watch + // This means there is no Watch record in the db. + // We never store this mode in the db and instead remove the record from the db. + // Furthermore, this means there is a WatchMode for all combinations of user and repo. + // We never go back to this state once we've been in a different state. + WatchModeNone WatchMode = iota // 0 + // WatchModeNormal watch repository (from other sources) + // This means the user explicitly chose to watch the repo. + WatchModeNormal // 1 + // WatchModeDont explicit don't auto-watch + // This means the user explicitly removed themselves as a watcher. + // Then the AutoWatchOnChanges feature doesn't make the user a watcher when they push to the repo. + WatchModeDont // 2 + // WatchModeAuto watch repository (from AutoWatchOnChanges) + // This is used when the user pushed to the repo and setting.Service.AutoWatchOnChanges is true. + // That way we can differentiate people explicitly watching the repo and people only watching it because of the AutoWatchOnChanges feature. + WatchModeAuto // 3 + ) + // end copy of old code // + + _, err = x.Exec("UPDATE `watch` SET source = ? WHERE mode IN (?, ?)", WatchSourceAutomatic, WatchModeNone, WatchModeAuto) + if err != nil { + return err + } + _, err = x.Exec("UPDATE `watch` SET source = ? WHERE mode IN (?, ?)", WatchSourceExplicit, WatchModeNormal, WatchModeDont) + if err != nil { + return err + } + + _, err = x.Exec("UPDATE `watch` SET watch_selection_issues = ? WHERE mode = ? OR mode = ?", false, WatchModeNone, WatchModeDont) + if err != nil { + return err + } + _, err = x.Exec("UPDATE `watch` SET watch_selection_pull_requests = ? WHERE mode = ? OR mode = ?", false, WatchModeNone, WatchModeDont) + if err != nil { + return err + } + _, err = x.Exec("UPDATE `watch` SET watch_selection_releases = ? WHERE mode = ? OR mode = ?", false, WatchModeNone, WatchModeDont) + if err != nil { + return err + } + _, err = x.Exec("UPDATE `watch` SET watch_selection_issues = ? WHERE mode = ? OR mode = ?", true, WatchModeNormal, WatchModeAuto) + if err != nil { + return err + } + _, err = x.Exec("UPDATE `watch` SET watch_selection_pull_requests = ? WHERE mode = ? OR mode = ?", true, WatchModeNormal, WatchModeAuto) + if err != nil { + return err + } + _, err = x.Exec("UPDATE `watch` SET watch_selection_releases = ? WHERE mode = ? OR mode = ?", true, WatchModeNormal, WatchModeAuto) + if err != nil { + return err + } + + _, err = x.Exec("ALTER TABLE watch DROP COLUMN `mode`") + if err != nil { + return err + } + return nil +} diff --git a/models/forgejo_migrations/v16e_add-granular-watch_test.go b/models/forgejo_migrations/v16e_add-granular-watch_test.go new file mode 100644 index 0000000000..abd79333ed --- /dev/null +++ b/models/forgejo_migrations/v16e_add-granular-watch_test.go @@ -0,0 +1,176 @@ +// Copyright 2025 The Forgejo Authors. +// SPDX-License-Identifier: GPL-3.0-or-later + +package forgejo_migrations + +import ( + "context" + "testing" + + "forgejo.org/models/db" + migration_tests "forgejo.org/models/gitea_migrations/test" + "forgejo.org/modules/timeutil" + + "code.forgejo.org/xorm/xorm/schemas" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type WatchSelection struct { + Issues bool + PullRequests bool + Releases bool +} + +// Watch is connection request for receiving repository notification. +type Watch struct { + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"UNIQUE(watch)"` + RepoID int64 `xorm:"UNIQUE(watch)"` + Source WatchSource `xorm:"BOOL DEFAULT TRUE"` + // In the next PR there will be another mode here, choosing the user preset or a custom selection. + + WatchSelectionIssues bool `xorm:"BOOL DEFAULT TRUE"` + WatchSelectionPullRequests bool `xorm:"BOOL DEFAULT TRUE"` + WatchSelectionReleases bool `xorm:"BOOL DEFAULT TRUE"` + + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` +} + +// Warning: this does not set the WatchMode. +// The caller needs to do that properly. +func (w *Watch) setWatchSelection(watchSelection WatchSelection) { + w.WatchSelectionIssues = watchSelection.Issues + w.WatchSelectionPullRequests = watchSelection.PullRequests + w.WatchSelectionReleases = watchSelection.Releases +} + +var ( + WatchAllSelection = WatchSelection{Issues: true, PullRequests: true, Releases: true} + WatchNoneSelection = WatchSelection{Issues: false, PullRequests: false, Releases: false} +) + +// GetWatch gets what kind of subscription a user has on a given repository; returns dummy record if none found +func GetWatch(ctx context.Context, userID, repoID int64) (Watch, error) { + watch := Watch{UserID: userID, RepoID: repoID} + has, err := db.GetEngine(ctx).Get(&watch) + if err != nil { + return watch, err + } + if !has { + watch.Source = WatchSourceAutomatic + watch.setWatchSelection(WatchNoneSelection) + } + return watch, nil +} + +func Test_addGranularWatchColumnsAndDropModeColumn(t *testing.T) { + // copy of old code // + type WatchMode uint8 + type Watch struct { + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"UNIQUE(watch)"` + RepoID int64 `xorm:"UNIQUE(watch)"` + Mode WatchMode `xorm:"SMALLINT NOT NULL DEFAULT 1"` + + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + } + // end copy of old code // + + // Prepare TestEnv + x, deferable := migration_tests.PrepareTestEnv(t, 0, + new(Watch), + ) + defer deferable() + if x == nil || t.Failed() { + return + } + + // test for expected results + getColumn := func(tn, co string) *schemas.Column { + tables, err := x.DBMetas() + require.NoError(t, err) + var table *schemas.Table + for _, elem := range tables { + if elem.Name == tn { + table = elem + break + } + } + return table.GetColumn(co) + } + + require.NotNil(t, getColumn("watch", "mode")) + cnt1, err := x.Table("watch").Count() + require.NoError(t, err) + require.Equal(t, int64(7), cnt1) + + require.NoError(t, addGranularWatchColumnsAndDropModeColumn(x)) + + require.Nil(t, getColumn("watch", "mode")) + require.NotNil(t, getColumn("watch", "watch_selection_issues")) + require.NotNil(t, getColumn("watch", "watch_selection_pull_requests")) + require.NotNil(t, getColumn("watch", "watch_selection_releases")) + cnt2, err := x.Table("watch").Count() + require.NoError(t, err) + require.Equal(t, int64(7), cnt2) + + { + watch, err := GetWatch(db.DefaultContext, 1, 1) + require.NoError(t, err) + assert.Equal(t, WatchSourceExplicit, watch.Source) + assert.True(t, watch.WatchSelectionIssues) + assert.True(t, watch.WatchSelectionPullRequests) + assert.True(t, watch.WatchSelectionReleases) + } + { + watch, err := GetWatch(db.DefaultContext, 4, 1) + require.NoError(t, err) + assert.Equal(t, WatchSourceExplicit, watch.Source) + assert.True(t, watch.WatchSelectionIssues) + assert.True(t, watch.WatchSelectionPullRequests) + assert.True(t, watch.WatchSelectionReleases) + } + { + watch, err := GetWatch(db.DefaultContext, 9, 1) + require.NoError(t, err) + assert.Equal(t, WatchSourceExplicit, watch.Source) + assert.True(t, watch.WatchSelectionIssues) + assert.True(t, watch.WatchSelectionPullRequests) + assert.True(t, watch.WatchSelectionReleases) + } + { + watch, err := GetWatch(db.DefaultContext, 8, 1) + require.NoError(t, err) + assert.Equal(t, WatchSourceExplicit, watch.Source) + assert.False(t, watch.WatchSelectionIssues) + assert.False(t, watch.WatchSelectionPullRequests) + assert.False(t, watch.WatchSelectionReleases) + } + { + watch, err := GetWatch(db.DefaultContext, 11, 1) + require.NoError(t, err) + assert.Equal(t, WatchSourceAutomatic, watch.Source) + assert.True(t, watch.WatchSelectionIssues) + assert.True(t, watch.WatchSelectionPullRequests) + assert.True(t, watch.WatchSelectionReleases) + } + { + watch, err := GetWatch(db.DefaultContext, 4, 2) + require.NoError(t, err) + assert.Equal(t, WatchSourceExplicit, watch.Source) + assert.True(t, watch.WatchSelectionIssues) + assert.True(t, watch.WatchSelectionPullRequests) + assert.True(t, watch.WatchSelectionReleases) + } + { + watch, err := GetWatch(db.DefaultContext, 5, 2) + require.NoError(t, err) + assert.Equal(t, WatchSourceAutomatic, watch.Source) + assert.False(t, watch.WatchSelectionIssues) + assert.False(t, watch.WatchSelectionPullRequests) + assert.False(t, watch.WatchSelectionReleases) + } +} diff --git a/models/gitea_migrations/fixtures/Test_addGranularWatchColumnsAndDropModeColumn/watch.yml b/models/gitea_migrations/fixtures/Test_addGranularWatchColumnsAndDropModeColumn/watch.yml new file mode 100644 index 0000000000..105879b1d3 --- /dev/null +++ b/models/gitea_migrations/fixtures/Test_addGranularWatchColumnsAndDropModeColumn/watch.yml @@ -0,0 +1,41 @@ +- + id: 1 + user_id: 1 + repo_id: 1 + mode: 1 # normal + +- + id: 2 + user_id: 4 + repo_id: 1 + mode: 1 # normal + +- + id: 3 + user_id: 9 + repo_id: 1 + mode: 1 # normal + +- + id: 4 + user_id: 8 + repo_id: 1 + mode: 2 # don't watch + +- + id: 5 + user_id: 11 + repo_id: 1 + mode: 3 # auto + +- + id: 6 + user_id: 4 + repo_id: 2 + mode: 1 # normal + +- + id: 7 + user_id: 5 + repo_id: 2 + mode: 0 # none; this doesn't actually ever existed in the db but let's check it anyways. diff --git a/models/issues/issue_search.go b/models/issues/issue_search.go index f96eea2e17..14d43a74fe 100644 --- a/models/issues/issue_search.go +++ b/models/issues/issue_search.go @@ -463,7 +463,7 @@ func applySubscribedCondition(sess *xorm.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.Eq{"`watch`.watch_selection_issues": true})), ), ), ) diff --git a/models/issues/issue_test.go b/models/issues/issue_test.go index f21df067a2..46d5fc1f0e 100644 --- a/models/issues/issue_test.go +++ b/models/issues/issue_test.go @@ -157,7 +157,7 @@ func TestUpdateIssueCols(t *testing.T) { func TestIssues(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) - for _, test := range []struct { + for idx, test := range []struct { Opts issues_model.IssuesOptions ExpectedIssueIDs []int64 }{ @@ -228,7 +228,16 @@ func TestIssues(t *testing.T) { issues_model.IssuesOptions{ SubscriberID: 4, }, - []int64{11, 5, 7, 4, 3, 2, 1}, + []int64{12, 11, 6, 5, 7, 4, 3, 2, 1}, + }, + { + // This user watches repo 5 but not its issues. + // Therefore, they are not a subscriber of its issues (namely issue_id 15) + // user 5 commented on issue 1 and 3. + issues_model.IssuesOptions{ + SubscriberID: 5, + }, + []int64{3, 1}, }, { issues_model.IssuesOptions{ @@ -245,9 +254,9 @@ func TestIssues(t *testing.T) { } { issues, err := issues_model.Issues(db.DefaultContext, &test.Opts) require.NoError(t, err) - if assert.Len(t, issues, len(test.ExpectedIssueIDs)) { + if assert.Len(t, issues, len(test.ExpectedIssueIDs), "idx: %d", idx) { for i, issue := range issues { - assert.Equal(t, test.ExpectedIssueIDs[i], issue.ID) + assert.Equal(t, test.ExpectedIssueIDs[i], issue.ID, "idx: %d", idx) } } } diff --git a/models/issues/issue_watch.go b/models/issues/issue_watch.go index ecc09e1e81..52f17afc70 100644 --- a/models/issues/issue_watch.go +++ b/models/issues/issue_watch.go @@ -67,7 +67,7 @@ func GetIssueWatch(ctx context.Context, userID, issueID int64) (iw *IssueWatch, return iw, exists, err } -// CheckIssueWatch check if an user is watching an issue +// CheckIssueWatch check if a user is watching an issue // it takes participants and repo watch into account func CheckIssueWatch(ctx context.Context, user *user_model.User, issue *Issue) (bool, error) { iw, exist, err := GetIssueWatch(ctx, user.ID, issue.ID) @@ -77,11 +77,7 @@ func CheckIssueWatch(ctx context.Context, user *user_model.User, issue *Issue) ( if exist { return iw.IsWatching, nil } - w, err := repo_model.GetWatch(ctx, user.ID, issue.RepoID) - if err != nil { - return false, err - } - return repo_model.IsWatchMode(w.Mode) || IsUserParticipantsOfIssue(ctx, user, issue), nil + return repo_model.GetWatchSelection(ctx, user.ID, issue.RepoID).Issues || IsUserParticipantsOfIssue(ctx, user, issue), nil } // GetIssueWatchersIDs returns IDs of subscribers or explicit unsubscribers to a given issue id diff --git a/models/org.go b/models/org.go index 6e191acff0..f7fe165783 100644 --- a/models/org.go +++ b/models/org.go @@ -73,7 +73,7 @@ func RemoveOrgUser(ctx context.Context, orgID, userID int64) error { return fmt.Errorf("GetUserRepositories [%d]: %w", userID, err) } for _, repoID := range repoIDs { - if err = repo_model.WatchRepo(ctx, userID, repoID, false); err != nil { + if err = repo_model.WatchRepoExplicitly(ctx, userID, repoID, repo_model.WatchNoneSelection); err != nil { return err } } diff --git a/models/org_team.go b/models/org_team.go index 187ea1560d..b74c267b31 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -54,7 +54,7 @@ func InsertTeamRepository(ctx context.Context, t *organization.Team, repo *repo_ return nil, fmt.Errorf("getMembers: %w", err) } for _, u := range t.Members { - if err = repo_model.WatchRepo(ctx, u.ID, repo.ID, true); err != nil { + if err = repo_model.WatchIfAutoWatchNewRepos(ctx, u.ID, repo.ID); err != nil { return nil, fmt.Errorf("watchRepo: %w", err) } } @@ -135,7 +135,7 @@ func removeAllRepositories(ctx context.Context, t *organization.Team) (err error continue } - if err = repo_model.WatchRepo(ctx, user.ID, repo.ID, false); err != nil { + if err = repo_model.WatchRepoExplicitly(ctx, user.ID, repo.ID, repo_model.WatchNoneSelection); err != nil { return err } @@ -450,7 +450,7 @@ func InsertTeamMember(ctx context.Context, team *organization.Team, userID int64 // FIXME: in the goroutine, it can't access the "ctx", it could only use db.DefaultContext at the moment go func(repos []*repo_model.Repository) { for _, repo := range repos { - if err = repo_model.WatchRepo(db.DefaultContext, userID, repo.ID, true); err != nil { + if err = repo_model.WatchIfAutoWatchNewRepos(db.DefaultContext, userID, repo.ID); err != nil { log.Error("watch repo failed: %v", err) } } @@ -559,7 +559,7 @@ func ReconsiderWatches(ctx context.Context, repo *repo_model.Repository, uid int if has, err := access_model.HasAccess(ctx, uid, repo); err != nil || has { return err } - if err := repo_model.WatchRepo(ctx, uid, repo.ID, false); err != nil { + if err := repo_model.WatchRepoExplicitly(ctx, uid, repo.ID, repo_model.WatchNoneSelection); err != nil { return err } diff --git a/models/repo.go b/models/repo.go index 6a4da96b95..b6d8fe9334 100644 --- a/models/repo.go +++ b/models/repo.go @@ -62,7 +62,7 @@ func StatsCorrectSQL(ctx context.Context, sql any, ids ...any) error { } func repoStatsCorrectNumWatches(ctx context.Context, id int64) error { - return StatsCorrectSQL(ctx, "UPDATE `repository` SET num_watches=(SELECT COUNT(*) FROM `watch` WHERE repo_id=? AND mode<>2) WHERE id=?", id, id) + return StatsCorrectSQL(ctx, "UPDATE `repository` SET num_watches=(SELECT COUNT(*) FROM `watch` WHERE repo_id=? AND (watch_selection_issues=? OR watch_selection_pull_requests=? OR watch_selection_releases=?)) WHERE id=?", id, true, true, true, id) } func repoStatsCorrectNumStars(ctx context.Context, id int64) error { @@ -143,7 +143,7 @@ func CheckRepoStats(ctx context.Context) error { checkers := []*repoChecker{ // Repository.NumWatches { - statsQuery("SELECT repo.id FROM `repository` repo WHERE repo.num_watches!=(SELECT COUNT(*) FROM `watch` WHERE repo_id=repo.id AND mode<>2)"), + statsQuery("SELECT repo.id FROM `repository` repo WHERE repo.num_watches!=(SELECT COUNT(*) FROM `watch` WHERE repo_id=repo.id AND (watch_selection_issues=? OR watch_selection_pull_requests=? OR watch_selection_releases=?))", true, true, true), repoStatsCorrectNumWatches, "repository count 'num_watches'", }, diff --git a/models/repo/repo_test.go b/models/repo/repo_test.go index a9591a357b..c65528fa31 100644 --- a/models/repo/repo_test.go +++ b/models/repo/repo_test.go @@ -69,12 +69,12 @@ func TestWatchRepo(t *testing.T) { const repoID = 3 const userID = 2 - require.NoError(t, repo_model.WatchRepo(db.DefaultContext, userID, repoID, true)) + require.NoError(t, repo_model.WatchRepoExplicitly(db.DefaultContext, userID, repoID, repo_model.WatchAllSelection)) unittest.AssertExistsAndLoadBean(t, &repo_model.Watch{RepoID: repoID, UserID: userID}) unittest.CheckConsistencyFor(t, &repo_model.Repository{ID: repoID}) - require.NoError(t, repo_model.WatchRepo(db.DefaultContext, userID, repoID, false)) - unittest.AssertNotExistsBean(t, &repo_model.Watch{RepoID: repoID, UserID: userID}) + require.NoError(t, repo_model.WatchRepoExplicitly(db.DefaultContext, userID, repoID, repo_model.WatchNoneSelection)) + assert.Equal(t, 1, unittest.GetCount(t, &repo_model.Watch{UserID: userID, RepoID: repoID}, "source = false AND watch_selection_issues = false AND watch_selection_pull_requests = false AND watch_selection_releases = false")) unittest.CheckConsistencyFor(t, &repo_model.Repository{ID: repoID}) } diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go index e46e5e1d5a..03f2f7c84a 100644 --- a/models/repo/user_repo.go +++ b/models/repo/user_repo.go @@ -41,7 +41,7 @@ func GetStarredRepos(ctx context.Context, userID int64, private bool, listOption func GetWatchedRepos(ctx context.Context, userID int64, private bool, listOptions db.ListOptions, reducer RepositoryAuthorizationReducer) ([]*Repository, int64, error) { sess := db.GetEngine(ctx). Where("watch.user_id=?", userID). - And("`watch`.mode<>?", WatchModeDont). + And(BuilderWatchAnything()). Join("LEFT", "watch", "`repository`.id=`watch`.repo_id") if !private { sess = sess.And("is_private=?", false) @@ -153,7 +153,10 @@ func GetReviewers(ctx context.Context, repo *Repository, doerID, posterID int64) ).Or(builder.In("`user`.id", builder.Select("user_id").From("watch"). Where(builder.Eq{"repo_id": repo.ID}. - And(builder.In("mode", WatchModeNormal, WatchModeAuto))), + And( + // Only care about user watching this repo's pull requests. + builder.Eq{"`watch`.watch_selection_pull_requests": true}, + )), ).Or(builder.In("`user`.id", builder.Select("uid").From("org_user"). Where(builder.Eq{"org_id": repo.OwnerID}), @@ -193,7 +196,7 @@ func GetWatchedRepoIDsOwnedBy(ctx context.Context, userID, ownedByUserID int64) Select("`repository`.id"). Join("LEFT", "watch", "`repository`.id=`watch`.repo_id"). Where("`watch`.user_id=?", userID). - And("`watch`.mode<>?", WatchModeDont). + And(BuilderWatchAnything()). And("`repository`.owner_id=?", ownedByUserID).Find(&repoIDs) return repoIDs, err } diff --git a/models/repo/watch.go b/models/repo/watch.go index 3a20880a5c..53fbff8734 100644 --- a/models/repo/watch.go +++ b/models/repo/watch.go @@ -5,45 +5,90 @@ package repo import ( "context" + "errors" "forgejo.org/models/db" user_model "forgejo.org/models/user" "forgejo.org/modules/setting" "forgejo.org/modules/timeutil" + + "xorm.io/builder" ) -// WatchMode specifies what kind of watch the user has on a repository -type WatchMode int8 +// The main purpose of the WatchSource is to respect explicit user choice and not overwrite that with some automatic system. +type WatchSource bool const ( - // WatchModeNone don't watch - // This means there is no Watch record in the db. - // We never store this mode in the db and instead remove the record from the db. - // Furthermore, this means there is a WatchMode for all combinations of user and repo. - WatchModeNone WatchMode = iota // 0 - // WatchModeNormal watch repository (from other sources) - // This means the user explicitly chose to watch the repo. - WatchModeNormal // 1 - // WatchModeDont explicit don't auto-watch - // This means the user explicitly removed themselves as a watcher. - // Then the AutoWatchOnChanges feature doesn't make the user a watcher when they push to the repo. - WatchModeDont // 2 - // WatchModeAuto watch repository (from AutoWatchOnChanges) - // This is used when the user pushed to the repo and setting.Service.AutoWatchOnChanges is true. - // That way we can differentiate people explicitly watching the repo and people only watching it because of the AutoWatchOnChanges feature. - WatchModeAuto // 3 + // WatchSourceExplicit means the user explicitly chose to watch certain things (or none or all) of this repo. + // It means that setting.Service.AutoWatchOnChanges doesn't have an effect on this user for this repo; they explicitly made their choice after all. + // This mode replaces the old WatchModeDont and WatchModeNormal states. + WatchSourceExplicit WatchSource = false + // WatchSourceAutomatic means the user didn't explicitly select whether to watch this repo or not. + // Instead, the user either doesn't watch the repo because they didn't ever click the watch/unwatch button. + // Or they do watch the repo but only because the user pushed to the repo and setting.Service.AutoWatchOnChanges is true. + // When there is no record in the db this is the same as WatchSourceAutomatic combined with all watch selections turned off (i.e., not watching anything). + // This used to be WatchModeNone. + // When in this mode the watch selection is never fully deselected. + // Otherwise there'd be some automatic method to unwatch a repo; which does not exist. + // This mode replaces the old WatchModeAuto and WatchModeNone states. + WatchSourceAutomatic WatchSource = true + + // There may not be more modes than the above two. +) + +type WatchSelection struct { + Issues bool + PullRequests bool + Releases bool + // When changing these options or adding more don't forget to update + // BuilderWatchAnything and checkForRepoConsistency in the consistency check. +} + +// When the user is watching at least one thing on a repo they count as a watcher on the repo. +func (w WatchSelection) IsWatching() bool { + return w.Issues || w.PullRequests || w.Releases +} + +// Return true iff the user is watching everything. +func (w WatchSelection) IsFullyWatching() bool { + return w.Issues && w.PullRequests && w.Releases +} + +var ( + WatchAllSelection = WatchSelection{Issues: true, PullRequests: true, Releases: true} + WatchNoneSelection = WatchSelection{Issues: false, PullRequests: false, Releases: false} ) // Watch is connection request for receiving repository notification. type Watch struct { - ID int64 `xorm:"pk autoincr"` - UserID int64 `xorm:"UNIQUE(watch)"` - RepoID int64 `xorm:"UNIQUE(watch)"` - Mode WatchMode `xorm:"SMALLINT NOT NULL DEFAULT 1"` + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"UNIQUE(watch)"` + RepoID int64 `xorm:"UNIQUE(watch)"` + Source WatchSource `xorm:"BOOL DEFAULT TRUE"` + // In the next PR there will be another mode here, choosing the user preset or a custom selection. + // TODO: figure out whether the user preset should count as watching + // TODO: (then change the description to IsWatching) + + WatchSelectionIssues bool `xorm:"BOOL DEFAULT TRUE"` + WatchSelectionPullRequests bool `xorm:"BOOL DEFAULT TRUE"` + WatchSelectionReleases bool `xorm:"BOOL DEFAULT TRUE"` + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` } +func (w Watch) GetWatchSelection() WatchSelection { + return WatchSelection{Issues: w.WatchSelectionIssues, PullRequests: w.WatchSelectionPullRequests, Releases: w.WatchSelectionReleases} +} + +// Warning: this does not set the WatchSource. +// The caller needs to do that properly. +func (w *Watch) setWatchSelection(watchSelection WatchSelection) { + w.WatchSelectionIssues = watchSelection.Issues + w.WatchSelectionPullRequests = watchSelection.PullRequests + w.WatchSelectionReleases = watchSelection.Releases +} + func init() { db.RegisterModel(new(Watch)) } @@ -56,116 +101,150 @@ func GetWatch(ctx context.Context, userID, repoID int64) (Watch, error) { return watch, err } if !has { - watch.Mode = WatchModeNone + watch.Source = WatchSourceAutomatic + watch.setWatchSelection(WatchNoneSelection) } return watch, nil } -// IsWatchMode Decodes watchability of WatchMode -func IsWatchMode(mode WatchMode) bool { - return mode != WatchModeNone && mode != WatchModeDont -} - -// IsWatching checks if user has watched given repository. -func IsWatching(ctx context.Context, userID, repoID int64) bool { +// IsWatcher checks if the user is a watcher of the repo. +func IsWatcher(ctx context.Context, userID, repoID int64) bool { watch, err := GetWatch(ctx, userID, repoID) - return err == nil && IsWatchMode(watch.Mode) + return err == nil && watch.GetWatchSelection().IsWatching() } -func watchRepoMode(ctx context.Context, watch Watch, mode WatchMode) (err error) { - if watch.Mode == mode { +// GetWatchSelection returns what parts of a repo a user is watching. +// When you don't care if the user watches something explicitly or automatically this should suffice. +func GetWatchSelection(ctx context.Context, userID, repoID int64) WatchSelection { + watch, err := GetWatch(ctx, userID, repoID) + if err != nil { + return WatchNoneSelection + } + return watch.GetWatchSelection() +} + +// Change the watch status on the provided Watch instance. +// oldWatch contains the prior state. +func updateWatchRepo(ctx context.Context, oldWatch Watch, source WatchSource, watchSelection WatchSelection) (err error) { + if oldWatch.Source == source && oldWatch.GetWatchSelection() == watchSelection { return nil } - if mode == WatchModeAuto && (watch.Mode == WatchModeDont || IsWatchMode(watch.Mode)) { - // Don't auto watch if already watching or deliberately not watching - return nil + if source == WatchSourceAutomatic && oldWatch.Source == WatchSourceExplicit { + // This should never happen and indicates a bug. + return errors.New("we should never switch from an explicit watch state to an automatic one") + } + + hadrec, err := db.GetEngine(ctx).Get(&oldWatch) + if err != nil { + return err } - hadrec := watch.Mode != WatchModeNone - // WatchModeNone means there is no record in the db. - needsrec := mode != WatchModeNone repodiff := 0 - - if IsWatchMode(mode) && !IsWatchMode(watch.Mode) { + if watchSelection.IsWatching() && !oldWatch.GetWatchSelection().IsWatching() { repodiff = 1 - } else if !IsWatchMode(mode) && IsWatchMode(watch.Mode) { + } else if !watchSelection.IsWatching() && oldWatch.GetWatchSelection().IsWatching() { repodiff = -1 } - watch.Mode = mode - - if !hadrec && needsrec { - watch.Mode = mode - if err = db.Insert(ctx, watch); err != nil { + if !hadrec { + oldWatch.Source = source + oldWatch.setWatchSelection(watchSelection) + if err = db.Insert(ctx, oldWatch); err != nil { return err } - } else if needsrec { - watch.Mode = mode - if _, err := db.GetEngine(ctx).ID(watch.ID).AllCols().Update(watch); err != nil { - return err - } - } else if _, err = db.DeleteByID[Watch](ctx, watch.ID); err != nil { - return err - } - if repodiff != 0 { - _, err = db.GetEngine(ctx).Exec("UPDATE `repository` SET num_watches = num_watches + ? WHERE id = ?", repodiff, watch.RepoID) - } - return err -} - -// WatchRepoMode watch repository in specific mode. -func WatchRepoMode(ctx context.Context, userID, repoID int64, mode WatchMode) (err error) { - var watch Watch - if watch, err = GetWatch(ctx, userID, repoID); err != nil { - return err - } - return watchRepoMode(ctx, watch, mode) -} - -// WatchRepo watch or unwatch repository. -func WatchRepo(ctx context.Context, userID, repoID int64, doWatch bool) (err error) { - var watch Watch - if watch, err = GetWatch(ctx, userID, repoID); err != nil { - return err - } - if !doWatch && watch.Mode == WatchModeAuto { - err = watchRepoMode(ctx, watch, WatchModeDont) - } else if !doWatch { - err = watchRepoMode(ctx, watch, WatchModeNone) } else { - err = watchRepoMode(ctx, watch, WatchModeNormal) + oldWatch.Source = source + oldWatch.setWatchSelection(watchSelection) + if _, err := db.GetEngine(ctx).ID(oldWatch.ID).AllCols().Update(oldWatch); err != nil { + return err + } + } + // Notice that we never delete a record. + // We could do that in the case of an automatic source and none selection. + // But that would only save some db space and we never go into that state when we've been in a different one at some point. + + if repodiff != 0 { + _, err = db.GetEngine(ctx).Exec("UPDATE `repository` SET num_watches = num_watches + ? WHERE id = ?", repodiff, oldWatch.RepoID) } return err } -// GetWatchers returns all watchers of given repository. -func GetWatchers(ctx context.Context, repoID int64) ([]*Watch, error) { +// WatchRepoExplicitly explicitly watch or unwatch repository according to some selection. +func WatchRepoExplicitly(ctx context.Context, userID, repoID int64, watchSelection WatchSelection) (err error) { + var watch Watch + if watch, err = GetWatch(ctx, userID, repoID); err != nil { + return err + } + err = updateWatchRepo(ctx, watch, WatchSourceExplicit, watchSelection) + return err +} + +// GetSelectWatchers returns all users watching any of the selected parts of the repo with the given repoID. +func GetSelectWatchers(ctx context.Context, repoID int64, selection WatchSelection) ([]*Watch, error) { + sqlSelection := []builder.Cond{} + if selection.Issues { + sqlSelection = append(sqlSelection, builder.Eq{"`watch`.watch_selection_issues": true}) + } + if selection.PullRequests { + sqlSelection = append(sqlSelection, builder.Eq{"`watch`.watch_selection_pull_requests": true}) + } + if selection.Releases { + sqlSelection = append(sqlSelection, builder.Eq{"`watch`.watch_selection_releases": true}) + } + // When we don't select anything, don't return anything. + if len(sqlSelection) == 0 { + return []*Watch{}, nil + } + watches := make([]*Watch, 0, 10) return watches, db.GetEngine(ctx).Where("`watch`.repo_id=?", repoID). - And("`watch`.mode<>?", WatchModeDont). + And(builder.Or(sqlSelection...)). And("`user`.is_active=?", true). And("`user`.prohibit_login=?", false). Join("INNER", "`user`", "`user`.id = `watch`.user_id"). Find(&watches) } -// GetRepoWatchersIDs returns IDs of watchers for a given repo ID +// GetSelectWatcherIDs returns IDs of users watching any of the selected parts of the repo with the given repoID. // but avoids joining with `user` for performance reasons // User permissions must be verified elsewhere if required -func GetRepoWatchersIDs(ctx context.Context, repoID int64) ([]int64, error) { +func GetSelectWatcherIDs(ctx context.Context, repoID int64, selection WatchSelection) ([]int64, error) { + sqlSelection := []builder.Cond{} + if selection.Issues { + sqlSelection = append(sqlSelection, builder.Eq{"`watch`.watch_selection_issues": true}) + } + if selection.PullRequests { + sqlSelection = append(sqlSelection, builder.Eq{"`watch`.watch_selection_pull_requests": true}) + } + if selection.Releases { + sqlSelection = append(sqlSelection, builder.Eq{"`watch`.watch_selection_releases": true}) + } + // When we don't select anything, don't return anything. + if len(sqlSelection) == 0 { + return []int64{}, nil + } + ids := make([]int64, 0, 64) return ids, db.GetEngine(ctx).Table("watch"). Where("watch.repo_id=?", repoID). - And("watch.mode<>?", WatchModeDont). + And(builder.Or(sqlSelection...)). Select("user_id"). Find(&ids) } +func BuilderWatchAnything() builder.Cond { + return builder.Or( + builder.Eq{"`watch`.watch_selection_issues": true}, + builder.Eq{"`watch`.watch_selection_pull_requests": true}, + builder.Eq{"`watch`.watch_selection_releases": true}, + ) +} + // GetRepoWatchers returns range of users watching given repository. func GetRepoWatchers(ctx context.Context, repoID int64, opts db.ListOptions) ([]*user_model.User, error) { sess := db.GetEngine(ctx).Where("watch.repo_id=?", repoID). Join("LEFT", "watch", "`user`.id=`watch`.user_id"). - And("`watch`.mode<>?", WatchModeDont) + And(BuilderWatchAnything()) if opts.Page > 0 { sess = db.SetSessionPagination(sess, &opts) users := make([]*user_model.User, 0, opts.PageSize) @@ -177,26 +256,52 @@ func GetRepoWatchers(ctx context.Context, repoID int64, opts db.ListOptions) ([] return users, sess.Find(&users) } -// WatchIfAuto subscribes to repo if AutoWatchOnChanges is set -func WatchIfAuto(ctx context.Context, userID, repoID int64) error { - if !setting.Service.AutoWatchOnChanges { - return nil - } +// Respect explicit user wish and don't change explicit watches. +func autoWatch(ctx context.Context, userID, repoID int64) error { watch, err := GetWatch(ctx, userID, repoID) if err != nil { return err } - if watch.Mode != WatchModeNone { + if watch.Source == WatchSourceExplicit { return nil } - return watchRepoMode(ctx, watch, WatchModeAuto) + // TODO: This would be a place to check that we are using the user preset (which doesn't exist yet). + if watch.GetWatchSelection().IsFullyWatching() { + return nil + } + return updateWatchRepo(ctx, watch, WatchSourceAutomatic, WatchAllSelection) +} + +// WatchIfAutoWatchOnChanges subscribes to repo if AutoWatchOnChanges is set +func WatchIfAutoWatchOnChanges(ctx context.Context, userID, repoID int64) error { + if !setting.Service.AutoWatchOnChanges { + return nil + } + return autoWatch(ctx, userID, repoID) +} + +// WatchIfAutoWatchNewRepos subscribes to repo if AutoWatchNewRepos is set +func WatchIfAutoWatchNewRepos(ctx context.Context, userID, repoID int64) error { + if !setting.Service.AutoWatchNewRepos { + return nil + } + return autoWatch(ctx, userID, repoID) } // UnwatchRepos will unwatch the user from all given repositories. func UnwatchRepos(ctx context.Context, userID int64, repoIDs []int64) error { // Unfortunatly, we can't simply delete the Watch records because we do watcher counting in the repo relation. for _, repoID := range repoIDs { - err := WatchRepoMode(ctx, userID, repoID, WatchModeNone) + var watch Watch + var err error + if watch, err = GetWatch(ctx, userID, repoID); err != nil { + return err + } + // This is a lot simpler than it could be. + // E.g. when a user explicitly watches a repo and gets blocked, they now explicitly unwatch the repo. + // That means that when the user won't receive the watch status automatically later on, even when they've been unblocked again. + // I think this is not too big a problem to add more logic here. + err = updateWatchRepo(ctx, watch, WatchSourceExplicit, WatchNoneSelection) if err != nil { return err } diff --git a/models/repo/watch_test.go b/models/repo/watch_test.go index ccc56ad168..4443cdfc75 100644 --- a/models/repo/watch_test.go +++ b/models/repo/watch_test.go @@ -18,30 +18,98 @@ import ( func TestIsWatching(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) - assert.True(t, repo_model.IsWatching(db.DefaultContext, 1, 1)) - assert.True(t, repo_model.IsWatching(db.DefaultContext, 4, 1)) - assert.True(t, repo_model.IsWatching(db.DefaultContext, 11, 1)) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 1, 1)) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 4, 1)) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 11, 1)) - assert.False(t, repo_model.IsWatching(db.DefaultContext, 1, 5)) - assert.False(t, repo_model.IsWatching(db.DefaultContext, 8, 1)) - assert.False(t, repo_model.IsWatching(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID)) + assert.False(t, repo_model.IsWatcher(db.DefaultContext, 1, 5)) + assert.False(t, repo_model.IsWatcher(db.DefaultContext, 8, 1)) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 4, 3)) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 4, 4)) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 4, 5)) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 5, 5)) + watch, err := repo_model.GetWatch(db.DefaultContext, 5, 5) + require.NoError(t, err) + assert.Equal(t, repo_model.WatchSourceExplicit, watch.Source) + assert.False(t, watch.WatchSelectionIssues) + assert.True(t, watch.WatchSelectionPullRequests) + assert.True(t, watch.WatchSelectionReleases) + + assert.False(t, repo_model.IsWatcher(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID)) } -func TestGetWatchers(t *testing.T) { - require.NoError(t, unittest.PrepareTestDatabase()) +func TestGetSelectWatchers(t *testing.T) { + for idx, test := range []struct { + RepoID int64 + Selection repo_model.WatchSelection + ExpectedUserIDs []int64 + }{ + { + 1, + repo_model.WatchNoneSelection, + // This should always be empty for any repo. + []int64{}, + }, + { + 3, + repo_model.WatchNoneSelection, + // This should always be empty for any repo. + []int64{}, + }, + { + 3, + repo_model.WatchAllSelection, + []int64{4}, + }, + { + 3, + repo_model.WatchSelection{Issues: true}, + []int64{4}, + }, + { + 3, + repo_model.WatchSelection{PullRequests: true}, + []int64{}, + }, + { + 3, + repo_model.WatchSelection{Releases: true}, + []int64{}, + }, + { + 3, + repo_model.WatchSelection{Releases: true, PullRequests: true}, + []int64{}, + }, - repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) - watches, err := repo_model.GetWatchers(db.DefaultContext, repo.ID) - require.NoError(t, err) - // One watchers are inactive, thus minus 1 - assert.Len(t, watches, repo.NumWatches-1) - for _, watch := range watches { - assert.Equal(t, repo.ID, watch.RepoID) + { + 5, + repo_model.WatchSelection{Releases: true}, + []int64{4, 5}, + }, + { + 5, + repo_model.WatchSelection{Releases: true, PullRequests: true}, + []int64{4, 5}, + }, + { + 5, + repo_model.WatchSelection{PullRequests: true}, + []int64{5}, + }, + } { + watchers, err := repo_model.GetSelectWatchers(db.DefaultContext, test.RepoID, test.Selection) + require.NoError(t, err) + if assert.Len(t, watchers, len(test.ExpectedUserIDs), "idx: %d", idx) { + for i, watcher := range watchers { + assert.Equal(t, test.ExpectedUserIDs[i], watcher.UserID) + } + } + + watcherIDs, err := repo_model.GetSelectWatcherIDs(db.DefaultContext, test.RepoID, test.Selection) + require.NoError(t, err) + assert.Equal(t, test.ExpectedUserIDs, watcherIDs, "idx: %d", idx) } - - watches, err = repo_model.GetWatchers(db.DefaultContext, unittest.NonexistentID) - require.NoError(t, err) - assert.Empty(t, watches) } func TestRepository_GetWatchers(t *testing.T) { @@ -61,7 +129,139 @@ func TestRepository_GetWatchers(t *testing.T) { assert.Empty(t, watchers) } -func TestWatchIfAuto(t *testing.T) { +func TestWatchRepoExplicitly(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + // There is no record for this watch in the fixture. + assert.False(t, repo_model.IsWatcher(db.DefaultContext, 1, 2)) + { + watch, err := repo_model.GetWatch(db.DefaultContext, 1, 2) + require.NoError(t, err) + assert.Equal(t, repo_model.WatchSourceAutomatic, watch.Source) + assert.False(t, watch.WatchSelectionIssues) + assert.False(t, watch.WatchSelectionPullRequests) + assert.False(t, watch.WatchSelectionReleases) + } + + require.NoError(t, repo_model.WatchRepoExplicitly(db.DefaultContext, 1, 2, repo_model.WatchAllSelection)) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 1, 2)) + { + watch, err := repo_model.GetWatch(db.DefaultContext, 1, 2) + require.NoError(t, err) + assert.Equal(t, repo_model.WatchSourceExplicit, watch.Source) + assert.True(t, watch.WatchSelectionIssues) + assert.True(t, watch.WatchSelectionPullRequests) + assert.True(t, watch.WatchSelectionReleases) + } + + require.NoError(t, repo_model.WatchRepoExplicitly(db.DefaultContext, 1, 2, repo_model.WatchNoneSelection)) + assert.False(t, repo_model.IsWatcher(db.DefaultContext, 1, 2)) + { + watch, err := repo_model.GetWatch(db.DefaultContext, 1, 2) + require.NoError(t, err) + assert.Equal(t, repo_model.WatchSourceExplicit, watch.Source) + assert.False(t, watch.WatchSelectionIssues) + assert.False(t, watch.WatchSelectionPullRequests) + assert.False(t, watch.WatchSelectionReleases) + } + + require.NoError(t, repo_model.WatchRepoExplicitly(db.DefaultContext, 1, 2, repo_model.WatchSelection{Issues: true})) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 1, 2)) + { + watch, err := repo_model.GetWatch(db.DefaultContext, 1, 2) + require.NoError(t, err) + assert.Equal(t, repo_model.WatchSourceExplicit, watch.Source) + assert.True(t, watch.WatchSelectionIssues) + assert.False(t, watch.WatchSelectionPullRequests) + assert.False(t, watch.WatchSelectionReleases) + } + + require.NoError(t, repo_model.WatchRepoExplicitly(db.DefaultContext, 1, 2, repo_model.WatchSelection{Issues: true, PullRequests: true})) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 1, 2)) + { + watch, err := repo_model.GetWatch(db.DefaultContext, 1, 2) + require.NoError(t, err) + assert.Equal(t, repo_model.WatchSourceExplicit, watch.Source) + assert.True(t, watch.WatchSelectionIssues) + assert.True(t, watch.WatchSelectionPullRequests) + assert.False(t, watch.WatchSelectionReleases) + } + + require.NoError(t, repo_model.WatchRepoExplicitly(db.DefaultContext, 1, 2, repo_model.WatchSelection{Releases: true})) + assert.True(t, repo_model.IsWatcher(db.DefaultContext, 1, 2)) + { + watch, err := repo_model.GetWatch(db.DefaultContext, 1, 2) + require.NoError(t, err) + assert.Equal(t, repo_model.WatchSourceExplicit, watch.Source) + assert.False(t, watch.WatchSelectionIssues) + assert.False(t, watch.WatchSelectionPullRequests) + assert.True(t, watch.WatchSelectionReleases) + } +} + +func TestWatchIfAutoWatchNewRepos(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + watchers, err := repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) + require.NoError(t, err) + assert.Len(t, watchers, repo.NumWatches) + + setting.Service.AutoWatchNewRepos = false + + prevCount := repo.NumWatches + + // Must not add watch + require.NoError(t, repo_model.WatchIfAutoWatchNewRepos(db.DefaultContext, 8, 1)) + watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) + require.NoError(t, err) + assert.Len(t, watchers, prevCount) + + // Should not add watch + require.NoError(t, repo_model.WatchIfAutoWatchNewRepos(db.DefaultContext, 10, 1)) + watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) + require.NoError(t, err) + assert.Len(t, watchers, prevCount) + + setting.Service.AutoWatchNewRepos = true + + // Must not add watch + require.NoError(t, repo_model.WatchIfAutoWatchNewRepos(db.DefaultContext, 8, 1)) + watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) + require.NoError(t, err) + assert.Len(t, watchers, prevCount) + + // Should not add watch + // We simply don't WatchIfAuto + watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) + require.NoError(t, err) + assert.Len(t, watchers, prevCount) + + // Should add watch + require.NoError(t, repo_model.WatchIfAutoWatchNewRepos(db.DefaultContext, 12, 1)) + watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) + require.NoError(t, err) + assert.Len(t, watchers, prevCount+1) + watch, err := repo_model.GetWatch(db.DefaultContext, 12, 1) + require.NoError(t, err) + require.Equal(t, repo_model.WatchSourceAutomatic, watch.Source) + require.True(t, watch.WatchSelectionIssues) + require.True(t, watch.WatchSelectionPullRequests) + require.True(t, watch.WatchSelectionReleases) + + // Should remove watch, inhibit from adding auto + require.NoError(t, repo_model.WatchRepoExplicitly(db.DefaultContext, 12, 1, repo_model.WatchNoneSelection)) + watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) + require.NoError(t, err) + assert.Len(t, watchers, prevCount) + + // Must not add watch + require.NoError(t, repo_model.WatchIfAutoWatchNewRepos(db.DefaultContext, 12, 1)) + watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) + require.NoError(t, err) + assert.Len(t, watchers, prevCount) +} + +func TestWatchIfAutoWatchOnChanges(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) @@ -74,13 +274,13 @@ func TestWatchIfAuto(t *testing.T) { prevCount := repo.NumWatches // Must not add watch - require.NoError(t, repo_model.WatchIfAuto(db.DefaultContext, 8, 1)) + require.NoError(t, repo_model.WatchIfAutoWatchOnChanges(db.DefaultContext, 8, 1)) watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) require.NoError(t, err) assert.Len(t, watchers, prevCount) // Should not add watch - require.NoError(t, repo_model.WatchIfAuto(db.DefaultContext, 10, 1)) + require.NoError(t, repo_model.WatchIfAutoWatchOnChanges(db.DefaultContext, 10, 1)) watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) require.NoError(t, err) assert.Len(t, watchers, prevCount) @@ -88,7 +288,7 @@ func TestWatchIfAuto(t *testing.T) { setting.Service.AutoWatchOnChanges = true // Must not add watch - require.NoError(t, repo_model.WatchIfAuto(db.DefaultContext, 8, 1)) + require.NoError(t, repo_model.WatchIfAutoWatchOnChanges(db.DefaultContext, 8, 1)) watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) require.NoError(t, err) assert.Len(t, watchers, prevCount) @@ -100,45 +300,30 @@ func TestWatchIfAuto(t *testing.T) { assert.Len(t, watchers, prevCount) // Should add watch - require.NoError(t, repo_model.WatchIfAuto(db.DefaultContext, 12, 1)) + require.NoError(t, repo_model.WatchIfAutoWatchOnChanges(db.DefaultContext, 12, 1)) watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) require.NoError(t, err) assert.Len(t, watchers, prevCount+1) + watch, err := repo_model.GetWatch(db.DefaultContext, 12, 1) + require.NoError(t, err) + require.Equal(t, repo_model.WatchSourceAutomatic, watch.Source) + require.True(t, watch.WatchSelectionIssues) + require.True(t, watch.WatchSelectionPullRequests) + require.True(t, watch.WatchSelectionReleases) // Should remove watch, inhibit from adding auto - require.NoError(t, repo_model.WatchRepo(db.DefaultContext, 12, 1, false)) + require.NoError(t, repo_model.WatchRepoExplicitly(db.DefaultContext, 12, 1, repo_model.WatchNoneSelection)) watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) require.NoError(t, err) assert.Len(t, watchers, prevCount) // Must not add watch - require.NoError(t, repo_model.WatchIfAuto(db.DefaultContext, 12, 1)) + require.NoError(t, repo_model.WatchIfAutoWatchOnChanges(db.DefaultContext, 12, 1)) watchers, err = repo_model.GetRepoWatchers(db.DefaultContext, repo.ID, db.ListOptions{Page: 1}) require.NoError(t, err) assert.Len(t, watchers, prevCount) } -func TestWatchRepoMode(t *testing.T) { - require.NoError(t, unittest.PrepareTestDatabase()) - - unittest.AssertCount(t, &repo_model.Watch{UserID: 12, RepoID: 1}, 0) - - require.NoError(t, repo_model.WatchRepoMode(db.DefaultContext, 12, 1, repo_model.WatchModeAuto)) - unittest.AssertCount(t, &repo_model.Watch{UserID: 12, RepoID: 1}, 1) - unittest.AssertCount(t, &repo_model.Watch{UserID: 12, RepoID: 1, Mode: repo_model.WatchModeAuto}, 1) - - require.NoError(t, repo_model.WatchRepoMode(db.DefaultContext, 12, 1, repo_model.WatchModeNormal)) - unittest.AssertCount(t, &repo_model.Watch{UserID: 12, RepoID: 1}, 1) - unittest.AssertCount(t, &repo_model.Watch{UserID: 12, RepoID: 1, Mode: repo_model.WatchModeNormal}, 1) - - require.NoError(t, repo_model.WatchRepoMode(db.DefaultContext, 12, 1, repo_model.WatchModeDont)) - unittest.AssertCount(t, &repo_model.Watch{UserID: 12, RepoID: 1}, 1) - unittest.AssertCount(t, &repo_model.Watch{UserID: 12, RepoID: 1, Mode: repo_model.WatchModeDont}, 1) - - require.NoError(t, repo_model.WatchRepoMode(db.DefaultContext, 12, 1, repo_model.WatchModeNone)) - unittest.AssertCount(t, &repo_model.Watch{UserID: 12, RepoID: 1}, 0) -} - func TestUnwatchRepos(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) @@ -148,6 +333,6 @@ func TestUnwatchRepos(t *testing.T) { err := repo_model.UnwatchRepos(db.DefaultContext, 4, []int64{1, 2}) require.NoError(t, err) - unittest.AssertNotExistsBean(t, &repo_model.Watch{UserID: 4, RepoID: 1}) - unittest.AssertNotExistsBean(t, &repo_model.Watch{UserID: 4, RepoID: 2}) + assert.Equal(t, 1, unittest.GetCount(t, &repo_model.Watch{UserID: 4, RepoID: 1}, "source = false AND watch_selection_issues = false AND watch_selection_pull_requests = false AND watch_selection_releases = false")) + assert.Equal(t, 1, unittest.GetCount(t, &repo_model.Watch{UserID: 4, RepoID: 2}, "source = false AND watch_selection_issues = false AND watch_selection_pull_requests = false AND watch_selection_releases = false")) } diff --git a/models/unittest/consistency.go b/models/unittest/consistency.go index fbff51bf81..feb95495a9 100644 --- a/models/unittest/consistency.go +++ b/models/unittest/consistency.go @@ -19,7 +19,6 @@ import ( const ( // these const values are copied from `models` package to prevent from cycle-import modelsUserTypeOrganization = 1 - modelsRepoWatchModeDont = 2 modelsCommentTypeComment = 0 ) @@ -90,7 +89,12 @@ func init() { } actual := GetCountByCond(t, "watch", builder.Eq{"repo_id": repo.int("ID")}. - And(builder.Neq{"mode": modelsRepoWatchModeDont})) + // This could be BuilderWatchAnything() but that would introduce an import cycle. + And(builder.Or( + builder.Eq{"`watch`.watch_selection_issues": true}, + builder.Eq{"`watch`.watch_selection_pull_requests": true}, + builder.Eq{"`watch`.watch_selection_releases": true}, + ))) assert.EqualValues(t, repo.int("NumWatches"), actual, "Unexpected number of watches for repo id: %d", repo.int("ID")) diff --git a/modules/repository/create.go b/modules/repository/create.go index becfed0370..c7182e2b48 100644 --- a/modules/repository/create.go +++ b/modules/repository/create.go @@ -153,7 +153,7 @@ func CreateRepositoryByExample(ctx context.Context, doer, u *user_model.User, re } if setting.Service.AutoWatchNewRepos { - if err = repo_model.WatchRepo(ctx, doer.ID, repo.ID, true); err != nil { + if err = repo_model.WatchIfAutoWatchNewRepos(ctx, doer.ID, repo.ID); err != nil { return fmt.Errorf("WatchRepo: %w", err) } } diff --git a/options/locale_next/locale_en-US.json b/options/locale_next/locale_en-US.json index fc0e0bd92d..a81d2b99f4 100644 --- a/options/locale_next/locale_en-US.json +++ b/options/locale_next/locale_en-US.json @@ -942,6 +942,11 @@ "editor.toggle_case": "Toggle case sensitivity", "editor.toggle_regex": "Toggle using regular expressions", "editor.toggle_whole_word": "Toggle matching whole words", + "repo.watching": "Watching", + "repo.watch.issues": "Issues", + "repo.watch.pull_requests": "Pull requests", + "repo.watch.releases": "Releases", + "repo.watch.releases.hint": "(Email only)", "markup.filepreview.line": "Line %[1]d in %[2]s", "markup.filepreview.lines": "Lines %[1]d to %[2]d in %[3]s", "markup.filepreview.truncated": "Preview has been truncated", diff --git a/routers/api/v1/user/watch.go b/routers/api/v1/user/watch.go index 6c167b854b..99d0dd8055 100644 --- a/routers/api/v1/user/watch.go +++ b/routers/api/v1/user/watch.go @@ -133,7 +133,7 @@ func IsWatching(ctx *context.APIContext) { // "404": // description: User is not watching this repo or repo do not exist - if repo_model.IsWatching(ctx, ctx.Doer().ID, ctx.Repo().Repository.ID) { + if repo_model.IsWatcher(ctx, ctx.Doer().ID, ctx.Repo().Repository.ID) { ctx.JSON(http.StatusOK, api.WatchInfo{ Subscribed: true, Ignored: false, @@ -169,7 +169,7 @@ func Watch(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - err := repo_model.WatchRepo(ctx, ctx.Doer().ID, ctx.Repo().Repository.ID, true) + err := repo_model.WatchRepoExplicitly(ctx, ctx.Doer().ID, ctx.Repo().Repository.ID, repo_model.WatchAllSelection) if err != nil { ctx.Error(http.StatusInternalServerError, "WatchRepo", err) return @@ -206,7 +206,7 @@ func Unwatch(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - err := repo_model.WatchRepo(ctx, ctx.Doer().ID, ctx.Repo().Repository.ID, false) + err := repo_model.WatchRepoExplicitly(ctx, ctx.Doer().ID, ctx.Repo().Repository.ID, repo_model.WatchNoneSelection) if err != nil { ctx.Error(http.StatusInternalServerError, "UnwatchRepo", err) return diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index e5ede13bf5..c0e997eba9 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -317,20 +317,45 @@ const ( tplStarUnstar base.TplName = "repo/star_unstar" ) -func ActionWatch(watch bool) func(ctx *context.Context) { +func ActionWatch(ctx *context.Context) { + watchSelection := repo_model.WatchSelection{ + Issues: ctx.FormBool("watch_issues"), + PullRequests: ctx.FormBool("watch_pull_requests"), + Releases: ctx.FormBool("watch_releases"), + } + + err := repo_model.WatchRepoExplicitly(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, watchSelection) + if err != nil { + ctx.ServerError(fmt.Sprintf("Action (watch, %t)", watchSelection), err) + return + } + + ctx.Data["RepoWatchSelection"] = repo_model.GetWatchSelection(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID) + + // we have to reload the repository because NumStars or NumWatching (used in the templates) has just changed + ctx.Data["Repository"], err = repo_model.GetRepositoryByName(ctx, ctx.Repo.Repository.OwnerID, ctx.Repo.Repository.Name) + if err != nil { + ctx.ServerError(fmt.Sprintf("Action (watch, %t)", watchSelection), err) + return + } + + ctx.HTML(http.StatusOK, tplWatchUnwatch) +} + +func ActionWatchConst(watchSelection repo_model.WatchSelection) func(ctx *context.Context) { return func(ctx *context.Context) { - err := repo_model.WatchRepo(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, watch) + err := repo_model.WatchRepoExplicitly(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID, watchSelection) if err != nil { - ctx.ServerError(fmt.Sprintf("Action (watch, %t)", watch), err) + ctx.ServerError(fmt.Sprintf("Action (watch, %t)", watchSelection), err) return } - ctx.Data["IsWatchingRepo"] = repo_model.IsWatching(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID) + ctx.Data["RepoWatchSelection"] = repo_model.GetWatchSelection(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID) - // we have to reload the repository because NumStars or NumWatching (used in the templates) has just changed + // We have to reload the repository because NumWatching (used in the templates) might have just changed. ctx.Data["Repository"], err = repo_model.GetRepositoryByName(ctx, ctx.Repo.Repository.OwnerID, ctx.Repo.Repository.Name) if err != nil { - ctx.ServerError(fmt.Sprintf("Action (watch, %t)", watch), err) + ctx.ServerError(fmt.Sprintf("Action (watch, %t)", watchSelection), err) return } diff --git a/routers/web/web.go b/routers/web/web.go index 3a91a05e1f..ca95477f3f 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -14,6 +14,7 @@ import ( auth_model "forgejo.org/models/auth" "forgejo.org/models/perm" quota_model "forgejo.org/models/quota" + repo_model "forgejo.org/models/repo" "forgejo.org/models/unit" "forgejo.org/modules/avatar" "forgejo.org/modules/log" @@ -1333,8 +1334,9 @@ func registerRoutes(m *web.Route) { }, reqSignIn, context.RepoAssignment, context.UnitTypes(), reqRepoAdmin, context.RepoRef()) m.Group("/{username}/{reponame}/action", func() { - m.Post("/watch", repo.ActionWatch(true)) - m.Post("/unwatch", repo.ActionWatch(false)) + m.Post("/watch/select", repo.ActionWatch) + m.Post("/watch", repo.ActionWatchConst(repo_model.WatchAllSelection)) + m.Post("/unwatch", repo.ActionWatchConst(repo_model.WatchNoneSelection)) m.Post("/accept_transfer", repo.ActionTransfer(true)) m.Post("/reject_transfer", repo.ActionTransfer(false)) if !setting.Repository.DisableStars { diff --git a/services/context/repo.go b/services/context/repo.go index f39b6d342d..ba8b2391ff 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -643,7 +643,7 @@ func RepoAssignment(ctx *Context) context.CancelFunc { } if ctx.IsSigned { - ctx.Data["IsWatchingRepo"] = repo_model.IsWatching(ctx, ctx.Doer.ID, repo.ID) + ctx.Data["RepoWatchSelection"] = repo_model.GetWatchSelection(ctx, ctx.Doer.ID, repo.ID) ctx.Data["IsStaringRepo"] = repo_model.IsStaring(ctx, ctx.Doer.ID, repo.ID) } diff --git a/services/mailer/fixtures/TestMailNewRelease/watch.yml b/services/mailer/fixtures/TestMailNewRelease/watch.yml index 4c488a6f85..8170d6d87e 100644 --- a/services/mailer/fixtures/TestMailNewRelease/watch.yml +++ b/services/mailer/fixtures/TestMailNewRelease/watch.yml @@ -2,4 +2,7 @@ id: 1001 user_id: 11 repo_id: 41 - mode: 1 + source: false # explicit + watch_selection_issues: true + watch_selection_pull_requests: true + watch_selection_releases: true diff --git a/services/mailer/mail_issue.go b/services/mailer/mail_issue.go index 9a1c8be37e..415260361b 100644 --- a/services/mailer/mail_issue.go +++ b/services/mailer/mail_issue.go @@ -97,7 +97,7 @@ func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*user_mo // =========== 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) + ids, err = repo_model.GetSelectWatcherIDs(ctx, ctx.Issue.RepoID, repo_model.WatchSelection{Issues: true, PullRequests: false, Releases: false}) if err != nil { return fmt.Errorf("GetRepoWatchersIDs(%d): %w", ctx.Issue.RepoID, err) } diff --git a/services/mailer/mail_release.go b/services/mailer/mail_release.go index 2111083bd4..433578b170 100644 --- a/services/mailer/mail_release.go +++ b/services/mailer/mail_release.go @@ -31,7 +31,7 @@ func MailNewRelease(ctx context.Context, rel *repo_model.Release) { return } - watcherIDList, err := repo_model.GetRepoWatchersIDs(ctx, rel.RepoID) + watcherIDList, err := repo_model.GetSelectWatcherIDs(ctx, rel.RepoID, repo_model.WatchSelection{Issues: false, PullRequests: false, Releases: true}) if err != nil { log.Error("GetRepoWatchersIDs(%d): %v", rel.RepoID, err) return diff --git a/services/repository/collaboration.go b/services/repository/collaboration.go index 7a0d7edb7f..87eef1838a 100644 --- a/services/repository/collaboration.go +++ b/services/repository/collaboration.go @@ -35,7 +35,7 @@ func DeleteCollaboration(ctx context.Context, repo *repo_model.Repository, uid i return err } - if err = repo_model.WatchRepo(ctx, uid, repo.ID, false); err != nil { + if err = repo_model.WatchRepoExplicitly(ctx, uid, repo.ID, repo_model.WatchNoneSelection); err != nil { return err } diff --git a/services/repository/delete.go b/services/repository/delete.go index 00cfff31b4..731a82a730 100644 --- a/services/repository/delete.go +++ b/services/repository/delete.go @@ -453,7 +453,7 @@ func removeRepositoryFromTeam(ctx context.Context, t *organization.Team, repo *r continue } - if err = repo_model.WatchRepo(ctx, teamUser.UID, repo.ID, false); err != nil { + if err = repo_model.WatchRepoExplicitly(ctx, teamUser.UID, repo.ID, repo_model.WatchNoneSelection); err != nil { return err } diff --git a/services/repository/push.go b/services/repository/push.go index f42b231f8b..5c09098515 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -271,7 +271,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { } // Even if user delete a branch on a repository which he didn't watch, he will be watch that. - if err = repo_model.WatchIfAuto(ctx, opts.PusherID, repo.ID); err != nil { + if err = repo_model.WatchIfAutoWatchOnChanges(ctx, opts.PusherID, repo.ID); err != nil { log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err) } } else { diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 6026d85ae1..838030167d 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -201,13 +201,13 @@ func transferOwnership(ctx context.Context, doer *user_model.User, newOwnerName return fmt.Errorf("decrease old owner repository count: %w", err) } - if err := repo_model.WatchRepo(ctx, doer.ID, repo.ID, true); err != nil { + if err := repo_model.WatchRepoExplicitly(ctx, doer.ID, repo.ID, repo_model.WatchAllSelection); err != nil { return fmt.Errorf("watchRepo: %w", err) } // Remove watch for organization. if oldOwner.IsOrganization() { - if err := repo_model.WatchRepo(ctx, oldOwner.ID, repo.ID, false); err != nil { + if err := repo_model.WatchRepoExplicitly(ctx, oldOwner.ID, repo.ID, repo_model.WatchNoneSelection); err != nil { return fmt.Errorf("watchRepo [false]: %w", err) } } diff --git a/services/uinotification/notify.go b/services/uinotification/notify.go index 25048e7b53..a156db7efa 100644 --- a/services/uinotification/notify.go +++ b/services/uinotification/notify.go @@ -139,7 +139,7 @@ func (ns *notificationService) NewPullRequest(ctx context.Context, pr *issues_mo return } toNotify := make(container.Set[int64], 32) - repoWatchers, err := repo_model.GetRepoWatchersIDs(ctx, pr.Issue.RepoID) + repoWatchers, err := repo_model.GetSelectWatcherIDs(ctx, pr.Issue.RepoID, repo_model.WatchSelection{Issues: false, PullRequests: true, Releases: false}) if err != nil { log.Error("GetRepoWatchersIDs: %v", err) return diff --git a/services/user/block_test.go b/services/user/block_test.go index 893180a43b..90039f57ec 100644 --- a/services/user/block_test.go +++ b/services/user/block_test.go @@ -46,7 +46,7 @@ func TestBlockUser(t *testing.T) { // Blocked user watch repository of doer. repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: doer.ID}) - require.NoError(t, repo_model.WatchRepo(db.DefaultContext, blockedUser.ID, repo.ID, true)) + require.NoError(t, repo_model.WatchRepoExplicitly(db.DefaultContext, blockedUser.ID, repo.ID, repo_model.WatchAllSelection)) repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: doer.ID}) oldNumWatchers := repo.NumWatches @@ -54,7 +54,7 @@ func TestBlockUser(t *testing.T) { require.NoError(t, BlockUser(db.DefaultContext, doer.ID, blockedUser.ID)) // Ensure blocked user isn't following doer's repository. - assert.False(t, repo_model.IsWatching(db.DefaultContext, blockedUser.ID, repo.ID)) + assert.False(t, repo_model.IsWatcher(db.DefaultContext, blockedUser.ID, repo.ID)) // Ensure the watcher count was reduced by one. repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: doer.ID}) diff --git a/services/user/delete.go b/services/user/delete.go index fe3b6276d9..3296d68f60 100644 --- a/services/user/delete.go +++ b/services/user/delete.go @@ -30,14 +30,14 @@ import ( "xorm.io/builder" ) -// deleteUser deletes models associated to an user. +// deleteUser deletes models associated to a user. func deleteUser(ctx context.Context, u *user_model.User, purge bool) (err error) { e := db.GetEngine(ctx) // ***** START: Watch ***** watchedRepoIDs, err := db.FindIDs(ctx, "watch", "watch.repo_id", builder.Eq{"watch.user_id": u.ID}. - And(builder.Neq{"watch.mode": repo_model.WatchModeDont})) + And(repo_model.BuilderWatchAnything())) if err != nil { return fmt.Errorf("get all watches: %w", err) } diff --git a/templates/repo/watch_unwatch.tmpl b/templates/repo/watch_unwatch.tmpl index e6ddf88f0f..25226180a8 100644 --- a/templates/repo/watch_unwatch.tmpl +++ b/templates/repo/watch_unwatch.tmpl @@ -1,16 +1,58 @@ -
-
- - + +
+ {{svg "octicon-eye" 16}} + {{if $.RepoWatchSelection.IsWatching}}{{ctx.Locale.Tr "repo.watching"}}{{else}}{{ctx.Locale.Tr "repo.watch"}}{{end}} + {{svg "octicon-triangle-down" 14}} +
+ + {{CountFmt .Repository.NumWatches}} + + +
+ +
+ + + +
+ + {{if $.RepoWatchSelection.IsWatching}} + + {{end}} +
+
+ +
+ +{{else}} + {{/* not logged-in user */}} +
+
+ {{svg "octicon-eye"}}{{ctx.Locale.Tr "repo.watch"}} +
+ {{CountFmt .Repository.NumWatches}}
- +{{end}} diff --git a/tests/e2e/issue-sidebar.test.e2e.ts b/tests/e2e/issue-sidebar.test.e2e.ts index 71b1371c31..6a573f966a 100644 --- a/tests/e2e/issue-sidebar.test.e2e.ts +++ b/tests/e2e/issue-sidebar.test.e2e.ts @@ -44,7 +44,7 @@ test.describe('Pull: Toggle WIP', () => { await page.locator('#issue-title-edit-show').click(); await page.locator('#issue-title-editor input').fill(title); const loadPromise = page.waitForEvent('load'); - await page.getByText('Save').click(); + await page.locator('#issue-title-editor').getByText('Save').click(); await loadPromise; } diff --git a/tests/e2e/repo-home.test.e2e.ts b/tests/e2e/repo-home.test.e2e.ts index 2927d07af3..0e7805ad3c 100644 --- a/tests/e2e/repo-home.test.e2e.ts +++ b/tests/e2e/repo-home.test.e2e.ts @@ -81,13 +81,38 @@ test('Watch/unwatch button URL retention', async ({page}) => { const response = await page.goto('/user2/repo1'); expect(response?.status()).toBe(200); - const button = page.locator('button[aria-label="Watch"], button[aria-label="Unwatch"]'); + const watchDropdown = page.locator('details.dropdown#watch-button'); + const watchSummary = watchDropdown.locator('summary'); + const watchMenu = watchDropdown.locator('.content'); - await button.click(); + // Open the dropdown + await watchSummary.click(); + await expect(watchMenu).toBeVisible(); + + // Select PRs + const prsCheckbox = watchMenu.locator('input[name="watch_pull_requests"]'); + await prsCheckbox.click(); + + // Save + const saveButton = watchMenu.locator('button:has-text("Save")'); + await saveButton.isVisible(); + await saveButton.click(); + + // Wait for menu to disappear + await expect(watchMenu).toBeHidden(); expect(page.url()).not.toContain('/watch'); expect(page.url()).not.toContain('/unwatch'); - await button.click(); + // Open the dropdown once more + await watchSummary.click(); + await expect(watchMenu).toBeVisible(); + + // Unwatch + const unwatchButton = watchMenu.locator('button:has-text("Unwatch")'); + await unwatchButton.click(); + + // Wait for menu to disappear + await expect(watchMenu).toBeHidden(); expect(page.url()).not.toContain('/watch'); expect(page.url()).not.toContain('/unwatch'); }); diff --git a/tests/e2e/repo-watch.test.e2e.ts b/tests/e2e/repo-watch.test.e2e.ts new file mode 100644 index 0000000000..e2166cc976 --- /dev/null +++ b/tests/e2e/repo-watch.test.e2e.ts @@ -0,0 +1,58 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +// @watch start +// templates/repo/watch_unwatch.tmpl +// templates/user/settings/notifications.tmpl +// web_src/css/repo/header.css +// @watch end + +import {expect} from '@playwright/test'; +import {test} from './utils_e2e.ts'; + +test.use({user: 'user2'}); + +test('Watch dropdown: toggle watch events', async ({page}, _) => { + await page.goto('/user2/repo1'); + + // Find the watch dropdown + const watchDropdown = page.locator('details.dropdown#watch-button'); + const watchSummary = watchDropdown.locator('summary'); + const watchMenu = watchDropdown.locator('.content'); + + // Open the dropdown + await watchSummary.click(); + await expect(watchMenu).toBeVisible(); + + // Verify checkboxes are present + const issuesCheckbox = watchMenu.locator('input[name="watch_issues"]'); + const prsCheckbox = watchMenu.locator('input[name="watch_pull_requests"]'); + const releasesCheckbox = watchMenu.locator('input[name="watch_releases"]'); + + await expect(issuesCheckbox).toBeVisible(); + await expect(prsCheckbox).toBeVisible(); + await expect(releasesCheckbox).toBeVisible(); + + // Close dropdown by clicking elsewhere + await page.locator('h1').click(); + await expect(watchMenu).toBeHidden(); +}); + +test('Watch dropdown: unwatch all shows proper state', async ({page}, _) => { + await page.goto('/user2/repo1'); + + const watchDropdown = page.locator('details.dropdown#watch-button'); + const watchSummary = watchDropdown.locator('summary > div'); + const watchMenu = watchDropdown.locator('.content'); + + // Open dropdown and click unwatch + await watchSummary.click(); + await expect(watchMenu).toBeVisible(); + + const unwatchButton = watchMenu.locator('button:has-text("Unwatch")'); + if (await unwatchButton.isVisible()) { + await unwatchButton.click(); + // After unwatch, the button should show "Watch" state + await expect(watchSummary).toContainText('Watch'); + } +}); diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index 91394ed710..23f76da498 100644 --- a/tests/integration/issue_test.go +++ b/tests/integration/issue_test.go @@ -374,7 +374,7 @@ func testNewIssue(t *testing.T, session *TestSession, user, repo, title, content resp := session.MakeRequest(t, req, http.StatusOK) htmlDoc := NewHTMLParser(t, resp.Body) - link, exists := htmlDoc.doc.Find("form.ui.form").Attr("action") + link, exists := htmlDoc.doc.Find("#new-issue").Attr("action") assert.True(t, exists, "The template has changed") req = NewRequestWithValues(t, "POST", link, map[string]string{ "title": title, diff --git a/tests/integration/pull_create_test.go b/tests/integration/pull_create_test.go index b63cdc2cd6..b49d5220c1 100644 --- a/tests/integration/pull_create_test.go +++ b/tests/integration/pull_create_test.go @@ -65,7 +65,7 @@ func testPullCreate(t *testing.T, session *TestSession, user, repo string, toSel // Submit the form for creating the pull htmlDoc = NewHTMLParser(t, resp.Body) - link, exists = htmlDoc.doc.Find("form.ui.form").Attr("action") + link, exists = htmlDoc.doc.Find("#new-issue").Attr("action") assert.True(t, exists, "The template has changed") req = NewRequestWithValues(t, "POST", link, map[string]string{ "title": title, @@ -88,7 +88,7 @@ func testPullCreateDirectly(t *testing.T, session *TestSession, baseRepoOwner, b // Submit the form for creating the pull htmlDoc := NewHTMLParser(t, resp.Body) - link, exists := htmlDoc.doc.Find("form.ui.form").Attr("action") + link, exists := htmlDoc.doc.Find("#new-issue").Attr("action") assert.True(t, exists, "The template has changed") req = NewRequestWithValues(t, "POST", link, map[string]string{ "title": title, diff --git a/tests/integration/release_test.go b/tests/integration/release_test.go index c253df125c..ed585e41bb 100644 --- a/tests/integration/release_test.go +++ b/tests/integration/release_test.go @@ -45,7 +45,7 @@ func createNewReleaseTarget(t *testing.T, session *TestSession, repoURL, tag, ti page.AssertElement(t, `form a.danger.button[data-modal-id="delete-release"]`, false) page.AssertElement(t, `form a.button[href$="/releases"]`, false) // Cancel - link, exists := page.Find("form.ui.form").Attr("action") + link, exists := page.Find("form[action$='/releases/new']").Attr("action") assert.True(t, exists, "The template has changed") postData := map[string]string{ diff --git a/tests/integration/repo_migrate_test.go b/tests/integration/repo_migrate_test.go index accd00c4eb..4a58b6c9b6 100644 --- a/tests/integration/repo_migrate_test.go +++ b/tests/integration/repo_migrate_test.go @@ -23,13 +23,10 @@ func testRepoMigrate(t testing.TB, session *TestSession, cloneAddr, repoName str resp := session.MakeRequest(t, req, http.StatusOK) htmlDoc := NewHTMLParser(t, resp.Body) - link, exists := htmlDoc.doc.Find("form.ui.form").Attr("action") - assert.True(t, exists, "The template has changed") - uid, exists := htmlDoc.doc.Find("#uid").Attr("value") assert.True(t, exists, "The template has changed") - req = NewRequestWithValues(t, "POST", link, map[string]string{ + req = NewRequestWithValues(t, "POST", "/repo/migrate", map[string]string{ "clone_addr": cloneAddr, "uid": uid, "repo_name": repoName, diff --git a/tests/integration/repo_starwatch_test.go b/tests/integration/repo_starwatch_test.go index e4b27b8767..5ef1f3fb23 100644 --- a/tests/integration/repo_starwatch_test.go +++ b/tests/integration/repo_starwatch_test.go @@ -4,7 +4,6 @@ package integration import ( - "fmt" "net/http" "strings" "testing" @@ -17,88 +16,132 @@ import ( "github.com/stretchr/testify/assert" ) -func testRepoStarringOrWatching(t *testing.T, action, listURI string, expectEmpty bool) { +func TestRepoStarUnstarUI(t *testing.T) { t.Helper() defer tests.PrepareTestEnv(t)() - oppositeAction := "un" + action session := loginUser(t, "user5") - // Star/Watch the repo as user5 - req := NewRequest(t, "POST", fmt.Sprintf("/user2/repo1/action/%s", action)) + // Star the repo as user5 + req := NewRequest(t, "POST", "/user2/repo1/action/star") session.MakeRequest(t, req, http.StatusOK) // Load the repo home as user5 req = NewRequest(t, "GET", "/user2/repo1") resp := session.MakeRequest(t, req, http.StatusOK) - // Verify that the star/watch button is now the opposite + // Verify that the star button is now unstar htmlDoc := NewHTMLParser(t, resp.Body) - actionButton := htmlDoc.Find(fmt.Sprintf("form[action='/user2/repo1/action/%s']", oppositeAction)) + actionButton := htmlDoc.Find("form[action='/user2/repo1/action/unstar']") assert.Equal(t, 1, actionButton.Length()) text := strings.ToLower(actionButton.Find("button span.text").Text()) - assert.Equal(t, oppositeAction, text) + assert.Equal(t, "unstar", text) - listLink := htmlDoc.Find(fmt.Sprintf("a[href$='/%s']", listURI)) + listLink := htmlDoc.Find("a[href$='/stars']") ariaLabel, _ := listLink.Attr("aria-label") - if listURI == "stars" { - assert.Equal(t, "1 star", ariaLabel) - } else { - assert.Equal(t, "5 watchers", ariaLabel) - } + assert.Equal(t, "1 star", ariaLabel) - // Load stargazers/watchers as user5 - req = NewRequestf(t, "GET", "/user2/repo1/%s", listURI) + // Load stargazers as user5 + req = NewRequestf(t, "GET", "/user2/repo1/stars") resp = session.MakeRequest(t, req, http.StatusOK) - // Verify that "user5" is among the stargazers/watchers + // Verify that "user5" is among the stargazers htmlDoc = NewHTMLParser(t, resp.Body) htmlDoc.AssertElement(t, ".user-cards .list .card > a[href='/user5']", true) - if expectEmpty { - // Verify which user-cards elements are present - htmlDoc.AssertElement(t, ".user-cards > .list", true) - htmlDoc.AssertElement(t, ".user-cards > div", false) - } + // Verify which user-cards elements are present + htmlDoc.AssertElement(t, ".user-cards > .list", true) + htmlDoc.AssertElement(t, ".user-cards > div", false) - // Unstar/unwatch the repo as user5 - req = NewRequest(t, "POST", fmt.Sprintf("/user2/repo1/action/%s", oppositeAction)) + // Unstar the repo as user5 + req = NewRequest(t, "POST", "/user2/repo1/action/unstar") session.MakeRequest(t, req, http.StatusOK) // Load the repo home as user5 req = NewRequest(t, "GET", "/user2/repo1") resp = session.MakeRequest(t, req, http.StatusOK) - // Verify that the star/watch button is now back to its default + // Verify that the star button is now back to star htmlDoc = NewHTMLParser(t, resp.Body) - actionButton = htmlDoc.Find(fmt.Sprintf("form[action='/user2/repo1/action/%s']", action)) + actionButton = htmlDoc.Find("form[action='/user2/repo1/action/star']") assert.Equal(t, 1, actionButton.Length()) text = strings.ToLower(actionButton.Find("button span.text").Text()) - assert.Equal(t, action, text) + assert.Equal(t, "star", text) - // Load stargazers/watchers as user5 - req = NewRequestf(t, "GET", "/user2/repo1/%s", listURI) + // Load stargazers as user5 + req = NewRequestf(t, "GET", "/user2/repo1/stars") resp = session.MakeRequest(t, req, http.StatusOK) - // Verify that "user5" is not among the stargazers/watchers + // Verify that "user5" is not among the stargazers htmlDoc = NewHTMLParser(t, resp.Body) htmlDoc.AssertElement(t, ".user-cards .list .item.ui.segment > a[href='/user2']", false) - if expectEmpty { - // Verify which user-cards elements are present - htmlDoc.AssertElement(t, ".user-cards > .list", false) - htmlDoc.AssertElement(t, ".user-cards > div", true) - } -} - -func TestRepoStarUnstarUI(t *testing.T) { - testRepoStarringOrWatching(t, "star", "stars", true) + // Verify which user-cards elements are present + htmlDoc.AssertElement(t, ".user-cards > .list", false) + htmlDoc.AssertElement(t, ".user-cards > div", true) } func TestRepoWatchUnwatchUI(t *testing.T) { - testRepoStarringOrWatching(t, "watch", "watchers", false) - // Empty list state is not checked because repo is watched by many users + t.Helper() + + defer tests.PrepareTestEnv(t)() + + session := loginUser(t, "user5") + + // Watch the repo as user5 (using watch/settings endpoint with all events) + req := NewRequestWithValues(t, "POST", "/user2/repo1/action/watch/select", map[string]string{ + "watch_issues": "true", + "watch_pull_requests": "true", + "watch_releases": "true", + }) + session.MakeRequest(t, req, http.StatusOK) + + // Load the repo home as user5 + req = NewRequest(t, "GET", "/user2/repo1") + resp := session.MakeRequest(t, req, http.StatusOK) + + // Verify that the watch dropdown shows "Watching" + htmlDoc := NewHTMLParser(t, resp.Body) + watchButton := htmlDoc.Find("details.dropdown#watch-button summary span.text") + assert.Equal(t, 1, watchButton.Length()) + text := strings.TrimSpace(watchButton.Text()) + assert.Equal(t, "Watching", text) + + listLink := htmlDoc.Find("a[href$='/watchers']") + ariaLabel, _ := listLink.Attr("aria-label") + assert.Equal(t, "5 watchers", ariaLabel) + + // Load watchers as user5 + req = NewRequestf(t, "GET", "/user2/repo1/watchers") + resp = session.MakeRequest(t, req, http.StatusOK) + + // Verify that "user5" is among the watchers + htmlDoc = NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, ".user-cards .list .card > a[href='/user5']", true) + + // Unwatch the repo as user5 + req = NewRequest(t, "POST", "/user2/repo1/action/unwatch") + session.MakeRequest(t, req, http.StatusOK) + + // Load the repo home as user5 + req = NewRequest(t, "GET", "/user2/repo1") + resp = session.MakeRequest(t, req, http.StatusOK) + + // Verify that the watch dropdown shows "Watch" + htmlDoc = NewHTMLParser(t, resp.Body) + watchButton = htmlDoc.Find("details.dropdown#watch-button summary span.text") + assert.Equal(t, 1, watchButton.Length()) + text = strings.TrimSpace(watchButton.Text()) + assert.Equal(t, "Watch", text) + + // Load watchers as user5 + req = NewRequestf(t, "GET", "/user2/repo1/watchers") + resp = session.MakeRequest(t, req, http.StatusOK) + + // Verify that "user5" is not among the watchers + htmlDoc = NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, ".user-cards .list .item.ui.segment > a[href='/user2']", false) } func TestDisabledStars(t *testing.T) { diff --git a/tests/integration/repo_watch_test.go b/tests/integration/repo_watch_test.go index 8d6af5b381..a8ea03db0d 100644 --- a/tests/integration/repo_watch_test.go +++ b/tests/integration/repo_watch_test.go @@ -10,6 +10,8 @@ import ( repo_model "forgejo.org/models/repo" "forgejo.org/models/unittest" "forgejo.org/modules/setting" + + "github.com/stretchr/testify/assert" ) func TestRepoWatch(t *testing.T) { @@ -19,6 +21,6 @@ func TestRepoWatch(t *testing.T) { session := loginUser(t, "user2") unittest.AssertNotExistsBean(t, &repo_model.Watch{UserID: 2, RepoID: 3}) testEditFile(t, session, "org3", "repo3", "master", "README.md", "Hello, World (Edited for watch)\n") - unittest.AssertExistsAndLoadBean(t, &repo_model.Watch{UserID: 2, RepoID: 3, Mode: repo_model.WatchModeAuto}) + assert.Equal(t, 1, unittest.GetCount(t, &repo_model.Watch{UserID: 2, RepoID: 3}, "source = true")) }) } diff --git a/web_src/css/repo/header.css b/web_src/css/repo/header.css index 9da5fe6acd..401756a54e 100644 --- a/web_src/css/repo/header.css +++ b/web_src/css/repo/header.css @@ -66,3 +66,8 @@ .repo-buttons .ui.labeled.button.disabled > .button { pointer-events: none !important; } + +/* ToDo: convert all .repo-buttons to dynamic-height buttons */ +.repo-buttons details.dropdown > summary.button { + --button-min-height: 33.133px; +} diff --git a/web_src/js/modules/dropdown.ts b/web_src/js/modules/dropdown.ts index 393e7150b1..f63c8d4257 100644 --- a/web_src/js/modules/dropdown.ts +++ b/web_src/js/modules/dropdown.ts @@ -57,7 +57,8 @@ export function initDropdowns() { // User pressed ArrowDown on a focused summary of a closed dropdown. // We'll open the dropdown and focus it's first item parentDropdown.setAttribute('open', 'true'); - parentDropdown.querySelector('.content > ul > li > :is(a, button)').focus(); + const firstFocusable = parentDropdown.querySelector('.content > ul > li :is(a, button, input)'); + firstFocusable?.focus(); event.preventDefault(); return; } @@ -99,7 +100,7 @@ export function initDropdowns() { dropdown.removeAttribute('open'); return; } - dropdownItems[activeLiIndex - 1].querySelector(':is(a, button)').focus(); + dropdownItems[activeLiIndex - 1].querySelector(':is(a, button, input)')?.focus(); } if (event.key === 'ArrowDown') { @@ -108,7 +109,7 @@ export function initDropdowns() { // First child is already selected return; } - dropdownItems[activeLiIndex + 1].querySelector(':is(a, button)').focus(); + dropdownItems[activeLiIndex + 1].querySelector(':is(a, button, input)')?.focus(); } }); }