Little refactoring of action in ProblemController

Cyril Mougel
1 parent 51c1d1bb
Showing 4 changed files with 69 additions and 39 deletions Show diff stats
app/controllers/problems_controller.rb
config/locales/en.yml
spec/controllers/problems_controller_spec.rb
spec/models/problem_spec.rb
@@ -6,10 +6,13 @@
 # COLLECTION => :index, :all, :destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several, :search
 class ProblemsController < ApplicationController
-  before_filter :find_selected_problems, :only => [:destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several]
   before_filter :set_sorting_params, :only => [:index, :all, :search]
   before_filter :set_tracker_params, :only => [:create_issue]
+  before_filter :need_selected_problem, :only => [
+    :resolve_several, :unresolve_several, :unmerge_several
+  ]
+
   expose(:app) {
     if current_user.admin?
       App.find(params[:app_id])
@@ -17,15 +20,23 @@ class ProblemsController &lt; ApplicationController
       current_user.apps.find(params[:app_id])
     end
   }
+
   expose(:problem) {
     app.problems.find(params[:id])
   }
+  expose(:err_ids) {
+    (params[:problems] || []).compact
+  }
+
+  expose(:selected_problems) {
+    Array(Problem.find(err_ids))
+  }
+
   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)
-    @selected_problems = params[:problems] || []
     respond_to do |format|
       format.html do
         @problems = @problems.page(params[:page]).per(current_user.per_page)
@@ -64,35 +75,40 @@ class ProblemsController &lt; ApplicationController
   end
   def resolve_several
-    @selected_problems.each(&:resolve!)
-    flash[:success] = "Great news everyone! #{I18n.t(:n_errs_have, :count => @selected_problems.count)} been resolved."
+    selected_problems.each(&:resolve!)
+    flash[:success] = "Great news everyone! #{I18n.t(:n_errs_have, :count => selected_problems.count)} been resolved."
     redirect_to :back
   end
   def unresolve_several
-    @selected_problems.each(&:unresolve!)
-    flash[:success] = "#{I18n.t(:n_errs_have, :count => @selected_problems.count)} been unresolved."
+    selected_problems.each(&:unresolve!)
+    flash[:success] = "#{I18n.t(:n_errs_have, :count => selected_problems.count)} been unresolved."
     redirect_to :back
   end
+  ##
+  # Action to merge several Problem in One problem
+  #
+  # @param [ Array<String> ] :problems the list of problem ids
+  #
   def merge_several
-    if @selected_problems.length < 2
-      flash[:notice] = "You must select at least two errors to merge"
+    if selected_problems.length < 2
+      flash[:notice] = I18n.t('controllers.problems.flash.need_two_errors_merge')
     else
-      @merged_problem = Problem.merge!(@selected_problems)
-      flash[:notice] = "#{@selected_problems.count} errors have been merged."
+      ProblemMerge.new(selected_problems).merge
+      flash[:notice] = I18n.t('controllers.problems.flash.merge_several.success', :nb => selected_problems.count)
     end
     redirect_to :back
   end
   def unmerge_several
-    all = @selected_problems.map(&:unmerge!).flatten
+    all = selected_problems.map(&:unmerge!).flatten
     flash[:success] = "#{I18n.t(:n_errs_have, :count => all.length)} been unmerged."
     redirect_to :back
   end
   def destroy_several
-    nb_problem_destroy = ProblemDestroy.execute(@selected_problems)
+    nb_problem_destroy = ProblemDestroy.execute(selected_problems)
     flash[:notice] = "#{I18n.t(:n_errs_have, :count => nb_problem_destroy)} been deleted."
     redirect_to :back
   end
@@ -117,20 +133,20 @@ class ProblemsController &lt; ApplicationController
       IssueTracker.default_url_options[:protocol] = request.scheme
     end
-    def find_selected_problems
-      err_ids = (params[:problems] || []).compact
-      if err_ids.empty?
-        flash[:notice] = "You have not selected any errors"
-        redirect_to :back
-      else
-        @selected_problems = Array(Problem.find(err_ids))
-      end
-    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
+  #
+  def need_selected_problem
+    if err_ids.empty?
+      flash[:notice] = I18n.t('controllers.problems.flash.no_select_problem')
+      redirect_to :back
+    end
+  end
 end
@@ -2,7 +2,7 @@
 # See http://github.com/svenfuchs/rails-i18n/tree/master/rails%2Flocale for starting points.
 en:
-  hello: "Hello world"
+
   flash:
     apps:
       create:
@@ -11,6 +11,7 @@ en:
         success: "Good news everyone! '%{app_name}' was successfully updated."
       destroy:
         success: "'%{app_name}' was successfully destroyed."
+
   n_errs_have:
     one:   "%{count} err has"
     other: "%{count} errs have"
@@ -38,6 +39,12 @@ en:
           error: "You can't delete yourself"
         update:
           success: "%{name}'s information was successfully updated."
+    problems:
+      flash:
+        no_select_problem: "You have not selected any errors"
+        need_two_errors_merge: "You must select at least two errors to merge"
+        merge_several:
+          success: "%{nb} errors have been merged."
   devise:
     registrations:
@@ -341,31 +341,25 @@ describe ProblemsController do
       @problem2 = Fabricate(:err, :problem => Fabricate(:problem, :resolved => false)).problem
     end
-    it "should apply to multiple problems" do
-      post :resolve_several, :problems => [@problem1.id.to_s, @problem2.id.to_s]
-      assigns(:selected_problems).should == [@problem1, @problem2]
-    end
-
-    it "should require at least one problem" do
-      post :resolve_several, :problems => []
-      request.flash[:notice].should match(/You have not selected any/)
-    end
-
     context "POST /problems/merge_several" do
       it "should require at least two problems" do
         post :merge_several, :problems => [@problem1.id.to_s]
-        request.flash[:notice].should match(/You must select at least two/)
+        request.flash[:notice].should eql I18n.t('controllers.problems.flash.need_two_errors_merge')
       end
       it "should merge the problems" do
-        lambda {
-          post :merge_several, :problems => [@problem1.id.to_s, @problem2.id.to_s]
-          assigns(:merged_problem).reload.errs.length.should == 2
-        }.should change(Problem, :count).by(-1)
+        ProblemMerge.should_receive(:new).and_return(double(:merge => true))
+        post :merge_several, :problems => [@problem1.id.to_s, @problem2.id.to_s]
       end
     end
     context "POST /problems/unmerge_several" do
+
+      it "should require at least one problem" do
+        post :unmerge_several, :problems => []
+        request.flash[:notice].should eql I18n.t('controllers.problems.flash.no_select_problem')
+      end
+
       it "should unmerge a merged problem" do
         merged_problem = Problem.merge!(@problem1, @problem2)
         merged_problem.errs.length.should == 2
@@ -374,9 +368,16 @@ describe ProblemsController do
           merged_problem.reload.errs.length.should == 1
         }.should change(Problem, :count).by(1)
       end
+
     end
     context "POST /problems/resolve_several" do
+
+      it "should require at least one problem" do
+        post :resolve_several, :problems => []
+        request.flash[:notice].should eql I18n.t('controllers.problems.flash.no_select_problem')
+      end
+
       it "should resolve the issue" do
         post :resolve_several, :problems => [@problem2.id.to_s]
         @problem2.reload.resolved?.should == true
@@ -390,10 +391,17 @@ describe ProblemsController do
       it "should display a message about 2 errs" do
         post :resolve_several, :problems => [@problem1.id.to_s, @problem2.id.to_s]
         flash[:success].should match(/2 errs have been resolved/)
+        controller.selected_problems.should == [@problem1, @problem2]
       end
     end
     context "POST /problems/unresolve_several" do
+
+      it "should require at least one problem" do
+        post :unresolve_several, :problems => []
+        request.flash[:notice].should eql I18n.t('controllers.problems.flash.no_select_problem')
+      end
+
       it "should unresolve the issue" do
         post :unresolve_several, :problems => [@problem1.id.to_s]
         @problem1.reload.resolved?.should == false
@@ -152,7 +152,6 @@ describe Problem do
     end
   end
-
   context "Scopes" do
     context "resolved" do
       it 'only finds resolved Problems' do
	@@ -2,7 +2,7 @@		@@ -2,7 +2,7 @@
2	# See http://github.com/svenfuchs/rails-i18n/tree/master/rails%2Flocale for starting points.	2	# See http://github.com/svenfuchs/rails-i18n/tree/master/rails%2Flocale for starting points.
3		3
4	en:	4	en:
5	- hello: "Hello world"	5	+
6	flash:	6	flash:
7	apps:	7	apps:
8	create:	8	create:
	@@ -11,6 +11,7 @@ en:		@@ -11,6 +11,7 @@ en:
11	success: "Good news everyone! '%{app_name}' was successfully updated."	11	success: "Good news everyone! '%{app_name}' was successfully updated."
12	destroy:	12	destroy:
13	success: "'%{app_name}' was successfully destroyed."	13	success: "'%{app_name}' was successfully destroyed."
		14	+
14	n_errs_have:	15	n_errs_have:
15	one: "%{count} err has"	16	one: "%{count} err has"
16	other: "%{count} errs have"	17	other: "%{count} errs have"
	@@ -38,6 +39,12 @@ en:		@@ -38,6 +39,12 @@ en:
38	error: "You can't delete yourself"	39	error: "You can't delete yourself"
39	update:	40	update:
40	success: "%{name}'s information was successfully updated."	41	success: "%{name}'s information was successfully updated."
		42	+ problems:
		43	+ flash:
		44	+ no_select_problem: "You have not selected any errors"
		45	+ need_two_errors_merge: "You must select at least two errors to merge"
		46	+ merge_several:
		47	+ success: "%{nb} errors have been merged."
41		48
42	devise:	49	devise:
43	registrations:	50	registrations:
	@@ -152,7 +152,6 @@ describe Problem do		@@ -152,7 +152,6 @@ describe Problem do
152	end	152	end
153	end	153	end
154		154
155	-
156	context "Scopes" do	155	context "Scopes" do
157	context "resolved" do	156	context "resolved" do
158	it 'only finds resolved Problems' do	157	it 'only finds resolved Problems' do