handlers/email: Ability to toggle sending emails or not #26
Conversation
|
@joeybloggs Hey sorry to ping you, any chance this could be merged? |
handlers/email/email.go
Outdated
| @@ -119,6 +130,14 @@ func (email *Email) SetEmailConfig(host string, port int, username string, passw | |||
| email.formatter = email.formatFunc(email) | |||
| } | |||
|
|
|||
| // SetSend enables or disables the email handler sending emails | |||
| func (email *Email) SetSend(send bool) { | |||
There was a problem hiding this comment.
I think a more precise name might be better like SetEnabled
handlers/email/email.go
Outdated
| @@ -146,6 +165,9 @@ func defaultFormatFunc(email *Email) Formatter { | |||
|
|
|||
| // Log handles the log entry | |||
| func (email *Email) Log(e log.Entry) { | |||
| if !email.send { | |||
There was a problem hiding this comment.
There is a race here, this needs to be surrounded by email.rw.Rlock() or changed to us atomic, which in this case might make more sense.
handlers/email/email_test.go
Outdated
| @@ -104,6 +104,7 @@ func TestEmailHandler(t *testing.T) { | |||
| email.SetTimestampFormat("MST") | |||
| email.SetTemplate(defaultTemplate) | |||
| email.SetEmailConfig("localhost", 3041, "", "", "from@email.com", []string{"to@email.com"}) | |||
| email.SetSend(true) | |||
There was a problem hiding this comment.
It would be great if you could add an email test where it is set to false to ensure that when disabled it works appropriately.
|
@Chiiruno so sorry about that, slipped through the cracks I've reviewed and a few minor things I think need to be addressed before it can be merged. |
|
@joeybloggs Atomics ended up being a mess, so I decided to just go with the mutex, since it's a lot less lines and abstraction. |
|
For what it's worth, I tested this with my own email and it sends when it's enabled, doesn't when it's not. |
|
@joeybloggs I'm not rushing you or anything, there's no real deadline I'm on, but here's a ping in case you forgot. |
handlers/email/email.go
Outdated
| email.rw.RLock() | ||
|
|
||
| if !email.enabled { | ||
| return |
There was a problem hiding this comment.
the Rlock will never be released here, I would also not have 2 lock and unlocks so close to each other, one here and another shortly below, I recommend expanding the scope to only lock/unlock once.
There was a problem hiding this comment.
Oh wow, I can't believe I didn't notice that haha.
|
Hey @Chiiruno not rushing me, I reviewed and apparently forgot to hit submit, I thought I was waiting on you lolz |
Chiiruno
force-pushed
the
dev/email-handler
branch
from
9af85dc
to
54e8146
Compare
|
@joeybloggs Okay, that should be a bit more proper, what a silly mistake I made. |
|
@Chiiruno only one more thing, can a test be added that hits the not enabled logic. then I can merge. |
|
@joeybloggs I can't figure out any way to do this without adding extra variables and frankly, bloat. |
|
I don't think I have a secure way of checking if |
|
Okay, so the test is part of the email package, so I actually have access to the struct. |
Chiiruno
force-pushed
the
dev/email-handler
branch
from
79d3510
to
d79ea53
Compare
|
@Chiiruno Thanks, I will merge tomorrow, I have no time today unfortunately |
Fixes #25
I also took care of a few possible data race hot spots if you don't mind.