From 9cb2aa989a4c331f9085126e5a88b5095b45ae54 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Thu, 9 May 2024 18:33:33 +0200 Subject: [PATCH] Teach activities.GetFeeds() how to avoid returning duplicates Before explaining the fix itself, lets look at the `action` table, and how it is populated. Data is only ever inserted into it via `activities_model.NotifyWatchers`, which will: - Insert a row for each activity with `UserID` set to the acting user's ID - this is the original activity, and is always inserted if anything is to be inserted at all. - It will insert a copy of each activity with the `UserID` set to the repo's owner, if the owner is an Organization, and isn't the acting user. - It will insert a copy of each activity for every watcher of the repo, as long as the watcher in question has read permission to the repo unit the activity is about. This means that if a repository belongs to an organizations, for most activities, it will have at least two rows in the table. For repositories watched by people other than their owner, an additional row for each watcher. These are useful duplicates, because they record which activities are relevant for a particular user. However, for cases where we wish to see the activities that happen around a repository, without limiting the results to a particular user, we're *not* interested in the duplicates stored for the watchers and the org. We only need the originals. And this is what this change does: it introduces an additional option to `GetFeedsOptions`: `OnlyPerformedByActor`. When this option is set, `activities.GetFeeds()` will only return the original activities, where the user id and the acting user id are the same. As these are *always* inserted, we're not missing out on any activities. We're just getting rid of the duplicates. As this is an additional `AND` condition, it can never introduce items that would not have been included in the result set before, it can only reduce, not extend. These duplicates were only affecting call sites where `RequestedRepo` was set, but `RequestedUser` and `RequestedTeam` were not. Both of those call sites were updated to set `OnlyPerformedByActor`. As a result, repository RSS feeds, and the `/repos/{owner}/{repo}/activities/feeds` API end points no longer return dupes, only the original activities. Rather than hardcoding this behaviour into `GetFeeds()` itself, I chose to implement it as an explicit option, for the sake of clarity. Fixes Codeberg/Community#684, and addresses gitea#20986. Signed-off-by: Gergely Nagy --- models/activities/action.go | 21 +++-- release-notes/8.0.0/fix/3598.md | 1 + routers/api/v1/repo/repo.go | 11 +-- routers/web/feed/repo.go | 9 +- tests/integration/api_repo_activities_test.go | 88 +++++++++++++++++++ 5 files changed, 113 insertions(+), 17 deletions(-) create mode 100644 release-notes/8.0.0/fix/3598.md create mode 100644 tests/integration/api_repo_activities_test.go diff --git a/models/activities/action.go b/models/activities/action.go index f85a493e2..b6c816f09 100644 --- a/models/activities/action.go +++ b/models/activities/action.go @@ -431,14 +431,15 @@ func (a *Action) GetIssueContent(ctx context.Context) string { // GetFeedsOptions options for retrieving feeds type GetFeedsOptions struct { db.ListOptions - RequestedUser *user_model.User // the user we want activity for - RequestedTeam *organization.Team // the team we want activity for - RequestedRepo *repo_model.Repository // the repo we want activity for - Actor *user_model.User // the user viewing the activity - IncludePrivate bool // include private actions - OnlyPerformedBy bool // only actions performed by requested user - IncludeDeleted bool // include deleted actions - Date string // the day we want activity for: YYYY-MM-DD + RequestedUser *user_model.User // the user we want activity for + RequestedTeam *organization.Team // the team we want activity for + RequestedRepo *repo_model.Repository // the repo we want activity for + Actor *user_model.User // the user viewing the activity + IncludePrivate bool // include private actions + OnlyPerformedBy bool // only actions performed by requested user + OnlyPerformedByActor bool // only actions performed by the original actor + IncludeDeleted bool // include deleted actions + Date string // the day we want activity for: YYYY-MM-DD } // GetFeeds returns actions according to the provided options @@ -481,6 +482,10 @@ func ActivityReadable(user, doer *user_model.User) bool { func activityQueryCondition(ctx context.Context, opts GetFeedsOptions) (builder.Cond, error) { cond := builder.NewCond() + if opts.OnlyPerformedByActor { + cond = cond.And(builder.Expr("`action`.user_id = `action`.act_user_id")) + } + if opts.RequestedTeam != nil && opts.RequestedUser == nil { org, err := user_model.GetUserByID(ctx, opts.RequestedTeam.OrgID) if err != nil { diff --git a/release-notes/8.0.0/fix/3598.md b/release-notes/8.0.0/fix/3598.md new file mode 100644 index 000000000..9d6b4ae0a --- /dev/null +++ b/release-notes/8.0.0/fix/3598.md @@ -0,0 +1 @@ +Fixed an issue that resulted in repository activity feeds (including RSS and Atom feeds) containing repeated activities. diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 66eb227c1..4be8c8ee7 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -1307,11 +1307,12 @@ func ListRepoActivityFeeds(ctx *context.APIContext) { listOptions := utils.GetListOptions(ctx) opts := activities_model.GetFeedsOptions{ - RequestedRepo: ctx.Repo.Repository, - Actor: ctx.Doer, - IncludePrivate: true, - Date: ctx.FormString("date"), - ListOptions: listOptions, + RequestedRepo: ctx.Repo.Repository, + OnlyPerformedByActor: true, + Actor: ctx.Doer, + IncludePrivate: true, + Date: ctx.FormString("date"), + ListOptions: listOptions, } feeds, count, err := activities_model.GetFeeds(ctx, opts) diff --git a/routers/web/feed/repo.go b/routers/web/feed/repo.go index bfcc3a37d..a0033c7d4 100644 --- a/routers/web/feed/repo.go +++ b/routers/web/feed/repo.go @@ -16,10 +16,11 @@ import ( // ShowRepoFeed shows user activity on the repo as RSS / Atom feed func ShowRepoFeed(ctx *context.Context, repo *repo_model.Repository, formatType string) { actions, _, err := activities_model.GetFeeds(ctx, activities_model.GetFeedsOptions{ - RequestedRepo: repo, - Actor: ctx.Doer, - IncludePrivate: true, - Date: ctx.FormString("date"), + OnlyPerformedByActor: true, + RequestedRepo: repo, + Actor: ctx.Doer, + IncludePrivate: true, + Date: ctx.FormString("date"), }) if err != nil { ctx.ServerError("GetFeeds", err) diff --git a/tests/integration/api_repo_activities_test.go b/tests/integration/api_repo_activities_test.go new file mode 100644 index 000000000..2b23ffc30 --- /dev/null +++ b/tests/integration/api_repo_activities_test.go @@ -0,0 +1,88 @@ +// Copyright 2024 The Forgejo Authors c/o Codeberg e.V.. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "fmt" + "net/http" + "testing" + + auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" +) + +func TestAPIRepoActivitiyFeeds(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + repo, _, f := CreateDeclarativeRepoWithOptions(t, owner, DeclarativeRepoOptions{}) + defer f() + + feedURL := fmt.Sprintf("/api/v1/repos/%s/activities/feeds", repo.FullName()) + assertAndReturnActivities := func(t *testing.T, length int) []api.Activity { + t.Helper() + + req := NewRequest(t, "GET", feedURL) + resp := MakeRequest(t, req, http.StatusOK) + var activities []api.Activity + DecodeJSON(t, resp, &activities) + + assert.Len(t, activities, length) + + return activities + } + createIssue := func(t *testing.T) { + session := loginUser(t, owner.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteIssue) + urlStr := fmt.Sprintf("/api/v1/repos/%s/issues?state=all", repo.FullName()) + req := NewRequestWithJSON(t, "POST", urlStr, &api.CreateIssueOption{ + Title: "ActivityFeed test", + Body: "Nothing to see here!", + }).AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + } + + t.Run("repo creation", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // Upon repo creation, there's a single activity. + assertAndReturnActivities(t, 1) + }) + + t.Run("single watcher, single issue", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // After creating an issue, we'll have two activities. + createIssue(t) + assertAndReturnActivities(t, 2) + }) + + t.Run("a new watcher, no new activities", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + watcher := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + watcherSession := loginUser(t, watcher.Name) + watcherToken := getTokenForLoggedInUser(t, watcherSession, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeReadUser) + + req := NewRequest(t, "PUT", fmt.Sprintf("/api/v1/repos/%s/subscription", repo.FullName())). + AddTokenAuth(watcherToken) + MakeRequest(t, req, http.StatusOK) + + assertAndReturnActivities(t, 2) + }) + + t.Run("multiple watchers, new issue", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + // After creating a second issue, we'll have three activities, even + // though we have multiple watchers. + createIssue(t) + assertAndReturnActivities(t, 3) + }) +}