Upgrade rails 5 #258
Upgrade rails 5 #258
Conversation
Dantemss
left a comment
Dantemss
left a comment
There was a problem hiding this comment.
Looks good! I think the 3 important comments are the one about rendering errors in exercises_controller and the 2 about whitespace between a method and the argument list. The rest is all minor.
Gemfile
Outdated
|
|
||
| # Bundle edge Rails instead: gem 'rails', github: 'rails/rails' | ||
| gem 'rails', '4.2.11' | ||
| gem 'rails', '~> 5.2' |
There was a problem hiding this comment.
This is ok for 5.2 because there is no Rails 5.3 (Rails goes from 5.2 to 6.0)
But in general we want to avoid this, as it allows upgrading to the next minor version, which with Rails usually means breaking a lot of things as you've seen.
There was a problem hiding this comment.
Ah good point. Will fix.
Gemfile
Outdated
|
|
||
| # Talks to Accounts (latest version is broken) | ||
| gem 'omniauth-oauth2', '~> 1.3.1' | ||
| # gem 'omniauth-oauth2', '~> 1.3.1' |
There was a problem hiding this comment.
Can remove the commented out one
Gemfile
Outdated
| gem 'mini_magick' | ||
|
|
||
| # Markdown parsing | ||
| # Pinned for Rails 4.X |
There was a problem hiding this comment.
I think this comment can be removed too
Gemfile
Outdated
| gem 'scout_apm', '~> 3.0.x' | ||
|
|
||
| # PostgreSQL database | ||
| # Pinned for rails 4.X |
There was a problem hiding this comment.
Ah hah, but I need to unpin, if I remove the comment. Yay for comments!
There was a problem hiding this comment.
unpinned, and comment deleted.
Gemfile
Outdated
|
|
||
| # Mute asset pipeline log messages | ||
| gem 'quiet_assets' | ||
| # gem 'quiet_assets' |
There was a problem hiding this comment.
Probably this one too. And the 2 pinned comments below.
app/routines/search_exercises.rb
Outdated
| elsif sanitized_versions.empty? | ||
| @items = @items.where(publication_groups: { number: sanitized_numbers }) | ||
| else | ||
| # Combine the id's one at a time using Squeel |
app/routines/search_vocab_terms.rb
Outdated
| next @items = @items.none if sanitized_names.empty? | ||
|
|
||
| @items = @items.where { name.like_any sanitized_names } | ||
| @items = @items.where ( vt[:name].matches_any(sanitized_names )) |
There was a problem hiding this comment.
The space between the where and the parenthesis should be removed too. There's no semantic difference here right now because nothing is chained to the where, but it could become a problem one day.
There was a problem hiding this comment.
did a recursive search for '\(where\|order\)\s(' and you caught them all! (both) Good eye.
app/routines/search_vocab_terms.rb
Outdated
| end | ||
|
|
||
| with.keyword :content do |contents| | ||
| vt = VocabTerm.arel_table |
There was a problem hiding this comment.
Could just put these at the top of the method, since they are everywhere.
There was a problem hiding this comment.
Yeah my peep-hole-optimizer-like conversion process didn't push things to lowest-common-namespace very well. I'll see what I can do. There are currently 150 .arel_table refs in 20 files.
There was a problem hiding this comment.
ok refactored positions of arel_table for a couple of these. I do like to keep the variable defs near their use, but in this case, they get reused a lot, across diff. methods.
There was a problem hiding this comment.
also moved most of them out of the local loop they where in
| end | ||
| alias_method_chain :add_link, :attachments | ||
| alias_method :add_link_without_attachments, :add_link | ||
| alias_method :add_link, :add_link_with_attachments |
There was a problem hiding this comment.
Yeah I spent 1/2 a day on this, and punted. Came this close to understanding how to do it with a module and prepend/super, but … realized this is used only in the deprecated importer, anyways.
| @@ -0,0 +1,7 @@ | |||
| # This migration comes from openstax_accounts (originally 13) | |||
| class AddSchoolTypeToAccountsAccounts < ActiveRecord::Migration[4.2][4.2] | |||
|
Apparently I commented too much so github hid half of my comments, so look for the button to show them above. |
reedstrm
force-pushed
the
upgrade-rails-5
branch
from
bab5964 to
d3f2915
Compare
|
@Dantemss all fixed |
|
The only thing I think it still needs is to add a |
|
Fixed that in ed145b9 (I also unwrapped it onto three lines) |
|
I told code climate that I'm not fixing any of that. :) |
|
This PR landed in v25.0.0 🎉 |
Completed upgrade to 5.2.3
Most of the work was converting squeel based DB queries to a combination of ActiveRecord and arel,
followed by converting to new defaults for model belong_to behavior, and controller spec parameter passing.