Fix user profile activity feed (#1848)

* Fix user profile activity feed

* gofmt, and avoid overlapping database connections
This commit is contained in:
Ethan Koenig 2017-06-01 20:42:25 -04:00 committed by Lunny Xiao
parent 5ca6867aaf
commit 4e5ee2b67a
5 changed files with 71 additions and 45 deletions

View file

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

View file

@ -305,13 +305,23 @@ func TestGetFeeds(t *testing.T) {
assert.NoError(t, PrepareTestDatabase()) assert.NoError(t, PrepareTestDatabase())
user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User) 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.NoError(t, err)
assert.Len(t, actions, 1) assert.Len(t, actions, 1)
assert.Equal(t, int64(1), actions[0].ID) assert.EqualValues(t, 1, actions[0].ID)
assert.Equal(t, user.ID, actions[0].UserID) assert.EqualValues(t, user.ID, actions[0].UserID)
actions, err = GetFeeds(user, user.ID, 0, true) actions, err = GetFeeds(GetFeedsOptions{
RequestedUser: user,
RequestingUserID: user.ID,
IncludePrivate: false,
OnlyPerformedBy: false,
})
assert.NoError(t, err) assert.NoError(t, err)
assert.Len(t, actions, 0) assert.Len(t, actions, 0)
} }
@ -319,15 +329,26 @@ func TestGetFeeds(t *testing.T) {
func TestGetFeeds2(t *testing.T) { func TestGetFeeds2(t *testing.T) {
// test with an organization user // test with an organization user
assert.NoError(t, PrepareTestDatabase()) assert.NoError(t, PrepareTestDatabase())
user := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User) org := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User)
userID := AssertExistsAndLoadBean(t, &OrgUser{OrgID: org.ID, IsOwner: true}).(*OrgUser).UID
actions, err := GetFeeds(user, user.ID, 0, false) actions, err := GetFeeds(GetFeedsOptions{
RequestedUser: org,
RequestingUserID: userID,
IncludePrivate: true,
OnlyPerformedBy: false,
})
assert.NoError(t, err) assert.NoError(t, err)
assert.Len(t, actions, 1) assert.Len(t, actions, 1)
assert.Equal(t, int64(2), actions[0].ID) assert.EqualValues(t, 2, actions[0].ID)
assert.Equal(t, user.ID, actions[0].UserID) assert.EqualValues(t, org.ID, actions[0].UserID)
actions, err = GetFeeds(user, user.ID, 0, true) actions, err = GetFeeds(GetFeedsOptions{
RequestedUser: org,
RequestingUserID: userID,
IncludePrivate: false,
OnlyPerformedBy: false,
})
assert.NoError(t, err) assert.NoError(t, err)
assert.Len(t, actions, 0) assert.Len(t, actions, 0)
} }

View file

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

View file

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

View file

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