WIP: Remove the forked project link relationbvl-remove-forked-project-link

14 files changed, 70 insertions, 169 deletions

diff --git a/app/models/forked_project_link.rb b/app/models/forked_project_link.rb
deleted file mode 100644
index 0f7067238cd..00000000000
--- a/app/models/forked_project_link.rb
+++ /dev/null
@@ -1,6 +0,0 @@
-# frozen_string_literal: true
-
-class ForkedProjectLink < ActiveRecord::Base
-  belongs_to :forked_to_project, -> { where.not(pending_delete: true) }, class_name: 'Project'
-  belongs_to :forked_from_project, -> { where.not(pending_delete: true) }, class_name: 'Project'
-end
diff --git a/app/models/project.rb b/app/models/project.rb
index 178d70757a1..d4e00c8b2fa 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -164,20 +164,15 @@ class Project < ActiveRecord::Base
   has_one :packagist_service
   has_one :hangouts_chat_service
 
-  # TODO: replace these relations with the fork network versions
-  has_one  :forked_project_link,  foreign_key: "forked_to_project_id"
-  has_one  :forked_from_project,  through:   :forked_project_link
-
-  has_many :forked_project_links, foreign_key: "forked_from_project_id"
-  has_many :forks,                through:     :forked_project_links, source: :forked_to_project
-  # TODO: replace these relations with the fork network versions
-
   has_one :root_of_fork_network,
           foreign_key: 'root_project_id',
           inverse_of: :root_project,
           class_name: 'ForkNetwork'
   has_one :fork_network_member
   has_one :fork_network, through: :fork_network_member
+  has_one :forked_from_project, through: :fork_network_member
+  has_many :forked_to_members, class_name: 'ForkNetworkMember', foreign_key: 'forked_from_project_id'
+  has_many :forks, through: :forked_to_members, source: :project
 
   has_one :import_state, autosave: true, class_name: 'ProjectImportState', inverse_of: :project
   has_one :import_export_upload, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
@@ -1236,12 +1231,7 @@ class Project < ActiveRecord::Base
   end
 
   def forked?
-    return true if fork_network && fork_network.root_project != self
-
-    # TODO: Use only the above conditional using the `fork_network`
-    # This is the old conditional that looks at the `forked_project_link`, we
-    # fall back to this while we're migrating the new models
-    !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?)
+    fork_network && fork_network.root_project != self
   end
 
   def fork_source
@@ -1517,9 +1507,7 @@ class Project < ActiveRecord::Base
   def visibility_level_allowed_as_fork?(level = self.visibility_level)
     return true unless forked?
 
-    # self.forked_from_project will be nil before the project is saved, so
-    # we need to go through the relation
-    original_project = forked_project_link&.forked_from_project
+    original_project = fork_source
     return true unless original_project
 
     level <= original_project.visibility_level
diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb
index 02a3a3eb096..12d4a47fff9 100644
--- a/app/services/projects/create_service.rb
+++ b/app/services/projects/create_service.rb
@@ -13,7 +13,6 @@ module Projects
         return ::Projects::CreateFromTemplateService.new(current_user, params).execute
       end
 
-      forked_from_project_id = params.delete(:forked_from_project_id)
       import_data = params.delete(:import_data)
 
       @project = Project.new(params)
@@ -24,11 +23,6 @@ module Projects
         return @project
       end
 
-      unless allowed_fork?(forked_from_project_id)
-        @project.errors.add(:forked_from_project_id, 'is forbidden')
-        return @project
-      end
-
       set_project_name_from_path
 
       # get namespace id
@@ -54,10 +48,6 @@ module Projects
 
       @project.creator = current_user
 
-      if forked_from_project_id
-        @project.build_forked_project_link(forked_from_project_id: forked_from_project_id)
-      end
-
       save_project_and_import_data(import_data)
 
       after_create_actions if @project.persisted?
@@ -79,13 +69,6 @@ module Projects
       @project.errors.add(:namespace, "is not valid")
     end
 
-    def allowed_fork?(source_project_id)
-      return true if source_project_id.nil?
-
-      source_project = Project.find_by(id: source_project_id)
-      current_user.can?(:fork_project, source_project)
-    end
-
     def allowed_namespace?(user, namespace_id)
       namespace = Namespace.find_by(id: namespace_id)
       current_user.can?(:create_projects, namespace)
diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb
index cbbb88a9410..6737baf79c8 100644
--- a/app/services/projects/fork_service.rb
+++ b/app/services/projects/fork_service.rb
@@ -12,9 +12,14 @@ module Projects
 
     private
 
+    def allowed_fork?
+      current_user.can?(:fork_project, @project)
+    end
+
     def link_existing_project(fork_to_project)
       return if fork_to_project.forked?
 
+      build_fork_network_member(fork_to_project)
       link_fork_network(fork_to_project)
 
       # A forked project stores its LFS objects in the `forked_from_project`.
@@ -30,7 +35,6 @@ module Projects
 
     def fork_new_project
       new_params = {
-        forked_from_project_id: @project.id,
         visibility_level:       allowed_visibility_level,
         description:            @project.description,
         name:                   @project.name,
@@ -43,41 +47,44 @@ module Projects
         new_params[:avatar] = @project.avatar
       end
 
-      new_project = CreateService.new(current_user, new_params).execute
+      new_project = CreateService.new(current_user, new_params).execute do |p|
+        # Assign the fork network to the project manually so the project knows
+        # it is forked. That way the import will be started afterwards.
+        p.fork_network = fork_network
+        build_fork_network_member(p)
+      end
+
       return new_project unless new_project.persisted?
 
       builds_access_level = @project.project_feature.builds_access_level
       new_project.project_feature.update(builds_access_level: builds_access_level)
 
-      link_fork_network(new_project)
-
       new_project
     end
 
     def fork_network
-      if @project.fork_network
-        @project.fork_network
-      elsif forked_from_project = @project.forked_from_project
-        # TODO: remove this case when all background migrations have completed
-        # this only happens when a project had a `forked_project_link` that was
-        # not migrated to the `fork_network` relation
-        forked_from_project.fork_network || forked_from_project.create_root_of_fork_network
+      @fork_network ||= if @project.fork_network
+                          @project.fork_network
+                        else
+                          @project.build_root_of_fork_network
+                        end
+    end
+
+    def build_fork_network_member(fork_to_project)
+      if allowed_fork?
+        fork_to_project.build_fork_network_member(forked_from_project: @project,
+                                                  fork_network: fork_network)
+
       else
-        @project.create_root_of_fork_network
+        fork_to_project.errors.add(:forked_from_project_id, 'is forbidden')
       end
     end
 
     def link_fork_network(fork_to_project)
-      fork_network.fork_network_members.create(project: fork_to_project,
-                                               forked_from_project: @project)
-
-      # TODO: remove this when ForkedProjectLink model is removed
-      unless fork_to_project.forked_project_link
-        fork_to_project.create_forked_project_link(forked_to_project: fork_to_project,
-                                                   forked_from_project: @project)
-      end
+      return if fork_to_project.errors.any?
 
-      refresh_forks_count
+      fork_to_project.fork_network_member.save &&
+        refresh_forks_count
     end
 
     def refresh_forks_count
diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb
index b570c6d4754..f173c004278 100644
--- a/app/services/projects/forks_count_service.rb
+++ b/app/services/projects/forks_count_service.rb
@@ -8,10 +8,7 @@ module Projects
     end
 
     def self.query(project_ids)
-      # We can't directly change ForkedProjectLink to ForkNetworkMember here
-      # Nowadays, when a call using v3 to projects/:id/fork is made,
-      # the relationship to ForkNetworkMember is not updated
-      ForkedProjectLink.where(forked_from_project: project_ids)
+      ForkNetworkMember.where(forked_from_project: project_ids)
     end
   end
 end
diff --git a/app/workers/namespaceless_project_destroy_worker.rb b/app/workers/namespaceless_project_destroy_worker.rb
index d9df42c9e17..2965f3b1150 100644
--- a/app/workers/namespaceless_project_destroy_worker.rb
+++ b/app/workers/namespaceless_project_destroy_worker.rb
@@ -32,7 +32,5 @@ class NamespacelessProjectDestroyWorker
     merge_requests = project.forked_from_project.merge_requests.opened.from_project(project)
 
     merge_requests.update_all(state: 'closed')
-
-    project.forked_project_link.destroy
   end
 end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 95b25d7351a..01f300ad40b 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -232,7 +232,7 @@ module API
         super(projects_relation).preload(:group)
                                 .preload(project_group_links: :group,
                                          fork_network: :root_project,
-                                         forked_project_link: :forked_from_project,
+                                         fork_network_member: :forked_from_project,
                                          forked_from_project: [:route, :forks, :tags, namespace: :route])
       end
 
diff --git a/spec/factories/forked_project_links.rb b/spec/factories/forked_project_links.rb
deleted file mode 100644
index bc59fea81ec..00000000000
--- a/spec/factories/forked_project_links.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-FactoryBot.define do
-  factory :forked_project_link do
-    association :forked_to_project, factory: [:project, :repository]
-    association :forked_from_project, factory: [:project, :repository]
-
-    after(:create) do |link|
-      link.forked_from_project.reload
-      link.forked_to_project.reload
-    end
-
-    trait :forked_to_empty_project do
-      association :forked_to_project, factory: [:project, :repository]
-    end
-  end
-end
diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml
index b4269bd5786..5e6319011db 100644
--- a/spec/lib/gitlab/import_export/all_models.yml
+++ b/spec/lib/gitlab/import_export/all_models.yml
@@ -228,9 +228,7 @@ project:
 - mock_ci_service
 - mock_deployment_service
 - mock_monitoring_service
-- forked_project_link
 - forked_from_project
-- forked_project_links
 - forks
 - merge_requests
 - fork_merge_requests
diff --git a/spec/models/forked_project_link_spec.rb b/spec/models/forked_project_link_spec.rb
deleted file mode 100644
index 32e33e8f42f..00000000000
--- a/spec/models/forked_project_link_spec.rb
+++ /dev/null
@@ -1,68 +0,0 @@
-require 'spec_helper'
-
-describe ForkedProjectLink, "add link on fork" do
-  include ProjectForksHelper
-
-  let(:project_from) { create(:project, :repository) }
-  let(:project_to) { fork_project(project_from, user) }
-  let(:user) { create(:user) }
-
-  before do
-    project_from.add_reporter(user)
-  end
-
-  it 'project_from knows its forks' do
-    _ = project_to
-
-    expect(project_from.forks.count).to eq(1)
-  end
-
-  it "project_to knows it is forked" do
-    expect(project_to.forked?).to be_truthy
-  end
-
-  it "project knows who it is forked from" do
-    expect(project_to.forked_from_project).to eq(project_from)
-  end
-
-  context 'project_to is pending_delete' do
-    before do
-      project_to.update!(pending_delete: true)
-    end
-
-    it { expect(project_from.forks.count).to eq(0) }
-  end
-
-  context 'project_from is pending_delete' do
-    before do
-      project_from.update!(pending_delete: true)
-    end
-
-    it { expect(project_to.forked_from_project).to be_nil }
-  end
-
-  describe '#forked?' do
-    let(:project_to) { create(:project, :repository, forked_project_link: forked_project_link) }
-    let(:forked_project_link) { create(:forked_project_link) }
-
-    before do
-      forked_project_link.forked_from_project = project_from
-      forked_project_link.forked_to_project = project_to
-      forked_project_link.save!
-    end
-
-    it "project_to knows it is forked" do
-      expect(project_to.forked?).to be_truthy
-    end
-
-    it "project_from is not forked" do
-      expect(project_from.forked?).to be_falsey
-    end
-
-    it "project_to.destroy destroys fork_link" do
-      project_to.destroy
-
-      expect(ForkedProjectLink.exists?(id: forked_project_link.id)).to eq(false)
-    end
-  end
-end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 7cfffbde42f..972c1f273f7 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -22,7 +22,6 @@ describe Project do
     it { is_expected.to have_many(:deploy_keys) }
     it { is_expected.to have_many(:hooks) }
     it { is_expected.to have_many(:protected_branches) }
-    it { is_expected.to have_one(:forked_project_link) }
     it { is_expected.to have_one(:slack_service) }
     it { is_expected.to have_one(:microsoft_teams_service) }
     it { is_expected.to have_one(:mattermost_service) }
@@ -55,7 +54,7 @@ describe Project do
     it { is_expected.to have_one(:statistics).class_name('ProjectStatistics') }
     it { is_expected.to have_one(:import_data).class_name('ProjectImportData') }
     it { is_expected.to have_one(:last_event).class_name('Event') }
-    it { is_expected.to have_one(:forked_from_project).through(:forked_project_link) }
+    it { is_expected.to have_one(:forked_from_project).through(:fork_network_member) }
     it { is_expected.to have_one(:auto_devops).class_name('ProjectAutoDevops') }
     it { is_expected.to have_many(:commit_statuses) }
     it { is_expected.to have_many(:pipelines) }
@@ -76,7 +75,8 @@ describe Project do
     it { is_expected.to have_many(:lfs_objects_projects) }
     it { is_expected.to have_many(:project_group_links) }
     it { is_expected.to have_many(:notification_settings).dependent(:delete_all) }
-    it { is_expected.to have_many(:forks).through(:forked_project_links) }
+    it { is_expected.to have_many(:forked_to_members).class_name('ForkNetworkMember') }
+    it { is_expected.to have_many(:forks).through(:forked_to_members) }
     it { is_expected.to have_many(:uploads) }
     it { is_expected.to have_many(:pipeline_schedules) }
     it { is_expected.to have_many(:members_and_requesters) }
@@ -1346,7 +1346,7 @@ describe Project do
 
     context 'when checking on forked project' do
       let(:project)        { create(:project, :internal) }
-      let(:forked_project) { create(:project, forked_from_project: project) }
+      let(:forked_project) { fork_project(project) }
 
       it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::PRIVATE)).to be_truthy }
       it { expect(forked_project.visibility_level_allowed?(Gitlab::VisibilityLevel::INTERNAL)).to be_truthy }
@@ -1958,9 +1958,12 @@ describe Project do
     let(:import_jid) { '123' }
 
     context 'forked' do
-      let(:forked_project_link) { create(:forked_project_link, :forked_to_empty_project) }
-      let(:forked_from_project) { forked_project_link.forked_from_project }
-      let(:project) { forked_project_link.forked_to_project }
+      let(:forked_from_project) { create(:project, :repository) }
+      let(:project) { create(:project) }
+
+      before do
+        fork_project(forked_from_project, nil, target_project: project)
+      end
 
       it 'schedules a RepositoryForkWorker job' do
         expect(RepositoryForkWorker).to receive(:perform_async).with(project.id).and_return(import_jid)
@@ -2247,6 +2250,12 @@ describe Project do
       end
     end
 
+    describe '#forks' do
+      it 'includes direct forks of the project' do
+        expect(project.forks).to contain_exactly(forked_project)
+      end
+    end
+
     describe '#lfs_storage_project' do
       it 'returns self for non-forks' do
         expect(project.lfs_storage_project).to eq project
diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb
index 947cb61038d..9060fc22861 100644
--- a/spec/services/projects/fork_service_spec.rb
+++ b/spec/services/projects/fork_service_spec.rb
@@ -31,6 +31,10 @@ describe Projects::ForkService do
 
           it { is_expected.not_to be_persisted }
           it { expect(subject.errors[:forked_from_project_id]).to eq(['is forbidden']) }
