Skip to content
This repository

Respect Time.zone #6

Merged
merged 7 commits into from over 2 years ago

2 participants

Brad Ary Borenszweig
Brad

This addresses the bug filed here: asterite/rgviz#1

Specs included.

I didnt 'fix' date parsing. I wasnt sure if this is something that should be addressed as I typically do not consider time zones when working with Date's. I am thinking the current date parsing is the expected behavior.

Ary Borenszweig
Owner

Looks fine. GitHub says I can't merge it automatically, probably because I commited to master (I just overwrote Rgviz::Parser, but I like your way of subclassing it better).

How can I merge the two forks?

Brad

I am not entirely sure. I have not accepted a pull request before... I imagine there is a way to manually resolve the conflict? If not, you could always revert the commit.

Brad

I was reading on this a bit and I think the common approach is for me to merge the upstream master and resolve conflicts. My latest commit merges your changes.

Ary Borenszweig asterite merged commit 53f627a into from
Ary Borenszweig asterite closed this
Ary Borenszweig
Owner

There, I just pushed version 0.63 of this gem. I use to push gems often... I hope that doesn't slow down rubygems. :-P

Ary Borenszweig
Owner

Hi Brad,

I'm not sure the pull request is correct. Try doing this:

  1. In spec/rgviz_rails/executor_spec.rb add the following to the before(:each) block:

    Time.zone = "Cairo"

Run the specs. You will see two tests fail because the label is incorrect. For the labels I use strftime of the parsed date/time values, but since you are changing their time by subtracting the timezone offset, it fails.

I'm not sure what's the point of subtracting the timezone offset. What's wrong about using Time.zone.parse to parse the time? If you are on a Rails app that's what you would expect to use... no? You wrote "TimeWithZone is not formatted as UTC when its used in ActiveRecord" but why do you need this? Is this a requirement of your app?

I need to understand more about this... I added the project to travis ci and it failed because Time.zone must be set to something different than GMT.

Brad

