From e9d993b8f66d2cd9ca2f8d6f618f334bb4c3f071 Mon Sep 17 00:00:00 2001 From: Maxwell Salzberg Date: Tue, 9 Aug 2011 13:02:25 -0700 Subject: [PATCH] throw 404s when the person is no found --- app/controllers/people_controller.rb | 96 ++++++++++------------ app/models/person.rb | 14 ++++ public/404.html | 11 ++- spec/controllers/people_controller_spec.rb | 8 +- spec/models/person_spec.rb | 22 +++++ 5 files changed, 93 insertions(+), 58 deletions(-) diff --git a/app/controllers/people_controller.rb b/app/controllers/people_controller.rb index a627d2498..95aabbf1b 100644 --- a/app/controllers/people_controller.rb +++ b/app/controllers/people_controller.rb @@ -9,6 +9,10 @@ class PeopleController < ApplicationController respond_to :json, :only => [:index, :show] respond_to :js, :only => [:tag_index] + rescue_from ActiveRecord::RecordNotFound do + render :file => "#{Rails.root}/public/404.html", :layout => false, :status => 404 + end + def index @aspect = :search params[:q] ||= params[:term] || '' @@ -61,10 +65,10 @@ class PeopleController < ApplicationController end def show - @person = find_person_from_id_or_username - if @person && @person.remote? && !user_signed_in? - render :file => "#{Rails.root}/public/404.html", :layout => false, :status => 404 - return + @person = Person.find_from_id_or_username(params) + + if remote_profile_with_no_user_session? + raise ActiveRecord::RecordNotFound end @post_type = :all @@ -72,53 +76,48 @@ class PeopleController < ApplicationController @share_with = (params[:share_with] == 'true') max_time = params[:max_time] ? Time.at(params[:max_time].to_i) : Time.now - if @person - @profile = @person.profile + @profile = @person.profile - unless params[:format] == "json" # hovercard - if current_user - @contact = current_user.contact_for(@person) - @aspects_with_person = [] - if @contact && !params[:only_posts] - @aspects_with_person = @contact.aspects - @aspect_ids = @aspects_with_person.map(&:id) - @contacts_of_contact_count = @contact.contacts.count - @contacts_of_contact = @contact.contacts.limit(8) + unless params[:format] == "json" # hovercard + if current_user + @contact = current_user.contact_for(@person) + @aspects_with_person = [] + if @contact && !params[:only_posts] + @aspects_with_person = @contact.aspects + @aspect_ids = @aspects_with_person.map(&:id) + @contacts_of_contact_count = @contact.contacts.count + @contacts_of_contact = @contact.contacts.limit(8) - else - @contact ||= Contact.new - @contacts_of_contact_count = 0 - @contacts_of_contact = [] - end - - if (@person != current_user.person) && !@contact.persisted? - @commenting_disabled = true - else - @commenting_disabled = false - end - @posts = current_user.posts_from(@person).where(:type => ["StatusMessage", "Reshare", "ActivityStreams::Photo"]).includes(:comments).limit(15).where(StatusMessage.arel_table[:created_at].lt(max_time)) else + @contact ||= Contact.new + @contacts_of_contact_count = 0 + @contacts_of_contact = [] + end + + if (@person != current_user.person) && !@contact.persisted? @commenting_disabled = true - @posts = @person.posts.where(:type => ["StatusMessage", "Reshare", "ActivityStreams::Photo"], :public => true).includes(:comments).limit(15).where(StatusMessage.arel_table[:created_at].lt(max_time)).order('posts.created_at DESC') + else + @commenting_disabled = false end - @posts.includes(:author => :profile) - end - - if params[:only_posts] - render :partial => 'shared/stream', :locals => {:posts => @posts} + @posts = current_user.posts_from(@person).where(:type => ["StatusMessage", "Reshare", "ActivityStreams::Photo"]).includes(:comments).limit(15).where(StatusMessage.arel_table[:created_at].lt(max_time)) else - respond_to do |format| - format.all { respond_with @person, :locals => {:post_type => :all} } - format.json { - render :json => @person.to_json(:includes => params[:includes]) - } - end + @commenting_disabled = true + @posts = @person.posts.where(:type => ["StatusMessage", "Reshare", "ActivityStreams::Photo"], :public => true).includes(:comments).limit(15).where(StatusMessage.arel_table[:created_at].lt(max_time)).order('posts.created_at DESC') end - - else - flash[:error] = I18n.t 'people.show.does_not_exist' - redirect_to people_path + @posts.includes(:author => :profile) end + + if params[:only_posts] + render :partial => 'shared/stream', :locals => {:posts => @posts} + else + respond_to do |format| + format.all { respond_with @person, :locals => {:post_type => :all} } + format.json { + render :json => @person.to_json(:includes => params[:includes]) + } + end + end + end def retrieve_remote @@ -156,13 +155,8 @@ class PeopleController < ApplicationController Resque.enqueue(Job::SocketWebfinger, current_user.id, account, opts) end - def find_person_from_id_or_username - if params[:id].present? - Person.where(:id => params[:id]).first - elsif params[:username].present? - User.find_by_username(params[:username]).person - else - nil - end + + def remote_profile_with_no_user_session? + @person && @person.remote? && !user_signed_in? end end diff --git a/app/models/person.rb b/app/models/person.rb index 91e039080..d2674bbfc 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -49,6 +49,20 @@ class Person < ActiveRecord::Base def self.featured_users AppConfig[:featured_users].present? ? Person.where(:diaspora_handle => AppConfig[:featured_users]) : [] end + + + def self.find_from_id_or_username(params) + p = if params[:id].present? + Person.where(:id => params[:id]).first + elsif params[:username].present? && u = User.find_by_username(params[:username]) + u.person + else + nil + end + raise ActiveRecord::RecordNotFound unless p.present? + p + end + def self.search_query_string(query) query = query.downcase diff --git a/public/404.html b/public/404.html index 16d6fc05f..eccf9c6ed 100644 --- a/public/404.html +++ b/public/404.html @@ -12,13 +12,18 @@ border-right-color: #999; border-bottom-color: #999; } - h1 { font-size: 100%; color: #f00; line-height: 1.5em; } + h1 { font-size: 100%; color: #f00; line-height: 1.5em; text-align:center; } - - +

