From 8f96f0c40d8032e4787f2548e0f0fd7da7e36a07 Mon Sep 17 00:00:00 2001 From: Etienne Massip Date: Mon, 24 Oct 2011 20:19:26 +0000 Subject: [PATCH] Refactor : convert queries to REST resources (also fixes #9108). git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@7649 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/queries_controller.rb | 58 +++++++++++++--------- app/helpers/queries_helper.rb | 43 ++++++++-------- app/views/issues/index.html.erb | 6 +-- app/views/news/new.html.erb | 16 +++--- app/views/queries/_form.html.erb | 1 - app/views/queries/edit.html.erb | 2 +- app/views/queries/index.html.erb | 6 +-- app/views/queries/new.html.erb | 2 +- config/routes.rb | 4 +- test/functional/queries_controller_test.rb | 35 ++++++++----- test/integration/routing_test.rb | 13 ++++- 11 files changed, 107 insertions(+), 79 deletions(-) diff --git a/app/controllers/queries_controller.rb b/app/controllers/queries_controller.rb index 0ec305e4b..241b5176c 100644 --- a/app/controllers/queries_controller.rb +++ b/app/controllers/queries_controller.rb @@ -17,11 +17,13 @@ class QueriesController < ApplicationController menu_item :issues - before_filter :find_query, :except => [:new, :index] - before_filter :find_optional_project, :only => :new + before_filter :find_query, :except => [:new, :create, :index] + before_filter :find_optional_project, :only => [:new, :create] accept_api_auth :index + include QueriesHelper + def index case params[:format] when 'xml', 'json' @@ -41,44 +43,52 @@ class QueriesController < ApplicationController end def new - @query = Query.new(params[:query]) - @query.project = params[:query_is_for_all] ? nil : @project + @query = Query.new @query.user = User.current + @query.project = @project @query.is_public = false unless User.current.allowed_to?(:manage_public_queries, @project) || User.current.admin? + build_query_from_params + end - @query.add_filters(params[:fields] || params[:f], params[:operators] || params[:op], params[:values] || params[:v]) if params[:fields] || params[:f] - @query.group_by ||= params[:group_by] - @query.column_names = params[:c] if params[:c] + verify :method => :post, :only => :create, :render => {:nothing => true, :status => :method_not_allowed } + def create + @query = Query.new(params[:query]) + @query.user = User.current + @query.project = params[:query_is_for_all] ? nil : @project + @query.is_public = false unless User.current.allowed_to?(:manage_public_queries, @project) || User.current.admin? + build_query_from_params @query.column_names = nil if params[:default_columns] - if request.post? && params[:confirm] && @query.save + if @query.save flash[:notice] = l(:notice_successful_create) redirect_to :controller => 'issues', :action => 'index', :project_id => @project, :query_id => @query - return + else + render :action => 'new', :layout => !request.xhr? end - render :layout => false if request.xhr? end def edit - if request.post? - @query.filters = {} - @query.add_filters(params[:fields] || params[:f], params[:operators] || params[:op], params[:values] || params[:v]) if params[:fields] || params[:f] - @query.attributes = params[:query] - @query.project = nil if params[:query_is_for_all] - @query.is_public = false unless User.current.allowed_to?(:manage_public_queries, @project) || User.current.admin? - @query.group_by ||= params[:group_by] - @query.column_names = params[:c] if params[:c] - @query.column_names = nil if params[:default_columns] + end - if @query.save - flash[:notice] = l(:notice_successful_update) - redirect_to :controller => 'issues', :action => 'index', :project_id => @project, :query_id => @query - end + verify :method => :put, :only => :update, :render => {:nothing => true, :status => :method_not_allowed } + def update + @query.attributes = params[:query] + @query.project = nil if params[:query_is_for_all] + @query.is_public = false unless User.current.allowed_to?(:manage_public_queries, @project) || User.current.admin? + build_query_from_params + @query.column_names = nil if params[:default_columns] + + if @query.save + flash[:notice] = l(:notice_successful_update) + redirect_to :controller => 'issues', :action => 'index', :project_id => @project, :query_id => @query + else + render :action => 'edit' end end + verify :method => :delete, :only => :destroy, :render => {:nothing => true, :status => :method_not_allowed } def destroy - @query.destroy if request.post? + @query.destroy redirect_to :controller => 'issues', :action => 'index', :project_id => @project, :set_filter => 1 end diff --git a/app/helpers/queries_helper.rb b/app/helpers/queries_helper.rb index 2ce6a96d8..0e82eb631 100644 --- a/app/helpers/queries_helper.rb +++ b/app/helpers/queries_helper.rb @@ -71,30 +71,31 @@ module QueriesHelper cond << " OR project_id = #{@project.id}" if @project @query = Query.find(params[:query_id], :conditions => cond) raise ::Unauthorized unless @query.visible? - @query.project = @project session[:query] = {:id => @query.id, :project_id => @query.project_id} sort_clear + elsif api_request? || params[:set_filter] || session[:query].nil? || session[:query][:project_id] != (@project ? @project.id : nil) + # Give it a name, required to be valid + @query = Query.new(:name => "_") + @query.project = @project + build_query_from_params + session[:query] = {:project_id => @query.project_id, :filters => @query.filters, :group_by => @query.group_by, :column_names => @query.column_names} else - if api_request? || params[:set_filter] || session[:query].nil? || session[:query][:project_id] != (@project ? @project.id : nil) - # Give it a name, required to be valid - @query = Query.new(:name => "_") - @query.project = @project - if params[:fields] || params[:f] - @query.filters = {} - @query.add_filters(params[:fields] || params[:f], params[:operators] || params[:op], params[:values] || params[:v]) - else - @query.available_filters.keys.each do |field| - @query.add_short_filter(field, params[field]) if params[field] - end - end - @query.group_by = params[:group_by] - @query.column_names = params[:c] || (params[:query] && params[:query][:column_names]) - session[:query] = {:project_id => @query.project_id, :filters => @query.filters, :group_by => @query.group_by, :column_names => @query.column_names} - else - @query = Query.find_by_id(session[:query][:id]) if session[:query][:id] - @query ||= Query.new(:name => "_", :project => @project, :filters => session[:query][:filters], :group_by => session[:query][:group_by], :column_names => session[:query][:column_names]) - @query.project = @project - end + # retrieve from session + @query = Query.find_by_id(session[:query][:id]) if session[:query][:id] + @query ||= Query.new(:name => "_", :project_id => session[:project_id] || @project, :filters => session[:query][:filters], :group_by => session[:query][:group_by], :column_names => session[:query][:column_names]) end end + + def build_query_from_params + if params[:fields] || params[:f] + @query.filters = {} + @query.add_filters(params[:fields] || params[:f], params[:operators] || params[:op], params[:values] || params[:v]) + else + @query.available_filters.keys.each do |field| + @query.add_short_filter(field, params[field]) if params[field] + end + end + @query.group_by = params[:group_by] || (params[:query] && params[:query][:group_by]) + @query.column_names = params[:c] || (params[:query] && params[:query][:column_names]) + end end diff --git a/app/views/issues/index.html.erb b/app/views/issues/index.html.erb index 4c2caa7ff..beff2fef6 100644 --- a/app/views/issues/index.html.erb +++ b/app/views/issues/index.html.erb @@ -1,7 +1,7 @@
<% if !@query.new_record? && @query.editable_by?(User.current) %> - <%= link_to l(:button_edit), {:controller => 'queries', :action => 'edit', :id => @query}, :class => 'icon icon-edit' %> - <%= link_to l(:button_delete), {:controller => 'queries', :action => 'destroy', :id => @query}, :confirm => l(:text_are_you_sure), :method => :post, :class => 'icon icon-del' %> + <%= link_to l(:button_edit), @query.project_id ? edit_project_query_path(:project_id => @query.project, :id => @query) : edit_query_path(@query), :class => 'icon icon-edit' %> + <%= link_to l(:button_delete), @query.project_id ? project_query_path(:project_id => @query.project, :id => @query) : query_path(@query), :confirm => l(:text_are_you_sure), :method => :delete, :class => 'icon icon-del' %> <% end %>
@@ -38,7 +38,7 @@ <%= link_to_function l(:button_apply), 'submit_query_form("query_form")', :class => 'icon icon-checked' %> <%= link_to l(:button_clear), { :set_filter => 1, :project_id => @project }, :class => 'icon icon-reload' %> <% if @query.new_record? && User.current.allowed_to?(:save_queries, @project, :global => true) %> - <%= link_to_function l(:button_save), "$('query_form').action='#{ url_for :controller => 'queries', :action => 'new', :project_id => @project }'; submit_query_form('query_form')", :class => 'icon icon-save' %> + <%= link_to_function l(:button_save), "$('query_form').action='#{ @project ? new_project_query_path(@project) : new_query_path }'; submit_query_form('query_form')", :class => 'icon icon-save' %> <% end %>

<% end %> diff --git a/app/views/news/new.html.erb b/app/views/news/new.html.erb index 0fbc72ec1..59589030a 100644 --- a/app/views/news/new.html.erb +++ b/app/views/news/new.html.erb @@ -2,13 +2,13 @@ <% labelled_tabular_form_for :news, @news, :url => project_news_index_path(@project), :html => { :id => 'news-form' } do |f| %> -<%= render :partial => 'news/form', :locals => { :f => f } %> -<%= submit_tag l(:button_create) %> -<%= link_to_remote l(:label_preview), - { :url => preview_news_path(:project_id => @project), - :method => 'get', - :update => 'preview', - :with => "Form.serialize('news-form')" - }, :accesskey => accesskey(:preview) %> + <%= render :partial => 'news/form', :locals => { :f => f } %> + <%= submit_tag l(:button_create) %> + <%= link_to_remote l(:label_preview), + { :url => preview_news_path(:project_id => @project), + :method => 'get', + :update => 'preview', + :with => "Form.serialize('news-form')" + }, :accesskey => accesskey(:preview) %> <% end %>
diff --git a/app/views/queries/_form.html.erb b/app/views/queries/_form.html.erb index 2d9d81d28..806944359 100644 --- a/app/views/queries/_form.html.erb +++ b/app/views/queries/_form.html.erb @@ -1,5 +1,4 @@ <%= error_messages_for 'query' %> -<%= hidden_field_tag 'confirm', 1 %>
diff --git a/app/views/queries/edit.html.erb b/app/views/queries/edit.html.erb index 1c99ac077..46a530f16 100644 --- a/app/views/queries/edit.html.erb +++ b/app/views/queries/edit.html.erb @@ -1,6 +1,6 @@

<%= l(:label_query) %>

-<% form_tag({:action => 'edit', :id => @query}, :onsubmit => 'selectAllOptions("selected_columns");') do %> +<% form_tag(@query.project_id ? project_query_path(:project_id => @query.project, :id => @query) : query_path(@query), :onsubmit => 'selectAllOptions("selected_columns");', :method => :put) do %> <%= render :partial => 'form', :locals => {:query => @query} %> <%= submit_tag l(:button_save) %> <% end %> diff --git a/app/views/queries/index.html.erb b/app/views/queries/index.html.erb index 066f30edb..b03ac6216 100644 --- a/app/views/queries/index.html.erb +++ b/app/views/queries/index.html.erb @@ -1,5 +1,5 @@
-<%= link_to_if_authorized l(:label_query_new), {:controller => 'queries', :action => 'new', :project_id => @project}, :class => 'icon icon-add' %> +<%= link_to_if_authorized l(:label_query_new), new_project_query_path(:project_id => @project), :class => 'icon icon-add' %>

<%= l(:label_query_plural) %>

@@ -16,8 +16,8 @@ <% if query.editable_by?(User.current) %> - <%= link_to l(:button_edit), {:controller => 'queries', :action => 'edit', :id => query}, :class => 'icon icon-edit' %> - <%= link_to l(:button_delete), {:controller => 'queries', :action => 'destroy', :id => query}, :confirm => l(:text_are_you_sure), :method => :post, :class => 'icon icon-del' %> + <%= link_to l(:button_edit), edit_query_path(query), :class => 'icon icon-edit' %> + <%= link_to l(:button_delete), query_path(query), :confirm => l(:text_are_you_sure), :method => :delete, :class => 'icon icon-del' %> <% end %> diff --git a/app/views/queries/new.html.erb b/app/views/queries/new.html.erb index a980d04ab..b5809fb88 100644 --- a/app/views/queries/new.html.erb +++ b/app/views/queries/new.html.erb @@ -1,6 +1,6 @@

<%= l(:label_query_new) %>

-<% form_tag({:action => 'new', :project_id => @query.project}, :onsubmit => 'selectAllOptions("selected_columns");') do %> +<% form_tag(@project ? project_queries_path(:project_id => @project) : queries_path, :onsubmit => 'selectAllOptions("selected_columns");') do %> <%= render :partial => 'form', :locals => {:query => @query} %> <%= submit_tag l(:button_save) %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index c07004e69..8f0f39e48 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -77,7 +77,7 @@ ActionController::Routing::Routes.draw do |map| end map.resources :issue_moves, :only => [:new, :create], :path_prefix => '/issues', :as => 'move' - map.resources :queries, :only => [:index] + map.resources :queries, :except => [:show] # Misc issue routes. TODO: move into resources map.auto_complete_issues '/issues/auto_complete', :controller => 'auto_completes', :action => 'issues' @@ -156,6 +156,7 @@ ActionController::Routing::Routes.draw do |map| project.resources :versions, :shallow => true, :collection => {:close_completed => :put}, :member => {:status_by => :post} project.resources :news, :shallow => true project.resources :time_entries, :controller => 'timelog', :path_prefix => 'projects/:project_id' + project.resources :queries, :except => [:show] project.wiki_start_page 'wiki', :controller => 'wiki', :action => 'show', :conditions => {:method => :get} project.wiki_index 'wiki/index', :controller => 'wiki', :action => 'index', :conditions => {:method => :get} @@ -226,7 +227,6 @@ ActionController::Routing::Routes.draw do |map| map.resources :groups #left old routes at the bottom for backwards compat - map.connect 'projects/:project_id/queries/:action', :controller => 'queries' map.connect 'projects/:project_id/issues/:action', :controller => 'issues' map.connect 'projects/:project_id/documents/:action', :controller => 'documents' map.connect 'projects/:project_id/boards/:action/:id', :controller => 'boards' diff --git a/test/functional/queries_controller_test.rb b/test/functional/queries_controller_test.rb index b40ea6e7c..dfa5d6153 100644 --- a/test/functional/queries_controller_test.rb +++ b/test/functional/queries_controller_test.rb @@ -60,9 +60,8 @@ class QueriesControllerTest < ActionController::TestCase def test_new_project_public_query @request.session[:user_id] = 2 - post :new, + post :create, :project_id => 'ecookbook', - :confirm => '1', :default_columns => '1', :f => ["status_id", "assigned_to_id"], :op => {"assigned_to_id" => "=", "status_id" => "o"}, @@ -78,9 +77,8 @@ class QueriesControllerTest < ActionController::TestCase def test_new_project_private_query @request.session[:user_id] = 3 - post :new, + post :create, :project_id => 'ecookbook', - :confirm => '1', :default_columns => '1', :fields => ["status_id", "assigned_to_id"], :operators => {"assigned_to_id" => "=", "status_id" => "o"}, @@ -96,8 +94,7 @@ class QueriesControllerTest < ActionController::TestCase def test_new_global_private_query_with_custom_columns @request.session[:user_id] = 3 - post :new, - :confirm => '1', + post :create, :fields => ["status_id", "assigned_to_id"], :operators => {"assigned_to_id" => "=", "status_id" => "o"}, :values => { "assigned_to_id" => ["me"], "status_id" => ["1"]}, @@ -112,10 +109,24 @@ class QueriesControllerTest < ActionController::TestCase assert q.valid? end + def test_new_global_query_with_custom_filters + @request.session[:user_id] = 3 + post :create, + :fields => ["assigned_to_id"], + :operators => {"assigned_to_id" => "="}, + :values => { "assigned_to_id" => ["me"]}, + :query => {"name" => "test_new_global_query"} + + q = Query.find_by_name('test_new_global_query') + assert_redirected_to :controller => 'issues', :action => 'index', :project_id => nil, :query_id => q + assert !q.has_filter?(:status_id) + assert_equal ['assigned_to_id'], q.filters.keys + assert q.valid? + end + def test_new_with_sort @request.session[:user_id] = 1 - post :new, - :confirm => '1', + post :create, :default_columns => '1', :operators => {"status_id" => "o"}, :values => {"status_id" => ["1"]}, @@ -144,9 +155,8 @@ class QueriesControllerTest < ActionController::TestCase def test_edit_global_public_query @request.session[:user_id] = 1 - post :edit, + put :update, :id => 4, - :confirm => '1', :default_columns => '1', :fields => ["status_id", "assigned_to_id"], :operators => {"assigned_to_id" => "=", "status_id" => "o"}, @@ -175,9 +185,8 @@ class QueriesControllerTest < ActionController::TestCase def test_edit_global_private_query @request.session[:user_id] = 3 - post :edit, + put :update, :id => 3, - :confirm => '1', :default_columns => '1', :fields => ["status_id", "assigned_to_id"], :operators => {"assigned_to_id" => "=", "status_id" => "o"}, @@ -234,7 +243,7 @@ class QueriesControllerTest < ActionController::TestCase def test_destroy @request.session[:user_id] = 2 - post :destroy, :id => 1 + delete :destroy, :id => 1 assert_redirected_to :controller => 'issues', :action => 'index', :project_id => 'ecookbook', :set_filter => 1, :query_id => nil assert_nil Query.find_by_id(1) end diff --git a/test/integration/routing_test.rb b/test/integration/routing_test.rb index 4591db053..cfdc5389e 100644 --- a/test/integration/routing_test.rb +++ b/test/integration/routing_test.rb @@ -218,8 +218,17 @@ class RoutingTest < ActionController::IntegrationTest should_route :get, "/queries/new", :controller => 'queries', :action => 'new' should_route :get, "/projects/redmine/queries/new", :controller => 'queries', :action => 'new', :project_id => 'redmine' - should_route :post, "/queries/new", :controller => 'queries', :action => 'new' - should_route :post, "/projects/redmine/queries/new", :controller => 'queries', :action => 'new', :project_id => 'redmine' + should_route :post, "/queries", :controller => 'queries', :action => 'create' + should_route :post, "/projects/redmine/queries", :controller => 'queries', :action => 'create', :project_id => 'redmine' + + should_route :get, "/queries/1/edit", :controller => 'queries', :action => 'edit', :id => '1' + should_route :get, "/projects/redmine/queries/1/edit", :controller => 'queries', :action => 'edit', :id => '1', :project_id => 'redmine' + + should_route :put, "/queries/1", :controller => 'queries', :action => 'update', :id => '1' + should_route :put, "/projects/redmine/queries/1", :controller => 'queries', :action => 'update', :id => '1', :project_id => 'redmine' + + should_route :delete, "/queries/1", :controller => 'queries', :action => 'destroy', :id => '1' + should_route :delete, "/projects/redmine/queries/1", :controller => 'queries', :action => 'destroy', :id => '1', :project_id => 'redmine' end context "repositories" do