From fd621429508ba3877e27a8187f0d491576b65ad0 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Thu, 1 Sep 2016 18:49:48 -0500 Subject: [PATCH 1/5] Added group-specific setting for LFS. Groups can enable/disable LFS, but this setting can be overridden at the project level. Admin only --- app/controllers/admin/groups_controller.rb | 10 ++- app/controllers/groups_controller.rb | 12 +++- app/helpers/groups_helper.rb | 5 ++ app/helpers/projects_helper.rb | 4 +- app/models/group.rb | 7 ++ app/models/namespace.rb | 5 ++ app/models/project.rb | 9 ++- app/views/admin/groups/_form.html.haml | 2 + app/views/admin/groups/show.html.haml | 7 ++ app/views/admin/projects/show.html.haml | 2 +- app/views/groups/edit.html.haml | 2 + app/views/shared/_visibility_level.html.haml | 2 +- .../groups/_group_lfs_settings.html.haml | 11 ++++ ...901213340_add_lfs_enabled_to_namespaces.rb | 29 ++++++++ db/schema.rb | 1 + doc/api/groups.md | 2 + lib/api/entities.rb | 2 +- lib/api/groups.rb | 6 +- spec/models/project_spec.rb | 66 +++++++++++++++++++ 19 files changed, 172 insertions(+), 12 deletions(-) create mode 100644 app/views/shared/groups/_group_lfs_settings.html.haml create mode 100644 db/migrate/20160901213340_add_lfs_enabled_to_namespaces.rb diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index cdfa8d91a28..adb8a891c32 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -60,6 +60,14 @@ class Admin::GroupsController < Admin::ApplicationController end def group_params - params.require(:group).permit(:name, :description, :path, :avatar, :visibility_level, :request_access_enabled) + params.require(:group).permit( + :name, + :description, + :path, + :avatar, + :visibility_level, + :request_access_enabled, + :lfs_enabled + ) end end diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index cb82d62616c..2f7113aa709 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -121,7 +121,17 @@ class GroupsController < Groups::ApplicationController end def group_params - params.require(:group).permit(:name, :description, :path, :avatar, :public, :visibility_level, :share_with_group_lock, :request_access_enabled) + params.require(:group).permit( + :name, + :description, + :path, + :avatar, + :public, + :visibility_level, + :share_with_group_lock, + :request_access_enabled, + :lfs_enabled + ) end def load_events diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index b9211e88473..e87197d2056 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -23,4 +23,9 @@ module GroupsHelper full_title end end + + def projects_with_lfs_enabled(group) + total = group.projects.size + "#{total - group.projects.select{ |p| !p.lfs_enabled? }.size}/#{total} projects have it enabled" + end end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 16a8e52a4ca..b4be679c72d 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -202,8 +202,8 @@ module ProjectsHelper nav_tabs.flatten end - def project_lfs_status(project) - if project.lfs_enabled? + def lfs_status_helper(subject) + if subject.lfs_enabled? content_tag(:span, class: 'lfs-enabled') do 'Enabled' end diff --git a/app/models/group.rb b/app/models/group.rb index c48869ae465..aefb94b2ada 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -95,6 +95,13 @@ class Group < Namespace end end + def lfs_enabled? + return false unless Gitlab.config.lfs.enabled + return Gitlab.config.lfs.enabled if self[:lfs_enabled].nil? + + self[:lfs_enabled] + end + def add_users(user_ids, access_level, current_user: nil, expires_at: nil) user_ids.each do |user_id| Member.add_user( diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 7c29d27ce97..919b3b1f095 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -141,6 +141,11 @@ class Namespace < ActiveRecord::Base projects.joins(:forked_project_link).find_by('forked_project_links.forked_from_project_id = ?', project.id) end + def lfs_enabled? + # User namespace will always default to the global setting + Gitlab.config.lfs.enabled + end + private def repository_storage_paths diff --git a/app/models/project.rb b/app/models/project.rb index f3f3ffbbd28..8e3bedffe1f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -393,10 +393,13 @@ class Project < ActiveRecord::Base end def lfs_enabled? - return false unless Gitlab.config.lfs.enabled - return Gitlab.config.lfs.enabled if self[:lfs_enabled].nil? + # Specifically check is lfs_enabled is false + return false if self[:lfs_enabled] == false - self[:lfs_enabled] + # Should only fallback to the namespace value if no value is set for the project + return namespace.lfs_enabled? if self[:lfs_enabled].nil? + + self[:lfs_enabled] && Gitlab.config.lfs.enabled end def repository_storage_path diff --git a/app/views/admin/groups/_form.html.haml b/app/views/admin/groups/_form.html.haml index 5f7fdfdb011..cb631cbfb8d 100644 --- a/app/views/admin/groups/_form.html.haml +++ b/app/views/admin/groups/_form.html.haml @@ -13,6 +13,8 @@ .col-sm-offset-2.col-sm-10 = render 'shared/allow_request_access', form: f + = render 'shared/groups/group_lfs_settings', f: f + - if @group.new_record? .form-group .col-sm-offset-2.col-sm-10 diff --git a/app/views/admin/groups/show.html.haml b/app/views/admin/groups/show.html.haml index bb374694400..b8e83075a72 100644 --- a/app/views/admin/groups/show.html.haml +++ b/app/views/admin/groups/show.html.haml @@ -37,6 +37,13 @@ %strong = @group.created_at.to_s(:medium) + %li + %span.light Group Git LFS status: + %strong + = lfs_status_helper(@group) + = projects_with_lfs_enabled(@group) + = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs') + .panel.panel-default .panel-heading %h3.panel-title diff --git a/app/views/admin/projects/show.html.haml b/app/views/admin/projects/show.html.haml index 6c7c3c48604..eecc69b125c 100644 --- a/app/views/admin/projects/show.html.haml +++ b/app/views/admin/projects/show.html.haml @@ -77,7 +77,7 @@ %li %span.light Git LFS status: %strong - = project_lfs_status(@project) + = lfs_status_helper(@project) = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs') - else %li diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml index decb89b2fd6..849d0f360e7 100644 --- a/app/views/groups/edit.html.haml +++ b/app/views/groups/edit.html.haml @@ -25,6 +25,8 @@ .col-sm-offset-2.col-sm-10 = render 'shared/allow_request_access', form: f + = render 'shared/groups/group_lfs_settings', f: f + .form-group %hr = f.label :share_with_group_lock, class: 'control-label' do diff --git a/app/views/shared/_visibility_level.html.haml b/app/views/shared/_visibility_level.html.haml index 107ad19177c..add4536a0a2 100644 --- a/app/views/shared/_visibility_level.html.haml +++ b/app/views/shared/_visibility_level.html.haml @@ -1,7 +1,7 @@ .form-group.project-visibility-level-holder = f.label :visibility_level, class: 'control-label' do Visibility Level - = link_to "(?)", help_page_path("public_access/public_access") + = link_to icon('question-circle'), help_page_path("public_access/public_access") .col-sm-10 - if can_change_visibility_level = render('shared/visibility_radios', model_method: :visibility_level, form: f, selected_level: visibility_level, form_model: form_model) diff --git a/app/views/shared/groups/_group_lfs_settings.html.haml b/app/views/shared/groups/_group_lfs_settings.html.haml new file mode 100644 index 00000000000..af57065f0fc --- /dev/null +++ b/app/views/shared/groups/_group_lfs_settings.html.haml @@ -0,0 +1,11 @@ +- if current_user.admin? + .form-group + .col-sm-offset-2.col-sm-10 + .checkbox + = f.label :lfs_enabled do + = f.check_box :lfs_enabled, checked: @group.lfs_enabled? + %strong + Allow projects within this group to use Git LFS + = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs') + %br/ + %span.descr This setting can be overridden in each project. \ No newline at end of file diff --git a/db/migrate/20160901213340_add_lfs_enabled_to_namespaces.rb b/db/migrate/20160901213340_add_lfs_enabled_to_namespaces.rb new file mode 100644 index 00000000000..9b3ee915e21 --- /dev/null +++ b/db/migrate/20160901213340_add_lfs_enabled_to_namespaces.rb @@ -0,0 +1,29 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddLfsEnabledToNamespaces < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + # When a migration requires downtime you **must** uncomment the following + # constant and define a short and easy to understand explanation as to why the + # migration requires downtime. + # DOWNTIME_REASON = '' + + # When using the methods "add_concurrent_index" or "add_column_with_default" + # you must disable the use of transactions as these methods can not run in an + # existing transaction. When using "add_concurrent_index" make sure that this + # method is the _only_ method called in the migration, any other changes + # should go in a separate migration. This ensures that upon failure _only_ the + # index creation fails and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + # disable_ddl_transaction! + + def change + add_column :namespaces, :lfs_enabled, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 61873e38113..70279f372c9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -650,6 +650,7 @@ ActiveRecord::Schema.define(version: 20160913162434) do t.integer "visibility_level", default: 20, null: false t.boolean "request_access_enabled", default: true, null: false t.datetime "deleted_at" + t.boolean "lfs_enabled" end add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree diff --git a/doc/api/groups.md b/doc/api/groups.md index a898387eaa2..cb49648c36f 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -288,6 +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 ## Transfer project to group @@ -317,6 +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 | ```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 4f736e4ec2b..1529a44f58d 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -120,7 +120,7 @@ module API end class Group < Grape::Entity - expose :id, :name, :path, :description, :visibility_level + expose :id, :name, :path, :description, :visibility_level, :lfs_enabled expose :avatar_url expose :web_url end diff --git a/lib/api/groups.rb b/lib/api/groups.rb index d2df77238d5..60ac9bdfa33 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -27,13 +27,14 @@ module API # path (required) - The path of the group # description (optional) - The description of the group # visibility_level (optional) - The visibility level of the group + # lfs_enabled (optional) - Enable/disable LFS for the projects in this group # Example Request: # POST /groups post do authorize! :create_group required_attributes! [:name, :path] - attrs = attributes_for_keys [:name, :path, :description, :visibility_level] + attrs = attributes_for_keys [:name, :path, :description, :visibility_level, :lfs_enabled] @group = Group.new(attrs) if @group.save @@ -51,13 +52,14 @@ module API # path (optional) - The path of the group # description (optional) - The description of the group # visibility_level (optional) - The visibility level of the group + # lfs_enabled (optional) - Enable/disable LFS for the projects in this group # Example Request: # PUT /groups/:id put ':id' do group = find_group(params[:id]) authorize! :admin_group, group - attrs = attributes_for_keys [:name, :path, :description, :visibility_level] + attrs = attributes_for_keys [:name, :path, :description, :visibility_level, :lfs_enabled] if ::Groups::UpdateService.new(group, current_user, attrs).execute present group, with: Entities::GroupDetail diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index f6e811828fc..7ca1bd1e5c9 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1417,6 +1417,68 @@ describe Project, models: true do end end + describe '#lfs_enabled?' do + let(:project) { create(:project) } + + shared_examples 'project overrides group' do + it 'returns true when enabled in project' do + project.update_attribute(:lfs_enabled, true) + + expect(project.lfs_enabled?).to be_truthy + end + + it 'returns false when disabled in project' do + project.update_attribute(:lfs_enabled, false) + + expect(project.lfs_enabled?).to be_falsey + end + + it 'returns the value from the namespace, when no value is set in project' do + expect(project.lfs_enabled?).to eq(project.namespace.lfs_enabled?) + end + end + + context 'LFS disabled in group' do + before do + project.namespace.update_attribute(:lfs_enabled, false) + enable_lfs + end + + it_behaves_like 'project overrides group' + end + + context 'LFS enabled in group' do + before do + project.namespace.update_attribute(:lfs_enabled, true) + enable_lfs + end + + it_behaves_like 'project overrides group' + end + + describe 'LFS disabled globally' do + shared_examples 'it always returns false' do + it do + expect(project.lfs_enabled?).to be_falsey + expect(project.namespace.lfs_enabled?).to be_falsey + end + end + + context 'when no values are set' do + it_behaves_like 'it always returns false' + end + + context 'when all values are set to true' do + before do + project.namespace.update_attribute(:lfs_enabled, true) + project.update_attribute(:lfs_enabled, true) + end + + it_behaves_like 'it always returns false' + end + end + end + describe '.where_paths_in' do context 'without any paths' do it 'returns an empty relation' do @@ -1581,4 +1643,8 @@ describe Project, models: true do expect(project.pushes_since_gc).to eq(0) end end + + def enable_lfs + allow(Gitlab.config.lfs).to receive(:enabled).and_return(true) + end end From a8b0d501017ed8ab8656e8cabe5c29ed7e3cbe89 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Thu, 1 Sep 2016 18:57:38 -0500 Subject: [PATCH 2/5] Added CHANGELOG item --- CHANGELOG | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 0056c6cc649..304839c7e77 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -76,6 +76,8 @@ v 8.12.0 (unreleased) - 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) - Remove redundant js-timeago-pending from user activity log (ClemMakesApps) - Ability to manage project issues, snippets, wiki, merge requests and builds access level From c788c66a85ba4682f5e27a12b5fa73489af23b5f Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Mon, 5 Sep 2016 13:14:49 -0500 Subject: [PATCH 3/5] Improved helper methods, better flow for `project.lfs_enabled?`, and UI fixes. --- app/helpers/groups_helper.rb | 23 +++++++++++++++++-- app/helpers/projects_helper.rb | 4 ++-- app/models/project.rb | 4 ---- app/views/admin/groups/show.html.haml | 3 +-- app/views/admin/projects/show.html.haml | 2 +- app/views/projects/edit.html.haml | 17 +++++++------- ...901213340_add_lfs_enabled_to_namespaces.rb | 17 -------------- 7 files changed, 33 insertions(+), 37 deletions(-) diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index e87197d2056..0352a48e050 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -25,7 +25,26 @@ module GroupsHelper end def projects_with_lfs_enabled(group) - total = group.projects.size - "#{total - group.projects.select{ |p| !p.lfs_enabled? }.size}/#{total} projects have it enabled" + lfs_enabled = group.projects.select(&:lfs_enabled?).size + size = group.projects.size + + if lfs_enabled == size || lfs_enabled == 0 + ' on all projects' + else + " on #{lfs_enabled}/#{size} projects" + end + end + + def group_lfs_status(group) + if group.lfs_enabled? + output = content_tag(:span, class: 'lfs-enabled') do + 'Enabled' + end + else + output = content_tag(:span, class: 'lfs-disabled') do + 'Disabled' + end + end + output << projects_with_lfs_enabled(group) end end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index b4be679c72d..16a8e52a4ca 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -202,8 +202,8 @@ module ProjectsHelper nav_tabs.flatten end - def lfs_status_helper(subject) - if subject.lfs_enabled? + def project_lfs_status(project) + if project.lfs_enabled? content_tag(:span, class: 'lfs-enabled') do 'Enabled' end diff --git a/app/models/project.rb b/app/models/project.rb index 8e3bedffe1f..8b5a6f167bd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -393,10 +393,6 @@ class Project < ActiveRecord::Base end def lfs_enabled? - # Specifically check is lfs_enabled is false - return false if self[:lfs_enabled] == false - - # Should only fallback to the namespace value if no value is set for the project return namespace.lfs_enabled? if self[:lfs_enabled].nil? self[:lfs_enabled] && Gitlab.config.lfs.enabled diff --git a/app/views/admin/groups/show.html.haml b/app/views/admin/groups/show.html.haml index b8e83075a72..0188ed448ce 100644 --- a/app/views/admin/groups/show.html.haml +++ b/app/views/admin/groups/show.html.haml @@ -40,8 +40,7 @@ %li %span.light Group Git LFS status: %strong - = lfs_status_helper(@group) - = projects_with_lfs_enabled(@group) + = group_lfs_status(@group) = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs') .panel.panel-default diff --git a/app/views/admin/projects/show.html.haml b/app/views/admin/projects/show.html.haml index eecc69b125c..6c7c3c48604 100644 --- a/app/views/admin/projects/show.html.haml +++ b/app/views/admin/projects/show.html.haml @@ -77,7 +77,7 @@ %li %span.light Git LFS status: %strong - = lfs_status_helper(@project) + = project_lfs_status(@project) = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs') - else %li diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index f6d751a343e..a04d53e02bf 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -84,15 +84,14 @@ = project_feature_access_select(:snippets_access_level) - if Gitlab.config.lfs.enabled && current_user.admin? - .form-group - .checkbox - = f.label :lfs_enabled do - = f.check_box :lfs_enabled, checked: @project.lfs_enabled? - %strong LFS - %br - %span.descr - Git Large File Storage - = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs') + .row + .col-md-9 + = f.label :lfs_enabled, 'LFS', class: 'label-light' + %span.help-block + Git Large File Storage + = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs') + .col-md-3 + = f.select :lfs_enabled, [%w(Enabled true), %w(Disabled false)], {}, selected: @project.lfs_enabled?, class: 'pull-right form-control' - if Gitlab.config.registry.enabled .form-group diff --git a/db/migrate/20160901213340_add_lfs_enabled_to_namespaces.rb b/db/migrate/20160901213340_add_lfs_enabled_to_namespaces.rb index 9b3ee915e21..fd413d1ca8c 100644 --- a/db/migrate/20160901213340_add_lfs_enabled_to_namespaces.rb +++ b/db/migrate/20160901213340_add_lfs_enabled_to_namespaces.rb @@ -4,25 +4,8 @@ class AddLfsEnabledToNamespaces < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers - # Set this constant to true if this migration requires downtime. DOWNTIME = false - # When a migration requires downtime you **must** uncomment the following - # constant and define a short and easy to understand explanation as to why the - # migration requires downtime. - # DOWNTIME_REASON = '' - - # When using the methods "add_concurrent_index" or "add_column_with_default" - # you must disable the use of transactions as these methods can not run in an - # existing transaction. When using "add_concurrent_index" make sure that this - # method is the _only_ method called in the migration, any other changes - # should go in a separate migration. This ensures that upon failure _only_ the - # index creation fails and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - def change add_column :namespaces, :lfs_enabled, :boolean end From d0279ccba5c4a2cd8611ddec04eeff67e0e9f9c6 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Mon, 5 Sep 2016 15:40:49 -0500 Subject: [PATCH 4/5] Correct helper for group LFS status. --- app/helpers/groups_helper.rb | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index 0352a48e050..76911efe354 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -24,27 +24,27 @@ module GroupsHelper end end - def projects_with_lfs_enabled(group) - lfs_enabled = group.projects.select(&:lfs_enabled?).size + 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 + size = group.projects.size - if lfs_enabled == size || lfs_enabled == 0 - ' on all projects' + if lfs_status == size || lfs_status == 0 + 'on all projects' else - " on #{lfs_enabled}/#{size} projects" + "on #{lfs_status} out of #{size} projects" end end def group_lfs_status(group) - if group.lfs_enabled? - output = content_tag(:span, class: 'lfs-enabled') do - 'Enabled' - end - else - output = content_tag(:span, class: 'lfs-disabled') do - 'Disabled' - end + status = group.lfs_enabled? ? 'enabled' : 'disabled' + + content_tag(:span, class: "lfs-#{status}") do + "#{status.humanize} #{projects_with_lfs_enabled(group, group.lfs_enabled?)}" end - output << projects_with_lfs_enabled(group) end end From 02ddb9dff4084f615f744614cf81dc4166d61668 Mon Sep 17 00:00:00 2001 From: Patricio Cano Date: Tue, 6 Sep 2016 11:48:00 -0500 Subject: [PATCH 5/5] Syntax fixes and better tests for helper methods. Updated docs. --- CHANGELOG | 1 - app/controllers/admin/groups_controller.rb | 10 +-- app/controllers/groups_controller.rb | 12 ++-- app/helpers/groups_helper.rb | 21 ++++--- app/views/admin/groups/_form.html.haml | 2 +- .../groups/_group_lfs_settings.html.haml | 0 app/views/groups/edit.html.haml | 2 +- doc/api/groups.md | 4 +- lib/api/entities.rb | 6 +- spec/helpers/groups_helper_spec.rb | 63 +++++++++++++++++++ spec/models/group_spec.rb | 46 ++++++++++++++ 11 files changed, 139 insertions(+), 28 deletions(-) rename app/views/{shared => }/groups/_group_lfs_settings.html.haml (100%) 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, - :description, - :path, :avatar, - :visibility_level, + :description, + :lfs_enabled, + :name, + :path, :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, - :description, - :path, :avatar, + :description, + :lfs_enabled, + :name, + :path, :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 similarity index 100% rename from app/views/shared/groups/_group_lfs_settings.html.haml rename to 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) }