Commit d0894cbe2f38472d24a64b400a31136530fef611

Authored by Cyril Mougel
1 parent 0b34e1a5
Exists in master and in 1 other branch production

[WIP] Start refactoring the problems_controller.

1) Limit some before_filter using the expose system
2) Extract some view test in view spec instead of controller spec
app/controllers/problems_controller.rb
  1 +##
  2 +# Manage problems
  3 +#
  4 +# List of actions available :
  5 +# MEMBER => :show, :edit, :update, :create, :destroy, :resolve, :unresolve, :create_issue, :unlink_issue
  6 +# COLLECTION => :index, :all, :destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several, :search
1 class ProblemsController < ApplicationController 7 class ProblemsController < ApplicationController
2 - before_filter :find_app, :except => [:index, :all, :destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several, :search] 8 +
3 before_filter :find_problem, :except => [:index, :all, :destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several, :search] 9 before_filter :find_problem, :except => [:index, :all, :destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several, :search]
4 before_filter :find_selected_problems, :only => [:destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several] 10 before_filter :find_selected_problems, :only => [:destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several]
5 before_filter :set_sorting_params, :only => [:index, :all, :search] 11 before_filter :set_sorting_params, :only => [:index, :all, :search]
6 before_filter :set_tracker_params, :only => [:create_issue] 12 before_filter :set_tracker_params, :only => [:create_issue]
7 13
  14 + expose(:app) {
  15 + if current_user.admin?
  16 + App.find(params[:app_id])
  17 + else
  18 + current_user.apps.find(params[:app_id])
  19 + end
  20 + }
  21 +
