Commit c7507da105837bb5177d9fd54ddae5e43b19b600

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

Refactoring of AppsController

 * Stop using inherited_resources and only decent_exposure
 * Use decent_exposure when it's possible
 * Extract some translation
Gemfile
... ... @@ -11,7 +11,6 @@ gem 'htmlentities'
11 11 gem 'rack-ssl', :require => 'rack/ssl' # force SSL
12 12  
13 13 gem 'useragent'
14   -gem 'inherited_resources'
15 14 gem 'decent_exposure'
16 15 gem 'strong_parameters'
17 16 gem 'SystemTimer', :platform => :ruby_18
... ...
Gemfile.lock
... ... @@ -123,6 +123,7 @@ GEM
123 123 multipart-post (~> 1.1)
124 124 faraday_middleware (0.8.8)
125 125 faraday (>= 0.7.4, < 0.9)
  126 + ffi (1.9.0)
126 127 flowdock (0.3.1)
127 128 httparty (~> 0.7)
128 129 multi_json
... ... @@ -133,7 +134,6 @@ GEM
133 134 tilt
134 135 happymapper (0.4.0)
135 136 libxml-ruby (~> 2.0)
136   - has_scope (0.5.1)
137 137 hashie (1.2.0)
138 138 highline (1.6.19)
139 139 hike (1.2.3)
... ... @@ -151,9 +151,6 @@ GEM
151 151 multi_xml (>= 0.5.2)
152 152 httpauth (0.2.0)
153 153 i18n (0.6.1)
154   - inherited_resources (1.4.0)
155   - has_scope (~> 0.5.0)
156   - responders (~> 0.9)
157 154 journey (1.0.4)
158 155 jquery-rails (2.1.4)
159 156 railties (>= 3.0, < 5.0)
... ... @@ -288,8 +285,6 @@ GEM
288 285 rdoc (3.12.2)
289 286 json (~> 1.4)
290 287 ref (1.0.5)
291   - responders (0.9.3)
292   - railties (~> 3.1)
293 288 rest-client (1.6.7)
294 289 mime-types (>= 1.16)
295 290 ri_cal (0.8.8)
... ... @@ -412,7 +407,6 @@ DEPENDENCIES
412 407 hoptoad_notifier (~> 2.4)
413 408 htmlentities
414 409 httparty
415   - inherited_resources
416 410 jquery-rails (~> 2.1.4)
417 411 kaminari (>= 0.14.1)
418 412 launchy
... ...
app/controllers/apps_controller.rb
1   -class AppsController < InheritedResources::Base
  1 +class AppsController < ApplicationController
2 2  
3 3 include ProblemsSearcher
4 4  
... ... @@ -7,57 +7,86 @@ class AppsController &lt; InheritedResources::Base
7 7 before_filter :parse_notice_at_notices_or_set_default, :only => [:create, :update]
8 8 respond_to :html
9 9  
10   - def show
11   - respond_to do |format|
12   - format.html do
13   - @all_errs = !!params[:all_errs]
  10 + expose(:app_scope) {
  11 + (current_user.admin? ? App : current_user.apps)
  12 + }
  13 +
  14 + expose(:apps) {
  15 + app_scope.all.sort
  16 + }
  17 +
  18 + expose(:app, :ancestor => :app_scope)
  19 +
  20 + expose(:all_errs) {
  21 + !!params[:all_errs]
  22 + }
  23 + expose(:problems) {
  24 + if request.format == :atom
  25 + app.problems.unresolved.ordered
  26 + else
  27 + pr = app.problems
  28 + pr = pr.unresolved unless all_errs
  29 + pr.in_env(
  30 + params[:environment]
  31 + ).ordered_by(params_sort, params_order).page(params[:page]).per(current_user.per_page)
  32 + end
  33 + }
14 34  
15   - @problems = resource.problems
16   - @problems = @problems.unresolved unless @all_errs
17   - @problems = @problems.in_env(params[:environment]).ordered_by(params_sort, params_order).page(params[:page]).per(current_user.per_page)
  35 + expose(:deploys) {
  36 + app.deploys.order_by(:created_at.desc).limit(5)
  37 + }
18 38  
19   - @deploys = @app.deploys.order_by(:created_at.desc).limit(5)
20   - end
21   - format.atom do
22   - @problems = resource.problems.unresolved.ordered
23   - end
24   - end
  39 + def index; end
  40 + def show
  41 + app
  42 + end
  43 +
  44 + def new
  45 + plug_params(app)
25 46 end
26 47  
27 48 def create
28   - @app = App.new(params[:app])
29 49 initialize_subclassed_issue_tracker
30 50 initialize_subclassed_notification_service
31   - create!
  51 + if app.save
  52 + redirect_to app_url(app), :flash => { :success => I18n.t('controllers.apps.flash.create.success') }
  53 + else
  54 + flash[:error] = I18n.t('controllers.apps.flash.create.error')
  55 + render :new
  56 + end
32 57 end
33 58  
34 59 def update
35   - @app = resource
36 60 initialize_subclassed_issue_tracker
37 61 initialize_subclassed_notification_service
38   - update!
  62 + if app.save
  63 + redirect_to app_url(app), :flash => { :success => I18n.t('controllers.apps.flash.update.success') }
  64 + else
  65 + flash[:error] = I18n.t('controllers.apps.flash.update.error')
  66 + render :edit
  67 + end
39 68 end
40 69  
41   - def new
42   - plug_params(build_resource)
43   - new!
  70 + def edit
  71 + plug_params(app)
44 72 end
45 73  
46   - def edit
47   - plug_params(resource)
48   - edit!
  74 + def destroy
  75 + if app.destroy
  76 + redirect_to apps_url, :flash => { :success => I18n.t('controllers.apps.flash.destroy.success') }
  77 + else
  78 + flash[:error] = I18n.t('controllers.apps.flash.destroy.error')
  79 + render :show
  80 + end
49 81 end
50 82  
51 83 protected
52   - def collection
53   - @apps ||= end_of_association_chain.all.sort
54   - end
55 84  
56 85 def initialize_subclassed_issue_tracker
57 86 # set the app's issue tracker
58 87 if params[:app][:issue_tracker_attributes] && tracker_type = params[:app][:issue_tracker_attributes][:type]
59 88 if IssueTracker.subclasses.map(&:name).concat(["IssueTracker"]).include?(tracker_type)
60   - @app.issue_tracker = tracker_type.constantize.new(params[:app][:issue_tracker_attributes])
  89 + app.issue_tracker = tracker_type.constantize.new(params[:app][:issue_tracker_attributes])
61 90 end
62 91 end
63 92 end
... ... @@ -66,21 +95,11 @@ class AppsController &lt; InheritedResources::Base
66 95 # set the app's notification service
67 96 if params[:app][:notification_service_attributes] && notification_type = params[:app][:notification_service_attributes][:type]
68 97 if NotificationService.subclasses.map(&:name).concat(["NotificationService"]).include?(notification_type)
69   - @app.notification_service = notification_type.constantize.new(params[:app][:notification_service_attributes])
  98 + app.notification_service = notification_type.constantize.new(params[:app][:notification_service_attributes])
70 99 end
71 100 end
72 101 end
73 102  
74   - def begin_of_association_chain
75   - # Filter the @apps collection to apps watched by the current user, unless user is an admin.
76   - # If user is an admin, then no filter is applied, and all apps are shown.
77   - current_user unless current_user.admin?
78   - end
79   -
80   - def interpolation_options
81   - {:app_name => resource.name}
82   - end
83   -
84 103 def plug_params app
85 104 app.watchers.build if app.watchers.none?
86 105 app.issue_tracker = IssueTracker.new unless app.issue_tracker_configured?
... ...
app/controllers/problems_searcher.rb
... ... @@ -29,5 +29,6 @@ module ProblemsSearcher
29 29 expose(:err_ids) {
30 30 (params[:problems] || []).compact
31 31 }
  32 +
32 33 end
33 34 end
... ...
app/helpers/apps_helper.rb
... ... @@ -41,7 +41,7 @@ module AppsHelper
41 41 def detect_any_apps_with_attributes
42 42 @any_github_repos = @any_issue_trackers = @any_deploys = @any_bitbucket_repos = @any_notification_services = false
43 43  
44   - @apps.each do |app|
  44 + apps.each do |app|
45 45 @any_github_repos ||= app.github_repo?
46 46 @any_bitbucket_repos ||= app.bitbucket_repo?
47 47 @any_issue_trackers ||= app.issue_tracker_configured?
... ...
app/views/apps/_fields.html.haml
1   -= errors_for @app
  1 += errors_for app
2 2  
3 3 %div.required
4 4 = f.label :name
... ...
app/views/apps/edit.html.haml
1 1 - content_for :title, 'Edit App'
2 2 - content_for :action_bar do
3 3 = link_to_copy_attributes_from_other_app
4   - = link_to 'destroy application', app_path(@app), :method => :delete, :data => { :confirm => 'Seriously?' }, :class => 'button'
5   - = link_to('cancel', app_path(@app), :class => 'button')
  4 + = link_to 'destroy application', app_path(app), :method => :delete, :data => { :confirm => 'Seriously?' }, :class => 'button'
  5 + = link_to('cancel', app_path(app), :class => 'button')
6 6  
7   -= form_for @app do |f|
  7 += form_for app do |f|
8 8  
9 9 = render 'fields', :f => f
10 10  
... ...
app/views/apps/index.html.haml
... ... @@ -16,7 +16,7 @@
16 16 %th Last Deploy
17 17 %th Errors
18 18 %tbody
19   - - @apps.each do |app|
  19 + - apps.each do |app|
20 20 %tr
21 21 %td.name= link_to app.name, app_path(app)
22 22 - if any_github_repos? or any_bitbucket_repos?
... ... @@ -50,7 +50,7 @@
50 50 - if app.problem_count > 0
51 51 - unresolved = app.unresolved_count
52 52 = link_to unresolved, app_path(app), :class => (unresolved == 0 ? "resolved" : nil)
53   - - if @apps.none?
  53 + - if apps.none?
54 54 %tr
55 55 %td{:colspan => 3}
56 56 %em
... ...
app/views/apps/new.html.haml
... ... @@ -3,7 +3,7 @@
3 3 = link_to_copy_attributes_from_other_app
4 4 = link_to('cancel', apps_path, :class => 'button')
5 5  
6   -= form_for @app do |f|
  6 += form_for app do |f|
7 7  
8 8 = render 'fields', :f => f
9 9  
... ...
app/views/apps/show.atom.builder
1 1 atom_feed do |feed|
2   - feed.title("Errbit notices for #{h @app.name} at #{root_url}")
  2 + feed.title("Errbit notices for #{h app.name} at #{root_url}")
3 3 render "problems/list", :feed => feed
4 4 end
... ...
app/views/apps/show.html.haml
1   -- content_for :title, @app.name
  1 +- content_for :title, app.name
2 2 - content_for :head do
3   - = 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}"
  3 + = 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}"
4 4 - content_for :meta do
5 5 %strong Errors Caught:
6   - = @app.problems.count
  6 + = app.problems.count
7 7 %strong Deploy Count:
8   - = @app.deploys.count
  8 + = app.deploys.count
9 9 %strong API Key:
10   - = @app.api_key
  10 + = app.api_key
11 11 - content_for :action_bar do
12 12 - if current_user.admin?
13   - = link_to 'edit', edit_app_path(@app), :class => 'button'
14   - - if @all_errs
15   - = link_to 'unresolved errs', app_path(@app), :class => 'button'
  13 + = link_to 'edit', edit_app_path(app), :class => 'button'
  14 + - if all_errs
  15 + = link_to 'unresolved errs', app_path(app), :class => 'button'
16 16 - else
17   - = link_to 'all errs', app_path(@app, :all_errs => true), :class => 'button'
  17 + = link_to 'all errs', app_path(app, :all_errs => true), :class => 'button'
18 18  
19 19 %h3#watchers_toggle
20 20 Watchers
21 21 %span.click_span (show/hide)
22 22 #watchers_div
23   - - if @app.notify_all_users
  23 + - if app.notify_all_users
