Fix user profile activity feed (#1848)

* Fix user profile activity feed

* gofmt, and avoid overlapping database connections

Author: ethantkoenig

models/action.go

@@ -672,33 +672,39 @@ func MergePullRequestAction(actUser *User, repo *Repository, pull *Issue) error
 	return mergePullRequestAction(x, actUser, repo, pull)
 }
 
-// GetFeeds returns action list of given user in given context.
-// actorID is the user who's requesting, ctxUserID is the user/org that is requested.
-// actorID can be -1 when isProfile is true or to skip the permission check.
-func GetFeeds(ctxUser *User, actorID, offset int64, isProfile bool) ([]*Action, error) {
-	actions := make([]*Action, 0, 20)
-	sess := x.
-		Limit(20, int(offset)).
-		Desc("id").
-		Where("user_id = ?", ctxUser.ID)
-	if isProfile {
-		sess.
-			And("is_private = ?", false).
-			And("act_user_id = ?", ctxUser.ID)
-	} else if actorID != -1 && ctxUser.IsOrganization() {
-		env, err := ctxUser.AccessibleReposEnv(actorID)
+// GetFeedsOptions options for retrieving feeds
+type GetFeedsOptions struct {
+	RequestedUser    *User
+	RequestingUserID int64
+	IncludePrivate   bool // include private actions
+	OnlyPerformedBy  bool // only actions performed by requested user
+}
+
+// GetFeeds returns actions according to the provided options
+func GetFeeds(opts GetFeedsOptions) ([]*Action, error) {
+	var repoIDs []int64
+	if opts.RequestedUser.IsOrganization() {
+		env, err := opts.RequestedUser.AccessibleReposEnv(opts.RequestingUserID)
 		if err != nil {
 			return nil, fmt.Errorf("AccessibleReposEnv: %v", err)
 		}
-		repoIDs, err := env.RepoIDs(1, ctxUser.NumRepos)
-		if err != nil {
+		if repoIDs, err = env.RepoIDs(1, opts.RequestedUser.NumRepos); err != nil {
 			return nil, fmt.Errorf("GetUserRepositories: %v", err)
 		}
-		if len(repoIDs) > 0 {
-			sess.In("repo_id", repoIDs)
-		}
 	}
 
-	err := sess.Find(&actions)
-	return actions, err
+	actions := make([]*Action, 0, 20)
+	sess := x.Limit(20).
+		Desc("id").
+		Where("user_id = ?", opts.RequestedUser.ID)
+	if opts.OnlyPerformedBy {
+		sess.And("act_user_id = ?", opts.RequestedUser.ID)
+	}
+	if !opts.IncludePrivate {
+		sess.And("is_private = ?", false)
+	}
+	if opts.RequestedUser.IsOrganization() {
+		sess.In("repo_id", repoIDs)
+	}
+	return actions, sess.Find(&actions)
 }

models/action_test.go

@@ -305,29 +305,50 @@ func TestGetFeeds(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 	user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
 
-	actions, err := GetFeeds(user, user.ID, 0, false)
+	actions, err := GetFeeds(GetFeedsOptions{
+		RequestedUser:    user,
+		RequestingUserID: user.ID,
+		IncludePrivate:   true,
+		OnlyPerformedBy:  false,
+	})
 	assert.NoError(t, err)
 	assert.Len(t, actions, 1)
-	assert.Equal(t, int64(1), actions[0].ID)
-	assert.Equal(t, user.ID, actions[0].UserID)
-
-	actions, err = GetFeeds(user, user.ID, 0, true)
+	assert.EqualValues(t, 1, actions[0].ID)
+	assert.EqualValues(t, user.ID, actions[0].UserID)
+
+	actions, err = GetFeeds(GetFeedsOptions{
+		RequestedUser:    user,
+		RequestingUserID: user.ID,
+		IncludePrivate:   false,
+		OnlyPerformedBy:  false,
+	})
 	assert.NoError(t, err)
 	assert.Len(t, actions, 0)
 }
 
 func TestGetFeeds2(t *testing.T) {
 	// test with an organization user
 	assert.NoError(t, PrepareTestDatabase())
-	user := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User)
-
-	actions, err := GetFeeds(user, user.ID, 0, false)
+	org := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User)
+	userID := AssertExistsAndLoadBean(t, &OrgUser{OrgID: org.ID, IsOwner: true}).(*OrgUser).UID
+
+	actions, err := GetFeeds(GetFeedsOptions{
+		RequestedUser:    org,
+		RequestingUserID: userID,
+		IncludePrivate:   true,
+		OnlyPerformedBy:  false,
+	})
 	assert.NoError(t, err)
 	assert.Len(t, actions, 1)
-	assert.Equal(t, int64(2), actions[0].ID)
-	assert.Equal(t, user.ID, actions[0].UserID)
-
-	actions, err = GetFeeds(user, user.ID, 0, true)
+	assert.EqualValues(t, 2, actions[0].ID)
+	assert.EqualValues(t, org.ID, actions[0].UserID)
+
+	actions, err = GetFeeds(GetFeedsOptions{
+		RequestedUser:    org,
+		RequestingUserID: userID,
+		IncludePrivate:   false,
+		OnlyPerformedBy:  false,
+	})
 	assert.NoError(t, err)
 	assert.Len(t, actions, 0)
 }

models/fixtures/action.yml

@@ -10,7 +10,7 @@
   id: 2
   user_id: 3
   op_type: 2 # rename repo
-  act_user_id: 3
+  act_user_id: 2
   repo_id: 3
   is_private: true
   content: oldRepoName

routers/user/home.go

@@ -53,19 +53,20 @@ func getDashboardContextUser(ctx *context.Context) *models.User {
 	return ctxUser
 }
 
-// retrieveFeeds loads feeds from database by given context user.
-// The user could be organization so it is not always the logged in user,
-// which is why we have to explicitly pass the context user ID.
-func retrieveFeeds(ctx *context.Context, ctxUser *models.User, userID, offset int64, isProfile bool) {
-	actions, err := models.GetFeeds(ctxUser, userID, offset, isProfile)
+// retrieveFeeds loads feeds for the specified user
+func retrieveFeeds(ctx *context.Context, user *models.User, includePrivate, isProfile bool) {
+	actions, err := models.GetFeeds(models.GetFeedsOptions{
+		RequestedUser:    user,
+		RequestingUserID: ctx.User.ID,
+		IncludePrivate:   includePrivate,
+		OnlyPerformedBy:  isProfile,
+	})
 	if err != nil {
 		ctx.Handle(500, "GetFeeds", err)
 		return
 	}
 
-	// Check access of private repositories.
-	feeds := make([]*models.Action, 0, len(actions))
-	userCache := map[int64]*models.User{ctxUser.ID: ctxUser}
+	userCache := map[int64]*models.User{user.ID: user}
 	repoCache := map[int64]*models.Repository{}
 	for _, act := range actions {
 		// Cache results to reduce queries.
@@ -108,10 +109,8 @@ func retrieveFeeds(ctx *context.Context, ctxUser *models.User, userID, offset in
 			}
 		}
 		repo.Owner = repoOwner
-
-		feeds = append(feeds, act)
 	}
-	ctx.Data["Feeds"] = feeds
+	ctx.Data["Feeds"] = actions
 }
 
 // Dashboard render the dashborad page
@@ -180,7 +179,7 @@ func Dashboard(ctx *context.Context) {
 	ctx.Data["MirrorCount"] = len(mirrors)
 	ctx.Data["Mirrors"] = mirrors
 
-	retrieveFeeds(ctx, ctxUser, ctx.User.ID, 0, false)
+	retrieveFeeds(ctx, ctxUser, true, false)
 	if ctx.Written() {
 		return
 	}

routers/user/profile.go

@@ -138,7 +138,7 @@ func Profile(ctx *context.Context) {
 	ctx.Data["Keyword"] = keyword
 	switch tab {
 	case "activity":
-		retrieveFeeds(ctx, ctxUser, -1, 0, !showPrivate)
+		retrieveFeeds(ctx, ctxUser, showPrivate, true)
 		if ctx.Written() {
 			return
 		}