From c7507da105837bb5177d9fd54ddae5e43b19b600 Mon Sep 17 00:00:00 2001 From: Cyril Mougel Date: Wed, 17 Jul 2013 17:53:51 +0200 Subject: [PATCH] Refactoring of AppsController --- Gemfile | 1 - Gemfile.lock | 8 +------- app/controllers/apps_controller.rb | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------- app/controllers/problems_searcher.rb | 1 + app/helpers/apps_helper.rb | 2 +- app/views/apps/_fields.html.haml | 2 +- app/views/apps/edit.html.haml | 6 +++--- app/views/apps/index.html.haml | 4 ++-- app/views/apps/new.html.haml | 2 +- app/views/apps/show.atom.builder | 2 +- app/views/apps/show.html.haml | 42 +++++++++++++++++++++--------------------- app/views/problems/_list.atom.builder | 4 ++-- config/initializers/inherited_resources.rb | 2 -- config/locales/en.yml | 14 +++++++++++++- spec/controllers/apps_controller_spec.rb | 49 +++++++++++++++++++++++-------------------------- spec/views/apps/edit.html.haml_spec.rb | 2 +- spec/views/apps/index.html.haml_spec.rb | 2 +- spec/views/problems/index.atom.builder_spec.rb | 2 +- 18 files changed, 131 insertions(+), 111 deletions(-) delete mode 100644 config/initializers/inherited_resources.rb diff --git a/Gemfile b/Gemfile index 43ed208..4675ebd 100644 --- a/Gemfile +++ b/Gemfile @@ -11,7 +11,6 @@ gem 'htmlentities' gem 'rack-ssl', :require => 'rack/ssl' # force SSL gem 'useragent' -gem 'inherited_resources' gem 'decent_exposure' gem 'strong_parameters' gem 'SystemTimer', :platform => :ruby_18 diff --git a/Gemfile.lock b/Gemfile.lock index 0e88608..887e0ba 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -123,6 +123,7 @@ GEM multipart-post (~> 1.1) faraday_middleware (0.8.8) faraday (>= 0.7.4, < 0.9) + ffi (1.9.0) flowdock (0.3.1) httparty (~> 0.7) multi_json @@ -133,7 +134,6 @@ GEM tilt happymapper (0.4.0) libxml-ruby (~> 2.0) - has_scope (0.5.1) hashie (1.2.0) highline (1.6.19) hike (1.2.3) @@ -151,9 +151,6 @@ GEM multi_xml (>= 0.5.2) httpauth (0.2.0) i18n (0.6.1) - inherited_resources (1.4.0) - has_scope (~> 0.5.0) - responders (~> 0.9) journey (1.0.4) jquery-rails (2.1.4) railties (>= 3.0, < 5.0) @@ -288,8 +285,6 @@ GEM rdoc (3.12.2) json (~> 1.4) ref (1.0.5) - responders (0.9.3) - railties (~> 3.1) rest-client (1.6.7) mime-types (>= 1.16) ri_cal (0.8.8) @@ -412,7 +407,6 @@ DEPENDENCIES hoptoad_notifier (~> 2.4) htmlentities httparty - inherited_resources jquery-rails (~> 2.1.4) kaminari (>= 0.14.1) launchy diff --git a/app/controllers/apps_controller.rb b/app/controllers/apps_controller.rb index b68b4a9..7aaaa0a 100644 --- a/app/controllers/apps_controller.rb +++ b/app/controllers/apps_controller.rb @@ -1,4 +1,4 @@ -class AppsController < InheritedResources::Base +class AppsController < ApplicationController include ProblemsSearcher @@ -7,57 +7,86 @@ class AppsController < InheritedResources::Base before_filter :parse_notice_at_notices_or_set_default, :only => [:create, :update] respond_to :html - def show - respond_to do |format| - format.html do - @all_errs = !!params[:all_errs] + expose(:app_scope) { + (current_user.admin? ? App : current_user.apps) + } + + expose(:apps) { + app_scope.all.sort + } + + expose(:app, :ancestor => :app_scope) + + expose(:all_errs) { + !!params[:all_errs] + } + expose(:problems) { + if request.format == :atom + app.problems.unresolved.ordered + else + pr = app.problems + pr = pr.unresolved unless all_errs + pr.in_env( + params[:environment] + ).ordered_by(params_sort, params_order).page(params[:page]).per(current_user.per_page) + end + } - @problems = resource.problems - @problems = @problems.unresolved unless @all_errs - @problems = @problems.in_env(params[:environment]).ordered_by(params_sort, params_order).page(params[:page]).per(current_user.per_page) + expose(:deploys) { + app.deploys.order_by(:created_at.desc).limit(5) + } - @deploys = @app.deploys.order_by(:created_at.desc).limit(5) - end - format.atom do - @problems = resource.problems.unresolved.ordered - end - end + def index; end + def show + app + end + + def new + plug_params(app) end def create - @app = App.new(params[:app]) initialize_subclassed_issue_tracker initialize_subclassed_notification_service - create! + if app.save + redirect_to app_url(app), :flash => { :success => I18n.t('controllers.apps.flash.create.success') } + else + flash[:error] = I18n.t('controllers.apps.flash.create.error') + render :new + end end def update - @app = resource initialize_subclassed_issue_tracker initialize_subclassed_notification_service - update! + if app.save + redirect_to app_url(app), :flash => { :success => I18n.t('controllers.apps.flash.update.success') } + else + flash[:error] = I18n.t('controllers.apps.flash.update.error') + render :edit + end end - def new - plug_params(build_resource) - new! + def edit + plug_params(app) end - def edit - plug_params(resource) - edit! + def destroy + if app.destroy + redirect_to apps_url, :flash => { :success => I18n.t('controllers.apps.flash.destroy.success') } + else + flash[:error] = I18n.t('controllers.apps.flash.destroy.error') + render :show + end end protected - def collection - @apps ||= end_of_association_chain.all.sort - end def initialize_subclassed_issue_tracker # set the app's issue tracker if params[:app][:issue_tracker_attributes] && tracker_type = params[:app][:issue_tracker_attributes][:type] if IssueTracker.subclasses.map(&:name).concat(["IssueTracker"]).include?(tracker_type) - @app.issue_tracker = tracker_type.constantize.new(params[:app][:issue_tracker_attributes]) + app.issue_tracker = tracker_type.constantize.new(params[:app][:issue_tracker_attributes]) end end end @@ -66,21 +95,11 @@ class AppsController < InheritedResources::Base # set the app's notification service if params[:app][:notification_service_attributes] && notification_type = params[:app][:notification_service_attributes][:type] if NotificationService.subclasses.map(&:name).concat(["NotificationService"]).include?(notification_type) - @app.notification_service = notification_type.constantize.new(params[:app][:notification_service_attributes]) + app.notification_service = notification_type.constantize.new(params[:app][:notification_service_attributes]) end end end - def begin_of_association_chain - # Filter the @apps collection to apps watched by the current user, unless user is an admin. - # If user is an admin, then no filter is applied, and all apps are shown. - current_user unless current_user.admin? - end - - def interpolation_options - {:app_name => resource.name} - end - def plug_params app app.watchers.build if app.watchers.none? app.issue_tracker = IssueTracker.new unless app.issue_tracker_configured? diff --git a/app/controllers/problems_searcher.rb b/app/controllers/problems_searcher.rb index 6f4ca0c..040d043 100644 --- a/app/controllers/problems_searcher.rb +++ b/app/controllers/problems_searcher.rb @@ -29,5 +29,6 @@ module ProblemsSearcher expose(:err_ids) { (params[:problems] || []).compact } + end end diff --git a/app/helpers/apps_helper.rb b/app/helpers/apps_helper.rb index fef8413..cae6c33 100644 --- a/app/helpers/apps_helper.rb +++ b/app/helpers/apps_helper.rb @@ -41,7 +41,7 @@ module AppsHelper def detect_any_apps_with_attributes @any_github_repos = @any_issue_trackers = @any_deploys = @any_bitbucket_repos = @any_notification_services = false - @apps.each do |app| + apps.each do |app| @any_github_repos ||= app.github_repo? @any_bitbucket_repos ||= app.bitbucket_repo? @any_issue_trackers ||= app.issue_tracker_configured? diff --git a/app/views/apps/_fields.html.haml b/app/views/apps/_fields.html.haml index 5bb0dbc..bacf103 100644 --- a/app/views/apps/_fields.html.haml +++ b/app/views/apps/_fields.html.haml @@ -1,4 +1,4 @@ -= errors_for @app += errors_for app %div.required = f.label :name diff --git a/app/views/apps/edit.html.haml b/app/views/apps/edit.html.haml index 92faea3..df224b0 100644 --- a/app/views/apps/edit.html.haml +++ b/app/views/apps/edit.html.haml @@ -1,10 +1,10 @@ - content_for :title, 'Edit App' - content_for :action_bar do = link_to_copy_attributes_from_other_app - = link_to 'destroy application', app_path(@app), :method => :delete, :data => { :confirm => 'Seriously?' }, :class => 'button' - = link_to('cancel', app_path(@app), :class => 'button') + = link_to 'destroy application', app_path(app), :method => :delete, :data => { :confirm => 'Seriously?' }, :class => 'button' + = link_to('cancel', app_path(app), :class => 'button') -= form_for @app do |f| += form_for app do |f| = render 'fields', :f => f diff --git a/app/views/apps/index.html.haml b/app/views/apps/index.html.haml index 29efcda..9f2db27 100644 --- a/app/views/apps/index.html.haml +++ b/app/views/apps/index.html.haml @@ -16,7 +16,7 @@ %th Last Deploy %th Errors %tbody - - @apps.each do |app| + - apps.each do |app| %tr %td.name= link_to app.name, app_path(app) - if any_github_repos? or any_bitbucket_repos? @@ -50,7 +50,7 @@ - if app.problem_count > 0 - unresolved = app.unresolved_count = link_to unresolved, app_path(app), :class => (unresolved == 0 ? "resolved" : nil) - - if @apps.none? + - if apps.none? %tr %td{:colspan => 3} %em diff --git a/app/views/apps/new.html.haml b/app/views/apps/new.html.haml index 994b7a2..9a1f1d3 100644 --- a/app/views/apps/new.html.haml +++ b/app/views/apps/new.html.haml @@ -3,7 +3,7 @@ = link_to_copy_attributes_from_other_app = link_to('cancel', apps_path, :class => 'button') -= form_for @app do |f| += form_for app do |f| = render 'fields', :f => f diff --git a/app/views/apps/show.atom.builder b/app/views/apps/show.atom.builder index 84d3120..439480e 100644 --- a/app/views/apps/show.atom.builder +++ b/app/views/apps/show.atom.builder @@ -1,4 +1,4 @@ atom_feed do |feed| - feed.title("Errbit notices for #{h @app.name} at #{root_url}") + feed.title("Errbit notices for #{h app.name} at #{root_url}") render "problems/list", :feed => feed end diff --git a/app/views/apps/show.html.haml b/app/views/apps/show.html.haml index 65c42af..ccd26d8 100644 --- a/app/views/apps/show.html.haml +++ b/app/views/apps/show.html.haml @@ -1,26 +1,26 @@ -- content_for :title, @app.name +- content_for :title, app.name - content_for :head do - = auto_discovery_link_tag :atom, app_path(@app, User.token_authentication_key => current_user.authentication_token, :format => "atom"), :title => "Errbit notices for #{@app.name} at #{request.host}" + = auto_discovery_link_tag :atom, app_path(app, User.token_authentication_key => current_user.authentication_token, :format => "atom"), :title => "Errbit notices for #{app.name} at #{request.host}" - content_for :meta do %strong Errors Caught: - = @app.problems.count + = app.problems.count %strong Deploy Count: - = @app.deploys.count + = app.deploys.count %strong API Key: - = @app.api_key + = app.api_key - content_for :action_bar do - if current_user.admin? - = link_to 'edit', edit_app_path(@app), :class => 'button' - - if @all_errs - = link_to 'unresolved errs', app_path(@app), :class => 'button' + = link_to 'edit', edit_app_path(app), :class => 'button' + - if all_errs + = link_to 'unresolved errs', app_path(app), :class => 'button' - else - = link_to 'all errs', app_path(@app, :all_errs => true), :class => 'button' + = link_to 'all errs', app_path(app, :all_errs => true), :class => 'button' %h3#watchers_toggle Watchers %span.click_span (show/hide) #watchers_div - - if @app.notify_all_users + - if app.notify_all_users %table.watchers %thead %tr @@ -31,15 +31,15 @@ %tr %th User or Email %tbody - - @app.watchers.each do |watcher| + - app.watchers.each do |watcher| %tr %td= watcher.label - - if @app.watchers.none? + - if app.watchers.none? %tr %td %em Sadly, no one is watching this app -- if @app.github_repo? +- if app.github_repo? %h3#repository_toggle Repository %span.click_span (show/hide) @@ -50,13 +50,13 @@ %th GitHub Repo %tbody %tr - %td= link_to(@app.github_repo, @app.github_url, :target => '_blank') + %td= link_to(app.github_repo, app.github_url, :target => '_blank') %h3#deploys_toggle Latest Deploys %span.click_span (show/hide) #deploys_div - - if @deploys.any? + - if deploys.any? %table.deploys %thead %tr @@ -68,7 +68,7 @@ %th Revision %tbody - - @deploys.each do |deploy| + - deploys.each do |deploy| %tr %td.when #{deploy.created_at.to_s(:micro)} %td.environment #{deploy.environment} @@ -76,20 +76,20 @@ %td.message #{deploy.message} %td.repository #{deploy.repository} %td.revision #{deploy.short_revision} - = link_to "All Deploys (#{@app.deploys.count})", app_deploys_path(@app), :class => 'button' + = link_to "All Deploys (#{app.deploys.count})", app_deploys_path(app), :class => 'button' - else %h3 No deploys -- if @app.problems.any? +- if app.problems.any? %h3.clear Errors %section - = form_tag search_problems_path(:all_errs => @all_errs, :app_id => @app.id), :method => :get, :remote => true do + = form_tag search_problems_path(:all_errs => all_errs, :app_id => app.id), :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 + = render 'problems/table', :problems => problems - else %h3.clear No errs have been caught yet, make sure you setup your app - = render 'configuration_instructions', :app => @app + = render 'configuration_instructions', :app => app diff --git a/app/views/problems/_list.atom.builder b/app/views/problems/_list.atom.builder index e39ca02..dc49b44 100644 --- a/app/views/problems/_list.atom.builder +++ b/app/views/problems/_list.atom.builder @@ -1,6 +1,6 @@ -feed.updated(@problems.first.try(:created_at) || Time.now) +feed.updated(problems.first.try(:created_at) || Time.now) -for problem in @problems +for problem in problems notice = problem.notices.first feed.entry(problem, :url => app_problem_url(problem.app.to_param, problem.to_param)) do |entry| diff --git a/config/initializers/inherited_resources.rb b/config/initializers/inherited_resources.rb deleted file mode 100644 index 60420c5..0000000 --- a/config/initializers/inherited_resources.rb +++ /dev/null @@ -1,2 +0,0 @@ -InheritedResources.flash_keys = [:success, :error] - diff --git a/config/locales/en.yml b/config/locales/en.yml index e0174b4..909b5bc 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2,7 +2,6 @@ # See http://github.com/svenfuchs/rails-i18n/tree/master/rails%2Flocale for starting points. en: - flash: apps: create: @@ -32,6 +31,18 @@ en: edit_profile: 'Edit profile' controllers: + apps: + flash: + create: + success: "Your app was successfully created." + error: "You app was successfully destroyed." + update: + success: "You app was successfully updated." + error: "You app was not updated" + destroy: + success: "You app was successfully destroyed." + error: "You app could not be destroyed." + users: flash: destroy: @@ -46,6 +57,7 @@ en: merge_several: success: "%{nb} errors have been merged." + devise: registrations: signed_up_but_unconfirmed: 'A message with a confirmation link has been sent to your email address. Please open the link to activate your account.' diff --git a/spec/controllers/apps_controller_spec.rb b/spec/controllers/apps_controller_spec.rb index 27308a8..8d50ae8 100644 --- a/spec/controllers/apps_controller_spec.rb +++ b/spec/controllers/apps_controller_spec.rb @@ -6,15 +6,13 @@ describe AppsController do it_requires_authentication it_requires_admin_privileges :for => {:new => :get, :edit => :get, :create => :post, :update => :put, :destroy => :delete} - describe "GET /apps" do context 'when logged in as an admin' do it 'finds all apps' do sign_in Fabricate(:admin) 3.times { Fabricate(:app) } - apps = App.all get :index - assigns(:apps).should == apps + controller.apps.should == App.all.sort.entries end end @@ -27,8 +25,8 @@ describe AppsController do Fabricate(:user_watcher, :user => user, :app => watched_app1) Fabricate(:user_watcher, :user => user, :app => watched_app2) get :index - assigns(:apps).should include(watched_app1, watched_app2) - assigns(:apps).should_not include(unwatched_app) + controller.apps.should include(watched_app1, watched_app2) + controller.apps.should_not include(unwatched_app) end end end @@ -44,7 +42,7 @@ describe AppsController do it 'finds the app' do get :show, :id => @app.id - assigns(:app).should == @app + controller.app.should == @app end it "should not raise errors for app with err without notices" do @@ -65,13 +63,13 @@ describe AppsController do it "should have default per_page value for user" do get :show, :id => @app.id - 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 :show, :id => @app.id - assigns(:problems).to_a.size.should == 10 + controller.problems.to_a.size.should == 10 end end @@ -85,14 +83,14 @@ describe AppsController do context 'and no params' do it 'shows only unresolved problems' do get :show, :id => @app.id - assigns(:problems).size.should == 1 + controller.problems.size.should == 1 end end context 'and all_problems=true params' do it 'shows all errors' do get :show, :id => @app.id, :all_errs => true - assigns(:problems).size.should == 2 + controller.problems.size.should == 2 end end end @@ -108,35 +106,35 @@ describe AppsController do context 'no params' do it 'shows errs for all environments' do get :show, :id => @app.id - assigns(:problems).size.should == 21 + controller.problems.size.should == 21 end end context 'environment production' do it 'shows errs for just production' do get :show, :id => @app.id, :environment => 'production' - assigns(:problems).size.should == 6 + controller.problems.size.should == 6 end end context 'environment staging' do it 'shows errs for just staging' do get :show, :id => @app.id, :environment => 'staging' - assigns(:problems).size.should == 5 + controller.problems.size.should == 5 end end context 'environment development' do it 'shows errs for just development' do get :show, :id => @app.id, :environment => 'development' - assigns(:problems).size.should == 5 + controller.problems.size.should == 5 end end context 'environment test' do it 'shows errs for just test' do get :show, :id => @app.id, :environment => 'test' - assigns(:problems).size.should == 5 + controller.problems.size.should == 5 end end end @@ -149,7 +147,7 @@ describe AppsController do watcher = Fabricate(:user_watcher, :app => app, :user => user) sign_in user get :show, :id => app.id - assigns(:app).should == app + controller.app.should == app end it 'does not find the app if the user is not watching it' do @@ -170,19 +168,19 @@ describe AppsController do describe "GET /apps/new" do it 'instantiates a new app with a prebuilt watcher' do get :new - assigns(:app).should be_a(App) - assigns(:app).should be_new_record - assigns(:app).watchers.should_not be_empty + controller.app.should be_a(App) + controller.app.should be_new_record + controller.app.watchers.should_not be_empty end it "should copy attributes from an existing app" do @app = Fabricate(:app, :name => "do not copy", :github_repo => "test/example") get :new, :copy_attributes_from => @app.id - assigns(:app).should be_a(App) - assigns(:app).should be_new_record - assigns(:app).name.should be_blank - assigns(:app).github_repo.should == "test/example" + controller.app.should be_a(App) + controller.app.should be_new_record + controller.app.name.should be_blank + controller.app.github_repo.should == "test/example" end end @@ -190,7 +188,7 @@ describe AppsController do it 'finds the correct app' do app = Fabricate(:app) get :edit, :id => app.id - assigns(:app).should == app + controller.app.should == app end end @@ -326,12 +324,11 @@ describe AppsController do describe "DELETE /apps/:id" do before do @app = Fabricate(:app) - App.stub(:find).with(@app.id).and_return(@app) end it "should find the app" do delete :destroy, :id => @app.id - assigns(:app).should == @app + controller.app.should == @app end it "should destroy the app" do diff --git a/spec/views/apps/edit.html.haml_spec.rb b/spec/views/apps/edit.html.haml_spec.rb index b5e4d80..f0ad618 100644 --- a/spec/views/apps/edit.html.haml_spec.rb +++ b/spec/views/apps/edit.html.haml_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe "apps/edit.html.haml" do before do app = stub_model(App) - assign :app, app + view.stub(:app).and_return(app) controller.stub(:current_user) { stub_model(User) } end diff --git a/spec/views/apps/index.html.haml_spec.rb b/spec/views/apps/index.html.haml_spec.rb index e5c595d..cf6ac0c 100644 --- a/spec/views/apps/index.html.haml_spec.rb +++ b/spec/views/apps/index.html.haml_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe "apps/index.html.haml" do before do app = stub_model(App, :deploys => [stub_model(Deploy, :created_at => Time.now, :revision => "123456789abcdef")]) - assign :apps, [app] + view.stub(:apps).and_return([app]) controller.stub(:current_user) { stub_model(User) } end diff --git a/spec/views/problems/index.atom.builder_spec.rb b/spec/views/problems/index.atom.builder_spec.rb index ea5742a..8483c7e 100644 --- a/spec/views/problems/index.atom.builder_spec.rb +++ b/spec/views/problems/index.atom.builder_spec.rb @@ -4,7 +4,7 @@ describe "problems/index.atom.builder" do it 'display problem message' do app = App.new(:new_record => false) - assign(:problems, [Problem.new( + view.stub(:problems).and_return([Problem.new( :message => 'foo', :new_record => false, :app => app), Problem.new(:new_record => false, :app => app)]) render -- libgit2 0.21.2