From 0c332f0480e9aa72454641afe53aebb3b9ab6e57 Mon Sep 17 00:00:00 2001 From: Ethan Koenig Date: Thu, 25 May 2017 21:38:18 -0400 Subject: [PATCH] Fix activity feed (#1779) * Fix activity feed Preserve actions after user/repo name change * Add missing comment * Fix migration, and remove fields completely * Tests --- integrations/mysql.ini | 22 ++--- models/action.go | 144 ++++++++++++++++++-------------- models/action_test.go | 92 ++++++++++---------- models/consistency.go | 6 -- models/fixtures/action.yml | 9 -- models/issue.go | 15 ++-- models/issue_comment.go | 13 ++- models/migrations/migrations.go | 2 + models/migrations/v34.go | 44 ++++++++++ models/pull.go | 15 ++-- routers/user/home.go | 41 +++++++-- 11 files changed, 238 insertions(+), 165 deletions(-) create mode 100644 models/migrations/v34.go diff --git a/integrations/mysql.ini b/integrations/mysql.ini index 9560b4ff9..02a6ca2dd 100644 --- a/integrations/mysql.ini +++ b/integrations/mysql.ini @@ -6,7 +6,7 @@ DB_TYPE = mysql HOST = 127.0.0.1:3306 NAME = testgitea USER = root -PASSWD = +PASSWD = SSL_MODE = disable PATH = data/gitea.db @@ -26,14 +26,14 @@ OFFLINE_MODE = false ENABLED = false [service] -REGISTER_EMAIL_CONFIRM = false -ENABLE_NOTIFY_MAIL = false -DISABLE_REGISTRATION = false -ENABLE_CAPTCHA = false -REQUIRE_SIGNIN_VIEW = false -DEFAULT_KEEP_EMAIL_PRIVATE = false +REGISTER_EMAIL_CONFIRM = false +ENABLE_NOTIFY_MAIL = false +DISABLE_REGISTRATION = false +ENABLE_CAPTCHA = false +REQUIRE_SIGNIN_VIEW = false +DEFAULT_KEEP_EMAIL_PRIVATE = false DEFAULT_ALLOW_CREATE_ORGANIZATION = true -NO_REPLY_ADDRESS = noreply.example.org +NO_REPLY_ADDRESS = noreply.example.org [picture] DISABLE_GRAVATAR = false @@ -53,5 +53,7 @@ LEVEL = Warn LEVEL = Info [security] -INSTALL_LOCK = true -SECRET_KEY = 9pCviYTWSb +INSTALL_LOCK = true +SECRET_KEY = 9pCviYTWSb +INTERNAL_TOKEN = eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYmYiOjE0OTU1NTE2MTh9.hhSVGOANkaKk3vfCd2jDOIww4pUk0xtg9JRde5UogyQ + diff --git a/models/action.go b/models/action.go index 6f76cbc4e..4af81ce80 100644 --- a/models/action.go +++ b/models/action.go @@ -70,20 +70,18 @@ func init() { // repository. It implemented interface base.Actioner so that can be // used in template render. type Action struct { - ID int64 `xorm:"pk autoincr"` - UserID int64 `xorm:"INDEX"` // Receiver user id. - OpType ActionType - ActUserID int64 `xorm:"INDEX"` // Action user id. - ActUserName string // Action user name. - ActAvatar string `xorm:"-"` - RepoID int64 `xorm:"INDEX"` - RepoUserName string - RepoName string - RefName string - IsPrivate bool `xorm:"INDEX NOT NULL DEFAULT false"` - Content string `xorm:"TEXT"` - Created time.Time `xorm:"-"` - CreatedUnix int64 `xorm:"INDEX"` + ID int64 `xorm:"pk autoincr"` + UserID int64 `xorm:"INDEX"` // Receiver user id. + OpType ActionType + ActUserID int64 `xorm:"INDEX"` // Action user id. + ActUser *User `xorm:"-"` + RepoID int64 `xorm:"INDEX"` + Repo *Repository `xorm:"-"` + RefName string + IsPrivate bool `xorm:"INDEX NOT NULL DEFAULT false"` + Content string `xorm:"TEXT"` + Created time.Time `xorm:"-"` + CreatedUnix int64 `xorm:"INDEX"` } // BeforeInsert will be invoked by XORM before inserting a record @@ -106,42 +104,71 @@ func (a *Action) GetOpType() int { return int(a.OpType) } +func (a *Action) loadActUser() { + if a.ActUser != nil { + return + } + var err error + a.ActUser, err = GetUserByID(a.ActUserID) + if err == nil { + return + } else if IsErrUserNotExist(err) { + a.ActUser = NewGhostUser() + } else { + log.Error(4, "GetUserByID(%d): %v", a.ActUserID, err) + } +} + +func (a *Action) loadRepo() { + if a.ActUser != nil { + return + } + var err error + a.Repo, err = GetRepositoryByID(a.RepoID) + if err != nil { + log.Error(4, "GetRepositoryByID(%d): %v", a.RepoID, err) + } +} + // GetActUserName gets the action's user name. func (a *Action) GetActUserName() string { - return a.ActUserName + a.loadActUser() + return a.ActUser.Name } // ShortActUserName gets the action's user name trimmed to max 20 // chars. func (a *Action) ShortActUserName() string { - return base.EllipsisString(a.ActUserName, 20) + return base.EllipsisString(a.GetActUserName(), 20) } // GetRepoUserName returns the name of the action repository owner. func (a *Action) GetRepoUserName() string { - return a.RepoUserName + a.loadRepo() + return a.Repo.MustOwner().Name } // ShortRepoUserName returns the name of the action repository owner // trimmed to max 20 chars. func (a *Action) ShortRepoUserName() string { - return base.EllipsisString(a.RepoUserName, 20) + return base.EllipsisString(a.GetRepoUserName(), 20) } // GetRepoName returns the name of the action repository. func (a *Action) GetRepoName() string { - return a.RepoName + a.loadRepo() + return a.Repo.Name } // ShortRepoName returns the name of the action repository // trimmed to max 33 chars. func (a *Action) ShortRepoName() string { - return base.EllipsisString(a.RepoName, 33) + return base.EllipsisString(a.GetRepoName(), 33) } // GetRepoPath returns the virtual path to the action repository. func (a *Action) GetRepoPath() string { - return path.Join(a.RepoUserName, a.RepoName) + return path.Join(a.GetRepoUserName(), a.GetRepoName()) } // ShortRepoPath returns the virtual path to the action repository @@ -205,13 +232,12 @@ func (a *Action) GetIssueContent() string { func newRepoAction(e Engine, u *User, repo *Repository) (err error) { if err = notifyWatchers(e, &Action{ - ActUserID: u.ID, - ActUserName: u.Name, - OpType: ActionCreateRepo, - RepoID: repo.ID, - RepoUserName: repo.Owner.Name, - RepoName: repo.Name, - IsPrivate: repo.IsPrivate, + ActUserID: u.ID, + ActUser: u, + OpType: ActionCreateRepo, + RepoID: repo.ID, + Repo: repo, + IsPrivate: repo.IsPrivate, }); err != nil { return fmt.Errorf("notify watchers '%d/%d': %v", u.ID, repo.ID, err) } @@ -227,14 +253,13 @@ func NewRepoAction(u *User, repo *Repository) (err error) { func renameRepoAction(e Engine, actUser *User, oldRepoName string, repo *Repository) (err error) { if err = notifyWatchers(e, &Action{ - ActUserID: actUser.ID, - ActUserName: actUser.Name, - OpType: ActionRenameRepo, - RepoID: repo.ID, - RepoUserName: repo.Owner.Name, - RepoName: repo.Name, - IsPrivate: repo.IsPrivate, - Content: oldRepoName, + ActUserID: actUser.ID, + ActUser: actUser, + OpType: ActionRenameRepo, + RepoID: repo.ID, + Repo: repo, + IsPrivate: repo.IsPrivate, + Content: oldRepoName, }); err != nil { return fmt.Errorf("notify watchers: %v", err) } @@ -521,15 +546,14 @@ func CommitRepoAction(opts CommitRepoActionOptions) error { refName := git.RefEndName(opts.RefFullName) if err = NotifyWatchers(&Action{ - ActUserID: pusher.ID, - ActUserName: pusher.Name, - OpType: opType, - Content: string(data), - RepoID: repo.ID, - RepoUserName: repo.MustOwner().Name, - RepoName: repo.Name, - RefName: refName, - IsPrivate: repo.IsPrivate, + ActUserID: pusher.ID, + ActUser: pusher, + OpType: opType, + Content: string(data), + RepoID: repo.ID, + Repo: repo, + RefName: refName, + IsPrivate: repo.IsPrivate, }); err != nil { return fmt.Errorf("NotifyWatchers: %v", err) } @@ -598,14 +622,13 @@ func CommitRepoAction(opts CommitRepoActionOptions) error { func transferRepoAction(e Engine, doer, oldOwner *User, repo *Repository) (err error) { if err = notifyWatchers(e, &Action{ - ActUserID: doer.ID, - ActUserName: doer.Name, - OpType: ActionTransferRepo, - RepoID: repo.ID, - RepoUserName: repo.Owner.Name, - RepoName: repo.Name, - IsPrivate: repo.IsPrivate, - Content: path.Join(oldOwner.Name, repo.Name), + ActUserID: doer.ID, + ActUser: doer, + OpType: ActionTransferRepo, + RepoID: repo.ID, + Repo: repo, + IsPrivate: repo.IsPrivate, + Content: path.Join(oldOwner.Name, repo.Name), }); err != nil { return fmt.Errorf("notifyWatchers: %v", err) } @@ -628,14 +651,13 @@ func TransferRepoAction(doer, oldOwner *User, repo *Repository) error { func mergePullRequestAction(e Engine, doer *User, repo *Repository, issue *Issue) error { return notifyWatchers(e, &Action{ - ActUserID: doer.ID, - ActUserName: doer.Name, - OpType: ActionMergePullRequest, - Content: fmt.Sprintf("%d|%s", issue.Index, issue.Title), - RepoID: repo.ID, - RepoUserName: repo.Owner.Name, - RepoName: repo.Name, - IsPrivate: repo.IsPrivate, + ActUserID: doer.ID, + ActUser: doer, + OpType: ActionMergePullRequest, + Content: fmt.Sprintf("%d|%s", issue.Index, issue.Title), + RepoID: repo.ID, + Repo: repo, + IsPrivate: repo.IsPrivate, }) } diff --git a/models/action_test.go b/models/action_test.go index cb36966ec..c6d3911a6 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -1,6 +1,7 @@ package models import ( + "path" "strings" "testing" @@ -10,22 +11,21 @@ import ( ) func TestAction_GetRepoPath(t *testing.T) { - action := &Action{ - RepoUserName: "username", - RepoName: "reponame", - } - assert.Equal(t, "username/reponame", action.GetRepoPath()) + assert.NoError(t, PrepareTestDatabase()) + repo := AssertExistsAndLoadBean(t, &Repository{}).(*Repository) + owner := AssertExistsAndLoadBean(t, &User{ID: repo.OwnerID}).(*User) + action := &Action{RepoID: repo.ID} + assert.Equal(t, path.Join(owner.Name, repo.Name), action.GetRepoPath()) } func TestAction_GetRepoLink(t *testing.T) { - action := &Action{ - RepoUserName: "username", - RepoName: "reponame", - } + assert.NoError(t, PrepareTestDatabase()) + repo := AssertExistsAndLoadBean(t, &Repository{}).(*Repository) + owner := AssertExistsAndLoadBean(t, &User{ID: repo.OwnerID}).(*User) + action := &Action{RepoID: repo.ID} setting.AppSubURL = "/suburl/" - assert.Equal(t, "/suburl/username/reponame", action.GetRepoLink()) - setting.AppSubURL = "" - assert.Equal(t, "/username/reponame", action.GetRepoLink()) + expected := path.Join(setting.AppSubURL, owner.Name, repo.Name) + assert.Equal(t, expected, action.GetRepoLink()) } func TestNewRepoAction(t *testing.T) { @@ -36,13 +36,12 @@ func TestNewRepoAction(t *testing.T) { repo.Owner = user actionBean := &Action{ - OpType: ActionCreateRepo, - ActUserID: user.ID, - RepoID: repo.ID, - ActUserName: user.Name, - RepoName: repo.Name, - RepoUserName: repo.Owner.Name, - IsPrivate: repo.IsPrivate, + OpType: ActionCreateRepo, + ActUserID: user.ID, + RepoID: repo.ID, + ActUser: user, + Repo: repo, + IsPrivate: repo.IsPrivate, } AssertNotExistsBean(t, actionBean) @@ -64,14 +63,13 @@ func TestRenameRepoAction(t *testing.T) { repo.LowerName = strings.ToLower(newRepoName) actionBean := &Action{ - OpType: ActionRenameRepo, - ActUserID: user.ID, - ActUserName: user.Name, - RepoID: repo.ID, - RepoName: repo.Name, - RepoUserName: repo.Owner.Name, - IsPrivate: repo.IsPrivate, - Content: oldRepoName, + OpType: ActionRenameRepo, + ActUserID: user.ID, + ActUser: user, + RepoID: repo.ID, + Repo: repo, + IsPrivate: repo.IsPrivate, + Content: oldRepoName, } AssertNotExistsBean(t, actionBean) assert.NoError(t, RenameRepoAction(user, oldRepoName, repo)) @@ -232,13 +230,13 @@ func TestCommitRepoAction(t *testing.T) { pushCommits.Len = len(pushCommits.Commits) actionBean := &Action{ - OpType: ActionCommitRepo, - ActUserID: user.ID, - ActUserName: user.Name, - RepoID: repo.ID, - RepoName: repo.Name, - RefName: "refName", - IsPrivate: repo.IsPrivate, + OpType: ActionCommitRepo, + ActUserID: user.ID, + ActUser: user, + RepoID: repo.ID, + Repo: repo, + RefName: "refName", + IsPrivate: repo.IsPrivate, } AssertNotExistsBean(t, actionBean) assert.NoError(t, CommitRepoAction(CommitRepoActionOptions{ @@ -265,13 +263,12 @@ func TestTransferRepoAction(t *testing.T) { repo.Owner = user4 actionBean := &Action{ - OpType: ActionTransferRepo, - ActUserID: user2.ID, - ActUserName: user2.Name, - RepoID: repo.ID, - RepoName: repo.Name, - RepoUserName: repo.Owner.Name, - IsPrivate: repo.IsPrivate, + OpType: ActionTransferRepo, + ActUserID: user2.ID, + ActUser: user2, + RepoID: repo.ID, + Repo: repo, + IsPrivate: repo.IsPrivate, } AssertNotExistsBean(t, actionBean) assert.NoError(t, TransferRepoAction(user2, user2, repo)) @@ -290,13 +287,12 @@ func TestMergePullRequestAction(t *testing.T) { issue := AssertExistsAndLoadBean(t, &Issue{ID: 3, RepoID: repo.ID}).(*Issue) actionBean := &Action{ - OpType: ActionMergePullRequest, - ActUserID: user.ID, - ActUserName: user.Name, - RepoID: repo.ID, - RepoName: repo.Name, - RepoUserName: repo.Owner.Name, - IsPrivate: repo.IsPrivate, + OpType: ActionMergePullRequest, + ActUserID: user.ID, + ActUser: user, + RepoID: repo.ID, + Repo: repo, + IsPrivate: repo.IsPrivate, } AssertNotExistsBean(t, actionBean) assert.NoError(t, MergePullRequestAction(user, repo, issue)) diff --git a/models/consistency.go b/models/consistency.go index 77308e538..0c279eaaf 100644 --- a/models/consistency.go +++ b/models/consistency.go @@ -162,11 +162,5 @@ func (team *Team) checkForConsistency(t *testing.T) { func (action *Action) checkForConsistency(t *testing.T) { repo := AssertExistsAndLoadBean(t, &Repository{ID: action.RepoID}).(*Repository) - owner := AssertExistsAndLoadBean(t, &User{ID: repo.OwnerID}).(*User) - actor := AssertExistsAndLoadBean(t, &User{ID: action.ActUserID}).(*User) - - assert.Equal(t, repo.Name, action.RepoName, "action: %+v", action) assert.Equal(t, repo.IsPrivate, action.IsPrivate, "action: %+v", action) - assert.Equal(t, owner.Name, action.RepoUserName, "action: %+v", action) - assert.Equal(t, actor.Name, action.ActUserName, "action: %+v", action) } diff --git a/models/fixtures/action.yml b/models/fixtures/action.yml index 94d8e5759..10c0dc129 100644 --- a/models/fixtures/action.yml +++ b/models/fixtures/action.yml @@ -3,10 +3,7 @@ user_id: 2 op_type: 12 # close issue act_user_id: 2 - act_user_name: user2 repo_id: 2 - repo_user_name: user2 - repo_name: repo2 is_private: true - @@ -14,10 +11,7 @@ user_id: 3 op_type: 2 # rename repo act_user_id: 3 - act_user_name: user3 repo_id: 3 - repo_user_name: user3 - repo_name: repo3 is_private: true content: oldRepoName @@ -26,8 +20,5 @@ user_id: 11 op_type: 1 # create repo act_user_id: 11 - act_user_name: user11 repo_id: 9 - repo_user_name: user11 - repo_name: repo9 is_private: false diff --git a/models/issue.go b/models/issue.go index 8de6ea9cb..b78c6ba3f 100644 --- a/models/issue.go +++ b/models/issue.go @@ -918,14 +918,13 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) } if err = NotifyWatchers(&Action{ - ActUserID: issue.Poster.ID, - ActUserName: issue.Poster.Name, - OpType: ActionCreateIssue, - Content: fmt.Sprintf("%d|%s", issue.Index, issue.Title), - RepoID: repo.ID, - RepoUserName: repo.Owner.Name, - RepoName: repo.Name, - IsPrivate: repo.IsPrivate, + ActUserID: issue.Poster.ID, + ActUser: issue.Poster, + OpType: ActionCreateIssue, + Content: fmt.Sprintf("%d|%s", issue.Index, issue.Title), + RepoID: repo.ID, + Repo: repo, + IsPrivate: repo.IsPrivate, }); err != nil { log.Error(4, "NotifyWatchers: %v", err) } diff --git a/models/issue_comment.go b/models/issue_comment.go index a65abfe01..69edf28f5 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -329,13 +329,12 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err // Compose comment action, could be plain comment, close or reopen issue/pull request. // This object will be used to notify watchers in the end of function. act := &Action{ - ActUserID: opts.Doer.ID, - ActUserName: opts.Doer.Name, - Content: fmt.Sprintf("%d|%s", opts.Issue.Index, strings.Split(opts.Content, "\n")[0]), - RepoID: opts.Repo.ID, - RepoUserName: opts.Repo.Owner.Name, - RepoName: opts.Repo.Name, - IsPrivate: opts.Repo.IsPrivate, + ActUserID: opts.Doer.ID, + ActUser: opts.Doer, + Content: fmt.Sprintf("%d|%s", opts.Issue.Index, strings.Split(opts.Content, "\n")[0]), + RepoID: opts.Repo.ID, + Repo: opts.Repo, + IsPrivate: opts.Repo.IsPrivate, } // Check comment type. diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 297306415..5822533ee 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -114,6 +114,8 @@ var migrations = []Migration{ NewMigration("add field for login source synchronization", addLoginSourceSyncEnabledColumn), // v32 -> v33 NewMigration("add units for team", addUnitsToRepoTeam), + // v33 -> v34 + NewMigration("remove columns from action", removeActionColumns), } // Migrate database to current version diff --git a/models/migrations/v34.go b/models/migrations/v34.go new file mode 100644 index 000000000..94fcd870f --- /dev/null +++ b/models/migrations/v34.go @@ -0,0 +1,44 @@ +// Copyright 2017 Gitea. 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 ( + "fmt" + + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" + + "github.com/go-xorm/xorm" +) + +// ActionV34 describes the removed fields +type ActionV34 struct { + ActUserName string `xorm:"-"` + RepoUserName string `xorm:"-"` + RepoName string `xorm:"-"` +} + +// TableName will be invoked by XORM to customize the table name +func (*ActionV34) TableName() string { + return "action" +} + +func removeActionColumns(x *xorm.Engine) error { + switch { + case setting.UseSQLite3: + log.Warn("Unable to drop columns in SQLite") + case setting.UseMySQL, setting.UsePostgreSQL, setting.UseMSSQL, setting.UseTiDB: + if _, err := x.Exec("ALTER TABLE action DROP COLUMN act_user_name"); err != nil { + return fmt.Errorf("DROP COLUMN act_user_name: %v", err) + } else if _, err = x.Exec("ALTER TABLE action DROP COLUMN repo_user_name"); err != nil { + return fmt.Errorf("DROP COLUMN repo_user_name: %v", err) + } else if _, err = x.Exec("ALTER TABLE action DROP COLUMN repo_name"); err != nil { + return fmt.Errorf("DROP COLUMN repo_name: %v", err) + } + default: + log.Fatal(4, "Unrecognized DB") + } + return nil +} diff --git a/models/pull.go b/models/pull.go index 64fa51b61..76a646200 100644 --- a/models/pull.go +++ b/models/pull.go @@ -635,14 +635,13 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str } if err = NotifyWatchers(&Action{ - ActUserID: pull.Poster.ID, - ActUserName: pull.Poster.Name, - OpType: ActionCreatePullRequest, - Content: fmt.Sprintf("%d|%s", pull.Index, pull.Title), - RepoID: repo.ID, - RepoUserName: repo.Owner.Name, - RepoName: repo.Name, - IsPrivate: repo.IsPrivate, + ActUserID: pull.Poster.ID, + ActUser: pull.Poster, + OpType: ActionCreatePullRequest, + Content: fmt.Sprintf("%d|%s", pull.Index, pull.Title), + RepoID: repo.ID, + Repo: repo, + IsPrivate: repo.IsPrivate, }); err != nil { log.Error(4, "NotifyWatchers: %v", err) } else if err = pull.MailParticipants(); err != nil { diff --git a/routers/user/home.go b/routers/user/home.go index 3fe8a57d0..bafe62754 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -65,25 +65,50 @@ func retrieveFeeds(ctx *context.Context, ctxUser *models.User, userID, offset in // Check access of private repositories. feeds := make([]*models.Action, 0, len(actions)) - unameAvatars := map[string]string{ - ctxUser.Name: ctxUser.RelAvatarLink(), - } + userCache := map[int64]*models.User{ctxUser.ID: ctxUser} + repoCache := map[int64]*models.Repository{} for _, act := range actions { // Cache results to reduce queries. - _, ok := unameAvatars[act.ActUserName] + u, ok := userCache[act.ActUserID] if !ok { - u, err := models.GetUserByName(act.ActUserName) + u, err = models.GetUserByID(act.ActUserID) if err != nil { if models.IsErrUserNotExist(err) { continue } - ctx.Handle(500, "GetUserByName", err) + ctx.Handle(500, "GetUserByID", err) + return + } + userCache[act.ActUserID] = u + } + act.ActUser = u + + repo, ok := repoCache[act.RepoID] + if !ok { + repo, err = models.GetRepositoryByID(act.RepoID) + if err != nil { + if models.IsErrRepoNotExist(err) { + continue + } + ctx.Handle(500, "GetRepositoryByID", err) + return + } + } + act.Repo = repo + + repoOwner, ok := userCache[repo.OwnerID] + if !ok { + repoOwner, err = models.GetUserByID(repo.OwnerID) + if err != nil { + if models.IsErrUserNotExist(err) { + continue + } + ctx.Handle(500, "GetUserByID", err) return } - unameAvatars[act.ActUserName] = u.RelAvatarLink() } + repo.Owner = repoOwner - act.ActAvatar = unameAvatars[act.ActUserName] feeds = append(feeds, act) } ctx.Data["Feeds"] = feeds