AO3-6893 Upgrade gems and configs to Rails 7.1.x #5039
Conversation
| .includes(blocked: [:pseuds, { default_pseud: { icon_attachment: { blob: { | ||
| variant_records: { image_attachment: :blob }, | ||
| preview_image_attachment: { blob: { variant_records: { image_attachment: :blob } } } | ||
| } } } }]) |
There was a problem hiding this comment.
This change and the same one in muted/users_controller.rb is based on the same change in Active Storage, because this is a copy of the with_attached_* scope: https://github.com/rails/rails/blob/v7.0.8.3/activestorage/lib/active_storage/attached/model.rb -> https://github.com/rails/rails/blob/v7.1.5.1/activestorage/lib/active_storage/attached/model.rb (track_variants is true for us)
There was a problem hiding this comment.
It's 7.1 so we should be able to re-enable these lines:
otwarchive/spec/requests/blocked_users_n_plus_one_spec.rb
Lines 15 to 17 in 603cb16
and
otwarchive/spec/requests/muted_users_n_plus_one_spec.rb
Lines 15 to 17 in 603cb16
However, with these lines restored, I didn't get any test failures on the current main branch / Rails 7.0. Not sure why yet.
There was a problem hiding this comment.
Maybe something related to these comments got lost in the whole active storage PR, it was quite large and there were changes to the related scope after these comments were added. I'll just uncomment the tests and we can be happy that they pass now, I guess?
| @@ -899,7 +899,7 @@ def build_options(params) | |||
| external_coauthor_name: params[:external_coauthor_name], | |||
| external_coauthor_email: params[:external_coauthor_email], | |||
| language_id: params[:language_id] | |||
| } | |||
| }.compact_blank! | |||
There was a problem hiding this comment.
Due to the nokogiri update, when encoding defaulted to empty string here, that empty string was getting used as the encoding instead of the actual encoding of the downloaded story. Getting rid of the empty string via compact_blank! makes it fall back to the encoding of the downloaded story as expected.
| @@ -84,7 +84,7 @@ def document(object) | |||
| json_object = object.as_json( | |||
| root: false, | |||
| only: [ | |||
| :id, :created_at, :bookmarkable_type, :bookmarkable_id, :user_id, | |||
| :id, :created_at, :bookmarkable_type, :bookmarkable_id, | |||
There was a problem hiding this comment.
This user_id was duplicate with the one in the merge call later (as_json uses string keys, the merge uses symbols, so it wasn't getting overwritten by the merge). Something changed in the json serialization for Elasticsearch so it wasn't getting rid of this duplicate "user_id": nil anymore, so deleting it here is the fix.
|
|
||
| # Use secret from archive config | ||
| config.secret_key_base = ArchiveConfig.SESSION_SECRET |
There was a problem hiding this comment.
According to the Rails documentation, it's preferred to do this in credentials.yml.enc instead: https://guides.rubyonrails.org/v7.1/configuring.html#config-secret-key-base / https://api.rubyonrails.org/v7.1.5.1/classes/Rails/Application.html#method-i-secret_key_base
| @@ -99,7 +103,6 @@ class Application < Rails::Application | |||
| "X-Frame-Options" => "SAMEORIGIN", | |||
| "X-XSS-Protection" => "1; mode=block", | |||
| "X-Content-Type-Options" => "nosniff", | |||
| "X-Download-Options" => "noopen", | |||
There was a problem hiding this comment.
This is suggested by the update script. It worsens Internet Explorer support.
| # This migration comes from active_storage (originally 20190112182829) | ||
| class AddServiceNameToActiveStorageBlobs < ActiveRecord::Migration[6.0] | ||
| def up | ||
| return unless table_exists?(:active_storage_blobs) |
There was a problem hiding this comment.
I am thinking we will want to use departure for this, and that the table should be added to https://github.com/otwcode/otwarchive/blob/master/.rubocop.yml#L84
There was a problem hiding this comment.
Our current DB schema has this:
add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id"
add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id"Meaning that we have foreign keys referencing active_storage_blobs (which is getting modified here), so if we use pt-online-schema-change / departure here, we run into issues with foreign keys:
When the tool renames the original table to let the new one take its place, the foreign keys “follow” the renamed table, and must be changed to reference the new table instead.
Departure currently sets --alter-foreign-keys-method auto.
|
The tests seem to be flaky, they didn't fail on the commit with the active storage changes, just the one with the robocop changes. I also had them fail on And I figured I know better than rubocop for the rest of the reviewdog complaints/didn't want to change the generated migrations. |
Issue
https://otwarchive.atlassian.net/browse/AO3-6893
Purpose
Upgrade to Rails 7.1. Should go in after Ruby 3.2 and AO3-6892.
Testing Instructions
Refer to Jira.
Credit
Bilka