Address feedback

13 files changed, 38 insertions, 36 deletions

diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index 9c8433c260b..657ee94cfd7 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -1,6 +1,7 @@
 class Projects::ApplicationController < ApplicationController
   skip_before_action :authenticate_user!
-  before_action :project, :repository
+  before_action :project
+  before_action :repository
   layout 'project'
 
   helper_method :repository, :can_collaborate_with_project?
diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb
index 94c51eeb94d..caed064dfbc 100644
--- a/app/controllers/projects/uploads_controller.rb
+++ b/app/controllers/projects/uploads_controller.rb
@@ -2,7 +2,7 @@ class Projects::UploadsController < Projects::ApplicationController
   skip_before_action :reject_blocked!, :project,
     :repository, if: -> { action_name == 'show' && image? }
 
-  before_action :authenticate_user!, only: [:create]
+  before_action :authorize_upload_file!, only: [:create]
 
   def create
     link_to_file = ::Projects::UploadService.new(project, params[:file]).
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index dd4208880b6..046286dd9e1 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -171,15 +171,13 @@ class IssuableFinder
   end
 
   def by_scope(items)
-    case params[:scope] || 'all'
-    when 'created-by-me', 'authored' then
+    case params[:scope]
+    when 'created-by-me', 'authored'
       items.where(author_id: current_user.id)
-    when 'all' then
-      items
-    when 'assigned-to-me' then
+    when 'assigned-to-me'
       items.where(assignee_id: current_user.id)
     else
-      raise 'You must specify default scope'
+      items
     end
   end
 
diff --git a/app/finders/joined_groups_finder.rb b/app/finders/joined_groups_finder.rb
index 2a3f0296d37..47174980258 100644
--- a/app/finders/joined_groups_finder.rb
+++ b/app/finders/joined_groups_finder.rb
@@ -5,11 +5,6 @@ class JoinedGroupsFinder < UnionFinder
 
   # Finds the groups of the source user, optionally limited to those visible to
   # the current user.
-  #
-  # current_user - If given the groups of "@user" will only include the groups
-  #                "current_user" can also see.
-  #
-  # Returns an ActiveRecord::Relation.
   def execute(current_user = nil)
     segments = all_groups(current_user)
 
diff --git a/app/helpers/visibility_level_helper.rb b/app/helpers/visibility_level_helper.rb
index 5b1bfb261a5..3a83ae15dd8 100644
--- a/app/helpers/visibility_level_helper.rb
+++ b/app/helpers/visibility_level_helper.rb
@@ -40,11 +40,11 @@ module VisibilityLevelHelper
   def group_visibility_level_description(level)
     case level
     when Gitlab::VisibilityLevel::PRIVATE
-      "The group can be accessed only by members."
+      "The group and its projects can only be viewed by members."
     when Gitlab::VisibilityLevel::INTERNAL
-      "The group can be accessed by any logged user."
+      "The group and any internal projects can be viewed by any logged in user."
     when Gitlab::VisibilityLevel::PUBLIC
-      "The group can be accessed without any authentication."
+      "The group and any public projects can be viewed without any authentication."
     end
   end
 
@@ -63,12 +63,21 @@ module VisibilityLevelHelper
     end
   end
 
-  def group_visibility_icon_description(group)
-    "#{visibility_level_label(group.visibility_level)} - #{group_visibility_level_description(group.visibility_level)}"
+  def visibility_icon_description(form_model)
+    case form_model
+    when Project
+      project_visibility_icon_description(form_model.visibility_level)
+    when Group
+      group_visibility_icon_description(form_model.visibility_level)
+    end
+  end
+
+  def group_visibility_icon_description(level)
+    "#{visibility_level_label(level)} - #{group_visibility_level_description(level)}"
   end
 
-  def project_visibility_icon_description(project)
-    "#{visibility_level_label(project.visibility_level)} - #{project_visibility_level_description(project.visibility_level)}"
+  def project_visibility_icon_description(level)
+    "#{visibility_level_label(level)} - #{project_visibility_level_description(level)}"
   end
 
   def visibility_level_label(level)
diff --git a/app/models/ability.rb b/app/models/ability.rb
index 42b978e04d5..fa2345f6faa 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -170,7 +170,8 @@ class Ability
         :read_note,
         :create_project,
         :create_issue,
-        :create_note
+        :create_note,
+        :upload_file
       ]
     end
 
@@ -298,8 +299,12 @@ class Ability
     end
 
     def can_read_group?(user, group)
