-
Notifications
You must be signed in to change notification settings - Fork 6
Filters on junior story model #16
Changes from 18 commits
ef6471a
d755bc3
fce0c7a
ea3ee19
27c156b
5db541b
daadee6
55d087d
5f37082
16fbfe1
7541125
429689a
6a6534f
8e51c28
022b352
af43203
7a55b10
a94752c
ff6ba83
0220014
e9e4dbe
84979cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,3 +15,4 @@ | |
/log/* | ||
!/log/.keep | ||
/tmp | ||
*.swp |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,15 @@ | ||
class JuniorStoriesController < ApplicationController | ||
|
||
def index | ||
@junior_stories = JuniorStory.all | ||
@junior_stories = JuniorStory. | ||
filter(allowed_params) | ||
|
||
@fields = fields | ||
@csv_params = allowed_params.merge({ format: 'csv' }) | ||
|
||
respond_to do |format| | ||
format.html | ||
format.csv { send_data @junior_stories.to_csv, type: 'application/csv' } | ||
format.csv { send_data @junior_stories.to_csv, type: 'application/csv', filename: 'junior_stories.csv' } | ||
end | ||
end | ||
|
||
|
@@ -33,4 +37,41 @@ def junior_story_params | |
:remote, :tech_team_size, :company_size, :company_age, :person_of_colour, :other, | ||
:publishing_consent, :freelancer) | ||
end | ||
|
||
def allowed_params | ||
params.permit(:job, :happy_in_job, :happy_info, | ||
:gender, :city, :country, :days_per_week, :salary, :currency, :technology, :focus, | ||
:age, :years_working_in_total, :years_working_at_job, :education, :first_job, | ||
:remote, :tech_team_size, :company_size, :company_age, :person_of_colour, :other, | ||
:publishing_consent, :freelancer) | ||
end | ||
|
||
# TODO: shouldn't hard code this, but I like having demo search terms in the list | ||
def fields | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would think that this belongs in the model, not the controller. wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this really truly belongs anywhere^^ What makes you say it's better suited for the model? Thin controllers - fat models? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup :) |
||
{ | ||
job: "junior developer", | ||
happy_in_job: "yes", | ||
happy_info: "It's ok", | ||
gender: "female", | ||
city: "Berlin", | ||
country: "DE", | ||
days_per_week: "5", | ||
salary: "15000", | ||
currency: "€", | ||
technology: "RoR", | ||
focus: "backend", | ||
age: 30, | ||
years_working_total: "less than 1 year", | ||
years_working_at_job: "less than 1 year", | ||
education: "self taught", | ||
first_job: "no", | ||
remote: "no", | ||
tech_team_size: "5 - 10 people", | ||
company_size: "less than 10 people", | ||
company_age: 5, | ||
person_of_colour: false, | ||
other: "Thanks", | ||
freelancer: "f" | ||
} | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
module Filterable | ||
extend ActiveSupport::Concern | ||
|
||
module ClassMethods | ||
def filter(params) | ||
results = self.all | ||
params.each do |key, value| | ||
if results.column_names.include? key | ||
results = results.where(key => value) if value.present? | ||
end | ||
end | ||
results | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,22 +2,80 @@ | |
|
||
describe JuniorStoriesController do | ||
describe 'GET index' do | ||
it 'assigns @junior_stories' do | ||
story = build :junior_story | ||
story.save | ||
get :index | ||
expect(assigns(:junior_stories)).to eq([story]) | ||
context 'without filter params' do | ||
it 'assigns @junior_stories' do | ||
story = build :junior_story | ||
story.save | ||
get :index | ||
expect(assigns(:junior_stories)).to eq([story]) | ||
end | ||
|
||
it 'renders the index template' do | ||
get :index | ||
expect(response).to render_template('index') | ||
end | ||
|
||
it 'does not render index, but instead responds with csv' do | ||
get :index, format: 'csv' | ||
expect(response).not_to render_template('index') | ||
expect(response.content_type).to include 'application/csv' | ||
end | ||
end | ||
|
||
it 'renders the index template' do | ||
get :index | ||
expect(response).to render_template('index') | ||
context 'with legal filter params' do | ||
it 'returns only the filter appropriate junior_story' do | ||
female_story = build :junior_story | ||
female_story.gender = 'female' | ||
female_story.save | ||
|
||
male_story = build :junior_story | ||
male_story.gender = 'male' | ||
male_story.save | ||
|
||
|
||
get :index, { 'gender' => 'female' } | ||
expect(assigns(:junior_stories)).to eq([female_story]) | ||
end | ||
|
||
it 'renders the index template' do | ||
get :index, { 'gender' => 'female' } | ||
get :index | ||
expect(response).to render_template('index') | ||
end | ||
|
||
it 'does not render index, but instead responds with csv' do | ||
get :index, { 'gender' => 'female', 'format' => 'csv' } | ||
expect(response).not_to render_template('index') | ||
expect(response.content_type).to include 'application/csv' | ||
end | ||
end | ||
|
||
it 'does not render index, but instead responds with csv' do | ||
get :index, format: 'csv' | ||
expect(response).not_to render_template('index') | ||
expect(response.content_type).to include 'application/csv' | ||
context 'with illegal filter params' do | ||
it 'ignores illegal filter params but respects legal filter params' do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I can see, you are testing here the possibility of someone sending illegal params through the filter... is that right? Does this need testing? Isn't this handled by strong parameters anyway? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a more useful spec could be a feature spec whereby a user chooses a filter & then gets the right junior story back. |
||
female_story = build :junior_story | ||
female_story.gender = 'female' | ||
female_story.save | ||
|
||
male_story = build :junior_story | ||
male_story.gender = 'male' | ||
male_story.save | ||
|
||
|
||
get :index, { 'makes_no_sense' => 'random', 'gender' => 'female' } | ||
expect(assigns(:junior_stories)).to eq([female_story]) | ||
end | ||
|
||
it 'renders the index template' do | ||
get :index, { 'makes_no_sense' => 'random' } | ||
get :index | ||
expect(response).to render_template('index') | ||
end | ||
|
||
it 'does not render index, but instead responds with csv' do | ||
get :index, { 'makes_no_sense' => 'random', 'format' => 'csv' } | ||
expect(response).not_to render_template('index') | ||
expect(response.content_type).to include 'application/csv' | ||
end | ||
end | ||
end | ||
end |
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.
these values are exactly the same as the
junior_story_params
above, aren't they - what's the benefit of doing it this way?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.
Nope, the difference is in the
require(:junior_story)
. Do we really need that above?