Rails 5 Upgrade

[dannypaz]

Please view the Rails 5 issue https://github.com/RefugeRestrooms/refugerestrooms/issues/287 for details or the supporting documentation for more info.

Description

This PR upgrades the refugerestrooms application to Rails 5 (awww yeee).

Changes

• Upgraded a boatload of gems to satisfy railties and rake requirements
• Removed rack-contrib and monkey patched Rack::JSONP support (temp fix)
• Removed deprecation for before_filter and changed to before_action
• Removed deprecation in passing Rack::Cors as a string to middleware
• Removed top_cities code from PagesControllers (part 2 of https://github.com/RefugeRestrooms/refugerestrooms/pull/327)

TODO

• Fix PagesController (not able to load site)
• remove requires from asset pipeline in favor of using @imports w/ rails-sass (please see rails-sass documentation for more info)
• bump application version numbers (or major versions)
• Make sure test suite works
• Update rails config files w/ bin/rails rails:update
• Update/Upgrade/Import remaining gems from Rails 5 gemfile
• Try to deploy to heroku w/ Cedar-14 (@teagan)

Test Suite:

# Rspec suite
Finished in 2.27 seconds (files took 6.29 seconds to load)
48 examples, 0 failures

Randomized with seed 60083

Coverage report generated for RSpec to /Users/dev/Documents/refugerestrooms/coverage. 453 / 520 LOC (87.12%) covered.

# Cucumber tests
9 scenarios (9 passed)
26 steps (26 passed)
0m34.397s

Screenshot

[cllns]

could we use https://github.com/crohr/rack-jsonp instead of monkey patching JSONP ourselves?

[dannypaz]

@cllns We can. FWIW, It is the same exact code -_- (I copied directly from that library specifically because the repo is stale)

[mi-wood]

@dannypaz thanks for submitting this! I'll try to get to it soon. @cllns since I'm pretty out of sync with rails these days, do you mind doing a review when you get a chance? @tkwidmer thinking we should wait to fix the maps API issues we're having before this? Just because that seems important to fix before putting more changes in. Thoughts?

[ghost]

@mi-wood I've updated the PR w/ current develop so we can start testing. @cllns let me know if you have any questions RE: the changes made for Rails 5 (it was pretty straight forward).

@mi-wood is there a PR or issue up for the maps API issues?

[ghost]

Also, FWIW... I still cannot figure out this testing error as I receive the error on develop and this branch. Can someone take a look at that?

[ghost]

@mi-wood Im taking a look at the maps issues w/ this PR and it looks like the only thing that is broken is the text search on the home page.

You can view the project on my test container here: https://secure-coast-64861.herokuapp.com/

UPDATE Also, I was able to get the text search working by running the following in JS console

var input = document.getElementById('search'); new google.maps.places.SearchBox(input)

ill have to look into it further, but not sure why the search isn't getting instantiated.

[mi-wood]

@dannypaz I added an API key for the maps, so that should be fine. The testing error is actually the same one as the homepage search. I wouldn't block this PR on it, although we should fix it before updating the app to point to the new deploy.

[mi-wood]

I'm mostly ok with this PR. Lots of config/deprecated syntax changes it seems. I'd just like a second pair of eyes before merging it.

[tkwidmer]

I'm going to defer to @cllns and @mi-wood when it comes to this PR.

[DeeDeeG]

~EDIT: I tested this on my machine, and none of the style changes I wrote about in this comment are showing up. So my comment probably only applies to the version hosted up at https://secure-coast-64861.herokuapp.com/ /EDIT~

(EDIT 2: I had the wrong github branch (develop) checked out when I tested above. This comment applies after all.)

This PR makes some small css style changes, probably unintentional. Example: the search/nav buttons look slightly different.

I'm looking into what changed in particular, and how to prevent it from changing. Might just come down to which stylesheets end up imported nearer to the top of the compiled application.scss, (less prioritized) vs toward the bottom of the css file (more prioritized), or something like that.

I think the @\import statements at the top of app/assets/stylesheets/application.css might need to be reordered to match develop, and see if that makes the styling the same as develop. I'll test this when I get the chance.

Another observation: There are 369 more style rules in the compiled application.css when visiting this PR branch (link), than when visiting refuge-production (link).

[DeeDeeG]

I think the application.scss changes could/should be reverted. Copying and pasting application.scss from develop into this branch solves all styling issues (that I can find) from my previous comment.

The main, most important step is leaving out @import active_admin. But there are some miscellaneous styles that get lost in the shuffle that I can't figure out. Reverting the whole file would be the easiest fix.

A lot of the original file from develop is wrapped in / / which looks like comment-out syntax. But all those require statements do get executed. I don't think there is a distinct benefit to switching to @import statements, right?

Some discussion here stackoverflow indicates that they work differently than *= require. I would lean toward recommeding any switch to @import statements be separated into its own PR since it has the potential to alter which rules get used when, and IMO that is a lot of testing work to ensure no unwanted changes occur.

(edit: I learned some things since I originally wrote this comment. For the record, I am actually pretty sure our SCSS files are pulled in on a page-by-page basis, with each page asking for only what SCSS files it needs, by way of our .haml files. Addressing crossed out stuff: The comment out syntax does apparently comment out/disable stuff. And @import statements can add css to a stylesheet that doesn't need to have it. Our scss files are optimized already, with regards to what gets imported when.)

[DeeDeeG]

It would be nice to see this added, as far as I'm concerned. I did test it (with vagrant) and it seems to work well.

I believe the TODO is:

• add Rspec tests (aka "specs") for Rack-jsonp to our repository, (or instead, just add rack-jsonp to our project as a gem via our Gemfile)
• remove files related to ActionCable,
• and consider undoing functional changes to the SCSS files. (However, renaming _mobile.css.scss -> _mobile.scss is fine and gets rid of some warnings, so I'm all for that.)

Edit: I read on Wikipedia that jsonp is insecure, and is being replaced by CORS (link to wiki article). Can we use rack-cors (link) instead? It's been updated as recently as February this year, as opposed to rack-jsonp, which was updated in 2014.

[dannypaz]

@DeeDeeG +1 to all the suggestions. FWIW, I would much rather switch to rack-cors than install rack-jsonp.

[dannypaz]

@DeeDeeG @cllns @tkwidmer Let me know if this sounds good.

For now, I will install rack-jsonp in this branch. Moving forward, i've created this issue to upgrade to rack-cors since this change is not required to move forward with upgrading to rails (although we should make the change eventually).

Issue can be viewed here: https://github.com/RefugeRestrooms/refugerestrooms/issues/341

UPDATE I've also reverted application.scss which should mean that we are GTG. @DeeDeeG @cllns @tkwidmer Let me know what you think

[DeeDeeG]

Looks good to me.

I poked around on all the user-facing pages, and (as far as I can tell) they all function and appear identical to develop branch.

[DeeDeeG]

As a minor thing, we are using the "restrooms/new" styles twice. There are two (very easy) ways to fix this:

• we can drop "@import restrooms/new" from the beginning of restrooms.scss ~OR~
• ~rename new.scss to _new.scss (the underscore turns it into a "partial", which Sass knows to not to send to users, on its own. Partials only get activated if they are called for with @import. partials actually get rendered into the file(s) that import(s) them, before the server sends the processed CSS out to users. Since new.scss is currently imported by restrooms.scss, users would get it, and only get it once, if new is converted to a partial.)~ going to recommend dropping the import as in the above bullet point, because the restrooms/new styles aren't needed on our results page, which they would be included in if new was turned into a partial and imported.

~(on a tangent, this is for another issue or PR: I think we might be double-importing SCSS files elsewhere in the repo, now that I take a closer look. It would be nice to tidy that up, and might help with performance especially during pageload/page refreshes.)~ (Nope.)

Edit: Minor thing number two: Travis is having trouble testing this and I don't get why, but it wants the updated Gemfile.lock added to the repo, from after vagrant up finishes.

[tkwidmer]

I dont know if we should commit this until we figured out why Travis isn't able to run it

[DeeDeeG]

I don't strictly speaking know why this happens, as in why bundler has this as an issue that can happen, because it seems like a weird design decision, but I do know what the problem is/how to fix it. It happened to me once. Just need to get an updated Gemfile.lock, which can even be done by deleting the old Gemfile.lock and then running bundle install. (It's supposed to be a security mechanism, but I can't image a scenario where it would be helpful. It's probably just that we use version control, and accidentally mismatching the Gemfile.lock to the current Gemfile happens much more easily with version control than without. :shrug:) So it genuinely confuses me why this is a thing, but I can describe the fix.

For our situation here (background info first):

Travis says the Gemfile.lock is out of date and needs to be checked in with "version control", aka git for our pusposes.

Gemfile.lock is a file Ruby (or bundler?) makes, keeping track of the list of dependencies (gems) that were pulled in (during the last successful run, i.e. finished without errors) of bundle install. So to fix it, running vagrant up will call the setup script, which runs bundler... Which produces the updated Gemfile.lock.

That said, the steps to fix this are:

1. Run vagrant up (This will slightly change Gemfile.lock, because we added the rack-jsonp gem since last time a copy of Gemfile.lock was uploaded to our repo)
2. Find Gemfile.lock in the working "refugerestrooms" git folder, and add the changed version to be tracked by git (git add Gemfile.lock)
3. Commit the change and push to github. (git commit [other commandline parameters here]; git push)

I know I might be overexplaining, but it's a safer bet than underexplaining, so I tend to lean toward overexplaining.

[DeeDeeG]

Also, here's my updated Gemfile.lock, which could just be copy-pasted and committed, if that's easiest.

Link to Gemfile.lock: https://gist.github.com/DeeDeeG/c199a73a6a9b332d99afaf9f30f3198f

[dannypaz]

@DeeDeeG i'm so embarrassed. I had the gemfile.lock changes still stashed. They are now commited :-).

UPDATE: I've also included a commit where i've removed the @import restrooms/new from restrooms.scss

[DeeDeeG]

@dannypaz it happens to the best of us. :)

So as expected, Travis is working again. :tada: No build failures related to this PR.

(Digging into what is unrelatedly failing: Interestingly, after a quick glance, we don't seem to be failing on the Google Maps unauthorized key issue right now. We only seem to be failing on a test for top cities, which makes sense knowing that the feature was completely removed. If we remove the top cities tests, we might be back to having passing builds!)

[dannypaz]

For record keeping, here is the PR @DeeDeeG is referencing: https://github.com/RefugeRestrooms/refugerestrooms/pull/327

I've removed the offending test, so we should have a passing build now. Was this the correct solution? @DeeDeeG

[DeeDeeG]

That's definitely the right part of the file to delete, but still a couple of the top cities tests are left in the commit. (This can be fixed again without adding extra commits, by making changes, git adding the changed file, and using git commit --amend, if you like.)

There's a lot of nested "do ... end" in the file, so it's not very readable to start with, and therefore a bit hard to edit?

Anyways: The only way I know to do this right, by hand, is to delete the innermost "do ... end" block, and the next innermost "do ... end" block, then move out one level, delete some more blocks one "do ... end" at a time, until the whole top cities test is gone.

I treat "do" as if it was "{", and "end" as if it was "}" because they mark blocks of code like I would expect brackets to.

[dannypaz]

@DeeDeeG Ahh. OK I think I understand correctly now. I think these tests and related controller code should have been removed with https://github.com/RefugeRestrooms/refugerestrooms/pull/327 but that is OK.

I've added a new screenshot to the description and have added removed top_cities code to the list of changes.

[dannypaz]

TY for the help. Looks like we are all green.

[mi-wood]

Awesome work ya'll! I should have some time to take a look at this today

[DeeDeeG]

I expect to be busy soon, and I'm new to slack, but sure, I can see what it's all about. Thank you.

[mi-wood]

I'm mikena@woodmail.me if you want to shoot me your email, I'll add you :)

[tkwidmer]

🎉 thank you so much @dannypaz