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

Upgrade to rails 4 #1958

Merged
merged 85 commits into from Feb 19, 2015

Conversation

Projects
None yet
8 participants
@tekin
Contributor

tekin commented Jan 30, 2015

This updates Whitehall to run on Rails 4.0. For the most part the changes deal with various deprecations and breakages due to changes in Rails' behaviour. There are a few key areas where more comprehensive changes were required:

Localised routing

The whitehall application patches Rails' routing code to support our particular flavour of localisation URLs, for example:

  /government/publications/some-publication
  /government/publications/some-publication.fr
  /government/publications/some-publication.fr.json

This patch had to be rewritten as the routing code had changed significantly in the move from Rails 3 to 4 (73d17ac).

Rails routing bug

There is a bug in the Rails routing code that was causing an issue with some of the localised routing, specifically when generating a localised route with a default format set. A fix has been submitted to Rails, and a monkey patch added to whitehall to cope with this in the meantime (6129a6f).

friendly_id conflict resolution change

The Rails 4 compatible version of friendly_id changes behaviour significantly, particularly around slug conflict resolution. Pre-Rails 4, slug conflicts were resolved by appending a sequence number to the end of the slug. friendly_id now resolves conflicts by appending a UUID to the end (simpler solution and also threadsafe). To maintain the previous behaviour in whitehall, the conflict resolution code in friendly_id is being overridden to generate sequential slugs again (5203d32)

friendly_id magic finders

Rails 4 compatible friendly_id no longer overrides ActiveRecord::Base#find so that it can be passed a slug string as well as an ID number by default. It can be switched back on, but it's not very stable as it requires hooking into ActiveRecord internals (and breaks often with newer versions of ActiveRecord). The preferred method is to use the new friendly scope, for example:

  Model.friendly.find(slug)

To keep the changes in this pull request to a minimum, the magic find override has been enabled (972e039). Ideally some future work would disable it and move all find calls to use the new friendly scope.

Globlize feature/Rails bug workaround

A change was made to the globalize gem that means a translated model is touched whenever one of its translations are updated/destroyed (useful if using the updated_at timestamp in cache keys). This unfortunately does not interact well with Rails' optimistic locking when attempting to destroy a translated model: the translated model destroy triggers the associated translations to be destroyed, which touch the translated model, which increments the lock_version, which results in the in-memory object becoming stale. This is ultimately a bug in ActiveRecord (it should know not to touch a record that is being destroyed), but fixing that appears to be non-trivial. To get around this, whitehall is now using forked version of globalize that doesn't include the touch: true directive on the translations assocation (95ee80a)

Other Globalize validation issue

There appears to be a bug in globalize that means presence validations are not being triggered when saving a translation using update_attributes (globalize/globalize#401). The workaround for now is to both explicitly set the locale attribute and also perform the action in a I18n.with_locale block. (5e6e665#diff-095e6f47d7355ba033961aeb9dd441b6R7).

shared_mustache gem change

The newest version of sprockets-rails is not correctly choosing the local templates.js over the copy in the shared_mustache gem, which was causing the compiled javascript in test mode to not include the compiled mustache templates. This has been resolved by removing the blank templates.js from the shared_mustache gem (it's only purpose was so that any app using the shared_mustache gem could include templates.js in it's asset manifest without having to actually have a local file present)

Changes to cookie signing

Rails 4 introduces a new way to generate and verify signed cookies. It has a mechanism to automatically upgrade old cookies transparently. However, because the new cookies are not backwards compatible, this has intentionally been left off until we are certain that the upgrade has gone smoothly and that we don't need to roll back to the Rails 3 version. Once the dust has settled, steps should be taken to enable the new cookie code and begin the upgrade process. See http://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#action-pack for more details.

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Jan 30, 2015

Contributor

There's a strange thing happening to the body text areas on the forms on this branch (currently visible on preview). On this version, the rows attribute is not being set, so the text areas are defaulting to two rows. On master, they somehow get rendered with rows set to 20. Thing is, I can't see anywhere in the app code where they are being set to 20! If anyone has any insight before I dig into this further, let me know /cc @edds

Contributor

tekin commented Jan 30, 2015

There's a strange thing happening to the body text areas on the forms on this branch (currently visible on preview). On this version, the rows attribute is not being set, so the text areas are defaulting to two rows. On master, they somehow get rendered with rows set to 20. Thing is, I can't see anywhere in the app code where they are being set to 20! If anyone has any insight before I dig into this further, let me know /cc @edds

@jackscotti

This comment has been minimized.

Show comment
Hide comment
@jackscotti

jackscotti Jan 30, 2015

Contributor

Tekin, you broke GitHub!

screen shot 2015-01-30 at 09 40 11

Contributor

jackscotti commented Jan 30, 2015

Tekin, you broke GitHub!

screen shot 2015-01-30 at 09 40 11

@bradwright

This comment has been minimized.

Show comment
Hide comment
@bradwright

bradwright Jan 30, 2015

Contributor

Epic PR of the day 👍

Contributor

bradwright commented Jan 30, 2015

Epic PR of the day 👍

@edds

This comment has been minimized.

Show comment
Hide comment
@edds

edds Jan 30, 2015

Contributor

@tekin following the apidocs through text areas were initialized using DEFAULT_TEXT_AREA_OPTIONS which are defined to have 20 rows if none are specified. http://apidock.com/rails/ActionView/Helpers/InstanceTagMethods

Though looking they have removed that default in rails 4.

Contributor

edds commented Jan 30, 2015

@tekin following the apidocs through text areas were initialized using DEFAULT_TEXT_AREA_OPTIONS which are defined to have 20 rows if none are specified. http://apidock.com/rails/ActionView/Helpers/InstanceTagMethods

Though looking they have removed that default in rails 4.

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Jan 30, 2015

Contributor

@edds Uurgh, glad they got rid of that particular piece of magic! I'll get this branch updated to fill in the blanks for text areas without an explicit row count. Thanks!

Contributor

tekin commented Jan 30, 2015

@edds Uurgh, glad they got rid of that particular piece of magic! I'll get this branch updated to fill in the blanks for text areas without an explicit row count. Thanks!

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Jan 30, 2015

Contributor

So looks like my fix for "Other Globalize validation issue" (5e6e665#diff-095e6f47d7355ba033961aeb9dd441b6R7) is causing another issue - although it causes the presence validations to kick-in, setting the :locale attribute in LocalisedModel will overwrite the locale attribute on the editions table, i.e. what we use to signify the edition's primary locale. This is causing the primary locale to be changed on an edition when a new translation is added, which makes it appear as a non-english edition.

Looking at this now, I think the presence of an attribute called "locale" on the editions table is almost certainly the root cause. I'm going to have to investigate this further.

Contributor

tekin commented Jan 30, 2015

So looks like my fix for "Other Globalize validation issue" (5e6e665#diff-095e6f47d7355ba033961aeb9dd441b6R7) is causing another issue - although it causes the presence validations to kick-in, setting the :locale attribute in LocalisedModel will overwrite the locale attribute on the editions table, i.e. what we use to signify the edition's primary locale. This is causing the primary locale to be changed on an edition when a new translation is added, which makes it appear as a non-english edition.

Looking at this now, I think the presence of an attribute called "locale" on the editions table is almost certainly the root cause. I'm going to have to investigate this further.

@heathd

This comment has been minimized.

Show comment
Hide comment
@heathd

heathd Feb 5, 2015

Contributor

ok I've finished reviewing this. All looks good, nice work! There were quite a few thorny issues to deal with along the way!

My only slight concern is the globalize fork. I'm not clear whether fixes in rails (ie rails 4.1+) will avoid the lock version issues or not. If we do need to patch globalize, it might be good to also work with them to develop an alternative solution, as it would be good to develop some confidence that we can avoid forking in the medium term.

Obviously there is also the issue with 5e6e665 which you mentioned.

Contributor

heathd commented Feb 5, 2015

ok I've finished reviewing this. All looks good, nice work! There were quite a few thorny issues to deal with along the way!

My only slight concern is the globalize fork. I'm not clear whether fixes in rails (ie rails 4.1+) will avoid the lock version issues or not. If we do need to patch globalize, it might be good to also work with them to develop an alternative solution, as it would be good to develop some confidence that we can avoid forking in the medium term.

Obviously there is also the issue with 5e6e665 which you mentioned.

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Feb 11, 2015

Contributor

This branch now includes a fix for the issue with non-english editions. The fix is to use a column not called "locale" to signify the primary locale of the edition as this conflicts with some of the internal behaviour of globalize: globalize uses a :locale key during mass-assignment to signify the locale of the attributes being set. Up to now we got away with it and the locale attribute was being correctly set on the edition table, but this has changed with newer versions of globalize.

Contributor

tekin commented Feb 11, 2015

This branch now includes a fix for the issue with non-english editions. The fix is to use a column not called "locale" to signify the primary locale of the edition as this conflicts with some of the internal behaviour of globalize: globalize uses a :locale key during mass-assignment to signify the locale of the attributes being set. Up to now we got away with it and the locale attribute was being correctly set on the edition table, but this has changed with newer versions of globalize.

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Feb 11, 2015

Contributor

Nice work on getting a patch for the optimistic locking issue into globalize @heathd! 🍰 We'll have to wait until we've got whitehall to Rails 4.2 before we can move back to the main fork though as master breaks compatibility with Rails versions below that. Either that or see if we can get the fix backported to 4-0-stable.

Contributor

tekin commented Feb 11, 2015

Nice work on getting a patch for the optimistic locking issue into globalize @heathd! 🍰 We'll have to wait until we've got whitehall to Rails 4.2 before we can move back to the main fork though as master breaks compatibility with Rails versions below that. Either that or see if we can get the fix backported to 4-0-stable.

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Feb 11, 2015

Contributor

Rebased against master

Contributor

tekin commented Feb 11, 2015

Rebased against master

@heathd

This comment has been minimized.

Show comment
Hide comment
@heathd

heathd Feb 11, 2015

Contributor

Thanks @tekin. Just for the record, here's a link to the PR which has just been merged into globalize globalize/globalize#408

Contributor

heathd commented Feb 11, 2015

Thanks @tekin. Just for the record, here's a link to the PR which has just been merged into globalize globalize/globalize#408

@heathd

This comment has been minimized.

Show comment
Hide comment
@heathd

heathd Feb 11, 2015

Contributor

Rather than forking globalize, perhaps we could now reference globalize@master (at a specific sha)

Contributor

heathd commented Feb 11, 2015

Rather than forking globalize, perhaps we could now reference globalize@master (at a specific sha)

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Feb 11, 2015

Contributor

Unfortunately not, master only works with Rails 4.2.

Contributor

tekin commented Feb 11, 2015

Unfortunately not, master only works with Rails 4.2.

@heathd

This comment has been minimized.

Show comment
Hide comment
@heathd

heathd Feb 18, 2015

Contributor

I've rebased this onto perforance-improvements-to-document-searches-controller (which in turn has been rebased on master) in anticipation of that being merged

Contributor

heathd commented Feb 18, 2015

I've rebased this onto perforance-improvements-to-document-searches-controller (which in turn has been rebased on master) in anticipation of that being merged

tekin and others added some commits Jan 8, 2015

Update Gemfile for Rails 4.0.13
Updating one major Rails release at a time to take advantage of all deprecation
warnings.

A change was also required to how webmock is loaded. Also, active_resource is no
longer part of Rails. As it isn't used by the app, we can remove it entirely.
Regenerate db/schema.rb
Rails 4.0.x changes the schema syntax in a few ways:
- uses ruby 1.9 hash syntax
- explicitly declares index types on indexes

This commit regenerates the schema to use the new syntax without
changing the actual semantic content.
Temporarily comment out localised routing patch
This is not compatible with Rails 4 so commenting out for the time being.
Re-nest PublishingApiPresenters::Edition
This was causing a circular dependency issue. Whitespace-only changes
Update app config for Rails 4 compatibility
This was mainly a case of generating a fresh Rails 4 app to see what the new
default configuration looks like and removing any configurations that are now
deprecated.
Remove unnecessary script/rails file
bundler/rails does the right thing
Remove unnecessary rake task hack
This is no longer required in Rails 4
ActiveRecord::Base#scoped is deprecated
The `all` method now returns a scope, use that instead.
Disable deprecated implicit join references
ActiveRecord has deprecated implicit join references in the queries it
generates, e.g. it will no longer automatically join other tables that are
referenced in the WHERE clause. Instead, the reference should now be explicitly
includes using the `references` scope.

This pull request also explicitly turns them off so that any code that would
result in an implicit join raises an exception so that we can catch them all now
before they are removed permanently.
Don't call deprecated `all` method on scopes
Either `load` or `to_a` should be used, depending on whether the scope should be
used.
Don't use deprecated magic find_by_* helpers
The find_by() method should be used instead. Note that the active record-like
interfaces define their own explicit find_by_* methods. I'd suggest these be
renamed to something else to avoid confusion, e.g. `by_slug` instead of
`find_by_slug`.
Update the localised routing patch for Rails 4.
This had to be rewritten to work with the new routing code in Rails 4. Because
the app is now running on Ruby 2, we can make use of the Module#prepend method
to avoid the alias method chaining.

tekin and others added some commits Jan 21, 2015

Patch bug in Rails routing
Whitehall.atom_feed_maker was having trouble generating urls for non-localised
resources, e.g. topics. This turned out to be due to a bug in Rails routing code
that meant the default format set in atom_feed_maker was being lost. This
patches Rails to fix the issue. There is also a pull request on Rails to fix it
there too:

rails/rails#18627
Get around stricter Rails routing
Rails routing is now much stricter about matching valid routes, especially when
it comes to matching routes against inherited controller classes - it will no
longer match a route to a base class like EditionsController unless there are
explicit routes mapping to its actions. This rewrites and reorganises the
controller test to make sure that valid routes are used, appropriate
controllers are tested and that the required test routes exist.
Remove unnecessary stubbing in test
This was broken by the fact that we're no longer calling the deprecated `#all`
method in the controller code. Only stub when absolutely necessary people!
Fix attachment ordering on new records
The code that set the ordering on new attachments fails when creating a brand
new attachable thing with more than one attachment. This appears to be because
the query generated to calculate the maximum available ordering is not correctly
setting the newly generated ID of the attachable thing, and thus no matching
attachments are being found. The fix is to explicitly generate the query to get
the maximum ordering, making sure the ID of the attachable thing is used.
Avoid issues with ActionController::Parameters
The code is modifying an ActionController::Parameters instance when it sets the
:creator_id. This causes an ActiveModel::ForbiddenAttributesError to be raised
in  the build_statistics_announcement_date_change method when an attempt is made
to reverse_merge in the previous date's values. We can avoid this issue by
converting it into a plain old hash before passing it to the model code. This is
safe as an error would have already been raised if any unsafe parameters had
been sent with the request.
Ensure valid locale before saving translation
Means failing earlier, plus prevents the change in the next commit from breaking
an import that has translation fields but no valid locale set.
Globlize bug workaround
There appears to be a bug in globalize that is causing update_attributes to
succeed even with invalid attributes when called in an I18n.with_locale block.
By also explicitly setting the locale in the update_attributes call, we get
around the bug.
Maintain ordering of inapplicable nations
The forms for editions display the potentially inapplicable nations in a set
order (Scotland, Wales, Northern Ireland). This was previously maintained by
building NationInapplicabilities in the controller and sorting the association
based on the nations. This no longer works in Rails 4 (the order of items in an
association cannot be manipulated in this way). Instead of doing this work in
the controller, we can build the form fields starting from the ordered nations.
SCSS Deprecation
The extra .css in SCSS files is unnecessary and is deprecated.
Get around code reloading issue
A strange interaction with Rails auto code-reloading and this class is happening
which is causing the following exception to be raised:

   ArgumentError A copy of EditionServiceCoordinator has been removed from the
   module

Being more explicit about the namespace of the classes referenced here makes the
problem go away.
Fix issue with invalid AttachmentData
The upgrade to Rails 4 broke mass-assignment of FileAttachments whilst the
associated Attachment file was awaiting virus scanning. The fix is to ensure
that an AttachmentData record will still be valid whilst awaiting virus
scanning.

This change is required because ActiveRecord now loads the associated
AttachmentData *before* it checks if the nested attributes should be assigned or
rejected because of the reject_if proc. This made the FileAttachment invalid for
newly-uploaded FilAttachments that are still awaiting virus scanning because of
the presence check on the AttachmentData `file` attribute.
Don't store ActionController::Parameters in session
Storing an instance of ActionController::Parameters in the session for the
filter options breaks `redirect_to session_filters` because:

  ActionController::ActionControllerError: Cannot redirect to a parameter hash!
Skipping ActiveRecord no longer necessary
We no longer need to skip the loading of ActiveRecord when compiling assets.
Don't ignore /bin directory
The rails-generated binstubs should be included in the git repository, otherwise
they need to be generated by each developer locally.

See https://github.com/sstephenson/rbenv/wiki/Understanding-binstubs for more
details.
Use latest shared_mustache
This newer version of shared_mustache gets around an issue where the latest
sprockets-rails was using the blank vendor version of template.js in
shared_mustache instead of the local copy in whitehall when loading non-compiled
assets in non-production environments. This was causing a javascript-enabled
cucumber feature to fail. Because template.js is no longer present in
shared_mustache, we now need a copy in the repository.
Set explicit rows for text areas
Rails 3 had default options for text area tags:

  DEFAULT_TEXT_AREA_OPTIONS = { "cols" => 40, "rows" => 20 }

This was removed in Rails 4. This commit only sets the rows as the width of
text areas is defined with CSS.
Replace "locale" column with "primary_locale"
The fact that we are using a column called "locale" on the editions table to
signify the locale for non-english editions causes an issue with later
versions of the globalize gem.

This is the second phase of migrating this column to a new name of
"primary_locale". We pre-emptively added the column to the table in
3f2a376. This commit includes a
migration to sync the `primary_locale` column with `locale` on
deployment.

At this stage we do not yet remove the old locale column, to avoid
breaking existing running processes which expect the `locale` field to
be present still. We deprecate the `locale` column in the `Edition`
model however.

Once this code has been deployed, the pending migration can be run to
safely drop the old column and the `deprecated_columns` directive can be
removed.
Update non-english locale fields JavaScript
The DOM selector changed when the locale column was renamed. Whilst I was here I
also tidied up the tests for the JS and changed the behaviour slightly so that
instead of hiding the locale fields on-load, the "js-hidden" class is applied to
the fieldset so that they are hidden right away (on JS-enabled browsers at
least). This prevents them briefly flashing up whilst the JavaScript is being
loaded and evaluated on the page.
Add note regarding globalize fork
@heathd has got a patch in to the main globalize fork that deals with the issue
my fork works around. Unfortunately, master branch globalize has been updated
for Rails 4.2 compatibility and is therefor not compatible with this version of
whitehall. Either the patch will need to be backported, or we'll have to wait
until whitehall is updated for Rails 4.2 before moving to the main fork of
globalize.
Use release of rack-test
A new version of rack-test has finally been released, meaning we no longer need
to reference a github reference for the params with arrays of hashes fix.
Avoid 'Cannot redirect to a parameter hash' error
Rails 4 introduced a [protection against passing parameters hash into
`redirect_to`](rails/rails#16170) as a security
measure.

In some circumstances the `session[:document_filters]` could contain a
paramters hash (I suspect this may only be the case for a session which
was serialized into someones session cookie in the pre-rails-4 whitehall
app and deserialized in this release).

Adding this `.to_h` forces the hash to always be converted to a ruby
hash.

It's safe to pass these parameters to redirect_to because they are all
features which are intended to be controlled by user input. The redirect
is just a feature to ensure consistency of urls.

@heathd heathd merged commit e354265 into master Feb 19, 2015

1 check passed

default Build #5179 succeeded on Jenkins
Details

@heathd heathd deleted the upgrade-to-rails-4 branch Feb 19, 2015

@evilstreak

This comment has been minimized.

Show comment
Hide comment
@evilstreak

evilstreak Feb 19, 2015

Contributor

🎆

Contributor

evilstreak commented Feb 19, 2015

🎆

@fofr

This comment has been minimized.

Show comment
Hide comment
@fofr

fofr Feb 19, 2015

Contributor

🎉 4️⃣ 🎈

Contributor

fofr commented Feb 19, 2015

🎉 4️⃣ 🎈

@tekin

This comment has been minimized.

Show comment
Hide comment
@tekin

tekin Feb 20, 2015

Contributor

🎊💃🎊

Contributor

tekin commented Feb 20, 2015

🎊💃🎊

@lazyatom

This comment has been minimized.

Show comment
Hide comment
@lazyatom

lazyatom Feb 26, 2015

Contributor

👏

Contributor

lazyatom commented Feb 26, 2015

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment