10000 Match `config.load_defaults` Version to Rails Version by aaronskiba · Pull Request #3496 · DMPRoadmap/roadmap · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 5 commits into from
Apr 10, 2025

Conversation

aaronskiba
Copy link
Contributor
@aaronskiba aaronskiba commented Mar 26, 2025

Changes proposed in this PR:

  • Updated config.load_defaults value to 7.1 to match our Rails version.
  • Added explicit coder: YAML to serialize calls to conform with Rails 7.1 changes
    • (Prior to Rails 7.1, config.active_record.default_column_serializer = YAML was implicit, so these explicit calls were unnecessary.)

8000
- 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.
@aaronskiba aaronskiba marked this pull request as draft March 26, 2025 22:12
@aaronskiba aaronskiba marked this pull request as ready for review March 27, 2025 15:26
@aaronskiba aaronskiba changed the title Aaron/config.load defaults 7.1 Match config.load_defaults Version to Rails Version Mar 27, 2025
@johnpinto1
Copy link
Contributor

@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.

@johnpinto1
Copy link
Contributor
johnpinto1 commented Apr 3, 2025

@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.

belongs_to :question
enum action_type: %i[remove add_webhook]
serialize :option_list, type: Array, coder: YAML
serialize :remove_data, type: Array, coder: YAML
serialize :webhook_data, coder: JSON

Also this change is incorrect it should (as is currently)
serialize :prefs, type: Hash, coder: JSON
# User Notification Preferences
serialize :prefs, type: Hash, coder: YAML

@aaronskiba
Copy link
Contributor Author

@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.

belongs_to :question
enum action_type: %i[remove add_webhook]
serialize :option_list, type: Array, coder: YAML
serialize :remove_data, type: Array, coder: YAML
serialize :webhook_data, coder: JSON

Also this change is incorrect it should (as is currently)
serialize :prefs, type: Hash, coder: JSON

# User Notification Preferences
serialize :prefs, type: Hash, coder: YAML

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 prefs column isn't actually part of the users table. Based on that, I don't think the code serves any purpose.

Here's a bit of additional information too:

rb(main):0015> user.pref
  Pref Load (0.4ms)  SELECT "prefs".* FROM "prefs" WHERE "prefs"."user_id" = $1 LIMIT $2
class Pref < ApplicationRecord
  ##
  # Serialize prefs to JSON
  serialize :settings, coder: JSON

has_one :pref exists in the User model. user.pref loads the data from that table and settings is correctly set to JSON.

@@ -70,7 +70,7 @@ class User < ApplicationRecord

##
# User Notification Preferences
serialize :prefs, type: Hash
serialize :prefs, type: Hash, coder: YAML
Copy link
Contributor Author
@aaronskiba aaronskiba Apr 3, 2025

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).

Copy link
Contributor

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

aaronskiba and others added 2 commits April 3, 2025 14:19
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
@aaronskiba aaronskiba merged commit 3506e05 into development Apr 10, 2025
8 checks passed
@aaronskiba aaronskiba deleted the aaron/config.load_defaults-7.1 branch April 10, 2025 15:24
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.

3 participants
0