-
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 start_date and end_date to surveys #179
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.
This is looking pretty solid! I'd like to see one more test added that validates that only surveys that meet the filter_currently_active
scope's criteria are included in the API output.
app/models/concerns/filterable.rb
Outdated
module Filterable | ||
extend ActiveSupport::Concern | ||
|
||
module ClassMethods |
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.
does this end up getting used anywhere? if not, please delete. if so, please include a doc comment that explains what it does and how it works.
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 Filterable module was intended to reduce repetitive code when applying multiple scopes in the controller. It dynamically applied filters based on provided parameters, but it's not currently in use, so I’m removing it to clean up the codebase
here is the full document:
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 test is added now
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 really good! I have a couple questions and some minor changes that I would like to see made before merging.
@@ -36,4 +43,20 @@ def require_stop_id_sensibility | |||
|
|||
errors.add(:require_stop_id_in_response, 'is only valid if Show on Map is false') | |||
end | |||
|
|||
def validate_date_presence |
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 whole method is very idiomatic Ruby. Great work!
spec/models/survey_spec.rb
Outdated
RSpec.describe Survey, type: :model do | ||
let(:study) { Study.create } # Assuming you have a Study model |
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.
why do you need this?
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 think we don't need it I forgot to remove it
spec/requests/admins/surveys_spec.rb
Outdated
@@ -31,6 +31,12 @@ | |||
post admin_study_surveys_path(study), params: { survey: attributes_for(:survey) } | |||
expect(response).to redirect_to(admin_study_survey_path(study, Survey.last)) | |||
end | |||
|
|||
it "does not create a new survey end_date is before start_date" do |
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 don't understand what this is testing. I would think based upon the use of the invalid
trait and this text does not create a new survey end_date is before start_date
that Survey.count wouldn't change. what am I misunderstanding?
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 think the issue is that when we create a study, it automatically creates a survey, so the initial Survey.count is set to 1. This is why the count changes in the test even when we expect it not to, so if we will create a new survey the with study the count will be 2 not 1
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.
looking good!
No description provided.