404: Not Found

+ + + +

+ Go Back +

diff --git a/spec/controllers/people_controller_spec.rb b/spec/controllers/people_controller_spec.rb index 90ad69e71..528ba4ace 100644 --- a/spec/controllers/people_controller_spec.rb +++ b/spec/controllers/people_controller_spec.rb @@ -117,14 +117,14 @@ describe PeopleController do end describe '#show' do - it "redirects to #index if the id is invalid" do + it "404s if the id is invalid" do get :show, :id => 'delicious' - response.should redirect_to people_path + response.code.should == "404" end - it "redirects to #index if no person is found" do + it "404s if no person is found" do get :show, :id => 3920397846 - response.should redirect_to people_path + response.code.should == "404" end it 'does not allow xss attacks' do diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index 3590e5763..13d9477d9 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -23,6 +23,28 @@ describe Person do Person.remote =~ [@user.person] end end + + describe '.find_person_from_id_or_username' do + it 'searchs for a person if id is passed' do + Person.find_from_id_or_username(:id => @person.id).id.should == @person.id + end + + it 'searchs a person from a user if username is passed' do + Person.find_from_id_or_username(:username => @user.username).id.should == @user.person.id + end + + it 'throws active record not found exceptions if no person is found via id' do + expect{ + Person.find_from_id_or_username(:id => 213123) + }.to raise_error ActiveRecord::RecordNotFound + end + + it 'throws active record not found exceptions if no person is found via username' do + expect{ + Person.find_from_id_or_username(:username => 'michael_jackson') + }.to raise_error ActiveRecord::RecordNotFound + end + end end describe "delegating" do it "delegates last_name to the profile" do