Refactor ProblemController

Extract some instance variable in DecentExposure extract same usage of params in ProblemController and AppController

Refactor ProblemController
Extract some instance variable in DecentExposure extract same usage of params in ProblemController and AppController
Cyril Mougel
1 parent 7c977e48
Showing 10 changed files with 128 additions and 70 deletions Show diff stats
app/controllers/apps_controller.rb
app/controllers/problems_controller.rb
app/controllers/problems_searcher.rb
app/helpers/sort_helper.rb
app/interactors/issue_creation.rb
app/models/issue_tracker.rb
app/views/problems/_table.html.haml
app/views/problems/index.html.haml
spec/controllers/problems_controller_spec.rb
spec/views/problems/index.html.haml_spec.rb
 class AppsController < InheritedResources::Base
+
+  include ProblemsSearcher
+
   before_filter :require_admin!, :except => [:index, :show]
   before_filter :parse_email_at_notices_or_set_default, :only => [:create, :update]
   before_filter :parse_notice_at_notices_or_set_default, :only => [:create, :update]
@@ -9,16 +12,10 @@ class AppsController &lt; InheritedResources::Base
       format.html do
         @all_errs = !!params[:all_errs]
-        @sort  = params[:sort]
-        @order = params[:order]
-        @sort  = "last_notice_at" unless %w{message app last_deploy_at last_notice_at count}.member?(@sort)
-        @order = "desc" unless %w{asc desc}.member?(@order)
-
         @problems = resource.problems
         @problems = @problems.unresolved unless @all_errs
-        @problems = @problems.in_env(params[:environment]).ordered_by(@sort, @order).page(params[:page]).per(current_user.per_page)
+        @problems = @problems.in_env(params[:environment]).ordered_by(params_sort, params_order).page(params[:page]).per(current_user.per_page)
-        @selected_problems = params[:problems] || []
         @deploys = @app.deploys.order_by(:created_at.desc).limit(5)
       end
       format.atom do
@@ -6,8 +6,8 @@
 # COLLECTION => :index, :all, :destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several, :search
 class ProblemsController < ApplicationController
-  before_filter :set_sorting_params, :only => [:index, :all, :search]
-  before_filter :set_tracker_params, :only => [:create_issue]
+
+  include ProblemsSearcher
   before_filter :need_selected_problem, :only => [
     :resolve_several, :unresolve_several, :unmerge_several
@@ -25,25 +25,35 @@ class ProblemsController &lt; ApplicationController
     app.problems.find(params[:id])
   }
