Added geocoded_through #66

Closed
wants to merge 17 commits into
from

Projects

None yet
@vpuzzella
class User < ActiveRecord::Base
  has_many :inventory_items
  geocoded_by :postal_code
  reverse_geocoded_by :latitude, :longitude
  after_validation :geocode
end

class InventoryItem < ActiveRecord::Base
  belongs_to :user
  geocoded_through :user
end

irb(main):013:0> InventoryItem.near('M4J3J8').first.user.postal_code
=> "M4J3J8"
irb(main):014:0> InventoryItem.near('M4J3J8').first.reverse_geocode
=> "127-135 Dewhurst Blvd, East York, ON M4J 3J9, Canada"
irb(main):015:0>

@swrobel
swrobel commented Sep 21, 2011

Has this been reviewed? This would be great functionality to have.

@Ozkar
Ozkar commented Oct 9, 2011

I tried this out, but it kept returning nil for distance.

I have this in my gemfile:

gem 'geocoder', :git => 'git://github.com/vpuzzella/geocoder.git'

class Address < ActiveRecord::Base
    has_many :items
    geocoded_by :full_address
end

class Item < ActiveRecord::Base
    belongs_to :address
    geocoded_through :address
end

Now if I do Address.near, it functions normally:

addresses = Address.near('12345 sample st, CA, 12345', 5, :order => :distance)

addresses.first.distance   ## returns a float for me

Now on the other hand if I did Item.near (the model that is using geocoded_through), calling 'distance' always returns nil:

items = Item.near('12345 sample st, CA, 12345', 5, :order => :distance)

items.first                                       ## returns nil
items.map(&:distance).compact    ## returns [] 

Tried it in sqlite and mysql

alexreisner / vpuzzella Any ideas?

@Ozkar
Ozkar commented Oct 9, 2011

Update: With geocoded_through, the array of returned items IS CORRECTLY sorted by distance, but that distance float just isn't automatically assigned to each item for some reason

My temporary solution was this

add attr_accessor :distance to Item:

class Item < ActiveRecord::Base
    belongs_to :address
    geocoded_through :address

    attr_accessor :distance
end

Then manually assign the distance to each item in the array afterwards using distance_to()

@items = Item.near(@address, @within, :order => :distance)

@items.each{|item| item.distance = item.distance_to(@address) }

Super inefficient I'm sure, but it will have to do for me for now, hopefully one of you guys figures out how to do it properly =)

btw, thx for your hard work guys, really appreciate it!

@vpuzzella

Cool, I'll see if I can get this stuff merged in. Sorry, ultra busy these days... Patches are welcome!

@Ozkar
Ozkar commented Oct 10, 2011

Ahhh nooo dont merge my stuff in, it was just a hack i did to get distance to show up =|

I'm not sure maybe there is a better, more efficient way then this =|

Does anyone know how where in the code the Model is suppose to get assigned the 'distance' value?

@jmsunseri

Thanks!!!!! need to get this into production

@tomtastico

Any chance to get this into master? I'm using it extensively in my app and I'm having to bundle Geocoder from vpuzzella's fork since 5 months! Would love to have geocoded_through + all the latest improvements on Geocoder, but until this is integrated I'm stuck using vpuzzella's outdated fork.

geocoded_through has worked beautifully for me so far. Distance was correctly assigned for each item as well. Please get it merged!

@vpuzzella

@raymccoy: I tried to rebase upstream mater, but there are too many conflicts for me to deal with at this time. Feel free to fork and have a crack at it.

@vpuzzella

@raymccoy: I merged upstream master in so the fork is no longer outdated. I didn't have time to thoroughly test the merge so you'll have to confirm that everything is working as it should.

Sorry, I'm just too busy these days....

@tomtastico

Thank you! Unfortunately it's not working now. When using Model.near it tries to use the (non-existing) model's latitude and longitude db columns instead of the colums from the geocoded_through model.

Don't worry, will fork yours and try to get it working with the current codebase, thanks for your time!

@vpuzzella

@raymccoy: Sorry about that. Let me know when you have a fix and I will merge it.

@tomtastico

@vpuzzella unfortunately it seems the changes are too substantial to attempt a fix from your version, so in the end I have forked the main repo and I'm adapting your changes to the new codebase from scratch.

I already have it working for what I use in my app (Model.near), but I'm trying to make it work for all other methods, i.e. Model.geocode, Model.to_coordinates and other methods that are failing right now when using them on a model with geocoded_though

@vpuzzella

@raymccoy: I made another feeble attempt to fix the problem. :) It's totally untested because I don't have access to my proper dev env at the moment.

@tomtastico

Thanks! But it's still broken, now it spits:

undefined local variable or method `through' for #<Class:0x000000045fe868>

When using Model.near. This is the relevant part of the trace

/lib/geocoder/stores/active_record.rb:237:in `default_near_scope_options'
/lib/geocoder/stores/active_record.rb:144:in `full_near_scope_options'
/lib/geocoder/stores/active_record.rb:96:in `near_scope_options'
/lib/geocoder/stores/active_record.rb:37:in `block (2 levels) in included'
@tomtastico

Great, that seems solved, but now it fails in the SQL query, it is built wrong.

It is doing "model_table.through_table.latitude" instead of "through_table.latitude" for the selects. I believe this happens where there is #{table_name}.#{lat_attr} in the code.

Same for longitude, for the record :)

@tomtastico

Yup, I changed all #{table_name}.#{lat_attr} to #{lat_attr} and .near seems to work now :) Will try other methods to see that all is working.

@tomtastico

Tried .geocode, .to_coordinates and .nearbys and they don't work.

I think commit 726fc47 messes things up. geocoder_options[:latitude] is used sometimes for accessing the database, but other times it is used to access the model's attributes, for example

  def to_coordinates
    [:latitude, :longitude].map do |i|
      if assoc = self.class.geocoder_options[:through]
        send assoc.name
      else
        self
      end.send self.class.geocoder_options[i]
    end
  end

So in there, when using geocoded_through is trying to access Model.table_name.latitude, instead of Model.through_relation.latitude.

In my case, my through model is "Address", so instead of doing Model.address.latitude it is doing Model.adresses.latitude (adresses being the table name) and fails.

I think it is safer to leave geocoder_options[:latitude] alone and use

through_table = geocoder_options[:through] ? geocoder_options[:through].table_name + '.' : ''

"POWER(SIN((#{latitude} - #{through_table}#{lat_attr}) * PI() / 180 / 2), 2) + "
[...]

In every method where lat_attr and lon_attr are used. Not very DRY I know...

Can do it for you and pull request if you don't want to waste more time with me! :)

@vpuzzella

@raymccoy Please go ahead and make the necessary changes :) Send me a pull request when you're done. Thank you for handling this!

@tomtastico

For the record:

Model.near, Model.to_coordinates, Model.geocode and Model.nearbys work now for geocoded_through models.

Some of these methods work only if you map latitude and longitude to the geocoded_through model manually, i.e.

has_one :address
geocoded_through :address

def latitude
  address.latitude
end

def longitude
  address.longitude
end

Docs would need to mention this if this pull is accepted.

@jmsunseri

When will 1.1.2 be released with this feature?

Sincerely

Justin Sunseri
Please don't print this e-mail unless you really need to.

On Fri, Mar 9, 2012 at 2:50 AM, raymccoy <
reply@reply.github.com

wrote:

For the record:

Model.near, Model.to_coordinates, Model.geocode and Model.nearbys
work now for geocoded_through models.


Reply to this email directly or view it on GitHub:
#66 (comment)

@tibbon
tibbon commented Apr 16, 2012

+1 for this feature. Very needed for me.

@ehtb
ehtb commented May 20, 2012

+1 as well, thanks for the work!

@dts
dts commented Nov 19, 2012

+1 - when will this be released?

@kmmndr
kmmndr commented Feb 18, 2013

+1 ... great feature !

@mledom
mledom commented Jun 3, 2013

Does anyone know if this was merged in?

@ncri
ncri commented Aug 29, 2013

I'd also like to see this merged. I used a hack on master for near queries with the location in an associated record:

class User < ActiveRecord::Base
  has_many :inventory_items
  geocoded_by :postal_code
end

class InventoryItem < ActiveRecord::Base
  belongs_to :user

  def self.near location, distance
    scope_options = User.send(:near_scope_options, location[0], location[1], distance, units: :km)
    joins(user).
    where(scope_options[:conditions]).
    select('inventory_items.*').
    select(scope_options[:select]).
    order(scope_options[:order])
  end

end

Works well for my simple use case.

@h4rrison-james

+1, would be good to have this functionality merged with master

@kmmndr
kmmndr commented Sep 2, 2013

I did the merge in my 'merge_all' branch.
Now the code has diverged but it might be possible to cherry pick what you need.

@asecondwill

I'm attempting to use with kaminari for pagination - and i'm getting issues with pagination when using geocodes_through only the first page is returned, but with no pagination links. If i comment out the geocode near part and click through to page 2, then uncomment the line the expected results (or at least some results) are returned. I think its the count part thats going wrong.

logging the active relations count shows it is

{"151.2872618"=>1}

when using geocode_through

but

10

without. Which looks suspicous. Is this an issue with the geocde_through fork, or might i be doing it wrong?

@brandoncc

Is there any chance this will ever be implemented? I must not be the only person who needs this...

@alexreisner
Owner

@brandoncc the code complexity added by this pull request is too great. If someone can come up with a way of adding geocoded_through that affects less of the existing code I'll consider merging it. The good news is that the code may have changed in a way that makes a simpler implementation possible.

Sorry for the slow response on this.

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