Don't render memberships forms, use #edit instead.

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

18 files changed, 112 insertions, 54 deletions

diff --git a/app/controllers/members_controller.rb b/app/controllers/members_controller.rb
index ca3dcd474..38cdfcd8a 100644
--- a/app/controllers/members_controller.rb
+++ b/app/controllers/members_controller.rb
@@ -80,6 +80,10 @@ class MembersController < ApplicationController
     end
   end
 
+  def edit
+    @roles = Role.givable.to_a
+  end
+
   def update
     if params[:membership]
       @member.set_editable_role_ids(params[:membership][:role_ids])
diff --git a/app/controllers/principal_memberships_controller.rb b/app/controllers/principal_memberships_controller.rb
index b03460b54..d4f4ba2de 100644
--- a/app/controllers/principal_memberships_controller.rb
+++ b/app/controllers/principal_memberships_controller.rb
@@ -21,7 +21,7 @@ class PrincipalMembershipsController < ApplicationController
 
   before_action :require_admin
   before_action :find_principal, :only => [:new, :create]
-  before_action :find_membership, :only => [:update, :destroy]
+  before_action :find_membership, :only => [:edit, :update, :destroy]
 
   def new
     @projects = Project.active.all
@@ -40,6 +40,10 @@ class PrincipalMembershipsController < ApplicationController
     end
   end
 
+  def edit
+    @roles = Role.givable.to_a
+  end
+
   def update
     @membership.attributes = params[:membership]
     @membership.save
diff --git a/app/helpers/principal_memberships_helper.rb b/app/helpers/principal_memberships_helper.rb
index e3ef37798..10f278ddc 100644
--- a/app/helpers/principal_memberships_helper.rb
+++ b/app/helpers/principal_memberships_helper.rb
@@ -46,6 +46,14 @@ module PrincipalMembershipsHelper
     end
   end
 
+  def edit_principal_membership_path(principal, *args)
+    if principal.is_a?(Group)
+      edit_group_membership_path(principal, *args)
+    else
+      edit_user_membership_path(principal, *args)
+    end
+  end
+
   def principal_membership_path(principal, membership, *args)
     if principal.is_a?(Group)
       group_membership_path(principal, membership, *args)
diff --git a/app/views/members/_edit.html.erb b/app/views/members/_edit.html.erb
new file mode 100644
index 000000000..91c340fec
--- /dev/null
+++ b/app/views/members/_edit.html.erb
@@ -0,0 +1,21 @@
+<%= form_for(@member, :url => membership_path(@member),
+                      :as => :membership,
+                      :remote => request.xhr?,
+                      :method => :put) do |f| %>
+  <p>
+    <% @roles.each do |role| %>
+    <label>
+      <%= check_box_tag('membership[role_ids][]',
+                        role.id, @member.roles.to_a.include?(role),
+                        :id => nil,
+                        :disabled => !@member.role_editable?(role)) %> <%= role %>
+    </label><br />
+    <% end %>
+  </p>
+  <%= hidden_field_tag 'membership[role_ids][]', '', :id => nil %>
+  <p>
+    <%= submit_tag l(:button_save), :class => "small" %>
+    <%= link_to_function l(:button_cancel),
+                         "$('#member-#{@member.id}-roles').show(); $('#member-#{@member.id}-form').empty(); return false;" if request.xhr? %>
+  </p>
+<% end %>
diff --git a/app/views/members/edit.html.erb b/app/views/members/edit.html.erb
new file mode 100644
index 000000000..99ef3161d
--- /dev/null
+++ b/app/views/members/edit.html.erb
@@ -0,0 +1,3 @@
+<%= title "#{@member.principal} - #{@member.project}" %>
+
+<%= render :partial => 'edit' %>
diff --git a/app/views/members/edit.js.erb b/app/views/members/edit.js.erb
new file mode 100644
index 000000000..379ed3ece
--- /dev/null
+++ b/app/views/members/edit.js.erb
@@ -0,0 +1,3 @@
+$("#member-<%= @member.id %>-roles").hide();
+$("#member-<%= @member.id %>-form").html("<%= escape_javascript(render :partial => "edit") %>");
+
diff --git a/app/views/principal_memberships/_edit.html.erb b/app/views/principal_memberships/_edit.html.erb
new file mode 100644
index 000000000..af05c4935
--- /dev/null
+++ b/app/views/principal_memberships/_edit.html.erb
@@ -0,0 +1,19 @@
+<%= form_for(:membership, :url => principal_membership_path(@principal, @membership),
+                          :remote => request.xhr?,
+                          :method => :put) do %>
+  <p>
+  <% @roles.each do |role| %>
+    <label>
+      <%= check_box_tag 'membership[role_ids][]', role.id, @membership.roles.to_a.include?(role),
+            :disabled => !@membership.role_editable?(role),
+            :id => nil %> <%= role.name %>
+    </label><br />
+  <% end %>
+  </p>
+  <%= hidden_field_tag 'membership[role_ids][]', '', :id => nil %>
+  <p>
+    <%= submit_tag l(:button_save) %>
+    <%= link_to_function l(:button_cancel),
+                         "$('#member-#{@membership.id}-roles').show(); $('#member-#{@membership.id}-form').empty(); return false;" if request.xhr? %>
+  </p>
+<% end %>
diff --git a/app/views/principal_memberships/_index.html.erb b/app/views/principal_memberships/_index.html.erb
index 76a629f95..b451f536b 100644
--- a/app/views/principal_memberships/_index.html.erb
+++ b/app/views/principal_memberships/_index.html.erb
@@ -1,5 +1,3 @@
-<% roles = Role.find_all_givable %>
-
 <p><%= link_to l(:label_add_projects), new_principal_membership_path(principal), :remote => true, :class => "icon icon-add" %></p>
 
 <% if principal.memberships.any? %>
@@ -19,26 +17,13 @@
   </td>
   <td class="roles">
     <span id="member-<%= membership.id %>-roles"><%=h membership.roles.sort.collect(&:to_s).join(', ') %></span>
-    <%= form_for(:membership, :remote => true,
-                 :url => principal_membership_path(principal, membership), :method => :put,
-                 :html => {:id => "member-#{membership.id}-roles-form",
-                           :style => 'display:none;'}) do %>
-        <p><% roles.each do |role| %>
-        <label><%= check_box_tag 'membership[role_ids][]', role.id, membership.roles.include?(role),
-                                                           :disabled => membership.member_roles.detect {|mr| mr.role_id == role.id && !mr.inherited_from.nil?},
-                                                           :id => nil %> <%=h role %></label><br />
-        <% end %></p>
-        <%= hidden_field_tag 'membership[role_ids][]', '', :id => nil %>
-        <p><%= submit_tag l(:button_change) %>
-        <%= link_to_function l(:button_cancel),
-                             "$('#member-#{membership.id}-roles').show(); $('#member-#{membership.id}-roles-form').hide(); return false;"
-            %></p>
-    <% end %>
+    <div id="member-<%= membership.id %>-form"></div>
   </td>
   <td class="buttons">
-      <%= link_to_function l(:button_edit),
-                           "$('#member-#{membership.id}-roles').hide(); $('#member-#{membership.id}-roles-form').show(); return false;",
-                           :class => 'icon icon-edit'
+      <%= link_to l(:button_edit),
+                  edit_principal_membership_path(principal, membership),
+                  :remote => true,
+                  :class => 'icon icon-edit'
           %>
       <%= delete_link principal_membership_path(principal, membership), :remote => true if membership.deletable? %>
   </td>
diff --git a/app/views/principal_memberships/edit.html.erb b/app/views/principal_memberships/edit.html.erb
new file mode 100644
index 000000000..15e64125a
--- /dev/null
+++ b/app/views/principal_memberships/edit.html.erb
@@ -0,0 +1,3 @@
+<%= title "#{@membership.principal} - #{@membership.project}" %>
+
+<%= render :partial => 'edit' %>
diff --git a/app/views/principal_memberships/edit.js.erb b/app/views/principal_memberships/edit.js.erb
new file mode 100644
index 000000000..083bac502
--- /dev/null
+++ b/app/views/principal_memberships/edit.js.erb
@@ -0,0 +1,2 @@
+$("#member-<%= @membership.id %>-roles").hide();
+$("#member-<%= @membership.id %>-form").html("<%= escape_javascript(render :partial => "edit") %>");
diff --git a/app/views/principal_memberships/update.js.erb b/app/views/principal_memberships/update.js.erb
index 2986c4e55..2b720ff9e 100644
--- a/app/views/principal_memberships/update.js.erb
+++ b/app/views/principal_memberships/update.js.erb
@@ -1,5 +1,8 @@
-<% if @membership.valid? %>
-  $('#tab-content-memberships').html('<%= escape_javascript(render :partial => 'principal_memberships/index', :locals => {:principal => @principal}) %>');
+<% if @membership.destroyed? %>
+  $("#member-<%= @membership.id %>").remove();
+<% elsif @membership.valid? %>
+  $("#member-<%= @membership.id %>-form").empty();
+  $("#member-<%= @membership.id %>-roles").html("<%= escape_javascript @membership.roles.sort.collect(&:to_s).join(', ') %>").show();
   $("#member-<%= @membership.id %>").effect("highlight");
 <% else %>
   alert('<%= raw(escape_javascript(l(:notice_failed_to_save_members, :errors => @membership.errors.full_messages.join(', ')))) %>');
