Fixed that user with "Manage public queries" permission, can create global public query (#19842).

git-svn-id: http://svn.redmine.org/redmine/trunk@14388 e93f8b46-1217-0410-a6f0-8f06a7374b81

5 files changed, 105 insertions, 24 deletions

diff --git a/app/controllers/queries_controller.rb b/app/controllers/queries_controller.rb
index ac91b1a15..e09790b94 100644
--- a/app/controllers/queries_controller.rb
+++ b/app/controllers/queries_controller.rb
@@ -48,17 +48,14 @@ class QueriesController < ApplicationController
     @query = IssueQuery.new
     @query.user = User.current
     @query.project = @project
-    @query.visibility = IssueQuery::VISIBILITY_PRIVATE unless User.current.allowed_to?(:manage_public_queries, @project) || User.current.admin?
     @query.build_from_params(params)
   end
 
   def create
-    @query = IssueQuery.new(params[:query])
+    @query = IssueQuery.new
     @query.user = User.current
-    @query.project = params[:query_is_for_all] ? nil : @project
-    @query.visibility = IssueQuery::VISIBILITY_PRIVATE unless User.current.allowed_to?(:manage_public_queries, @project) || User.current.admin?
-    @query.build_from_params(params)
-    @query.column_names = nil if params[:default_columns]
+    @query.project = @project
+    update_query_from_params
 
     if @query.save
       flash[:notice] = l(:notice_successful_create)
@@ -72,11 +69,7 @@ class QueriesController < ApplicationController
   end
 
   def update
-    @query.attributes = params[:query]
-    @query.project = nil if params[:query_is_for_all]
-    @query.visibility = IssueQuery::VISIBILITY_PRIVATE unless User.current.allowed_to?(:manage_public_queries, @project) || User.current.admin?
-    @query.build_from_params(params)
-    @query.column_names = nil if params[:default_columns]
+    update_query_from_params
 
     if @query.save
       flash[:notice] = l(:notice_successful_update)
@@ -107,6 +100,20 @@ private
     render_404
   end
 
+  def update_query_from_params
+    @query.project = params[:query_is_for_all] ? nil : @project
+    @query.build_from_params(params)
+    @query.column_names = nil if params[:default_columns]
+    @query.sort_criteria = params[:query] && params[:query][:sort_criteria]
+    @query.name = params[:query] && params[:query][:name]
+    if User.current.allowed_to?(:manage_public_queries, @query.project) || User.current.admin?
+      @query.visibility = (params[:query] && params[:query][:visibility]) || IssueQuery::VISIBILITY_PRIVATE
+    else
+      @query.visibility = IssueQuery::VISIBILITY_PRIVATE
+    end
+    @query
+  end
+
   def redirect_to_issues(options)
     if params[:gantt]
       if @project
diff --git a/app/models/query.rb b/app/models/query.rb
index cac43a4e4..b77f76c2e 100644
--- a/app/models/query.rb
+++ b/app/models/query.rb
@@ -487,7 +487,9 @@ class Query < ActiveRecord::Base
     if arg.is_a?(Hash)
       arg = arg.keys.sort.collect {|k| arg[k]}
     end
-    c = arg.select {|k,o| !k.to_s.blank?}.slice(0,3).collect {|k,o| [k.to_s, (o == 'desc' || o == false) ? 'desc' : 'asc']}
+    if arg
+      c = arg.select {|k,o| !k.to_s.blank?}.slice(0,3).collect {|k,o| [k.to_s, (o == 'desc' || o == false) ? 'desc' : 'asc']}
+    end
     write_attribute(:sort_criteria, c)
   end
 
diff --git a/app/views/queries/_form.html.erb b/app/views/queries/_form.html.erb
index fc03b857d..753eaedab 100644
--- a/app/views/queries/_form.html.erb
+++ b/app/views/queries/_form.html.erb
@@ -7,21 +7,20 @@
 <p><label for="query_name"><%=l(:field_name)%></label>
 <%= text_field 'query', 'name', :size => 80 %></p>
 
-<% if User.current.admin? || User.current.allowed_to?(:manage_public_queries, @project) %>
+<% if User.current.admin? || User.current.allowed_to?(:manage_public_queries, @query.project) %>
 <p><label><%=l(:field_visible)%></label>
   <label class="block"><%= radio_button 'query', 'visibility', Query::VISIBILITY_PRIVATE %> <%= l(:label_visibility_private) %></label>
+  <label class="block"><%= radio_button 'query', 'visibility', Query::VISIBILITY_PUBLIC %> <%= l(:label_visibility_public) %></label>
   <label class="block"><%= radio_button 'query', 'visibility', Query::VISIBILITY_ROLES %> <%= l(:label_visibility_roles) %>:</label>
   <% Role.givable.sorted.each do |role| %>
     <label class="block role-visibility"><%= check_box_tag 'query[role_ids][]', role.id, @query.roles.include?(role), :id => nil %> <%= role.name %></label>
   <% end %>
-  <label class="block"><%= radio_button 'query', 'visibility', Query::VISIBILITY_PUBLIC %> <%= l(:label_visibility_public) %></label>
   <%= hidden_field_tag 'query[role_ids][]', '' %>
 </p>
 <% end %>
 
 <p><label for="query_is_for_all"><%=l(:field_is_for_all)%></label>
-<%= check_box_tag 'query_is_for_all', 1, @query.project.nil?,
-      :disabled => (!@query.new_record? && (@query.project.nil? || (@query.is_public? && !User.current.admin?))) %></p>
+<%= check_box_tag 'query_is_for_all', 1, @query.project.nil?, :class => (User.current.admin? ? '' : 'disable-unless-private') %></p>
 
 <% unless params[:gantt] %>
 <fieldset><legend><%= l(:label_options) %></legend>
@@ -80,8 +79,11 @@
 <%= javascript_tag do %>
 $(document).ready(function(){
   $("input[name='query[visibility]']").change(function(){
-    var checked = $('#query_visibility_1').is(':checked');
-    $("input[name='query[role_ids][]'][type=checkbox]").attr('disabled', !checked);
+    var roles_checked = $('#query_visibility_1').is(':checked');
+    var private_checked = $('#query_visibility_0').is(':checked');
+    $("input[name='query[role_ids][]'][type=checkbox]").attr('disabled', !roles_checked);
+    if (!private_checked) $("input.disable-unless-private").attr('checked', false);
+    $("input.disable-unless-private").attr('disabled', !private_checked);
   }).trigger('change');
 });
 <% end %>
diff --git a/test/functional/queries_controller_test.rb b/test/functional/queries_controller_test.rb
index ab2321bc5..dd93de4bf 100644
--- a/test/functional/queries_controller_test.rb
+++ b/test/functional/queries_controller_test.rb
@@ -83,7 +83,7 @@ class QueriesControllerTest < ActionController::TestCase
          :fields => ["status_id", "assigned_to_id"],
          :operators => {"assigned_to_id" => "=", "status_id" => "o"},
          :values => { "assigned_to_id" => ["1"], "status_id" => ["1"]},
-         :query => {"name" => "test_new_project_private_query", "visibility" => "2"}
+         :query => {"name" => "test_new_project_private_query", "visibility" => "0"}
 
     q = Query.find_by_name('test_new_project_private_query')
     assert_redirected_to :controller => 'issues', :action => 'index', :project_id => 'ecookbook', :query_id => q
@@ -98,7 +98,7 @@ class QueriesControllerTest < ActionController::TestCase
          :fields => ["status_id", "assigned_to_id"],
          :operators => {"assigned_to_id" => "=", "status_id" => "o"},
          :values => { "assigned_to_id" => ["me"], "status_id" => ["1"]},
-         :query => {"name" => "test_new_global_private_query", "visibility" => "2"},
+         :query => {"name" => "test_new_global_private_query", "visibility" => "0"},
          :c => ["", "tracker", "subject", "priority", "category"]
 
     q = Query.find_by_name('test_new_global_private_query')
@@ -119,6 +119,7 @@ class QueriesControllerTest < ActionController::TestCase
 
     q = Query.find_by_name('test_new_global_query')
     assert_redirected_to :controller => 'issues', :action => 'index', :project_id => nil, :query_id => q
+    assert !q.is_public?
     assert !q.has_filter?(:status_id)
     assert_equal ['assigned_to_id'], q.filters.keys
     assert q.valid?
@@ -186,13 +187,73 @@ class QueriesControllerTest < ActionController::TestCase
     assert_equal false, query.draw_progress_line
   end
 
+  def test_create_project_public_query_should_force_private_without_manage_public_queries_permission
+    @request.session[:user_id] = 3
+    query = new_record(Query) do
+      post :create,
+           :project_id => 'ecookbook',
+           :query => {"name" => "name", "visibility" => "2"}
+      assert_response 302
+    end
+    assert_not_nil query.project
+    assert_equal Query::VISIBILITY_PRIVATE, query.visibility
+  end
+
+  def test_create_global_public_query_should_force_private_without_manage_public_queries_permission
+    @request.session[:user_id] = 3
+    query = new_record(Query) do
+      post :create,
+           :project_id => 'ecookbook', :query_is_for_all => '1',
+           :query => {"name" => "name", "visibility" => "2"}
+      assert_response 302
+    end
+    assert_nil query.project
+    assert_equal Query::VISIBILITY_PRIVATE, query.visibility
+  end
+
+  def test_create_project_public_query_with_manage_public_queries_permission
+    @request.session[:user_id] = 2
+    query = new_record(Query) do
+      post :create,
+           :project_id => 'ecookbook',
+           :query => {"name" => "name", "visibility" => "2"}
+      assert_response 302
+    end
+    assert_not_nil query.project
+    assert_equal Query::VISIBILITY_PUBLIC, query.visibility
+  end
+
+  def test_create_global_public_query_should_force_private_with_manage_public_queries_permission
+    @request.session[:user_id] = 2
+    query = new_record(Query) do
+      post :create,
+           :project_id => 'ecookbook', :query_is_for_all => '1',
+           :query => {"name" => "name", "visibility" => "2"}
+      assert_response 302
+    end
+    assert_nil query.project
+    assert_equal Query::VISIBILITY_PRIVATE, query.visibility
+  end
+
+  def test_create_global_public_query_by_admin
+    @request.session[:user_id] = 1
+    query = new_record(Query) do
+      post :create,
+           :project_id => 'ecookbook', :query_is_for_all => '1',
+           :query => {"name" => "name", "visibility" => "2"}
+      assert_response 302
+    end
+    assert_nil query.project
+    assert_equal Query::VISIBILITY_PUBLIC, query.visibility
+  end
+
   def test_edit_global_public_query
     @request.session[:user_id] = 1
     get :edit, :id => 4
     assert_response :success
     assert_template 'edit'
     assert_select 'input[name=?][value="2"][checked=checked]', 'query[visibility]'
-    assert_select 'input[name=query_is_for_all][type=checkbox][checked=checked][disabled=disabled]'
+    assert_select 'input[name=query_is_for_all][type=checkbox][checked=checked]'
   end
 
   def test_edit_global_private_query
@@ -201,7 +262,7 @@ class QueriesControllerTest < ActionController::TestCase
     assert_response :success
     assert_template 'edit'
     assert_select 'input[name=?]', 'query[visibility]', 0
-    assert_select 'input[name=query_is_for_all][type=checkbox][checked=checked][disabled=disabled]'
+    assert_select 'input[name=query_is_for_all][type=checkbox][checked=checked]'
   end
 
   def test_edit_project_private_query
@@ -210,7 +271,7 @@ class QueriesControllerTest < ActionController::TestCase
     assert_response :success
     assert_template 'edit'
     assert_select 'input[name=?]', 'query[visibility]', 0
-    assert_select 'input[name=query_is_for_all][type=checkbox]:not([checked]):not([disabled])'
+    assert_select 'input[name=query_is_for_all][type=checkbox]:not([checked])'
   end
 
   def test_edit_project_public_query
@@ -219,7 +280,7 @@ class QueriesControllerTest < ActionController::TestCase
     assert_response :success
     assert_template 'edit'
     assert_select 'input[name=?][value="2"][checked=checked]', 'query[visibility]'
-    assert_select 'input[name=query_is_for_all][type=checkbox][disabled=disabled]:not([checked])'
+    assert_select 'input[name=query_is_for_all][type=checkbox]:not([checked])'
   end
 
   def test_edit_sort_criteria
diff --git a/test/test_helper.rb b/test/test_helper.rb
index be4e68e18..73987a592 100644
--- a/test/test_helper.rb
+++ b/test/test_helper.rb
@@ -180,6 +180,15 @@ class ActiveSupport::TestCase
     ActiveRecord::Base.connection.quoted_date(date)
   end
 
+	# Asserts that a new record for the given class is created
+	# and returns it
+  def new_record(klass, &block)
+    assert_difference "#{klass}.count" do
+      yield
+    end
+    klass.order(:id => :desc).first
+  end
+
   def assert_save(object)
     saved = object.save
     message = "#{object.class} could not be saved"