diff options
| -rw-r--r-- | config/locales/en.yml | 1 | ||||
| -rw-r--r-- | help/app/controllers/tickets_controller.rb | 67 | ||||
| -rw-r--r-- | users/app/controllers/users_controller.rb | 7 | ||||
| -rw-r--r-- | users/test/functional/users_controller_test.rb | 25 |
4 files changed, 62 insertions, 38 deletions
diff --git a/config/locales/en.yml b/config/locales/en.yml index 179c14c..fc61c31 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3,3 +3,4 @@ en: hello: "Hello world" + no_such_thing: "No such %{thing}." diff --git a/help/app/controllers/tickets_controller.rb b/help/app/controllers/tickets_controller.rb index d4aa378..b613088 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 => t(:no_such_thing, :thing => '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 diff --git a/users/app/controllers/users_controller.rb b/users/app/controllers/users_controller.rb index eb93fcb..c0fe243 100644 --- a/users/app/controllers/users_controller.rb +++ b/users/app/controllers/users_controller.rb @@ -2,6 +2,8 @@ class UsersController < ApplicationController skip_before_filter :verify_authenticity_token, :only => [:create] + + before_filter :authorize, :only => [:show, :edit, :update, :destroy] before_filter :fetch_user, :only => [:show, :edit, :update, :destroy] before_filter :set_anchor, :only => [:edit, :update] before_filter :authorize_admin, :only => [:index] @@ -48,7 +50,12 @@ class UsersController < ApplicationController protected def fetch_user + # authorize filter has been checked first, so won't get here unless authenticated @user = User.find_by_param(params[:id]) + if !@user and admin? + redirect_to users_path, :alert => t(:no_such_thing, :thing => 'user') + return + end access_denied unless admin? or (@user == current_user) end diff --git a/users/test/functional/users_controller_test.rb b/users/test/functional/users_controller_test.rb index 46db4d1..9fb06c9 100644 --- a/users/test/functional/users_controller_test.rb +++ b/users/test/functional/users_controller_test.rb @@ -10,10 +10,12 @@ class UsersControllerTest < ActionController::TestCase end test "failed show without login" do - user = find_record :user + user = FactoryGirl.build(:user) + user.save get :show, :id => user.id assert_response :redirect assert_redirected_to login_path + user.destroy end test "user can see user" do @@ -42,7 +44,7 @@ class UsersControllerTest < ActionController::TestCase assert_response :success end - + test "user cannot see other user" do user = find_record :user, :email => nil, @@ -57,6 +59,25 @@ class UsersControllerTest < ActionController::TestCase assert_access_denied end + test "show for non-existing user" do + nonid = 'thisisnotanexistinguserid' + + # when unauthenticated: + get :show, :id => nonid + assert_access_denied(true, false) + + # when authenticated but not admin: + login + get :show, :id => nonid + assert_access_denied + + # when authenticated as admin: + login :is_admin? => true + get :show, :id => nonid + assert_response :redirect + assert_equal({:alert => "No such user."}, flash.to_hash) + assert_redirected_to users_path + end test "should create new user" do user_attribs = record_attributes_for :user |