Address some suggestions from first code review
This commit is contained in:
parent
0a8d0924fe
commit
6f03ddcdc3
|
@ -3,7 +3,6 @@ class ProjectsController < Projects::ApplicationController
|
||||||
include ExtractsPath
|
include ExtractsPath
|
||||||
|
|
||||||
before_action :authenticate_user!, except: [:index, :show, :activity, :refs]
|
before_action :authenticate_user!, except: [:index, :show, :activity, :refs]
|
||||||
before_action :namespace, only: [:new]
|
|
||||||
before_action :project, except: [:index, :new, :create]
|
before_action :project, except: [:index, :new, :create]
|
||||||
before_action :repository, except: [:index, :new, :create]
|
before_action :repository, except: [:index, :new, :create]
|
||||||
before_action :assign_ref_vars, only: [:show], if: :repo_exists?
|
before_action :assign_ref_vars, only: [:show], if: :repo_exists?
|
||||||
|
@ -21,7 +20,7 @@ class ProjectsController < Projects::ApplicationController
|
||||||
end
|
end
|
||||||
|
|
||||||
def new
|
def new
|
||||||
build_project
|
@project ||= Project.new(params.permit(:namespace_id))
|
||||||
end
|
end
|
||||||
|
|
||||||
def edit
|
def edit
|
||||||
|
@ -396,12 +395,4 @@ class ProjectsController < Projects::ApplicationController
|
||||||
def project_export_enabled
|
def project_export_enabled
|
||||||
render_404 unless current_application_settings.project_export_enabled?
|
render_404 unless current_application_settings.project_export_enabled?
|
||||||
end
|
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
|
end
|
||||||
|
|
|
@ -6,7 +6,6 @@ module NamespacesHelper
|
||||||
def namespaces_options(selected = :current_user, display_path: false, extra_group: nil)
|
def namespaces_options(selected = :current_user, display_path: false, extra_group: nil)
|
||||||
groups = current_user.owned_groups + current_user.masters_groups
|
groups = current_user.owned_groups + current_user.masters_groups
|
||||||
users = [current_user.namespace]
|
users = [current_user.namespace]
|
||||||
options = []
|
|
||||||
|
|
||||||
unless extra_group.nil? || extra_group.is_a?(Group)
|
unless extra_group.nil? || extra_group.is_a?(Group)
|
||||||
extra_group = Group.find(extra_group) if Namespace.find(extra_group).kind == 'group'
|
extra_group = Group.find(extra_group) if Namespace.find(extra_group).kind == 'group'
|
||||||
|
@ -16,8 +15,9 @@ module NamespacesHelper
|
||||||
groups |= [extra_group]
|
groups |= [extra_group]
|
||||||
end
|
end
|
||||||
|
|
||||||
options << options_for_group(groups, display_path)
|
options = []
|
||||||
options << options_for_group(users, display_path)
|
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
|
if selected == :current_user && current_user.namespace
|
||||||
selected = current_user.namespace.id
|
selected = current_user.namespace.id
|
||||||
|
@ -36,13 +36,12 @@ module NamespacesHelper
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def options_for_group(namespaces, display_path)
|
def options_for_group(namespaces, display_path:, type:)
|
||||||
type = namespaces.first.is_a?(Group) ? 'group' : 'users'
|
group_label = type.pluralize
|
||||||
|
|
||||||
elements = namespaces.sort_by(&:human_name).map! do |n|
|
elements = namespaces.sort_by(&:human_name).map! do |n|
|
||||||
[display_path ? n.full_path : n.human_name, n.id,
|
[display_path ? n.full_path : n.human_name, n.id,
|
||||||
data: {
|
data: {
|
||||||
options_parent: type,
|
options_parent: group_label,
|
||||||
visibility_level: n.visibility_level_value,
|
visibility_level: n.visibility_level_value,
|
||||||
visibility: n.visibility,
|
visibility: n.visibility,
|
||||||
name: n.name,
|
name: n.name,
|
||||||
|
@ -51,6 +50,6 @@ module NamespacesHelper
|
||||||
}]
|
}]
|
||||||
end
|
end
|
||||||
|
|
||||||
[type.camelize, elements]
|
[group_label.camelize, elements]
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -65,7 +65,7 @@ module VisibilityLevelHelper
|
||||||
|
|
||||||
def restricted_visibility_level_description(level)
|
def restricted_visibility_level_description(level)
|
||||||
level_name = Gitlab::VisibilityLevel.level_name(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
|
end
|
||||||
|
|
||||||
def disallowed_visibility_level_description(level, form_model)
|
def disallowed_visibility_level_description(level, form_model)
|
||||||
|
@ -82,11 +82,11 @@ module VisibilityLevelHelper
|
||||||
reasons = []
|
reasons = []
|
||||||
|
|
||||||
unless project.visibility_level_allowed_as_fork?(level)
|
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
|
end
|
||||||
|
|
||||||
unless project.visibility_level_allowed_by_group?(level)
|
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
|
end
|
||||||
|
|
||||||
reasons = reasons.any? ? ' because ' + reasons.to_sentence : ''
|
reasons = reasons.any? ? ' because ' + reasons.to_sentence : ''
|
||||||
|
@ -98,15 +98,15 @@ module VisibilityLevelHelper
|
||||||
reasons = []
|
reasons = []
|
||||||
|
|
||||||
unless group.visibility_level_allowed_by_projects?(level)
|
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
|
end
|
||||||
|
|
||||||
unless group.visibility_level_allowed_by_sub_groups?(level)
|
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
|
end
|
||||||
|
|
||||||
unless group.visibility_level_allowed_by_parent?(level)
|
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
|
end
|
||||||
|
|
||||||
reasons = reasons.any? ? ' because ' + reasons.to_sentence : ''
|
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 :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
|
||||||
validate :visibility_level_allowed_by_projects
|
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
|
validate :visibility_level_allowed_by_parent
|
||||||
|
|
||||||
validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
|
validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
|
||||||
|
@ -104,24 +104,6 @@ class Group < Namespace
|
||||||
full_name
|
full_name
|
||||||
end
|
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)
|
def visibility_level_allowed_by_parent?(level = self.visibility_level)
|
||||||
return true unless parent_id && parent_id.nonzero?
|
return true unless parent_id && parent_id.nonzero?
|
||||||
|
|
||||||
|
@ -304,11 +286,29 @@ class Group < Namespace
|
||||||
list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten
|
list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten
|
||||||
end
|
end
|
||||||
|
|
||||||
protected
|
private
|
||||||
|
|
||||||
def update_two_factor_requirement
|
def update_two_factor_requirement
|
||||||
return unless require_two_factor_authentication_changed? || two_factor_grace_period_changed?
|
return unless require_two_factor_authentication_changed? || two_factor_grace_period_changed?
|
||||||
|
|
||||||
users.find_each(&:update_two_factor_requirement)
|
users.find_each(&:update_two_factor_requirement)
|
||||||
end
|
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
|
end
|
||||||
|
|
|
@ -671,14 +671,16 @@ class Project < ActiveRecord::Base
|
||||||
|
|
||||||
level_name = Gitlab::VisibilityLevel.level_name(self.visibility_level).downcase
|
level_name = Gitlab::VisibilityLevel.level_name(self.visibility_level).downcase
|
||||||
group_level_name = Gitlab::VisibilityLevel.level_name(self.group.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
|
end
|
||||||
|
|
||||||
def visibility_level_allowed_as_fork
|
def visibility_level_allowed_as_fork
|
||||||
return if visibility_level_allowed_as_fork?
|
return if visibility_level_allowed_as_fork?
|
||||||
|
|
||||||
level_name = Gitlab::VisibilityLevel.level_name(self.visibility_level).downcase
|
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
|
end
|
||||||
|
|
||||||
def check_wiki_path_conflict
|
def check_wiki_path_conflict
|
||||||
|
|
|
@ -130,5 +130,23 @@ module Gitlab
|
||||||
def visibility=(level)
|
def visibility=(level)
|
||||||
self[visibility_level_field] = Gitlab::VisibilityLevel.level_value(level)
|
self[visibility_level_field] = Gitlab::VisibilityLevel.level_value(level)
|
||||||
end
|
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
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in New Issue