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

Lotus::Mailer::MissingDeliveryDataError buble up Mail::ArgumentError messages #41

Merged
merged 2 commits into from Dec 16, 2015
Merged

Lotus::Mailer::MissingDeliveryDataError buble up Mail::ArgumentError messages #41

merged 2 commits into from Dec 16, 2015

Conversation

mrbongiolo
Copy link
Contributor

Per issue: #38

…gumentError message, so we have a more descriptive error message, helpful if your setup code is wrong or something.
@AlfonsoUceda
Copy link
Contributor

to me the PR is ok, I'm not a big fan concat string with + but is 👍

cc @lotus/core-team

@mrbongiolo
Copy link
Contributor Author

Will remember to use / next time :D

@@ -22,6 +22,16 @@
-> { MissingToMailer.deliver }.must_raise Lotus::Mailer::MissingDeliveryDataError
end

it "Lotus::Mailer::MissingDeliveryDataError bubles up the original Mail::ArgumentError message" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about rewording it so that a verb follows after it.

s/bubles/bubbles

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, will change it and resend :)

@mrbongiolo
Copy link
Contributor Author

Updated it. Should be good to go xD

runlevel5 pushed a commit that referenced this pull request Dec 16, 2015
Lotus::Mailer::MissingDeliveryDataError buble up Mail::ArgumentError messages
@runlevel5 runlevel5 merged commit ba95e82 into hanami:master Dec 16, 2015
@jodosha
Copy link
Member

jodosha commented Dec 16, 2015

@mrbongiolo @joneslee85 We should reconsider this code.

I'm for detailed exception messages, and showing the original reason of the error is a good practice.
But in this case, the result is misleading:

"Missing delivery data, please check " \
"'from', or 'to'. Mail::ArgumentError message => An SMTP From " \
"address is required to send a message. Set the message " \
"smtp_envelope_from, return_path, sender, or from address."
  1. There isn't a Mail::ArgumentError exception
  2. We don't have a smtp_envelope_from, return_path, sender.

This can generate confusion. I'm gonna revert this commit.
If we can find a less confusing solution, we can merge it again. Thank you very much.

@mrbongiolo
Copy link
Contributor Author

Maybe we could inform just to check the config or something? The main thing is that not only a missing from or to raises those errors.

@mrbongiolo
Copy link
Contributor Author

Since basically I just had to "check" the Original errors message and figure out that I had an error on my Mailer.config and not on the mailer itself.

@mrbongiolo
Copy link
Contributor Author

"Missing delivery data, please check 'from', or 'to'. If those are ok please check your Lotus::Mailer config file." something like this. Sorry about the 3 messages in a row.

@jodosha
Copy link
Member

jodosha commented Dec 16, 2015

@mrbongiolo Which was your case that left you confused? Which configuration you did it wrong?

@mrbongiolo
Copy link
Contributor Author

Lotus::Mailer.configure do
  root 'app/notifiers/mailers/templates'

  case ENV['RACK_ENV']
  when 'production', 'staging'
    delivery_method :smtp,
                    address: 'email-smtp.us-east-1.amazonaws.com',
                    port: '587',
                    domain: 'domain.com.br',
                    user_name: ENV['AWS_SES_SMTP_KEY'],
                    password: ENV['AWS_SES_SMTP_SECRET'],
                    authentication: 'plain',
                    enable_starttls_auto: true
  when 'development', 'test'
    delivery_method :test
  end
end.load!

Here inside my config block I had a wrong value for one of the options, can't really remember which one, but when I tested to send an email the Exception message said only to check from or to. Just a complementary warning saying to checkout your config block could solve this. Since I had to check the original message and see that something else was missing. The main problem that it happened the first time I was trying Lotus::Mailer.

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

Successfully merging this pull request may close these issues.

None yet

4 participants