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

#by_distance, #distance_to etc. should not include objects with no location set #73

Open
jensb opened this issue Apr 4, 2015 · 10 comments

Comments

@jensb
Copy link
Contributor

jensb commented Apr 4, 2015

In my code, Thing is an acts_as_mappable ActiveRecord object:

loc = Thing.new(latitude: 49.4, longitude: 11.07)
t = Thing.first
=> #<Thing id: 1, latitude: #<BigDecimal:7fca691e87b0,'0.50643049E2',18(27)>, longitude: #<BigDecimal:7fca691e8580,'0.4068654E1',18(27)>, ....>

t.distance_to(loc)
=> 519.1853786717959

The above works fine. But here is a problem:

loc0 = Thing.new
=> #<Thing id: nil, latitude: nil, longitude: nil, ....>
t.distance_to(loc0)
=> 5649.522116357252

loc1 = Thing.new(latitude: 0, longitude: 0)
=> #<Thing id: nil, latitude: 0, longitude: 0, ....>
t.distance_to(loc1)
=> 5649.522116357252

It seems that nil values for latitude/longitude are treated as being zero. This is wrong.

IMHO, t.distance_to should return nil when either the caller object or the parameter has no location. Also, by_distance should not include records that have nil lat/long values at all. Everything else makes no sense.

This behaviour was in geokit-rails 1.x if I remember correctly.

Can it be restored?

@jensb
Copy link
Contributor Author

jensb commented May 2, 2015

Hello, can anybody reproduce this problem? I tried to find the cause but so far I wasn't successful. Please try to reproduce it so I at least know that this is a real bug. Then I'll try to fix it and provide a pull request. Thanks!

@jensb
Copy link
Contributor Author

jensb commented May 14, 2015

Is nobody at all interested in this bug? There's a very trivial test - just add another location in both locations.yml and custom_locations.yml in the test fixtures that has no lat (or latitude) and no lng (longitude) - that should make a lot of tests fail (which they should not). Unfortunately I cannot get the tests to run on my machine.

@dipth
Copy link

dipth commented May 29, 2015

I fixed a similar problem by creating a new initializer in rails: config/initializers/geokit_rails_patch.rb with the following content:

module Geokit::ActsAsMappable
  module ClassMethods
    def within(distance, options = {})
      options[:within] = distance
      where("#{lat_column_name} IS NOT NULL AND #{lng_column_name} IS NOT NULL").
        where(distance_conditions(options))
    end
  end
end

You could probably add something like that for distance_to as well

@andrewhamon
Copy link

I can confirm that when using by_distance I am getting objects that have nil lat and/or long

@mnoack
Copy link
Member

mnoack commented Jun 9, 2015

If someone can write a basic test (look at existing tests and it should be fairly simple) and then take @dipth's good start code as part of overriding the within method (and perhaps related methods) I'm happy to merge (provided it passes the tests)

@andrewhamon
Copy link

@mnoack I though i would try and poke around but i can't seem to get the tests to run:

