-
Notifications
You must be signed in to change notification settings - Fork 523
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
AO3-6674 Upgrade gems and config to Rails 7.0 #4727
Conversation
@@ -129,7 +129,7 @@ group :test do | |||
gem 'launchy' # So you can do Then show me the page | |||
gem 'delorean' | |||
# Record and replay data from external URLs | |||
gem 'vcr', '~> 3.0', '>= 3.0.1' | |||
gem "vcr", "~> 6.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A newer version of this is required to work with Cucumber, which is forcing its way in along with all the other things that need upgrading to support Rails 7. It also seemed like a good idea to just grab the newest since we're quite out of date
@@ -1,5 +1,5 @@ | |||
class ApplicationController < ActionController::Base | |||
include Pundit | |||
include Pundit::Authorization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a warning message in the log output about Pundit
without this being deprecated, drop-in replacement seems to work in tests and local dev
@external_work = ExternalWork.where(url: url).first | ||
end | ||
respond_to do |format| | ||
format.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something about the way Rails resolves templates has changed in version 7, and this was easier than fighting it. The client seems not to care, and the file is JS anyways...
@@ -1,6 +1,6 @@ | |||
module SkinCacheHelper | |||
def cache_timestamp | |||
Time.now.utc.to_s(:usec) | |||
Time.now.utc.to_fs(:usec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw a deprecation warning for this during Cucumber tests, so I figured I'd change it to avoid pain later. (Similar change below.)
@@ -128,5 +128,7 @@ class Application < Rails::Application | |||
authentication: ArchiveConfig.SMTP_AUTHENTICATION | |||
}) | |||
end | |||
|
|||
config.active_support.disable_to_s_conversion = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New Rails 7 parameter, but per the docs it needs to be put here and not in the overrides file
Rails.application.config.active_record.partial_inserts = false | ||
|
||
# Protect from open redirect attacks in `redirect_back_or_to` and `redirect_to`. | ||
# Rails.application.config.action_controller.raise_on_open_redirects = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks several Cucumber tests, so I'd rather flip it in another PR to keep the diff more contained...
@@ -0,0 +1,110 @@ | |||
# Be sure to restart your server when you modify this file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've turned on things that are relatively safe already, others left for future PRs
@@ -1,7 +1,7 @@ | |||
@works | |||
Feature: Languages | |||
|
|||
Scenario: Browse works by language | |||
# Scenario: Browse works by language |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cucumber now fails on undefined steps (scenario, when, etc.) I think that's the right call, but it means that some TODO tests broke the pipeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the odds we are ever going to do these and wouldn't be better off removing them entirely.
...who said that?
@@ -1,13 +1,14 @@ | |||
# Adapted from https://github.com/tpope/fivemat | |||
require "cucumber/formatter/progress" | |||
require_relative "./elapsed_time" | |||
|
|||
module Ao3Cucumber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our Cucumber version was... quite old. Several internals have changed, so I had to update this to get the same information in a slightly different way
@zz9pzza do you want to do the Ruby 3.1 PR first? If not, I'll grab the commit from that removing the mail starttls patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a typo left, I think! Thank you so much for working on this.
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing
)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-6674