Send email on Workflow Run Success/Failure

[NorthRealm]

Closes #23725

/claim #23725

[NorthRealm]

Note: For code reusability, there's a refactor/renaming in mail.go, mail_issue_common.go and mail_test.go in c8b64c7, ed7b2bc

[NorthRealm]

[lunny]

replace #33601

[wxiaoguang]

outdated

And by reading the history discussions:

• Fix #23725: Actions - send Email on Success/Failure #33601 (comment)

I think the concern is not addressed? should we send the email on "workflow job" level, or "workflow run" level? (I am not certain about the details, just FYI)

---

It looks right, the concerns in "Actions - send Email on Success/Failure #33601" should all be addressed. 🙏

[NorthRealm]

🤦‍♂️

[wxiaoguang]

🤦‍♂️

You can first understand what they do and what they affect before copying them.

[NorthRealm]

🤦‍♂️

You can first understand what they do and what they affect before copying them.

I haven't seen anywhere crappy so I would like to hear your thoughts.
I refer a lot from elsewhere in the whole code base, and I see it potentially reusable/re-applicable if it's been there.
Other than that I'm fine.
Related #28145

[wxiaoguang]

I haven't seen anywhere crappy so I would like to hear your thoughts.
I refer a lot from elsewhere in the whole code base, and I see it potentially reusable/re-applicable if it's been there.
Other than that I'm fine.
Related #28145

• _blank already means noopener
• <meta name="referrer" content="no-referrer"> already means noreferrer

[wxiaoguang]

I refer a lot from elsewhere in the whole code base, and I see it potentially reusable/re-applicable if it's been there.

Actually we should never trust existing code, there are bugs and bad-smells. If you just copy-paste existing code, then there will be more bugs and bad-smells.

[wxiaoguang]

Will explain more "bad smells" in old code that I have made changes to:

1. It shouldn't duplicate log.Error when using ServerError which already logs the err
2. It shouldn't show 500 error page when the user input is invalid.
3. It shouldn't set selected active for a dropdown item, selected and active have different meanings
4. It shouldn't duplicate {{if eq ...}}selected{{end}} when using dropdown, the input value is the selected one and the framework already handles it
5. It shouldn't duplicate rel="noopener noreferer", it only makes the translation more difficult (the affect has been explained above)

---

Actually I can tolerate the nits in code (not serious bugs) if the PR was reviewed and merged by others, or I missed some of them. While if I am reviewing or writing code, I will always do my best to avoid the bad smells.