Move group members index from /members to /group_members.

app/assets/javascripts/dispatcher.js.coffee

@@ -73,7 +73,7 @@ class Dispatcher
         new Activities()
         shortcut_handler = new ShortcutsNavigation()
         new ProjectsList()
-      when 'groups:members'
+      when 'groups:group_members:index'
         new GroupMembers()
         new UsersSelect()
       when 'groups:new', 'groups:edit', 'admin:groups:edit'

app/controllers/groups/application_controller.rb

@@ -2,9 +2,27 @@ class Groups::ApplicationController < ApplicationController
 
   private
 
+  def authorize_read_group!
+    unless @group and can?(current_user, :read_group, @group)
+      if current_user.nil?
+        return authenticate_user!
+      else
+        return render_404
+      end
+    end
+  end
+
   def authorize_admin_group!
     unless can?(current_user, :manage_group, group)
       return render_404
     end
   end
+
+  def determine_layout
+    if current_user
+      'group'
+    else
+      'public_group'
+    end
+  end
 end

app/controllers/groups/group_members_controller.rb

@@ -1,15 +1,30 @@
 class Groups::GroupMembersController < Groups::ApplicationController
+  skip_before_filter :authenticate_user!, only: [:index]
   before_filter :group
 
   # Authorize
-  before_filter :authorize_admin_group!
+  before_filter :authorize_read_group!
+  before_filter :authorize_admin_group!, except: [:index, :leave]
 
-  layout 'group'
+  layout :determine_layout
+
+  def index
+    @project = @group.projects.find(params[:project_id]) if params[:project_id]
+    @members = @group.group_members
+
+    if params[:search].present?
+      users = @group.users.search(params[:search]).to_a
+      @members = @members.where(user_id: users)
+    end
+
+    @members = @members.order('access_level DESC').page(params[:page]).per(50)
+    @group_member = GroupMember.new
+  end
 
   def create
     @group.add_users(params[:user_ids].split(','), params[:access_level])
 
-    redirect_to members_group_path(@group), notice: 'Users were successfully added.'
+    redirect_to group_group_members_path(@group), notice: 'Users were successfully added.'
   end
 
   def update
@@ -23,7 +38,7 @@ def destroy
     if can?(current_user, :destroy_group_member, @group_member)  # May fail if last owner.
       @group_member.destroy
       respond_to do |format|
-        format.html { redirect_to members_group_path(@group), notice: 'User was  successfully removed from group.' }
+        format.html { redirect_to group_group_members_path(@group), notice: 'User was  successfully removed from group.' }
         format.js { render nothing: true }
       end
     else

app/controllers/groups_controller.rb

@@ -1,5 +1,5 @@
 class GroupsController < Groups::ApplicationController
-  skip_before_filter :authenticate_user!, only: [:show, :issues, :members, :merge_requests]
+  skip_before_filter :authenticate_user!, only: [:show, :issues, :merge_requests]
   respond_to :html
   before_filter :group, except: [:new, :create]
 
@@ -67,19 +67,6 @@ def issues
     end
   end
 
-  def members
-    @project = group.projects.find(params[:project_id]) if params[:project_id]
-    @members = group.group_members
-
-    if params[:search].present?
-      users = group.users.search(params[:search]).to_a
-      @members = @members.where(user_id: users)
-    end
-
-    @members = @members.order('access_level DESC').page(params[:page]).per(50)
-    @users_group = GroupMember.new
-  end
-
   def edit
   end
 

app/views/groups/_new_group_member.html.haml → app/views/groups/group_members/_new_group_member.html.haml

@@ -1,12 +1,12 @@
-= form_for @users_group, url: group_group_members_path(@group), html: { class: 'form-horizontal users-group-form' } do |f|
+= form_for @group_member, url: group_group_members_path(@group), html: { class: 'form-horizontal users-group-form' } do |f|
   .form-group
     = f.label :user_ids, "People", class: 'control-label'
     .col-sm-10= users_select_tag(:user_ids, multiple: true, class: 'input-large')
 
   .form-group
     = f.label :access_level, "Group Access", class: 'control-label'
     .col-sm-10
-      = select_tag :access_level, options_for_select(GroupMember.access_level_roles, @users_group.access_level), class: "project-access-select select2"
+      = select_tag :access_level, options_for_select(GroupMember.access_level_roles, @group_member.access_level), class: "project-access-select select2"
       .help-block
         Read more about role permissions
         %strong= link_to "here", help_page_path("permissions", "permissions"), class: "vlink"

app/views/groups/members.html.haml → app/views/groups/group_members/index.html.haml

