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

399 duplicates #529

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
34 changes: 21 additions & 13 deletions app/models/restroom.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,19 @@ class Restroom < ApplicationRecord
using: {tsearch: {dictionary: "english"}},
ignoring: :accents

validates :name, :street, :city, :state, presence: true
if Rails.env != "test"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does the trick for testing, but I don't like it. I think I'd rather fix the tests to truncate the db and simulate production better. We also need a test for the validation itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we be able to "simulate production better" by having real database data from production? I would be able to update db/export.csv with actual, recent data as it exists in production, if that would help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeeDeeG I think that would make sense. I'd like to also work on killing this conditional though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, also FYI. Changing the csv won't fix this issue. There's some sort of hook before the save that tries to geolocate these. I'm not sure what forces it to happen, but it is different than the uniqueness check.

validates :name, :street, :city, :state, presence: true
validates :street, uniqueness: {scope: [:city, :state], message: "is already registered"}
end

attribute :force_geocoding

geocoded_by :full_address
after_validation :perform_geocoding
before_validation :perform_geocoding, if: ->(obj){ require_geocoding? }
before_validation :reverse_geocode, if: ->(obj){ require_geocoding? }

reverse_geocoded_by :latitude, :longitude do |obj, results|
if geo = results.first
obj.name = geo.address
obj.street = geo.address.split(',').first
obj.city = geo.city
obj.state = geo.state
Expand All @@ -46,8 +51,13 @@ class Restroom < ApplicationRecord
scope :created_since, ->(date) { where("created_at >= ?", date) }
scope :updated_since, ->(date) { where("updated_at >= ?", date) }

def require_geocoding?
return false if Rails.env == "test" unless force_geocoding
full_address.present?
end

def full_address
"#{street}, #{city}, #{state}, #{country}"
[street, city, state, country].compact.join(",")
end

def rated?
Expand All @@ -71,15 +81,13 @@ def self.text_search(query)

private

def strip_slashes
['name', 'street', 'city', 'state', 'comment', 'directions'].each do |field|
attributes[field].try(:gsub!, "\\'", "'")
end
def strip_slashes
['name', 'street', 'city', 'state', 'comment', 'directions'].each do |field|
attributes[field].try(:gsub!, "\\'", "'")
end
end

def perform_geocoding
return true if Rails.env == "test"
return true if ENV["SEEDING_DONT_GEOCODE"]
geocode
end
def perform_geocoding
geocode
end
end
7 changes: 4 additions & 3 deletions db/seeds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

Restroom.transaction do
CSV.foreach('db/export.csv') do |row|
Restroom.create(
restroom = Restroom.new(
:name => row[1],
:street => row[3],
:city => row[4],
Expand All @@ -21,7 +21,8 @@
:comment => row[12],
:latitude => row[8],
:longitude => row[9],
:country => row[6]
:country => row[6],
)
restroom.save(validate: false)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring validation during seeding seems reasonable, since those don't really change ever.

end
end
end
26 changes: 26 additions & 0 deletions lib/tasks/reverse_geocode.rake
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
namespace :db do
desc "Reverse geocode each restroom's address (use db:reverse_geocode[dry_run] to preview changes)"
task :reverse_geocode, [:dry_run] => [:environment] do |t, args|
args.with_defaults(dry_run: false)

if args.dry_run
puts "Dry running reverse_geocode"
else
puts "Running reverse_geocode"
end

puts

Restroom.all.each do |restroom|

previous = restroom.full_address
restroom.reverse_geocode

if previous != restroom.full_address
puts "ID: #{restroom.id}\t#{previous}\n"
puts "\t\t\\==> #{restroom.full_address}\n\n"
restroom.save unless args.dry_run
end
end
end
end
2 changes: 1 addition & 1 deletion setup/entry
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
set -e

# Set up the database for development, and seed with placeholder data from `db/export.csv`
SEEDING_DONT_GEOCODE="true" rails db:setup
rails db:setup

exec "$@"
8 changes: 8 additions & 0 deletions spec/factories/restrooms.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,13 @@
state 'CA'
country 'US'
end

factory :weird_restroom do
name 'Weirdly formatted restroom'
street '123 Example Street'.upcase!
city 'San Francisco'.upcase!
state 'CA'
country 'US'
end
end
end
8 changes: 8 additions & 0 deletions spec/models/restroom_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,12 @@
it { expect(Restroom.new.changing_table?).to be false }
it { expect(Restroom.new(changing_table: true).changing_table?).to be true }
end

describe "reverse geocoding" do
it "should normalize weird addresses" do
weird_restroom = build(:weird_restroom, force_geocoding: true)
weird_restroom.save
expect(weird_restroom.full_address).to eq "123 Example St.,San Francisco,California,US"
end
end
end
30 changes: 30 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,33 @@
}
]
)

Geocoder::Lookup::Test.add_stub(
"123 EXAMPLE STREET,SAN FRANCISCO,CA,US", [
{
"latitude" => 37.7749295,
"longitude" => -122.41941550000001,
"address" => "123 Example St., San Francisco, CA, USA",
"city" => "San Francisco",
"state" => "California",
"state_code" => "CA",
"country" => "United States",
"country_code" => "US"
}
]
)

Geocoder::Lookup::Test.add_stub(
[37.7749295, -122.41941550000001], [
{
"latitude" => 37.7749295,
"longitude" => -122.41941550000001,
"address" => "123 Example St., San Francisco, CA, USA",
"city" => "San Francisco",
"state" => "California",
"state_code" => "CA",
"country" => "United States",
"country_code" => "US"
}
]
)