From 462b61810e93871092c54c13ea6f97fddfce888f Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 9 Aug 2022 21:18:21 +0200 Subject: [PATCH 1/4] Fix duplicated actions on repository feed - Before understanding why this patch is here. The actions table stores the actions, this includes commenting on a issue, merging a pull request and pushing a tag etc. However, due to historical (and likely performance) reason, each action(e.g. commenting on a issue) is duplicated for each user(such as the poster, watcher of that issue and repository etc.). - This means, if you only specify the `repo_id` you will end up with a lot of duplicated actions. We fix this by de-duplicating the actions by their `created_unix`. While this isn't a perfect way of solving this problem, it will do the job for 99%. Only problems will arise for highly active repositories in which actions are being taken on the same second. --- models/action.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/models/action.go b/models/action.go index 14e021389a393..675af06be99b2 100644 --- a/models/action.go +++ b/models/action.go @@ -361,6 +361,13 @@ func GetFeeds(ctx context.Context, opts GetFeedsOptions) (ActionList, error) { actions := make([]*Action, 0, opts.PageSize) + // De-duplicate any action that has the same created unix, as actions are stored for each user + // who can see that action. This probably (though there is no guarantee) means that they were the same + // action. + if opts.RequestedRepo != nil { + sess = sess.GroupBy("`action`.created_unix") + } + if err := sess.Desc("`action`.created_unix").Find(&actions); err != nil { return nil, fmt.Errorf("Find: %v", err) } From 11a82cb5f1a42ae9f4bce5e5eab1b90330c3285d Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 10 Aug 2022 03:21:47 +0200 Subject: [PATCH 2/4] Add basic test-case --- models/action_test.go | 8 ++++++++ models/fixtures/action.yml | 18 ++++++++++++++++++ models/user_heatmap_test.go | 2 +- 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/models/action_test.go b/models/action_test.go index 2d46bd3e80e11..ffb011bf0542b 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -68,6 +68,7 @@ func TestGetFeedsForRepos(t *testing.T) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User) privRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}).(*repo_model.Repository) pubRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 8}).(*repo_model.Repository) + repoWithCollabs := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}).(*repo_model.Repository) // private repo & no login actions, err := GetFeeds(db.DefaultContext, GetFeedsOptions{ @@ -102,6 +103,13 @@ func TestGetFeedsForRepos(t *testing.T) { }) assert.NoError(t, err) assert.Len(t, actions, 1) + + // public repo & login + actions, err = GetFeeds(db.DefaultContext, GetFeedsOptions{ + RequestedRepo: repoWithCollabs, + }) + assert.NoError(t, err) + assert.Len(t, actions, 1) } func TestGetFeeds2(t *testing.T) { diff --git a/models/fixtures/action.yml b/models/fixtures/action.yml index a75092feb0ecc..64a4a227ee985 100644 --- a/models/fixtures/action.yml +++ b/models/fixtures/action.yml @@ -64,3 +64,21 @@ repo_id: 1700 # dangling intentional is_private: false created_unix: 1603011541 + +- id: 9 + user_id: 10 + op_type: 10 + act_user_id: 10 + repo_id: 1 + content: HelloWorld + is_private: false + created_unix: 1603011301 + +- id: 10 + user_id: 9 + op_type: 10 + act_user_id: 10 + repo_id: 1 + content: HelloWorld + is_private: false + created_unix: 1603011301 diff --git a/models/user_heatmap_test.go b/models/user_heatmap_test.go index 9361cb3452fa8..1ed387ccf8a3c 100644 --- a/models/user_heatmap_test.go +++ b/models/user_heatmap_test.go @@ -52,7 +52,7 @@ func TestGetUserHeatmapDataByUser(t *testing.T) { }, { "multiple actions performed with two grouped together", - 10, 10, 3, `[{"timestamp":1603009800,"contributions":1},{"timestamp":1603010700,"contributions":2}]`, + 10, 10, 4, `[{"timestamp":1603009800,"contributions":1},{"timestamp":1603010700,"contributions":3}]`, }, } // Prepare From f4b0d75726f9e95a5802b7e7c380523893037a58 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 10 Aug 2022 04:09:05 +0200 Subject: [PATCH 3/4] Use more advanced GroupBy + test case --- models/action.go | 2 +- models/action_test.go | 11 +++++++---- models/fixtures/action.yml | 21 +++++++++++++++++++-- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/models/action.go b/models/action.go index 675af06be99b2..5377ccb05cf75 100644 --- a/models/action.go +++ b/models/action.go @@ -365,7 +365,7 @@ func GetFeeds(ctx context.Context, opts GetFeedsOptions) (ActionList, error) { // who can see that action. This probably (though there is no guarantee) means that they were the same // action. if opts.RequestedRepo != nil { - sess = sess.GroupBy("`action`.created_unix") + sess = sess.GroupBy("`action`.created_unix, `action`.op_type, `action`.act_user_id") } if err := sess.Desc("`action`.created_unix").Find(&actions); err != nil { diff --git a/models/action_test.go b/models/action_test.go index ffb011bf0542b..e4d5aa21c8630 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -68,7 +68,7 @@ func TestGetFeedsForRepos(t *testing.T) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User) privRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}).(*repo_model.Repository) pubRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 8}).(*repo_model.Repository) - repoWithCollabs := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}).(*repo_model.Repository) + repoWithActions := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}).(*repo_model.Repository) // private repo & no login actions, err := GetFeeds(db.DefaultContext, GetFeedsOptions{ @@ -104,12 +104,15 @@ func TestGetFeedsForRepos(t *testing.T) { assert.NoError(t, err) assert.Len(t, actions, 1) - // public repo & login + // Check repository with four actions, all of them were created + // at the same time, but two of them are comments(one should be de-duped) + // the other one is closing a issue and the last one is also comment but by a different user. + // So in told there are 3 de-deduped actions. actions, err = GetFeeds(db.DefaultContext, GetFeedsOptions{ - RequestedRepo: repoWithCollabs, + RequestedRepo: repoWithActions, }) assert.NoError(t, err) - assert.Len(t, actions, 1) + assert.Len(t, actions, 3) } func TestGetFeeds2(t *testing.T) { diff --git a/models/fixtures/action.yml b/models/fixtures/action.yml index 64a4a227ee985..619fd62afa2e6 100644 --- a/models/fixtures/action.yml +++ b/models/fixtures/action.yml @@ -72,7 +72,7 @@ repo_id: 1 content: HelloWorld is_private: false - created_unix: 1603011301 + created_unix: 1603011301 # same as 10, 11, 12 - id: 10 user_id: 9 @@ -81,4 +81,21 @@ repo_id: 1 content: HelloWorld is_private: false - created_unix: 1603011301 + created_unix: 1603011301 # same as id 9, 11, 12 + +- id: 11 + user_id: 9 + op_type: 12 # differ op_type + act_user_id: 10 + repo_id: 1 + is_private: false + created_unix: 1603011301 # same as id 9, 10, 12 + +- id: 12 + user_id: 9 + op_type: 10 + act_user_id: 9 # differ act_user_id + repo_id: 1 + content: HelloWorld + is_private: false + created_unix: 1603011301 # same as id 9, 10, 11 From 4c90914b07e793fbc4ff717c43323872bd316d8c Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 10 Aug 2022 05:49:03 +0200 Subject: [PATCH 4/4] Use standardized SQL --- models/action.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/action.go b/models/action.go index 5377ccb05cf75..ab08ee07d5059 100644 --- a/models/action.go +++ b/models/action.go @@ -365,7 +365,7 @@ func GetFeeds(ctx context.Context, opts GetFeedsOptions) (ActionList, error) { // who can see that action. This probably (though there is no guarantee) means that they were the same // action. if opts.RequestedRepo != nil { - sess = sess.GroupBy("`action`.created_unix, `action`.op_type, `action`.act_user_id") + sess = sess.In("`action`.id", builder.Select("min(id) as id").From("action").GroupBy("`action`.created_unix, `action`.op_type, `action`.act_user_id")) } if err := sess.Desc("`action`.created_unix").Find(&actions); err != nil {