-  expose(:err_ids) {
-    (params[:problems] || []).compact
+
+  expose(:all_errs) {
+    params[:all_errs]
+  }
+
+  expose(:app_scope) {
+    apps = current_user.admin? ? App.all : current_user.apps
+    params[:app_id] ? apps.where(:_id => params[:app_id]) : apps
   }
-  expose(:selected_problems) {
-    Array(Problem.find(err_ids))
+  expose(:params_environement) {
+    params[:environment]
   }
-  def index
-    app_scope = current_user.admin? ? App.all : current_user.apps
-    @all_errs = params[:all_errs]
-    @problems = Problem.for_apps(app_scope).in_env(params[:environment]).all_else_unresolved(@all_errs).ordered_by(@sort, @order)
-    respond_to do |format|
-      format.html do
-        @problems = @problems.page(params[:page]).per(current_user.per_page)
-      end
-      format.atom
+  expose(:problems) {
+    pro = Problem.for_apps(
+      app_scope
+    ).in_env(
+      params_environement
+    ).all_else_unresolved(all_errs).ordered_by(params_sort, params_order)
+
+    if request.format == :html
+      pro.page(params[:page]).per(current_user.per_page)
+    else
+      pro
     end
-  end
+  }
+
+  def index; end
   def show
     @notices  = problem.notices.reverse_ordered.page(params[:notice]).per(1)
@@ -52,10 +62,11 @@ class ProblemsController &lt; ApplicationController
   end
   def create_issue
+    IssueTracker.update_url_options(request)
     issue_creation = IssueCreation.new(problem, current_user, params[:tracker])
     unless issue_creation.execute
-      flash[:error] = issue_creation.errors[:base].first
+      flash[:error] = issue_creation.errors.full_messages.join(', ')
     end
     redirect_to app_problem_path(app, problem)
@@ -114,12 +125,7 @@ class ProblemsController &lt; ApplicationController
   end
   def search
-    if params[:app_id]
-      app_scope = App.where(:_id => params[:app_id])
-    else
-      app_scope = current_user.admin? ? App.all : current_user.apps
-    end
-    @problems = Problem.search(params[:search]).for_apps(app_scope).in_env(params[:environment]).all_else_unresolved(params[:all_errs]).ordered_by(@sort, @order)
+    @problems = Problem.search(params[:search]).for_apps(app_scope).in_env(params[:environment]).all_else_unresolved(params[:all_errs]).ordered_by(params_sort, params_order)
     @selected_problems = params[:problems] || []
     @problems = @problems.page(params[:page]).per(current_user.per_page)
     render :content_type => 'text/javascript'
@@ -127,18 +133,6 @@ class ProblemsController &lt; ApplicationController
   protected
-    def set_tracker_params
-      IssueTracker.default_url_options[:host] = request.host
-      IssueTracker.default_url_options[:port] = request.port
-      IssueTracker.default_url_options[:protocol] = request.scheme
-    end
-
-    def set_sorting_params
-      @sort = params[:sort]
-      @sort = "last_notice_at" unless %w{app message last_notice_at last_deploy_at count}.member?(@sort)
-      @order = params[:order] || "desc"
-    end
-
   ##
   # Redirect :back if no errors selected
   #
@@ -0,0 +1,33 @@
+# Include to do a Search
+# TODO: Need to be in a Dedicated Object ProblemsSearch with params like input
+#
+module ProblemsSearcher
+  extend ActiveSupport::Concern
+
+  included do
+
+    expose(:params_sort) {
+      unless %w{app message last_notice_at last_deploy_at count}.member?(params[:sort])
+        "last_notice_at"
+      else
+        params[:sort]
+      end
+    }
+
+    expose(:params_order){
+      unless %w{asc desc}.member?(params[:order])
+        'desc'
+      else
+        params[:order]
+      end
+    }
+
+    expose(:selected_problems) {
+      Array(Problem.find(err_ids))
+    }
+
+    expose(:err_ids) {
+      (params[:problems] || []).compact
+    }
+  end
+end
 # encoding: utf-8
 module SortHelper
-  
+
   def link_for_sort(name, field=nil)
     field ||= name.underscore
-    current = (@sort == field)
-    order   = (current && (@order == "asc")) ? "desc" : "asc"
+    current = (params_sort == field)
+    order   = (current && (params_order == "asc")) ? "desc" : "asc"
     url     = request.path + "?sort=#{field}&order=#{order}"
     options = {}
     options.merge!(:class => "current #{order}") if current
     link_to(name, url, options)
   end
-  
+
 end
@@ -41,15 +41,11 @@ class IssueCreation
   end
   def execute
-    if tracker
-      begin
-        tracker.create_issue problem, user
-      rescue => ex
-        Rails.logger.error "Error during issue creation: " << ex.message
-        errors.add :base, "There was an error during issue creation: #{ex.message}"
-      end
-    end
-
+    tracker.create_issue problem, user if tracker
     errors.empty?
+  rescue => ex
+    Rails.logger.error "Error during issue creation: " << ex.message
+    errors.add :base, "There was an error during issue creation: #{ex.message}"
+    false
   end
 end
@@ -40,4 +40,15 @@ class IssueTracker
   def configured?
     project_id.present?
   end
+
+  ##
+  # Update default_url_option with valid data from the request information
+  #
+  # @param [ Request ] a request with host, port and protocol
+  #
+  def self.update_url_options(request)
+    IssueTracker.default_url_options[:host] = request.host
+    IssueTracker.default_url_options[:port] = request.port
+    IssueTracker.default_url_options[:protocol] = request.scheme
+  end
 end
@@ -16,7 +16,7 @@
       - problems.each do |problem|
         %tr{:class => problem.resolved? ? 'resolved' : 'unresolved'}
           %td.select
-            = check_box_tag "problems[]", problem.id, @selected_problems.member?(problem.id.to_s)
+            = check_box_tag "problems[]", problem.id, selected_problems.member?(problem.id.to_s)
           %td.app
             = link_to problem.app.name, app_path(problem.app)
             - if current_page?(:controller => 'problems')
 - content_for :title, @all_errs ? 'All Errors' : 'Unresolved Errors'
 - content_for :head do
   = auto_discovery_link_tag :atom, problems_path(User.token_authentication_key => current_user.authentication_token, :format => "atom"), :title => "Errbit notices at #{request.host}"
+
 - content_for :action_bar do
   - if @all_errs
     = link_to 'hide resolved', problems_path, :class => 'button'
   - else
     = link_to 'show resolved', problems_path(:all_errs => true), :class => 'button'
+
 %section
   = form_tag search_problems_path(:all_errs => @all_errs), :method => :get, :remote => true do
     = text_field_tag :search, params[:search], :placeholder => 'Search for issues'
 %br
 %section
-  .problem_table{:id => 'problem_table'}
-    = render 'problems/table', :problems => @problems
+  #problem_table.problem_table
+    = render 'problems/table'
@@ -27,13 +27,13 @@ describe ProblemsController do
         it "should have default per_page value for user" do
           get :index
-          assigns(:problems).to_a.size.should == User::PER_PAGE
+          controller.problems.to_a.size.should == User::PER_PAGE
         end
         it "should be able to override default per_page value" do
           @user.update_attribute :per_page, 10
           get :index
-          assigns(:problems).to_a.size.should == 10
+          controller.problems.to_a.size.should == 10
         end
       end
@@ -48,35 +48,35 @@ describe ProblemsController do
         context 'no params' do
           it 'shows problems for all environments' do
             get :index
-            assigns(:problems).size.should == 21
+            controller.problems.size.should == 21
           end
         end
         context 'environment production' do
           it 'shows problems for just production' do
             get :index, :environment => 'production'
-            assigns(:problems).size.should == 6
+            controller.problems.size.should == 6
           end
         end
         context 'environment staging' do
           it 'shows problems for just staging' do
             get :index, :environment => 'staging'
-            assigns(:problems).size.should == 5
+            controller.problems.size.should == 5
           end
         end
         context 'environment development' do
           it 'shows problems for just development' do
             get :index, :environment => 'development'
-            assigns(:problems).size.should == 5
+            controller.problems.size.should == 5
           end
         end
         context 'environment test' do
           it 'shows problems for just test' do
             get :index, :environment => 'test'
-            assigns(:problems).size.should == 5
+            controller.problems.size.should == 5
           end
         end
       end
@@ -89,8 +89,8 @@ describe ProblemsController do
         watched_unresolved_err = Fabricate(:err, :problem => Fabricate(:problem, :app => Fabricate(:user_watcher, :user => user).app, :resolved => false))
         watched_resolved_err = Fabricate(:err, :problem => Fabricate(:problem, :app => Fabricate(:user_watcher, :user => user).app, :resolved => true))
         get :index
-        assigns(:problems).should include(watched_unresolved_err.problem)
-        assigns(:problems).should_not include(unwatched_err.problem, watched_resolved_err.problem)
+        controller.problems.should include(watched_unresolved_err.problem)
+        controller.problems.should_not include(unwatched_err.problem, watched_resolved_err.problem)
       end
     end
   end
@@ -106,7 +106,7 @@ describe ProblemsController do
           mock('proxy', :page => mock('other_proxy', :per => problems))
         )
         get :index, :all_errs => true
-        assigns(:problems).should == problems
+        controller.problems.should == problems
       end
     end
@@ -117,8 +117,8 @@ describe ProblemsController do
         watched_unresolved_problem = Fabricate(:problem, :app => Fabricate(:user_watcher, :user => user).app, :resolved => false)
         watched_resolved_problem = Fabricate(:problem, :app => Fabricate(:user_watcher, :user => user).app, :resolved => true)
         get :index, :all_errs => true
-        assigns(:problems).should include(watched_resolved_problem, watched_unresolved_problem)
-        assigns(:problems).should_not include(unwatched_problem)
+        controller.problems.should include(watched_resolved_problem, watched_unresolved_problem)
+        controller.problems.should_not include(unwatched_problem)
       end
     end
   end
@@ -0,0 +1,25 @@
+require 'spec_helper'
+
+describe "problems/index.html.haml" do
+  let(:problem_1) { Fabricate(:problem) }
+  let(:problem_2) { Fabricate(:problem, :app => problem_1.app) }
+
+  before do
+    # view.stub(:app).and_return(problem.app)
+    view.stub(:selected_problems).and_return([])
+    view.stub(:problems).and_return(Kaminari.paginate_array([problem_1, problem_2]).page(1).per(10))
+    view.stub(:params_sort).and_return('asc')
+    controller.stub(:current_user) { Fabricate(:user) }
+  end
+
+  describe "with problem" do
+    before { problem_1 && problem_2 }
+
+    it 'should works' do
+      render
+      rendered.should have_selector('div#problem_table.problem_table')
+    end
+  end
+
+end
+
@@ -0,0 +1,33 @@		@@ -0,0 +1,33 @@
	1	+# Include to do a Search
	2	+# TODO: Need to be in a Dedicated Object ProblemsSearch with params like input
	3	+#
	4	+module ProblemsSearcher
	5	+ extend ActiveSupport::Concern
	6	+
	7	+ included do
	8	+
	9	+ expose(:params_sort) {
	10	+ unless %w{app message last_notice_at last_deploy_at count}.member?(params[:sort])
	11	+ "last_notice_at"
	12	+ else
	13	+ params[:sort]
	14	+ end
	15	+ }
	16	+
	17	+ expose(:params_order){
	18	+ unless %w{asc desc}.member?(params[:order])
	19	+ 'desc'
	20	+ else
	21	+ params[:order]
	22	+ end
	23	+ }
	24	+
	25	+ expose(:selected_problems) {
	26	+ Array(Problem.find(err_ids))
	27	+ }
	28	+
	29	+ expose(:err_ids) {
	30	+ (params[:problems] \|\| []).compact
	31	+ }
	32	+ end
	33	+end
	@@ -40,4 +40,15 @@ class IssueTracker		@@ -40,4 +40,15 @@ class IssueTracker
40	def configured?	40	def configured?
41	project_id.present?	41	project_id.present?
42	end	42	end
		43	+
		44	+ ##
		45	+ # Update default_url_option with valid data from the request information
		46	+ #
		47	+ # @param [ Request ] a request with host, port and protocol
		48	+ #
		49	+ def self.update_url_options(request)
		50	+ IssueTracker.default_url_options[:host] = request.host
		51	+ IssueTracker.default_url_options[:port] = request.port
		52	+ IssueTracker.default_url_options[:protocol] = request.scheme
		53	+ end
43	end	54	end
	@@ -16,7 +16,7 @@		@@ -16,7 +16,7 @@
16	- problems.each do \|problem\|	16	- problems.each do \|problem\|
17	%tr{:class => problem.resolved? ? 'resolved' : 'unresolved'}	17	%tr{:class => problem.resolved? ? 'resolved' : 'unresolved'}
18	%td.select	18	%td.select
19	- = check_box_tag "problems[]", problem.id, @selected_problems.member?(problem.id.to_s)	19	+ = check_box_tag "problems[]", problem.id, selected_problems.member?(problem.id.to_s)
20	%td.app	20	%td.app
21	= link_to problem.app.name, app_path(problem.app)	21	= link_to problem.app.name, app_path(problem.app)
22	- if current_page?(:controller => 'problems')	22	- if current_page?(:controller => 'problems')
@@ -0,0 +1,25 @@		@@ -0,0 +1,25 @@
	1	+require 'spec_helper'
	2	+
	3	+describe "problems/index.html.haml" do
	4	+ let(:problem_1) { Fabricate(:problem) }
	5	+ let(:problem_2) { Fabricate(:problem, :app => problem_1.app) }
	6	+
	7	+ before do
	8	+ # view.stub(:app).and_return(problem.app)
	9	+ view.stub(:selected_problems).and_return([])
	10	+ view.stub(:problems).and_return(Kaminari.paginate_array([problem_1, problem_2]).page(1).per(10))
	11	+ view.stub(:params_sort).and_return('asc')
	12	+ controller.stub(:current_user) { Fabricate(:user) }
	13	+ end
	14	+
	15	+ describe "with problem" do
	16	+ before { problem_1 && problem_2 }
	17	+
	18	+ it 'should works' do
	19	+ render
	20	+ rendered.should have_selector('div#problem_table.problem_table')
	21	+ end
	22	+ end
	23	+
	24	+end
	25	+