throw 404s when the person is no found

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

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

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; }
   </style>
 </head>
 
 <body>
   <!-- This file lives in public/404.html -->
-	<img src="/images/404.png"/>
-	
+  <h1> 404: Not Found </h1>
+  <a href="javascript:history.go(-1)">
+    <img src="/images/404.png"/>
+  </a>
+  <h1>
+    <a href="javascript:history.go(-1)"> Go Back </a>
+  </h1>
 </body>
 </html>

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

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