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
Jl calendar day bug #686
Jl calendar day bug #686
Conversation
@@ -216,7 +216,7 @@ def visits_select_options(arm, pages) | |||
(beginning_visit..ending_visit).each do |visit_number| | |||
visit_group = arm.visit_groups[visit_number - 1] | |||
|
|||
if visit_group.day.present? | |||
unless visit_group.day.nil? |
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.
if visit_group.day
would be equivalent, and a little more understandable, in my opinion.
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 agree. I'd like to see this changed before merge.
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 fine either way. I think I prefer it the way it is. Both ways make sense to me.
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 accidentally changed that when I was experimenting. I'll change it back.
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.
Reverted back to original.
@@ -48,6 +48,14 @@ class Arm < ActiveRecord::Base | |||
validates :visit_count, numericality: { greater_than: 0 } | |||
validates :subject_count, numericality: { greater_than: 0 } | |||
|
|||
validate do |arm| |
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.
Is there a better way of doing this? This seems too hard-coded
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 agree, but I couldn't figure out another way to make the validations chain back down to the visit groups. Let's think on it and try to refactor down the road.
No description provided.