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

Make sure users enter correct city when creating ads #107

Merged
merged 37 commits into from
Jan 6, 2021

Conversation

vlado
Copy link
Owner

@vlado vlado commented Jan 5, 2021

This is a follow up on #102

@mmorava I tested your changes with production data locally and I had to make some changes. I think that it is almost ready for merge (see below for the only issue I've found). Notable changes that I made:

  • Splitted migration to 2 files, import cities and counties, connect them with ads (will probably break with. your current db so rollback or recreate the db).
  • Made sure migration works with production data.
  • Added new city (Zagreb) and New county (Inozemstvo) to be able to store foreign cities that are already used in ads.
  • Cities filter now only has cities that are used in ads
  • Tried to switch cities to grouped select but it has rendering issue. Maybe you can know what is wrong. If it is too hard I suggest we use standard select.

@vlado vlado self-assigned this Jan 5, 2021
@vlado vlado temporarily deployed to potres-features-filter--ovc3gb January 5, 2021 10:07 Inactive
@vlado
Copy link
Owner Author

vlado commented Jan 5, 2021

I've copied production database to review app on https://potres-features-filter--ovc3gb.herokuapp.com. It seems to me that data is ok but not sure why the select2 plugin is not working (no JS errors)?

@mmorava @vfonic @radanskoric @shime @berislavbabic @Xenosb @Macxim Any ideas what could be wrong?

UPDATE: forgot to say that locally everything works ok for me.

@vlado vlado temporarily deployed to potres-features-filter--ovc3gb January 5, 2021 10:39 Inactive
Copy link
Collaborator

@vfonic vfonic left a comment

Choose a reason for hiding this comment

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

This is still failing on the first city it cannot find in the db (in my case it failed so many times on "Krizevci". Perhaps we should add it to the cities_mapping.yml?

We should also get rid of NormalizeCityName that's no longer in use as well as cities_mapping.yml and ads.rake which calls cities_mapping.yml and will no longer be relevant after this PR.

EDIT: Nevermind, my seed data had Krizevci: city: %w[Slatina Neum Makarska Varaždin Varazdin Bibinje Zagreb Križevci Krizevci].sample,
Glad to hear everything works well with live site data.

app/models/ad.rb Outdated Show resolved Hide resolved
db/migrate/20210103181832_connect_ad_with_city.rb Outdated Show resolved Hide resolved
app/assets/stylesheets/cities.scss Outdated Show resolved Hide resolved
app/helpers/ads_helper.rb Outdated Show resolved Hide resolved
app/helpers/cities_helper.rb Outdated Show resolved Hide resolved
db/migrate/20210103181821_seed_locations.rb Show resolved Hide resolved
@vfonic
Copy link
Collaborator

vfonic commented Jan 6, 2021

@vlado I'll have a look now

@vfonic vfonic temporarily deployed to potres-features-filter--ovc3gb January 6, 2021 10:59 Inactive
@vfonic vfonic temporarily deployed to potres-features-filter--ovc3gb January 6, 2021 11:20 Inactive
@vfonic vfonic temporarily deployed to potres-features-filter--ovc3gb January 6, 2021 11:24 Inactive
@vfonic vfonic force-pushed the features/filter-by-county-and-city branch from c8c252e to fcdbfd9 Compare January 6, 2021 11:26
@vfonic vfonic temporarily deployed to potres-features-filter--ovc3gb January 6, 2021 11:26 Inactive
@vfonic
Copy link
Collaborator

vfonic commented Jan 6, 2021

@vlado this should be ready for review: https://potres-features-filter--ovc3gb.herokuapp.com/

Here's the list of changes I've made:

  • Hide select2 dropdown arrow
    I figured that's the easiest. When dropdown opens, the arrown down stays as arrow down unfortunately. But it was too much work to change it to arrow up as the arrow element comes from Bulma and "is-expanded" is set on an element deeper in the DOM, so the only way to make this work would be to create a stimulus controller to handle this, so I chose not to do all that. :)
  • Fix checking if city filter is applied
    We were still checking params[:city] while it should be params[:city_id]
  • Added stylesheet_pack_tag to prod environments
  • Match select2 styles with Bulma dropdown styles
  • Make sure select2 uses HR language translations
  • New ad: Make sure users can type unaccented city names in search
    This uncovered this new issue:
    Screen Shot 2021-01-06 at 12 23 02
  • New ad: Prevent showing "No results" if user just opened city search box

EDIT: Nevermind, there IS a city of Sibenik in Bjelovarsko-bilogorska apparently.

@vfonic vfonic self-requested a review January 6, 2021 12:36
…county-and-city

# Conflicts:
#	app/controllers/ads_controller.rb
#	app/models/ad.rb
#	app/views/ads/new.html.erb
#	config/routes.rb
#	db/schema.rb
@vfonic vfonic temporarily deployed to potres-features-filter--ovc3gb January 6, 2021 15:04 Inactive
@vfonic
Copy link
Collaborator

vfonic commented Jan 6, 2021

@vlado please have a look. I believe this is ready to be merged.

@vfonic
Copy link
Collaborator

vfonic commented Jan 6, 2021

@vlado I take that back. :) One sec. I'm fixing some issue.

@vfonic vfonic temporarily deployed to potres-features-filter--ovc3gb January 6, 2021 15:14 Inactive
@vfonic
Copy link
Collaborator

vfonic commented Jan 6, 2021

@vlado all good. We should be able to deploy this to live site now.

@vlado
Copy link
Owner Author

vlado commented Jan 6, 2021

@vfonic looks perfect

I'll deploy later tonight.

  • Put site to maintenance mode
  • Import production database locally
  • Run migrations locally
  • Add mappings for newly added cities
  • Deploy to production when all is ok locally
  • Switch off maintenance mode (if needed after deploy)

@vlado vlado temporarily deployed to potres-features-filter--ovc3gb January 6, 2021 21:57 Inactive
@vlado vlado merged commit a76101b into master Jan 6, 2021
@vlado vlado deleted the features/filter-by-county-and-city branch January 6, 2021 21:58
@vlado
Copy link
Owner Author

vlado commented Jan 6, 2021

@mmorava @vfonic This is not deployed and working fine on production. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants