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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintenance: Upgrade to Rails 6 #2679

Merged
merged 52 commits into from Dec 2, 2019
Merged

Maintenance: Upgrade to Rails 6 #2679

merged 52 commits into from Dec 2, 2019

Conversation

kevinrobinson
Copy link
Contributor

@kevinrobinson kevinrobinson commented Oct 25, 2019

and all the gems because optimism (and batching manual testing work) 馃槃

EDIT: Rails 6 also includes zeitwerk as the class loader, which has some changes breaking the current config. Read more from Rails or here. This led to four issues:

  • One issue was acronyms (eg, MockLDAP or PowerBIExporter), which can be resolved by adding these acronyms to the Rails inflections config or by just CamelCasing everything, which is what I did.
  • The main issue was that the Devise plugin for LDAP communication was loaded in a way that didn't work with zeitwerk. To fix this, this PR renames the files and folders so they match what zeitwerk expects, which also lets us remove some manual config. So while a breaking change, it was probably a good improvement for simplifying that config anyway.
  • accommodations_importer.rb was a stray file, revealed by zeitwerk check
  • add importers/star to autoload path

To catch issues like this going forward, this PR also adds a check for this to the build (since the check tool revealed failures that were obvious in dev mode, but not revealed by running the tests).

Also:

  • Migrate to FactoryBot block syntax, fix stricter checks for only factory-ing existing methods
  • update_attributes! is deprecated, migrate to update!
  • Also, organize the gems and trim where we can (eg, remove probability monkey patches)
  • Added a note about Rails content security policy and secure_headers in the Rails CSP stub as well. This PR doesn't change that config.

EDIT: A few more issues resolved:

  • From the changelog, "Fix ActiveModel::Serializers::JSON#as_json method for timestamps" rails/rails#31503 impacted many specs, but in a mechanical way
  • ActiveModel::Serializers::JSON#as_json BigDecimal changes rails/rails#37604 is related fallout from the previous issue, not in the changelog and still an open issue. This only impacted a few tests, so changed that and will verify no regression in production.
  • Another change from Zeitwert, where a spec needed to eager load to reflect on all Model classes. This ran into an open issue with the eager_load method not yet working in Rails 6 outside the production env, so that's commented, linked to the issues, and worked around.

EDIT: And some more:

  • Zeitwerk: Monkey patch for LogSubscriber to scrub student names from downloaded filenames.
  • Encode Content-Disposition filenames on send_data and send_file. rails/rails#33829
  • DEPRECATION WARNING: Controller-level force_ssl is deprecated and will be removed from Rails 6.1. Please enable config.force_ssl in your environment configuration...
  • In feature tests, there was a method to simulate clicking on ujs links (since no JS running in them). These used to include a step that did an http > https redirect manually, but that's no longer necessary. Maybe came from the force_ssl config change.
  • require for FakeNames
  • DEPRECATION WARNING: ActionView::Template#initialize requires a locals parameter, fixed by upgrading to Rails 6.0.1
  • ActionView::Template::Error: wrong number of arguments (given 2, expected 1) and DEPRECATION WARNING: formats is deprecated and will be removed from Rails 6.1 (eg, in profile_pdf_controller_spec.rb). From rspec-rails (rspec/rspec-rails#2086), updated to rspec-rails '~> 4.0.0.beta3'.
  • FactoryBot build/create behavior changed in 4>5, required updating model specs since #build wouldn't create association models, so validations would fail.

EDIT:

  • update PerDistrict#sports_season_key to work for demo, when rebuilding site

kevinrobinson added 30 commits Oct 25, 2019
@kevinrobinson kevinrobinson merged commit 74a4ac2 into master Dec 2, 2019
1 check was pending
@kevinrobinson kevinrobinson deleted the upgrade/rails-6 branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant