[WIP] Improve filtering by adding county/city models #102

mmorava · 2021-01-03T19:04:10Z

WIP for improving ad search by city/county

As a source of city/county data I used: https://uprava.gov.hr/o-ministarstvu/ustrojstvo/uprava-za-politicki-sustav-i-organizaciju-uprave/lokalna-i-podrucna-regionalna-samouprava/popis-zupanija-gradova-i-opcina/846

For the sake of simplicity i did not cater for "opcina/grad" difference so all locations are treated like Cities but if needed I can add support for that. I propose that we drop the "City" name and call it just 'Location' which belongs to a County

TODO: Proper PR description :)

vfonic

I'm not sure if only covering the cities, and not also villages would be good enough. The hardest hit places are actually couple of villages around Petrinja. It would be misleading if the user would have to select "I'm requesting help in Petrinja", while that's actually not true. We could perhaps use your location suggestion to go around this... 🤔

Another issue is that the migration will raise error on live site because City.find_by_name won't find all the names that we currently have. I suggest we fix those manually if we decide to go this route:

        ad.update!(city: City.find_by_name(ad.city_zdel)) # might return nil, but won't fail

db/migrate/20210103181832_connect_ad_with_city.rb

vfonic · 2021-01-03T19:24:55Z

db/migrate/20210103181832_connect_ad_with_city.rb

+      rename_column :ads, :city, :city_zdel
+      add_reference :ads, :city, foreign_key: true, index: true
+      Ad.all.each do |ad|
+        ad.update!(city_id: City.find_by_name(ad.city_zdel).id )


This will fail if the city is not found by the name and we definitely have cities in the database on live site that don't match any city name.

Maybe something like this would be ok

Suggested change

ad.update!(city_id: City.find_by_name(ad.city_zdel).id )

city = City.find_by(name: ad.city_zdel)

if city

ad.update!(city_id: city.id)

else

ad.update!(city_id: City.first.id)

puts "* No matching city for ad #{ad.id}."

end

After @vfonic normalisation I suppose we shouldn't have lot of this and we can correct them manually.

I spoke about this with Vlado, we need to have normalized data first in the DB, any irregularities we can manually change. idea is for this to fail so it alert us we have mismatched data that we can than alter

vlado

Looks promising, left few improvement suggestions. Looking forward for the UI :)

vlado · 2021-01-03T20:32:24Z

db/migrate/20210103181832_connect_ad_with_city.rb

+      rename_column :ads, :city, :city_zdel
+      add_reference :ads, :city, foreign_key: true, index: true
+      Ad.all.each do |ad|
+        ad.update!(city_id: City.find_by_name(ad.city_zdel).id )


Maybe something like this would be ok

Suggested change

ad.update!(city_id: City.find_by_name(ad.city_zdel).id )

city = City.find_by(name: ad.city_zdel)

if city

ad.update!(city_id: city.id)

else

ad.update!(city_id: City.first.id)

puts "* No matching city for ad #{ad.id}."

end

After @vfonic normalisation I suppose we shouldn't have lot of this and we can correct them manually.

vlado · 2021-01-03T20:34:47Z

db/migrate/20210103181832_connect_ad_with_city.rb

+class ConnectAdWithCity < ActiveRecord::Migration[6.1]
+  def change


Suggested change

class ConnectAdWithCity < ActiveRecord::Migration[6.1]

def change

class ConnectAdWithCity < ActiveRecord::Migration[6.1]

Ad = Class.new(ActiveRecord::Base)

City = Class.new(ActiveRecord::Base)

def change

https://guides.rubyonrails.org/v3.2/migrations.html#using-models-in-your-migrations

vlado · 2021-01-03T20:36:56Z

db/migrate/20210103181832_connect_ad_with_city.rb

+    ActiveRecord::Base.transaction do
+      Rake::Task['db:seed_locations'].invoke
+      rename_column :ads, :city, :city_zdel
+      add_reference :ads, :city, foreign_key: true, index: true


Suggested change

add_reference :ads, :city, foreign_key: true, index: true

add_reference :ads, :city, foreign_key: true, index: true, null: false

This won't work immediately, because we have ads in our database and, when we add city_id reference, none of the ads will have city_id that is not null.

We can do:

add_reference :ads, :city, foreign_key: true, index: true # ... do the data migration change_column_null :ads, :city_id, false

I think we should use this as opportunity to clean up data before we have too many ads

db/seeds.rb

vlado · 2021-01-03T20:46:21Z

