Refactor mail notification

[lunny]

extract from #4001

[codecov-io]

Codecov Report

Merging #5110 into master will decrease coverage by 0.02%.
The diff coverage is 21.21%.

@@            Coverage Diff             @@
##           master    #5110      +/-   ##
==========================================
- Coverage   37.78%   37.75%   -0.03%     
==========================================
  Files         323      325       +2     
  Lines       47596    47638      +42     
==========================================
+ Hits        17986    17988       +2     
- Misses      27025    27056      +31     
- Partials     2585     2594       +9

Impacted Files	Coverage Δ
models/issue.go	47.72% <ø> (+0.12%)	⬆️
models/pull.go	50.75% <ø> (+0.1%)	⬆️
modules/notification/ui/ui.go	80.48% <ø> (+21.55%)	⬆️
models/issue_comment.go	46.66% <100%> (+0.05%)	⬆️
modules/notification/notification.go	28.88% <100%> (+0.79%)	⬆️
modules/notification/mail/mail.go	20% <20%> (ø)
modules/notification/base/null.go	9.09% <9.09%> (ø)
models/repo_indexer.go	44.49% <0%> (-3.82%)	⬇️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5228b8...b3f3a13. Read the comment docs.

[lunny]

extract from #4001

[zeripath]

I don't much like having the multiple empty functions. I guess we need to move to more generic notify interface with typed event objects? Does accepting this PR help with that move or Is there a current bug that needs this?

I guess this PR helps to place things in to standard structure but I think that this might be better shown as a commit in a well structured PR that would compile and pass the tests at each step along the way. Was this extracted from such a larger commit?

[kolaente]

Could you update the date to 2019?

[lunny]

@zeripath This PR is focus on refactoring. It makes the notification interface clearer to add new feature or find the bugs. But I haven't fix any bug or implement new feature in this PR. I think that's why there are so many blank functions here. I think further PR could do that. This is extract from #4001, but that PR is too big to review.

[zeripath]

OK, can we create a NullNotifier that has these empty methods, then change the definition of mailNotifier to be:

type mailNotifier {
   NullNotifier
}

and then just reimplement the methods that need reimplementing? I appreciate that that just moves these empty methods elsewhere but then implementing a notifier becomes much easier.

[lunny]

@zeripath good idea & done.

[zeripath]

Thanks @lunny !