[35815] Add visible show page for all users to groups (#8949)

To that end, move the show group routes out of /admin, that requires we
use a different path helper for it as `group_path` will still link to
the first admin resource.

app/controllers/groups_controller.rb

@@ -33,92 +33,56 @@ class GroupsController < ApplicationController
 
   helper_method :gon
 
-  before_action :require_admin
+  before_action :require_admin, except: %i[show]
   before_action :find_group, only: %i[destroy show create_memberships destroy_membership
                                       edit_membership add_users]
 
-  # GET /groups
-  # GET /groups.xml
   def index
     @groups = Group.order(Arel.sql('lastname ASC'))
-
-    respond_to do |format|
-      format.html # index.html.erb
-      format.xml  do render xml: @groups end
-    end
   end
 
-  # GET /groups/1
-  # GET /groups/1.xml
   def show
-    respond_to do |format|
-      format.html # show.html.erb
-      format.xml  do render xml: @group end
-    end
+    @group_users = group_members
+    render layout: 'no_menu'
   end
 
-  # GET /groups/new
-  # GET /groups/new.xml
   def new
     @group = Group.new
-
-    respond_to do |format|
-      format.html # new.html.erb
-      format.xml  do render xml: @group end
-    end
   end
 
-  # GET /groups/1/edit
   def edit
     @group = Group.includes(:members, :users).find(params[:id])
 
     set_filters_for_user_autocompleter
   end
 
-  # POST /groups
-  # POST /groups.xml
   def create
     @group = Group.new permitted_params.group
 
-    respond_to do |format|
-      if @group.save
-        flash[:notice] = I18n.t(:notice_successful_create)
-        format.html do redirect_to(groups_path) end
-        format.xml  do render xml: @group, status: :created, location: @group end
-      else
-        format.html do render action: 'new' end
-        format.xml  do render xml: @group.errors, status: :unprocessable_entity end
-      end
+    if @group.save
+      flash[:notice] = I18n.t(:notice_successful_create)
+      redirect_to(groups_path)
+    else
+      render action: :new
     end
   end
 
-  # PUT /groups/1
-  # PUT /groups/1.xml
   def update
     @group = Group.includes(:users).find(params[:id])
 
-    respond_to do |format|
-      if @group.update(permitted_params.group)
-        flash[:notice] = I18n.t(:notice_successful_update)
-        format.html do redirect_to(groups_path) end
-        format.xml  do head :ok end
-      else
-        format.html do render action: 'edit' end
-        format.xml  do render xml: @group.errors, status: :unprocessable_entity end
-      end
+    if @group.update(permitted_params.group)
+      flash[:notice] = I18n.t(:notice_successful_update)
+      redirect_to action: :index
+    else
+      render action: :edit
     end
   end
 
-  # DELETE /groups/1
-  # DELETE /groups/1.xml
   def destroy
     @group.destroy
 
-    respond_to do |format|
-      flash[:notice] = I18n.t(:notice_successful_delete)
-      format.html do redirect_to(groups_url) end
-      format.xml  do head :ok end
-    end
+    flash[:notice] = I18n.t(:notice_successful_delete)
+    redirect_to action: :index
   end
 
   def add_users
@@ -181,8 +145,21 @@ class GroupsController < ApplicationController
     @group = Group.find(params[:id])
   end
 
+  def group_members
+    if visible_group_members?
+      @group.users
+    else
+      User.none
+    end
+  end
+
+  def visible_group_members?
+    current_user.allowed_to_globally?(:manage_members) ||
+      Group.in_project(Project.allowed_to(current_user, :view_members)).exists?
+  end
+
   def default_breadcrumb
-    if action_name == 'index'
+    if action_name == 'index' || !current_user.admin?
       t('label_group_plural')
     else
       ActionController::Base.helpers.link_to(t('label_group_plural'), groups_path)

app/views/groups/show.html.erb

@@ -40,14 +40,17 @@ See docs/COPYRIGHT.rdoc for more details.
                   data: { confirm: t(:text_are_you_sure) },
                   class: 'button -danger',
                   method: :delete do %>
-          <%= op_icon('button--icon icon-delete') %>
-          <span class="button--text"><%= t(:button_delete) %></span>
+        <%= op_icon('button--icon icon-delete') %>
+        <span class="button--text"><%= t(:button_delete) %></span>
       <% end %>
     </li>
   <% end %>
-<% end  %>
-<ul>
-  <% @group.users.each do |user| %>
-    <li><%=h user %></li>
-  <% end %>
-</ul>
+<% end %>
+
+<% if @group_users.any? %>
+  <ul>
+    <% @group_users.each do |user| %>
+      <li><%= h user %></li>
+    <% end %>
+  </ul>
+<% end %>

config/routes.rb

@@ -356,7 +356,7 @@ OpenProject::Application.routes.draw do
 
     resources :attribute_help_texts, only: %i(index new create edit update destroy)
 
-    resources :groups do
+    resources :groups, except: %i[show] do
       member do
         # this should be put into it's own resource
         match '/members' => 'groups#add_users', via: :post, as: 'members_of'
@@ -465,6 +465,9 @@ OpenProject::Application.routes.draw do
     end
   end
 
+  # The show page of groups is public and thus moved out of the admin scope
+  resources :groups, only: %i[show], as: :show_group
+
   scope controller: 'users_settings' do
     get 'users_settings' => 'users_settings#index'
     post 'users_settings' => 'users_settings#edit'

spec/controllers/groups_controller_spec.rb

@@ -0,0 +1,195 @@
+#-- encoding: UTF-8
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2021 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+require 'spec_helper'
+
+describe GroupsController, type: :controller do
+  let(:group) { FactoryBot.create :group }
+
+  before do
+    login_as current_user
+  end
+
+  context 'as admin' do
+    using_shared_fixtures :admin
+    let(:current_user) { admin }
+
+    it 'should index' do
+      get :index
+      expect(response).to be_successful
+      expect(response).to render_template 'index'
+    end
+
+    it 'should show' do
+      get :show, params: { id: group.id }
+      expect(response).to be_successful
+      expect(response).to render_template 'show'
+    end
+
+    it 'should new' do
+      get :new
+      expect(response).to be_successful
+      expect(response).to render_template 'new'
+    end
+
+    it 'should create' do
+      expect do
+        post :create, params: { group: { lastname: 'New group' } }
+      end.to change { Group.count }.by(1)
+      expect(response).to redirect_to groups_path
+    end
+
+    it 'should edit' do
+      get :edit, params: { id: group.id }
+
+      expect(response).to be_successful
+      expect(response).to render_template 'edit'
+    end
+
+    it 'should update' do
+      expect do
+        put :update, params: { id: group.id, group: { lastname: 'new name' } }
+      end.to change { group.reload.name }.to('new name')
+
+      expect(response).to redirect_to groups_path
+    end
+
+    it 'should destroy' do
+      delete :destroy, params: { id: group.id }
+      expect { group.reload }.to raise_error ActiveRecord::RecordNotFound
+
+      expect(response).to redirect_to groups_path
+    end
+
+    context 'with two existing users' do
+      let(:user1) { FactoryBot.create :user }
+      let(:user2) { FactoryBot.create :user }
+
+      it 'should add users' do
+        post :add_users, params: { id: group.id, user_ids: [user1.id, user2.id] }
+        expect(group.reload.users.count).to eq 2
+      end
+    end
+
+    context 'with a group member' do
+      let(:user1) { FactoryBot.create :user }
+      let(:user2) { FactoryBot.create :user }
+
+      it 'should add users' do
+        group.add_members! user1
+
+        post :add_users, params: { id: group.id, user_ids: [user2.id] }
+        expect(group.reload.users.count).to eq 2
+      end
+    end
+
+    context 'with project and role' do
+      let(:project) { FactoryBot.create :project }
+      let(:role1) { FactoryBot.create :role }
+      let(:role2) { FactoryBot.create :role }
+
+      it 'should create membership' do
+        post :create_memberships,
+             params: { id: group.id, new_membership: { project_id: project.id, role_ids: [role1.id, role2.id] } }
+
+        expect(group.reload.members.count).to eq 1
+        expect(group.members.first.roles.count).to eq 2
+      end
+
+      context 'with an existing membership' do
+        let!(:member_group) do
+          FactoryBot.create(:member,
+                            project: project,
+                            principal: group,
+                            roles: [role1])
+        end
+
+        it 'should edit a membership' do
+          expect(group.members.count).to eq 1
+          expect(group.members.first.roles.count).to eq 1
+
+          put :edit_membership,
+              params: {
+                id: group.id,
+                membership_id: group.members.first.id,
+                membership: { project_id: project.id, role_ids: [role1.id, role2.id] }
+              }
+
+          group.reload
+          expect(group.members.count).to eq 1
+          expect(group.members.first.roles.count).to eq 2
+        end
+
+        it 'can destroy the membership' do
+          delete :destroy_membership, params: { id: group.id, membership_id: group.members.first.id }
+          expect(group.reload.members.count).to eq 0
+        end
+      end
+    end
+  end
+
+  context 'as regular user' do
+    let(:user) { FactoryBot.create :user }
+    let(:current_user) { user }
+
+    it 'should forbid index' do
+      get :index
+      expect(response).not_to be_successful
+      expect(response.status).to eq 403
+    end
+
+    it 'should show' do
+      get :show, params: { id: group.id }
+      expect(response).to be_successful
+      expect(response).to render_template 'show'
+    end
+
+    it 'should forbid new' do
+      get :new
+      expect(response).not_to be_successful
+      expect(response.status).to eq 403
+    end
+
+    it 'should forbid create' do
+      expect {
+        post :create, params: { group: { lastname: 'New group' } }
+      }.not_to change { Group.count }
+
+      expect(response).not_to be_successful
+      expect(response.status).to eq 403
+    end
+
+    it 'should forbid edit' do
+      get :edit, params: { id: group.id }
+
+      expect(response).not_to be_successful
+      expect(response.status).to eq 403
+    end
+  end
+end

spec/features/groups/group_show_spec.rb

@@ -0,0 +1,61 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2021 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See docs/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+
+feature 'group show page', type: :feature do
+  let!(:member) { FactoryBot.create :user }
+  let!(:group) { FactoryBot.create :group, lastname: "Bob's Team", members: [member] }
+
+  before do
+    login_as current_user
+  end
+
+  context 'as an admin' do
+    using_shared_fixtures :admin
+    let(:current_user) { admin }
+
+    scenario 'I can visit the group page' do
+      visit show_group_path(group)
+      expect(page).to have_selector('h2', text: "Bob's Team")
+      expect(page).to have_selector('.toolbar-item', text: 'Edit')
+      expect(page).to have_selector('li', text: member.name)
+    end
+  end
+
+  context 'as a regular user' do
+    let(:current_user) { FactoryBot.create :user }
+
+    scenario 'I can visit the group page' do
+      visit show_group_path(group)
+      expect(page).to have_selector('h2', text: "Bob's Team")
+      expect(page).to have_no_selector('.toolbar-item')
+      expect(page).to have_no_selector('li', text: member.name)
+    end
+  end
+end

spec/routing/groups_spec.rb

@@ -45,9 +45,9 @@ describe 'groups routes', type: :routing do
   }
 
   it {
-    is_expected.to route(:get, '/admin/groups/4').to(controller: 'groups',
-                                                     action: 'show',
-                                                     id: '4')
+    is_expected.to route(:get, '/groups/4').to(controller: 'groups',
+                                               action: 'show',
+                                               id: '4')
   }
 
   it {

spec_legacy/functional/groups_controller_spec.rb

@@ -1,113 +0,0 @@
-#-- encoding: UTF-8
-
-#-- copyright
-# OpenProject is an open source project management software.
-# Copyright (C) 2012-2021 the OpenProject GmbH
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License version 3.
-#
-# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
-# Copyright (C) 2006-2013 Jean-Philippe Lang
-# Copyright (C) 2010-2013 the ChiliProject Team
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License
-# as published by the Free Software Foundation; either version 2
-# of the License, or (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
-#
-# See docs/COPYRIGHT.rdoc for more details.
-#++
-require_relative '../legacy_spec_helper'
-require 'groups_controller'
-
-describe GroupsController, type: :controller do
-  fixtures :all
-
-  before do
-    User.current = nil
-    session[:user_id] = 1
-  end
-
-  it 'should index' do
-    get :index
-    assert_response :success
-    assert_template 'index'
-  end
-
-  it 'should show' do
-    get :show, params: { id: 10 }
-    assert_response :success
-    assert_template 'show'
-  end
-
-  it 'should new' do
-    get :new
-    assert_response :success
-    assert_template 'new'
-  end
-
-  it 'should create' do
-    assert_difference 'Group.count' do
-      post :create, params: { group: { lastname: 'New group' } }
-    end
-    assert_redirected_to groups_path
-  end
-
-  it 'should edit' do
-    get :edit, params: { id: 10 }
-    assert_response :success
-    assert_template 'edit'
-  end
-
-  it 'should update' do
-    put :update, params: { id: 10, group: { lastname: 'new name' } }
-    assert_redirected_to groups_path
-  end
-
-  it 'should destroy' do
-    assert_difference 'Group.count', -1 do
-      delete :destroy, params: { id: 10 }
-    end
-    assert_redirected_to groups_path
-  end
-
-  it 'should add users' do
-    assert_difference 'Group.find(10).users.count', 2 do
-      post :add_users, params: { id: 10, user_ids: ['2', '3'] }
-    end
-  end
-
-  it 'should remove user' do
-    assert_difference 'Group.find(10).users.count', -1 do
-      delete :remove_user, params: { id: 10, user_id: '8' }
-    end
-  end
-
-  it 'should create membership' do
-    assert_difference 'Group.find(10).members.count' do
-      post :create_memberships, params: { id: 10, new_membership: { project_id: 2, role_ids: ['1', '2'] } }
-    end
-  end
-
-  it 'should edit membership' do
-    assert_no_difference 'Group.find(10).members.count' do
-      put :edit_membership, params: { id: 10, membership_id: 6, membership: { role_ids: ['1', '3'] } }
-    end
-  end
-
-  it 'should destroy membership' do
-    assert_difference 'Group.find(10).members.count', -1 do
-      delete :destroy_membership, params: { id: 10, membership_id: 6 }
-    end
-  end
-end