-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add option to control if the questions is required or not #166
Conversation
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.
Code changes look great! I'd love to see two tests exercising these new changes: one in the relevant model spec, and one in the relevant request spec to ensure that we don't regress the behavior you've added in the future. LMK if you need clarity on what these tests should look like
Done |
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.
Nice work! One small change remaining and then it’ll be ready to merge.
@@ -12,12 +12,13 @@ | |||
sign_in admin | |||
post admin_study_survey_questions_path(study, survey), | |||
params: { | |||
question: { content_attributes: { type: 'label', label_text: 'Question' } } | |||
question: { content_attributes: { type: 'label', required: true, label_text: 'Question' } } |
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.
Can you change the value of required to false so we can verify that the value changes through the controller work as expected?
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.
of course
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.
Done
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.
great work 🚀
Fixes #165.
Added a checkbox to control if a question requires a response or not