From 3dc8886beb7d3689c87d9aa1e5ad2d4c6c5b4c55 Mon Sep 17 00:00:00 2001 From: jessib Date: Tue, 15 Jan 2013 11:55:12 -0800 Subject: Refactoring of tickets controller to fetch the ticket in a before filter for relevant actions. --- help/app/controllers/tickets_controller.rb | 67 ++++++++++++++---------------- 1 file changed, 31 insertions(+), 36 deletions(-) (limited to 'help/app') diff --git a/help/app/controllers/tickets_controller.rb b/help/app/controllers/tickets_controller.rb index d4aa378..d47939e 100644 --- a/help/app/controllers/tickets_controller.rb +++ b/help/app/controllers/tickets_controller.rb @@ -6,6 +6,7 @@ class TicketsController < ApplicationController before_filter :set_strings before_filter :authorize, :only => [:index] + before_filter :fetch_ticket, :only => [:show, :update, :destroy] # don't now have an edit method def new @ticket = Ticket.new @@ -31,52 +32,44 @@ class TicketsController < ApplicationController =begin def edit - @ticket = Ticket.find(params[:id]) @ticket.comments.build # build ticket comments? end =end def show - @ticket = Ticket.find(params[:id]) @comment = TicketComment.new if !@ticket redirect_to tickets_path, :alert => "No such ticket" return end - ticket_access_denied? #authorize_ticket_access - # @ticket.comments.build - # build ticket comments? end def update - @ticket = Ticket.find(params[:id]) - if !ticket_access_denied? - if params[:post] #currently changes to title or is_open status - @ticket.attributes = params[:post] - # TODO: do we want to keep the history of title changes? one possibility was adding a comment that said something like 'user changed the title from a to b' - - else - params[:ticket][:comments_attributes] = nil if params[:ticket][:comments_attributes].values.first[:body].blank? #unset comments hash if no new comment was typed - @ticket.attributes = params[:ticket] #this will call comments_attributes= - @ticket.close if params[:commit] == @reply_close_str #this overrides is_open selection - # what if there is an update and no new comment? Confirm that there is a new comment to update posted_by: - @ticket.comments.last.posted_by = (current_user ? current_user.id : nil) if @ticket.comments_changed? #protecting posted_by isn't working, so this should protect it. - end - if @ticket.changed? and @ticket.save - flash[:notice] = 'Ticket was successfully updated.' - if @ticket.is_open || !logged_in? - respond_with @ticket - else #for closed tickets with authenticated users, redirect to index. - redirect_to tickets_path - end - else - #redirect_to [:show, @ticket] # - flash[:alert] = 'Ticket has not been changed' - redirect_to @ticket - #respond_with(@ticket) # why does this go to edit?? redirect??? + if params[:post] #currently changes to title or is_open status + @ticket.attributes = params[:post] + # TODO: do we want to keep the history of title changes? one possibility was adding a comment that said something like 'user changed the title from a to b' + + else + params[:ticket][:comments_attributes] = nil if params[:ticket][:comments_attributes].values.first[:body].blank? #unset comments hash if no new comment was typed + @ticket.attributes = params[:ticket] #this will call comments_attributes= + @ticket.close if params[:commit] == @reply_close_str #this overrides is_open selection + # what if there is an update and no new comment? Confirm that there is a new comment to update posted_by: + @ticket.comments.last.posted_by = (current_user ? current_user.id : nil) if @ticket.comments_changed? #protecting posted_by isn't working, so this should protect it. + end + if @ticket.changed? and @ticket.save + flash[:notice] = 'Ticket was successfully updated.' + if @ticket.is_open || !logged_in? + respond_with @ticket + else #for closed tickets with authenticated users, redirect to index. + redirect_to tickets_path end + else + #redirect_to [:show, @ticket] # + flash[:alert] = 'Ticket has not been changed' + redirect_to @ticket + #respond_with(@ticket) # why does this go to edit?? redirect??? end end @@ -88,7 +81,6 @@ class TicketsController < ApplicationController def destroy # should we allow non-admins to delete their own tickets? i don't think necessary. - @ticket = Ticket.find(params[:id]) @ticket.destroy if admin? redirect_to tickets_path end @@ -99,16 +91,19 @@ class TicketsController < ApplicationController @ticket and (admin? or !@ticket.created_by or (current_user and current_user.id == @ticket.created_by)) end - def ticket_access_denied? - access_denied unless ticket_access? - end - - def set_strings @post_reply_str = 'Post reply' #t :post_reply @reply_close_str = 'Reply and close' #t :reply_and_close end + def fetch_ticket + @ticket = Ticket.find(params[:id]) + if !@ticket and admin? + redirect_to tickets_path, :alert => "No such ticket" + return + end + access_denied unless ticket_access? + end # not using now, as we are using comment_attributes= from the Ticket model =begin def add_comment -- cgit v1.2.3 From e2021bdcc40b51ab5e571c97e882bba10dc80ad6 Mon Sep 17 00:00:00 2001 From: jessib Date: Tue, 15 Jan 2013 12:52:09 -0800 Subject: For both users and tickets, if the object is not found and the current user is an admin, they should see an alert that the object wasn't found, and be redirected to the current controller. If the object isn't found and the current user is not an admin, then we will continue to give an error about no access, so as not to leak information about what IDs do and don't exist. --- help/app/controllers/tickets_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'help/app') diff --git a/help/app/controllers/tickets_controller.rb b/help/app/controllers/tickets_controller.rb index d47939e..b613088 100644 --- a/help/app/controllers/tickets_controller.rb +++ b/help/app/controllers/tickets_controller.rb @@ -99,7 +99,7 @@ class TicketsController < ApplicationController def fetch_ticket @ticket = Ticket.find(params[:id]) if !@ticket and admin? - redirect_to tickets_path, :alert => "No such ticket" + redirect_to tickets_path, :alert => t(:no_such_thing, :thing => 'ticket') return end access_denied unless ticket_access? -- cgit v1.2.3