From 42f9dc7d2c5ac27b538ad5f33fcb132437ed5975 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Tue, 5 Jul 2011 16:47:34 +0000 Subject: [PATCH] Makes relations resource shallow (#7366). git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@6184 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/issue_relations_controller.rb | 25 +++++++++++-------- app/models/issue_relation.rb | 10 ++++++++ app/views/issue_relations/show.api.rsb | 2 +- app/views/issues/_relations.rhtml | 4 +-- config/routes.rb | 2 +- .../issue_relations_controller_test.rb | 9 +++---- .../api_test/issue_relations_test.rb | 6 ++--- test/integration/routing_test.rb | 12 ++++----- 8 files changed, 41 insertions(+), 29 deletions(-) diff --git a/app/controllers/issue_relations_controller.rb b/app/controllers/issue_relations_controller.rb index 36ee8d6d7..9a1754674 100644 --- a/app/controllers/issue_relations_controller.rb +++ b/app/controllers/issue_relations_controller.rb @@ -16,7 +16,9 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. class IssueRelationsController < ApplicationController - before_filter :find_issue, :find_project_from_association, :authorize + before_filter :find_issue, :find_project_from_association, :authorize, :only => [:index, :create] + before_filter :find_relation, :except => [:index, :create] + accept_key_auth :index, :show, :create, :destroy def index @@ -29,7 +31,7 @@ class IssueRelationsController < ApplicationController end def show - @relation = @issue.find_relation(params[:id]) + raise Unauthorized unless @relation.visible? respond_to do |format| format.html { render :nothing => true } @@ -62,7 +64,7 @@ class IssueRelationsController < ApplicationController end format.api { if saved - render :action => 'show', :status => :created, :location => issue_relation_url(@issue, @relation) + render :action => 'show', :status => :created, :location => relation_url(@relation) else render_validation_errors(@relation) end @@ -72,16 +74,13 @@ class IssueRelationsController < ApplicationController verify :method => :delete, :only => :destroy, :render => {:nothing => true, :status => :method_not_allowed } def destroy - relation = @issue.find_relation(params[:id]) - relation.destroy + raise Unauthorized unless @relation.deletable? + @relation.destroy respond_to do |format| format.html { redirect_to :controller => 'issues', :action => 'show', :id => @issue } - format.js { - @relations = @issue.reload.relations.select {|r| r.other_issue(@issue) && r.other_issue(@issue).visible? } - render(:update) {|page| page.replace_html "relations", :partial => 'issues/relations'} - } - format.api { head :ok } + format.js { render(:update) {|page| page.remove "relation-#{@relation.id}"} } + format.api { head :ok } end rescue ActiveRecord::RecordNotFound render_404 @@ -93,4 +92,10 @@ private rescue ActiveRecord::RecordNotFound render_404 end + + def find_relation + @relation = IssueRelation.find(params[:id]) + rescue ActiveRecord::RecordNotFound + render_404 + end end diff --git a/app/models/issue_relation.rb b/app/models/issue_relation.rb index 5b050c9a3..2d5086332 100644 --- a/app/models/issue_relation.rb +++ b/app/models/issue_relation.rb @@ -43,6 +43,16 @@ class IssueRelation < ActiveRecord::Base attr_protected :issue_from_id, :issue_to_id + def visible?(user=User.current) + (issue_from.nil? || issue_from.visible?(user)) && (issue_to.nil? || issue_to.visible?(user)) + end + + def deletable?(user=User.current) + visible?(user) && + ((issue_from.nil? || user.allowed_to?(:manage_issue_relations, issue_from.project)) || + (issue_to.nil? || user.allowed_to?(:manage_issue_relations, issue_to.project))) + end + def after_initialize if new_record? if relation_type.blank? diff --git a/app/views/issue_relations/show.api.rsb b/app/views/issue_relations/show.api.rsb index 0a3e2918a..bffad94ab 100644 --- a/app/views/issue_relations/show.api.rsb +++ b/app/views/issue_relations/show.api.rsb @@ -2,6 +2,6 @@ api.relation do api.id @relation.id api.issue_id @relation.issue_from_id api.issue_to_id @relation.issue_to_id - api.relation_type @relation.relation_type_for(@issue) + api.relation_type @relation.relation_type api.delay @relation.delay end diff --git a/app/views/issues/_relations.rhtml b/app/views/issues/_relations.rhtml index 065f8da9b..12a39ddc4 100644 --- a/app/views/issues/_relations.rhtml +++ b/app/views/issues/_relations.rhtml @@ -10,7 +10,7 @@
<% @relations.each do |relation| %> - + - diff --git a/config/routes.rb b/config/routes.rb index df1b62086..dfac70dc5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -110,7 +110,7 @@ ActionController::Routing::Routes.draw do |map| map.resources :issues, :member => { :edit => :post }, :collection => {} do |issues| issues.resources :time_entries, :controller => 'timelog' - issues.resources :relations, :controller => 'issue_relations', :only => [:index, :show, :create, :destroy] + issues.resources :relations, :shallow => true, :controller => 'issue_relations', :only => [:index, :show, :create, :destroy] end map.resources :issues, :path_prefix => '/projects/:project_id', :collection => { :create => :post } do |issues| diff --git a/test/functional/issue_relations_controller_test.rb b/test/functional/issue_relations_controller_test.rb index 3f1408785..46be9ba29 100644 --- a/test/functional/issue_relations_controller_test.rb +++ b/test/functional/issue_relations_controller_test.rb @@ -97,7 +97,7 @@ class IssueRelationsControllerTest < ActionController::TestCase def test_destroy assert_difference 'IssueRelation.count', -1 do @request.session[:user_id] = 3 - delete :destroy, :id => '2', :issue_id => '3' + delete :destroy, :id => '2' end end @@ -109,11 +109,8 @@ class IssueRelationsControllerTest < ActionController::TestCase assert_difference 'IssueRelation.count', -1 do @request.session[:user_id] = 3 - xhr :delete, :destroy, :id => '2', :issue_id => '3' - assert_select_rjs 'relations' do - assert_select 'table', 1 - assert_select 'tr', 1 # relation left - end + xhr :delete, :destroy, :id => '2' + assert_select_rjs :remove, 'relation-2' end end end diff --git a/test/integration/api_test/issue_relations_test.rb b/test/integration/api_test/issue_relations_test.rb index d02f63506..ab2da8731 100644 --- a/test/integration/api_test/issue_relations_test.rb +++ b/test/integration/api_test/issue_relations_test.rb @@ -73,10 +73,10 @@ class ApiTest::IssueRelationsTest < ActionController::IntegrationTest end end - context "/issues/:issue_id/relations/:id" do + context "/relations/:id" do context "GET" do should "return the relation" do - get '/issues/3/relations/2.xml', {}, :authorization => credentials('jsmith') + get '/relations/2.xml', {}, :authorization => credentials('jsmith') assert_response :success assert_equal 'application/xml', @response.content_type @@ -87,7 +87,7 @@ class ApiTest::IssueRelationsTest < ActionController::IntegrationTest context "DELETE" do should "delete the relation" do assert_difference('IssueRelation.count', -1) do - delete '/issues/3/relations/2.xml', {}, :authorization => credentials('jsmith') + delete '/relations/2.xml', {}, :authorization => credentials('jsmith') end assert_response :ok diff --git a/test/integration/routing_test.rb b/test/integration/routing_test.rb index 199dc1b27..c78f4082b 100644 --- a/test/integration/routing_test.rb +++ b/test/integration/routing_test.rb @@ -126,13 +126,13 @@ class RoutingTest < ActionController::IntegrationTest should_route :post, "/issues/1/relations.xml", :controller => 'issue_relations', :action => 'create', :issue_id => '1', :format => 'xml' should_route :post, "/issues/1/relations.json", :controller => 'issue_relations', :action => 'create', :issue_id => '1', :format => 'json' - should_route :get, "/issues/1/relations/23", :controller => 'issue_relations', :action => 'show', :issue_id => '1', :id => '23' - should_route :get, "/issues/1/relations/23.xml", :controller => 'issue_relations', :action => 'show', :issue_id => '1', :id => '23', :format => 'xml' - should_route :get, "/issues/1/relations/23.json", :controller => 'issue_relations', :action => 'show', :issue_id => '1', :id => '23', :format => 'json' + should_route :get, "/relations/23", :controller => 'issue_relations', :action => 'show', :id => '23' + should_route :get, "/relations/23.xml", :controller => 'issue_relations', :action => 'show', :id => '23', :format => 'xml' + should_route :get, "/relations/23.json", :controller => 'issue_relations', :action => 'show', :id => '23', :format => 'json' - should_route :delete, "/issues/1/relations/23", :controller => 'issue_relations', :action => 'destroy', :issue_id => '1', :id => '23' - should_route :delete, "/issues/1/relations/23.xml", :controller => 'issue_relations', :action => 'destroy', :issue_id => '1', :id => '23', :format => 'xml' - should_route :delete, "/issues/1/relations/23.json", :controller => 'issue_relations', :action => 'destroy', :issue_id => '1', :id => '23', :format => 'json' + should_route :delete, "/relations/23", :controller => 'issue_relations', :action => 'destroy', :id => '23' + should_route :delete, "/relations/23.xml", :controller => 'issue_relations', :action => 'destroy', :id => '23', :format => 'xml' + should_route :delete, "/relations/23.json", :controller => 'issue_relations', :action => 'destroy', :id => '23', :format => 'json' end context "issue reports" do -- 2.39.5
<%= check_box_tag("ids[]", relation.other_issue(@issue).id, false, :id => nil) %> <%= l(relation.label_for(@issue)) %> <%= "(#{l('datetime.distance_in_words.x_days', :count => relation.delay)})" if relation.delay && relation.delay != 0 %> <%= h(relation.other_issue(@issue).project) + ' - ' if Setting.cross_project_issue_relations? %> @@ -19,7 +19,7 @@ <%= relation.other_issue(@issue).status.name %> <%= format_date(relation.other_issue(@issue).start_date) %> <%= format_date(relation.other_issue(@issue).due_date) %><%= link_to_remote(image_tag('link_break.png'), { :url => {:controller => 'issue_relations', :action => 'destroy', :issue_id => @issue, :id => relation}, +<%= link_to_remote(image_tag('link_break.png'), { :url => {:controller => 'issue_relations', :action => 'destroy', :id => relation}, :method => :delete }, :title => l(:label_relation_delete)) if authorize_for('issue_relations', 'destroy') %>