diff --git a/app/views/projects/settings/_members.html.erb b/app/views/projects/settings/_members.html.erb
index 1dd665a34..4009e7b15 100644
--- a/app/views/projects/settings/_members.html.erb
+++ b/app/views/projects/settings/_members.html.erb
@@ -1,5 +1,4 @@
-<% roles = Role.find_all_givable
-   members = @project.memberships.sorted.to_a %>
+<% members = @project.memberships.sorted.to_a %>
 
 <p><%= link_to l(:label_member_new), new_project_membership_path(@project), :remote => true, :class => "icon icon-add" %></p>
 
@@ -20,34 +19,13 @@
   <td class="name icon icon-<%= member.principal.class.name.downcase %>"><%= link_to_user member.principal %></td>
   <td class="roles">
     <span id="member-<%= member.id %>-roles"><%= member.roles.sort.collect(&:to_s).join(', ') %></span>
-      <%= form_for(member,
-                   {:as => :membership, :remote => true,
-                    :url => membership_path(member),
-                    :method => :put,
-                    :html => { :id => "member-#{member.id}-roles-form", :class => 'hol' }}
-             ) do |f| %>
-        <p>
-          <% roles.each do |role| %>
-          <label>
-            <%= check_box_tag('membership[role_ids][]',
-                              role.id, member.roles.include?(role),
-                              :id => nil,
-                              :disabled => !member.role_editable?(role)) %> <%= role %>
-          </label><br />
-          <% end %>
-        </p>
-        <%= hidden_field_tag 'membership[role_ids][]', '', :id => nil %>
-        <p>
-          <%= submit_tag l(:button_save), :class => "small" %>
-          <%= link_to_function(l(:button_cancel),
-                               "$('#member-#{member.id}-roles').show(); $('#member-#{member.id}-roles-form').hide(); return false;") %>
-        </p>
-      <% end %>
+    <div id="member-<%= member.id %>-form"></div>
   </td>
   <td class="buttons">
-      <%= link_to_function l(:button_edit),
-                           "$('#member-#{member.id}-roles').hide(); $('#member-#{member.id}-roles-form').show(); return false;",
-                           :class => 'icon icon-edit' %>
+      <%= link_to l(:button_edit),
+                  edit_membership_path(member),
+                  :remote => true,
+                  :class => 'icon icon-edit' %>
       <%= delete_link membership_path(member),
                       :remote => true,
                       :data => (!User.current.admin? && member.include?(User.current) ? {:confirm => l(:text_own_membership_delete_confirmation)} : {}) if member.deletable? %>
diff --git a/config/routes.rb b/config/routes.rb
index 3a096e127..374e2b294 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -113,7 +113,7 @@ Rails.application.routes.draw do
     end
 
     shallow do
-      resources :memberships, :controller => 'members', :only => [:index, :show, :new, :create, :update, :destroy] do
+      resources :memberships, :controller => 'members' do
         collection do
           get 'autocomplete'
         end
diff --git a/lib/redmine.rb b/lib/redmine.rb
index 4ad182046..e3ff60fd1 100644
--- a/lib/redmine.rb
+++ b/lib/redmine.rb
@@ -83,7 +83,7 @@ Redmine::AccessControl.map do |map|
   map.permission :close_project, {:projects => [:close, :reopen]}, :require => :member, :read => true
   map.permission :select_project_modules, {:projects => :modules}, :require => :member
   map.permission :view_members, {:members => [:index, :show]}, :public => true, :read => true
-  map.permission :manage_members, {:projects => :settings, :members => [:index, :show, :new, :create, :update, :destroy, :autocomplete]}, :require => :member
+  map.permission :manage_members, {:projects => :settings, :members => [:index, :show, :new, :create, :edit, :update, :destroy, :autocomplete]}, :require => :member
   map.permission :manage_versions, {:projects => :settings, :versions => [:new, :create, :edit, :update, :close_completed, :destroy]}, :require => :member
   map.permission :add_subprojects, {:projects => [:new, :create]}, :require => :member
 
diff --git a/test/functional/members_controller_test.rb b/test/functional/members_controller_test.rb
index d132c8293..6de763fd1 100644
--- a/test/functional/members_controller_test.rb
+++ b/test/functional/members_controller_test.rb
@@ -110,6 +110,17 @@ class MembersControllerTest < Redmine::ControllerTest
     assert_match /alert/, response.body, "Alert message not sent"
   end
 
+  def test_edit
+    get :edit, :id => 2
+    assert_response :success
+    assert_select 'input[name=?][value=?][checked=checked]', 'membership[role_ids][]', '2'
+  end
+
+  def test_xhr_edit
+    xhr :get, :edit, :id => 2
+    assert_response :success
+  end
+
   def test_update
     assert_no_difference 'Member.count' do
       put :update, :id => 2, :membership => {:role_ids => [1], :user_id => 3}
diff --git a/test/functional/principal_memberships_controller_test.rb b/test/functional/principal_memberships_controller_test.rb
index f20a52099..ca0b2c53a 100644
--- a/test/functional/principal_memberships_controller_test.rb
+++ b/test/functional/principal_memberships_controller_test.rb
@@ -105,6 +105,17 @@ class PrincipalMembershipsControllerTest < Redmine::ControllerTest
     assert_include 'Role cannot be empty', response.body, "Error message not sent"
   end
 
+  def test_edit_user_membership
+    get :edit, :user_id => 2, :id => 1
+    assert_response :success
+    assert_select 'input[name=?][value=?][checked=checked]', 'membership[role_ids][]', '1'
+  end
+
+  def test_xhr_edit_user_membership
+    xhr :get, :edit, :user_id => 2, :id => 1
+    assert_response :success
+  end
+
   def test_update_user_membership
     assert_no_difference 'Member.count' do
       put :update, :user_id => 2, :id => 1, :membership => {:role_ids => [2]}
@@ -120,7 +131,7 @@ class PrincipalMembershipsControllerTest < Redmine::ControllerTest
       assert_equal 'text/javascript', response.content_type
     end
     assert_equal [2], Member.find(1).role_ids
-    assert_include 'tab-content-memberships', response.body
+    assert_include '$("#member-1-roles").html("Developer").show();', response.body
   end
 
   def test_destroy_user_membership
diff --git a/test/integration/routing/members_test.rb b/test/integration/routing/members_test.rb
index ae3eb5b5e..985d697e0 100644
--- a/test/integration/routing/members_test.rb
+++ b/test/integration/routing/members_test.rb
@@ -22,6 +22,7 @@ class RoutingMembersTest < Redmine::RoutingTest
     should_route 'GET /projects/foo/memberships/new' => 'members#new', :project_id => 'foo'
     should_route 'POST /projects/foo/memberships' => 'members#create', :project_id => 'foo'
 
+    should_route 'GET /memberships/5234/edit' => 'members#edit', :id => '5234'
     should_route 'PUT /memberships/5234' => 'members#update', :id => '5234'
     should_route 'DELETE /memberships/5234' => 'members#destroy', :id => '5234'
 
diff --git a/test/integration/routing/principal_memberships_test.rb b/test/integration/routing/principal_memberships_test.rb
index b40eb2ca3..db91c850b 100644
--- a/test/integration/routing/principal_memberships_test.rb
+++ b/test/integration/routing/principal_memberships_test.rb
@@ -21,6 +21,7 @@ class RoutingPrincipalMembershipsTest < Redmine::RoutingTest
   def test_user_memberships
     should_route 'GET /users/123/memberships/new' => 'principal_memberships#new', :user_id => '123'
     should_route 'POST /users/123/memberships' => 'principal_memberships#create', :user_id => '123'
+    should_route 'GET /users/123/memberships/55/edit' => 'principal_memberships#edit', :user_id => '123', :id => '55'
     should_route 'PUT /users/123/memberships/55' => 'principal_memberships#update', :user_id => '123', :id => '55'
     should_route 'DELETE /users/123/memberships/55' => 'principal_memberships#destroy', :user_id => '123', :id => '55'
   end
@@ -28,6 +29,7 @@ class RoutingPrincipalMembershipsTest < Redmine::RoutingTest
   def test_group_memberships
     should_route 'GET /groups/123/memberships/new' => 'principal_memberships#new', :group_id => '123'
     should_route 'POST /groups/123/memberships' => 'principal_memberships#create', :group_id => '123'
+    should_route 'GET /groups/123/memberships/55/edit' => 'principal_memberships#edit', :group_id => '123', :id => '55'
     should_route 'PUT /groups/123/memberships/55' => 'principal_memberships#update', :group_id => '123', :id => '55'
     should_route 'DELETE /groups/123/memberships/55' => 'principal_memberships#destroy', :group_id => '123', :id => '55'
   end