1
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2025-06-27 16:35:57 +00:00

bug: Forgejo Actions email notifications are opt-in (#8242)

* Add the `notify-email` column / NotifyEmail to ActionRun and set it:
  * services/actions/workflows.go `Dispatch`
  * services/actions/schedule_tasks.go `CreateScheduleTask`
  * services/actions/notifier_helper.go `handleWorkflows`
* Only send an email if the workflow has `enable-email-notifications: true` by having `MailActionRun` return immediately if `NotifyEmail` is false.
* Ignore or silently fail on `enable-email-notifications: true` parsing errors. Reporting such errors  belongs in workflow validation, not when it is evaluated for the notifications.
* Add unit and integration tests.

Refs: https://codeberg.org/forgejo/forgejo/issues/8187

## 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...
  - [x] in their respective `*_test.go` for unit tests.
  - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.

### Documentation

- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [x] I do not want this change to show in the release notes.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8242
Reviewed-by: Christopher Besch <mail@chris-besch.com>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>
This commit is contained in:
Earl Warren 2025-06-21 13:11:01 +02:00 committed by Earl Warren
parent 9e6f722f94
commit b2c4fc9f94
11 changed files with 274 additions and 0 deletions

View file

@ -55,6 +55,7 @@ type ActionRun struct {
PreviousDuration time.Duration
Created timeutil.TimeStamp `xorm:"created"`
Updated timeutil.TimeStamp `xorm:"updated"`
NotifyEmail bool
}
func init() {

View file

@ -105,6 +105,8 @@ var migrations = []*Migration{
NewMigration("Migrate maven package name concatenation", ChangeMavenArtifactConcatenation),
// v32 -> v33
NewMigration("Add federated user activity tables, update the `federated_user` table & add indexes", FederatedUserActivityMigration),
// v33 -> v34
NewMigration("Add `notify-email` column to `action_run` table", AddNotifyEmailToActionRun),
}
// GetCurrentDBVersion returns the current Forgejo database version.

View file

@ -0,0 +1,14 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package forgejo_migrations //nolint:revive
import "xorm.io/xorm"
func AddNotifyEmailToActionRun(x *xorm.Engine) error {
type ActionRun struct {
ID int64
NotifyEmail bool
}
return x.Sync(new(ActionRun))
}

View file

@ -345,6 +345,14 @@ func handleWorkflows(
Status: actions_model.StatusWaiting,
}
if workflow, err := model.ReadWorkflow(bytes.NewReader(dwf.Content)); err == nil {
notifications, err := workflow.Notifications()
if err != nil {
log.Error("Notifications: %w", err)
}
run.NotifyEmail = notifications
}
need, err := ifNeedApproval(ctx, run, input.Repo, input.Doer)
if err != nil {
log.Error("check if need approval for repo %d with user %d: %v", input.Repo.ID, input.Doer.ID, err)

View file

@ -4,6 +4,7 @@
package actions
import (
"bytes"
"context"
"errors"
"fmt"
@ -18,6 +19,7 @@ import (
webhook_module "forgejo.org/modules/webhook"
"github.com/nektos/act/pkg/jobparser"
act_model "github.com/nektos/act/pkg/model"
"xorm.io/builder"
)
@ -140,6 +142,16 @@ func CreateScheduleTask(ctx context.Context, cron *actions_model.ActionSchedule)
return err
}
workflow, err := act_model.ReadWorkflow(bytes.NewReader(cron.Content))
if err != nil {
return err
}
notifications, err := workflow.Notifications()
if err != nil {
return err
}
run.NotifyEmail = notifications
// Parse the workflow specification from the cron schedule
workflows, err := jobparser.Parse(cron.Content, jobparser.WithVars(vars))
if err != nil {

View file

@ -0,0 +1,121 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package actions
import (
"testing"
actions_model "forgejo.org/models/actions"
repo_model "forgejo.org/models/repo"
"forgejo.org/models/unittest"
webhook_module "forgejo.org/modules/webhook"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestCreateScheduleTask(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2, OwnerID: 2})
assertConstant := func(t *testing.T, cron *actions_model.ActionSchedule, run *actions_model.ActionRun) {
t.Helper()
assert.Equal(t, cron.Title, run.Title)
assert.Equal(t, cron.RepoID, run.RepoID)
assert.Equal(t, cron.OwnerID, run.OwnerID)
assert.Equal(t, cron.WorkflowID, run.WorkflowID)
assert.Equal(t, cron.TriggerUserID, run.TriggerUserID)
assert.Equal(t, cron.Ref, run.Ref)
assert.Equal(t, cron.CommitSHA, run.CommitSHA)
assert.Equal(t, cron.Event, run.Event)
assert.Equal(t, cron.EventPayload, run.EventPayload)
assert.Equal(t, cron.ID, run.ScheduleID)
assert.Equal(t, actions_model.StatusWaiting, run.Status)
}
assertMutable := func(t *testing.T, expected, run *actions_model.ActionRun) {
t.Helper()
assert.Equal(t, expected.NotifyEmail, run.NotifyEmail)
}
testCases := []struct {
name string
cron actions_model.ActionSchedule
want []actions_model.ActionRun
}{
{
name: "simple",
cron: actions_model.ActionSchedule{
Title: "scheduletitle1",
RepoID: repo.ID,
OwnerID: repo.OwnerID,
WorkflowID: "some.yml",
TriggerUserID: repo.OwnerID,
Ref: "branch",
CommitSHA: "fakeSHA",
Event: webhook_module.HookEventSchedule,
EventPayload: "fakepayload",
Content: []byte(
`
name: test
on: push
jobs:
job2:
runs-on: ubuntu-latest
steps:
- run: true
`),
},
want: []actions_model.ActionRun{
{
Title: "scheduletitle1",
NotifyEmail: false,
},
},
},
{
name: "enable-email-notifications is true",
cron: actions_model.ActionSchedule{
Title: "scheduletitle2",
RepoID: repo.ID,
OwnerID: repo.OwnerID,
WorkflowID: "some.yml",
TriggerUserID: repo.OwnerID,
Ref: "branch",
CommitSHA: "fakeSHA",
Event: webhook_module.HookEventSchedule,
EventPayload: "fakepayload",
Content: []byte(
`
name: test
enable-email-notifications: true
on: push
jobs:
job2:
runs-on: ubuntu-latest
steps:
- run: true
`),
},
want: []actions_model.ActionRun{
{
Title: "scheduletitle2",
NotifyEmail: true,
},
},
},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
require.NoError(t, CreateScheduleTask(t.Context(), &testCase.cron))
require.Equal(t, len(testCase.want), unittest.GetCount(t, actions_model.ActionRun{RepoID: repo.ID}))
for _, expected := range testCase.want {
run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{Title: expected.Title})
assertConstant(t, &testCase.cron, run)
assertMutable(t, &expected, run)
}
unittest.AssertSuccessfulDelete(t, actions_model.ActionRun{RepoID: repo.ID})
})
}
}

View file

@ -111,6 +111,11 @@ func (entry *Workflow) Dispatch(ctx context.Context, inputGetter InputValueGette
return nil, nil, err
}
notifications, err := wf.Notifications()
if err != nil {
return nil, nil, err
}
run := &actions_model.ActionRun{
Title: title,
RepoID: repo.ID,
@ -125,6 +130,7 @@ func (entry *Workflow) Dispatch(ctx context.Context, inputGetter InputValueGette
EventPayload: string(p),
TriggerEvent: string(webhook.HookEventWorkflowDispatch),
Status: actions_model.StatusWaiting,
NotifyEmail: notifications,
}
vars, err := actions_model.GetVariablesOfRun(ctx, run)

View file

@ -23,6 +23,10 @@ func MailActionRun(run *actions_model.ActionRun, priorStatus actions_model.Statu
return nil
}
if !run.NotifyEmail {
return nil
}
user := run.TriggerUser
// this happens e.g. when this is a scheduled run
if user.IsSystem() {

View file

@ -80,6 +80,7 @@ func TestActionRunNowDoneNotificationMail(t *testing.T) {
for _, run := range []*actions_model.ActionRun{run1, run2} {
run.TriggerUser = triggerUser
run.TriggerUserID = triggerUser.ID
run.NotifyEmail = true
}
repo.Owner = owner
repo.OwnerID = owner.ID
@ -100,6 +101,17 @@ func TestActionRunNowDoneNotificationMail(t *testing.T) {
notify_service.ActionRunNowDone(ctx, run2, actions_model.StatusRunning, nil)
})
t.Run("WorkflowEnableEmailNotificationIsFalse", func(t *testing.T) {
user := getActionsNowDoneTestUser(t, "new_user1", "new_user1@example.com", "enabled")
defer CleanUpUsers(ctx, []*user_model.User{user})
assignUsers(user, user)
defer MockMailSettings(func(msgs ...*Message) {
assert.Fail(t, "no mail should be sent")
})()
run2.NotifyEmail = false
notify_service.ActionRunNowDone(ctx, run2, actions_model.StatusRunning, nil)
})
for _, testCase := range []struct {
name string
triggerUser *user_model.User

View file

@ -0,0 +1,88 @@
// Copyright 2025 The Forgejo Authors. All rights reserved.
// SPDX-License-Identifier: GPL-3.0-or-later
package integration
import (
"fmt"
"net/url"
"testing"
actions_model "forgejo.org/models/actions"
auth_model "forgejo.org/models/auth"
"forgejo.org/models/unittest"
user_model "forgejo.org/models/user"
"forgejo.org/modules/setting"
"forgejo.org/tests"
"github.com/stretchr/testify/assert"
)
func TestActionNotifications(t *testing.T) {
if !setting.Database.Type.IsSQLite3() {
t.Skip()
}
testCases := []struct {
name string
treePath string
fileContent string
notifyEmail bool
}{
{
name: "enabled",
treePath: ".forgejo/workflows/enabled.yml",
fileContent: `name: enabled
on:
push:
enable-email-notifications: true
jobs:
job1:
runs-on: ubuntu-latest
steps:
- run: echo job1
`,
notifyEmail: true,
},
{
name: "disabled",
treePath: ".forgejo/workflows/disabled.yml",
fileContent: `name: disabled
on:
push:
jobs:
job1:
runs-on: ubuntu-latest
steps:
- run: echo job1
`,
notifyEmail: false,
},
}
onGiteaRun(t, func(t *testing.T, u *url.URL) {
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
session := loginUser(t, user2.Name)
token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
apiRepo := createActionsTestRepo(t, token, testCase.name, false)
runner := newMockRunner()
runner.registerAsRepoRunner(t, user2.Name, apiRepo.Name, "mock-runner", []string{"ubuntu-latest"})
opts := getWorkflowCreateFileOptions(user2, apiRepo.DefaultBranch, fmt.Sprintf("create %s", testCase.treePath), testCase.fileContent)
createWorkflowFile(t, token, user2.Name, apiRepo.Name, testCase.treePath, opts)
task := runner.fetchTask(t)
actionTask := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionTask{ID: task.Id})
actionRunJob := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRunJob{ID: actionTask.JobID})
actionRun := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: actionRunJob.RunID})
assert.Equal(t, testCase.notifyEmail, actionRun.NotifyEmail)
httpContext := NewAPITestContext(t, user2.Name, apiRepo.Name, auth_model.AccessTokenScopeWriteRepository)
doAPIDeleteRepository(httpContext)(t)
})
}
})
}

