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.

Sign up for GitHub

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

Jump to bottom

Upgrade LMS Rails version to 6.0.5 #9343

Merged
merged 8 commits into from
Jul 12, 2022
Merged

Upgrade LMS Rails version to 6.0.5 #9343

merged 8 commits into from
Jul 12, 2022

Conversation

brendanshean
Copy link
Member

@brendanshean brendanshean commented Jul 7, 2022 •
edited

WHAT

Upgrade LMS from 5.2.8 to 6.0.5

WHY

Newer versions of Rails provide new features as well as security enhancements.

HOW

Update Gemfile with new Rails version. Then, after lots of flipping switches with Gemfile.lock, run bundle.

A couple of specs had to be fixed:
1. Fixing module_parent calls: parent will be deprecated in Rails 6 so update those calls to module_parent
2. Fix time comparison: as_json calls in Rails 6 converts ActiveSupport::TimeWithZone values to strings whereas it currently leaves the object as a ActiveSupport::TimeWithZone. (NB: Comparisons with these objects are tricky, see here and here)

Addendum 1:
Cookie serialization is updated to hybrid per this post

Addendum 2:
Removed livingstyleguide gem from development group since it breaks development build. This was missed on heroku since the gem doesn't run.

Screenshots

(If applicable. Also, please censor any sensitive data)

Notion Card Links

(Please provide links to any relevant Notion card(s) relevant to this PR.)

PR Checklist Your Answer
Have you added and/or updated tests? YES
Have you deployed to Staging? YES
Self-Review: Have you done an initial self-review of the code below on Github? YES
Spec Review: Have you reviewed the spec and ensured this meets requirements and/or matches design mockups? N/A

@brendanshean brendanshean changed the title Bs poc rails 6 Upgrade LMS Rails version to 6.0.5 Jul 7, 2022
@brendanshean brendanshean requested review from anathomical, happythenewsad and dandrabik and removed request for anathomical and happythenewsad July 7, 2022 20:05
anathomical
anathomical approved these changes Jul 7, 2022
View reviewed changes
services/QuillLMS/Gemfile
Comment on lines +142 to +151
# Memory profiling
gem 'puma_worker_killer', '~> 0.1.0'

# temp for migrations
gem 'paperclip'

# TBD
gem 'rb-readline'
gem 'awesome_print'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the consolidation you did here for items we should do later analysis on regarding whether we use these or not. It may be worth going ahead and creating a Task card in Notion so that we remember to come back and do this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm planning on doing an audit after the 6.1 upgrade.

services/QuillLMS/spec/models/feedback_history_spec.rb
expect(json_hash['metadata']).to eq(@feedback_history.metadata)
expect(json_hash['rule_uid']).to eq(@feedback_history.rule_uid)

expect(json_hash['prompt']).to eq(@feedback_history.prompt.as_json)
expect(json_hash['prompt']['text']).to eq(@prompt.text)

expect(Time.iso8601(json_hash['time'])).to be_within(1.second).of @feedback_history.time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a reasonable solution to this problem. I've generally fallen back to coercing string representations, but this feel cleaner.

@brendanshean brendanshean force-pushed the bs-poc-rails-6 branch 2 times, most recently from a417c89 to fedb2c2 Compare July 8, 2022 18:20
@delete-merged-branch delete-merged-branch bot deleted the branch develop July 11, 2022 16:08
@brendanshean brendanshean changed the base branch from bs-upgrade-active-model-serializers to develop July 11, 2022 16:33
dandrabik
dandrabik approved these changes Jul 11, 2022
View reviewed changes
Copy link
Member

@dandrabik dandrabik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for your work on this. I left a few small comments on things to double check, but nothing is blocking.

services/QuillLMS/Gemfile Outdated
@@ -5,7 +5,7 @@ source 'https://rubygems.org'
ruby '2.7.6'

# CORE DEPS
gem 'rails', '5.2.8'
gem 'rails', '~> 6.0.5'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious the about why the change away from a locked patch version. I would think even a patch upgrade of Rails would be a manually planned/tested endeavor, so curious what we gain from this change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've viewed patch upgrades even with Rails as safe updates, but I will change it back to the locked version since I don't really have a strong preference.

services/QuillLMS/config/initializers/content_security_policy.rb Outdated
@@ -0,0 +1,30 @@
# frozen_string_literal: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, does this empty file mean this functionality is off (that's what we would want)? Wondering since we are using the secure_headers gem to set CSP here. We'd want to ensure that these don't conflict.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was autogenerated, but I agree that it's confusing. I will remove it.

services/QuillLMS/config/initializers/cookies_serializer.rb

# Specify a serializer for the signed and encrypted cookie jars.
# Valid options are :json, :marshal, and :hybrid.
Rails.application.config.action_dispatch.cookies_serializer = :hybrid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that they provide a migration path. I don't think this is an issue for how we use cookies, but just flagging it in case it comes up:

SPOILER! When using the :json or :hybrid serializer, you should be aware that not all Ruby objects can be serialized as JSON. For example, Date and Time objects will be serialized as strings, and Hashes will have their keys stringified.

@brendanshean brendanshean merged commit c68bc98 into develop Jul 12, 2022
@delete-merged-branch delete-merged-branch bot deleted the bs-poc-rails-6 branch July 12, 2022 10:15
brendanshean added a commit that referenced this pull request Jul 12, 2022
@brendanshean
Merge pull request #9353 from empirical-org/develop
3e64bdc 
Upgrade LMS Rails version to 6.0.5 (#9343)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anathomical anathomical anathomical approved these changes

@dandrabik dandrabik dandrabik approved these changes

@happythenewsad happythenewsad Awaiting requested review from happythenewsad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@brendanshean @anathomical @dandrabik