From: Jan Provaznik jprovazn@redhat.com
* html_error_page chose layout by @layout, but @layout is nil when error occurs (for example in controller) and then popup-error.rhtml template is used (this template should be used for ajax-requests, shouldn't be?) -> request.xhr? is used instead of @layout * when @layout was set, html_error_page set only layout and not template, so error wasn't displayed -> added simple layout/error.rhtml * passing of title and errmsg is now done with :locals, passing with @title and @errmsg doesn't work, if TemplateError occurs (for example use of undefined variable in template) --- src/app/controllers/application_controller.rb | 10 +++++----- src/app/views/layouts/error.rhtml | 4 ++++ src/app/views/layouts/popup-error.rhtml | 4 ++-- 3 files changed, 11 insertions(+), 7 deletions(-) create mode 100644 src/app/views/layouts/error.rhtml
diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb index a563d57..0bc3ca7 100644 --- a/src/app/controllers/application_controller.rb +++ b/src/app/controllers/application_controller.rb @@ -111,12 +111,12 @@ class ApplicationController < ActionController::Base end
def html_error_page(title, msg) - @title = title - @errmsg = msg - if @layout - render :layout => 'aggregator' + if request.xhr? + render :template => 'layouts/popup-error', :layout => 'popup', + :locals => {:title => title, :errmsg => msg} else - render :template => 'layouts/popup-error', :layout => 'popup' + render :template => 'layouts/error', :layout => 'aggregator', + :locals => {:title => title, :errmsg => msg} end end
diff --git a/src/app/views/layouts/error.rhtml b/src/app/views/layouts/error.rhtml new file mode 100644 index 0000000..094c13a --- /dev/null +++ b/src/app/views/layouts/error.rhtml @@ -0,0 +1,4 @@ +<%- content_for :title do -%> + <%= title %> +<%- end -%> +<%= errmsg %> diff --git a/src/app/views/layouts/popup-error.rhtml b/src/app/views/layouts/popup-error.rhtml index 8e58a31..094c13a 100644 --- a/src/app/views/layouts/popup-error.rhtml +++ b/src/app/views/layouts/popup-error.rhtml @@ -1,4 +1,4 @@ <%- content_for :title do -%> - <%= @title %> + <%= title %> <%- end -%> -<%= @errmsg %> +<%= errmsg %>
From: Jan Provaznik jprovazn@redhat.com
Moved H2 title on some forms into dcloud_form div to keep same style as other Aggregator forms. --- src/app/views/user_sessions/new.html.erb | 3 +-- src/app/views/users/edit.html.erb | 3 +-- src/app/views/users/new.html.erb | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/app/views/user_sessions/new.html.erb b/src/app/views/user_sessions/new.html.erb index d6dc85b..a058b8a 100644 --- a/src/app/views/user_sessions/new.html.erb +++ b/src/app/views/user_sessions/new.html.erb @@ -2,9 +2,8 @@ TODO: decide if we want to have the concept of named Aggregators and add that below if so %> -<h2 class="greeting">Welcome to the Deltacloud Aggregator. Please sign in.</h2> - <div class="dcloud_form"> + <h2>Welcome to the Deltacloud Aggregator. Please sign in.</h2> <% form_for @user_session, :url => user_session_path do |f| %> <%= f.error_messages %> <ul> diff --git a/src/app/views/users/edit.html.erb b/src/app/views/users/edit.html.erb index 0d2de48..79e1a1c 100644 --- a/src/app/views/users/edit.html.erb +++ b/src/app/views/users/edit.html.erb @@ -1,6 +1,5 @@ -<h2 class="greeting">Edit my profile.</h2> - <div class="dcloud_form"> + <h2>Edit my profile.</h2> <% form_for @user, :url => account_path do |f| %> <%= f.error_messages %> <%= render :partial => "form", :object => f %> diff --git a/src/app/views/users/new.html.erb b/src/app/views/users/new.html.erb index 0027ef6..e88cd12 100644 --- a/src/app/views/users/new.html.erb +++ b/src/app/views/users/new.html.erb @@ -1,6 +1,5 @@ -<h2 class="greeting">New Account</h2> - <div class="dcloud_form"> + <h2>New Account</h2> <% form_for @user, :url => account_path do |f| %> <%= f.error_messages %> <%= render :partial => "form", :object => f %>
On Mon, 2010-08-02 at 14:47 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznik jprovazn@redhat.com
Moved H2 title on some forms into dcloud_form div to keep same style as other Aggregator forms.
src/app/views/user_sessions/new.html.erb | 3 +-- src/app/views/users/edit.html.erb | 3 +-- src/app/views/users/new.html.erb | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-)
ACK
On Mon, 2010-08-02 at 14:47 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznik jprovazn@redhat.com
- html_error_page chose layout by @layout, but @layout is nil when error occurs (for example in controller) and then popup-error.rhtml template is used (this template should be used for ajax-requests, shouldn't be?) -> request.xhr? is used instead of @layout
- when @layout was set, html_error_page set only layout and not template, so error wasn't displayed -> added simple layout/error.rhtml
- passing of title and errmsg is now done with :locals, passing with @title and @errmsg doesn't work, if TemplateError occurs (for example use of undefined variable in template)
This appears to be fine, looks reasonable, pages stil seem to render fine, but kind of hard to tell for sure without being able to easily test for the error before the patch, and lack of error after. I'll say ACK for now, but if you could get a test case in for this I would be much happier with the fix.
-j
From: Jan Provaznik jprovazn@redhat.com
--- src/spec/controllers/users_controller_spec.rb | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/src/spec/controllers/users_controller_spec.rb b/src/spec/controllers/users_controller_spec.rb index c6ce46c..29c2f41 100644 --- a/src/spec/controllers/users_controller_spec.rb +++ b/src/spec/controllers/users_controller_spec.rb @@ -99,4 +99,14 @@ describe UsersController do put :update, :id => @tuser.id, :user => { } response.should redirect_to(account_path) end + + # checks whether proper error template is rendered when an exception raises + # "layouts/error" template should be displayed for all non-ajax error + # responses, "layouts/popup-error" should be displayed for ajax + # (see "Fixed error handling" patch for details) + it "should render error template when getting nonexisting user" do + UserSession.create(@tuser) + get :show, :id => "unknown_id" + response.should render_template("layouts/error") + end end
On Mon, 2010-08-09 at 14:45 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznik jprovazn@redhat.com
src/spec/controllers/users_controller_spec.rb | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/src/spec/controllers/users_controller_spec.rb b/src/spec/controllers/users_controller_spec.rb index c6ce46c..29c2f41 100644 --- a/src/spec/controllers/users_controller_spec.rb +++ b/src/spec/controllers/users_controller_spec.rb @@ -99,4 +99,14 @@ describe UsersController do put :update, :id => @tuser.id, :user => { } response.should redirect_to(account_path) end
- # checks whether proper error template is rendered when an exception raises
- # "layouts/error" template should be displayed for all non-ajax error
- # responses, "layouts/popup-error" should be displayed for ajax
- # (see "Fixed error handling" patch for details)
- it "should render error template when getting nonexisting user" do
- UserSession.create(@tuser)
- get :show, :id => "unknown_id"
- response.should render_template("layouts/error")
- end
end
ACK
deltacloud-devel@lists.fedorahosted.org