From 82526900e4e6bbec17eecfb2bec3a7f83d188121 Mon Sep 17 00:00:00 2001 From: hustewart Date: Mon, 24 Jun 2024 14:28:34 -0400 Subject: [PATCH 1/8] Start drafting test --- app/services/snippets/create_service.rb | 1 + spec/services/snippets/create_service_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index 0dfce8f59d7eef..cac7470f8407c5 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -58,6 +58,7 @@ def create_params end def save_and_commit + # binding.pry snippet_saved = @snippet.save if snippet_saved diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index f5d9b997224604..08c22057da9c32 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -366,6 +366,14 @@ let(:extra_opts) { { description: description, title: title, files: files } } + it 'sets an organization_id' do + stub_file_mover(text_file) + stub_file_mover(picture_file) + + snippet = subject.payload[:snippet] + expect(snippet.organization_id).to eq(expected_description) + end + it 'stores the snippet description correctly' do stub_file_mover(text_file) stub_file_mover(picture_file) -- GitLab From c20300203f5a73b08295ed160f1699c2d9748638 Mon Sep 17 00:00:00 2001 From: hustewart Date: Fri, 28 Jun 2024 09:47:26 -0400 Subject: [PATCH 2/8] Set PersonalSnippet organization_id This commit uses the Current.organization_id to attempt to set the value on PersonalSnippets in their create service. If no value is returned, the default value of 1 is used. --- app/models/snippet.rb | 6 ++- app/services/snippets/create_service.rb | 9 +++- spec/models/snippet_spec.rb | 1 + spec/services/snippets/create_service_spec.rb | 41 +++++++++++++++---- 4 files changed, 46 insertions(+), 11 deletions(-) diff --git a/app/models/snippet.rb b/app/models/snippet.rb index c8858a7e1e660a..7b76d8fe6e06ab 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -28,6 +28,10 @@ class Snippet < ApplicationRecord DESCRIPTION_LENGTH_MAX = 1.megabyte + # Default is set in database + # db/migrate/20240606124806_add_organization_id_to_snippets.rb + DEFAULT_ORGANIZATION_ID = 1 + cache_markdown_field :title, pipeline: :single_line cache_markdown_field :description cache_markdown_field :content @@ -60,7 +64,7 @@ def content_html_invalidated? delegate :name, :email, to: :author, prefix: true, allow_nil: true - validates :author, presence: true + validates :author, :organization_id, presence: true validates :title, presence: true, length: { maximum: 255 } validates :file_name, length: { maximum: 255 } diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index cac7470f8407c5..f71290326c1d28 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -44,10 +44,16 @@ def build_from_params if project project.snippets.build(create_params) else - PersonalSnippet.new(create_params) + PersonalSnippet.new( + create_params.merge(organization_id: organization_id) + ) end end + def organization_id + Current.organization_id || Snippet::DEFAULT_ORGANIZATION_ID + end + # If the snippet_actions param is present # we need to fill content and file_name from # the model @@ -58,7 +64,6 @@ def create_params end def save_and_commit - # binding.pry snippet_saved = @snippet.save if snippet_saved diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index fcb6fe8b1dddf2..1c36765c3d7985 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -45,6 +45,7 @@ describe 'validation' do it { is_expected.to validate_presence_of(:author) } + it { is_expected.to validate_presence_of(:organization_id) } it { is_expected.to validate_presence_of(:title) } it { is_expected.to validate_length_of(:title).is_at_most(255) } diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 08c22057da9c32..e7203a0b09eb87 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -330,6 +330,20 @@ subject end end + + context 'when Current.organization is set', :with_current_organization do + it 'still uses the default organization_id' do + snippet = subject.payload[:snippet] + expect(snippet.organization_id).to eq(Snippet::DEFAULT_ORGANIZATION_ID) + end + end + + context 'when Current.organization is not set' do + it 'still uses the default organization_id' do + snippet = subject.payload[:snippet] + expect(snippet.organization_id).to eq(Snippet::DEFAULT_ORGANIZATION_ID) + end + end end context 'when PersonalSnippet' do @@ -345,6 +359,25 @@ 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 + snippet = subject.payload[:snippet] + expect(snippet.organization_id).to eq(Current.organization_id) + end + + it 'does not set organization_id to the default organization' do + snippet = subject.payload[:snippet] + expect(snippet.organization_id).not_to eq(Snippet::DEFAULT_ORGANIZATION_ID) + end + end + + context 'when Current.organization is not set' do + it 'still uses the default organization_id' do + snippet = subject.payload[:snippet] + expect(snippet.organization_id).to eq(Snippet::DEFAULT_ORGANIZATION_ID) + end + end + context 'when the snippet description contains files' do include FileMoverHelpers @@ -366,14 +399,6 @@ let(:extra_opts) { { description: description, title: title, files: files } } - it 'sets an organization_id' do - stub_file_mover(text_file) - stub_file_mover(picture_file) - - snippet = subject.payload[:snippet] - expect(snippet.organization_id).to eq(expected_description) - end - it 'stores the snippet description correctly' do stub_file_mover(text_file) stub_file_mover(picture_file) -- GitLab From 9768c44e9212b958ec3fdce265e10470e2b327cc Mon Sep 17 00:00:00 2001 From: hustewart Date: Fri, 28 Jun 2024 10:58:03 -0400 Subject: [PATCH 3/8] Change nullifcation and validation approach It turns out that we want to - nullify the organization_id column for project based snippets - validate presence of organization_id only on personal snippets - allow default value for personal snippets of no organization is selected This commit adds those changes and updates specs. --- app/models/personal_snippet.rb | 2 ++ app/models/snippet.rb | 2 +- app/services/snippets/create_service.rb | 4 +++- spec/models/personal_snippet_spec.rb | 4 ++++ spec/models/snippet_spec.rb | 1 - spec/services/snippets/create_service_spec.rb | 4 ++-- 6 files changed, 12 insertions(+), 5 deletions(-) diff --git a/app/models/personal_snippet.rb b/app/models/personal_snippet.rb index 299f1f7a63048f..0a51bfd55f873b 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/models/snippet.rb b/app/models/snippet.rb index 7b76d8fe6e06ab..1b760d3af92be2 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -64,7 +64,7 @@ def content_html_invalidated? delegate :name, :email, to: :author, prefix: true, allow_nil: true - validates :author, :organization_id, presence: true + validates :author, presence: true validates :title, presence: true, length: { maximum: 255 } validates :file_name, length: { maximum: 255 } diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index f71290326c1d28..6db0be245f6062 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -42,7 +42,9 @@ def execute 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.merge(organization_id: organization_id) diff --git a/spec/models/personal_snippet_spec.rb b/spec/models/personal_snippet_spec.rb index 212605445ff864..ef5564739297f5 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 'validation' do + it { is_expected.to validate_presence_of(:organization_id) } + end + describe '#embeddable?' do [ { snippet: :public, embeddable: true }, diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index 1c36765c3d7985..fcb6fe8b1dddf2 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -45,7 +45,6 @@ describe 'validation' do it { is_expected.to validate_presence_of(:author) } - it { is_expected.to validate_presence_of(:organization_id) } it { is_expected.to validate_presence_of(:title) } it { is_expected.to validate_length_of(:title).is_at_most(255) } diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index e7203a0b09eb87..1978f2966b130b 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -334,14 +334,14 @@ context 'when Current.organization is set', :with_current_organization do it 'still uses the default organization_id' do snippet = subject.payload[:snippet] - expect(snippet.organization_id).to eq(Snippet::DEFAULT_ORGANIZATION_ID) + expect(snippet.organization_id).to be_nil end end context 'when Current.organization is not set' do it 'still uses the default organization_id' do snippet = subject.payload[:snippet] - expect(snippet.organization_id).to eq(Snippet::DEFAULT_ORGANIZATION_ID) + expect(snippet.organization_id).to be_nil end end end -- GitLab From 3e51f8b03fa1772e027b993277b27ccab8ee65ce Mon Sep 17 00:00:00 2001 From: Hunter Stewart Date: Mon, 1 Jul 2024 12:03:51 +0000 Subject: [PATCH 4/8] Apply 3 suggestion(s) to 3 file(s) Co-authored-by: Javiera Tapia Co-authored-by: Vasilii Iakliushin --- app/services/snippets/create_service.rb | 2 +- spec/models/personal_snippet_spec.rb | 2 +- spec/services/snippets/create_service_spec.rb | 6 ++---- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index 6db0be245f6062..1b0c58c1686baf 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -53,7 +53,7 @@ def build_from_params end def organization_id - Current.organization_id || Snippet::DEFAULT_ORGANIZATION_ID + Current.organization_id || Organizations::Organization::DEFAULT_ORGANIZATION_ID end # If the snippet_actions param is present diff --git a/spec/models/personal_snippet_spec.rb b/spec/models/personal_snippet_spec.rb index ef5564739297f5..d74ba1f5483e7d 100644 --- a/spec/models/personal_snippet_spec.rb +++ b/spec/models/personal_snippet_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe PersonalSnippet do - describe 'validation' do + describe 'validations' do it { is_expected.to validate_presence_of(:organization_id) } end diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index 1978f2966b130b..eca43b07fe69ea 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -332,15 +332,13 @@ end context 'when Current.organization is set', :with_current_organization do - it 'still uses the default organization_id' do - snippet = subject.payload[:snippet] + 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 'still uses the default organization_id' do - snippet = subject.payload[:snippet] + it 'sets the organization_id to nil' do expect(snippet.organization_id).to be_nil end end -- GitLab From 6b7921fa166d976fcee8f3501e70d1978cf2247a Mon Sep 17 00:00:00 2001 From: hustewart Date: Mon, 1 Jul 2024 09:57:29 -0400 Subject: [PATCH 5/8] Update constant usage, simplify more specs --- app/models/snippet.rb | 4 ---- spec/services/snippets/create_service_spec.rb | 9 ++++----- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 1b760d3af92be2..c8858a7e1e660a 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -28,10 +28,6 @@ class Snippet < ApplicationRecord DESCRIPTION_LENGTH_MAX = 1.megabyte - # Default is set in database - # db/migrate/20240606124806_add_organization_id_to_snippets.rb - DEFAULT_ORGANIZATION_ID = 1 - cache_markdown_field :title, pipeline: :single_line cache_markdown_field :description cache_markdown_field :content diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index eca43b07fe69ea..60f1b0de922044 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -359,20 +359,19 @@ context 'when Current.organization is set', :with_current_organization do it 'sets the organization_id to the current organization' do - snippet = subject.payload[:snippet] expect(snippet.organization_id).to eq(Current.organization_id) end it 'does not set organization_id to the default organization' do - snippet = subject.payload[:snippet] - expect(snippet.organization_id).not_to eq(Snippet::DEFAULT_ORGANIZATION_ID) + 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 - snippet = subject.payload[:snippet] - expect(snippet.organization_id).to eq(Snippet::DEFAULT_ORGANIZATION_ID) + expect(snippet.organization_id) + .to eq(Organizations::Organization::DEFAULT_ORGANIZATION_ID) end end -- GitLab From e66fc1c01d85168b8bf612f90076c5b65ca3f59a Mon Sep 17 00:00:00 2001 From: hustewart Date: Mon, 1 Jul 2024 11:05:11 -0400 Subject: [PATCH 6/8] Add comment and issue link --- app/services/snippets/create_service.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index 1b0c58c1686baf..5dbfbee1a266aa 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -40,6 +40,10 @@ 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( -- GitLab From 51619f9144e8c0f34dfc820403172ae682c89eef Mon Sep 17 00:00:00 2001 From: hustewart Date: Mon, 1 Jul 2024 15:33:14 -0400 Subject: [PATCH 7/8] Conditionally add organization_id in gist_importer This class creates PeronsalSnippet without using the Snippets::CreateService so we need to update it in this same MR. We have an issue to refactor the service which should also handle updating the gist_importer to make use of the service. Link to refactor issue: https://gitlab.com/gitlab-org/gitlab/-/issues/469564 --- .../importer/gist_importer.rb | 11 ++++++ .../importer/gist_importer_spec.rb | 38 ++++++++++++++----- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/github_gists_import/importer/gist_importer.rb b/lib/gitlab/github_gists_import/importer/gist_importer.rb index e26c1c3729ee75..2fa35f2bc28913 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 f07486547e0d25..78ca9bffef6221 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(: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 -- GitLab From 60b582cb7b0bb04a9d916f3d41f8e95b950716cc Mon Sep 17 00:00:00 2001 From: hustewart Date: Tue, 2 Jul 2024 10:44:18 -0400 Subject: [PATCH 8/8] Use let it be with reload --- .../gitlab/github_gists_import/importer/gist_importer_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 78ca9bffef6221..25fcb9c580ff71 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(: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' } } -- GitLab