Merge branch '42936-improve-ns-factory-avoid-duplicates' into 'master'

Resolve "Namespace factory is problematic"

Closes #42936

See merge request gitlab-org/gitlab-ce!18464

app/models/concerns/routable.rb

@@ -102,7 +102,7 @@ module Routable
   # the route. Caching this per request ensures that even if we have multiple instances,
   # we will not have to duplicate work, avoiding N+1 queries in some cases.
   def full_path
-    return uncached_full_path unless RequestStore.active?
+    return uncached_full_path unless RequestStore.active? && persisted?
 
     RequestStore[full_path_key] ||= uncached_full_path
   end
@@ -124,6 +124,11 @@ module Routable
     end
   end
 
+  # Group would override this to check from association
+  def owned_by?(user)
+    owner == user
+  end
+
   private
 
   def set_path_errors

app/models/group.rb

@@ -125,6 +125,10 @@ class Group < Namespace
     self[:lfs_enabled]
   end
 
+  def owned_by?(user)
+    owners.include?(user)
+  end
+
   def add_users(users, access_level, current_user: nil, expires_at: nil)
     GroupMember.add_users(
       self,

changelogs/unreleased/42936-improve-ns-factory-avoid-duplicates.yml

+---
+title: Fix discussions API setting created_at for notable in a group or notable in
+  a project in a group with owners
+merge_request: 18464
+author:
+type: fixed

lib/api/helpers/notes_helpers.rb

@@ -64,8 +64,10 @@ module API
         authorize! :create_note, noteable
 
         parent = noteable_parent(noteable)
+
         if opts[:created_at]
-          opts.delete(:created_at) unless current_user.admin? || parent.owner == current_user
+          opts.delete(:created_at) unless
+            current_user.admin? || parent.owned_by?(current_user)
         end
 
         project = parent if parent.is_a?(Project)

spec/controllers/import/bitbucket_controller_spec.rb

@@ -223,11 +223,12 @@ describe Import::BitbucketController do
     end
 
     context 'user has chosen an existing nested namespace and name for the project', :postgresql do
-      let(:parent_namespace) { create(:group, name: 'foo', owner: user) }
+      let(:parent_namespace) { create(:group, name: 'foo') }
       let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
       let(:test_name) { 'test_name' }
 
       before do
+        parent_namespace.add_owner(user)
         nested_namespace.add_owner(user)
       end
 
@@ -273,7 +274,7 @@ describe Import::BitbucketController do
 
     context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do
       let(:test_name) { 'test_name' }
-      let!(:parent_namespace) { create(:group, name: 'foo', owner: user) }
+      let!(:parent_namespace) { create(:group, name: 'foo') }
 
       before do
         parent_namespace.add_owner(user)

spec/controllers/import/gitlab_controller_spec.rb

@@ -196,10 +196,11 @@ describe Import::GitlabController do
       end
 
       context 'user has chosen an existing nested namespace for the project', :postgresql do
-        let(:parent_namespace) { create(:group, name: 'foo', owner: user) }
+        let(:parent_namespace) { create(:group, name: 'foo') }
         let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
 
         before do
+          parent_namespace.add_owner(user)
           nested_namespace.add_owner(user)
         end
 
@@ -245,7 +246,7 @@ describe Import::GitlabController do
 
       context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do
         let(:test_name) { 'test_name' }
-        let!(:parent_namespace) { create(:group, name: 'foo', owner: user) }
+        let!(:parent_namespace) { create(:group, name: 'foo') }
 
         before do
           parent_namespace.add_owner(user)

spec/controllers/projects/forks_controller_spec.rb

@@ -4,7 +4,11 @@ describe Projects::ForksController do
   let(:user) { create(:user) }
   let(:project) { create(:project, :public, :repository) }
   let(:forked_project) { Projects::ForkService.new(project, user).execute }
-  let(:group) { create(:group, owner: forked_project.creator) }
+  let(:group) { create(:group) }
+
+  before do
+    group.add_owner(user)
+  end
 
   describe 'GET index' do
     def get_forks

spec/factories/groups.rb

@@ -5,6 +5,14 @@ FactoryBot.define do
     type 'Group'
     owner nil
 
+    after(:create) do |group|
+      if group.owner
+        # We could remove this after we have proper constraint:
+        # https://gitlab.com/gitlab-org/gitlab-ce/issues/43292
+        raise "Don't set owner for groups, use `group.add_owner(user)` instead"
+      end
+    end
+
     trait :public do
       visibility_level Gitlab::VisibilityLevel::PUBLIC
     end

spec/factories/namespaces.rb

@@ -2,6 +2,22 @@ FactoryBot.define do
   factory :namespace do
     sequence(:name) { |n| "namespace#{n}" }
     path { name.downcase.gsub(/\s/, '_') }
-    owner
+
+    # This is a workaround to avoid the user creating another namespace via
+    # User#ensure_namespace_correct. We should try to remove it and then
+    # we could remove this workaround
+    association :owner, factory: :user, strategy: :build
+    before(:create) do |namespace|
+      owner = namespace.owner
+
+      if owner
+        # We're changing the username here because we want to keep our path,
+        # and User#ensure_namespace_correct would change the path based on
+        # username, so we're forced to do this otherwise we'll need to change
+        # a lot of existing tests.
+        owner.username = namespace.path
+        owner.namespace = namespace
+      end
+    end
   end
 end

spec/features/projects/new_project_spec.rb

@@ -64,7 +64,7 @@ feature 'New project' do
     end
 
     context 'with group namespace' do
-      let(:group) { create(:group, :private, owner: user) }
+      let(:group) { create(:group, :private) }
 
       before do
         group.add_owner(user)
@@ -81,7 +81,7 @@ feature 'New project' do
     end
 
     context 'with subgroup namespace' do
-      let(:group) { create(:group, owner: user) }
+      let(:group) { create(:group) }
       let(:subgroup) { create(:group, parent: group) }
 
       before do

spec/lib/gitlab/bitbucket_import/project_creator_spec.rb

@@ -15,7 +15,7 @@ describe Gitlab::BitbucketImport::ProjectCreator do
            has_wiki?: false)
   end
 
-  let(:namespace) { create(:group, owner: user) }
+  let(:namespace) { create(:group) }
   let(:token) { "asdasd12345" }
   let(:secret) { "sekrettt" }
   let(:access_params) { { bitbucket_access_token: token, bitbucket_access_token_secret: secret } }

spec/lib/gitlab/gitlab_import/project_creator_spec.rb

@@ -12,7 +12,7 @@ describe Gitlab::GitlabImport::ProjectCreator do
       owner: { name: "john" }
     }.with_indifferent_access
   end
-  let(:namespace) { create(:group, owner: user) }
+  let(:namespace) { create(:group) }
   let(:token) { "asdffg" }
   let(:access_params) { { gitlab_access_token: token } }
 

spec/lib/gitlab/google_code_import/project_creator_spec.rb

@@ -9,7 +9,7 @@ describe Gitlab::GoogleCodeImport::ProjectCreator do
       "repositoryUrls" => ["https://vim.googlecode.com/git/"]
     )
   end
-  let(:namespace) { create(:group, owner: user) }
+  let(:namespace) { create(:group) }
 
   before do
     namespace.add_owner(user)

spec/lib/gitlab/legacy_github_import/project_creator_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Gitlab::LegacyGithubImport::ProjectCreator do
   let(:user) { create(:user) }
-  let(:namespace) { create(:group, owner: user) }
+  let(:namespace) { create(:group) }
 
   let(:repo) do
     OpenStruct.new(

spec/models/project_spec.rb

@@ -325,7 +325,7 @@ describe Project do
     let(:owner)     { create(:user, name: 'Gitlab') }
     let(:namespace) { create(:namespace, path: 'sample-namespace', owner: owner) }
     let(:project)   { create(:project, path: 'sample-project', namespace: namespace) }
-    let(:group)     { create(:group, name: 'Group', path: 'sample-group', owner: owner) }
+    let(:group)     { create(:group, name: 'Group', path: 'sample-group') }
 
     context 'when nil argument' do
       it 'returns nil' do

spec/models/user_spec.rb

@@ -1164,8 +1164,12 @@ describe User do
     end
 
     context 'with a group route matching the given path' do
+      let!(:group) { create(:group, path: 'group_path') }
+
       context 'when the group namespace has an owner_id (legacy data)' do
-        let!(:group) { create(:group, path: 'group_path', owner: user) }
+        before do
+          group.update!(owner_id: user.id)
+        end
 
         it 'returns nil' do
           expect(described_class.find_by_full_path('group_path')).to eq(nil)
@@ -1173,8 +1177,6 @@ describe User do
       end
 
       context 'when the group namespace does not have an owner_id' do
-        let!(:group) { create(:group, path: 'group_path') }
-
         it 'returns nil' do
           expect(described_class.find_by_full_path('group_path')).to eq(nil)
         end

spec/services/groups/nested_create_service_spec.rb

@@ -59,8 +59,11 @@ describe Groups::NestedCreateService do
 
     describe "#execute" do
       it 'returns the group if it already existed' do
-        parent = create(:group, path: 'a-group', owner: user)
-        child = create(:group, path: 'a-sub-group', parent: parent, owner: user)
+        parent = create(:group, path: 'a-group')
+        child = create(:group, path: 'a-sub-group', parent: parent)
+
+        parent.add_owner(user)
+        child.add_owner(user)
 
         expect(service.execute).to eq(child)
       end

spec/support/controllers/githubish_import_controller_shared_examples.rb

@@ -56,7 +56,7 @@ shared_examples 'a GitHub-ish import controller: GET status' do
   end
 
   it "assigns variables" do
-    project = create(:project, import_type: provider, creator_id: user.id)
+    project = create(:project, import_type: provider, namespace: user.namespace)
     stub_client(repos: [repo, org_repo], orgs: [org], org_repos: [org_repo])
 
     get :status
@@ -69,7 +69,7 @@ shared_examples 'a GitHub-ish import controller: GET status' do
   end
 
   it "does not show already added project" do
-    project = create(:project, import_type: provider, creator_id: user.id, import_source: 'asd/vim')
+    project = create(:project, import_type: provider, namespace: user.namespace, import_source: 'asd/vim')
     stub_client(repos: [repo], orgs: [])
 
     get :status
@@ -257,11 +257,12 @@ shared_examples 'a GitHub-ish import controller: POST create' do
     end
 
     context 'user has chosen an existing nested namespace and name for the project', :postgresql do
-      let(:parent_namespace) { create(:group, name: 'foo', owner: user) }
+      let(:parent_namespace) { create(:group, name: 'foo') }
       let(:nested_namespace) { create(:group, name: 'bar', parent: parent_namespace) }
       let(:test_name) { 'test_name' }
 
       before do
+        parent_namespace.add_owner(user)
         nested_namespace.add_owner(user)
       end
 
@@ -307,7 +308,7 @@ shared_examples 'a GitHub-ish import controller: POST create' do
 
     context 'user has chosen existent and non-existent nested namespaces and name for the project', :postgresql do
       let(:test_name) { 'test_name' }
-      let!(:parent_namespace) { create(:group, name: 'foo', owner: user) }
+      let!(:parent_namespace) { create(:group, name: 'foo') }
 
       before do
         parent_namespace.add_owner(user)