From ec20fdf366843e60ed30abb5322c3c1b8f471b4a Mon Sep 17 00:00:00 2001
From: Felipe Artur <felipefac@gmail.com>
Date: Wed, 16 Mar 2016 19:44:33 -0300
Subject: [PATCH] Code improvements and add Create group service
---
app/assets/stylesheets/framework/common.scss | 46 ++++++++-----------
app/assets/stylesheets/framework/mobile.scss | 2 +-
app/assets/stylesheets/pages/projects.scss | 22 ---------
app/controllers/groups_controller.rb | 4 +-
app/controllers/users_controller.rb | 9 +---
app/finders/joined_groups_finder.rb | 2 +-
app/models/ability.rb | 4 +-
app/services/groups/base_service.rb | 4 ++
app/services/groups/create_service.rb | 17 +++++++
app/services/groups/update_service.rb | 13 ++----
app/services/projects/create_service.rb | 6 ++-
app/views/groups/show.html.haml | 4 +-
app/views/projects/_home_panel.html.haml | 2 +-
...roup_visibility_to_application_settings.rb | 20 +++++++-
db/schema.rb | 2 +-
.../security/group/internal_access_spec.rb | 2 -
spec/services/groups/create_service_spec.rb | 22 +++++++++
17 files changed, 97 insertions(+), 84 deletions(-)
create mode 100644 app/services/groups/create_service.rb
create mode 100644 spec/services/groups/create_service_spec.rb
diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss
index 18044b25b87..0931090b840 100644
--- a/app/assets/stylesheets/framework/common.scss
+++ b/app/assets/stylesheets/framework/common.scss
@@ -383,35 +383,25 @@ table {
margin-right: -$gl-padding;
border-top: 1px solid $border-color;
}
-.message {
- border: 1px solid #ccc;
- padding: 10px;
- color: #333;
-}
-.message {
- border: 1px solid #ccc;
- padding: 10px;
- color: #333;
-}
-.group-projects-show-title{
- h1 {
- color: #313236;
- margin: 0;
- margin-bottom: 6px;
- font-size: 23px;
- font-weight: normal;
- }
+.cover-title{
+ h1 {
+ color: #313236;
+ margin: 0;
+ margin-bottom: 6px;
+ font-size: 23px;
+ font-weight: normal;
+ }
- .visibility-icon {
- display: inline-block;
- margin-left: 5px;
- font-size: 18px;
- color: $gray;
- }
+ .visibility-icon {
+ display: inline-block;
+ margin-left: 5px;
+ font-size: 18px;
+ color: $gray;
+ }
- p {
- padding: 0 $gl-padding;
- color: #5c5d5e;
- }
+ p {
+ padding: 0 $gl-padding;
+ color: #5c5d5e;
}
+}
diff --git a/app/assets/stylesheets/framework/mobile.scss b/app/assets/stylesheets/framework/mobile.scss
index 3bfac2ad9b5..d088228fe4c 100644
--- a/app/assets/stylesheets/framework/mobile.scss
+++ b/app/assets/stylesheets/framework/mobile.scss
@@ -48,7 +48,7 @@
display: block;
}
- .project-home-desc {
+ #project-home-desc {
font-size: 21px;
}
diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss
index b1b76edfb32..1a7b0c1e278 100644
--- a/app/assets/stylesheets/pages/projects.scss
+++ b/app/assets/stylesheets/pages/projects.scss
@@ -61,28 +61,6 @@
}
}
- .project-home-desc {
- h1 {
- color: #313236;
- margin: 0;
- margin-bottom: 6px;
- font-size: 23px;
- font-weight: normal;
- }
-
- .visibility-icon {
- display: inline-block;
- margin-left: 5px;
- font-size: 18px;
- color: $gray;
- }
-
- p {
- padding: 0 $gl-padding;
- color: #5c5d5e;
- }
- }
-
.project-repo-buttons {
margin-top: 20px;
margin-bottom: 0px;
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 54f14e62ead..5baeb3def08 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -29,10 +29,8 @@ class GroupsController < Groups::ApplicationController
def create
@group = Group.new(group_params)
- @group.name = @group.path.dup unless @group.name
- if @group.save
- @group.add_owner(current_user)
+ if Groups::CreateService.new(@group, current_user, group_params).execute
redirect_to @group, notice: "Group '#{@group.name}' was successfully created."
else
render action: "new"
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 7b32572f822..481d00d6aae 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -3,13 +3,6 @@ class UsersController < ApplicationController
before_action :set_user
def show
- @contributed_projects = contributed_projects.joined(@user).reject(&:forked?)
-
- @projects = PersonalProjectsFinder.new(@user).execute(current_user)
- @projects = @projects.page(params[:page]).per(PER_PAGE)
-
- @groups = JoinedGroupsFinder.new(@user).execute(current_user)
-
respond_to do |format|
format.html
@@ -115,7 +108,7 @@ class UsersController < ApplicationController
end
def load_groups
- @groups = @user.groups.order_id_desc
+ @groups = JoinedGroupsFinder.new(@user).execute(current_user)
end
def projects_for_current_user
diff --git a/app/finders/joined_groups_finder.rb b/app/finders/joined_groups_finder.rb
index 131b518563e..fbdf492c965 100644
--- a/app/finders/joined_groups_finder.rb
+++ b/app/finders/joined_groups_finder.rb
@@ -1,6 +1,6 @@
#Shows only authorized groups of a user
class JoinedGroupsFinder
- def initialize(user = nil)
+ def initialize(user)
@user = user
end
diff --git a/app/models/ability.rb b/app/models/ability.rb
index fe460ccdaca..bd001ef1545 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -295,8 +295,8 @@ class Ability
end
def can_read_group?(user, group)
- is_project_member = ProjectsFinder.new.execute(user, group: group).any?
- user.admin? || group.public? || group.internal? || is_project_member || group.users.include?(user)
+ user.admin? || group.public? || group.internal? || group.users.include?(user) ||
+ ProjectsFinder.new.execute(user, group: group).any?
end
def namespace_abilities(user, namespace)
diff --git a/app/services/groups/base_service.rb b/app/services/groups/base_service.rb
index 5becd475d3a..644ec7c013e 100644
--- a/app/services/groups/base_service.rb
+++ b/app/services/groups/base_service.rb
@@ -5,5 +5,9 @@ module Groups
def initialize(group, user, params = {})
@group, @current_user, @params = group, user, params.dup
end
+
+ def add_error_message(message)
+ group.errors.add(:visibility_level, message)
+ end
end
end
diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb
new file mode 100644
index 00000000000..e2875aafb94
--- /dev/null
+++ b/app/services/groups/create_service.rb
@@ -0,0 +1,17 @@
+module Groups
+ class CreateService < Groups::BaseService
+ def execute
+ return false unless visibility_level_allowed?(params[:visibility_level])
+ @group.name = @group.path.dup unless @group.name
+ @group.save(params) && @group.add_owner(current_user)
+ end
+
+ private
+
+ def visibility_level_allowed?(level)
+ allowed = Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
+ add_error_message("Visibility level restricted by admin.") unless allowed
+ allowed
+ end
+ end
+end
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index acb6c529c17..a7382c1e07c 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -5,7 +5,8 @@
module Groups
class UpdateService < Groups::BaseService
def execute
- visibility_level_allowed?(params[:visibility_level]) ? group.update_attributes(params) : false
+ return false unless visibility_level_allowed?(params[:visibility_level])
+ group.update_attributes(params)
end
private
@@ -22,7 +23,7 @@ module Groups
def visibility_by_project(level)
projects_visibility = group.projects.pluck(:visibility_level)
- allowed_by_projects = !projects_visibility.any?{|project_visibility| level.to_i < project_visibility }
+ allowed_by_projects = !projects_visibility.any?{ |project_visibility| level.to_i < project_visibility }
add_error_message("Cannot be changed. There are projects with higher visibility permissions.") unless allowed_by_projects
allowed_by_projects
end
@@ -32,13 +33,5 @@ module Groups
add_error_message("You are not authorized to set this permission level.") unless allowed_by_user
allowed_by_user
end
-
- def add_error_message(message)
- level_name = Gitlab::VisibilityLevel.level_name(params[:visibility_level])
- group.errors.add(:visibility_level, message)
- end
end
end
-
-
-
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index 522fae79503..4c121106bda 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -12,7 +12,7 @@ module Projects
# Make sure that the user is allowed to use the specified visibility
# level
- unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level]) && @project.visibility_level_allowed?(@project.visibility_level)
+ unless visibility_level_allowed?
deny_visibility_level(@project)
return @project
end
@@ -100,5 +100,9 @@ module Projects
@project.import_start if @project.import?
end
+
+ def visibility_level_allowed?
+ Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level]) && @project.visibility_level_allowed?(@project.visibility_level)
+ end
end
end
diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml
index 0a8c2da7207..641191edf05 100644
--- a/app/views/groups/show.html.haml
+++ b/app/views/groups/show.html.haml
@@ -14,11 +14,11 @@
.avatar-holder
= link_to group_icon(@group), target: '_blank' do
= image_tag group_icon(@group), class: "avatar group-avatar s90"
- .group-projects-show-title
+ .cover-title
%h1
= @group.name
- %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: "#{visibility_level_label(@group.visibility_level)} - #{project_visibility_level_description(@group.visibility_level)}"}
+ %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: "#{visibility_level_label(@group.visibility_level)} - #{group_visibility_description(@group)}"}
= visibility_level_icon(@group.visibility_level, fw: false)
.cover-desc.username
diff --git a/app/views/projects/_home_panel.html.haml b/app/views/projects/_home_panel.html.haml
index dd16f504c92..e8434b5292c 100644
--- a/app/views/projects/_home_panel.html.haml
+++ b/app/views/projects/_home_panel.html.haml
@@ -2,7 +2,7 @@
.project-home-panel.cover-block.clearfix{:class => ("empty-project" if empty_repo)}
.project-identicon-holder
= project_icon(@project, alt: '', class: 'project-avatar avatar s90')
- .group-projects-show-title
+ .cover-title#project-home-desc
%h1
= @project.name
%span.visibility-icon.has_tooltip{data: { container: 'body' },
diff --git a/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb b/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb
index c2bdd7b31fa..b71322376fa 100644
--- a/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb
+++ b/db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb
@@ -1,11 +1,27 @@
+#Create visibility level field on DB
+#Sets default_visibility_level to value on settings if not restricted
+#If value is restricted takes higher visibility level allowed
+
class AddDefaultGroupVisibilityToApplicationSettings < ActiveRecord::Migration
def up
add_column :application_settings, :default_group_visibility, :integer
- visibility = Settings.gitlab.default_groups_features['visibility_level']
- execute("update application_settings set default_group_visibility = #{visibility}")
+ execute("update application_settings set default_group_visibility = #{allowed_visibility_level}")
end
def down
remove_column :application_settings, :default_group_visibility
end
+
+ private
+ def allowed_visibility_level
+ default_visibility = Settings.gitlab.default_groups_features['visibility_level']
+ restricted_levels = current_application_settings.restricted_visibility_levels
+ return default_visibility unless restricted_levels.present?
+
+ if restricted_levels.include?(default_visibility)
+ Gitlab::VisibilityLevel.values.select{ |vis_level| vis_level unless restricted_levels.include?(vis_level) }.last
+ else
+ default_visibility
+ end
+ end
end
diff --git a/db/schema.rb b/db/schema.rb
index 082d681a176..292a9100d9c 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20160308212903) do
+ActiveRecord::Schema.define(version: 20160309140734) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
diff --git a/spec/features/security/group/internal_access_spec.rb b/spec/features/security/group/internal_access_spec.rb
index 69a0fbb4468..4e781c23ee0 100644
--- a/spec/features/security/group/internal_access_spec.rb
+++ b/spec/features/security/group/internal_access_spec.rb
@@ -4,8 +4,6 @@ describe 'Internal group access', feature: true do
include AccessMatchers
include GroupAccessHelper
-
-
describe 'GET /groups/:path' do
subject { group_path(group(Gitlab::VisibilityLevel::INTERNAL)) }
diff --git a/spec/services/groups/create_service_spec.rb b/spec/services/groups/create_service_spec.rb
new file mode 100644
index 00000000000..7dbc5297978
--- /dev/null
+++ b/spec/services/groups/create_service_spec.rb
@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+describe Groups::CreateService, services: true do
+ let!(:user) { create(:user) }
+ let!(:private_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PRIVATE) }
+ let!(:internal_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::INTERNAL) }
+ let!(:public_group) { create(:group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) }
+
+ describe "execute" do
+ let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::PUBLIC ) }
+ subject { service.execute }
+
+ context "create groups without restricted visibility level" do
+ it { is_expected.to be_truthy }
+ end
+
+ context "cannot create group with restricted visibility level" do
+ before { allow(current_application_settings).to receive(:restricted_visibility_levels).and_return([Gitlab::VisibilityLevel::PUBLIC]) }
+ it { is_expected.to be_falsy }
+ end
+ end
+end
--
GitLab