24 24 %table.watchers
25 25 %thead
26 26 %tr
... ... @@ -31,15 +31,15 @@
31 31 %tr
32 32 %th User or Email
33 33 %tbody
34   - - @app.watchers.each do |watcher|
  34 + - app.watchers.each do |watcher|
35 35 %tr
36 36 %td= watcher.label
37   - - if @app.watchers.none?
  37 + - if app.watchers.none?
38 38 %tr
39 39 %td
40 40 %em Sadly, no one is watching this app
41 41  
42   -- if @app.github_repo?
  42 +- if app.github_repo?
43 43 %h3#repository_toggle
44 44 Repository
45 45 %span.click_span (show/hide)
... ... @@ -50,13 +50,13 @@
50 50 %th GitHub Repo
51 51 %tbody
52 52 %tr
53   - %td= link_to(@app.github_repo, @app.github_url, :target => '_blank')
  53 + %td= link_to(app.github_repo, app.github_url, :target => '_blank')
54 54  
55 55 %h3#deploys_toggle
56 56 Latest Deploys
57 57 %span.click_span (show/hide)
58 58 #deploys_div
59   - - if @deploys.any?
  59 + - if deploys.any?
60 60 %table.deploys
61 61 %thead
62 62 %tr
... ... @@ -68,7 +68,7 @@
68 68 %th Revision
69 69  
70 70 %tbody
71   - - @deploys.each do |deploy|
  71 + - deploys.each do |deploy|
72 72 %tr
73 73 %td.when #{deploy.created_at.to_s(:micro)}
74 74 %td.environment #{deploy.environment}
... ... @@ -76,20 +76,20 @@
76 76 %td.message #{deploy.message}
77 77 %td.repository #{deploy.repository}
78 78 %td.revision #{deploy.short_revision}
79   - = link_to "All Deploys (#{@app.deploys.count})", app_deploys_path(@app), :class => 'button'
  79 + = link_to "All Deploys (#{app.deploys.count})", app_deploys_path(app), :class => 'button'
80 80 - else
81 81 %h3 No deploys
82 82  
83   -- if @app.problems.any?
  83 +- if app.problems.any?
84 84 %h3.clear Errors
85 85 %section
86   - = form_tag search_problems_path(:all_errs => @all_errs, :app_id => @app.id), :method => :get, :remote => true do
  86 + = form_tag search_problems_path(:all_errs => all_errs, :app_id => app.id), :method => :get, :remote => true do
87 87 = text_field_tag :search, params[:search], :placeholder => 'Search for issues'
88 88 %br
89 89 %section
90 90 .problem_table{:id => 'problem_table'}
91   - = render 'problems/table', :problems => @problems
  91 + = render 'problems/table', :problems => problems
92 92 - else
93 93 %h3.clear No errs have been caught yet, make sure you setup your app
94   - = render 'configuration_instructions', :app => @app
  94 + = render 'configuration_instructions', :app => app
95 95  
... ...
app/views/problems/_list.atom.builder
1   -feed.updated(@problems.first.try(:created_at) || Time.now)
  1 +feed.updated(problems.first.try(:created_at) || Time.now)
2 2  
3   -for problem in @problems
  3 +for problem in problems
4 4 notice = problem.notices.first
5 5  
6 6 feed.entry(problem, :url => app_problem_url(problem.app.to_param, problem.to_param)) do |entry|
... ...
config/initializers/inherited_resources.rb
... ... @@ -1,2 +0,0 @@
1   -InheritedResources.flash_keys = [:success, :error]
2   -
config/locales/en.yml
... ... @@ -2,7 +2,6 @@
2 2 # See http://github.com/svenfuchs/rails-i18n/tree/master/rails%2Flocale for starting points.
3 3  
4 4 en:
5   -
6 5 flash:
7 6 apps:
8 7 create:
... ... @@ -32,6 +31,18 @@ en:
32 31 edit_profile: 'Edit profile'
33 32  
34 33 controllers:
  34 + apps:
  35 + flash:
  36 + create:
  37 + success: "Your app was successfully created."
  38 + error: "You app was successfully destroyed."
  39 + update:
  40 + success: "You app was successfully updated."
  41 + error: "You app was not updated"
  42 + destroy:
  43 + success: "You app was successfully destroyed."
  44 + error: "You app could not be destroyed."
  45 +