db/migrate/20210103180930_create_cities.rb

+  def change
+    create_table :cities do |t|
+      t.string :name
+      t.references :county, foreign_key: true


Suggested change

t.references :county, foreign_key: true

t.references :county, foreign_key: true, null: false

vlado · 2021-01-03T21:11:51Z

For the sake of simplicity i did not cater for "opcina/grad" difference so all locations are treated like Cities but if needed I can add support for that. I propose that we drop the "City" name and call it just 'Location' which belongs to a County

I think we can just keep calling them cities, even if they are not real cities.

I'm not sure if only covering the cities, and not also villages would be good enough. The hardest hit places are actually couple of villages around Petrinja. It would be misleading if the user would have to select "I'm requesting help in Petrinja", while that's actually not true. We could perhaps use your location suggestion to go around this... 🤔

From what I see the list @mmorava used has 500+ cities (cities and bigger villages). Full list with smaller villages from https://www.posta.hr/preuzimanje-podataka-o-postanskim-uredima-6543/6543 has 6000+ cities.

I think going with 500+ cities makes things simpler, you can enter village name in the address field Moja ulica 23, Majske Poljane (I suppose this should be enough to geolocate later).
6000+ could also work but not sure if it is worth the effort.

Looking forward for the opinions.

mmorava · 2021-01-04T21:02:53Z

Added UI changes, resolved conflicts

Thanks for the comments, I will have a look at them in the morning and amend the code before I do a proper QA on this card

vlado · 2021-01-05T01:19:03Z

@mmorava Please check #107

mmorava · 2021-01-05T07:14:53Z

deprecated by #107

mmorava added 3 commits January 3, 2021 19:18

Add and seed county/city models

ed53cbe

naive approach to connect ads with cities

5b34517

Update readme and seed files

f5d22cf

vfonic requested changes Jan 3, 2021

View reviewed changes

This was referenced Jan 3, 2021

Normalize city names for easier filtering #41

Merged

Improve city filter #92

Closed

vlado reviewed Jan 3, 2021

View reviewed changes

mmorava added 7 commits January 4, 2021 20:02

Populate City date from post office csv

310a787

CSV formating fix

c0bb6c1

Add select2 to the city search

77053cc

IMplement city.to_s

aa58feb

Fix issues with creating and filtering ads

af97a9a

Fix select2 padding for bulma

373cdfe

Merge branch 'master' into features/filter-by-county-and-city

47b1832

IMplement some of the PR comments

8609a6e

vlado mentioned this pull request Jan 5, 2021

Make sure users enter correct city when creating ads #107

Merged

mmorava closed this Jan 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Improve filtering by adding county/city models #102

[WIP] Improve filtering by adding county/city models #102

mmorava commented Jan 3, 2021 •

edited

Loading

vfonic left a comment

vfonic Jan 3, 2021

vlado Jan 3, 2021

mmorava Jan 4, 2021

vlado left a comment

vlado Jan 3, 2021

vlado Jan 3, 2021

vlado Jan 3, 2021

vfonic Jan 3, 2021 •

edited

Loading

vlado Jan 3, 2021

mmorava Jan 4, 2021

vlado Jan 3, 2021

vlado commented Jan 3, 2021

mmorava commented Jan 4, 2021

vlado commented Jan 5, 2021

mmorava commented Jan 5, 2021

-        ad.update!(city_id: City.find_by_name(ad.city_zdel).id )
+        city = City.find_by(name: ad.city_zdel)
+        if city
+          ad.update!(city_id: city.id)
+        else
+          ad.update!(city_id: City.first.id)
+          puts "* No matching city for ad #{ad.id}."
+        end

		class ConnectAdWithCity < ActiveRecord::Migration[6.1]
		def change

	add_reference :ads, :city, foreign_key: true, index: true
	add_reference :ads, :city, foreign_key: true, index: true, null: false

	t.references :county, foreign_key: true
	t.references :county, foreign_key: true, null: false

[WIP] Improve filtering by adding county/city models #102

[WIP] Improve filtering by adding county/city models #102

Conversation

mmorava commented Jan 3, 2021 • edited Loading

vfonic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vfonic Jan 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlado commented Jan 3, 2021

mmorava commented Jan 4, 2021

vlado commented Jan 5, 2021

mmorava commented Jan 5, 2021

mmorava commented Jan 3, 2021 •

edited

Loading

vfonic Jan 3, 2021 •

edited

Loading