View file

@ -44,30 +44,35 @@ func (m *mockNotifier) ActionRunNowDone(ctx context.Context, run *actions_model.
assert.Equal(m.t, actions_model.StatusSuccess, run.Status)
assert.Equal(m.t, actions_model.StatusRunning, priorStatus)
assert.Nil(m.t, lastRun)
assert.True(m.t, run.NotifyEmail)
case 1:
assert.Equal(m.t, m.runID, run.ID)
assert.Equal(m.t, actions_model.StatusFailure, run.Status)
assert.Equal(m.t, actions_model.StatusRunning, priorStatus)
assert.Equal(m.t, m.lastRunID, lastRun.ID)
assert.Equal(m.t, actions_model.StatusSuccess, lastRun.Status)
assert.True(m.t, run.NotifyEmail)
case 2:
assert.Equal(m.t, m.runID, run.ID)
assert.Equal(m.t, actions_model.StatusCancelled, run.Status)
assert.Equal(m.t, actions_model.StatusRunning, priorStatus)
assert.Equal(m.t, m.lastRunID, lastRun.ID)
assert.Equal(m.t, actions_model.StatusFailure, lastRun.Status)
assert.True(m.t, run.NotifyEmail)
case 3:
assert.Equal(m.t, m.runID, run.ID)
assert.Equal(m.t, actions_model.StatusSuccess, run.Status)
assert.Equal(m.t, actions_model.StatusRunning, priorStatus)
assert.Equal(m.t, m.lastRunID, lastRun.ID)
assert.Equal(m.t, actions_model.StatusCancelled, lastRun.Status)
assert.True(m.t, run.NotifyEmail)
case 4:
assert.Equal(m.t, m.runID, run.ID)
assert.Equal(m.t, actions_model.StatusSuccess, run.Status)
assert.Equal(m.t, actions_model.StatusRunning, priorStatus)
assert.Equal(m.t, m.lastRunID, lastRun.ID)
assert.Equal(m.t, actions_model.StatusSuccess, lastRun.Status)
assert.True(m.t, run.NotifyEmail)
default:
assert.Fail(m.t, "too many notifications")
}
@ -101,6 +106,7 @@ func TestActionNowDoneNotification(t *testing.T) {
TreePath: ".forgejo/workflows/dispatch.yml",
ContentReader: strings.NewReader(
"name: test\n" +
"enable-email-notifications: true\n" +
"on: [workflow_dispatch]\n" +
"jobs:\n" +
" test:\n" +