-      user.admin? || group.public? || (group.internal? && !user.external?) || group.users.include?(user) ||
-        GroupProjectsFinder.new(group).execute(user).any?
+      return true if user.admin?
+      return true if group.public?
+      return true if group.internal? && !user.external?
+      return true if group.users.include?(user)
+
+      GroupProjectsFinder.new(group).execute(user).any?
     end
 
     def namespace_abilities(user, namespace)
diff --git a/app/models/group.rb b/app/models/group.rb
index 900fcd71ff3..b332601c59b 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -7,7 +7,7 @@
 #  path                   :string(255)      not null
 #  owner_id               :integer
 #  visibility_level       :integer          default(20), not null
-#  created_at             :key => "value", datetime
+#  created_at             :datetime
 #  updated_at             :datetime
 #  type                   :string(255)
 #  description            :string(255)      default(""), not null
@@ -83,9 +83,7 @@ class Group < Namespace
   end
 
   def visibility_level_allowed_by_projects
-    projects_visibility = self.projects.pluck(:visibility_level)
-
-    allowed_by_projects = projects_visibility.all? { |project_visibility| self.visibility_level >= project_visibility }
+    allowed_by_projects = self.projects.where('visibility_level > ?', self.visibility_level).none?
 
     unless allowed_by_projects
       level_name = Gitlab::VisibilityLevel.level_name(visibility_level).downcase
diff --git a/app/services/groups/create_service.rb b/app/services/groups/create_service.rb
index 46c2a53e1f6..2bccd584dde 100644
--- a/app/services/groups/create_service.rb
+++ b/app/services/groups/create_service.rb
@@ -12,7 +12,7 @@ module Groups
         return @group
       end
 
-      @group.name = @group.path.dup unless @group.name
+      @group.name ||= @group.path.dup
       @group.save
       @group.add_owner(current_user)
       @group
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index b70e2e4aaa9..99ad12b1003 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -1,7 +1,3 @@
-# Checks visibility level permission check before updating a group
-# Do not allow to put Group visibility level smaller than its projects
-# Do not allow unauthorized permission levels
-
 module Groups
   class UpdateService < Groups::BaseService
     def execute
diff --git a/app/views/groups/show.html.haml b/app/views/groups/show.html.haml
index 5a9fa5d9a4d..820743dc8dd 100644
--- a/app/views/groups/show.html.haml
+++ b/app/views/groups/show.html.haml
@@ -17,7 +17,7 @@
   .cover-title
     %h1
       = @group.name
-      %span.visibility-icon.has_tooltip{ data: { container: 'body' }, title: group_visibility_icon_description(@group) }
+      %span.visibility-icon.has_tooltip{ data: { container: 'body' }, title: visibility_icon_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 d4bbafbd40f..514cbfa339d 100644
--- a/app/views/projects/_home_panel.html.haml
+++ b/app/views/projects/_home_panel.html.haml
@@ -5,7 +5,7 @@
   .cover-title.project-home-desc
     %h1
       = @project.name
-      %span.visibility-icon.has_tooltip{data: { container: 'body' }, title: project_visibility_icon_description(@project)}
+      %span.visibility-icon.has_tooltip{data: { container: 'body' }, title: visibility_icon_description(@project)}
         = visibility_level_icon(@project.visibility_level, fw: false)
 
   - if @project.description.present?
diff --git a/app/views/shared/groups/_group.html.haml b/app/views/shared/groups/_group.html.haml
index db416b9d91a..66b7ef99650 100644
--- a/app/views/shared/groups/_group.html.haml
+++ b/app/views/shared/groups/_group.html.haml
@@ -21,7 +21,7 @@
       = icon('users')
       = number_with_delimiter(group.users.count)
 
-    %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: group_visibility_icon_description(group)}
+    %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: visibility_icon_description(group)}
       = visibility_level_icon(group.visibility_level, fw: false)
 
   = image_tag group_icon(group), class: "avatar s40 hidden-xs"
diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml
index 3b987987676..803dd95bc65 100644
--- a/app/views/shared/projects/_project.html.haml
+++ b/app/views/shared/projects/_project.html.haml
@@ -27,7 +27,7 @@
         %span
           = icon('star')
           = project.star_count
-      %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: project_visibility_icon_description(project)}
+      %span.visibility-icon.has_tooltip{data: { container: 'body', placement: 'left' }, title: visibility_icon_description(project)}
         = visibility_level_icon(project.visibility_level, fw: false)
 
     .title