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

Fix empty restrooms db during development #508

Merged

Conversation

DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Oct 8, 2018

Fixes db being empty during development.
Ensures restroom entries from db/export.csv are loaded.

Context

Summary of Changes

  • Remove db:create and db:migrate commands in setup/entry
  • Replace with bundle exec rake db:setup

Checklist

  • Tested Mobile Responsiveness
  • Added Unit Tests
  • CI Passes
  • Deploys to Heroku on test Correctly (Maintainers will handle)
  • Added Documentation (Service and Code when required)

Screenshots

Before

Restrooms search results showing no results and a mostly blank page

After

Restrooms search results showing full page of restroom entries

Fixes db being empty during development.
Ensures restroom entries from `db/export.csv` are loaded.
@DeeDeeG DeeDeeG added the bug label Oct 8, 2018
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Oct 8, 2018

A more-minimal change might be to keep db:create and db:migrate, followed by db:seed.

I don't have time to test that at the moment. But the bundle exec rake db:setup command does work in my testing.

Edit: I have tested with the following commands, after deleting my local db docker image and starting from scratch with regard to the db, and it seems to function exactly the same as the commit above:

rake db:create
rake db:migrate
rake db:seed

I would go with either approach.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Oct 8, 2018

I'm hoping to merge this soon (in whichever configuration of commands we decide to go with) since it makes manual testing easier. It would particularly make the set of pull requests we have queued up easier to test.

@DeeDeeG DeeDeeG mentioned this pull request Oct 8, 2018
5 tasks
@btyy77c
Copy link
Contributor

btyy77c commented Oct 8, 2018

Thanks for this!

Some comments:

  1. In Rails 5 they changed the command from rake to rails. You would need to use rails db:setup now. I think rake db:setup will stop working with Rails 6:
    https://github.com/rails/rails/blob/master/railties/lib/rails/tasks/initializers.rake
    Move the initializers rake task to Rails::Command rails/rails#33631

  2. Just to confirm, this docker-compose file is not used for production, correct? rails db:seed will harm your production database.
    If this is used for production, I would recommend instead running docker-compose run web rails db:setup in your terminal before you run docker-compose up to test things out locally... or creating a separate docker process for production.

  3. Your restroom model has a geocoding method:
    https://github.com/RefugeRestrooms/refugerestrooms/blob/develop/app/models/restroom.rb

def perform_geocoding
      return true if Rails.env == "test"
      geocode
end

I think this is connecting to Google Api maps. When I run it, I get Google Geocoding API error: over query limit. It's not a huge deal, just so you're aware things may not be loading the way you want. I assume it's ok since you're skipping it for testing.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Oct 8, 2018

@btyy77c, thanks again for the informative feedback.

  1. rails vs rake

Sounds right. I had this in the back of my head, but I don't think we've drop the rake command from our project, so I used rake out of habit and not "rocking the boat" just for this small pull request. Might be good to switch every rake in the repo at once to rails just to make sure it all works and get that convention change out of the way for the project.

  1. Just to confirm, this docker-compose file is not used for production, correct? rails db:seed will harm your production database. . . .

I wish I knew this with high confidence. But I have the sense we are not using Docker whatsoever in production.

Production is a Heroku instance. I believe in that environment we get a minimal Linux "slug" that is able to then install ruby and our Gemfile and nodejs dependencies, etc. and generally run through our config files to set up a "native"-like Rails app on "bare" (virtual) metal. As in, no Docker or Vagrant etc.

Here is a file that I believe means there are three separate databases: one for development, one for testing, and by process of elimination, one for production (the "default" db): https://github.com/RefugeRestrooms/refugerestrooms/blob/develop/config/database.yml

Much of our app, especially as it pertains to Heroku, is set up roughly according to these guidelines: https://devcenter.heroku.com/articles/getting-started-with-rails5

  1. Your restroom model has a geocoding method: . . .

I believe this is an inconsequential quirk/bug in our configs. I haven't seen this cause problems for us. I hope/presume it isn't actually counting against our quota. But other than that, this has felt safe to ignore. It would be nice to get rid of those warnings so people don't think something is going wrong, though.

(And the reason this isn't a big deal is that the entries we are loading at that point, from db/export.csv, already have lat/long coordinates. We don't need to geocode right there at all. I'm not sure why it happens to be honest.)


At bare minimum, I' going to switch this pull request to not use bundle exec (seems unnecessary) and to use rails db:[so-and-so] rather than rake db:[so-and-so].

`rake` commands are deprecated, and are being rolled into
the `rails` command.

See:
https://guides.rubyonrails.org/v4.0/command_line.html#rake (rails 4.0)
vs 
https://guides.rubyonrails.org/v5.0/command_line.html#bin-rails (rails 5.0)
vs
https://edgeguides.rubyonrails.org/command_line.html#command-line-basics (rails next)

(Also moves away from `bundle exec`, which is unnecessary in this case.)
@tkwidmer
Copy link
Contributor

tkwidmer commented Oct 8, 2018

This LGTM @DeeDeeG, let's merge it asap since it seems like this is causing you and other people issues.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Oct 8, 2018

Alright. Testing with the latest commit locally just to be sure this works, then will merge.

I think our test environment (and our tests themselves) don't need the db/export.csv entries, so that's why CI didn't catch this.

edit: so for the moment this PR is mostly not able to trip up CI, but CI also isn't telling us anything useful about this being a good PR or not.

@DeeDeeG DeeDeeG merged commit 81bd2d0 into RefugeRestrooms:develop Oct 8, 2018
@tkwidmer
Copy link
Contributor

tkwidmer commented Oct 8, 2018

Awesome. Thanks @DeeDeeG

DeeDeeG added a commit to DeeDeeG/refugerestrooms that referenced this pull request Oct 15, 2018
* setup/entry: Use different command to seed db

Fixes db being empty during development.
Ensures restroom entries from `db/export.csv` are loaded.

side note: command now uses `rails`, not `rake`.

`rake` commands are deprecated, and are being rolled into
the `rails` command.

See:
https://guides.rubyonrails.org/v4.0/command_line.html#rake (rails 4.0)
vs 
https://guides.rubyonrails.org/v5.0/command_line.html#bin-rails (rails 5.0)
vs
https://edgeguides.rubyonrails.org/command_line.html#command-line-basics (rails next)
DeeDeeG added a commit to DeeDeeG/refugerestrooms that referenced this pull request Nov 3, 2018
* setup/entry: Use different command to seed db

Fixes db being empty during development.
Ensures restroom entries from `db/export.csv` are loaded.

side note: command now uses `rails`, not `rake`.

`rake` commands are deprecated, and are being rolled into
the `rails` command.

See:
https://guides.rubyonrails.org/v4.0/command_line.html#rake (rails 4.0)
vs 
https://guides.rubyonrails.org/v5.0/command_line.html#bin-rails (rails 5.0)
vs
https://edgeguides.rubyonrails.org/command_line.html#command-line-basics (rails next)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants