Syntax fixes and better tests for helper methods. Updated docs.group-specific-lfs-settings

11 files changed, 137 insertions, 26 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 304839c7e77..868d4af439a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -75,7 +75,6 @@ v 8.12.0 (unreleased)
   - Add last commit time to repo view (ClemMakesApps)
   - Fix accessibility and visibility of project list dropdown button !6140
   - Fix missing flash messages on service edit page (airatshigapov)
-  - Added project specific enable/disable setting for LFS !5997
   - Added project-specific enable/disable setting for LFS !5997
   - Added group-specific enable/disable setting for LFS !6164
   - Don't expose a user's token in the `/api/v3/user` API (!6047)
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index adb8a891c32..aed77d0358a 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -61,13 +61,13 @@ class Admin::GroupsController < Admin::ApplicationController
 
   def group_params
     params.require(:group).permit(
-      :name,
+      :avatar,
       :description,
+      :lfs_enabled,
+      :name,
       :path,
-      :avatar,
-      :visibility_level,
       :request_access_enabled,
-      :lfs_enabled
+      :visibility_level
     )
   end
 end
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 2f7113aa709..b83c3a872cf 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -122,15 +122,15 @@ class GroupsController < Groups::ApplicationController
 
   def group_params
     params.require(:group).permit(
-      :name,
+      :avatar,
       :description,
+      :lfs_enabled,
+      :name,
       :path,
-      :avatar,
       :public,
-      :visibility_level,
-      :share_with_group_lock,
       :request_access_enabled,
-      :lfs_enabled
+      :share_with_group_lock,
+      :visibility_level
     )
   end
 
diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb
index 76911efe354..ab880ed6de0 100644
--- a/app/helpers/groups_helper.rb
+++ b/app/helpers/groups_helper.rb
@@ -24,19 +24,20 @@ module GroupsHelper
     end
   end
 
-  def projects_with_lfs_enabled(group, status)
-    if status
-      lfs_status = group.projects.select(&:lfs_enabled?).size
-    else
-      lfs_status = group.projects.select{ |p| !p.lfs_enabled? }.size
-    end
+  def projects_lfs_status(group)
+    lfs_status =
+      if group.lfs_enabled?
+        group.projects.select(&:lfs_enabled?).size
+      else
+        group.projects.reject(&:lfs_enabled?).size
+      end
 
     size = group.projects.size
 
-    if lfs_status == size || lfs_status == 0
-      'on all projects'
+    if lfs_status == size
+      'for all projects'
     else
-      "on #{lfs_status} out of #{size} projects"
+      "for #{lfs_status} out of #{pluralize(size, 'project')}"
     end
   end
 
@@ -44,7 +45,7 @@ module GroupsHelper
     status = group.lfs_enabled? ? 'enabled' : 'disabled'
 
     content_tag(:span, class: "lfs-#{status}") do
-      "#{status.humanize} #{projects_with_lfs_enabled(group, group.lfs_enabled?)}"
+      "#{status.humanize} #{projects_lfs_status(group)}"
     end
   end
 end
diff --git a/app/views/admin/groups/_form.html.haml b/app/views/admin/groups/_form.html.haml
index cb631cbfb8d..817910f7ddf 100644
--- a/app/views/admin/groups/_form.html.haml
+++ b/app/views/admin/groups/_form.html.haml
@@ -13,7 +13,7 @@
     .col-sm-offset-2.col-sm-10
       = render 'shared/allow_request_access', form: f
 
-  = render 'shared/groups/group_lfs_settings', f: f
+  = render 'groups/group_lfs_settings', f: f
 
   - if @group.new_record?
     .form-group
diff --git a/app/views/shared/groups/_group_lfs_settings.html.haml b/app/views/groups/_group_lfs_settings.html.haml
index af57065f0fc..af57065f0fc 100644
--- a/app/views/shared/groups/_group_lfs_settings.html.haml
+++ b/app/views/groups/_group_lfs_settings.html.haml
diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml
index 849d0f360e7..c766370d5a0 100644
--- a/app/views/groups/edit.html.haml
+++ b/app/views/groups/edit.html.haml
@@ -25,7 +25,7 @@
         .col-sm-offset-2.col-sm-10
           = render 'shared/allow_request_access', form: f
 
-      = render 'shared/groups/group_lfs_settings', f: f
+      = render 'group_lfs_settings', f: f
 
       .form-group
         %hr