35 46 users:
36 47 flash:
37 48 destroy:
... ... @@ -46,6 +57,7 @@ en:
46 57 merge_several:
47 58 success: "%{nb} errors have been merged."
48 59  
  60 +
49 61 devise:
50 62 registrations:
51 63 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.'
... ...
spec/controllers/apps_controller_spec.rb
... ... @@ -6,15 +6,13 @@ describe AppsController do
6 6 it_requires_authentication
7 7 it_requires_admin_privileges :for => {:new => :get, :edit => :get, :create => :post, :update => :put, :destroy => :delete}
8 8  
9   -
10 9 describe "GET /apps" do
11 10 context 'when logged in as an admin' do
12 11 it 'finds all apps' do
13 12 sign_in Fabricate(:admin)
14 13 3.times { Fabricate(:app) }
15   - apps = App.all
16 14 get :index
17   - assigns(:apps).should == apps
  15 + controller.apps.should == App.all.sort.entries
18 16 end
19 17 end
20 18  
... ... @@ -27,8 +25,8 @@ describe AppsController do
27 25 Fabricate(:user_watcher, :user => user, :app => watched_app1)
28 26 Fabricate(:user_watcher, :user => user, :app => watched_app2)
29 27 get :index
30   - assigns(:apps).should include(watched_app1, watched_app2)
31   - assigns(:apps).should_not include(unwatched_app)
  28 + controller.apps.should include(watched_app1, watched_app2)
  29 + controller.apps.should_not include(unwatched_app)
32 30 end
33 31 end
34 32 end
... ... @@ -44,7 +42,7 @@ describe AppsController do
44 42  
45 43 it 'finds the app' do
46 44 get :show, :id => @app.id
47   - assigns(:app).should == @app
  45 + controller.app.should == @app
48 46 end
49 47  
50 48 it "should not raise errors for app with err without notices" do
... ... @@ -65,13 +63,13 @@ describe AppsController do
65 63  
66 64 it "should have default per_page value for user" do
67 65 get :show, :id => @app.id
68   - assigns(:problems).to_a.size.should == User::PER_PAGE
  66 + controller.problems.to_a.size.should == User::PER_PAGE
69 67 end
70 68  
71 69 it "should be able to override default per_page value" do
72 70 @user.update_attribute :per_page, 10
73 71 get :show, :id => @app.id
74   - assigns(:problems).to_a.size.should == 10
  72 + controller.problems.to_a.size.should == 10
75 73 end
76 74 end
77 75  
... ... @@ -85,14 +83,14 @@ describe AppsController do
85 83 context 'and no params' do
86 84 it 'shows only unresolved problems' do
87 85 get :show, :id => @app.id
88   - assigns(:problems).size.should == 1
  86 + controller.problems.size.should == 1
89 87 end
90 88 end
91 89  
92 90 context 'and all_problems=true params' do
93 91 it 'shows all errors' do
94 92 get :show, :id => @app.id, :all_errs => true
95   - assigns(:problems).size.should == 2
  93 + controller.problems.size.should == 2
96 94 end
97 95 end
98 96 end
... ... @@ -108,35 +106,35 @@ describe AppsController do
108 106 context 'no params' do
109 107 it 'shows errs for all environments' do
110 108 get :show, :id => @app.id
111   - assigns(:problems).size.should == 21
  109 + controller.problems.size.should == 21
112 110 end
113 111 end
114 112  
115 113 context 'environment production' do
116 114 it 'shows errs for just production' do
117 115 get :show, :id => @app.id, :environment => 'production'
118   - assigns(:problems).size.should == 6
  116 + controller.problems.size.should == 6
119 117 end
120 118 end
121 119  
122 120 context 'environment staging' do
123 121 it 'shows errs for just staging' do
124 122 get :show, :id => @app.id, :environment => 'staging'
125   - assigns(:problems).size.should == 5
  123 + controller.problems.size.should == 5
126 124 end
127 125 end
128 126  
129 127 context 'environment development' do
130 128 it 'shows errs for just development' do
131 129 get :show, :id => @app.id, :environment => 'development'
132   - assigns(:problems).size.should == 5
  130 + controller.problems.size.should == 5
133 131 end
134 132 end
135 133  
136 134 context 'environment test' do
137 135 it 'shows errs for just test' do
138 136 get :show, :id => @app.id, :environment => 'test'
139   - assigns(:problems).size.should == 5
  137 + controller.problems.size.should == 5
140 138 end
141 139 end
142 140 end
... ... @@ -149,7 +147,7 @@ describe AppsController do
149 147 watcher = Fabricate(:user_watcher, :app => app, :user => user)
150 148 sign_in user
151 149 get :show, :id => app.id
152   - assigns(:app).should == app
  150 + controller.app.should == app
153 151 end
154 152  
155 153 it 'does not find the app if the user is not watching it' do
... ... @@ -170,19 +168,19 @@ describe AppsController do
170 168 describe "GET /apps/new" do
171 169 it 'instantiates a new app with a prebuilt watcher' do
172 170 get :new
173   - assigns(:app).should be_a(App)
174   - assigns(:app).should be_new_record
175   - assigns(:app).watchers.should_not be_empty
  171 + controller.app.should be_a(App)
  172 + controller.app.should be_new_record
  173 + controller.app.watchers.should_not be_empty
176 174 end
177 175  
178 176 it "should copy attributes from an existing app" do
179 177 @app = Fabricate(:app, :name => "do not copy",
180 178 :github_repo => "test/example")
181 179 get :new, :copy_attributes_from => @app.id
182   - assigns(:app).should be_a(App)
183   - assigns(:app).should be_new_record
184   - assigns(:app).name.should be_blank
185   - assigns(:app).github_repo.should == "test/example"
  180 + controller.app.should be_a(App)
  181 + controller.app.should be_new_record
  182 + controller.app.name.should be_blank
  183 + controller.app.github_repo.should == "test/example"
186 184 end
187 185 end
188 186  
... ... @@ -190,7 +188,7 @@ describe AppsController do
190 188 it 'finds the correct app' do
191 189 app = Fabricate(:app)
192 190 get :edit, :id => app.id
193   - assigns(:app).should == app
  191 + controller.app.should == app
194 192 end
195 193 end
196 194  
... ... @@ -326,12 +324,11 @@ describe AppsController do
326 324 describe "DELETE /apps/:id" do
327 325 before do
328 326 @app = Fabricate(:app)
329   - App.stub(:find).with(@app.id).and_return(@app)
330 327 end
331 328  
332 329 it "should find the app" do
333 330 delete :destroy, :id => @app.id
334   - assigns(:app).should == @app
  331 + controller.app.should == @app
335 332 end
336 333  
337 334 it "should destroy the app" do
... ...
spec/views/apps/edit.html.haml_spec.rb
... ... @@ -3,7 +3,7 @@ require &#39;spec_helper&#39;
3 3 describe "apps/edit.html.haml" do
4 4 before do
5 5 app = stub_model(App)
6   - assign :app, app
  6 + view.stub(:app).and_return(app)
7 7 controller.stub(:current_user) { stub_model(User) }
8 8 end
9 9  
... ...
spec/views/apps/index.html.haml_spec.rb
... ... @@ -3,7 +3,7 @@ require &#39;spec_helper&#39;
3 3 describe "apps/index.html.haml" do
4 4 before do
5 5 app = stub_model(App, :deploys => [stub_model(Deploy, :created_at => Time.now, :revision => "123456789abcdef")])
6   - assign :apps, [app]
  6 + view.stub(:apps).and_return([app])
7 7 controller.stub(:current_user) { stub_model(User) }
8 8 end
9 9  
... ...
spec/views/problems/index.atom.builder_spec.rb
... ... @@ -4,7 +4,7 @@ describe &quot;problems/index.atom.builder&quot; do
4 4  
5 5 it 'display problem message' do
6 6 app = App.new(:new_record => false)
7   - assign(:problems, [Problem.new(
  7 + view.stub(:problems).and_return([Problem.new(
8 8 :message => 'foo',
9 9 :new_record => false, :app => app), Problem.new(:new_record => false, :app => app)])
10 10 render
... ...