From 9524b8c3702e204d9f942090acb39a3549c80ca8 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Mon, 11 Aug 2025 07:39:17 +0200 Subject: [PATCH] fix: PR review dismissals were not appearing in activity feed (#8853) Discovered that `NotifyPullRevieweDismiss` was dead code while working on another issue; it should have been `PullReviewDismiss` when originally implemented. Related fixes to the activity feed view which were incomplete as well. Dismissing a PR review now appears on the activity feed: ![image](/attachments/99bd37c5-218d-4ecf-a74c-d01451ceae17) ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8853 Reviewed-by: Earl Warren Co-authored-by: Mathieu Fenniak Co-committed-by: Mathieu Fenniak --- services/feed/action.go | 2 +- templates/user/dashboard/feeds.tmpl | 6 +-- tests/integration/user_dashboard_test.go | 50 ++++++++++++++++++++++-- 3 files changed, 50 insertions(+), 8 deletions(-) diff --git a/services/feed/action.go b/services/feed/action.go index d1d899a28e..f178e99001 100644 --- a/services/feed/action.go +++ b/services/feed/action.go @@ -307,7 +307,7 @@ func (*actionNotifier) AutoMergePullRequest(ctx context.Context, doer *user_mode } } -func (*actionNotifier) NotifyPullRevieweDismiss(ctx context.Context, doer *user_model.User, review *issues_model.Review, comment *issues_model.Comment) { +func (*actionNotifier) PullReviewDismiss(ctx context.Context, doer *user_model.User, review *issues_model.Review, comment *issues_model.Comment) { reviewerName := review.Reviewer.Name if len(review.OriginalAuthor) > 0 { reviewerName = review.OriginalAuthor diff --git a/templates/user/dashboard/feeds.tmpl b/templates/user/dashboard/feeds.tmpl index cdf0429714..82fc0b8e6e 100644 --- a/templates/user/dashboard/feeds.tmpl +++ b/templates/user/dashboard/feeds.tmpl @@ -76,7 +76,7 @@ {{else if .GetOpType.InActions "publish_release"}} {{$linkText := .Content | RenderEmoji $.Context}} {{ctx.Locale.Tr "action.publish_release" (.GetRepoLink ctx) (printf "%s/releases/tag/%s" (.GetRepoLink ctx) .GetTag) (.ShortRepoPath ctx) $linkText}} - {{else if .GetOpType.InActions "review_dismissed"}} + {{else if .GetOpType.InActions "pull_review_dismissed"}} {{$index := index .GetIssueInfos 0}} {{$reviewer := index .GetIssueInfos 1}} {{ctx.Locale.Tr "action.review_dismissed" (printf "%s/pulls/%s" (.GetRepoLink ctx) $index) $index (.ShortRepoPath ctx) $reviewer}} @@ -121,8 +121,8 @@ {{else if .GetOpType.InActions "close_issue" "reopen_issue" "close_pull_request" "reopen_pull_request"}} {{RenderIssueTitle ctx (.GetIssueTitle ctx) (.Repo.ComposeMetas ctx)}} {{else if .GetOpType.InActions "pull_review_dismissed"}} -
{{ctx.Locale.Tr "action.review_dismissed_reason"}}
-
{{index .GetIssueInfos 2 | RenderEmoji $.Context}}
+
{{ctx.Locale.Tr "action.review_dismissed_reason"}}
+
{{index .GetIssueInfos 2 | RenderEmoji $.Context}}
{{end}}
diff --git a/tests/integration/user_dashboard_test.go b/tests/integration/user_dashboard_test.go index d6ef1736d0..fefe5f1399 100644 --- a/tests/integration/user_dashboard_test.go +++ b/tests/integration/user_dashboard_test.go @@ -10,11 +10,13 @@ import ( "strings" "testing" - "forgejo.org/models/db" + "forgejo.org/models/issues" unit_model "forgejo.org/models/unit" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" + "forgejo.org/modules/gitrepo" issue_service "forgejo.org/services/issue" + pr_service "forgejo.org/services/pull" files_service "forgejo.org/services/repository/files" "forgejo.org/tests" @@ -62,10 +64,10 @@ func TestDashboardTitleRendering(t *testing.T) { issue := createIssue(t, user4, repo, "`:exclamation:` not rendered", "Hi there!") pr := createPullRequest(t, user4, repo, "testing", "`:exclamation:` not rendered") - _, err := issue_service.CreateIssueComment(db.DefaultContext, user4, repo, issue, "hi", nil) + _, err := issue_service.CreateIssueComment(t.Context(), user4, repo, issue, "hi", nil) require.NoError(t, err) - _, err = issue_service.CreateIssueComment(db.DefaultContext, user4, repo, pr.Issue, "hi", nil) + _, err = issue_service.CreateIssueComment(t.Context(), user4, repo, pr.Issue, "hi", nil) require.NoError(t, err) testIssueClose(t, sess, repo.OwnerName, repo.Name, strconv.Itoa(int(issue.Index)), false) @@ -101,7 +103,7 @@ func TestDashboardActionEscaping(t *testing.T) { issue := createIssue(t, user4, repo, "Issue with | in title", "Hey here's a | for you") - _, err := issue_service.CreateIssueComment(db.DefaultContext, user4, repo, issue, "Comment with a | in it", nil) + _, err := issue_service.CreateIssueComment(t.Context(), user4, repo, issue, "Comment with a | in it", nil) require.NoError(t, err) testIssueClose(t, sess, repo.OwnerName, repo.Name, strconv.Itoa(int(issue.Index)), false) @@ -122,3 +124,43 @@ func TestDashboardActionEscaping(t *testing.T) { assert.Equal(t, 4, count) }) } + +func TestDashboardReviewWorkflows(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + sess := loginUser(t, user4.Name) + + repo, _, f := tests.CreateDeclarativeRepo(t, user4, "", + []unit_model.Type{unit_model.TypePullRequests, unit_model.TypeIssues}, nil, + []*files_service.ChangeRepoFile{}, + ) + defer f() + gitRepo, err := gitrepo.OpenRepository(t.Context(), repo) + require.NoError(t, err) + + pr := createPullRequest(t, user4, repo, "testing", "My very first PR!") + + review, _, err := pr_service.SubmitReview(t.Context(), user4, gitRepo, pr.Issue, issues.ReviewTypeReject, "This isn't good enough!", "HEAD", []string{}) + require.NoError(t, err) + + _, err = pr_service.DismissReview(t.Context(), review.ID, repo.ID, "Come on, give the newbie a break!", user4, true, true) + require.NoError(t, err) + + response := sess.MakeRequest(t, NewRequest(t, "GET", "/"), http.StatusOK) + htmlDoc := NewHTMLParser(t, response.Body) + + count := 0 + htmlDoc.doc.Find("#activity-feed .flex-item-main .title").Each(func(i int, s *goquery.Selection) { + count++ + assert.Equal(t, "My very first PR!", s.Text()) + }) + htmlDoc.doc.Find("#activity-feed .flex-item-main .flex-item-body").Each(func(i int, s *goquery.Selection) { + count++ + if s.Text() != "Reason:" && s.Text() != "Come on, give the newbie a break!" { + assert.Fail(t, "Unexpected feed text", "Expected 'Reason:' and reason explanation, but found: %q", s.Text()) + } + }) + + assert.Equal(t, 4, count) + }) +}