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

Move search logic from controller into model #25

Open
wants to merge 6 commits into
base: dry-up-posts-controller
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 6 additions & 40 deletions app/controllers/posts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@ class PostsController < ApplicationController
:confirm]

def index
if check_for_search_params
@posts = post_search
else
@posts = Post.active
end
@posts = Post.active
.filter_near(search_params[:location])
Copy link
Member

Choose a reason for hiding this comment

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

I assume these all just handle 'nil' gracefully in the queries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea! I did it wrong (see next comment) but I fixed it!

.filter_query(search_params[:query])
.filter_type(search_params[:type])
end

def new
Expand Down Expand Up @@ -89,40 +88,7 @@ def find_post_params
params.permit(:id, :validation)
end

def check_for_search_params
[:type, :location, :query].any? { |k| params.key?(k) }
end

def query_location(posts)
if params[:location].blank?
posts
else
posts.near(params[:location])
end
end

def query_description(posts)
if params[:query].blank?
posts
else
posts.search(params[:query])
end
end

def filter_type(posts)
if params[:type] == "available"
posts.available
elsif params[:type] == "needed"
posts.needed
else
posts
end
end

def post_search
posts = Post.active
posts = query_location(posts)
posts = query_description(posts)
filter_type(posts)
def search_params
params.permit(:location, :query, :type)
end
end
28 changes: 28 additions & 0 deletions app/models/post.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,34 @@ def address
[street, city, state, postal_code, country].compact.join(", ")
end

def self.filter_type(type)
if type == "needed" || type == "available"
send(type.to_sym)
Copy link
Member

Choose a reason for hiding this comment

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

what's the reason for 'send' in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just making our conditional shorter, and DRYing it up. I should make this less clever though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was leveraging the enum helpers but that's not needed, especially when I make it all complicated by using send 😂

else
all
Copy link
Member

Choose a reason for hiding this comment

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

for some reason I'm not able to find docs for an 'all' method anywhere. I can infer what it's doing, but is this a builtin for rails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just like calling Post.all, but since we're within Post, we can just say all. This lets us chain them.
For example, you can say Post.where(id: [1,2,3,4,5]).all.all.all.all and it just returns the where.
http://apidock.com/rails/ActiveRecord/Scoping/Named/ClassMethods/all

Copy link
Member

Choose a reason for hiding this comment

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

I don't know why I didn't realize this haha. Probably would have answered that immediately by opening a terminal :P.

end
end

def self.filter_near(location)
results = near(location)

if results.blank?
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this going to return everything if there is nothing within the range? Wouldn't we prefer a 'there were no results' in this case and return something else here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, here I was filtering based on the results were blank, I should've been checking if the query was blank.

all
else
results
end
end

def self.filter_query(query)
results = search(query)

if results.blank?
all
else
results
end
end

def self.generate_validation
require "securerandom"
# a collision here has low probability, but might as well check
Expand Down
25 changes: 21 additions & 4 deletions spec/controllers/post_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
describe "GET #index" do
it "responds successfully with an HTTP 200 status code" do
get :index

expect(response).to be_success
Copy link
Member

Choose a reason for hiding this comment

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

probably should have a different description, since it's not checking for a '200' anymore, but 2xx. I wouldn't be opposed to just the have_http_status(200), since that's always the code we would want in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always thought :success was just for 200, but that's not the case!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

expect(response).to have_http_status(200)
end

it "renders the index template" do
Expand All @@ -21,7 +21,7 @@
expect(assigns(:posts)).to match_array([post1, post2])
end

it "searches for location" do
it "searches by location" do
post1 = create(
:boston_post,
show: true
Expand All @@ -37,7 +37,24 @@
expect(assigns(:posts)).to match_array([post1])
end

it "searches for description" do
it "searches by title" do
post1 = create(
:post,
title: "Xyz",
show: true
)

create(
:post,
show: true
)

get :index, query: "xyz"
Copy link
Member

Choose a reason for hiding this comment

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

I prefer title = 'Xyz' -> post1 = create( title: title) -> get :index query title to ensure they're the same. Seeing case difference here. Also maybe make the other post have a different title rather than a default value so it makes the filter more explicit in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

def better!


expect(assigns(:posts)).to match_array([post1])
end

it "searches by description" do
post1 = create(
:post,
description: "Need housing in Boston",
Expand All @@ -54,7 +71,7 @@
expect(assigns(:posts)).to match_array([post1])
end

it "searches for description and location" do
it "searches by description and location" do
post1 = create(
:boston_post,
description: "Need housing in Boston",
Expand Down
2 changes: 1 addition & 1 deletion spec/features/post_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
expect(page).to have_content post.title
end

it "shows a post with a query in it's description" do
it "shows a post with a query in its description" do
post1 = create(
:available_post,
description: "I want some tofu pups",
Expand Down