Read about our upcoming Code of Conduct on this issue

Commit 6f03ddcd authored by Rubén Dávila's avatar Rubén Dávila
Browse files

Address some suggestions from first code review

parent 3ef9374c1d23
......@@ -3,7 +3,6 @@ class ProjectsController < Projects::ApplicationController
include ExtractsPath
before_action :authenticate_user!, except: [:index, :show, :activity, :refs]
before_action :namespace, only: [:new]
before_action :project, except: [:index, :new, :create]
before_action :repository, except: [:index, :new, :create]
before_action :assign_ref_vars, only: [:show], if: :repo_exists?
......@@ -21,7 +20,7 @@ def index
end
def new
build_project
@project ||= Project.new(params.permit(:namespace_id))
end
def edit
......@@ -396,12 +395,4 @@ def build_canonical_path(project)
def project_export_enabled
render_404 unless current_application_settings.project_export_enabled?
end
def namespace
@namespace ||= Namespace.find(params[:namespace_id]) if params[:namespace_id].present?
end
def build_project
@project ||= namespace ? namespace.projects.new : Project.new
end
end
......@@ -6,7 +6,6 @@ def namespace_id_from(params)
def namespaces_options(selected = :current_user, display_path: false, extra_group: nil)
groups = current_user.owned_groups + current_user.masters_groups
users = [current_user.namespace]
options = []
unless extra_group.nil? || extra_group.is_a?(Group)
extra_group = Group.find(extra_group) if Namespace.find(extra_group).kind == 'group'
......@@ -16,8 +15,9 @@ def namespaces_options(selected = :current_user, display_path: false, extra_grou
groups |= [extra_group]
end
options << options_for_group(groups, display_path)
options << options_for_group(users, display_path)
options = []
options << options_for_group(groups, display_path: display_path, type: 'group')
options << options_for_group(users, display_path: display_path, type: 'user')
if selected == :current_user && current_user.namespace
selected = current_user.namespace.id
......@@ -36,13 +36,12 @@ def namespace_icon(namespace, size = 40)
private
def options_for_group(namespaces, display_path)
type = namespaces.first.is_a?(Group) ? 'group' : 'users'
def options_for_group(namespaces, display_path:, type:)
group_label = type.pluralize
elements = namespaces.sort_by(&:human_name).map! do |n|
[display_path ? n.full_path : n.human_name, n.id,
data: {
options_parent: type,
options_parent: group_label,
visibility_level: n.visibility_level_value,
visibility: n.visibility,
name: n.name,
......@@ -51,6 +50,6 @@ def options_for_group(namespaces, display_path)
}]
end
[type.camelize, elements]
[group_label.camelize, elements]
end
end
......@@ -65,7 +65,7 @@ def snippet_visibility_level_description(level, snippet = nil)
def restricted_visibility_level_description(level)
level_name = Gitlab::VisibilityLevel.level_name(level)
"#{level_name.capitalize} visibilitiy has been restricted by the administrator."
"#{level_name.capitalize} visibility has been restricted by the administrator."
end
def disallowed_visibility_level_description(level, form_model)
......@@ -82,11 +82,11 @@ def disallowed_project_visibility_level_description(level, project)
reasons = []
unless project.visibility_level_allowed_as_fork?(level)
reasons << "the fork source project has lower visibility"
reasons << project.visibility_error_for(:fork, level: level_name)
end
unless project.visibility_level_allowed_by_group?(level)
reasons << "the visibility of #{project.group.name} is #{project.group.visibility}"
reasons << project.visibility_error_for(:group, level: level_name, group_level: project.group.visibility)
end
reasons = reasons.any? ? ' because ' + reasons.to_sentence : ''
......@@ -98,15 +98,15 @@ def disallowed_group_visibility_level_description(level, group)
reasons = []
unless group.visibility_level_allowed_by_projects?(level)
reasons << "it contains projects with higher visibility"
reasons << group.visibility_error_for(:projects, level: level_name)
end
unless group.visibility_level_allowed_by_sub_groups?(level)
reasons << "it contains sub-groups with higher visibility"
reasons << group.visibility_error_for(:sub_groups, level: level_name)
end
unless group.visibility_level_allowed_by_parent?(level)
reasons << "the visibility of its parent group is #{group.parent.visibility}"
reasons << group.visibility_error_for(:parent, level: level_name, parent_level: group.parent.visibility)
end
reasons = reasons.any? ? ' because ' + reasons.to_sentence : ''
......
......@@ -26,7 +26,7 @@ class Group < Namespace
validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
validate :visibility_level_allowed_by_projects
validate :visibility_level_allowed_by_sub_groups, if: :visibility_level_changed?
validate :visibility_level_allowed_by_sub_groups
validate :visibility_level_allowed_by_parent
validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
......@@ -104,24 +104,6 @@ def human_name
full_name
end
def visibility_level_allowed_by_parent
return if visibility_level_allowed_by_parent?
errors.add(:visibility_level, "#{visibility} is not allowed since the parent group has a #{parent.visibility} visibility.")
end
def visibility_level_allowed_by_projects
return if visibility_level_allowed_by_projects?
errors.add(:visibility_level, "#{visibility} is not allowed since this group contains projects with higher visibility.")
end
def visibility_level_allowed_by_sub_groups
return if visibility_level_allowed_by_sub_groups?
errors.add(:visibility_level, "#{visibility} is not allowed since there are sub-groups with higher visibility.")
end
def visibility_level_allowed_by_parent?(level = self.visibility_level)
return true unless parent_id && parent_id.nonzero?
......@@ -304,11 +286,29 @@ def secret_variables_for(ref, project)
list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten
end
protected
private
def update_two_factor_requirement
return unless require_two_factor_authentication_changed? || two_factor_grace_period_changed?
users.find_each(&:update_two_factor_requirement)
end
def visibility_level_allowed_by_parent
return if visibility_level_allowed_by_parent?
errors.add(:visibility_level, visibility_error_for(:parent, level: visibility, parent_level: parent.visibility))
end
def visibility_level_allowed_by_projects
return if visibility_level_allowed_by_projects?
errors.add(:visibility_level, visibility_error_for(:projects, level: visibility))
end
def visibility_level_allowed_by_sub_groups
return if visibility_level_allowed_by_sub_groups?
errors.add(:visibility_level, visibility_error_for(:sub_groups, level: visibility))
end
end
......@@ -671,14 +671,16 @@ def visibility_level_allowed_by_group
level_name = Gitlab::VisibilityLevel.level_name(self.visibility_level).downcase
group_level_name = Gitlab::VisibilityLevel.level_name(self.group.visibility_level).downcase
self.errors.add(:visibility_level, "#{level_name} is not allowed in a #{group_level_name} group.")
self.errors.add(:visibility_level, visibility_error_for(:group, level: level_name, group_level: group_level_name))
end
def visibility_level_allowed_as_fork
return if visibility_level_allowed_as_fork?
level_name = Gitlab::VisibilityLevel.level_name(self.visibility_level).downcase
self.errors.add(:visibility_level, "#{level_name} is not allowed since the fork source project has lower visibility.")
self.errors.add(:visibility_level, visibility_error_for(:fork, level: level_name))
end
def check_wiki_path_conflict
......
......@@ -130,5 +130,23 @@ def visibility
def visibility=(level)
self[visibility_level_field] = Gitlab::VisibilityLevel.level_value(level)
end
def visibility_errors_template
@visibility_errors ||= {
'Project' => {
group: "%{level} is not allowed in a %{group_level} group",
fork: "%{level} is not allowed since the fork source project has lower visibility"
},
'Group' => {
parent: "%{level} is not allowed since the parent group has a %{parent_level} visibility",
projects: "%{level} is not allowed since this group contains projects with higher visibility",
sub_groups: "%{level} is not allowed since there are sub-groups with higher visibility"
}
}
end
def visibility_error_for(section, variables)
visibility_errors_template.dig(model_name.to_s, section) % variables
end
end
end
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment