Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kemal?? #1

Closed
asterite opened this issue Oct 31, 2017 · 7 comments
Closed

Kemal?? #1

asterite opened this issue Oct 31, 2017 · 7 comments
Labels

Comments

@asterite
Copy link

What is this line doing here?

if ENV["KEMAL_ENV"]? != "test" # prevent accidental email sending in kemal

If this is a shard to send mails for kemal it should be called kemal_mailer or something like that.

If possible, I'd like shards in Crystal to avoid what happens in Ruby: Ruby is Rails, so let's make every gem know about Rails, just in case...

Could this be modeled in a different way?

@crisward
Copy link
Owner

You have a point. It'd be nice if there was a more generic env var that all devs would agree on like ie node has NODE_ENV, perhaps CRYSTAL_ENV.

This lib isn't designed to be used specifically with Kemal, I just happen to use it that way and I'm paranoid about tests triggering real mail.

Thanks for raising this issue BTW, reminds me to get around to finishing this.

@kazzkiq
Copy link

kazzkiq commented Oct 31, 2017

Perhaps we could transfer this verification to Mailer.config?

Something like:

# Current
Mailer.config(provider: Provider::Here)

# New
Mailer.config(provider: Provider::Here, enableSending: true))

enableSending could be false by default, thus preventing emails getting sent by unsuspecting developers.

@asterite
Copy link
Author

@crisward Makes sense. And I agree, it would be nice to know at least that you are in a "test" environment, or running specs.

@crisward
Copy link
Owner

@kazzkiq seems a bit odd to have a param which basically disables something by default. I think env vars are ideal for this as they describe something about the environment.

@crisward
Copy link
Owner

For running tests, I actually have a mock adaptor - https://github.com/crisward/mailer/blob/master/src/mailer/mock.cr

So I don't really need that line. It could just get moved up to the calling app. The actual tests explained in my readme are integration tests which send a real emails. Though I still think a unified CRYSTAL_ENV is a useful idea.

@crisward
Copy link
Owner

eg

if ENV.production?
  Mailer.config(provider: Mailer::Mailgun.new(key: ENV["MAILGUN_KEY"], domain: ENV["MAILGUN_DOMAIN"]))
else
  Mailer.config(provider:Mailer::Mock.new())
end

@crisward
Copy link
Owner

Removed ref to kemal, updated readme to help users config there way out of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants