Merge pull request #174 from azul/bugfix/admin-navigates-all-tickets

Admin navigates all tickets - fixes #5879

8 files changed, 40 insertions, 39 deletions

diff --git a/engines/billing/test/test_helper.rb b/engines/billing/test/test_helper.rb
index 7ad3869..57cdd63 100644
--- a/engines/billing/test/test_helper.rb
+++ b/engines/billing/test/test_helper.rb
@@ -1,7 +1,6 @@
 # Configure Rails Environment
 ENV["RAILS_ENV"] = "test"
 
-require File.expand_path("../../../../dummy/config/environment.rb",  __FILE__)
 require "rails/test_help"
 
 Rails.backtrace_cleaner.remove_silencers!
diff --git a/engines/support/app/controllers/tickets_controller.rb b/engines/support/app/controllers/tickets_controller.rb
index 1ccbd16..602bbd9 100644
--- a/engines/support/app/controllers/tickets_controller.rb
+++ b/engines/support/app/controllers/tickets_controller.rb
@@ -4,10 +4,10 @@ class TicketsController < ApplicationController
   respond_to :html, :json
   #has_scope :open, :type => boolean
 
-  before_filter :fetch_user
   before_filter :require_login, :only => [:index]
   before_filter :fetch_ticket, except: [:new, :create, :index]
-  before_filter :require_ticket_access, except: [:new, :create]
+  before_filter :require_ticket_access, except: [:new, :create, :index]
+  before_filter :fetch_user
   before_filter :set_title
 
   def new
@@ -129,22 +129,14 @@ class TicketsController < ApplicationController
   end
 
   def ticket_access?
-    admin? or (
-      @ticket &&
-      @ticket.created_by.blank?
-    ) or (
-      @ticket &&
-      @ticket.created_by == current_user.id
-    ) or (
-      @ticket.nil? &&
-      @user &&
-      @user.id == current_user.id
-    )
+    admin? or
+      @ticket.created_by.blank? or
+      current_user.id == @ticket.created_by
   end
 
   def fetch_user
-    if params[:user_id]
-      @user = User.find(params[:user_id])
+    if admin?
+      @user = User.find(params[:user_id]) if params[:user_id]
     else
       @user = current_user
     end
@@ -156,7 +148,7 @@ class TicketsController < ApplicationController
   def search_options(params)
     params.merge(
       :admin_status => params[:user_id] ? 'mine' : 'all',
-      :user_id      => @user.id,
+      :user_id      => @user ? @user.id : current_user.id,
       :is_admin     => admin?
     )
   end
diff --git a/engines/support/app/helpers/auto_tickets_path_helper.rb b/engines/support/app/helpers/auto_tickets_path_helper.rb
index 5638222..bc98a0a 100644
--- a/engines/support/app/helpers/auto_tickets_path_helper.rb
+++ b/engines/support/app/helpers/auto_tickets_path_helper.rb
@@ -15,7 +15,7 @@ module AutoTicketsPathHelper
   def auto_tickets_path(options={})
     return unless options.class == Hash
     options = ticket_view_options.merge options
-    if @user
+    if @user.is_a? User
       user_tickets_path(@user, options)
     else
       tickets_path(options)
@@ -25,7 +25,7 @@ module AutoTicketsPathHelper
   def auto_ticket_path(ticket, options={})
     return unless ticket.persisted?
     options = ticket_view_options.merge options
-    if @user
+    if @user.is_a? User
       user_ticket_path(@user, ticket, options)
     else
       ticket_path(ticket, options)
@@ -45,10 +45,8 @@ module AutoTicketsPathHelper
   private
 
   def ticket_view_options
-    hsh = {}
-    hsh[:open_status] = params[:open_status] if params[:open_status] && !params[:open_status].empty?
-    hsh[:sort_order]  = params[:sort_order]  if params[:sort_order]  && !params[:sort_order].empty?
-    hsh
+    hash = params.slice(:open_status, :sort_order)
+    hash.reject {|k,v| v.blank?}
   end
 
 end
diff --git a/engines/support/app/helpers/tickets_helper.rb b/engines/support/app/helpers/tickets_helper.rb
index 7db31dc..58b67ea 100644
--- a/engines/support/app/helpers/tickets_helper.rb
+++ b/engines/support/app/helpers/tickets_helper.rb
@@ -36,7 +36,8 @@ module TicketsHelper
 
   def link_to_status(new_status)
     label = ".#{new_status}"
-    link_to_navigation label, auto_tickets_path(open_status: new_status, sort_order: search_order)
+    link_to_navigation label, auto_tickets_path(open_status: new_status),
+      active: search_status == new_status
   end
 
   def link_to_order(order_field)
@@ -45,11 +46,12 @@ module TicketsHelper
     # for not-currently-filtered field link to descending direction
     direction ||= 'desc'
     label = ".#{order_field}"
-    link_to_navigation label, auto_tickets_path(sort_order: order_field + '_at_' + direction, open_status: search_status),
+    link_to_navigation label,
+      auto_tickets_path(sort_order: order_field + '_at_' + direction),
+      active: search_order.start_with?(order_field),
       icon: icon
   end
 
-
   def new_direction_for_order(order_field)
     # return if we're not filtering by this field
     return unless search_order.start_with?(order_field)
diff --git a/engines/support/app/views/tickets/_tabs.html.haml b/engines/support/app/views/tickets/_tabs.html.haml
index 7872bb5..a7347e1 100644
--- a/engines/support/app/views/tickets/_tabs.html.haml
+++ b/engines/support/app/views/tickets/_tabs.html.haml
@@ -4,8 +4,7 @@
 - unless action?(:new) or action?(:create)
   %ul.nav.nav-pills.pull-right.slim
     - %w(created updated).each do |order|
-      %li{:class=> ("active" if search_order.start_with? order)}
-        = link_to_order(order)
+      = link_to_order(order)
 
 -#
 -# STATUS FILTER TABS
@@ -13,7 +12,7 @@
 %ul.nav.nav-tabs
   - if logged_in?
     - %w(open closed all).each do |status|
-      %li{:class => ("active" if search_status == status)}
-        = link_to_status status
-  %li{:class => ("active" if action?(:new) || action?(:create))}
-    = link_to icon(:plus, :black) + t(".new", cascade: true), auto_new_ticket_path
+      = link_to_status status
+  = link_to_navigation ".new", auto_new_ticket_path,
+    active: action?(:new) || action?(:create),
+    icon: :plus
diff --git a/engines/support/test/functional/tickets_controller_test.rb b/engines/support/test/functional/tickets_controller_test.rb
index ebaa3a4..e36f5f6 100644
--- a/engines/support/test/functional/tickets_controller_test.rb
+++ b/engines/support/test/functional/tickets_controller_test.rb
@@ -64,11 +64,12 @@ class TicketsControllerTest < ActionController::TestCase
     assert_access_denied
   end
 
-  test "ticket list of other user is not visible" do
+  test "normal user only gets own ticket list" do
     other_user = find_record :user
     login
     get :index, :user_id => other_user.id
-    assert_access_denied
+    assert_equal @current_user, assigns(:user)
+    assert_nil assigns(:tickets).detect{|t| t.created_by != @user}
   end
 
   test "should create unauthenticated ticket" do
diff --git a/engines/support/test/integration/navigation_test.rb b/engines/support/test/integration/navigation_test.rb
index eec8c0e..13d51b6 100644
--- a/engines/support/test/integration/navigation_test.rb
+++ b/engines/support/test/integration/navigation_test.rb
@@ -1,9 +1,20 @@
 require 'test_helper'
 
-class NavigationTest < ActionDispatch::IntegrationTest
+class NavigationTest < BrowserIntegrationTest
 
-  # test "the truth" do
-  #   assert true
-  # end
+  #
+  # this is a regression test for #5879
+  #
+  test "admin can navigate all tickets" do
+    login
+    with_config admins: [@user.login] do
+      visit '/'
+      click_on 'Tickets'
+      click_on 'Created at'
+      uri = URI.parse(current_url)
+      assert_equal '/tickets', uri.path
+      assert_equal 'sort_order=created_at_desc', uri.query
+    end
+  end
 end
 
diff --git a/engines/support/test/test_helper.rb b/engines/support/test/test_helper.rb
index fff9173..57cdd63 100644
--- a/engines/support/test/test_helper.rb
+++ b/engines/support/test/test_helper.rb
@@ -1,7 +1,6 @@
 # Configure Rails Environment
 ENV["RAILS_ENV"] = "test"
 
-require File.expand_path('../../../../test/dummy/config/environment', __FILE__)
 require "rails/test_help"
 
 Rails.backtrace_cleaner.remove_silencers!