diff --git a/app/models/personal_snippet.rb b/app/models/personal_snippet.rb index 299f1f7a63048f0f23d3c81943aa777c7d0601c5..0a51bfd55f873b74cad25bd047add868d4bd6a1d 100644 --- a/app/models/personal_snippet.rb +++ b/app/models/personal_snippet.rb @@ -5,6 +5,8 @@ class PersonalSnippet < Snippet include WithUploads + validates :organization_id, presence: true + def parent_user author end diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index 0dfce8f59d7eefbe06634655a96f4954f04e029c..5dbfbee1a266aa90f2f86455b06c68190643c1c7 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -40,14 +40,26 @@ def execute attr_reader :snippet, :perform_spam_check + # If the snippet is a "project snippet", specifically set it to nil to override the default database value of 1. + # We only want organization_id on the PersonalSnippet subclass. + # + # See https://gitlab.com/gitlab-org/gitlab/-/issues/460827 def build_from_params if project - project.snippets.build(create_params) + project.snippets.build( + create_params.merge(organization_id: nil) + ) else - PersonalSnippet.new(create_params) + PersonalSnippet.new( + create_params.merge(organization_id: organization_id) + ) end end + def organization_id + Current.organization_id || Organizations::Organization::DEFAULT_ORGANIZATION_ID + end + # If the snippet_actions param is present # we need to fill content and file_name from # the model diff --git a/lib/gitlab/github_gists_import/importer/gist_importer.rb b/lib/gitlab/github_gists_import/importer/gist_importer.rb index e26c1c3729ee752d941b48523ba7918872ddbeb0..2fa35f2bc28913b29eb71b333dbf506930583277 100644 --- a/lib/gitlab/github_gists_import/importer/gist_importer.rb +++ b/lib/gitlab/github_gists_import/importer/gist_importer.rb @@ -18,6 +18,9 @@ def initialize(gist, user_id) @user = User.find(user_id) end + # Marking for refactor in: + # https://gitlab.com/gitlab-org/gitlab/-/issues/469564 + # We'll want to use the Snippets::CreateService here. def execute validate_gist! @@ -32,8 +35,16 @@ def execute private + # Duplicate logic in + # app/services/snippets/create_service.rb + # Remove as part of refactor in: https://gitlab.com/gitlab-org/gitlab/-/issues/469564 + def organization_id + Current.organization_id || Organizations::Organization::DEFAULT_ORGANIZATION_ID + end + def build_snippet attrs = { + organization_id: organization_id, title: gist.truncated_title, visibility_level: gist.visibility_level, content: gist.first_file[:file_content], diff --git a/spec/lib/gitlab/github_gists_import/importer/gist_importer_spec.rb b/spec/lib/gitlab/github_gists_import/importer/gist_importer_spec.rb index f07486547e0d25f67e56a6d925026e14038cd331..25fcb9c580ff71efeaad4781ec956139deff39ec 100644 --- a/spec/lib/gitlab/github_gists_import/importer/gist_importer_spec.rb +++ b/spec/lib/gitlab/github_gists_import/importer/gist_importer_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Gitlab::GithubGistsImport::Importer::GistImporter, feature_category: :importers do subject { described_class.new(gist_object, user.id) } - let_it_be(:user) { create(:user) } + let_it_be_with_reload(:user) { create(:user) } let(:created_at) { Time.utc(2022, 1, 9, 12, 15) } let(:updated_at) { Time.utc(2022, 5, 9, 12, 17) } let(:gist_file) { { file_name: '_Summary.md', file_content: 'File content' } } @@ -41,17 +41,37 @@ instance_double(ServiceResponse, error?: false) end - it 'creates expected snippet and snippet repository' do - expect_next_instance_of(Snippets::RepositoryValidationService) do |validator| - expect(validator).to receive(:execute).and_return(validator_result) - end + context 'when Current.organization is not set', :with_current_organization do + it 'creates expected snippet and snippet repository' do + expect_next_instance_of(Snippets::RepositoryValidationService) do |validator| + expect(validator).to receive(:execute).and_return(validator_result) + end - expect_next_instance_of(Repository) do |repository| - expect(repository).to receive(:fetch_as_mirror) + expect_next_instance_of(Repository) do |repository| + expect(repository).to receive(:fetch_as_mirror) + end + + expect { subject.execute }.to change { user.snippets.count }.by(1) + expect(user.snippets[0].attributes).to include expected_snippet_attrs + expect(user.snippets[0].organization_id).to eq(Current.organization_id) end + end - expect { subject.execute }.to change { user.snippets.count }.by(1) - expect(user.snippets[0].attributes).to include expected_snippet_attrs + context 'when Current.organization is not set' do + it 'still uses the default organization_id' do + expect_next_instance_of(Snippets::RepositoryValidationService) do |validator| + expect(validator).to receive(:execute).and_return(validator_result) + end + + expect_next_instance_of(Repository) do |repository| + expect(repository).to receive(:fetch_as_mirror) + end + + expect { subject.execute }.to change { user.snippets.count }.by(1) + expect(user.snippets[0].attributes).to include expected_snippet_attrs + expect(user.snippets[0].organization_id) + .to eq(Organizations::Organization::DEFAULT_ORGANIZATION_ID) + end end end diff --git a/spec/models/personal_snippet_spec.rb b/spec/models/personal_snippet_spec.rb index 212605445ff8642a5d1645949c7b944d17b3f19a..d74ba1f5483e7df9e96cf3557959c5f34e4777ab 100644 --- a/spec/models/personal_snippet_spec.rb +++ b/spec/models/personal_snippet_spec.rb @@ -3,6 +3,10 @@ require 'spec_helper' RSpec.describe PersonalSnippet do + describe 'validations' do + it { is_expected.to validate_presence_of(:organization_id) } + end + describe '#embeddable?' do [ { snippet: :public, embeddable: true }, diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index f5d9b99722460403f4953e10f67ac8db054ba79f..60f1b0de922044a8cba11ee4b3113d76985782a2 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -330,6 +330,18 @@ subject end end + + context 'when Current.organization is set', :with_current_organization do + it 'sets the organization_id to nil' do + expect(snippet.organization_id).to be_nil + end + end + + context 'when Current.organization is not set' do + it 'sets the organization_id to nil' do + expect(snippet.organization_id).to be_nil + end + end end context 'when PersonalSnippet' do @@ -345,6 +357,24 @@ it_behaves_like 'when snippet_actions param is present' it_behaves_like 'invalid params error response' + context 'when Current.organization is set', :with_current_organization do + it 'sets the organization_id to the current organization' do + expect(snippet.organization_id).to eq(Current.organization_id) + end + + it 'does not set organization_id to the default organization' do + expect(snippet.organization_id) + .not_to eq(Organizations::Organization::DEFAULT_ORGANIZATION_ID) + end + end + + context 'when Current.organization is not set' do + it 'still uses the default organization_id' do + expect(snippet.organization_id) + .to eq(Organizations::Organization::DEFAULT_ORGANIZATION_ID) + end + end + context 'when the snippet description contains files' do include FileMoverHelpers