Upgrade to rails 5 #126

h-lame · 2017-05-23T11:25:41Z

Best reviewed commit-by-commit.

Still one dep warning at startup for kaminari, but doing the jump to 1.0 to avoid doesn't seem worth it.

For: https://trello.com/c/u8hJdr3O/161-migrate-to-rails-5-need-api

This is a first stepping stone to going up to rails 5. We've run `rake rails:update` to standardise config files and make sure we have all the new initializers too.

This avoids a deprecation warning from minitest about preferring to use assert_nil when comparing against nil. It also makes the users in test scenarios more like real users who will have emails.

13.0.0 is the version with rails 5 compatibliity, but the latest (13.2.0) doesn't have anything dangerous to us.

Latest is 1.x but that introduces many changes we don't want to deal with and v0.17 is enough for rails 5 compatibility. This does introduce a deprecation warning about mongoid functionality being extracted to a separate gem in the next release of kaminari, but it's a one-time message at startup so I think we can live with it for now. It'll serve as a pointer for future work when we do decide to upgrade to kaminari 1.x.

Mongoid 6.x brings rails 5 compatibility so we'll want to jump to that eventually, but we wanted to do it slowly.

We need to upgrade these in lockstep because mongoid 5 doesn't support rails 5 and mongoid 6 doesn't support rails 4. We've run `rake rails:update` to get new config file details and also compared with railsdiff to get details not handled by the update script.

We don't have any assets in this app because it's an API app. We don't include sprockets and in rails 5 this means we don't have the `config.assets` object to configure. Much like the default active_record configuration options, we comment these assets ones out instead of removing them entirely to make it easier to update in the future.

tuzz

A few minor suggestions. LGTM 👍

tuzz · 2017-05-23T15:18:28Z

app/models/need.rb

+      @during_record_revision_callback = 'yes'
+      record_revision "update"
+      @during_record_revision_callback = nil
+    end


Perhaps we could wrap this behaviour?

prevent_additional_callbacks { record_revision "create" }

Oh, yes; good call. Makes it more obvious, will do.

Done in a rebase.

tuzz · 2017-05-23T15:19:33Z

app/models/need_revision.rb

@@ -8,7 +8,7 @@ class NeedRevision

  default_scope -> { order_by(:created_at.desc) }

-  belongs_to :need
+  belongs_to :need, touch: false, inverse_of: :revisions


If you take the touch: false away, does that re-introduce the problem?

TBH, I dunno. I'll try it and see (I was trying all the things in one go). It'll be better to introduce the smallest change that avoids the problem.

Done in a rebase - it all still worked without touch: false so I dropped it.

tuzz · 2017-05-23T15:21:00Z

Gemfile

@@ -34,7 +34,7 @@ else
  gem 'gds-sso', '13.2.0'
 end

-gem 'airbrake', '~> 4.3.4'
+gem 'airbrake', github: 'alphagov/airbrake', branch: 'silence-dep-warnings-for-rails-5'


Do you know if the intention is for this branch to stay around for a while? I wonder if we could wrap its silencing behaviour in something that checks if the Rails version is 5 and then merge it into master?

This is probably out of scope for this upgrade, though.

I think it's a stopgap until we move off errbit and onto something else entirely. Be totally worth having this discussion in a wider forum though.

tuzz · 2017-05-23T15:28:20Z

FYI, I think you rebase-pushed so a couple of comments are showing as 'outdated'

After upgrading to rails 5 and mongoid 6 saving a need caused an infinite callback loop. Saving a Need causes one of the after_create/after_update callbacks that build a new NeedRevision to run. Saving this object in turn causes the need itself to re-save (perhaps as part of a touch event?). This then triggers another after_update callback to run and build a new NeedRevision, which in turn ... To avoid this we make sure we set inverse_of arguments so we are always dealing with the same objects and then add guard clauses round the call to create the NeedRevision in Need. If we're alread inside a callback we won't run it again. Once we successfully create the NeedRevision we discard the guard and fall out of the callback.

This silences deprecation warnings for rails 5.

The `render :nothing` method is deprecated in favour of `head` in Rails 5. We also prefer to use names instead of http status codes to make it easier to understand what's going on.

