-
Notifications
You must be signed in to change notification settings - Fork 17
Change ActiveRecord::Migration to ActiveRecord::Migration[7.1] #788
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
base: master
Are you sure you want to change the base?
Conversation
| def initialize_profile | ||
| create_profile!( | ||
| checkin_reminder: true, | ||
| onboarding_step_id: "onboarding-personal", | ||
| most_recent_doses: {}, | ||
| most_recent_conditions_positions: {}, | ||
| most_recent_symptoms_positions: {}, | ||
| most_recent_treatments_positions: {} | ||
| ) | ||
| end | ||
|
|
||
| factory :user do | ||
| sequence(:email) { |number| "user#{number}@example.com" } | ||
| password { "password123" } | ||
| password_confirmation { "password123" } | ||
| after(:create) { initialize_profile } |
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.
Modified the User factory to have the init_profile model from the User model and renamed it to initialize_profile because of the error below:
Done.
bin/rails aborted!
NameError: undefined local variable or method `init_profile' for #<FactoryBot::SyntaxRunner:0x00007eaac2e3c560> (NameError)
after(:create) { init_profile }
^^^^^^^^^^^^
Did you mean? initialize
/app/spec/factories/users.rb:45:in `block (3 levels) in <top (required)>'
/app/db/seeds/development/users.seeds.rb:4:in `block in evaluate'
/app/db/seeds/development/users.seeds.rb:3:in `each'
/app/db/seeds/development/users.seeds.rb:3:in `evaluate'
Tasks: TOP => db:seed => db:seed:development => db:seed:development:users
(See full trace by running task with --trace)| Rake::Task["db:migrate"].invoke | ||
| end | ||
| Rake::Task["db:structure:load"].invoke | ||
| #Rake::Task["db:schema:load"].invoke |
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.
Had to comment out the db:schema:load task, after updating from db:structure:load since this was removed in Rails 6.2, in order to be able to run the rails app:setup command.
The error I got when I tried to run rails db:schema:load independently was:
psql:/app/db/structure.sql:60: ERROR: relation "ar_internal_metadata" already exists
bin/rails aborted!
failed to execute:
psql --set ON_ERROR_STOP=1 --quiet --no-psqlrc --output /dev/null --file /app/db/structure.sql flaredown_development
Please check the output above for any errors and make sure that `psql` is installed in your PATH and has proper permissions.
/app/lib/tasks/app.rake:47:in `build_database'
/app/lib/tasks/app.rake:32:in `setup'
/app/lib/tasks/app.rake:4:in `block (2 levels) in <top (required)>'
Tasks: TOP => db:schema:load
(See full trace by running task with --trace)It seems psql needs to be installed in the Rails container in order for the schema loading to work. Not sure why since pg is in the backend/Gemfile and I would think that should cover the database connection needs for the app.
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'm curious if it would work as intended if we put the db:migrate here, and then the schema:load within the Rails.env.development? block. I'm not sure if this is ran in our deploy process for production anyway, or this is generally just used for development purposes.
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 wouldn't know since I was running this for development setup purposes 🙃
Maybe the contributors who've deployed Flaredown in production can comment?
compwron
left a comment
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 is looking good!!
haydenrou
left a comment
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.
thank you! Just left a couple of thoughts
| Rake::Task["db:migrate"].invoke | ||
| end | ||
| Rake::Task["db:structure:load"].invoke | ||
| #Rake::Task["db:schema:load"].invoke |
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'm curious if it would work as intended if we put the db:migrate here, and then the schema:load within the Rails.env.development? block. I'm not sure if this is ran in our deploy process for production anyway, or this is generally just used for development purposes.
| t.string :location_name, null: false | ||
| t.decimal :latitude, {precision: 10, scale: 7} | ||
| t.decimal :longitude, {precision: 10, scale: 7} | ||
| #t.decimal :longitude, {precision: 10, scale: 7} |
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.
Was this causing a problem?
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.
Probably the migration was failing to go through with the field as is. Can check and confirm.
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.
Finally got around to this. Here is the traceback with the above line not commented out:
== 20170817154145 CreatePositions: migrating ==================================
-- create_table(:positions)
bin/rails aborted!
StandardError: An error has occurred, this and all later migrations canceled: (StandardError)
you can't define an already defined column '{:precision=>10, :scale=>7}' on 'positions'.
/app/db/migrate/20170817154145_create_positions.rb:7:in `block in change'
/app/db/migrate/20170817154145_create_positions.rb:3:in `change'
/app/lib/tasks/app.rake:45:in `build_database'
/app/lib/tasks/app.rake:32:in `setup'
/app/lib/tasks/app.rake:4:in `block (2 levels) in <top (required)>'
Caused by:
ArgumentError: you can't define an already defined column '{:precision=>10, :scale=>7}' on 'positions'. (ArgumentError)
raise ArgumentError, "you can't define an already defined column '#{name}' on '#{@name}'."
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/app/db/migrate/20170817154145_create_positions.rb:7:in `block in change'
/app/db/migrate/20170817154145_create_positions.rb:3:in `change'
/app/lib/tasks/app.rake:45:in `build_database'
/app/lib/tasks/app.rake:32:in `setup'
/app/lib/tasks/app.rake:4:in `block (2 levels) in <top (required)>'
Tasks: TOP => db:migrate
(See full trace by running task with --trace)
make: *** [Makefile:23: seed] Error 1From what I can tell, it seems that the database for some reason thinks latitude and longtitude are the same field. If it helps, I am using the Docker setup via make build and make seed.
|
There's some good stuff in here, rebase and we can merge? |
Yes. Will do on Monday 🙌 |
Changed the line ActiveRecord::Migration to ActiveRecord::Migration[7.1] so that schema loading could work (it is now required for newer versions of Rails) Updated and commented out the load schema task since it fails to find and run the loading of data into the database PostgreSQL container Added the initialize_profile method and add it to the `after_create` portion of the User factory because of the NameError I encountered: NameError: undefined local variable or method `init_profile' for #<FactoryBot::SyntaxRunner:0x00007eaac2e3c560> (NameError)
afe4059 to
6b387ae
Compare
Apologies it took longer than I expected to get to this. The branch should now be up to date. |
|
lint error in CI |
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.
Pull request overview
This PR attempts to upgrade the Rails application from version 5.1 to 7.1 by updating migration version declarations and addressing related compatibility issues. However, the implementation contains several critical errors that would break core functionality.
- Updates all ActiveRecord::Migration class declarations from [5.1] to [7.1] across 44 migration files
- Attempts to fix a NameError in the User factory by adding profile initialization logic
- Modifies the app setup rake task to handle schema/fixtures loading differently
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| backend/spec/factories/users.rb | Adds duplicate profile initialization that conflicts with existing User model callback |
| backend/lib/tasks/app.rake | Incorrectly changes db:fixtures:load to commented db:schema:load |
| backend/db/migrate/20170817154145_create_positions.rb | Comments out required longitude column, breaking Position model |
| backend/db/migrate/*.rb (41 files) | Updates migration version from [5.1] to [7.1] |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def initialize_profile | ||
| create_profile!( | ||
| checkin_reminder: true, | ||
| onboarding_step_id: "onboarding-personal", | ||
| most_recent_doses: {}, | ||
| most_recent_conditions_positions: {}, | ||
| most_recent_symptoms_positions: {}, | ||
| most_recent_treatments_positions: {} | ||
| ) | ||
| end | ||
|
|
||
| factory :user do | ||
| sequence(:email) { |number| "user#{number}@example.com" } | ||
| password { "password123" } | ||
| password_confirmation { "password123" } | ||
| after(:create) { initialize_profile } |
Copilot
AI
Dec 11, 2025
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 initialize_profile method and after(:create) callback are unnecessary and will cause issues. The User model already has an after_create :init_profile callback (see backend/app/models/user.rb line 60) that creates the profile with the exact same attributes. This duplicate callback could cause errors or create duplicate profiles. Remove both the initialize_profile method definition and the after(:create) callback from this factory.
| t.string :location_name, null: false | ||
| t.decimal :latitude, {precision: 10, scale: 7} | ||
| t.decimal :longitude, {precision: 10, scale: 7} | ||
| #t.decimal :longitude, {precision: 10, scale: 7} |
Copilot
AI
Dec 11, 2025
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.
The longitude column is commented out but it's required by the Position model. The Position model uses self.longitude in the set_location_name callback (backend/app/models/position.rb line 12), and the schema.rb shows this column should exist (backend/db/schema.rb line 76). Commenting out this column will break the Position model functionality and cause database errors. Uncomment this line to match the schema definition.
| Rake::Task["db:migrate"].invoke | ||
| end | ||
| Rake::Task["db:fixtures:load"].invoke | ||
| #Rake::Task["db:schema:load"].invoke |
Copilot
AI
Dec 11, 2025
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 change is incorrect and alters the original functionality. The original code invoked db:fixtures:load, but this PR changed it to db:schema:load (which is now commented out). The comment in the PR description mentions issues with schema loading, but the original task was for loading fixtures, not schema. If fixtures loading is failing, that should be investigated and fixed separately. Either restore the original db:fixtures:load invocation or remove the line entirely if fixtures are not needed, rather than changing it to a different commented-out task.
ActiveRecord::Migration to ActiveRecord::Migration[7.1]so that schema loading could work (it is now required for newer versions of Rails)after_createportion of the User factory because of theNameErrorI encountered: