Tweaks, refactoring, and specs

app/helpers/visibility_level_helper.rb

@@ -63,6 +63,14 @@ module VisibilityLevelHelper
     end
   end
+  def group_visibility_icon_description(group)
+    "#{visibility_level_label(group.visibility_level)} - #{group_visibility_level_description(group.visibility_level)}"
+  end
+
+  def project_visibility_icon_description(project)
+    "#{visibility_level_label(project.visibility_level)} - #{project_visibility_level_description(project.visibility_level)}"
+  end
+
   def visibility_level_label(level)
     Project.visibility_levels.key(level)
   end

app/models/ability.rb

@@ -114,6 +114,13 @@ class Ability
         # Push abilities on the users team role
         rules.push(*project_team_rules(project.team, user))
+        if project.owner == user ||
+          (project.group && project.group.has_owner?(user)) ||
+          user.admin?
+
+          rules.push(*project_owner_rules)
+        end
+
         if project.public? || (project.internal? && !user.external?)
           rules.push(*public_project_rules)
@@ -121,14 +128,6 @@ class Ability
           rules << :read_build if project.public_builds?
         end
-        if project.owner == user || user.admin?
-          rules.push(*project_admin_rules)
-        end
-
-        if project.group && project.group.has_owner?(user)
-          rules.push(*project_admin_rules)
-        end
-
         if project.archived?
           rules -= project_archived_rules
         end
@@ -228,8 +227,8 @@ class Ability
       ]
     end
-    def project_admin_rules
-      @project_admin_rules ||= project_master_rules + [
+    def project_owner_rules
+      @project_owner_rules ||= project_master_rules + [
         :change_namespace,
         :change_visibility_level,
         :rename_project,
@@ -275,7 +274,7 @@ class Ability
       rules << :read_group if can_read_group?(user, group)
-      # Only group masters and group owners can create new projects and change permission level
+      # Only group masters and group owners can create new projects
       if group.has_master?(user) || group.has_owner?(user) || user.admin?
         rules += [
           :create_projects,
@@ -298,7 +297,7 @@ class Ability
     def can_read_group?(user, group)
       user.admin? || group.public? || (group.internal? && !user.external?) || group.users.include?(user) ||
-        ProjectsFinder.new.execute(user, group: group).any?
+        GroupProjectsFinder.new(group).execute(user).any?
     end
     def namespace_abilities(user, namespace)

app/models/group.rb

@@ -83,16 +83,9 @@ class Group < Namespace
   end
   def visibility_level_allowed_by_projects
-    unless visibility_level_allowed?
-      level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase
-      self.errors.add(:visibility_level, "#{level_name} is not allowed since there are projects with higher visibility.")
-    end
-  end
-
-  def visibility_level_allowed?
     projects_visibility = self.projects.pluck(:visibility_level)
-    allowed_by_projects = projects_visibility.none? { |project_visibility| self.visibility_level < project_visibility }
+    allowed_by_projects = projects_visibility.all? { |project_visibility| self.visibility_level >= project_visibility }
     unless allowed_by_projects
       level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase

app/models/project.rb

@@ -197,7 +197,8 @@ class Project < ActiveRecord::Base
   validate :avatar_type,
     if: ->(project) { project.avatar.present? && project.avatar_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
-  validate :visibility_level_allowed_in_group
+  validate :visibility_level_allowed_by_group
+  validate :visibility_level_allowed_as_fork
   add_authentication_token_field :runners_token
   before_save :ensure_runners_token
@@ -441,16 +442,25 @@ class Project < ActiveRecord::Base
   def check_limit
     unless creator.can_create_project? or namespace.kind == 'group'
-      errors[:limit_reached] << ("Your project limit is #{creator.projects_limit} projects! Please contact your administrator to increase it")
+      self.errors.add(:limit_reached, "Your project limit is #{creator.projects_limit} projects! Please contact your administrator to increase it")
     end
   rescue
-    errors[:base] << ("Can't check your ability to create project")
+    self.errors.add(:base, "Can't check your ability to create project")
   end
-  def visibility_level_allowed_in_group
-    unless visibility_level_allowed?
-      self.errors.add(:visibility_level, "#{self.visibility_level} is not allowed in a #{self.group.visibility_level} group.")
-    end
+  def visibility_level_allowed_by_group
+    return if 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.")
+  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.")
   end
   def to_param
@@ -965,22 +975,22 @@ class Project < ActiveRecord::Base
     issues.opened.count
   end
-  def visibility_level_allowed?(level = self.visibility_level)
-    allowed_by_forks =  if forked? && forked_project_link.forked_from_project_id.present?
-                          from_project = eager_load_forked_from_project
-                          Gitlab::VisibilityLevel.allowed_fork_levels(from_project.visibility_level).include?(level)
-                        else
-                          true
-                        end
-    allowed_by_groups = group.present? ? level <= group.visibility_level : true
-    allowed_by_forks && allowed_by_groups
+  def visibility_level_allowed_as_fork?(level = self.visibility_level)
+    return true unless forked? && forked_project_link.forked_from_project_id.present?
+
+    from_project = self.forked_from_project
+    from_project ||= Project.find(forked_project_link.forked_from_project_id)
+    Gitlab::VisibilityLevel.allowed_fork_levels(from_project.visibility_level).include?(level)
+  end
+  def visibility_level_allowed_by_group?(level = self.visibility_level)
+    return true unless group
+    level <= group.visibility_level
   end
-  #Necessary to retrieve many-to-many associations on new forks before validating visibility level
-  def eager_load_forked_from_project
-    Project.find(forked_project_link.forked_from_project_id)
+  def visibility_level_allowed?(level = self.visibility_level)
+    visibility_level_allowed_as_fork?(level) && visibility_level_allowed_by_group?(level)
   end
   def runners_token

app/services/base_service.rb

@@ -43,12 +43,9 @@ class BaseService
   def deny_visibility_level(model, denied_visibility_level = nil)
     denied_visibility_level ||= model.visibility_level
-    level_name = Gitlab::VisibilityLevel.level_name(denied_visibility_level)
-    model.errors.add(
-      :visibility_level,
-      "#{level_name} visibility has been restricted by your GitLab administrator"
-    )
+    level_name = Gitlab::VisibilityLevel.level_name(denied_visibility_level).downcase
+    model.errors.add(:visibility_level, "#{level_name} has been restricted by your GitLab administrator")
   end
   private

app/services/create_snippet_service.rb

@@ -6,8 +6,7 @@ class CreateSnippetService < BaseService
       snippet = project.snippets.build(params)
     end
-    unless Gitlab::VisibilityLevel.allowed_for?(current_user,
-                                                params[:visibility_level])
+    unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
       deny_visibility_level(snippet)
       return snippet
     end

app/services/groups/base_service.rb

 module Groups
-  class BaseService
+  class BaseService < BaseService
     attr_accessor :group, :current_user, :params
     def initialize(group, user, params = {})
       @group, @current_user, @params = group, user, params.dup
     end
-
-    private
-
-    def visibility_allowed_for_user?
-      level = group.visibility_level
-      allowed_by_user  = Gitlab::VisibilityLevel.allowed_for?(current_user, level)
-
-      group.errors.add(:visibility_level, "#{level} has been restricted by your GitLab administrator.") unless allowed_by_user
-      
-      allowed_by_user
-    end
   end
 end

app/services/groups/create_service.rb

@@ -7,7 +7,10 @@ module Groups
     def execute
       @group = Group.new(params)
-      return @group unless visibility_allowed_for_user?
+      unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
+        deny_visibility_level(@group)
+        return @group
+      end
       @group.name = @group.path.dup unless @group.name
       @group.save

app/services/groups/update_service.rb

@@ -5,9 +5,18 @@
 module Groups
   class UpdateService < Groups::BaseService
     def execute
-      group.assign_attributes(params)
-      return false unless visibility_allowed_for_user?
+      # check that user is allowed to set specified visibility_level
+      new_visibility = params[:visibility_level]
+      if new_visibility && new_visibility.to_i != group.visibility_level
+        unless can?(current_user, :change_visibility_level, group) &&
+          Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
+
+          deny_visibility_level(group, new_visibility)
+          return group
+        end
+      end
+      group.assign_attributes(params)
       group.save
     end

app/services/projects/create_service.rb

@@ -10,7 +10,7 @@ module Projects
       @project = Project.new(params)
       # Make sure that the user is allowed to use the specified visibility level
-      unless visibility_level_allowed?
+      unless Gitlab::VisibilityLevel.allowed_for?(current_user, params[:visibility_level])
         deny_visibility_level(@project)
         return @project
       end
@@ -96,9 +96,5 @@ 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

app/services/projects/update_service.rb

@@ -3,16 +3,13 @@ module Projects
     def execute
       # check that user is allowed to set specified visibility_level
       new_visibility = params[:visibility_level]
-      if new_visibility
-        if new_visibility.to_i != project.visibility_level
-          unless can?(current_user, :change_visibility_level, project) &&
-            Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
-            deny_visibility_level(project, new_visibility)
-            return project
-          end
+      if new_visibility && new_visibility.to_i != project.visibility_level
+        unless can?(current_user, :change_visibility_level, project) &&
+          Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
+          
+          deny_visibility_level(project, new_visibility)
+          return project
         end
-
-        return false unless visibility_level_allowed?(new_visibility)
       end
       new_branch = params[:default_branch]
@@ -27,19 +24,5 @@ module Projects
         end
       end
     end
-
-    private
-
-    def visibility_level_allowed?(level)
-      return true if project.visibility_level_allowed?(level)
-
-      level_name = Gitlab::VisibilityLevel.level_name(level)
-      project.errors.add(
-        :visibility_level,
-        "#{level_name} could not be set as visibility level of this project - parent project settings are more restrictive"
-      )
-
-      false
-    end
   end
 end

app/services/update_snippet_service.rb

@@ -9,7 +9,6 @@ class UpdateSnippetService < BaseService
   def execute
     # check that user is allowed to set specified visibility_level
     new_visibility = params[:visibility_level]
-
     if new_visibility && new_visibility.to_i != snippet.visibility_level
       unless Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility)
         deny_visibility_level(snippet, new_visibility)

app/views/groups/show.html.haml

@@ -17,7 +17,7 @@
   .cover-title
     %h1
       = @group.name
-      %span.visibility-icon.has_tooltip{ data: { container: 'body', placement: 'left' }, title: group_visibility_description(@group) }
+      %span.visibility-icon.has_tooltip{ data: { container: 'body' }, title: group_visibility_icon_description(@group) }
         = visibility_level_icon(@group.visibility_level, fw: false)
   .cover-desc.username
@@ -27,34 +27,29 @@
     .cover-desc.description
       = markdown(@group.description, pipeline: :description)
-- if can?(current_user, :read_group, @group)
-  %div{ class: container_class }
-    .top-area
-      %ul.nav-links
-        %li.active
-          = link_to "#projects", 'data-toggle' => 'tab' do
-            All Projects
-        - if @shared_projects.present?
-          %li
-            = link_to "#shared", 'data-toggle' => 'tab' do
-              Shared Projects
-      .nav-controls
-        = form_tag request.original_url, method: :get, class: 'project-filter-form', id: 'project-filter-form' do |f|
-          = search_field_tag :filter_projects, nil, placeholder: 'Filter by name', class: 'projects-list-filter form-control', spellcheck: false
-        = render 'shared/projects/dropdown'
-        - if can? current_user, :create_projects, @group
-          = link_to new_project_path(namespace_id: @group.id), class: 'btn btn-new pull-right' do
-            = icon('plus')
-            New Project
-
-    .tab-content
-      .tab-pane.active#projects
-        = render "projects", projects: @projects
-
+%div{ class: container_class }
+  .top-area
+    %ul.nav-links
+      %li.active
+        = link_to "#projects", 'data-toggle' => 'tab' do
+          All Projects
       - if @shared_projects.present?
-        .tab-pane#shared
-          = render "shared_projects", projects: @shared_projects
-
-- else
-  %p.nav-links.no-top
-    No projects to show
+        %li
+          = link_to "#shared", 'data-toggle' => 'tab' do
+            Shared Projects
+    .nav-controls
+      = form_tag request.original_url, method: :get, class: 'project-filter-form', id: 'project-filter-form' do |f|
+        = search_field_tag :filter_projects, nil, placeholder: 'Filter by name', class: 'projects-list-filter form-control', spellcheck: false
+      = render 'shared/projects/dropdown'
+      - if can? current_user, :create_projects, @group
+        = link_to new_project_path(namespace_id: @group.id), class: 'btn btn-new pull-right' do
+          = icon('plus')
+          New Project
+
+  .tab-content
+    .tab-pane.active#projects
+      = render "projects", projects: @projects
+
+    - if @shared_projects.present?
+      .tab-pane#shared
+        = render "shared_projects", projects: @shared_projects

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

@@ -12,40 +12,38 @@
       = icon('group fw')
       %span
         Group
-  - if can?(current_user, :read_group, @group)
-    = nav_link(path: 'groups#activity') do
-      = link_to activity_group_path(@group), title: 'Activity' do
-        = icon('dashboard fw')
-        %span
-          Activity
-    - if current_user
-      = nav_link(controller: [:group, :milestones]) do
-        = link_to group_milestones_path(@group), title: 'Milestones' do
-          = icon('clock-o fw')
-          %span
-            Milestones
-    = nav_link(path: 'groups#issues') do
-      = link_to issues_group_path(@group), title: 'Issues' do
-        = icon('exclamation-circle fw')
-        %span
-          Issues
-          - if current_user
-            %span.count= number_with_delimiter(Issue.opened.of_group(@group).count)
-    = nav_link(path: 'groups#merge_requests') do
-      = link_to merge_requests_group_path(@group), title: 'Merge Requests' do
-        = icon('tasks fw')
-        %span
-          Merge Requests
-          - if current_user
-            %span.count= number_with_delimiter(MergeRequest.opened.of_group(@group).count)
-    = nav_link(controller: [:group_members]) do
-      = link_to group_group_members_path(@group), title: 'Members' do
-        = icon('users fw')
+  = nav_link(path: 'groups#activity') do
+    = link_to activity_group_path(@group), title: 'Activity' do
+      = icon('dashboard fw')
+      %span
+        Activity
+  = nav_link(controller: [:group, :milestones]) do
+    = link_to group_milestones_path(@group), title: 'Milestones' do
+      = icon('clock-o fw')
+      %span
+        Milestones
+  = nav_link(path: 'groups#issues') do
+    = link_to issues_group_path(@group), title: 'Issues' do
+      = icon('exclamation-circle fw')
+      %span
+        Issues
+        - issues = IssuesFinder.new(current_user, group_id: @group.id, state: 'opened').execute
+        %span.count= number_with_delimiter(issues.count)
+  = nav_link(path: 'groups#merge_requests') do
+    = link_to merge_requests_group_path(@group), title: 'Merge Requests' do
+      = icon('tasks fw')
+      %span
+        Merge Requests
+        - merge_requests = MergeRequestsFinder.new(current_user, group_id: @group.id, state: 'opened').execute
+        %span.count= number_with_delimiter(merge_requests.count)
+  = nav_link(controller: [:group_members]) do
+    = link_to group_group_members_path(@group), title: 'Members' do
+      = icon('users fw')
+      %span
+        Members
+  - if can?(current_user, :admin_group, @group)
+    = nav_link(html_options: { class: "separate-item" }) do
+      = link_to edit_group_path(@group), title: 'Settings' do
+        = icon ('cogs fw')
         %span
-          Members
-    - if can?(current_user, :admin_group, @group)
-      = nav_link(html_options: { class: "separate-item" }) do
-        = link_to edit_group_path(@group), title: 'Settings' do
-          = icon ('cogs fw')
-          %span
-            Settings
+          Settings

app/views/projects/_home_panel.html.haml

@@ -2,21 +2,21 @@
 .project-home-panel.cover-block.clearfix{:class => ("empty-project" if empty_repo)}
   .project-identicon-holder
     = project_icon(@project, alt: '', class: 'project-avatar avatar s90')
-  .cover-title#project-home-desc
+  .cover-title.project-home-desc
     %h1
       = @project.name
-      %span.visibility-icon.has_tooltip{data: { container: 'body' },
-        title: "#{visibility_level_label(@project.visibility_level)} - #{project_visibility_level_description(@project.visibility_level)}"}
+      %span.visibility-icon.has_tooltip{data: { container: 'body' }, title: project_visibility_icon_description(@project)}
         = visibility_level_icon(@project.visibility_level, fw: false)
-    - if @project.description.present?
+  - if @project.description.present?
+    .cover-desc.project-home-desc
       = markdown(@project.description, pipeline: :description)
-    - if forked_from_project = @project.forked_from_project
-      %p
-        Forked from
-        = link_to project_path(forked_from_project) do
-          = forked_from_project.namespace.try(:name)
+  - if forked_from_project = @project.forked_from_project
+    .cover-desc
+      Forked from
+      = link_to project_path(forked_from_project) do
+        = forked_from_project.namespace.try(:name)
   .cover-controls
     - if current_user

app/views/shared/groups/_group.html.haml

@@ -21,7 +21,7 @@
       = icon('users')
       = number_with_delimiter(group.users.count)
-    %span{title: group_visibility_description(group)}
+    %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: group_visibility_icon_description(group)}
       = visibility_level_icon(group.visibility_level, fw: false)
   = image_tag group_icon(group), class: "avatar s40 hidden-xs"

app/views/shared/projects/_project.html.haml

@@ -27,8 +27,7 @@
         %span
           = icon('star')
           = project.star_count
-      %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' },
-        title: "#{visibility_level_label(project.visibility_level)} - #{project_visibility_level_description(project.visibility_level)}"}
+      %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: project_visibility_icon_description(project)}
         = visibility_level_icon(project.visibility_level, fw: false)
     .title

db/migrate/20160308212903_add_default_group_visibility_to_application_settings.rb

@@ -5,7 +5,7 @@
 class AddDefaultGroupVisibilityToApplicationSettings < ActiveRecord::Migration
   def up
     add_column :application_settings, :default_group_visibility, :integer
-    execute("update application_settings set default_group_visibility = #{allowed_visibility_level}")
+    execute("UPDATE application_settings SET default_group_visibility = #{allowed_visibility_level}")
   end
   def down
@@ -15,6 +15,7 @@ class AddDefaultGroupVisibilityToApplicationSettings < ActiveRecord::Migration
   private
   def allowed_visibility_level
+    # TODO: Don't use `current_application_settings`
     allowed_levels = Gitlab::VisibilityLevel.values - current_application_settings.restricted_visibility_levels
     allowed_levels.max
   end

db/schema.rb

@@ -590,8 +590,8 @@ ActiveRecord::Schema.define(version: 20160316204731) do
     t.string   "type"
     t.string   "description",           default: "",    null: false
     t.string   "avatar"
-    t.integer  "visibility_level",      default: 0,     null: false
     t.boolean  "share_with_group_lock", default: false
+    t.integer  "visibility_level",      default: 20,    null: false
   end
   add_index "namespaces", ["created_at", "id"], name: "index_namespaces_on_created_at_and_id", using: :btree

doc/api/groups.md

@@ -111,6 +111,7 @@ Parameters:
 - `name` (required) - The name of the group
 - `path` (required) - The path of the group
 - `description` (optional) - The group's description
+- `visibility_level` (optional) - The group's visibility. 0 for private, 10 for internal, 20 for public.
 
 ## Transfer project to group