8 def index 22 def index
9 app_scope = current_user.admin? ? App.all : current_user.apps 23 app_scope = current_user.admin? ? App.all : current_user.apps
10 @all_errs = params[:all_errs] 24 @all_errs = params[:all_errs]
@@ -31,12 +45,12 @@ class ProblemsController &lt; ApplicationController @@ -31,12 +45,12 @@ class ProblemsController &lt; ApplicationController
31 flash[:error] = issue_creation.errors[:base].first 45 flash[:error] = issue_creation.errors[:base].first
32 end 46 end
33 47
34 - redirect_to app_problem_path(@app, @problem) 48 + redirect_to app_problem_path(app, @problem)
35 end 49 end
36 50
37 def unlink_issue 51 def unlink_issue
38 @problem.update_attribute :issue_link, nil 52 @problem.update_attribute :issue_link, nil
39 - redirect_to app_problem_path(@app, @problem) 53 + redirect_to app_problem_path(app, @problem)
40 end 54 end
41 55
42 def resolve 56 def resolve
@@ -44,7 +58,7 @@ class ProblemsController &lt; ApplicationController @@ -44,7 +58,7 @@ class ProblemsController &lt; ApplicationController
44 flash[:success] = 'Great news everyone! The err has been resolved.' 58 flash[:success] = 'Great news everyone! The err has been resolved.'
45 redirect_to :back 59 redirect_to :back
46 rescue ActionController::RedirectBackError 60 rescue ActionController::RedirectBackError
47 - redirect_to app_path(@app) 61 + redirect_to app_path(app)
48 end 62 end
49 63
50 def resolve_several 64 def resolve_several
@@ -94,16 +108,9 @@ class ProblemsController &lt; ApplicationController @@ -94,16 +108,9 @@ class ProblemsController &lt; ApplicationController
94 end 108 end
95 109
96 protected 110 protected
97 - def find_app  
98 - @app = App.find(params[:app_id])  
99 -  
100 - # Mongoid Bug: could not chain: current_user.apps.find_by_id!  
101 - # apparently finding by 'watchers.email' and 'id' is broken  
102 - raise(Mongoid::Errors::DocumentNotFound.new(App,@app.id)) unless current_user.admin? || current_user.watching?(@app)  
103 - end  
104 111
105 def find_problem 112 def find_problem
106 - @problem = @app.problems.find(params[:id]) 113 + @problem = app.problems.find(params[:id])
107 end 114 end
108 115
109 def set_tracker_params 116 def set_tracker_params
app/views/problems/_issue_tracker_links.html.haml
1 -- if @app.issue_tracker_configured? || current_user.github_account? 1 +- if app.issue_tracker_configured? || current_user.github_account?
2 - if @problem.issue_link.present? 2 - if @problem.issue_link.present?
3 %span= link_to 'go to issue', @problem.issue_link, :class => "#{@problem.issue_type}_goto goto-issue" 3 %span= link_to 'go to issue', @problem.issue_link, :class => "#{@problem.issue_type}_goto goto-issue"
4 - = link_to 'unlink issue', unlink_issue_app_problem_path(@app, @problem), :method => :delete, :data => { :confirm => "Unlink err issues?" }, :class => "unlink-issue" 4 + = link_to 'unlink issue', unlink_issue_app_problem_path(app, @problem), :method => :delete, :data => { :confirm => "Unlink err issues?" }, :class => "unlink-issue"
5 - elsif @problem.issue_link == "pending" 5 - elsif @problem.issue_link == "pending"
6 %span.disabled= link_to 'creating...', '#', :class => "#{@problem.issue_type}_inactive create-issue" 6 %span.disabled= link_to 'creating...', '#', :class => "#{@problem.issue_type}_inactive create-issue"
7 - = link_to 'retry', create_issue_app_problem_path(@app, @problem), :method => :post 7 + = link_to 'retry', create_issue_app_problem_path(app, @problem), :method => :post
8 - else 8 - else
9 - - if @app.github_repo? 9 + - if app.github_repo?
10 - if current_user.can_create_github_issues? 10 - if current_user.can_create_github_issues?
11 - %span= link_to 'create issue', create_issue_app_problem_path(@app, @problem, :tracker => 'user_github'), :method => :post, :class => "github_create create-issue"  
12 - - elsif @app.issue_tracker_configured? && @app.issue_tracker.label.eql?('github')  
13 - %span= link_to 'create issue', create_issue_app_problem_path(@app, @problem), :method => :post, :class => "github_create create-issue"  
14 - - if @app.issue_tracker_configured? && !@app.issue_tracker.label.eql?('github')  
15 - %span= link_to 'create issue', create_issue_app_problem_path(@app, @problem), :method => :post, :class => "#{@app.issue_tracker.label}_create create-issue" 11 + %span= link_to 'create issue', create_issue_app_problem_path(app, @problem, :tracker => 'user_github'), :method => :post, :class => "github_create create-issue"
  12 + - elsif app.issue_tracker_configured? && app.issue_tracker.label.eql?('github')
  13 + %span= link_to 'create issue', create_issue_app_problem_path(app, @problem), :method => :post, :class => "github_create create-issue"
  14 + - if app.issue_tracker_configured? && !app.issue_tracker.label.eql?('github')
  15 + %span= link_to 'create issue', create_issue_app_problem_path(app, @problem), :method => :post, :class => "#{app.issue_tracker.label}_create create-issue"
app/views/problems/_list.atom.builder
@@ -3,7 +3,7 @@ feed.updated(@problems.first.try(:created_at) || Time.now) @@ -3,7 +3,7 @@ feed.updated(@problems.first.try(:created_at) || Time.now)
3 for problem in @problems 3 for problem in @problems
4 notice = problem.notices.first 4 notice = problem.notices.first
5 5
6 - feed.entry(problem, :url => app_problem_url(problem.app, problem)) do |entry| 6 + feed.entry(problem, :url => app_problem_url(problem.app.to_param, problem.to_param)) do |entry|
7 entry.title "[#{ problem.where }] #{problem.message.to_s.truncate(27)}" 7 entry.title "[#{ problem.where }] #{problem.message.to_s.truncate(27)}"
8 entry.author do |author| 8 entry.author do |author|
9 author.name "#{ problem.app.name } [#{ problem.environment }]" 9 author.name "#{ problem.app.name } [#{ problem.environment }]"
app/views/problems/show.html.haml
@@ -3,7 +3,7 @@ @@ -3,7 +3,7 @@
3 - content_for :title, @problem.error_class || truncate(@problem.message, :length => 32) 3 - content_for :title, @problem.error_class || truncate(@problem.message, :length => 32)
4 - content_for :meta do 4 - content_for :meta do
5 %strong App: 5 %strong App:
6 - = link_to @app.name, @app 6 + = link_to app.name, app
7 %strong Where: 7 %strong Where:
8 = @problem.where 8 = @problem.where
9 %br 9 %br
@@ -13,10 +13,10 @@ @@ -13,10 +13,10 @@
13 = @problem.last_notice_at.to_s(:precise) 13 = @problem.last_notice_at.to_s(:precise)
14 - content_for :action_bar do 14 - content_for :action_bar do
15 - if @problem.unresolved? 15 - if @problem.unresolved?
16 - %span= link_to 'resolve', [:resolve, @app, @problem], :method => :put, :data => { :confirm => problem_confirm }, :class => 'resolve' 16 + %span= link_to 'resolve', [:resolve, app, @problem], :method => :put, :data => { :confirm => problem_confirm }, :class => 'resolve'
17 - if current_user.authentication_token 17 - if current_user.authentication_token
18 - %span= link_to 'iCal', polymorphic_path([@app, @problem], :format => "ics", :auth_token => current_user.authentication_token), :class => "calendar_link"  
19 - %span>= link_to 'up', (request.env['HTTP_REFERER'] ? :back : app_problems_path(@app)), :class => 'up' 18 + %span= link_to 'iCal', polymorphic_path([app, @problem], :format => "ics", :auth_token => current_user.authentication_token), :class => "calendar_link"
  19 + %span>= link_to 'up', (request.env['HTTP_REFERER'] ? :back : app_problems_path(app)), :class => 'up'
20 %br 20 %br
21 = render "issue_tracker_links" 21 = render "issue_tracker_links"
22 22
@@ -37,11 +37,11 @@ @@ -37,11 +37,11 @@
37 - else 37 - else
38 %span.comment-info 38 %span.comment-info
39 = time_ago_in_words(comment.created_at, true) << " ago by [Unknown User]" 39 = time_ago_in_words(comment.created_at, true) << " ago by [Unknown User]"
40 - %span.delete= link_to '&#10008;'.html_safe, [@app, @problem, comment], :method => :delete, :data => { :confirm => "Are you sure you don't need this comment?" }, :class => "destroy-comment" 40 + %span.delete= link_to '&#10008;'.html_safe, [app, @problem, comment], :method => :delete, :data => { :confirm => "Are you sure you don't need this comment?" }, :class => "destroy-comment"
41 %tr 41 %tr
42 %td= simple_format comment.body 42 %td= simple_format comment.body
43 - if @problem.comments_allowed? 43 - if @problem.comments_allowed?
44 - = form_for [@app, @problem, @comment] do |comment_form| 44 + = form_for [app, @problem, @comment] do |comment_form|
45 %p Add a comment 45 %p Add a comment
46 = comment_form.text_area :body 46 = comment_form.text_area :body
47 = comment_form.submit "Save Comment" 47 = comment_form.submit "Save Comment"
spec/controllers/problems_controller_spec.rb
@@ -12,7 +12,7 @@ describe ProblemsController do @@ -12,7 +12,7 @@ describe ProblemsController do
12 12
13 13
14 describe "GET /problems" do 14 describe "GET /problems" do
15 - render_views 15 + #render_views
16 context 'when logged in as an admin' do 16 context 'when logged in as an admin' do
17 before(:each) do 17 before(:each) do
18 @user = Fabricate(:admin) 18 @user = Fabricate(:admin)
@@ -20,18 +20,6 @@ describe ProblemsController do @@ -20,18 +20,6 @@ describe ProblemsController do
20 @problem = Fabricate(:notice, :err => Fabricate(:err, :problem => Fabricate(:problem, :app => app, :environment => "production"))).problem 20 @problem = Fabricate(:notice, :err => Fabricate(:err, :problem => Fabricate(:problem, :app => app, :environment => "production"))).problem
21 end 21 end
22 22
23 - it "should successfully list problems" do  
24 - get :index  
25 - response.should be_success  
26 - response.body.gsub("&#8203;", "").should match(@problem.message)  
27 - end  
28 -  
29 - it "should list atom feed successfully" do  
30 - get :index, :format => "atom"  
31 - response.should be_success  
32 - response.body.should match(@problem.message)  
33 - end  
34 -  
35 context "pagination" do 23 context "pagination" do
36 before(:each) do 24 before(:each) do
37 35.times { Fabricate :err } 25 35.times { Fabricate :err }
@@ -136,7 +124,7 @@ describe ProblemsController do @@ -136,7 +124,7 @@ describe ProblemsController do
136 end 124 end
137 125
138 describe "GET /apps/:app_id/problems/:id" do 126 describe "GET /apps/:app_id/problems/:id" do
139 - render_views 127 + #render_views
140 128
141 context 'when logged in as an admin' do 129 context 'when logged in as an admin' do
142 before do 130 before do
@@ -145,7 +133,7 @@ describe ProblemsController do @@ -145,7 +133,7 @@ describe ProblemsController do
145 133
146 it "finds the app" do 134 it "finds the app" do
147 get :show, :app_id => app.id, :id => err.problem.id 135 get :show, :app_id => app.id, :id => err.problem.id
148 - assigns(:app).should == app 136 + controller.app.should == app
149 end 137 end
150 138
151 it "finds the problem" do 139 it "finds the problem" do
@@ -178,32 +166,6 @@ describe ProblemsController do @@ -178,32 +166,6 @@ describe ProblemsController do
178 end 166 end
179 end 167 end
180 168
181 - context "create issue button" do  
182 - let(:button_matcher) { match(/create issue/) }  
183 -  
184 - it "should not exist for problem's app without issue tracker" do  
185 - err = Fabricate :err  
186 - get :show, :app_id => err.app.id, :id => err.problem.id  
187 -  
188 - response.body.should_not button_matcher  
189 - end  
190 -  
191 - it "should exist for problem's app with issue tracker" do  
192 - tracker = Fabricate(:lighthouse_tracker)  
193 - err = Fabricate(:err, :problem => Fabricate(:problem, :app => tracker.app))  
194 - get :show, :app_id => err.app.id, :id => err.problem.id  
195 -  
196 - response.body.should button_matcher  
197 - end  
198 -  
199 - it "should not exist for problem with issue_link" do  
200 - tracker = Fabricate(:lighthouse_tracker)  
201 - err = Fabricate(:err, :problem => Fabricate(:problem, :app => tracker.app, :issue_link => "http://some.host"))  
202 - get :show, :app_id => err.app.id, :id => err.problem.id  
203 -  
204 - response.body.should_not button_matcher  
205 - end  
206 - end  
207 end 169 end
208 170
209 context 'when logged in as a user' do 171 context 'when logged in as a user' do
@@ -242,7 +204,7 @@ describe ProblemsController do @@ -242,7 +204,7 @@ describe ProblemsController do
242 App.should_receive(:find).with(@problem.app.id).and_return(@problem.app) 204 App.should_receive(:find).with(@problem.app.id).and_return(@problem.app)
243 @problem.app.problems.should_receive(:find).and_return(@problem.problem) 205 @problem.app.problems.should_receive(:find).and_return(@problem.problem)
244 put :resolve, :app_id => @problem.app.id, :id => @problem.problem.id 206 put :resolve, :app_id => @problem.app.id, :id => @problem.problem.id
245 - assigns(:app).should == @problem.app 207 + controller.app.should == @problem.app
246 assigns(:problem).should == @problem.problem 208 assigns(:problem).should == @problem.problem
247 end 209 end
248 210
@@ -269,7 +231,7 @@ describe ProblemsController do @@ -269,7 +231,7 @@ describe ProblemsController do
269 end 231 end
270 232
271 describe "POST /apps/:app_id/problems/:id/create_issue" do 233 describe "POST /apps/:app_id/problems/:id/create_issue" do
272 - render_views 234 + #render_views
273 235
274 before(:each) do 236 before(:each) do
275 sign_in Fabricate(:admin) 237 sign_in Fabricate(:admin)
spec/views/problems/index.atom.builder_spec.rb 0 → 100644
@@ -0,0 +1,14 @@ @@ -0,0 +1,14 @@
  1 +require 'spec_helper'
  2 +
  3 +describe "problems/index.atom.builder" do
  4 +
  5 + it 'display problem message' do
  6 + app = App.new(:new_record => false)
  7 + assign(:problems, [Problem.new(
  8 + :message => 'foo',
  9 + :new_record => false, :app => app), Problem.new(:new_record => false, :app => app)])
  10 + render
  11 + rendered.should match('foo')
  12 + end
  13 +
  14 +end