This means we make sure we use the strong parameters API where we can, and also that we call `.to_h` on the outcome of using the API to convert from the rails 5 parameter object to the hash we used to get. We also have to deal with differences for how empty params are parsed. Before rails 5 a query string like `?foo=&bar=baz` would be parsed into `{ foo: nil, bar: 'baz' }`, but in rails 5 it is parsed into `{ foo: '', bar: 'baz' }`. Ultimately this means checking values with `.present?` instead of relying on nil checks.

Instead of `post :action, param_1: value_1, param_1: value_2` rails 5 prefers `post :action, params: { param_1: value_1, param_2: value_2 }` which is more explicit.

In Rails 5 belongs_to associations in activerecord have a default validation to make sure the object at the other end of the association is present. Mongoid 6 also introduces this functionality for parity with activerecord. We didn't have this validation before, and it's not clear to me if we should have it, but it's easy enough to disable the functionality in an intializer.

Nothing controversial, mostly spacing.

We can use the buildProject() script without specifying all our own things. Also we delete the scripts for the old jenkins infrastructure.

h-lame · 2017-05-23T16:07:23Z

@tuzz fixed what you said in a rebase; if you want to re-look.

h-lame · 2017-05-24T11:13:06Z

Tested the branch on integration and checked that whitehall can still show needs in the admin. I can't test metadata-api as it's broken.

h-lame added 7 commits May 22, 2017 11:19

Upgrade to latest rails 4.2

29eb784

This is a first stepping stone to going up to rails 5. We've run `rake rails:update` to standardise config files and make sure we have all the new initializers too.

Add email to factory built users

7d21d05

This avoids a deprecation warning from minitest about preferring to use assert_nil when comparing against nil. It also makes the users in test scenarios more like real users who will have emails.

Use Rails 5 compatible version of gds-sso

8a1dbab

13.0.0 is the version with rails 5 compatibliity, but the latest (13.2.0) doesn't have anything dangerous to us.

Upgrade to mongoid 5.x

bbe0b21

Mongoid 6.x brings rails 5 compatibility so we'll want to jump to that eventually, but we wanted to do it slowly.

h-lame force-pushed the upgrade-to-rails-5 branch from 31a8139 to fd9a6d8 Compare May 23, 2017 11:26

tuzz approved these changes May 23, 2017

View reviewed changes

h-lame force-pushed the upgrade-to-rails-5 branch from fd9a6d8 to 575fdd9 Compare May 23, 2017 15:53

h-lame added 9 commits May 23, 2017 15:56

Upgrade to our fork of airbrake 4

3a65f0a

This silences deprecation warnings for rails 5.

Avoid render :nothing, and prefer status code names to numbers

58a0a0c

The `render :nothing` method is deprecated in favour of `head` in Rails 5. We also prefer to use names instead of http status codes to make it easier to understand what's going on.

Use new rails 5 controller testing API

ff2add2

Instead of `post :action, param_1: value_1, param_1: value_2` rails 5 prefers `post :action, params: { param_1: value_1, param_2: value_2 }` which is more explicit.

Use rails 5 API for before action instead of before filter

078cff5

Bump to latest govuk-lint and fix style violations

461739b

Nothing controversial, mostly spacing.

Use new jenkins infrastructure

6052ad9

We can use the buildProject() script without specifying all our own things. Also we delete the scripts for the old jenkins infrastructure.

h-lame force-pushed the upgrade-to-rails-5 branch from 575fdd9 to 6052ad9 Compare May 23, 2017 15:56

Bump database_cleaner to get mongoid 5+ support

2dff4e7

h-lame merged commit 1c3b14c into master May 24, 2017

h-lame deleted the upgrade-to-rails-5 branch May 24, 2017 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to rails 5 #126

Upgrade to rails 5 #126

h-lame commented May 23, 2017

tuzz left a comment

tuzz May 23, 2017

h-lame May 23, 2017

h-lame May 23, 2017

tuzz May 23, 2017

h-lame May 23, 2017

h-lame May 23, 2017

tuzz May 23, 2017

h-lame May 23, 2017

tuzz commented May 23, 2017

h-lame commented May 23, 2017

h-lame commented May 24, 2017

Upgrade to rails 5 #126

Upgrade to rails 5 #126

Conversation

h-lame commented May 23, 2017

tuzz 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

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

Choose a reason for hiding this comment

tuzz commented May 23, 2017

h-lame commented May 23, 2017

h-lame commented May 24, 2017