+
+          it 'does not create a fork network' do
+            expect { subject }.not_to change { @from_project.reload.fork_network }
+          end
         end
 
         describe "successfully creates project in the user namespace" do
@@ -70,6 +74,12 @@ describe Projects::ForkService do
             expect(fork_network.root_project).to eq(@from_project)
             expect(fork_network.projects).to contain_exactly(@from_project, to_project)
           end
+
+          it 'imports the repository of the forked project' do
+            to_project = fork_project(@from_project, @to_user, repository: true)
+
+            expect(to_project.empty_repo?).to be_falsy
+          end
         end
 
         context 'creating a fork of a fork' do
@@ -247,11 +257,13 @@ describe Projects::ForkService do
 
     context 'if project is not forked' do
       it 'creates fork relation' do
-        expect(fork_to_project.forked?).to be false
+        expect(fork_to_project.forked?).to be_falsy
         expect(forked_from_project(fork_to_project)).to be_nil
 
         subject.execute(fork_to_project)
 
+        fork_to_project.reload
+
         expect(fork_to_project.forked?).to be true
         expect(forked_from_project(fork_to_project)).to eq fork_from_project
         expect(fork_to_project.forked_from_project).to eq fork_from_project
diff --git a/spec/support/helpers/project_forks_helper.rb b/spec/support/helpers/project_forks_helper.rb
index 2c501a2a27c..6a7132c3093 100644
--- a/spec/support/helpers/project_forks_helper.rb
+++ b/spec/support/helpers/project_forks_helper.rb
@@ -24,7 +24,7 @@ module ProjectForksHelper
       allow(service).to receive(:gitlab_shell).and_return(shell)
     end
 
-    forked_project = service.execute
+    forked_project = service.execute(params[:target_project])
 
     # Reload the both projects so they know about their newly created fork_network
     if forked_project.persisted?
diff --git a/spec/workers/rebase_worker_spec.rb b/spec/workers/rebase_worker_spec.rb
index 20aff020dbb..936b9deaecc 100644
--- a/spec/workers/rebase_worker_spec.rb
+++ b/spec/workers/rebase_worker_spec.rb
@@ -1,25 +1,23 @@
 require 'spec_helper'
 
 describe RebaseWorker, '#perform' do
+  include ProjectForksHelper
+
   context 'when rebasing an MR from a fork where upstream has protected branches' do
     let(:upstream_project) { create(:project, :repository) }
-    let(:fork_project) { create(:project, :repository) }
+    let(:forked_project) { fork_project(upstream_project, nil, repository: true) }
 
     let(:merge_request) do
       create(:merge_request,
-             source_project: fork_project,
+             source_project: forked_project,
              source_branch: 'feature_conflict',
              target_project: upstream_project,
              target_branch: 'master')
     end
 
-    before do
-      create(:forked_project_link, forked_to_project: fork_project, forked_from_project: upstream_project)
-    end
-
     it 'sets the correct project for running hooks' do
       expect(MergeRequests::RebaseService)
-        .to receive(:new).with(fork_project, merge_request.author).and_call_original
+        .to receive(:new).with(forked_project, merge_request.author).and_call_original
 
       subject.perform(merge_request, merge_request.author)
     end