spec/views/problems/show.html.haml_spec.rb
@@ -6,7 +6,7 @@ describe &quot;problems/show.html.haml&quot; do @@ -6,7 +6,7 @@ describe &quot;problems/show.html.haml&quot; do
6 comment = Fabricate(:comment) 6 comment = Fabricate(:comment)
7 assign :problem, problem 7 assign :problem, problem
8 assign :comment, comment 8 assign :comment, comment
9 - assign :app, problem.app 9 + view.stub(:app).and_return(problem.app)
10 assign :notices, problem.notices.page(1).per(1) 10 assign :notices, problem.notices.page(1).per(1)
11 assign :notice, problem.notices.first 11 assign :notice, problem.notices.first
12 controller.stub(:current_user) { Fabricate(:user) } 12 controller.stub(:current_user) { Fabricate(:user) }
@@ -15,7 +15,7 @@ describe &quot;problems/show.html.haml&quot; do @@ -15,7 +15,7 @@ describe &quot;problems/show.html.haml&quot; do
15 def with_issue_tracker(tracker, problem) 15 def with_issue_tracker(tracker, problem)
16 problem.app.issue_tracker = tracker.new :api_token => "token token token", :project_id => "1234" 16 problem.app.issue_tracker = tracker.new :api_token => "token token token", :project_id => "1234"
17 assign :problem, problem 17 assign :problem, problem
18 - assign :app, problem.app 18 + view.stub(:app).and_return(problem.app)
19 end 19 end
20 20
21 describe "content_for :action_bar" do 21 describe "content_for :action_bar" do
@@ -55,7 +55,7 @@ describe &quot;problems/show.html.haml&quot; do @@ -55,7 +55,7 @@ describe &quot;problems/show.html.haml&quot; do
55 controller.request.env['HTTP_REFERER'] = nil 55 controller.request.env['HTTP_REFERER'] = nil
56 problem = Fabricate(:problem_with_comments) 56 problem = Fabricate(:problem_with_comments)
57 assign :problem, problem 57 assign :problem, problem
58 - assign :app, problem.app 58 + view.stub(:app).and_return(problem.app)
59 render 59 render
60 60
61 action_bar.should have_selector("span a.up[href='#{app_problems_path(problem.app)}']", :text => 'up') 61 action_bar.should have_selector("span a.up[href='#{app_problems_path(problem.app)}']", :text => 'up')
@@ -68,7 +68,7 @@ describe &quot;problems/show.html.haml&quot; do @@ -68,7 +68,7 @@ describe &quot;problems/show.html.haml&quot; do
68 68
69 problem = Fabricate(:problem_with_comments, :app => Fabricate(:app, :github_repo => "test_user/test_repo")) 69 problem = Fabricate(:problem_with_comments, :app => Fabricate(:app, :github_repo => "test_user/test_repo"))
70 assign :problem, problem 70 assign :problem, problem
71 - assign :app, problem.app 71 + view.stub(:app).and_return(problem.app)
72 render 72 render
73 73
74 action_bar.should have_selector("span a.github_create.create-issue", :text => 'create issue') 74 action_bar.should have_selector("span a.github_create.create-issue", :text => 'create issue')
@@ -78,11 +78,53 @@ describe &quot;problems/show.html.haml&quot; do @@ -78,11 +78,53 @@ describe &quot;problems/show.html.haml&quot; do
78 problem = Fabricate(:problem_with_comments, :app => Fabricate(:app, :github_repo => "test_user/test_repo")) 78 problem = Fabricate(:problem_with_comments, :app => Fabricate(:app, :github_repo => "test_user/test_repo"))
79 with_issue_tracker(GithubIssuesTracker, problem) 79 with_issue_tracker(GithubIssuesTracker, problem)
80 assign :problem, problem 80 assign :problem, problem
81 - assign :app, problem.app 81 + view.stub(:app).and_return(problem.app)
82 render 82 render
83 83
84 action_bar.should have_selector("span a.github_create.create-issue", :text => 'create issue') 84 action_bar.should have_selector("span a.github_create.create-issue", :text => 'create issue')
85 end 85 end
  86 +
  87 + context "without issue tracker associate on app" do
  88 + let(:problem){ Problem.new(:new_record => false, :app => app) }
  89 + let(:app) { App.new(:new_record => false) }
  90 +
  91 + it 'not see link to create issue' do
  92 + assign :problem, problem
  93 + view.stub(:app).and_return(problem.app)
  94 + render
  95 + expect(view.content_for(:action_bar)).to_not match(/create issue/)
  96 + end
  97 +
  98 + end
  99 +
  100 + context "with lighthouse tracker on app" do
  101 + let(:app) { App.new(:new_record => false, :issue_tracker => tracker ) }
  102 + let(:tracker) {
  103 + IssueTrackers::LighthouseTracker.new(:project_id => 'x')
  104 + }
  105 + context "with problem without issue link" do
  106 + let(:problem){ Problem.new(:new_record => false, :app => app) }
  107 + it 'not see link if no issue tracker' do
  108 + assign :problem, problem
  109 + view.stub(:app).and_return(problem.app)
  110 + render
  111 + expect(view.content_for(:action_bar)).to match(/create issue/)
  112 + end
  113 +
  114 + end
  115 +
  116 + context "with problem with issue link" do
  117 + let(:problem){ Problem.new(:new_record => false, :app => app, :issue_link => 'http://foo') }
  118 +
  119 + it 'not see link if no issue tracker' do
  120 + assign :problem, problem
  121 + view.stub(:app).and_return(problem.app)
  122 + render
  123 + expect(view.content_for(:action_bar)).to_not match(/create issue/)
  124 + end
  125 + end
  126 +
  127 + end
86 end 128 end
87 end 129 end
88 130
@@ -95,7 +137,7 @@ describe &quot;problems/show.html.haml&quot; do @@ -95,7 +137,7 @@ describe &quot;problems/show.html.haml&quot; do
95 it 'should display comments and new comment form when no issue tracker' do 137 it 'should display comments and new comment form when no issue tracker' do
96 problem = Fabricate(:problem_with_comments) 138 problem = Fabricate(:problem_with_comments)
97 assign :problem, problem 139 assign :problem, problem
98 - assign :app, problem.app 140 + view.stub(:app).and_return(problem.app)
99 render 141 render
100 142
101 view.content_for(:comments).should include('Test comment') 143 view.content_for(:comments).should include('Test comment')