Makes relations resource shallow (#7366).

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@6184 e93f8b46-1217-0410-a6f0-8f06a7374b81

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 @@
 <form>
 <table class="list issues">
 <% @relations.each do |relation| %>
-<tr class="issue hascontextmenu">
+<tr class="issue hascontextmenu" id="relation-<%= relation.id %>">
 <td class="checkbox"><%= check_box_tag("ids[]", relation.other_issue(@issue).id, false, :id => nil) %></td>
 <td class="subject"><%= 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 @@
 <td class="status"><%= relation.other_issue(@issue).status.name %></td>
 <td class="start_date"><%= format_date(relation.other_issue(@issue).start_date) %></td>
 <td class="due_date"><%= format_date(relation.other_issue(@issue).due_date) %></td>
-<td class="buttons"><%= link_to_remote(image_tag('link_break.png'), { :url => {:controller => 'issue_relations', :action => 'destroy', :issue_id => @issue, :id => relation},                                              
+<td class="buttons"><%= 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') %></td>
 </tr>
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