From d0894cbe2f38472d24a64b400a31136530fef611 Mon Sep 17 00:00:00 2001 From: Cyril Mougel Date: Thu, 27 Jun 2013 10:45:28 +0200 Subject: [PATCH] [WIP] Start refactoring the problems_controller. --- app/controllers/problems_controller.rb | 31 +++++++++++++++++++------------ app/views/problems/_issue_tracker_links.html.haml | 18 +++++++++--------- app/views/problems/_list.atom.builder | 2 +- app/views/problems/show.html.haml | 12 ++++++------ spec/controllers/problems_controller_spec.rb | 48 +++++------------------------------------------- spec/views/problems/index.atom.builder_spec.rb | 14 ++++++++++++++ spec/views/problems/show.html.haml_spec.rb | 54 ++++++++++++++++++++++++++++++++++++++++++++++++------ 7 files changed, 102 insertions(+), 77 deletions(-) create mode 100644 spec/views/problems/index.atom.builder_spec.rb diff --git a/app/controllers/problems_controller.rb b/app/controllers/problems_controller.rb index 12c7a38..fa72a72 100644 --- a/app/controllers/problems_controller.rb +++ b/app/controllers/problems_controller.rb @@ -1,10 +1,24 @@ +## +# Manage problems +# +# List of actions available : +# MEMBER => :show, :edit, :update, :create, :destroy, :resolve, :unresolve, :create_issue, :unlink_issue +# COLLECTION => :index, :all, :destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several, :search class ProblemsController < ApplicationController - before_filter :find_app, :except => [:index, :all, :destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several, :search] + before_filter :find_problem, :except => [:index, :all, :destroy_several, :resolve_several, :unresolve_several, :merge_several, :unmerge_several, :search] 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] + expose(:app) { + if current_user.admin? + App.find(params[:app_id]) + else + current_user.apps.find(params[:app_id]) + end + } + def index app_scope = current_user.admin? ? App.all : current_user.apps @all_errs = params[:all_errs] @@ -31,12 +45,12 @@ class ProblemsController < ApplicationController flash[:error] = issue_creation.errors[:base].first end - redirect_to app_problem_path(@app, @problem) + redirect_to app_problem_path(app, @problem) end def unlink_issue @problem.update_attribute :issue_link, nil - redirect_to app_problem_path(@app, @problem) + redirect_to app_problem_path(app, @problem) end def resolve @@ -44,7 +58,7 @@ class ProblemsController < ApplicationController flash[:success] = 'Great news everyone! The err has been resolved.' redirect_to :back rescue ActionController::RedirectBackError - redirect_to app_path(@app) + redirect_to app_path(app) end def resolve_several @@ -94,16 +108,9 @@ class ProblemsController < ApplicationController end protected - def find_app - @app = App.find(params[:app_id]) - - # Mongoid Bug: could not chain: current_user.apps.find_by_id! - # apparently finding by 'watchers.email' and 'id' is broken - raise(Mongoid::Errors::DocumentNotFound.new(App,@app.id)) unless current_user.admin? || current_user.watching?(@app) - end def find_problem - @problem = @app.problems.find(params[:id]) + @problem = app.problems.find(params[:id]) end def set_tracker_params diff --git a/app/views/problems/_issue_tracker_links.html.haml b/app/views/problems/_issue_tracker_links.html.haml index f6735a9..5c43d2b 100644 --- a/app/views/problems/_issue_tracker_links.html.haml +++ b/app/views/problems/_issue_tracker_links.html.haml @@ -1,15 +1,15 @@ -- if @app.issue_tracker_configured? || current_user.github_account? +- if app.issue_tracker_configured? || current_user.github_account? - if @problem.issue_link.present? %span= link_to 'go to issue', @problem.issue_link, :class => "#{@problem.issue_type}_goto goto-issue" - = link_to 'unlink issue', unlink_issue_app_problem_path(@app, @problem), :method => :delete, :data => { :confirm => "Unlink err issues?" }, :class => "unlink-issue" + = link_to 'unlink issue', unlink_issue_app_problem_path(app, @problem), :method => :delete, :data => { :confirm => "Unlink err issues?" }, :class => "unlink-issue" - elsif @problem.issue_link == "pending" %span.disabled= link_to 'creating...', '#', :class => "#{@problem.issue_type}_inactive create-issue" - = link_to 'retry', create_issue_app_problem_path(@app, @problem), :method => :post + = link_to 'retry', create_issue_app_problem_path(app, @problem), :method => :post - else - - if @app.github_repo? + - if app.github_repo? - if current_user.can_create_github_issues? - %span= link_to 'create issue', create_issue_app_problem_path(@app, @problem, :tracker => 'user_github'), :method => :post, :class => "github_create create-issue" - - elsif @app.issue_tracker_configured? && @app.issue_tracker.label.eql?('github') - %span= link_to 'create issue', create_issue_app_problem_path(@app, @problem), :method => :post, :class => "github_create create-issue" - - if @app.issue_tracker_configured? && !@app.issue_tracker.label.eql?('github') - %span= link_to 'create issue', create_issue_app_problem_path(@app, @problem), :method => :post, :class => "#{@app.issue_tracker.label}_create create-issue" + %span= link_to 'create issue', create_issue_app_problem_path(app, @problem, :tracker => 'user_github'), :method => :post, :class => "github_create create-issue" + - elsif app.issue_tracker_configured? && app.issue_tracker.label.eql?('github') + %span= link_to 'create issue', create_issue_app_problem_path(app, @problem), :method => :post, :class => "github_create create-issue" + - if app.issue_tracker_configured? && !app.issue_tracker.label.eql?('github') + %span= link_to 'create issue', create_issue_app_problem_path(app, @problem), :method => :post, :class => "#{app.issue_tracker.label}_create create-issue" diff --git a/app/views/problems/_list.atom.builder b/app/views/problems/_list.atom.builder index 4201144..e39ca02 100644 --- a/app/views/problems/_list.atom.builder +++ b/app/views/problems/_list.atom.builder @@ -3,7 +3,7 @@ feed.updated(@problems.first.try(:created_at) || Time.now) for problem in @problems notice = problem.notices.first - feed.entry(problem, :url => app_problem_url(problem.app, problem)) do |entry| + feed.entry(problem, :url => app_problem_url(problem.app.to_param, problem.to_param)) do |entry| entry.title "[#{ problem.where }] #{problem.message.to_s.truncate(27)}" entry.author do |author| author.name "#{ problem.app.name } [#{ problem.environment }]" diff --git a/app/views/problems/show.html.haml b/app/views/problems/show.html.haml index cd793e5..4b0c74d 100644 --- a/app/views/problems/show.html.haml +++ b/app/views/problems/show.html.haml @@ -3,7 +3,7 @@ - content_for :title, @problem.error_class || truncate(@problem.message, :length => 32) - content_for :meta do %strong App: - = link_to @app.name, @app + = link_to app.name, app %strong Where: = @problem.where %br @@ -13,10 +13,10 @@ = @problem.last_notice_at.to_s(:precise) - content_for :action_bar do - if @problem.unresolved? - %span= link_to 'resolve', [:resolve, @app, @problem], :method => :put, :data => { :confirm => problem_confirm }, :class => 'resolve' + %span= link_to 'resolve', [:resolve, app, @problem], :method => :put, :data => { :confirm => problem_confirm }, :class => 'resolve' - if current_user.authentication_token - %span= link_to 'iCal', polymorphic_path([@app, @problem], :format => "ics", :auth_token => current_user.authentication_token), :class => "calendar_link" - %span>= link_to 'up', (request.env['HTTP_REFERER'] ? :back : app_problems_path(@app)), :class => 'up' + %span= link_to 'iCal', polymorphic_path([app, @problem], :format => "ics", :auth_token => current_user.authentication_token), :class => "calendar_link" + %span>= link_to 'up', (request.env['HTTP_REFERER'] ? :back : app_problems_path(app)), :class => 'up' %br = render "issue_tracker_links" @@ -37,11 +37,11 @@ - else %span.comment-info = time_ago_in_words(comment.created_at, true) << " ago by [Unknown User]" - %span.delete= link_to '✘'.html_safe, [@app, @problem, comment], :method => :delete, :data => { :confirm => "Are you sure you don't need this comment?" }, :class => "destroy-comment" + %span.delete= link_to '✘'.html_safe, [app, @problem, comment], :method => :delete, :data => { :confirm => "Are you sure you don't need this comment?" }, :class => "destroy-comment" %tr %td= simple_format comment.body - if @problem.comments_allowed? - = form_for [@app, @problem, @comment] do |comment_form| + = form_for [app, @problem, @comment] do |comment_form| %p Add a comment = comment_form.text_area :body = comment_form.submit "Save Comment" diff --git a/spec/controllers/problems_controller_spec.rb b/spec/controllers/problems_controller_spec.rb index 741d193..6703a1c 100644 --- a/spec/controllers/problems_controller_spec.rb +++ b/spec/controllers/problems_controller_spec.rb @@ -12,7 +12,7 @@ describe ProblemsController do describe "GET /problems" do - render_views + #render_views context 'when logged in as an admin' do before(:each) do @user = Fabricate(:admin) @@ -20,18 +20,6 @@ describe ProblemsController do @problem = Fabricate(:notice, :err => Fabricate(:err, :problem => Fabricate(:problem, :app => app, :environment => "production"))).problem end - it "should successfully list problems" do - get :index - response.should be_success - response.body.gsub("​", "").should match(@problem.message) - end - - it "should list atom feed successfully" do - get :index, :format => "atom" - response.should be_success - response.body.should match(@problem.message) - end - context "pagination" do before(:each) do 35.times { Fabricate :err } @@ -136,7 +124,7 @@ describe ProblemsController do end describe "GET /apps/:app_id/problems/:id" do - render_views + #render_views context 'when logged in as an admin' do before do @@ -145,7 +133,7 @@ describe ProblemsController do it "finds the app" do get :show, :app_id => app.id, :id => err.problem.id - assigns(:app).should == app + controller.app.should == app end it "finds the problem" do @@ -178,32 +166,6 @@ describe ProblemsController do end end - context "create issue button" do - let(:button_matcher) { match(/create issue/) } - - it "should not exist for problem's app without issue tracker" do - err = Fabricate :err - get :show, :app_id => err.app.id, :id => err.problem.id - - response.body.should_not button_matcher - end - - it "should exist for problem's app with issue tracker" do - tracker = Fabricate(:lighthouse_tracker) - err = Fabricate(:err, :problem => Fabricate(:problem, :app => tracker.app)) - get :show, :app_id => err.app.id, :id => err.problem.id - - response.body.should button_matcher - end - - it "should not exist for problem with issue_link" do - tracker = Fabricate(:lighthouse_tracker) - err = Fabricate(:err, :problem => Fabricate(:problem, :app => tracker.app, :issue_link => "http://some.host")) - get :show, :app_id => err.app.id, :id => err.problem.id - - response.body.should_not button_matcher - end - end end context 'when logged in as a user' do @@ -242,7 +204,7 @@ describe ProblemsController do App.should_receive(:find).with(@problem.app.id).and_return(@problem.app) @problem.app.problems.should_receive(:find).and_return(@problem.problem) put :resolve, :app_id => @problem.app.id, :id => @problem.problem.id - assigns(:app).should == @problem.app + controller.app.should == @problem.app assigns(:problem).should == @problem.problem end @@ -269,7 +231,7 @@ describe ProblemsController do end describe "POST /apps/:app_id/problems/:id/create_issue" do - render_views + #render_views before(:each) do sign_in Fabricate(:admin) diff --git a/spec/views/problems/index.atom.builder_spec.rb b/spec/views/problems/index.atom.builder_spec.rb new file mode 100644 index 0000000..ea5742a --- /dev/null +++ b/spec/views/problems/index.atom.builder_spec.rb @@ -0,0 +1,14 @@ +require 'spec_helper' + +describe "problems/index.atom.builder" do + + it 'display problem message' do + app = App.new(:new_record => false) + assign(:problems, [Problem.new( + :message => 'foo', + :new_record => false, :app => app), Problem.new(:new_record => false, :app => app)]) + render + rendered.should match('foo') + end + +end diff --git a/spec/views/problems/show.html.haml_spec.rb b/spec/views/problems/show.html.haml_spec.rb index 6a1e305..6d24981 100644 --- a/spec/views/problems/show.html.haml_spec.rb +++ b/spec/views/problems/show.html.haml_spec.rb @@ -6,7 +6,7 @@ describe "problems/show.html.haml" do comment = Fabricate(:comment) assign :problem, problem assign :comment, comment - assign :app, problem.app + view.stub(:app).and_return(problem.app) assign :notices, problem.notices.page(1).per(1) assign :notice, problem.notices.first controller.stub(:current_user) { Fabricate(:user) } @@ -15,7 +15,7 @@ describe "problems/show.html.haml" do def with_issue_tracker(tracker, problem) problem.app.issue_tracker = tracker.new :api_token => "token token token", :project_id => "1234" assign :problem, problem - assign :app, problem.app + view.stub(:app).and_return(problem.app) end describe "content_for :action_bar" do @@ -55,7 +55,7 @@ describe "problems/show.html.haml" do controller.request.env['HTTP_REFERER'] = nil problem = Fabricate(:problem_with_comments) assign :problem, problem - assign :app, problem.app + view.stub(:app).and_return(problem.app) render action_bar.should have_selector("span a.up[href='#{app_problems_path(problem.app)}']", :text => 'up') @@ -68,7 +68,7 @@ describe "problems/show.html.haml" do problem = Fabricate(:problem_with_comments, :app => Fabricate(:app, :github_repo => "test_user/test_repo")) assign :problem, problem - assign :app, problem.app + view.stub(:app).and_return(problem.app) render action_bar.should have_selector("span a.github_create.create-issue", :text => 'create issue') @@ -78,11 +78,53 @@ describe "problems/show.html.haml" do problem = Fabricate(:problem_with_comments, :app => Fabricate(:app, :github_repo => "test_user/test_repo")) with_issue_tracker(GithubIssuesTracker, problem) assign :problem, problem - assign :app, problem.app + view.stub(:app).and_return(problem.app) render action_bar.should have_selector("span a.github_create.create-issue", :text => 'create issue') end + + context "without issue tracker associate on app" do + let(:problem){ Problem.new(:new_record => false, :app => app) } + let(:app) { App.new(:new_record => false) } + + it 'not see link to create issue' do + assign :problem, problem + view.stub(:app).and_return(problem.app) + render + expect(view.content_for(:action_bar)).to_not match(/create issue/) + end + + end + + context "with lighthouse tracker on app" do + let(:app) { App.new(:new_record => false, :issue_tracker => tracker ) } + let(:tracker) { + IssueTrackers::LighthouseTracker.new(:project_id => 'x') + } + context "with problem without issue link" do + let(:problem){ Problem.new(:new_record => false, :app => app) } + it 'not see link if no issue tracker' do + assign :problem, problem + view.stub(:app).and_return(problem.app) + render + expect(view.content_for(:action_bar)).to match(/create issue/) + end + + end + + context "with problem with issue link" do + let(:problem){ Problem.new(:new_record => false, :app => app, :issue_link => 'http://foo') } + + it 'not see link if no issue tracker' do + assign :problem, problem + view.stub(:app).and_return(problem.app) + render + expect(view.content_for(:action_bar)).to_not match(/create issue/) + end + end + + end end end @@ -95,7 +137,7 @@ describe "problems/show.html.haml" do it 'should display comments and new comment form when no issue tracker' do problem = Fabricate(:problem_with_comments) assign :problem, problem - assign :app, problem.app + view.stub(:app).and_return(problem.app) render view.content_for(:comments).should include('Test comment') -- libgit2 0.21.2