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

Already on GitHub? Sign in to your account

check mail params for every delivery method #400

Merged
merged 1 commit into from Nov 17, 2012

Conversation

Projects
None yet
5 participants
Contributor

dmathieu commented May 24, 2012

Currently, when sending an email via SMTP without a sender or recipient, an exception is raised.
This occurs only in SMTP though, when it should happen for every delivery method.

For example, if I don't specify a sender or recipient with the test delivery method, no error will be raised.
But when going to staging/production, there will be.

@dmathieu dmathieu check mail params for every delivery method
If we don't have a sender of recipient, we don't want to send the email, whatever the delivery method is.
ad92936
Collaborator

arunagw commented Jun 10, 2012

Looks good. 👍

cc @mikel

👍

Contributor

dmathieu commented Jul 10, 2012

:shipit: @arunagw @mikel bump :)

Owner

mikel commented Nov 17, 2012

Hi there, requiring a to, bcc or cc field violates the RFC..... which is why I didn't require.

However... it does make sense to raise an exception if you are trying to deliver something that has no destination, so pulling down now, testing and merging.

@mikel mikel merged commit ad92936 into mikel:master Nov 17, 2012

Contributor

dmathieu commented Nov 17, 2012

Yay ! Thank you @mikel ❤️

@alup alup commented on the diff Mar 19, 2013

lib/mail/check_delivery_params.rb
@@ -0,0 +1,30 @@
+module Mail
+
+ module CheckDeliveryParams
+
+ def self.included(klass)
+ klass.class_eval do
+
+ def check_params(mail)
+ envelope_from = mail.return_path || mail.sender || mail.from_addrs.first
+ if envelope_from.blank?
+ raise ArgumentError.new('A sender (Return-Path, Sender or From) required to send a message')
+ end
+
+ destinations ||= mail.destinations if mail.respond_to?(:destinations) && mail.destinations
@alup

alup Mar 19, 2013

Is && mail.destinations necessary here?

@dmathieu

dmathieu Mar 19, 2013

Contributor

I kept the condition as it was before. And we don't want to set destinations to nil, as it wouldn't be valid.

@alup

alup Mar 19, 2013

nil check is performed below, using blank?

@dmathieu

dmathieu Mar 19, 2013

Contributor

Yep, that's right. It can be removed then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment