From 01a4a7cb14b3a48f9e8115d5bc93af7ae17f1275 Mon Sep 17 00:00:00 2001 From: guillep2k <18600385+guillep2k@users.noreply.github.com> Date: Sun, 10 Nov 2019 06:22:19 -0300 Subject: [PATCH] Auto-subscribe user to repository when they commit/tag to it (#7657) * Add support for AUTO_WATCH_ON_CHANGES and AUTO_WATCH_ON_CLONE * Update models/repo_watch.go Co-Authored-By: Lauris BH * Round up changes suggested by lafriks * Added changes suggested from automated tests * Updated deleteUser to take RepoWatchModeDont into account, corrected inverted DefaultWatchOnClone and DefaultWatchOnChanges behaviour, updated and added tests. * Reinsert import "github.com/Unknwon/com" on http.go * Add migration for new column `watch`.`mode` * Remove serv code * Remove WATCH_ON_CLONE; use hooks, add integrations * Renamed watch_test.go to repo_watch_test.go * Correct fmt * Add missing EOL * Correct name of test function * Reword cheat and ini descriptions * Add update to migration to ensure column value * Clarify comment Co-Authored-By: zeripath * Simplify if condition --- custom/conf/app.ini.sample | 3 + .../doc/advanced/config-cheat-sheet.en-us.md | 1 + integrations/repo_watch_test.go | 24 +++ models/consistency.go | 7 +- models/fixtures/repository.yml | 2 +- models/fixtures/watch.yml | 15 ++ models/migrations/migrations.go | 2 + models/migrations/v106.go | 26 ++++ models/repo.go | 4 +- models/repo_watch.go | 141 +++++++++++++++--- models/repo_watch_test.go | 90 ++++++++++- models/user.go | 3 +- modules/repofiles/update.go | 5 + modules/setting/service.go | 2 + 14 files changed, 296 insertions(+), 29 deletions(-) create mode 100644 integrations/repo_watch_test.go create mode 100644 models/migrations/v106.go diff --git a/custom/conf/app.ini.sample b/custom/conf/app.ini.sample index 17fcc0de2..5e26171d9 100644 --- a/custom/conf/app.ini.sample +++ b/custom/conf/app.ini.sample @@ -501,6 +501,9 @@ SHOW_REGISTRATION_BUTTON = true ; When adding a repo to a team or creating a new repo all team members will watch the ; repo automatically if enabled AUTO_WATCH_NEW_REPOS = true +; Default value for AutoWatchOnChanges +; Make the user watch a repository When they commit for the first time +AUTO_WATCH_ON_CHANGES = false [webhook] ; Hook task queue length, increase if webhook shooting starts hanging diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 96b529c0b..e5236205f 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -303,6 +303,7 @@ relation to port exhaustion. on this instance. - `SHOW_REGISTRATION_BUTTON`: **! DISABLE\_REGISTRATION**: Show Registration Button - `AUTO_WATCH_NEW_REPOS`: **true**: Enable this to let all organisation users watch new repos when they are created +- `AUTO_WATCH_ON_CHANGES`: **false**: Enable this to make users watch a repository after their first commit to it - `DEFAULT_ORG_VISIBILITY`: **public**: Set default visibility mode for organisations, either "public", "limited" or "private". - `DEFAULT_ORG_MEMBER_VISIBLE`: **false** True will make the membership of the users visible when added to the organisation. diff --git a/integrations/repo_watch_test.go b/integrations/repo_watch_test.go new file mode 100644 index 000000000..d96b014a7 --- /dev/null +++ b/integrations/repo_watch_test.go @@ -0,0 +1,24 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package integrations + +import ( + "net/url" + "testing" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/setting" +) + +func TestRepoWatch(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + // Test round-trip auto-watch + setting.Service.AutoWatchOnChanges = true + session := loginUser(t, "user2") + models.AssertNotExistsBean(t, &models.Watch{UserID: 2, RepoID: 3}) + testEditFile(t, session, "user3", "repo3", "master", "README.md", "Hello, World (Edited for watch)\n") + models.AssertExistsAndLoadBean(t, &models.Watch{UserID: 2, RepoID: 3, Mode: models.RepoWatchModeAuto}) + }) +} diff --git a/models/consistency.go b/models/consistency.go index f9fa3028f..62d1d2e87 100644 --- a/models/consistency.go +++ b/models/consistency.go @@ -84,14 +84,17 @@ func (user *User) checkForConsistency(t *testing.T) { func (repo *Repository) checkForConsistency(t *testing.T) { assert.Equal(t, repo.LowerName, strings.ToLower(repo.Name), "repo: %+v", repo) assertCount(t, &Star{RepoID: repo.ID}, repo.NumStars) - assertCount(t, &Watch{RepoID: repo.ID}, repo.NumWatches) assertCount(t, &Milestone{RepoID: repo.ID}, repo.NumMilestones) assertCount(t, &Repository{ForkID: repo.ID}, repo.NumForks) if repo.IsFork { AssertExistsAndLoadBean(t, &Repository{ID: repo.ForkID}) } - actual := getCount(t, x.Where("is_pull=?", false), &Issue{RepoID: repo.ID}) + actual := getCount(t, x.Where("Mode<>?", RepoWatchModeDont), &Watch{RepoID: repo.ID}) + assert.EqualValues(t, repo.NumWatches, actual, + "Unexpected number of watches for repo %+v", repo) + + actual = getCount(t, x.Where("is_pull=?", false), &Issue{RepoID: repo.ID}) assert.EqualValues(t, repo.NumIssues, actual, "Unexpected number of issues for repo %+v", repo) diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index cf7d24c6c..32903723e 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -10,7 +10,7 @@ num_closed_pulls: 0 num_milestones: 3 num_closed_milestones: 1 - num_watches: 3 + num_watches: 4 status: 0 - diff --git a/models/fixtures/watch.yml b/models/fixtures/watch.yml index 5cd3b55fc..c29f6bb65 100644 --- a/models/fixtures/watch.yml +++ b/models/fixtures/watch.yml @@ -2,13 +2,28 @@ 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 diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 5ed70dc4f..71ffe2edb 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -266,6 +266,8 @@ var migrations = []Migration{ NewMigration("remove unnecessary columns from label", removeLabelUneededCols), // v105 -> v106 NewMigration("add includes_all_repositories to teams", addTeamIncludesAllRepositories), + // v106 -> v107 + NewMigration("add column `mode` to table watch", addModeColumnToWatch), } // Migrate database to current version diff --git a/models/migrations/v106.go b/models/migrations/v106.go new file mode 100644 index 000000000..201fc1026 --- /dev/null +++ b/models/migrations/v106.go @@ -0,0 +1,26 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "xorm.io/xorm" +) + +// RepoWatchMode specifies what kind of watch the user has on a repository +type RepoWatchMode int8 + +// Watch is connection request for receiving repository notification. +type Watch struct { + ID int64 `xorm:"pk autoincr"` + Mode RepoWatchMode `xorm:"SMALLINT NOT NULL DEFAULT 1"` +} + +func addModeColumnToWatch(x *xorm.Engine) (err error) { + if err = x.Sync2(new(Watch)); err != nil { + return + } + _, err = x.Exec("UPDATE `watch` SET `mode` = 1") + return err +} diff --git a/models/repo.go b/models/repo.go index 812460e92..90918025f 100644 --- a/models/repo.go +++ b/models/repo.go @@ -2410,8 +2410,8 @@ func CheckRepoStats() { checkers := []*repoChecker{ // Repository.NumWatches { - "SELECT repo.id FROM `repository` repo WHERE repo.num_watches!=(SELECT COUNT(*) FROM `watch` WHERE repo_id=repo.id)", - "UPDATE `repository` SET num_watches=(SELECT COUNT(*) FROM `watch` WHERE repo_id=?) WHERE id=?", + "SELECT repo.id FROM `repository` repo WHERE repo.num_watches!=(SELECT COUNT(*) FROM `watch` WHERE repo_id=repo.id AND mode<>2)", + "UPDATE `repository` SET num_watches=(SELECT COUNT(*) FROM `watch` WHERE repo_id=? AND mode<>2) WHERE id=?", "repository count 'num_watches'", }, // Repository.NumStars diff --git a/models/repo_watch.go b/models/repo_watch.go index 53a34efda..cb864fb46 100644 --- a/models/repo_watch.go +++ b/models/repo_watch.go @@ -4,42 +4,118 @@ package models -import "fmt" +import ( + "fmt" + + "code.gitea.io/gitea/modules/setting" +) + +// RepoWatchMode specifies what kind of watch the user has on a repository +type RepoWatchMode int8 + +const ( + // RepoWatchModeNone don't watch + RepoWatchModeNone RepoWatchMode = iota // 0 + // RepoWatchModeNormal watch repository (from other sources) + RepoWatchModeNormal // 1 + // RepoWatchModeDont explicit don't auto-watch + RepoWatchModeDont // 2 + // RepoWatchModeAuto watch repository (from AutoWatchOnChanges) + RepoWatchModeAuto // 3 +) // 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)"` + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"UNIQUE(watch)"` + RepoID int64 `xorm:"UNIQUE(watch)"` + Mode RepoWatchMode `xorm:"SMALLINT NOT NULL DEFAULT 1"` } -func isWatching(e Engine, userID, repoID int64) bool { - has, _ := e.Get(&Watch{UserID: userID, RepoID: repoID}) - return has +// getWatch gets what kind of subscription a user has on a given repository; returns dummy record if none found +func getWatch(e Engine, userID, repoID int64) (Watch, error) { + watch := Watch{UserID: userID, RepoID: repoID} + has, err := e.Get(&watch) + if err != nil { + return watch, err + } + if !has { + watch.Mode = RepoWatchModeNone + } + return watch, nil +} + +// Decodes watchability of RepoWatchMode +func isWatchMode(mode RepoWatchMode) bool { + return mode != RepoWatchModeNone && mode != RepoWatchModeDont } // IsWatching checks if user has watched given repository. func IsWatching(userID, repoID int64) bool { - return isWatching(x, userID, repoID) + watch, err := getWatch(x, userID, repoID) + return err == nil && isWatchMode(watch.Mode) } -func watchRepo(e Engine, userID, repoID int64, watch bool) (err error) { - if watch { - if isWatching(e, userID, repoID) { - return nil - } - if _, err = e.Insert(&Watch{RepoID: repoID, UserID: userID}); err != nil { +func watchRepoMode(e Engine, watch Watch, mode RepoWatchMode) (err error) { + if watch.Mode == mode { + return nil + } + if mode == RepoWatchModeAuto && (watch.Mode == RepoWatchModeDont || isWatchMode(watch.Mode)) { + // Don't auto watch if already watching or deliberately not watching + return nil + } + + hadrec := watch.Mode != RepoWatchModeNone + needsrec := mode != RepoWatchModeNone + repodiff := 0 + + if isWatchMode(mode) && !isWatchMode(watch.Mode) { + repodiff = 1 + } else if !isWatchMode(mode) && isWatchMode(watch.Mode) { + repodiff = -1 + } + + watch.Mode = mode + + if !hadrec && needsrec { + watch.Mode = mode + if _, err = e.Insert(watch); err != nil { return err } - _, err = e.Exec("UPDATE `repository` SET num_watches = num_watches + 1 WHERE id = ?", repoID) - } else { - if !isWatching(e, userID, repoID) { - return nil - } - if _, err = e.Delete(&Watch{0, userID, repoID}); err != nil { + } else if needsrec { + watch.Mode = mode + if _, err := e.ID(watch.ID).AllCols().Update(watch); err != nil { return err } - _, err = e.Exec("UPDATE `repository` SET num_watches = num_watches - 1 WHERE id = ?", repoID) + } else if _, err = e.Delete(Watch{ID: watch.ID}); err != nil { + return err + } + if repodiff != 0 { + _, err = e.Exec("UPDATE `repository` SET num_watches = num_watches + ? WHERE id = ?", repodiff, watch.RepoID) + } + return err +} + +// WatchRepoMode watch repository in specific mode. +func WatchRepoMode(userID, repoID int64, mode RepoWatchMode) (err error) { + var watch Watch + if watch, err = getWatch(x, userID, repoID); err != nil { + return err + } + return watchRepoMode(x, watch, mode) +} + +func watchRepo(e Engine, userID, repoID int64, doWatch bool) (err error) { + var watch Watch + if watch, err = getWatch(e, userID, repoID); err != nil { + return err + } + if !doWatch && watch.Mode == RepoWatchModeAuto { + err = watchRepoMode(e, watch, RepoWatchModeDont) + } else if !doWatch { + err = watchRepoMode(e, watch, RepoWatchModeNone) + } else { + err = watchRepoMode(e, watch, RepoWatchModeNormal) } return err } @@ -52,6 +128,7 @@ func WatchRepo(userID, repoID int64, watch bool) (err error) { func getWatchers(e Engine, repoID int64) ([]*Watch, error) { watches := make([]*Watch, 0, 10) return watches, e.Where("`watch`.repo_id=?", repoID). + And("`watch`.mode<>?", RepoWatchModeDont). And("`user`.is_active=?", true). And("`user`.prohibit_login=?", false). Join("INNER", "`user`", "`user`.id = `watch`.user_id"). @@ -67,7 +144,8 @@ func GetWatchers(repoID int64) ([]*Watch, error) { func (repo *Repository) GetWatchers(page int) ([]*User, error) { users := make([]*User, 0, ItemsPerPage) sess := x.Where("watch.repo_id=?", repo.ID). - Join("LEFT", "watch", "`user`.id=`watch`.user_id") + Join("LEFT", "watch", "`user`.id=`watch`.user_id"). + And("`watch`.mode<>?", RepoWatchModeDont) if page > 0 { sess = sess.Limit(ItemsPerPage, (page-1)*ItemsPerPage) } @@ -137,3 +215,22 @@ func notifyWatchers(e Engine, act *Action) error { func NotifyWatchers(act *Action) error { return notifyWatchers(x, act) } + +func watchIfAuto(e Engine, userID, repoID int64, isWrite bool) error { + if !isWrite || !setting.Service.AutoWatchOnChanges { + return nil + } + watch, err := getWatch(e, userID, repoID) + if err != nil { + return err + } + if watch.Mode != RepoWatchModeNone { + return nil + } + return watchRepoMode(e, watch, RepoWatchModeAuto) +} + +// WatchIfAuto subscribes to repo if AutoWatchOnChanges is set +func WatchIfAuto(userID int64, repoID int64, isWrite bool) error { + return watchIfAuto(x, userID, repoID, isWrite) +} diff --git a/models/repo_watch_test.go b/models/repo_watch_test.go index 852f09f1c..c3d40ec91 100644 --- a/models/repo_watch_test.go +++ b/models/repo_watch_test.go @@ -7,6 +7,8 @@ package models import ( "testing" + "code.gitea.io/gitea/modules/setting" + "github.com/stretchr/testify/assert" ) @@ -15,8 +17,10 @@ func TestIsWatching(t *testing.T) { assert.True(t, IsWatching(1, 1)) assert.True(t, IsWatching(4, 1)) + assert.True(t, IsWatching(11, 1)) assert.False(t, IsWatching(1, 5)) + assert.False(t, IsWatching(8, 1)) assert.False(t, IsWatching(NonexistentID, NonexistentID)) } @@ -78,7 +82,7 @@ func TestNotifyWatchers(t *testing.T) { } assert.NoError(t, NotifyWatchers(action)) - // One watchers are inactive, thus action is only created for user 8, 1, 4 + // One watchers are inactive, thus action is only created for user 8, 1, 4, 11 AssertExistsAndLoadBean(t, &Action{ ActUserID: action.ActUserID, UserID: 8, @@ -97,4 +101,88 @@ func TestNotifyWatchers(t *testing.T) { RepoID: action.RepoID, OpType: action.OpType, }) + AssertExistsAndLoadBean(t, &Action{ + ActUserID: action.ActUserID, + UserID: 11, + RepoID: action.RepoID, + OpType: action.OpType, + }) +} + +func TestWatchIfAuto(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository) + watchers, err := repo.GetWatchers(1) + assert.NoError(t, err) + assert.Len(t, watchers, repo.NumWatches) + + setting.Service.AutoWatchOnChanges = false + + prevCount := repo.NumWatches + + // Must not add watch + assert.NoError(t, WatchIfAuto(8, 1, true)) + watchers, err = repo.GetWatchers(1) + assert.NoError(t, err) + assert.Len(t, watchers, prevCount) + + // Should not add watch + assert.NoError(t, WatchIfAuto(10, 1, true)) + watchers, err = repo.GetWatchers(1) + assert.NoError(t, err) + assert.Len(t, watchers, prevCount) + + setting.Service.AutoWatchOnChanges = true + + // Must not add watch + assert.NoError(t, WatchIfAuto(8, 1, true)) + watchers, err = repo.GetWatchers(1) + assert.NoError(t, err) + assert.Len(t, watchers, prevCount) + + // Should not add watch + assert.NoError(t, WatchIfAuto(12, 1, false)) + watchers, err = repo.GetWatchers(1) + assert.NoError(t, err) + assert.Len(t, watchers, prevCount) + + // Should add watch + assert.NoError(t, WatchIfAuto(12, 1, true)) + watchers, err = repo.GetWatchers(1) + assert.NoError(t, err) + assert.Len(t, watchers, prevCount+1) + + // Should remove watch, inhibit from adding auto + assert.NoError(t, WatchRepo(12, 1, false)) + watchers, err = repo.GetWatchers(1) + assert.NoError(t, err) + assert.Len(t, watchers, prevCount) + + // Must not add watch + assert.NoError(t, WatchIfAuto(12, 1, true)) + watchers, err = repo.GetWatchers(1) + assert.NoError(t, err) + assert.Len(t, watchers, prevCount) +} + +func TestWatchRepoMode(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + AssertCount(t, &Watch{UserID: 12, RepoID: 1}, 0) + + assert.NoError(t, WatchRepoMode(12, 1, RepoWatchModeAuto)) + AssertCount(t, &Watch{UserID: 12, RepoID: 1}, 1) + AssertCount(t, &Watch{UserID: 12, RepoID: 1, Mode: RepoWatchModeAuto}, 1) + + assert.NoError(t, WatchRepoMode(12, 1, RepoWatchModeNormal)) + AssertCount(t, &Watch{UserID: 12, RepoID: 1}, 1) + AssertCount(t, &Watch{UserID: 12, RepoID: 1, Mode: RepoWatchModeNormal}, 1) + + assert.NoError(t, WatchRepoMode(12, 1, RepoWatchModeDont)) + AssertCount(t, &Watch{UserID: 12, RepoID: 1}, 1) + AssertCount(t, &Watch{UserID: 12, RepoID: 1, Mode: RepoWatchModeDont}, 1) + + assert.NoError(t, WatchRepoMode(12, 1, RepoWatchModeNone)) + AssertCount(t, &Watch{UserID: 12, RepoID: 1}, 0) } diff --git a/models/user.go b/models/user.go index 7aa1e143e..4a8c644cc 100644 --- a/models/user.go +++ b/models/user.go @@ -1082,7 +1082,7 @@ func deleteUser(e *xorm.Session, u *User) error { // ***** START: Watch ***** watchedRepoIDs := make([]int64, 0, 10) if err = e.Table("watch").Cols("watch.repo_id"). - Where("watch.user_id = ?", u.ID).Find(&watchedRepoIDs); err != nil { + Where("watch.user_id = ?", u.ID).And("watch.mode <>?", RepoWatchModeDont).Find(&watchedRepoIDs); err != nil { return fmt.Errorf("get all watches: %v", err) } if _, err = e.Decr("num_watches").In("id", watchedRepoIDs).NoAutoTime().Update(new(Repository)); err != nil { @@ -1543,6 +1543,7 @@ func GetStarredRepos(userID int64, private bool) ([]*Repository, error) { // GetWatchedRepos returns the repos watched by a particular user func GetWatchedRepos(userID int64, private bool) ([]*Repository, error) { sess := x.Where("watch.user_id=?", userID). + And("`watch`.mode<>?", RepoWatchModeDont). Join("LEFT", "watch", "`repository`.id=`watch`.repo_id") if !private { sess = sess.And("is_private=?", false) diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 8e057700a..5479616c4 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -504,5 +504,10 @@ func PushUpdate(repo *models.Repository, branch string, opts models.PushUpdateOp if opts.RefFullName == git.BranchPrefix+repo.DefaultBranch { models.UpdateRepoIndexer(repo) } + + if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil { + log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err) + } + return nil } diff --git a/modules/setting/service.go b/modules/setting/service.go index 93629100a..6cbee8234 100644 --- a/modules/setting/service.go +++ b/modules/setting/service.go @@ -44,6 +44,7 @@ var Service struct { NoReplyAddress string EnableUserHeatmap bool AutoWatchNewRepos bool + AutoWatchOnChanges bool DefaultOrgMemberVisible bool // OpenID settings @@ -85,6 +86,7 @@ func newService() { Service.NoReplyAddress = sec.Key("NO_REPLY_ADDRESS").MustString("noreply.example.org") Service.EnableUserHeatmap = sec.Key("ENABLE_USER_HEATMAP").MustBool(true) Service.AutoWatchNewRepos = sec.Key("AUTO_WATCH_NEW_REPOS").MustBool(true) + Service.AutoWatchOnChanges = sec.Key("AUTO_WATCH_ON_CHANGES").MustBool(false) Service.DefaultOrgVisibility = sec.Key("DEFAULT_ORG_VISIBILITY").In("public", structs.ExtractKeysFromMapString(structs.VisibilityModes)) Service.DefaultOrgVisibilityMode = structs.VisibilityModes[Service.DefaultOrgVisibility] Service.DefaultOrgMemberVisible = sec.Key("DEFAULT_ORG_MEMBER_VISIBLE").MustBool()