Skip to content
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

updated question as requested #2969

Merged
merged 3 commits into from Sep 23, 2022
Merged

updated question as requested #2969

merged 3 commits into from Sep 23, 2022

Conversation

Ito-Eta
Copy link
Collaborator

@Ito-Eta Ito-Eta commented Sep 19, 2022

As requested in the pivotal story, updated one of the study type questions to include new text.

pivotal story: https://www.pivotaltracker.com/story/show/182204348

@@ -0,0 +1,6 @@
class ChangeConsentQuestionInStudyQuestions < ActiveRecord::Migration[5.2]
def change
@stq = StudyTypeQuestion.find(13)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding by ID can be dangerous. I think this might be better suited as a post deploy manual fix but maybe others can chime in

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just updated the migration, if that helps. Otherwise, we can go with your idea, if necessary. I just feel like it does need to be a migration just to make sure that anyone who pulls the code has the most current things in the database since these records are programmatically significant instead of arbitrary data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ito-Eta This is great work, but I think I agree with @amcates here in that this should be a rake task instead of a migration. We should only be using migrations for schema changes (the exception here is if you need to move data around as the result of a schema change). If you put a note in the release marker your rake task will get run when production is updated. Me and Sherly have a couple of them in there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Updated to rake task, the migration was deleted, and a note was added to the release marker.

@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2022_09_07_153451) do
ActiveRecord::Schema.define(version: 2022_09_19_141351) do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that this changed. I guess it doesn't cause any issues

Copy link
Contributor

@jleonardw9 jleonardw9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@amcates amcates merged commit c45b5d0 into v3.10.0 Sep 23, 2022
@amcates amcates deleted the ie-update_epic_questions branch September 23, 2022 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants