Repository: go-gitea/gitea
File: services/mailer/mail_workflow_run.go (Go)

Code before:
func SendActionsWorkflowRunStatusEmail(ctx context.Context, sender *user_model.User, repo *repo_model.Repository, run *actions_model.ActionRun) {
	if setting.MailService == nil {
		return
	}
	if run.Status.IsSkipped() {
		return
	}

	recipients := make([]*user_model.User, 0)

	if !sender.IsGiteaActions() && !sender.IsGhost() && sender.IsMailable() {
		notifyPref, err := user_model.GetUserSetting(ctx, sender.ID,
			user_model.SettingsEmailNotificationGiteaActions, user_model.EmailNotificationGiteaActionsFailureOnly)
		if err != nil {
			log.Error("GetUserSetting: %v", err)
			return
		}
		if run.Status.IsSuccess() {
			if notifyPref == user_model.EmailNotificationGiteaActionsAll {
				recipients = append(recipients, sender)
			}
			sendActionsWorkflowRunStatusEmail(ctx, repo, run, sender, recipients)
			return
		} else if notifyPref != user_model.EmailNotificationGiteaActionsDisabled {
			recipients = append(recipients, sender)
		}
	}

	if len(recipients) > 0 {
		sendActionsWorkflowRunStatusEmail(ctx, repo, run, sender, recipients)
	}
}


Review comment (bug):
_Functionally this works right now, this are optional change request comments at very high level_

 > Initially I do prepare a list of recipients. Right now just send to the trigger, without updating func arguments once again

Yes I read the code like: you prepare recipients then call sendActionsWorkflowRunStatusEmail

However now we have multiple places where this function calls an exists without doing anything else.
```go
sendActionsWorkflowRunStatusEmail(ctx, repo, run, sender, recipients)
```
So this one still calls this code if the workflow succeeded, even if the user only gets notified on failure.

> without updating func arguments once again

Doesn't line 159-162 update the arguments "updating func arguments once again"?
```go
			if notifyPref == user_model.EmailNotificationGiteaActionsAll {
				recipients = append(recipients, sender)
			}
```

Idk, maybe a linter complained due to an nested if inside an if... then the following refactoring would resolve this:
```suggestion
		if notifyPref == user_model.EmailNotificationGiteaActionsAll || !run.Status.IsSuccess() && notifyPref != user_model.EmailNotificationGiteaActionsDisabled {
			recipients = append(recipients, sender)
		}
```

Code after:
	if setting.MailService == nil {
		return
	}
	if run.Status.IsSkipped() {
		return
	}

	recipients := make([]*user_model.User, 0)

	if !sender.IsGiteaActions() && !sender.IsGhost() && sender.IsMailable() {
		notifyPref, err := user_model.GetUserSetting(ctx, sender.ID,
			user_model.SettingsKeyEmailNotificationGiteaActions, user_model.SettingEmailNotificationGiteaActionsFailureOnly)
		if err != nil {
			log.Error("GetUserSetting: %v", err)
			return
		}
		if notifyPref == user_model.SettingEmailNotificationGiteaActionsAll || !run.Status.IsSuccess() && notifyPref != user_model.SettingEmailNotificationGiteaActionsDisabled {
			recipients = append(recipients, sender)
		}
	}

	if len(recipients) > 0 {
		composeAndSendActionsWorkflowRunStatusEmail(ctx, repo, run, sender, recipients)
	}
}