Commit dab58c88 authored by Jan Provaznik's avatar Jan Provaznik

Merge branch '337102-backfill-usernamespace-for-every-personal-namespace' into 'master'

Ensure code can handle both UserNamespace and type == nil namespaces

See merge request gitlab-org/gitlab!68894
parents 5bf2452f 4804df2e
......@@ -101,7 +101,9 @@ class Namespace < ApplicationRecord
saved_change_to_name? || saved_change_to_path? || saved_change_to_parent_id?
}
scope :for_user, -> { where(type: nil) }
# TODO: change to `type: Namespaces::UserNamespace.sti_name` when
# working on issue https://gitlab.com/gitlab-org/gitlab/-/issues/341070
scope :user_namespaces, -> { where(type: [nil, Namespaces::UserNamespace.sti_name]) }
scope :sort_by_type, -> { order(Gitlab::Database.nulls_first_order(:type)) }
scope :include_route, -> { includes(:route) }
scope :by_parent, -> (parent) { where(parent_id: parent) }
......@@ -143,9 +145,7 @@ class Namespace < ApplicationRecord
when Namespaces::ProjectNamespace.sti_name
Namespaces::ProjectNamespace
when Namespaces::UserNamespace.sti_name
# TODO: We create a normal Namespace until
# https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68894 is ready
Namespace
Namespaces::UserNamespace
else
Namespace
end
......
# frozen_string_literal: true
# TODO: currently not created/mapped in the database, will be done in another issue
# https://gitlab.com/gitlab-org/gitlab/-/issues/337102
# https://gitlab.com/gitlab-org/gitlab/-/issues/341070
module Namespaces
class UserNamespace < Namespace
def self.sti_name
......
......@@ -112,7 +112,9 @@ class User < ApplicationRecord
#
# Namespace for personal projects
has_one :namespace, -> { where(type: nil) }, dependent: :destroy, foreign_key: :owner_id, inverse_of: :owner, autosave: true # rubocop:disable Cop/ActiveRecordDependent
# TODO: change to `type: Namespaces::UserNamespace.sti_name` when
# working on issue https://gitlab.com/gitlab-org/gitlab/-/issues/341070
has_one :namespace, -> { where(type: [nil, Namespaces::UserNamespace.sti_name]) }, dependent: :destroy, foreign_key: :owner_id, inverse_of: :owner, autosave: true # rubocop:disable Cop/ActiveRecordDependent
# Profile
has_many :keys, -> { regular_keys }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
......@@ -728,7 +730,7 @@ class User < ApplicationRecord
end
def find_by_full_path(path, follow_redirects: false)
namespace = Namespace.for_user.find_by_full_path(path, follow_redirects: follow_redirects)
namespace = Namespace.user_namespaces.find_by_full_path(path, follow_redirects: follow_redirects)
namespace&.owner
end
......
......@@ -452,16 +452,20 @@ module EE
end
end
# TODO: change to `type: ::Namespaces::UserNamespace.sti_name` when
# working on issue https://gitlab.com/gitlab-org/gitlab/-/issues/341070
def namespace_union_for_owned(select = :id)
::Gitlab::SQL::Union.new([
::Namespace.select(select).where(type: nil, owner: self),
::Namespace.select(select).where(type: [nil, ::Namespaces::UserNamespace.sti_name], owner: self),
owned_groups.select(select).where(parent_id: nil)
]).to_sql
end
# TODO: change to `type: ::Namespaces::UserNamespace.sti_name` when
# working on issue https://gitlab.com/gitlab-org/gitlab/-/issues/341070
def namespace_union_for_reporter_developer_maintainer_owned(select = :id)
::Gitlab::SQL::Union.new([
::Namespace.select(select).where(type: nil, owner: self),
::Namespace.select(select).where(type: [nil, ::Namespaces::UserNamespace.sti_name], owner: self),
reporter_developer_maintainer_owned_groups.select(select).where(parent_id: nil)
]).to_sql
end
......
......@@ -5,7 +5,9 @@ require 'spec_helper'
RSpec.describe 'Project Insights' do
it_behaves_like 'Insights page' do
let_it_be(:entity) { create(:project) }
let(:route) { url_for([entity.namespace, entity, :insights]) }
let(:path) { project_insights_path(entity) }
let(:route) do
project_insights_url(entity)
end
end
end
......@@ -1336,66 +1336,75 @@ RSpec.describe User do
end
context 'paid namespaces' do
let_it_be(:user) { create(:user) }
using RSpec::Parameterized::TableSyntax
let_it_be(:ultimate_group) { create(:group_with_plan, plan: :ultimate_plan) }
let_it_be(:bronze_group) { create(:group_with_plan, plan: :bronze_plan) }
let_it_be(:free_group) { create(:group_with_plan, plan: :free_plan) }
let_it_be(:group_without_plan) { create(:group) }
describe '#has_paid_namespace?' do
context 'when the user has Reporter or higher on at least one paid group' do
it 'returns true' do
ultimate_group.add_reporter(user)
bronze_group.add_guest(user)
# TODO: this `where/when` can be removed in issue https://gitlab.com/gitlab-org/gitlab/-/issues/341070
# At that point we only need to check `user_namespace`
where(namespace_type: [:namespace, :user_namespace])
with_them do
let(:user) { create(:user, namespace: create(namespace_type)) }
describe '#has_paid_namespace?' do
context 'when the user has Reporter or higher on at least one paid group' do
it 'returns true' do
ultimate_group.add_reporter(user)
bronze_group.add_guest(user)
expect(user.has_paid_namespace?).to eq(true)
expect(user.has_paid_namespace?).to eq(true)
end
end
end
context 'when the user is only a Guest on paid groups' do
it 'returns false' do
ultimate_group.add_guest(user)
bronze_group.add_guest(user)
free_group.add_owner(user)
context 'when the user is only a Guest on paid groups' do
it 'returns false' do
ultimate_group.add_guest(user)
bronze_group.add_guest(user)
free_group.add_owner(user)
expect(user.has_paid_namespace?).to eq(false)
expect(user.has_paid_namespace?).to eq(false)
end
end
end
context 'when the user is not a member of any groups with plans' do
it 'returns false' do
group_without_plan.add_owner(user)
context 'when the user is not a member of any groups with plans' do
it 'returns false' do
group_without_plan.add_owner(user)
expect(user.has_paid_namespace?).to eq(false)
expect(user.has_paid_namespace?).to eq(false)
end
end
end
end
describe '#owns_paid_namespace?' do
context 'when the user is an owner of at least one paid group' do
it 'returns true' do
ultimate_group.add_owner(user)
bronze_group.add_owner(user)
describe '#owns_paid_namespace?' do
context 'when the user is an owner of at least one paid group' do
it 'returns true' do
ultimate_group.add_owner(user)
bronze_group.add_owner(user)
expect(user.owns_paid_namespace?).to eq(true)
expect(user.owns_paid_namespace?).to eq(true)
end
end
end
context 'when the user is only a Maintainer on paid groups' do
it 'returns false' do
ultimate_group.add_maintainer(user)
bronze_group.add_maintainer(user)
free_group.add_owner(user)
context 'when the user is only a Maintainer on paid groups' do
it 'returns false' do
ultimate_group.add_maintainer(user)
bronze_group.add_maintainer(user)
free_group.add_owner(user)
expect(user.owns_paid_namespace?).to eq(false)
expect(user.owns_paid_namespace?).to eq(false)
end
end
end
context 'when the user is not a member of any groups with plans' do
it 'returns false' do
group_without_plan.add_owner(user)
context 'when the user is not a member of any groups with plans' do
it 'returns false' do
group_without_plan.add_owner(user)
expect(user.owns_paid_namespace?).to eq(false)
expect(user.owns_paid_namespace?).to eq(false)
end
end
end
end
......
# frozen_string_literal: true
FactoryBot.define do
factory :user_namespace, class: 'Namespaces::UserNamespace', parent: :namespace do
sequence(:name) { |n| "user_namespace#{n}" }
type { Namespaces::UserNamespace.sti_name }
end
end
......@@ -178,7 +178,7 @@ RSpec.describe Namespace do
context 'creating a Group' do
let(:namespace_type) { group_sti_name }
it 'is valid' do
it 'is the correct type of namespace' do
expect(namespace).to be_a(Group)
expect(namespace.kind).to eq('group')
expect(namespace.group_namespace?).to be_truthy
......@@ -189,7 +189,7 @@ RSpec.describe Namespace do
let(:namespace_type) { project_sti_name }
let(:parent) { create(:group) }
it 'is valid' do
it 'is the correct type of namespace' do
expect(Namespace.find(namespace.id)).to be_a(Namespaces::ProjectNamespace)
expect(namespace.kind).to eq('project')
expect(namespace.project_namespace?).to be_truthy
......@@ -199,10 +199,8 @@ RSpec.describe Namespace do
context 'creating a UserNamespace' do
let(:namespace_type) { user_sti_name }
it 'is valid' do
# TODO: We create a normal Namespace until
# https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68894 is ready
expect(Namespace.find(namespace.id)).to be_a(Namespace)
it 'is the correct type of namespace' do
expect(Namespace.find(namespace.id)).to be_a(Namespaces::UserNamespace)
expect(namespace.kind).to eq('user')
expect(namespace.user_namespace?).to be_truthy
end
......@@ -211,7 +209,7 @@ RSpec.describe Namespace do
context 'creating a default Namespace' do
let(:namespace_type) { nil }
it 'is valid' do
it 'is the correct type of namespace' do
expect(Namespace.find(namespace.id)).to be_a(Namespace)
expect(namespace.kind).to eq('user')
expect(namespace.user_namespace?).to be_truthy
......@@ -221,7 +219,7 @@ RSpec.describe Namespace do
context 'creating an unknown Namespace type' do
let(:namespace_type) { 'One' }
it 'defaults to a Namespace' do
it 'creates a default Namespace' do
expect(Namespace.find(namespace.id)).to be_a(Namespace)
expect(namespace.kind).to eq('user')
expect(namespace.user_namespace?).to be_truthy
......
# frozen_string_literal: true
require 'spec_helper'
# Main user namespace functionality it still in `Namespace`, so most
# of the specs are in `namespace_spec.rb`.
# UserNamespace specific specs will end up being migrated here.
RSpec.describe Namespaces::UserNamespace, type: :model do
describe 'validations' do
it { is_expected.to validate_presence_of(:owner) }
end
end
......@@ -2542,71 +2542,79 @@ RSpec.describe User do
end
describe '.find_by_full_path' do
let!(:user) { create(:user) }
using RSpec::Parameterized::TableSyntax
context 'with a route matching the given path' do
let!(:route) { user.namespace.route }
# TODO: this `where/when` can be removed in issue https://gitlab.com/gitlab-org/gitlab/-/issues/341070
# At that point we only need to check `user_namespace`
where(namespace_type: [:namespace, :user_namespace])
it 'returns the user' do
expect(described_class.find_by_full_path(route.path)).to eq(user)
end
with_them do
let!(:user) { create(:user, namespace: create(namespace_type)) }
it 'is case-insensitive' do
expect(described_class.find_by_full_path(route.path.upcase)).to eq(user)
expect(described_class.find_by_full_path(route.path.downcase)).to eq(user)
end
end
context 'with a route matching the given path' do
let!(:route) { user.namespace.route }
context 'with a redirect route matching the given path' do
let!(:redirect_route) { user.namespace.redirect_routes.create!(path: 'foo') }
it 'returns the user' do
expect(described_class.find_by_full_path(route.path)).to eq(user)
end
context 'without the follow_redirects option' do
it 'returns nil' do
expect(described_class.find_by_full_path(redirect_route.path)).to eq(nil)
it 'is case-insensitive' do
expect(described_class.find_by_full_path(route.path.upcase)).to eq(user)
expect(described_class.find_by_full_path(route.path.downcase)).to eq(user)
end
end
context 'with the follow_redirects option set to true' do
it 'returns the user' do
expect(described_class.find_by_full_path(redirect_route.path, follow_redirects: true)).to eq(user)
context 'with a redirect route matching the given path' do
let!(:redirect_route) { user.namespace.redirect_routes.create!(path: 'foo') }
context 'without the follow_redirects option' do
it 'returns nil' do
expect(described_class.find_by_full_path(redirect_route.path)).to eq(nil)
end
end
it 'is case-insensitive' do
expect(described_class.find_by_full_path(redirect_route.path.upcase, follow_redirects: true)).to eq(user)
expect(described_class.find_by_full_path(redirect_route.path.downcase, follow_redirects: true)).to eq(user)
context 'with the follow_redirects option set to true' do
it 'returns the user' do
expect(described_class.find_by_full_path(redirect_route.path, follow_redirects: true)).to eq(user)
end
it 'is case-insensitive' do
expect(described_class.find_by_full_path(redirect_route.path.upcase, follow_redirects: true)).to eq(user)
expect(described_class.find_by_full_path(redirect_route.path.downcase, follow_redirects: true)).to eq(user)
end
end
end
end
context 'without a route or a redirect route matching the given path' do
context 'without the follow_redirects option' do
it 'returns nil' do
expect(described_class.find_by_full_path('unknown')).to eq(nil)
context 'without a route or a redirect route matching the given path' do
context 'without the follow_redirects option' do
it 'returns nil' do
expect(described_class.find_by_full_path('unknown')).to eq(nil)
end
end
end
context 'with the follow_redirects option set to true' do
it 'returns nil' do
expect(described_class.find_by_full_path('unknown', follow_redirects: true)).to eq(nil)
context 'with the follow_redirects option set to true' do
it 'returns nil' do
expect(described_class.find_by_full_path('unknown', follow_redirects: true)).to eq(nil)
end
end
end
end
context 'with a group route matching the given path' do
let!(:group) { create(:group, path: 'group_path') }
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
before do
group.update!(owner_id: user.id)
end
context 'when the group namespace has an owner_id (legacy data)' do
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)
it 'returns nil' do
expect(described_class.find_by_full_path('group_path')).to eq(nil)
end
end
end
context 'when the group namespace does not have an owner_id' do
it 'returns nil' do
expect(described_class.find_by_full_path('group_path')).to eq(nil)
context 'when the group namespace does not have an owner_id' do
it 'returns nil' do
expect(described_class.find_by_full_path('group_path')).to eq(nil)
end
end
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment