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

@@ -55,3 +55,5 @@ LEVEL = Info
 [security]
 INSTALL_LOCK   = true
 SECRET_KEY     = 9pCviYTWSb
+INTERNAL_TOKEN = eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYmYiOjE0OTU1NTE2MTh9.hhSVGOANkaKk3vfCd2jDOIww4pUk0xtg9JRde5UogyQ
+

models/action.go

@@ -74,11 +74,9 @@ type Action struct {
 	UserID      int64 `xorm:"INDEX"` // Receiver user id.
 	OpType      ActionType
 	ActUserID   int64       `xorm:"INDEX"` // Action user id.
-	ActUserName  string // Action user name.
-	ActAvatar    string `xorm:"-"`
+	ActUser     *User       `xorm:"-"`
 	RepoID      int64       `xorm:"INDEX"`
-	RepoUserName string
-	RepoName     string
+	Repo        *Repository `xorm:"-"`
 	RefName     string
 	IsPrivate   bool      `xorm:"INDEX NOT NULL DEFAULT false"`
 	Content     string    `xorm:"TEXT"`
@@ -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
@@ -206,11 +233,10 @@ 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,
+		ActUser:   u,
 		OpType:    ActionCreateRepo,
 		RepoID:    repo.ID,
-		RepoUserName: repo.Owner.Name,
-		RepoName:     repo.Name,
+		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
 	}); err != nil {
 		return fmt.Errorf("notify watchers '%d/%d': %v", u.ID, repo.ID, err)
@@ -228,11 +254,10 @@ 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,
+		ActUser:   actUser,
 		OpType:    ActionRenameRepo,
 		RepoID:    repo.ID,
-		RepoUserName: repo.Owner.Name,
-		RepoName:     repo.Name,
+		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
 		Content:   oldRepoName,
 	}); err != nil {
@@ -522,12 +547,11 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
 	refName := git.RefEndName(opts.RefFullName)
 	if err = NotifyWatchers(&Action{
 		ActUserID: pusher.ID,
-		ActUserName:  pusher.Name,
+		ActUser:   pusher,
 		OpType:    opType,
 		Content:   string(data),
 		RepoID:    repo.ID,
-		RepoUserName: repo.MustOwner().Name,
-		RepoName:     repo.Name,
+		Repo:      repo,
 		RefName:   refName,
 		IsPrivate: repo.IsPrivate,
 	}); err != nil {
@@ -599,11 +623,10 @@ 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,
+		ActUser:   doer,
 		OpType:    ActionTransferRepo,
 		RepoID:    repo.ID,
-		RepoUserName: repo.Owner.Name,
-		RepoName:     repo.Name,
+		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
 		Content:   path.Join(oldOwner.Name, repo.Name),
 	}); err != nil {
@@ -629,12 +652,11 @@ 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,
+		ActUser:   doer,
 		OpType:    ActionMergePullRequest,
 		Content:   fmt.Sprintf("%d|%s", issue.Index, issue.Title),
 		RepoID:    repo.ID,
-		RepoUserName: repo.Owner.Name,
-		RepoName:     repo.Name,
+		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
 	})
 }

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) {
@@ -39,9 +39,8 @@ func TestNewRepoAction(t *testing.T) {
 		OpType:    ActionCreateRepo,
 		ActUserID: user.ID,
 		RepoID:    repo.ID,
-		ActUserName:  user.Name,
-		RepoName:     repo.Name,
-		RepoUserName: repo.Owner.Name,
+		ActUser:   user,
+		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
 	}
 
@@ -66,10 +65,9 @@ func TestRenameRepoAction(t *testing.T) {
 	actionBean := &Action{
 		OpType:    ActionRenameRepo,
 		ActUserID: user.ID,
-		ActUserName:  user.Name,
+		ActUser:   user,
 		RepoID:    repo.ID,
-		RepoName:     repo.Name,
-		RepoUserName: repo.Owner.Name,
+		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
 		Content:   oldRepoName,
 	}
@@ -234,9 +232,9 @@ func TestCommitRepoAction(t *testing.T) {
 	actionBean := &Action{
 		OpType:    ActionCommitRepo,
 		ActUserID: user.ID,
-		ActUserName: user.Name,
+		ActUser:   user,
 		RepoID:    repo.ID,
-		RepoName:    repo.Name,
+		Repo:      repo,
 		RefName:   "refName",
 		IsPrivate: repo.IsPrivate,
 	}
@@ -267,10 +265,9 @@ func TestTransferRepoAction(t *testing.T) {
 	actionBean := &Action{
 		OpType:    ActionTransferRepo,
 		ActUserID: user2.ID,
-		ActUserName:  user2.Name,
+		ActUser:   user2,
 		RepoID:    repo.ID,
-		RepoName:     repo.Name,
-		RepoUserName: repo.Owner.Name,
+		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
 	}
 	AssertNotExistsBean(t, actionBean)
@@ -292,10 +289,9 @@ func TestMergePullRequestAction(t *testing.T) {
 	actionBean := &Action{
 		OpType:    ActionMergePullRequest,
 		ActUserID: user.ID,
-		ActUserName:  user.Name,
+		ActUser:   user,
 		RepoID:    repo.ID,
-		RepoName:     repo.Name,
-		RepoUserName: repo.Owner.Name,
+		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
 	}
 	AssertNotExistsBean(t, actionBean)

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)
 }

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

models/issue.go

@@ -919,12 +919,11 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string)
 
 	if err = NotifyWatchers(&Action{
 		ActUserID: issue.Poster.ID,
-		ActUserName:  issue.Poster.Name,
+		ActUser:   issue.Poster,
 		OpType:    ActionCreateIssue,
 		Content:   fmt.Sprintf("%d|%s", issue.Index, issue.Title),
 		RepoID:    repo.ID,
-		RepoUserName: repo.Owner.Name,
-		RepoName:     repo.Name,
+		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
 	}); err != nil {
 		log.Error(4, "NotifyWatchers: %v", err)

models/issue_comment.go

@@ -330,11 +330,10 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err
 	// This object will be used to notify watchers in the end of function.
 	act := &Action{
 		ActUserID: opts.Doer.ID,
-		ActUserName:  opts.Doer.Name,
+		ActUser:   opts.Doer,
 		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,
+		Repo:      opts.Repo,
 		IsPrivate: opts.Repo.IsPrivate,
 	}
 

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

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
+}

models/pull.go

@@ -636,12 +636,11 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str
 
 	if err = NotifyWatchers(&Action{
 		ActUserID: pull.Poster.ID,
-		ActUserName:  pull.Poster.Name,
+		ActUser:   pull.Poster,
 		OpType:    ActionCreatePullRequest,
 		Content:   fmt.Sprintf("%d|%s", pull.Index, pull.Title),
 		RepoID:    repo.ID,
-		RepoUserName: repo.Owner.Name,
-		RepoName:     repo.Name,
+		Repo:      repo,
 		IsPrivate: repo.IsPrivate,
 	}); err != nil {
 		log.Error(4, "NotifyWatchers: %v", err)

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
 			}
-			unameAvatars[act.ActUserName] = u.RelAvatarLink()
+			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
+			}
+		}
+		repo.Owner = repoOwner
 
-		act.ActAvatar = unameAvatars[act.ActUserName]
 		feeds = append(feeds, act)
 	}
 	ctx.Data["Feeds"] = feeds