I had not given any consideration to labels. :(

My comments regarding TimeZoneWith might be best understood with an example. Suppose 1:00am is sent to the server in a query. If we use Time.zone.parse, this would be parsed as 1:00am PST (my timezone) and would be correct. The parsed time object is a TimeWithZone. When this value gets used in SQL via ActiveRecord, it has a string value of "1:00am" again. The correct value should be 1:00am - 7 hours (the UTC offset for PST). Essentially, TimeWithZone does not respect the timezone that our db is in. So SQL queries end up being off by some offset.

The goal of subtracting the offset is to get the time into the correct UTC time since this is the internal representation of Time anyway. I am on vacation now so I wont be able to really look into this further until next week. If this is urgent, hopefully my comments help shed some light on it.

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

Showing 7 unique commits by 1 author.

Mar 08, 2012
Brad Seefeld Respect the time zone c342858
Brad Seefeld Merge remote-tracking branch 'upstream/master' aec5f5e
Brad Seefeld Revert "Respect the time zone"
This reverts commit c342858.
b02e2b1
Mar 09, 2012
Brad Seefeld Updated rgviz 8d773a4
Brad Seefeld Added customized parser to handle time zones 50c8854
Brad Seefeld We want to subtract the offset - not add it. 1c6e07c
Brad Seefeld Fixed merge conflicts 15560a6
This page is out of date. Refresh to see the latest.
10 Gemfile.lock
... ... @@ -1,7 +1,7 @@
1 1 PATH
2 2 remote: .
3 3 specs:
4   - rgviz-rails (0.60.1)
  4 + rgviz-rails (0.61)
5 5 rails
6 6 rgviz
7 7
@@ -46,7 +46,7 @@ GEM
46 46 journey (1.0.3)
47 47 json (1.6.5)
48 48 machinist (1.0.6)
49   - mail (2.4.1)
  49 + mail (2.4.3)
50 50 i18n (>= 0.4.0)
51 51 mime-types (~> 1.16)
52 52 treetop (~> 1.4.8)
@@ -54,7 +54,7 @@ GEM
54 54 multi_json (1.1.0)
55 55 polyglot (0.3.3)
56 56 rack (1.4.1)
57   - rack-cache (1.1)
  57 + rack-cache (1.2)
58 58 rack (>= 0.4)
59 59 rack-ssl (1.3.2)
60 60 rack
@@ -78,7 +78,7 @@ GEM
78 78 rake (0.9.2.2)
79 79 rdoc (3.12)
80 80 json (~> 1.4)
81   - rgviz (0.42)
  81 + rgviz (0.43)
82 82 rspec (2.8.0)
83 83 rspec-core (~> 2.8.0)
84 84 rspec-expectations (~> 2.8.0)
@@ -97,7 +97,7 @@ GEM
97 97 treetop (1.4.10)
98 98 polyglot
99 99 polyglot (>= 0.3.1)
100   - tzinfo (0.3.31)
  100 + tzinfo (0.3.32)
101 101
102 102 PLATFORMS
103 103 ruby
20 lib/rgviz_rails.rb
... ... @@ -1,18 +1,8 @@
1   -require 'rgviz'
2   -require 'rgviz_rails/init.rb'
3   -require 'rgviz_rails/executor.rb'
4   -require 'rgviz_rails/js_renderer.rb'
5   -require 'rgviz_rails/tqx.rb'
6   -
7   -class Rgviz::Parser
8   - def parse_time(string)
9   - Time.zone.parse string
10   - end
11   -
12   - def parse_date(string)
13   - parse_time(string).to_date
14   - end
15   -end
  1 +require "rgviz_rails/init"
  2 +require "rgviz_rails/executor"
  3 +require "rgviz_rails/js_renderer"
  4 +require "rgviz_rails/tqx"
  5 +require "rgviz_rails/parser"
16 6
17 7 module RgvizRails
18 8 end
4 lib/rgviz_rails/executor.rb
@@ -27,8 +27,8 @@ def initialize(model_class)
27 27
28 28 def execute(query, options = {})
29 29 @query = query
30   - @query = Parser.parse(@query, options) unless @query.kind_of?(Query)
31   -
  30 + @query = RgvizRails::Parser.parse(@query, options) unless @query.kind_of?(Query)
  31 +
32 32 @table = Table.new
33 33 @extra_conditions = options[:conditions]
34 34
23 lib/rgviz_rails/parser.rb
... ... @@ -0,0 +1,23 @@
  1 +module RgvizRails
  2 + class Parser < Rgviz::Parser
  3 + def self.parse(string, options = {})
  4 + new(string, options).parse
  5 + end
  6 +
  7 + protected
  8 +
  9 + def parse_time(time_string)
  10 + time = Time.parse(time_string)
  11 +
  12 + # Honor the user's time zone if possible. We add the offset instead of using
  13 + # Time.zone.parse because we do not want a TimeWithZone object. TimeWithZone is not
  14 + # formatted as UTC when its used in ActiveRecord. We want the UTC
  15 + # (or system time zone time) time for the given local time.
  16 + if Time.zone
  17 + time -= Time.zone.utc_offset.seconds
  18 + end
  19 +
  20 + time
  21 + end
  22 + end
  23 +end
2  lib/rgviz_rails/view_helper.rb
@@ -87,7 +87,7 @@ def s.encode_json(options = {})
87 87 url = url_for url unless custom_executor
88 88
89 89 # Parse the query
90   - query = Parser.parse query, :extensions => extensions
  90 + query = RgvizRails::Parser.parse query, :extensions => extensions
91 91
92 92 # And replace the html_ and javascript_ magic names
93 93 query.accept visitor
1  rgviz-rails.gemspec
@@ -15,6 +15,7 @@ spec = Gem::Specification.new do |s|
15 15 "lib/rgviz_rails.rb",
16 16 "lib/rgviz_rails/executor.rb",
17 17 "lib/rgviz_rails/js_renderer.rb",
  18 + "lib/rgviz_rails/parser.rb",
18 19 "lib/rgviz_rails/tqx.rb",
19 20 "lib/rgviz_rails/view_helper.rb",
20 21 "lib/rgviz_rails/adapters/mysql_adapter.rb",
61 spec/rgviz_rails/parser_spec.rb
... ... @@ -0,0 +1,61 @@
  1 +require "spec_helper"
  2 +
  3 +describe RgvizRails::Parser do
  4 + describe :parse_time do
  5 + before :all do
  6 + @format = "%B %d, %Y %H:%M:%S"
  7 + end
  8 +
  9 + # Provide a proxy for testing the otherwise protected method
  10 + class RgvizRails::Parser
  11 + def public_parse_time(time_string)
  12 + parse_time(time_string)
  13 + end
  14 + end
  15 +
  16 + before :each do
  17 + @parser = RgvizRails::Parser.new("select *")
  18 + end
  19 +
  20 + context :default_time_zone do
  21 + before :each do
  22 + Time.zone = nil
  23 + end
  24 +
  25 + it "parses a time string" do
  26 + now = Time.now
  27 + @parser.public_parse_time(now.strftime(@format)).to_i.should == now.to_i
  28 + end
  29 + end
  30 +
  31 + context :custom_time_zone do
  32 +
  33 + def parse_with_zone(time_zone)
  34 + Time.zone = time_zone
  35 + now = Time.now
  36 + parsed = @parser.public_parse_time(now.strftime(@format))
  37 + (now.to_i - parsed.to_i).should == Time.zone.utc_offset
  38 + end
  39 +
  40 + it "parses a time string with the Pacific time zone" do
  41 + parse_with_zone("Pacific Time (US & Canada)")
  42 + end
  43 +
  44 + it "parses a time string with the Cairo time zone" do
  45 + parse_with_zone("Cairo")
  46 + end
  47 +
  48 + it "parses a string with the London time zone" do
  49 + parse_with_zone("London")
  50 + end
  51 +
  52 + it "parses a string with the Sydney time zone" do
  53 + parse_with_zone("Sydney")
  54 + end
  55 +
  56 + it "parses a string with the Central time zone" do
  57 + parse_with_zone("Central Time (US & Canada)")
  58 + end
  59 + end
  60 + end
  61 +end
37 spec/spec_helper.rb
... ... @@ -1,13 +1,13 @@
1   -$:.unshift(File.dirname(__FILE__) + '/../lib')
  1 +$:.unshift(File.dirname(__FILE__) + "/../lib")
2 2
3   -require 'rubygems'
4   -require 'logger'
  3 +require "rubygems"
  4 +require "logger"
  5 +require "rails/all"
  6 +require "active_record"
5 7
6   -require 'active_record'
7   -
8   -ActiveRecord::Base.establish_connection(:adapter => 'sqlite3', :database => ':memory:')
9   -#ActiveRecord::Base.establish_connection(:adapter => 'mysql2', :database => 'rgviz_rails', :username => 'root', :password => '')
10   -#ActiveRecord::Base.establish_connection(:adapter => 'postgresql', :database => 'rgviz_rails', :username => 'postgres', :password => '###', :host => '/var/run/postgresql/')
  8 +ActiveRecord::Base.establish_connection(:adapter => "sqlite3", :database => ":memory:")
  9 +#ActiveRecord::Base.establish_connection(:adapter => "mysql2", :database => "rgviz_rails", :username => "root", :password => "")
  10 +#ActiveRecord::Base.establish_connection(:adapter => "postgresql", :database => "rgviz_rails", :username => "postgres", :password => "###", :host => "/var/run/postgresql/")
11 11
12 12 ActiveRecord::Schema.define do
13 13 create_table "cities", :force => true do |t|
@@ -40,18 +40,15 @@
40 40 end
41 41 end
42 42
43   -require File.dirname(__FILE__) + '/models/person'
44   -require File.dirname(__FILE__) + '/models/city'
45   -require File.dirname(__FILE__) + '/models/country'
46   -require File.dirname(__FILE__) + '/models/foo'
47   -require File.dirname(__FILE__) + '/models/foo_bar'
48   -
49   -require File.dirname(__FILE__) + '/blueprints'
  43 +require File.dirname(__FILE__) + "/models/person"
  44 +require File.dirname(__FILE__) + "/models/city"
  45 +require File.dirname(__FILE__) + "/models/country"
  46 +require File.dirname(__FILE__) + "/models/foo"
  47 +require File.dirname(__FILE__) + "/models/foo_bar"
50 48
51   -require 'rgviz'
52   -require 'rgviz_rails/executor'
  49 +require File.dirname(__FILE__) + "/blueprints"
53 50
54   -RAILS_ENV = 'test'
  51 +require "rgviz"
  52 +require "rgviz_rails"
55 53
56   -# Add this directory so the ActiveSupport autoloading works
57   -#ActiveSupport::Dependencies.load_paths << File.dirname(__FILE__)
  54 +RAILS_ENV = "test"

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.