Warning: you should require 'minitest/autorun' instead.
Warning: or add 'gem "minitest"' before 'require "minitest/autorun"'
From:
  /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/test/unit.rb:1:in `<top (required)>'
  /Users/andrew/geokit-rails/test/boot.rb:3:in `<top (required)>'
  /Users/andrew/geokit-rails/test/test_helper.rb:3:in `<top (required)>'
  /Users/andrew/geokit-rails/test/acts_as_mappable_test.rb:1:in `<top (required)>'
  /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb:10:in `block (2 levels) in <main>'
  /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb:9:in `each'
  /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb:9:in `block in <main>'
  /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb:4:in `select'
  /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb:4:in `<main>'
MiniTest::Unit::TestCase is now Minitest::Test. From /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/test/unit/testcase.rb:8:in `<module:Unit>'
/Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/test/unit.rb:676:in `<class:Runner>': undefined method `_run_suite' for class `Test::Unit::Runner' (NameError)
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/test/unit.rb:261:in `<module:Unit>'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/test/unit.rb:15:in `<module:Test>'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/test/unit.rb:7:in `<top (required)>'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /Users/andrew/geokit-rails/test/boot.rb:3:in `<top (required)>'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /Users/andrew/geokit-rails/test/test_helper.rb:3:in `<top (required)>'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /Users/andrew/geokit-rails/test/acts_as_mappable_test.rb:1:in `<top (required)>'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in `require'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb:10:in `block (2 levels) in <main>'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb:9:in `each'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb:9:in `block in <main>'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb:4:in `select'
        from /Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb:4:in `<main>'
rake aborted!
Command failed with status (1): [ruby -I"lib:lib:test" -I"/Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib" "/Users/andrew/.rbenv/versions/2.1.5/lib/ruby/gems/2.1.0/gems/rake-10.4.2/lib/rake/rake_test_loader.rb" "test/*_test.rb" ]

@mnoack
Copy link
Member

mnoack commented Jun 9, 2015

I see you are using Ruby 2.1.6 - To make things easier on me/others trying to replicate your problem can you use the latest e.g. 2.1.6 or latest 2.2 as that's all we test on travis-ci.org and locally.

Even if you can't can you past your Gemfile.lock so I can be using the exact same versions of all your gems and debug this.

Also I would recommend trying a "bundle update" to get the latest gems versions as that's what travis/myself tends to use.

@andrewhamon
Copy link

Upgraded to Ruby 2.2.2 and rebundled and I got

/Users/andrew/.rbenv/versions/2.2.2/bin/ruby -I"lib:lib:test"  -I"/Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0" "/Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb" "test/*_test.rb"
/Users/andrew/geokit-rails/test/boot.rb:3:in `require': cannot load such file -- test/unit (LoadError)
        from /Users/andrew/geokit-rails/test/boot.rb:3:in `<top (required)>'
        from /Users/andrew/geokit-rails/test/test_helper.rb:3:in `require'
        from /Users/andrew/geokit-rails/test/test_helper.rb:3:in `<top (required)>'
        from /Users/andrew/geokit-rails/test/acts_as_mappable_test.rb:1:in `require'
        from /Users/andrew/geokit-rails/test/acts_as_mappable_test.rb:1:in `<top (required)>'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:10:in `require'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:10:in `block (2 levels) in <main>'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:9:in `each'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:9:in `block in <main>'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:4:in `select'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:4:in `<main>'
rake aborted!

I tried manually installing: gem install test-unit
and running rake without using bundle exec and now I am getting

Users/andrew/.rbenv/versions/2.2.2/bin/ruby -I"lib:lib:test"  "/Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb" "test/*_test.rb"
/Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rubygems/core_ext/kernel_require.rb:121:in `require': cannot load such file -- active_record/test_case (LoadError)
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rubygems/core_ext/kernel_require.rb:121:in `require'
        from /Users/andrew/geokit-rails/test/boot.rb:7:in `<top (required)>'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rubygems/core_ext/kernel_require.rb:54:in `require'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rubygems/core_ext/kernel_require.rb:54:in `require'
        from /Users/andrew/geokit-rails/test/test_helper.rb:3:in `<top (required)>'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rubygems/core_ext/kernel_require.rb:54:in `require'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rubygems/core_ext/kernel_require.rb:54:in `require'
        from /Users/andrew/geokit-rails/test/acts_as_mappable_test.rb:1:in `<top (required)>'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rubygems/core_ext/kernel_require.rb:54:in `require'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rubygems/core_ext/kernel_require.rb:54:in `require'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:10:in `block (2 levels) in <main>'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:9:in `each'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:9:in `block in <main>'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:4:in `select'
        from /Users/andrew/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb:4:in `<main>'
rake aborted!

@douglasmiller
Copy link

Please provide better documentation about how to run these tests.

This is what I have managed so far using Ruby 2.2.2 using postgresql:

  1. Add test-unit to the Gemfile and bundle install
  2. Edit test/boot.rb and replace require 'active_record/test_case' with require 'active_support/test_case'
  3. Create the tests user: createuser tests
  4. Create the geokit_rails_tests database: createdb geokit_rails_tests
  5. Run the tests: DB=postgresql bundle exec rake test
┌[dmiller@sterces]─[~/clones/geokit-rails]─[master *]
└[$]› DB=postgresql bundle exec rake test
/Users/dmiller/.rbenv/versions/2.2.2/bin/ruby -I"lib:lib:test" -I"/Users/dmiller/.rbenv/versions/2.2.2/lib/ruby/2.2.0" "/Users/dmiller/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb" "test/*_test.rb" 
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's default settings.
Coverage report Rcov style generated for Unit Tests to /Users/dmiller/clones/geokit-rails/coverage/rcov
[Coveralls] Outside the CI environment, not sending data.
Coverage must be above 49%. It is 38.78%
rake aborted!
Command failed with status (1): [ruby -I"lib:lib:test" -I"/Users/dmiller/.rbenv/versions/2.2.2/lib/ruby/2.2.0" "/Users/dmiller/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb" "test/*_test.rb" ]

Tasks: TOP => test
(See full trace by running task with --trace)

Then I got frustrated and rage quit. I am more than willing to help fix this issue. I have a locations table where the lat/long columns are sometimes nil. I have had to work around this by adding: .where.not(:latitude => nil, :longitude => nil) after my within call. I can't do anything if I can't run the tests.

Here are the contents of my Gemfile.lock if it is of use:

PATH
  remote: .
  specs:
    geokit-rails (2.1.0)
      geokit (~> 1.5)
      rails (>= 3.0)

GEM
  remote: https://rubygems.org/
  specs:
    actionmailer (4.2.3)
      actionpack (= 4.2.3)
      actionview (= 4.2.3)
      activejob (= 4.2.3)
      mail (~> 2.5, >= 2.5.4)
      rails-dom-testing (~> 1.0, >= 1.0.5)
    actionpack (4.2.3)
      actionview (= 4.2.3)
      activesupport (= 4.2.3)
      rack (~> 1.6)
      rack-test (~> 0.6.2)
      rails-dom-testing (~> 1.0, >= 1.0.5)
      rails-html-sanitizer (~> 1.0, >= 1.0.2)
    actionview (4.2.3)
      activesupport (= 4.2.3)
      builder (~> 3.1)
      erubis (~> 2.7.0)
      rails-dom-testing (~> 1.0, >= 1.0.5)
      rails-html-sanitizer (~> 1.0, >= 1.0.2)
    activejob (4.2.3)
      activesupport (= 4.2.3)
      globalid (>= 0.3.0)
    activemodel (4.2.3)
      activesupport (= 4.2.3)
      builder (~> 3.1)
    activerecord (4.2.3)
      activemodel (= 4.2.3)
      activesupport (= 4.2.3)
      arel (~> 6.0)
    activerecord-mysql2spatial-adapter (0.4.3)
      mysql2 (>= 0.2.13)
      rgeo-activerecord (~> 0.4.5)
    activesupport (4.2.3)
      i18n (~> 0.7)
      json (~> 1.7, >= 1.7.7)
      minitest (~> 5.1)
      thread_safe (~> 0.3, >= 0.3.4)
      tzinfo (~> 1.1)
    arel (6.0.3)
    builder (3.2.2)
    coveralls (0.8.2)
      json (~> 1.8)
      rest-client (>= 1.6.8, < 2)
      simplecov (~> 0.10.0)
      term-ansicolor (~> 1.3)
      thor (~> 0.19.1)
    docile (1.1.5)
    domain_name (0.5.24)
      unf (>= 0.0.5, < 1.0.0)
    erubis (2.7.0)
    geokit (1.9.0)
      multi_json (>= 1.3.2)
    globalid (0.3.6)
      activesupport (>= 4.1.0)
    http-cookie (1.0.2)
      domain_name (~> 0.5)
    i18n (0.7.0)
    json (1.8.3)
    loofah (2.0.2)
      nokogiri (>= 1.5.9)
    mail (2.6.3)
      mime-types (>= 1.16, < 3)
    metaclass (0.0.4)
    mime-types (2.6.1)
    mini_portile (0.6.2)
    minitest (5.8.0)
    mocha (0.14.0)
      metaclass (~> 0.0.1)
    multi_json (1.11.2)
    mysql (2.9.1)
    mysql2 (0.3.19)
    netrc (0.10.3)
    nokogiri (1.6.6.2)
      mini_portile (~> 0.6.0)
    pg (0.18.2)
    power_assert (0.2.4)
    rack (1.6.4)
    rack-test (0.6.3)
      rack (>= 1.0)
    rails (4.2.3)
      actionmailer (= 4.2.3)
      actionpack (= 4.2.3)
      actionview (= 4.2.3)
      activejob (= 4.2.3)
      activemodel (= 4.2.3)
      activerecord (= 4.2.3)
      activesupport (= 4.2.3)
      bundler (>= 1.3.0, < 2.0)
      railties (= 4.2.3)
      sprockets-rails
    rails-deprecated_sanitizer (1.0.3)
      activesupport (>= 4.2.0.alpha)
    rails-dom-testing (1.0.6)
      activesupport (>= 4.2.0.beta, < 5.0)
      nokogiri (~> 1.6.0)
      rails-deprecated_sanitizer (>= 1.0.1)
    rails-html-sanitizer (1.0.2)
      loofah (~> 2.0)
    railties (4.2.3)
      actionpack (= 4.2.3)
      activesupport (= 4.2.3)
      rake (>= 0.8.7)
      thor (>= 0.18.1, < 2.0)
    rake (10.4.2)
    rest-client (1.8.0)
      http-cookie (>= 1.0.2, < 2.0)
      mime-types (>= 1.16, < 3.0)
      netrc (~> 0.7)
    rgeo (0.3.20)
    rgeo-activerecord (0.4.6)
      activerecord (>= 3.0.3)
      arel (>= 2.0.6)
      rgeo (>= 0.3.20)
    simplecov (0.10.0)
      docile (~> 1.1.0)
      json (~> 1.8)
      simplecov-html (~> 0.10.0)
    simplecov-html (0.10.0)
    simplecov-rcov (0.2.3)
      simplecov (>= 0.4.1)
    sprockets (3.2.0)
      rack (~> 1.0)
    sprockets-rails (2.3.2)
      actionpack (>= 3.0)
      activesupport (>= 3.0)
      sprockets (>= 2.8, < 4.0)
    sqlite3 (1.3.10)
    term-ansicolor (1.3.2)
      tins (~> 1.0)
    test-unit (3.1.3)
      power_assert
    thor (0.19.1)
    thread_safe (0.3.5)
    tins (1.5.4)
    tzinfo (1.2.2)
      thread_safe (~> 0.1)
    unf (0.1.4)
      unf_ext
    unf_ext (0.0.7.1)

PLATFORMS
  ruby

DEPENDENCIES
  activerecord-mysql2spatial-adapter
  bundler (> 1.0)
  coveralls
  geokit-rails!
  mocha (~> 0.9)
  mysql (~> 2.8)
  mysql2 (~> 0.2)
  pg (~> 0.10)
  rake
  simplecov
  simplecov-rcov
  sqlite3
  test-unit

@douglasmiller
Copy link

It looks like test_case hasn't been in lib/active_record since 4-0-stable. I tried locking myself down to the 4.0 version of active record by adding gem 'activerecord', '~> 4.0.0' to the Gemfile before the gemspec.

Nothing very interesting happens, but I do see this depreciation warning:

DEPRECATION WARNING: ActiveRecord::TestCase is deprecated, please use ActiveSupport::TestCase. (called from require at /Users/dmiller/clones/geokit-rails/test/boot.rb:7)

I'm more used to rspec testing. Shouldn't I see more than this output:

┌[dmiller@sterces]─[~/clones/geokit-rails]─[master *]
└[$]› DB=postgresql bundle exec rake test
/Users/dmiller/.rbenv/versions/2.2.2/bin/ruby -I"lib:lib:test" -I"/Users/dmiller/.rbenv/versions/2.2.2/lib/ruby/2.2.0" "/Users/dmiller/.rbenv/versions/2.2.2/lib/ruby/2.2.0/rake/rake_test_loader.rb" "test/*_test.rb" 
DEPRECATION WARNING: ActiveRecord::TestCase is deprecated, please use ActiveSupport::TestCase. (called from require at /Users/dmiller/clones/geokit-rails/test/boot.rb:7)
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's default settings.
Coverage report Rcov style generated for Unit Tests to /Users/dmiller/clones/geokit-rails/coverage/rcov
[Coveralls] Outside the CI environment, not sending data.

With this diff:

┌[dmiller@sterces]─[~/clones/geokit-rails]─[master *]
└[$]› git diff
diff --git a/Gemfile b/Gemfile
index fa75df1..89de79b 100644
--- a/Gemfile
+++ b/Gemfile
@@ -1,3 +1,5 @@
 source 'https://rubygems.org'

+gem 'activerecord', '~> 4.0.0'
 gemspec
+gem 'test-unit'
diff --git a/test/acts_as_mappable_test.rb b/test/acts_as_mappable_test.rb
index 56f7c84..00fed1f 100644
--- a/test/acts_as_mappable_test.rb
+++ b/test/acts_as_mappable_test.rb
@@ -78,6 +78,7 @@ class ActsAsMappableTest < GeokitTestCase
   def test_find_with_distance_condition
     #locations = Location.geo_scope(:origin => @loc_a, :within => 3.97)
     locations = Location.within(3.97, :origin => @loc_a)
+    assert_equal true, false
     assert_equal 5, locations.to_a.size
     assert_equal 5, locations.count
   end

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

No branches or pull requests

5 participants