-
Notifications
You must be signed in to change notification settings - Fork 112
Match config.load_defaults
Version to Rails Version
#3496
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
Conversation
- Added explicit `coder: YAML` to `serialize` calls to conform with Rails 7.1 changes - (Rather than explicitly adding this, `config.active_record.default_column_serializer = YAML` could've been added to `config/application.rb` to restore the pre-Rails 7.1 behaviour) - Retained `type:` where specified to enforce the expected type of deserialized objects.
config.load_defaults
Version to Rails Version
@aaronskiba I wonder if this file https://github.com/rails/rails/blob/v7.0.0/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt (with .tt remove) should be included in config/initializers for completenes. So config commented out config can be switched on if necessary. |
@aaronskiba The Condition coder: YAML will need to be be overridden by PR CDLUC3#667 (or the Conditional questions will fail as we moved from YAML to JSON. roadmap/app/models/condition.rb Lines 29 to 33 in 76cc7eb
Also this change is incorrect it should (as is currently) serialize :prefs, type: Hash, coder: JSON Lines 72 to 74 in 76cc7eb
|
Thank you. Yes, the PR you mentioned will migrate those serialized columns from YAML to JSON. We'll have to be sure we end up with the JSON type in the end. I'm wondering what to do with the other code you mentioned: # User Notification Preferences
serialize :prefs, type: Hash, coder: YAML The Here's a bit of additional information too:
class Pref < ApplicationRecord
##
# Serialize prefs to JSON
serialize :settings, coder: JSON
|
app/models/user.rb
Outdated
@@ -70,7 +70,7 @@ class User < ApplicationRecord | |||
|
|||
## | |||
# User Notification Preferences | |||
serialize :prefs, type: Hash | |||
serialize :prefs, type: Hash, coder: YAML |
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.
Hi @briri, does it look like this code could be removed entirely? I can see an older migration that moved prefs to its own table, so I'm thinking this code no longer serves a purpose (https://github.com/DMPRoadmap/roadmap/blob/main/db/migrate/20170712084314_move_prefs_to_table.rb).
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.
Yeah, I think so. There's a Pref model now and separate table and I don't see anywhere where user.prefs
is being accessed
The `serialize :prefs, type: Hash` line in the `User` model was redundant since the `prefs` column does not exist in the `users` table. User preferences are handled via the `Pref` model, which serializes its `settings` column as a JSON hash. This change aligns with the update to store preferences in a separate model, as introduced in the following commit: 0405973
Changes proposed in this PR:
config.load_defaults
value to7.1
to match our Rails version.coder: YAML
toserialize
calls to conform with Rails 7.1 changesconfig.active_record.default_column_serializer = YAML
was implicit, so these explicit calls were unnecessary.)