@@ -1,4 +1,5 @@
 - show_roles = should_user_see_group_roles?(current_user, @group)
+
 %h3.page-title
   Group members
 - if show_roles
@@ -10,7 +11,7 @@
 %hr
 
 .clearfix.js-toggle-container
-  = form_tag members_group_path(@group), method: :get, class: 'form-inline member-search-form'  do
+  = form_tag group_group_members_path(@group), method: :get, class: 'form-inline member-search-form'  do
     .form-group
       = search_field_tag :search, params[:search], { placeholder: 'Find existing member by name', class: 'form-control search-text-input input-mn-300' }
     = button_tag 'Search', class: 'btn'
@@ -33,6 +34,7 @@
   %ul.well-list
     - @members.each do |member|
       = render 'groups/group_members/group_member', member: member, show_roles: show_roles, show_controls: true
+
 = paginate @members, theme: 'gitlab'
 
 :coffeescript

app/views/layouts/nav/_group.html.haml

@@ -24,8 +24,8 @@
         Merge Requests
         - if current_user
           %span.count= MergeRequest.opened.of_group(@group).count
-  = nav_link(path: 'groups#members') do
-    = link_to members_group_path(@group), title: 'Members' do
+  = nav_link(controller: [:group_members]) do
+    = link_to group_group_members_path(@group), title: 'Members' do
       %i.fa.fa-users
       %span
         Members

config/routes.rb

@@ -236,12 +236,13 @@
     member do
       get :issues
       get :merge_requests
-      get :members
       get :projects
     end
 
     scope module: :groups do
-      resources :group_members, only: [:create, :update, :destroy]
+      resources :group_members, only: [:index, :create, :update, :destroy] do
+      end
+
       resource :avatar, only: [:destroy]
       resources :milestones, only: [:index, :show, :update]
     end

features/steps/explore/groups.rb

@@ -35,7 +35,7 @@ class Spinach::Features::ExploreGroups < Spinach::FeatureSteps
   end
 
   step 'I visit group "TestGroup" members page' do
-    visit members_group_path(Group.find_by(name: "TestGroup"))
+    visit group_group_members_path(Group.find_by(name: "TestGroup"))
   end
 
   step 'I should not see project "Enterprise" items' do

features/steps/shared/paths.rb

@@ -32,7 +32,7 @@ module SharedPaths
   end
 
   step 'I visit group "Owned" members page' do
-    visit members_group_path(Group.find_by(name:"Owned"))
+    visit group_group_members_path(Group.find_by(name:"Owned"))
   end
 
   step 'I visit group "Owned" settings page' do
@@ -52,7 +52,7 @@ module SharedPaths
   end
 
   step 'I visit group "Guest" members page' do
-    visit members_group_path(Group.find_by(name:"Guest"))
+    visit group_group_members_path(Group.find_by(name:"Guest"))
   end
 
   step 'I visit group "Guest" settings page' do

spec/features/security/group/group_access_spec.rb

@@ -59,8 +59,8 @@
       it { is_expected.to be_denied_for :visitor }
     end
 
-    describe "GET /groups/:path/members" do
-      subject { members_group_path(group) }
+    describe "GET /groups/:path/group_members" do
+      subject { group_group_members_path(group) }
 
       it { is_expected.to be_allowed_for owner }
       it { is_expected.to be_allowed_for master }

spec/features/security/group/internal_group_access_spec.rb

@@ -55,8 +55,8 @@
       it { is_expected.to be_denied_for :visitor }
     end
 
-    describe "GET /groups/:path/members" do
-      subject { members_group_path(group) }
+    describe "GET /groups/:path/group_members" do
+      subject { group_group_members_path(group) }
 
       it { is_expected.to be_allowed_for owner }
       it { is_expected.to be_allowed_for master }

spec/features/security/group/mixed_group_access_spec.rb

@@ -56,8 +56,8 @@
       it { is_expected.to be_allowed_for :visitor }
     end
 
-    describe "GET /groups/:path/members" do
-      subject { members_group_path(group) }
+    describe "GET /groups/:path/group_members" do
+      subject { group_group_members_path(group) }
 
       it { is_expected.to be_allowed_for owner }
       it { is_expected.to be_allowed_for master }

spec/features/security/group/public_group_access_spec.rb

@@ -55,8 +55,8 @@
       it { is_expected.to be_allowed_for :visitor }
     end
 
-    describe "GET /groups/:path/members" do
-      subject { members_group_path(group) }
+    describe "GET /groups/:path/group_members" do
+      subject { group_group_members_path(group) }
 
       it { is_expected.to be_allowed_for owner }
       it { is_expected.to be_allowed_for master }