Move sort order to a before filter, because this index method is waaaaay too big.

app/controllers/aspects_controller.rb

@@ -4,6 +4,7 @@
 
 class AspectsController < ApplicationController
   before_filter :authenticate_user!
+  before_filter :save_sort_order, :only => :index
 
   respond_to :html
   respond_to :json, :only => [:show, :create]
@@ -15,33 +16,27 @@ class AspectsController < ApplicationController
     else
       @aspects = current_user.aspects.includes(:contacts => {:person => :profile})
     end
-    @selected_contacts = @aspects.inject([]){|arr, aspect| arr.concat(aspect.contacts)}
+    @selected_contacts = @aspects.inject([]) { |arr, aspect| arr.concat(aspect.contacts) }
     @selected_contacts.uniq!
 
     # redirect to signup
     if (current_user.getting_started == true || @aspects.blank?) && !request.format.mobile? && !request.format.js?
       redirect_to getting_started_path
     else
-      if params[:sort_order].blank? and session[:sort_order].blank?
+      @aspect_ids = @aspects.map { |a| a.id }
-         session[:sort_order] = 'updated_at'
-      elsif not params[:sort_order].blank? and not session[:sort_order] == params[:sort_order]
-        session[:sort_order] = params[:sort_order] == 'created_at' ? 'created_at' : 'updated_at'
-      end
-      sort_order = session[:sort_order] == 'created_at' ? 'created_at' : 'updated_at'
-      @aspect_ids = @aspects.map{|a| a.id}
 
       @posts = StatusMessage.joins(:aspects).where(:pending => false,
-               :aspects => {:id => @aspect_ids}).includes(:comments, :photos, :likes, :dislikes).select('DISTINCT `posts`.*').paginate(
+                                                   :aspects => {:id => @aspect_ids}).includes(:comments, :photos, :likes, :dislikes).select('DISTINCT `posts`.*').paginate(
-               :page => params[:page], :per_page => 15, :order => sort_order + ' DESC')
+        :page => params[:page], :per_page => 15, :order => session[:sort_order] + ' DESC')
       @fakes = PostsFake.new(@posts)
 
       @contact_count = current_user.contacts.count
 
       @aspect = :all unless params[:a_ids]
       @aspect ||= @aspects.first #used in mobile
-
     end
   end
+
   def create
     @aspect = current_user.aspects.create(params[:aspect])
     #hack, we don't know why mass assignment is not working
@@ -61,12 +56,12 @@ class AspectsController < ApplicationController
 
         respond_to do |format|
           format.js { render :json => {:html => render_to_string(
-              :partial => 'aspects/aspect_list_item',
+            :partial => 'aspects/aspect_list_item',
-              :locals => {:aspect => @aspect,
+            :locals => {:aspect => @aspect,
-                            :person => @person,
+                        :person => @person,
-                            :contact => @contact}
+                        :contact => @contact}
-                                      ), :aspect_id => @aspect.id},:status => 201 }
+          ), :aspect_id => @aspect.id}, :status => 201 }
-              end
+        end
       else
         respond_with @aspect
       end
@@ -85,10 +80,10 @@ class AspectsController < ApplicationController
 
     begin
       current_user.drop_aspect @aspect
-      flash[:notice] = I18n.t 'aspects.destroy.success',:name => @aspect.name
+      flash[:notice] = I18n.t 'aspects.destroy.success', :name => @aspect.name
       redirect_to aspects_path
     rescue ActiveRecord::StatementInvalid => e
-      flash[:error] = I18n.t 'aspects.destroy.failure',:name => @aspect.name
+      flash[:error] = I18n.t 'aspects.destroy.failure', :name => @aspect.name
       redirect_to aspects_path
     end
   end
@@ -104,7 +99,7 @@ class AspectsController < ApplicationController
 
   def edit
     @aspect = current_user.aspects.where(:id => params[:id]).includes(:contacts => {:person => :profile}).first
-    @contacts = current_user.contacts.includes(:person => :profile).all.sort!{|x, y| x.person.name <=> y.person.name}.reverse!
+    @contacts = current_user.contacts.includes(:person => :profile).all.sort! { |x, y| x.person.name <=> y.person.name }.reverse!
     unless @aspect
       render :file => "#{Rails.root}/public/404.html", :layout => false, :status => 404
     else
@@ -124,13 +119,13 @@ class AspectsController < ApplicationController
   def update
     @aspect = current_user.aspects.where(:id => params[:id]).first
 
-    if @aspect.update_attributes!( params[:aspect] )
+    if @aspect.update_attributes!(params[:aspect])
       #hack, we don't know why mass assignment is not working
       @aspect.contacts_visible = params[:aspect][:contacts_visible]
       @aspect.save
-      flash[:notice] = I18n.t 'aspects.update.success',:name => @aspect.name
+      flash[:notice] = I18n.t 'aspects.update.success', :name => @aspect.name
     else
-      flash[:error] = I18n.t 'aspects.update.failure',:name => @aspect.name
+      flash[:error] = I18n.t 'aspects.update.failure', :name => @aspect.name
     end
 
     respond_with @aspect
@@ -146,4 +141,16 @@ class AspectsController < ApplicationController
     end
     @aspect.save
   end
+
+  protected
+
+  def save_sort_order
+    if params[:sort_order].present?
+      session[:sort_order] = (params[:sort_order] == 'created_at') ? 'created_at' : 'updated_at'
+    elsif session[:sort_order].blank?
+      session[:sort_order] = 'updated_at'
+    else
+      session[:sort_order] = (session[:sort_order] == 'created_at') ? 'created_at' : 'updated_at'
+    end
+  end
 end

spec/controllers/aspects_controller_spec.rb

@@ -126,6 +126,13 @@ describe AspectsController do
           get :index, :sort_order => "updated_at"
           assigns(:posts).should == @posts
         end
+
+        it "doesn't allow SQL injection" do
+          get :index, :sort_order => "\"; DROP TABLE users;"
+          assigns(:posts).should == @posts
+          get :index, :sort_order => "created_at"
+          assigns(:posts).should == @posts.reverse
+        end
       end
 
       it "returns all posts by default" do