Refactor forking restriction to ProjectFeature

Since we reduced the scope significantly, forking restriction makes more
sense as a ProjectFeature.

app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue

@@ -104,7 +104,7 @@ export default {
       visibilityLevel: visibilityOptions.PUBLIC,
       issuesAccessLevel: 20,
       repositoryAccessLevel: 20,
-      forkingEnabled: true,
+      forkingAccessLevel: 20,
       mergeRequestsAccessLevel: 20,
       buildsAccessLevel: 20,
       wikiAccessLevel: 20,
@@ -307,10 +307,11 @@ export default {
             s__('ProjectSettings|Allow users to make copies of your repository to a new project')
           "
         >
-          <project-feature-toggle
-            v-model="forkingEnabled"
+          <project-feature-setting
+            v-model="forkingAccessLevel"
+            :options="featureAccessLevelOptions"
             :disabled-input="!repositoryEnabled"
-            name="project[project_setting_attributes][forking_enabled]"
+            name="project[project_feature_attributes][forking_access_level]"
           />
         </project-setting-row>
         <project-setting-row

app/assets/javascripts/pages/projects/shared/permissions/mixins/settings_pannel_mixin.js

@@ -12,13 +12,11 @@ export default {
         this.buildsAccessLevel = Math.min(this.buildsAccessLevel, value);
 
         if (value === 0) {
-          this.forkingEnabled = false;
           this.containerRegistryEnabled = false;
           this.lfsEnabled = false;
         }
       } else if (oldValue === 0) {
         this.mergeRequestsAccessLevel = value;
-        this.forkingEnabled = true;
         this.buildsAccessLevel = value;
         this.containerRegistryEnabled = true;
         this.lfsEnabled = true;

app/controllers/projects/forks_controller.rb

@@ -9,7 +9,7 @@ class Projects::ForksController < Projects::ApplicationController
   before_action :require_non_empty_project
   before_action :authorize_download_code!
   before_action :authenticate_user!, only: [:new, :create]
-  before_action :check_forking_availability, only: [:new, :create]
+  before_action :authorize_fork_project!, only: [:new, :create]
 
   # rubocop: disable CodeReuse/ActiveRecord
   def index
@@ -67,8 +67,4 @@ class Projects::ForksController < Projects::ApplicationController
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42335')
   end
-
-  def check_forking_availability
-    access_denied!(_('Forking is disabled for this project.')) unless @project.forking_enabled
-  end
 end

app/controllers/projects_controller.rb

@@ -391,15 +391,12 @@ class ProjectsController < Projects::ApplicationController
       project_feature_attributes: %i[
         builds_access_level
         issues_access_level
+        forking_access_level
         merge_requests_access_level
         repository_access_level
         snippets_access_level
         wiki_access_level
         pages_access_level
-      ],
-
-      project_setting_attributes: %i[
-        forking_enabled
       ]
     ]
   end

app/helpers/projects_helper.rb

@@ -563,7 +563,7 @@ module ProjectsHelper
       requestAccessEnabled: !!project.request_access_enabled,
       issuesAccessLevel: feature.issues_access_level,
       repositoryAccessLevel: feature.repository_access_level,
-      forkingEnabled: project.forking_enabled,
+      forkingAccessLevel: feature.forking_access_level,
       mergeRequestsAccessLevel: feature.merge_requests_access_level,
       buildsAccessLevel: feature.builds_access_level,
       wikiAccessLevel: feature.wiki_access_level,

app/models/concerns/project_features_compatibility.rb

@@ -50,6 +50,10 @@ module ProjectFeaturesCompatibility
     write_feature_attribute_string(:merge_requests_access_level, value)
   end
 
+  def forking_access_level=(value)
+    write_feature_attribute_string(:forking_access_level, value)
+  end
+
   def issues_access_level=(value)
     write_feature_attribute_string(:issues_access_level, value)
   end

app/models/project.rb

@@ -235,7 +235,6 @@ class Project < ApplicationRecord
 
   has_one :import_data, class_name: 'ProjectImportData', inverse_of: :project, autosave: true
   has_one :project_feature, inverse_of: :project
-  has_one :project_setting
   has_one :statistics, class_name: 'ProjectStatistics'
 
   has_one :cluster_project, class_name: 'Clusters::Project'
@@ -305,7 +304,6 @@ class Project < ApplicationRecord
 
   accepts_nested_attributes_for :variables, allow_destroy: true
   accepts_nested_attributes_for :project_feature, update_only: true
-  accepts_nested_attributes_for :project_setting, update_only: true
   accepts_nested_attributes_for :import_data
   accepts_nested_attributes_for :auto_devops, update_only: true
   accepts_nested_attributes_for :ci_cd_settings, update_only: true
@@ -319,10 +317,12 @@ class Project < ApplicationRecord
   accepts_nested_attributes_for :metrics_setting, update_only: true, allow_destroy: true
   accepts_nested_attributes_for :grafana_integration, update_only: true, allow_destroy: true
 
-  delegate :feature_available?, :builds_enabled?, :wiki_enabled?, :merge_requests_enabled?,
-    :issues_enabled?, :pages_enabled?, :public_pages?, :private_pages?,
-    :merge_requests_access_level, :issues_access_level, :wiki_access_level,
-    :snippets_access_level, :builds_access_level, :repository_access_level,
+  delegate :feature_available?, :builds_enabled?, :wiki_enabled?,
+    :merge_requests_enabled?, :forking_enabled?, :issues_enabled?,
+    :pages_enabled?, :public_pages?, :private_pages?,
+    :merge_requests_access_level, :forking_access_level, :issues_access_level,
+    :wiki_access_level, :snippets_access_level, :builds_access_level,
+    :repository_access_level,
     to: :project_feature, allow_nil: true
   delegate :scheduled?, :started?, :in_progress?, :failed?, :finished?,
     prefix: :import, to: :import_state, allow_nil: true
@@ -336,7 +336,6 @@ class Project < ApplicationRecord
   delegate :group_runners_enabled, :group_runners_enabled=, :group_runners_enabled?, to: :ci_cd_settings
   delegate :root_ancestor, to: :namespace, allow_nil: true
   delegate :last_pipeline, to: :commit, allow_nil: true
-  delegate :forking_enabled, to: :project_setting, allow_nil: true
   delegate :external_dashboard_url, to: :metrics_setting, allow_nil: true, prefix: true
   delegate :default_git_depth, :default_git_depth=, to: :ci_cd_settings, prefix: :ci
 
@@ -667,10 +666,6 @@ class Project < ApplicationRecord
     super
   end
 
-  def project_setting
-    super || build_project_setting
-  end
-
   def all_pipelines
     if builds_enabled?
       super

app/models/project_feature.rb

@@ -22,7 +22,7 @@ class ProjectFeature < ApplicationRecord
   ENABLED  = 20
   PUBLIC   = 30
 
-  FEATURES = %i(issues merge_requests wiki snippets builds repository pages).freeze
+  FEATURES = %i(issues forking merge_requests wiki snippets builds repository pages).freeze
   PRIVATE_FEATURES_MIN_ACCESS_LEVEL = { merge_requests: Gitlab::Access::REPORTER }.freeze
   PRIVATE_FEATURES_MIN_ACCESS_LEVEL_FOR_PRIVATE_PROJECT = { repository: Gitlab::Access::REPORTER }.freeze
   STRING_OPTIONS = HashWithIndifferentAccess.new({
@@ -92,6 +92,7 @@ class ProjectFeature < ApplicationRecord
 
   default_value_for :builds_access_level,         value: ENABLED, allows_nil: false
   default_value_for :issues_access_level,         value: ENABLED, allows_nil: false
+  default_value_for :forking_access_level,        value: ENABLED, allows_nil: false
   default_value_for :merge_requests_access_level, value: ENABLED, allows_nil: false
   default_value_for :snippets_access_level,       value: ENABLED, allows_nil: false
   default_value_for :wiki_access_level,           value: ENABLED, allows_nil: false
@@ -132,6 +133,10 @@ class ProjectFeature < ApplicationRecord
     merge_requests_access_level > DISABLED
   end
 
+  def forking_enabled?
+    forking_access_level > DISABLED
+  end
+
   def issues_enabled?
     issues_access_level > DISABLED
   end

app/models/project_setting.rb

-# frozen_string_literal: true
-
-class ProjectSetting < ApplicationRecord
-  self.primary_key = :project_id
-
-  belongs_to :project
-
-  validates :forking_enabled, inclusion: { in: [true, false] }
-end

app/policies/project_policy.rb

@@ -83,8 +83,10 @@ class ProjectPolicy < BasePolicy
     project.merge_requests_allowing_push_to_user(user).any?
   end
 
-  desc "Project allows forking"
-  condition(:forking_allowed) { @subject.forking_enabled }
+  with_scope :subject
+  condition(:forking_allowed) do
+    @subject.feature_available?(:forking, @user)
+  end
 
   with_scope :global
   condition(:mirror_available, score: 0) do

db/migrate/20190215151545_create_project_settings.rb

-# frozen_string_literal: true
-
-class CreateProjectSettings < ActiveRecord::Migration[5.0]
-  DOWNTIME = false
-
-  def change
-    create_table(:project_settings, id: false) do |t|
-      t.references :project,
-                   primary_key: true,
-                   null: false,
-                   index: { unique: true },
-                   foreign_key: { on_delete: :cascade }
-
-      t.boolean :forking_enabled,
-                default: true,
-                null: false
-
-      t.datetime_with_timezone :updated_at, null: false
-    end
-  end
-end

db/migrate/20200104113850_add_forking_access_level_to_project_feature.rb

+# frozen_string_literal: true
+
+class AddForkingAccessLevelToProjectFeature < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    add_column :project_features, :forking_access_level, :integer
+  end
+end

db/schema.rb

@@ -3166,6 +3166,7 @@ ActiveRecord::Schema.define(version: 2020_01_14_204949) do
     t.datetime "updated_at"
     t.integer "repository_access_level", default: 20, null: false
     t.integer "pages_access_level", null: false
+    t.integer "forking_access_level"
     t.index ["project_id"], name: "index_project_features_on_project_id", unique: true
   end
 
@@ -3253,12 +3254,6 @@ ActiveRecord::Schema.define(version: 2020_01_14_204949) do
     t.index ["project_id"], name: "index_project_repository_states_on_project_id", unique: true
   end
 
-  create_table "project_settings", primary_key: "project_id", id: :serial, force: :cascade do |t|
-    t.boolean "forking_enabled", default: true, null: false
-    t.datetime_with_timezone "updated_at", null: false
-    t.index ["project_id"], name: "index_project_settings_on_project_id", unique: true
-  end
-
   create_table "project_statistics", id: :serial, force: :cascade do |t|
     t.integer "project_id", null: false
     t.integer "namespace_id", null: false
@@ -4782,7 +4777,6 @@ ActiveRecord::Schema.define(version: 2020_01_14_204949) do
   add_foreign_key "project_repositories", "projects", on_delete: :cascade
   add_foreign_key "project_repositories", "shards", on_delete: :restrict
   add_foreign_key "project_repository_states", "projects", on_delete: :cascade
-  add_foreign_key "project_settings", "projects", on_delete: :cascade
   add_foreign_key "project_statistics", "projects", on_delete: :cascade
   add_foreign_key "project_tracing_settings", "projects", on_delete: :cascade
   add_foreign_key "projects", "pool_repositories", name: "fk_6e5c14658a", on_delete: :nullify

ee/lib/ee/audit/project_feature_changes_auditor.rb

@@ -6,6 +6,7 @@ module EE
       attr_accessor :project
 
       COLUMNS = [:merge_requests_access_level,
+                 :forking_access_level,
                  :issues_access_level,
                  :wiki_access_level,
                  :snippets_access_level,

lib/gitlab/import_export/import_export.yml

@@ -90,7 +90,6 @@ tree:
     - protected_tags:
       - :create_access_levels
     - :project_feature
-    - :project_setting
     - :custom_attributes
     - :prometheus_metrics
     - :project_badges

locale/gitlab.pot

@@ -8285,9 +8285,6 @@ msgstr ""
 msgid "Forking in progress"
 msgstr ""
 
-msgid "Forking is disabled for this project."
-msgstr ""
-
 msgid "Forking repository"
 msgstr ""
 

spec/controllers/projects/forks_controller_spec.rb

@@ -13,18 +13,17 @@ describe Projects::ForksController do
   end
 
   shared_examples 'forking disabled' do
-    let(:project) { create(:project, :private, :repository) }
+    let(:project) { create(:project, :private, :repository, :forking_disabled) }
 
     before do
-      create(:project_setting, { project: project, forking_enabled: false })
       project.add_developer(user)
       sign_in(user)
     end
 
-    it 'returns with 403' do
+    it 'returns with 404' do
       subject
 
-      expect(response).to have_gitlab_http_status(403)
+      expect(response).to have_gitlab_http_status(404)
     end
   end
 
@@ -157,8 +156,8 @@ describe Projects::ForksController do
     subject do
       get :new,
           params: {
-              namespace_id: project.namespace,
-              project_id: project
+            namespace_id: project.namespace,
+            project_id: project
           }
     end
 
@@ -188,9 +187,9 @@ describe Projects::ForksController do
   describe 'POST create' do
     let(:params) do
       {
-          namespace_id: project.namespace,
-          project_id: project,
-          namespace_key: user.namespace.id
+        namespace_id: project.namespace,
+        project_id: project,
+        namespace_key: user.namespace.id
       }
     end
 
@@ -213,16 +212,16 @@ describe Projects::ForksController do
       context 'continue params' do
         let(:params) do
           {
-              namespace_id: project.namespace,
-              project_id: project,
-              namespace_key: user.namespace.id,
-              continue: continue_params
+            namespace_id: project.namespace,
+            project_id: project,
+            namespace_key: user.namespace.id,
+            continue: continue_params
           }
         end
         let(:continue_params) do
           {
-              to: '/-/ide/project/path',
-              notice: 'message'
+            to: '/-/ide/project/path',
+            notice: 'message'
           }
         end
 

spec/factories/project_settings.rb

-# frozen_string_literal: true
-
-FactoryBot.define do
-  factory :project_setting do
-    project
-  end
-end

spec/factories/projects.rb

@@ -25,6 +25,7 @@ FactoryBot.define do
       builds_access_level { ProjectFeature::ENABLED }
       snippets_access_level { ProjectFeature::ENABLED }
       issues_access_level { ProjectFeature::ENABLED }
+      forking_access_level { ProjectFeature::ENABLED }
       merge_requests_access_level { ProjectFeature::ENABLED }
       repository_access_level { ProjectFeature::ENABLED }
       pages_access_level do
@@ -48,6 +49,7 @@ FactoryBot.define do
         builds_access_level: builds_access_level,
         snippets_access_level: evaluator.snippets_access_level,
         issues_access_level: evaluator.issues_access_level,
+        forking_access_level: evaluator.forking_access_level,
         merge_requests_access_level: merge_requests_access_level,
         repository_access_level: evaluator.repository_access_level
       }
@@ -264,6 +266,9 @@ FactoryBot.define do
     trait(:issues_disabled)         { issues_access_level { ProjectFeature::DISABLED } }
     trait(:issues_enabled)          { issues_access_level { ProjectFeature::ENABLED } }
     trait(:issues_private)          { issues_access_level { ProjectFeature::PRIVATE } }
+    trait(:forking_disabled)         { forking_access_level { ProjectFeature::DISABLED } }
+    trait(:forking_enabled)          { forking_access_level { ProjectFeature::ENABLED } }
+    trait(:forking_private)          { forking_access_level { ProjectFeature::PRIVATE } }
     trait(:merge_requests_enabled)  { merge_requests_access_level { ProjectFeature::ENABLED } }
     trait(:merge_requests_disabled) { merge_requests_access_level { ProjectFeature::DISABLED } }
     trait(:merge_requests_private)  { merge_requests_access_level { ProjectFeature::PRIVATE } }

spec/features/projects/fork_spec.rb

@@ -28,18 +28,13 @@ describe 'Project fork' do
   end
 
   context 'forking enabled / disabled in project settings' do
-    let(:project) { create(:project, :private, :repository) }
-
     before do
-      project.add_developer(user)
-
-      create(:project_setting,
-             { project: project,
-               forking_enabled: forking_enabled })
+      project.project_feature.update_attribute(
+        :forking_access_level, forking_access_level)
     end
 
     context 'forking is enabled' do
-      let(:forking_enabled) { true }
+      let(:forking_access_level) { ProjectFeature::ENABLED }
 
       it 'enables fork button' do
         visit project_path(project)
@@ -57,7 +52,7 @@ describe 'Project fork' do
     end
 
     context 'forking is disabled' do
-      let(:forking_enabled) { false }
+      let(:forking_access_level) { ProjectFeature::DISABLED }
 
       it 'does not render fork button' do
         visit project_path(project)
@@ -68,8 +63,49 @@ describe 'Project fork' do
       it 'does not render new project fork page' do
         visit new_project_fork_path(project)
 
-        expect(page.status_code).to eq(403)
-        expect(page).to have_text('Forking is disabled for this project')
+        expect(page.status_code).to eq(404)
+      end
+    end
+
+    context 'forking is private' do
+      let(:forking_access_level) { ProjectFeature::PRIVATE }
+
+      before do
+        project.update(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+      end
+
+      context 'user is not a team member' do
+        it 'does not render fork button' do
+          visit project_path(project)
+
+          expect(page).not_to have_css('a', text: 'Fork')
+        end
+
+        it 'does not render new project fork page' do
+          visit new_project_fork_path(project)
+
+          expect(page.status_code).to eq(404)
+        end
+      end
+
+      context 'user is a team member' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'enables fork button' do
+          visit project_path(project)
+
+          expect(page).to have_css('a', text: 'Fork')
+          expect(page).not_to have_css('a.disabled', text: 'Fork')
+        end
+
+        it 'renders new project fork page' do
+          visit new_project_fork_path(project)
+
+          expect(page.status_code).to eq(200)
+          expect(page).to have_text(' Select a namespace to fork the project ')
+        end
       end
     end
   end

spec/features/projects/settings/project_settings_spec.rb

@@ -38,10 +38,10 @@ describe 'Projects settings' do
     it 'toggles forking enabled / disabled' do
       visit edit_project_path(project)
 
-      forking_enabled_input = find('input[name="project[project_setting_attributes][forking_enabled]"]', visible: :hidden)
-      forking_enabled_button = find('input[name="project[project_setting_attributes][forking_enabled]"] + button')
+      forking_enabled_input = find('input[name="project[project_feature_attributes][forking_access_level]"]', visible: :hidden)
+      forking_enabled_button = find('input[name="project[project_feature_attributes][forking_access_level]"] + label > button')
 
-      expect(forking_enabled_input.value).to eq('true')
+      expect(forking_enabled_input.value).to eq('20')
 
       # disable by clicking toggle
       forking_enabled_button.click
@@ -50,7 +50,7 @@ describe 'Projects settings' do
       end
       wait_for_requests
 
-      expect(forking_enabled_input.value).to eq('false')
+      expect(forking_enabled_input.value).to eq('0')
     end
   end
 

spec/lib/gitlab/import_export/all_models.yml

@@ -373,7 +373,6 @@ project:
 - environments_for_dashboard
 - deployments
 - project_feature
-- project_setting
 - auto_devops
 - pages_domains
 - authorized_users

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -544,6 +544,7 @@ ProjectFeature:
 - id
 - project_id
 - merge_requests_access_level
+- forking_access_level
 - issues_access_level
 - wiki_access_level
 - snippets_access_level
@@ -552,12 +553,6 @@ ProjectFeature:
 - pages_access_level
 - created_at
 - updated_at
-ProjectSetting:
-- id
-- project_id
-- forking_enabled
-- created_at
-- updated_at
 ProtectedBranch::MergeAccessLevel:
 - id
 - protected_branch_id

spec/models/project_setting_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe ProjectSetting do
-  describe 'relations' do
-    it { is_expected.to belong_to(:project) }
-  end
-
-  describe 'validations' do
-    it { is_expected.to validate_inclusion_of(:forking_enabled).in_array([true, false]) }
-  end
-end

spec/models/project_spec.rb

@@ -62,7 +62,6 @@ describe Project do
     it { is_expected.to have_one(:external_wiki_service) }
     it { is_expected.to have_one(:project_feature) }
     it { is_expected.to have_one(:project_repository) }
-    it { is_expected.to have_one(:project_setting) }
     it { is_expected.to have_one(:container_expiration_policy) }
     it { is_expected.to have_one(:statistics).class_name('ProjectStatistics') }
     it { is_expected.to have_one(:import_data).class_name('ProjectImportData') }

spec/requests/api/projects_spec.rb

@@ -2861,8 +2861,8 @@ describe API::Projects do
 
     context 'forking disabled' do
       before do
-        create(:project_setting,
-               { project: project, forking_enabled: false })
+        project.project_feature.update_attribute(
+          :forking_access_level, ProjectFeature::DISABLED)
       end
 
       it 'denies project to be forked' do

spec/services/projects/fork_service_spec.rb

@@ -227,8 +227,8 @@ describe Projects::ForkService do
 
       context 'when forking is disabled' do
         before do
-          create(:project_setting,
-                 { project: @from_project, forking_enabled: false })
+          @from_project.project_feature.update_attribute(
+            :forking_access_level, ProjectFeature::DISABLED)
         end
 
         it 'fails' do

spec/services/projects/update_service_spec.rb

@@ -8,11 +8,8 @@ describe Projects::UpdateService do
 
   let(:user) { create(:user) }
   let(:project) do
-    create(:project, { creator: user,
-                       namespace: user.namespace,
-                       visibility_level: project_visibility_level })
+    create(:project, creator: user, namespace: user.namespace)
   end
-  let(:project_visibility_level) { Gitlab::VisibilityLevel::PRIVATE }
 
   describe '#execute' do
     let(:gitlab_shell) { Gitlab::Shell.new }
@@ -157,40 +154,6 @@ describe Projects::UpdateService do
       end
     end
 
-    context 'when changing forking enabled' do
-      subject do
-        update_project(project, user,
-                       project_setting_attributes: { forking_enabled: forking_enabled } )
-      end
-
-      shared_examples 'valid forking_enabled change' do
-        it 'succeeds' do
-          expect(subject).to eq({ status: :success })
-        end
-
-        it 'updates the project' do
-          expect { subject }.to change(project, :forking_enabled).to(forking_enabled)
-        end
-      end
-
-      context 'enables forking' do
-        let(:forking_enabled) { true }
-
-        before do
-          create(:project_setting,
-                 { project: project, forking_enabled: false })
-        end
-
-        it_behaves_like 'valid forking_enabled change'
-      end
-
-      context 'disables forking' do
-        let(:forking_enabled) { false }
-
-        it_behaves_like 'valid forking_enabled change'
-      end
-    end
-
     describe 'when updating project that has forks' do
       let(:project) { create(:project, :internal) }
       let(:forked_project) { fork_project(project) }