From b698b256f1f687ae357d07b36e66931247bab22f Mon Sep 17 00:00:00 2001 From: "Gabriel A. Giovanini" Date: Mon, 16 Dec 2024 15:34:41 +0100 Subject: [PATCH 1/2] fix: Ignore self review request notification A notification would be trigger if a user request itself as review of a PR. --- services/issue/assignee.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/issue/assignee.go b/services/issue/assignee.go index 9c2ef74bb0..3d6d0b881a 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -72,7 +72,8 @@ func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer, reviewe return nil, err } - if comment != nil { + // don't notify if the user is requesting itself as reviewer + if comment != nil && doer.ID != reviewer.ID { notify_service.PullRequestReviewRequest(ctx, doer, issue, reviewer, isAdd, comment) } From d7fa527605b32359d7956d22d5dc28bf209805e1 Mon Sep 17 00:00:00 2001 From: "Gabriel A. Giovanini" Date: Tue, 17 Dec 2024 17:51:41 +0100 Subject: [PATCH 2/2] test: Test notification count for self review This tests the case where the user adds itself as reviewer and it shouldn't get a notification. --- tests/integration/pull_review_test.go | 98 +++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index b3380cc6b8..1319db29bf 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -5,6 +5,7 @@ package integration import ( "context" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -19,6 +20,7 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/gitrepo" + repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/test" issue_service "code.gitea.io/gitea/services/issue" repo_service "code.gitea.io/gitea/services/repository" @@ -62,6 +64,74 @@ func loadComment(t *testing.T, commentID string) *issues_model.Comment { return unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: id}) } +func TestPullView_SelfReviewNotification(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + user1Session := loginUser(t, "user1") + user2Session := loginUser(t, "user2") + + user1csrf := GetCSRF(t, user1Session, "/") + oldUser1NotificationCount := getUserNotificationCount(t, user1Session, user1csrf) + + user2csrf := GetCSRF(t, user2Session, "/") + oldUser2NotificationCount := getUserNotificationCount(t, user2Session, user2csrf) + + user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + repo, _, f := tests.CreateDeclarativeRepo(t, user2, "test_reviewer", nil, nil, []*files_service.ChangeRepoFile{ + { + Operation: "create", + TreePath: "CODEOWNERS", + ContentReader: strings.NewReader("README.md @user5\n"), + }, + }) + defer f() + + // we need to add user1 as collaborator so it can be added as reviewer + err := repo_module.AddCollaborator(db.DefaultContext, repo, user1) + require.NoError(t, err) + + // create a new branch to prepare for pull request + _, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{ + NewBranch: "codeowner-basebranch", + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + ContentReader: strings.NewReader("# This is a new project\n"), + }, + }, + }) + require.NoError(t, err) + + // Create a pull request. + resp := testPullCreate(t, user2Session, "user2", "test_reviewer", false, repo.DefaultBranch, "codeowner-basebranch", "Test Pull Request") + prURL := test.RedirectURL(resp) + elem := strings.Split(prURL, "/") + assert.EqualValues(t, "pulls", elem[3]) + + req := NewRequest(t, http.MethodGet, prURL) + resp = MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + attributeFilter := fmt.Sprintf("[data-update-url='/%s/%s/issues/request_review']", user2.Name, repo.Name) + issueID, ok := doc.Find(attributeFilter).Attr("data-issue-id") + assert.True(t, ok, "doc must contain data-issue-id") + + user1csrf = GetCSRF(t, user1Session, "/") + testAssignReviewer(t, user1Session, user1csrf, user2.Name, repo.Name, issueID, "1", http.StatusOK) + + // both user notification should keep the same notification count since + // user2 added itself as reviewer. + user1csrf = GetCSRF(t, user1Session, "/") + notificationCount := getUserNotificationCount(t, user1Session, user1csrf) + assert.Equal(t, oldUser1NotificationCount, notificationCount) + + user2csrf = GetCSRF(t, user2Session, "/") + notificationCount = getUserNotificationCount(t, user2Session, user2csrf) + assert.Equal(t, oldUser2NotificationCount, notificationCount) + }) +} + func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { defer tests.PrepareTestEnv(t)() session := loginUser(t, "user1") @@ -474,6 +544,28 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) { }) } +func testNofiticationCount(t *testing.T, session *TestSession, csrf string, expectedSubmitStatus int) *httptest.ResponseRecorder { + options := map[string]string{ + "_csrf": csrf, + } + + req := NewRequestWithValues(t, "GET", "/", options) + return session.MakeRequest(t, req, expectedSubmitStatus) +} + +func testAssignReviewer(t *testing.T, session *TestSession, csrf, owner, repo, pullID, reviewer string, expectedSubmitStatus int) *httptest.ResponseRecorder { + options := map[string]string{ + "_csrf": csrf, + "action": "attach", + "issue_ids": pullID, + "id": reviewer, + } + + submitURL := path.Join(owner, repo, "issues", "request_review") + req := NewRequestWithValues(t, "POST", submitURL, options) + return session.MakeRequest(t, req, expectedSubmitStatus) +} + func testSubmitReview(t *testing.T, session *TestSession, csrf, owner, repo, pullNumber, commitID, reviewType string, expectedSubmitStatus int) *httptest.ResponseRecorder { options := map[string]string{ "_csrf": csrf, @@ -502,3 +594,9 @@ func testIssueClose(t *testing.T, session *TestSession, owner, repo, issueNumber req = NewRequestWithValues(t, "POST", closeURL, options) return session.MakeRequest(t, req, http.StatusOK) } + +func getUserNotificationCount(t *testing.T, session *TestSession, csrf string) string { + resp := testNofiticationCount(t, session, csrf, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + return doc.Find(`.notification_count`).Text() +}