From 550ed0aa483d0b31a6f844a728340e5a81a753ed Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Tue, 11 Mar 2014 12:53:49 +0000 Subject: Graceful failure of new_comment route Fixes https://github.com/mysociety/alaveteli/issues/662 If /annotate/request/:url_title is accessed when comments are disabled an exception is incorrectly thrown. Conditionals should be used for control flow, so now the action redirects to the info_request path and displays a notice. --- app/controllers/comment_controller.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'app/controllers/comment_controller.rb') diff --git a/app/controllers/comment_controller.rb b/app/controllers/comment_controller.rb index cda56a211..4904463b5 100644 --- a/app/controllers/comment_controller.rb +++ b/app/controllers/comment_controller.rb @@ -21,13 +21,15 @@ class CommentController < ApplicationController else raise "Unknown type " + params[:type] end - + # Are comments disabled on this request? # # There is no “add comment” link when comments are disabled, so users should - # not usually hit this unless they are explicitly attempting to avoid the comment - # block, so we just raise an exception. - raise "Comments are not allowed on this request" if !@info_request.comments_allowed? + # not usually hit this unless they are explicitly attempting to avoid the comment block + unless @info_request.comments_allowed? + redirect_to request_url(@info_request), :notice => "Comments are not allowed on this request" + return + end # Banned from adding comments? if !authenticated_user.nil? && !authenticated_user.can_make_comments? -- cgit v1.2.3 From 10fbde73f2782df0f007c668d07c8067b2fbb4dc Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Tue, 11 Mar 2014 14:32:19 +0000 Subject: Extract find_info_request from CommentController Use a before_filter to make @info_request available to all filters called on the same action --- app/controllers/comment_controller.rb | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) (limited to 'app/controllers/comment_controller.rb') diff --git a/app/controllers/comment_controller.rb b/app/controllers/comment_controller.rb index 4904463b5..c2315b73e 100644 --- a/app/controllers/comment_controller.rb +++ b/app/controllers/comment_controller.rb @@ -6,20 +6,16 @@ class CommentController < ApplicationController before_filter :check_read_only, :only => [ :new ] + before_filter :find_info_request, :only => [ :new ] protect_from_forgery :only => [ :new ] def new - if params[:type] == 'request' - @info_request = InfoRequest.find_by_url_title!(params[:url_title]) - @track_thing = TrackThing.create_track_for_request(@info_request) - if params[:comment] - @comment = Comment.new(params[:comment].merge({ - :comment_type => 'request', - :user => @user - })) - end - else - raise "Unknown type " + params[:type] + @track_thing = TrackThing.create_track_for_request(@info_request) + if params[:comment] + @comment = Comment.new(params[:comment].merge({ + :comment_type => 'request', + :user => @user + })) end # Are comments disabled on this request? @@ -94,5 +90,14 @@ class CommentController < ApplicationController end end -end + private + def find_info_request + if params[:type] == 'request' + @info_request = InfoRequest.find_by_url_title!(params[:url_title]) + else + raise "Unknown type #{ params[:type] }" + end + end + +end -- cgit v1.2.3 From ec925323afce0e2f2c3dbca0b3a1efb054d468e6 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Tue, 11 Mar 2014 14:33:51 +0000 Subject: Extract create_track_thing from CommentController Use a before_filter to make @track_thing available to all filters called on the same action and remove responsibility from the #new method --- app/controllers/comment_controller.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'app/controllers/comment_controller.rb') diff --git a/app/controllers/comment_controller.rb b/app/controllers/comment_controller.rb index c2315b73e..18544a98f 100644 --- a/app/controllers/comment_controller.rb +++ b/app/controllers/comment_controller.rb @@ -7,10 +7,10 @@ class CommentController < ApplicationController before_filter :check_read_only, :only => [ :new ] before_filter :find_info_request, :only => [ :new ] + before_filter :create_track_thing, :only => [ :new ] protect_from_forgery :only => [ :new ] def new - @track_thing = TrackThing.create_track_for_request(@info_request) if params[:comment] @comment = Comment.new(params[:comment].merge({ :comment_type => 'request', @@ -100,4 +100,8 @@ class CommentController < ApplicationController end end + def create_track_thing + @track_thing = TrackThing.create_track_for_request(@info_request) + end + end -- cgit v1.2.3 From 68a9536dc68f35965156aeb84c9f4c5bee391159 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Tue, 11 Mar 2014 14:35:32 +0000 Subject: Use filter to reject unless comments allowed Extract checking whether comments are allowed on an InfoRequest to a filter in CommentController. Removes responsibility from the #new method. --- app/controllers/comment_controller.rb | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) (limited to 'app/controllers/comment_controller.rb') diff --git a/app/controllers/comment_controller.rb b/app/controllers/comment_controller.rb index 18544a98f..ce022f000 100644 --- a/app/controllers/comment_controller.rb +++ b/app/controllers/comment_controller.rb @@ -8,6 +8,7 @@ class CommentController < ApplicationController before_filter :check_read_only, :only => [ :new ] before_filter :find_info_request, :only => [ :new ] before_filter :create_track_thing, :only => [ :new ] + before_filter :reject_unless_comments_allowed, :only => [ :new ] protect_from_forgery :only => [ :new ] def new @@ -18,15 +19,6 @@ class CommentController < ApplicationController })) end - # Are comments disabled on this request? - # - # There is no “add comment” link when comments are disabled, so users should - # not usually hit this unless they are explicitly attempting to avoid the comment block - unless @info_request.comments_allowed? - redirect_to request_url(@info_request), :notice => "Comments are not allowed on this request" - return - end - # Banned from adding comments? if !authenticated_user.nil? && !authenticated_user.can_make_comments? @details = authenticated_user.can_fail_html @@ -104,4 +96,14 @@ class CommentController < ApplicationController @track_thing = TrackThing.create_track_for_request(@info_request) end + # Are comments disabled on this request? + # + # There is no “add comment” link when comments are disabled, so users should + # not usually hit this unless they are explicitly attempting to avoid the comment block + def reject_unless_comments_allowed + unless @info_request.comments_allowed? + redirect_to request_url(@info_request), :notice => "Comments are not allowed on this request" + end + end + end -- cgit v1.2.3 From 73d0f361fd4e49f11b3b99db7b3dc2b06dc9e9d7 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Tue, 11 Mar 2014 14:37:48 +0000 Subject: Use filter to reject if user is banned Extract checking whether a user is banned from making Comments on an InfoRequest to a filter in CommentController. Removes responsibility from the #new method. Adds a missing spec. --- app/controllers/comment_controller.rb | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) (limited to 'app/controllers/comment_controller.rb') diff --git a/app/controllers/comment_controller.rb b/app/controllers/comment_controller.rb index ce022f000..5e39c3a2c 100644 --- a/app/controllers/comment_controller.rb +++ b/app/controllers/comment_controller.rb @@ -9,6 +9,7 @@ class CommentController < ApplicationController before_filter :find_info_request, :only => [ :new ] before_filter :create_track_thing, :only => [ :new ] before_filter :reject_unless_comments_allowed, :only => [ :new ] + before_filter :reject_if_user_banned, :only => [ :new ] protect_from_forgery :only => [ :new ] def new @@ -19,13 +20,6 @@ class CommentController < ApplicationController })) end - # Banned from adding comments? - if !authenticated_user.nil? && !authenticated_user.can_make_comments? - @details = authenticated_user.can_fail_html - render :template => 'user/banned' - return - end - if params[:comment] # XXX this check should theoretically be a validation rule in the model @existing_comment = Comment.find_existing(@info_request.id, params[:comment][:body]) @@ -106,4 +100,12 @@ class CommentController < ApplicationController end end + # Banned from adding comments? + def reject_if_user_banned + if authenticated_user && !authenticated_user.can_make_comments? + @details = authenticated_user.can_fail_html + render :template => 'user/banned' + end + end + end -- cgit v1.2.3