diff --git a/doc/api/groups.md b/doc/api/groups.md
index cb49648c36f..3e94e1e4efe 100644
--- a/doc/api/groups.md
+++ b/doc/api/groups.md
@@ -288,7 +288,7 @@ Parameters:
 - `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.
-- `lfs_enabled` (optional)      - Enable/disable LFS for the projects in this group
+- `lfs_enabled` (optional)      - Enable/disable Large File Storage (LFS) for the projects in this group
 
 ## Transfer project to group
 
@@ -318,7 +318,7 @@ PUT /groups/:id
 | `path` | string | no | The path of the group |
 | `description` | string | no | The description of the group |
 | `visibility_level` | integer | no | The visibility level of the group. 0 for private, 10 for internal, 20 for public. |
-| `lfs_enabled` (optional) | boolean | no | Enable/disable LFS for the projects in this group |
+| `lfs_enabled` (optional) | boolean | no | Enable/disable Large File Storage (LFS) for the projects in this group |
 
 ```bash
 curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/groups/5?name=Experimental"
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 1529a44f58d..bfee4b6c752 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -86,7 +86,8 @@ module API
       expose(:snippets_enabled) { |project, options| project.feature_available?(:snippets, options[:user]) }
 
       expose :created_at, :last_activity_at
-      expose :shared_runners_enabled, :lfs_enabled
+      expose :shared_runners_enabled
+      expose :lfs_enabled?, as: :lfs_enabled
       expose :creator_id
       expose :namespace
       expose :forked_from_project, using: Entities::BasicProjectDetails, if: lambda{ |project, options| project.forked? }
@@ -120,7 +121,8 @@ module API
     end
 
     class Group < Grape::Entity
-      expose :id, :name, :path, :description, :visibility_level, :lfs_enabled
+      expose :id, :name, :path, :description, :visibility_level
+      expose :lfs_enabled?, as: :lfs_enabled
       expose :avatar_url
       expose :web_url
     end
diff --git a/spec/helpers/groups_helper_spec.rb b/spec/helpers/groups_helper_spec.rb
index 0807534720a..233d00534e5 100644
--- a/spec/helpers/groups_helper_spec.rb
+++ b/spec/helpers/groups_helper_spec.rb
@@ -18,4 +18,67 @@ describe GroupsHelper do
       expect(group_icon(group.path)).to match('group_avatar.png')
     end
   end
+
+  describe 'group_lfs_status' do
+    let(:group) { create(:group) }
+    let!(:project) { create(:empty_project, namespace_id: group.id) }
+
+    before do
+      allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+    end
+
+    context 'only one project in group' do
+      before do
+        group.update_attribute(:lfs_enabled, true)
+      end
+
+      it 'returns all projects as enabled' do
+        expect(group_lfs_status(group)).to include('Enabled for all projects')
+      end
+
+      it 'returns all projects as disabled' do
+        project.update_attribute(:lfs_enabled, false)
+
+        expect(group_lfs_status(group)).to include('Enabled for 0 out of 1 project')
+      end
+    end
+
+    context 'more than one project in group' do
+      before do
+        create(:empty_project, namespace_id: group.id)
+      end
+
+      context 'LFS enabled in group' do
+        before do
+          group.update_attribute(:lfs_enabled, true)
+        end
+
+        it 'returns both projects as enabled' do
+          expect(group_lfs_status(group)).to include('Enabled for all projects')
+        end
+
+        it 'returns only one as enabled' do
+          project.update_attribute(:lfs_enabled, false)
+
+          expect(group_lfs_status(group)).to include('Enabled for 1 out of 2 projects')
+        end
+      end
+
+      context 'LFS disabled in group' do
+        before do
+          group.update_attribute(:lfs_enabled, false)
+        end
+
+        it 'returns both projects as disabled' do
+          expect(group_lfs_status(group)).to include('Disabled for all projects')
+        end
+
+        it 'returns only one as disabled' do
+          project.update_attribute(:lfs_enabled, true)
+
+          expect(group_lfs_status(group)).to include('Disabled for 1 out of 2 projects')
+        end
+      end
+    end
+  end
 end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index ea4b59c26b1..0b3ef9b98fd 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -187,6 +187,52 @@ describe Group, models: true do
     it { expect(group.has_master?(@members[:requester])).to be_falsey }
   end
 
+  describe '#lfs_enabled?' do
+    context 'LFS enabled globally' do
+      before do
+        allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+      end
+
+      it 'returns true when nothing is set' do
+        expect(group.lfs_enabled?).to be_truthy
+      end
+
+      it 'returns false when set to false' do
+        group.update_attribute(:lfs_enabled, false)
+
+        expect(group.lfs_enabled?).to be_falsey
+      end
+
+      it 'returns true when set to true' do
+        group.update_attribute(:lfs_enabled, true)
+
+        expect(group.lfs_enabled?).to be_truthy
+      end
+    end
+
+    context 'LFS disabled globally' do
+      before do
+        allow(Gitlab.config.lfs).to receive(:enabled).and_return(false)
+      end
+
+      it 'returns false when nothing is set' do
+        expect(group.lfs_enabled?).to be_falsey
+      end
+
+      it 'returns false when set to false' do
+        group.update_attribute(:lfs_enabled, false)
+
+        expect(group.lfs_enabled?).to be_falsey
+      end
+
+      it 'returns false when set to true' do
+        group.update_attribute(:lfs_enabled, true)
+
+        expect(group.lfs_enabled?).to be_falsey
+      end
+    end
+  end
+
   describe '#owners' do
     let(:owner) { create(:user) }
     let(:developer) { create(:user) }