kshnurov / mandrill_dm

A basic Mandrill delivery method for Rails.
MIT License
46 stars 45 forks source link

Add send_at support #43

Closed spovich closed 8 years ago

spovich commented 8 years ago

Mostly use code from @sbauch, thank you! Did some refactoring and added tests for date/time handling.

spovich commented 8 years ago

@kshnurov please have a look and let me what you think.

kshnurov commented 8 years ago

@spovich is_a?(DateTime) make no sense here, you can remove it:

2.2.3 :016 > DateTime.current.is_a?(Date)
 => true 

to_datetime was used for a reason:

2.2.3 :012 > Date.today.to_time.utc
 => 2016-04-24 21:00:00 UTC 
2.2.3 :013 > Date.today.to_datetime.utc
 => Mon, 25 Apr 2016 00:00:00 +0000 

There's a way to fix it, but it works only for Date - Date.today.to_time(:utc).utc. For DateTime we should use the default one - DateTime.current.to_time.utc

spovich commented 8 years ago

@kshnurov good catch, I didn't know that DateTime responds true to is_a?(Date). I've removed that condition.

I don't understand why the behavior you demonstrate is desirable. Date.today is the date in your local timezone. Casting to_time.utc is the beginning of the day in your local timezone. When you convert to to_datetime.utc you are getting the beginning of the day in GMT. That doesn't seem like what the normal user would want (unless you live in GMT zone).

Preferably, users should pass DateTime or Time to get predictable send time. But, the reason I used time is because in non-rails projects, DateTime doesn't respond to utc but Time does:

In rails:

2.3.0 :006 > DateTime.now.utc
 => Mon, 25 Apr 2016 05:07:32 +0000

Ruby (no ActiveSupport extensions to DateTime):

irb -I . -r 'lib/mandrill_dm'

2.2.3 :001 > DateTime.now
 => #<DateTime: 2016-04-24T22:07:57-07:00 ((2457504j,18477s,759410000n),-25200s,2299161j)>
2.2.3 :002 > DateTime.now.utc
NoMethodError: undefined method `utc' for #<DateTime:0x007fb80c33e890>
2.2.3 :003 > Time.now.utc
 => 2016-04-25 05:13:11 UTC
kshnurov commented 8 years ago

Date.today is the date in your local timezone

Nope! Date has no timezone info, it's just a date. So it's better to keep it timezone-free = in UTC. If you care about time and timezone, you should use Time/DateTime. If you're passing a plain Date, you probably know what you're doing and you don't expect it to be changed to another date (25 Apr will turn into 24 Apr in GMT+).

Another way is to accept only Time and DateTime - maybe it'be better, since Mandrill won't accept plain date. How about this:

def send_at_formatted_string(obj)
  obj = obj.to_time if obj.is_a?(DateTime)
  return obj.utc.strftime('%Y-%m-%d %H:%M:%S') if obj.is_a?(Time)
  return obj if obj.is_a?(String)
  fail ArgumentError, 'send_at should be Time/DateTime or String'
end
sbauch commented 8 years ago

yeah I certainly had some trouble with dates / times and time zones. I'm no longer with the company whose account I had forked this to, so I lost access to the repo, but seems you have it under control. thanks!

spovich commented 8 years ago

@kshnurov Yes, I know that Date doesn't have a timezone. I didn't complete my thought... I meant to say Date.today is the date in your local timezone when you are running ruby locally (like running the tests for this project) or the date in your server's timezone (or UTC since most server's are set to UTC) when running remotely.

I like your idea of avoiding all the problems with Date. Only accept Time or DateTIme (or string). Works for me!

kshnurov commented 8 years ago

It's time to release a new version :)

spovich commented 8 years ago

Yep, I'll do that tonight. thanks!

spovich commented 8 years ago

done! I did 1.3.0 and then saw that minimum_ruby_version is displayed on rubygems.org page, so I added that as >= 2.0 and pushed 1.3.1