Merge branch 'fj-remove-flag-version-snippets' into 'master'

Remove feature flag version_snippets

See merge request gitlab-org/gitlab!27705

app/controllers/concerns/snippets_actions.rb

@@ -53,10 +53,10 @@ module SnippetsActions
   def blob
     return unless snippet
 
-    @blob ||= if Feature.enabled?(:version_snippets, current_user) && !snippet.repository.empty?
-                snippet.blobs.first
-              else
+    @blob ||= if snippet.empty_repo?
                 snippet.blob
+              else
+                snippet.blobs.first
               end
   end
   # rubocop:enable Gitlab/ModuleWithInstanceVariables

app/controllers/repositories/git_http_controller.rb

@@ -4,7 +4,6 @@ module Repositories
   class GitHttpController < Repositories::GitHttpClientController
     include WorkhorseRequest
 
-    before_action :snippet_request_allowed?
     before_action :access_check
     prepend_before_action :deny_head_requests, only: [:info_refs]
 
@@ -121,13 +120,6 @@ module Repositories
     def log_user_activity
       Users::ActivityService.new(user).execute
     end
-
-    def snippet_request_allowed?
-      if repo_type.snippet? && Feature.disabled?(:version_snippets, user)
-        Gitlab::AppLogger.info('Snippet access attempt with feature disabled')
-        render plain: 'Snippet git access is disabled.', status: :forbidden
-      end
-    end
   end
 end
 

app/models/snippet.rb

@@ -319,10 +319,6 @@ class Snippet < ApplicationRecord
     Digest::SHA256.hexdigest("#{title}#{description}#{created_at}#{updated_at}")
   end
 
-  def versioned_enabled_for?(user)
-    ::Feature.enabled?(:version_snippets, user) && repository_exists?
-  end
-
   def file_name_on_repo
     return if repository.empty?
 

app/presenters/snippet_presenter.rb

@@ -12,11 +12,11 @@ class SnippetPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def ssh_url_to_repo
-    snippet.ssh_url_to_repo if snippet.versioned_enabled_for?(current_user)
+    snippet.ssh_url_to_repo if snippet.repository_exists?
   end
 
   def http_url_to_repo
-    snippet.http_url_to_repo if snippet.versioned_enabled_for?(current_user)
+    snippet.http_url_to_repo if snippet.repository_exists?
   end
 
   def can_read_snippet?
@@ -36,10 +36,10 @@ class SnippetPresenter < Gitlab::View::Presenter::Delegated
   end
 
   def blob
-    if Feature.enabled?(:version_snippets, current_user) && !snippet.repository.empty?
-      snippet.blobs.first
-    else
+    if snippet.empty_repo?
       snippet.blob
+    else
+      snippet.blobs.first
     end
   end
 

app/services/snippets/create_service.rb

@@ -40,7 +40,7 @@ module Snippets
     def save_and_commit
       snippet_saved = @snippet.save
 
-      if snippet_saved && Feature.enabled?(:version_snippets, current_user)
+      if snippet_saved
         create_repository
         create_commit
       end

app/services/snippets/update_service.rb

@@ -43,17 +43,8 @@ module Snippets
       # the repository we can just return
       return true unless committable_attributes?
 
-      # In order to avoid non migrated snippets scenarios,
-      # if the snippet does not have a repository we created it
-      # We don't need to check if the repository exists
-      # because `create_repository` already handles it
-      if Feature.enabled?(:version_snippets, current_user)
-        create_repository_for(snippet)
-      end
-
-      # If the snippet repository exists we commit always
-      # the changes
-      create_commit(snippet) if snippet.repository_exists?
+      create_repository_for(snippet)
+      create_commit(snippet)
 
       true
     rescue => e

changelogs/unreleased/fj-remove-flag-version-snippets.yml

+---
+title: Enabling git versioned snippets
+merge_request: 27705
+author:
+type: added

lib/api/entities/snippet.rb

@@ -13,10 +13,9 @@ module API
       expose :raw_url do |snippet|
         Gitlab::UrlBuilder.build(snippet, raw: true)
       end
-      expose :ssh_url_to_repo, :http_url_to_repo, if: ->(snippet) { snippet.versioned_enabled_for?(options[:current_user]) }
+      expose :ssh_url_to_repo, :http_url_to_repo, if: ->(snippet) { snippet.repository_exists? }
       expose :file_name do |snippet|
-        (::Feature.enabled?(:version_snippets, options[:current_user]) && snippet.file_name_on_repo) ||
-          snippet.file_name
+        snippet.file_name_on_repo || snippet.file_name
       end
     end
   end

lib/api/helpers/snippets_helpers.rb

@@ -4,12 +4,12 @@ module API
   module Helpers
     module SnippetsHelpers
       def content_for(snippet)
-        if ::Feature.enabled?(:version_snippets, current_user) && !snippet.empty_repo?
+        if snippet.empty_repo?
+          snippet.content
+        else
           blob = snippet.blobs.first
           blob.load_all_data!
           blob.data
-        else
-          snippet.content
         end
       end
     end

lib/api/internal/base.rb

@@ -112,10 +112,6 @@ module API
         #   check_ip - optional, only in EE version, may limit access to
         #     group resources based on its IP restrictions
         post "/allowed" do
-          if repo_type.snippet? && params[:protocol] != 'web' && Feature.disabled?(:version_snippets, actor.user)
-            break response_with_status(code: 401, success: false, message: 'Snippet git access is disabled.')
-          end
-
           # It was moved to a separate method so that EE can alter its behaviour more
           # easily.
           check_allowed(params)

lib/gitlab/import_export/snippets_repo_restorer.rb

@@ -10,7 +10,6 @@ module Gitlab
       end
 
       def restore
-        return true unless Feature.enabled?(:version_snippets, @user)
         return true unless Dir.exist?(snippets_repo_bundle_path)
 
         @project.snippets.find_each.all? do |snippet|

lib/gitlab/import_export/snippets_repo_saver.rb

@@ -12,8 +12,6 @@ module Gitlab
       end
 
       def save
-        return true unless Feature.enabled?(:version_snippets, @current_user)
-
         create_snippets_repo_directory
 
         @project.snippets.find_each.all? do |snippet|

spec/controllers/projects/snippets_controller_spec.rb

@@ -346,20 +346,6 @@ describe Projects::SnippetsController do
 
       expect(assigns(:blob)).to eq(project_snippet.blobs.first)
     end
-
-    context 'when feature flag version_snippets is disabled' do
-      before do
-        stub_feature_flags(version_snippets: false)
-      end
-
-      it 'returns the snippet database content' do
-        subject
-
-        blob = assigns(:blob)
-
-        expect(blob.data).to eq(project_snippet.content)
-      end
-    end
   end
 
   %w[show raw].each do |action|

spec/controllers/repositories/git_http_controller_spec.rb

@@ -165,44 +165,6 @@ describe Repositories::GitHttpController do
     end
   end
 
-  shared_examples 'snippet feature flag disabled behavior' do
-    before do
-      stub_feature_flags(version_snippets: false)
-
-      request.headers.merge! auth_env(user.username, user.password, nil)
-    end
-
-    describe 'GET #info_refs' do
-      let(:params) { container_params.merge(service: 'git-upload-pack') }
-
-      it 'returns 403' do
-        expect(controller).not_to receive(:access_check)
-
-        get :info_refs, params: params
-
-        expect(response).to have_gitlab_http_status(:forbidden)
-        expect(response.body).to eq 'Snippet git access is disabled.'
-      end
-    end
-
-    describe 'POST #git_upload_pack' do
-      before do
-        allow(controller).to receive(:authenticate_user).and_return(true)
-        allow(controller).to receive(:verify_workhorse_api!).and_return(true)
-        allow(controller).to receive(:access_check).and_return(nil)
-      end
-
-      it 'returns 403' do
-        expect(controller).not_to receive(:access_check)
-
-        post :git_upload_pack, params: params
-
-        expect(response).to have_gitlab_http_status(:forbidden)
-        expect(response.body).to eq 'Snippet git access is disabled.'
-      end
-    end
-  end
-
   context 'when repository container is a project' do
     it_behaves_like 'info_refs behavior' do
       let(:user) { project.owner }
@@ -226,9 +188,6 @@ describe Repositories::GitHttpController do
       let(:expected_class) { Gitlab::GitAccessSnippet }
       let(:expected_object) { personal_snippet }
     end
-    it_behaves_like 'snippet feature flag disabled behavior' do
-      let(:user) { personal_snippet.author }
-    end
   end
 
   context 'when repository container is a project snippet' do
@@ -243,8 +202,5 @@ describe Repositories::GitHttpController do
       let(:expected_class) { Gitlab::GitAccessSnippet }
       let(:expected_object) { project_snippet }
     end
-    it_behaves_like 'snippet feature flag disabled behavior' do
-      let(:user) { project_snippet.author }
-    end
   end
 end

spec/controllers/snippets_controller_spec.rb

@@ -86,20 +86,6 @@ describe SnippetsController do
 
         expect(assigns(:blob)).to eq(personal_snippet.blobs.first)
       end
-
-      context 'when feature flag version_snippets is disabled' do
-        before do
-          stub_feature_flags(version_snippets: false)
-        end
-
-        it 'returns the snippet database content' do
-          subject
-
-          blob = assigns(:blob)
-
-          expect(blob.data).to eq(personal_snippet.content)
-        end
-      end
     end
 
     context 'when the personal snippet is private' do
@@ -572,24 +558,6 @@ describe SnippetsController do
         expect(response.cache_control[:public]).to eq snippet.public?
       end
 
-      context 'when feature flag version_snippets is disabled' do
-        before do
-          stub_feature_flags(version_snippets: false)
-        end
-
-        it_behaves_like '200 status'
-        it_behaves_like 'CRLF line ending'
-
-        it 'returns snippet database content' do
-          subject
-
-          expect(response.body).to eq snippet.content
-          expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
-        end
-
-        it_behaves_like 'content disposition headers'
-      end
-
       context 'when snippet repository is empty' do
         before do
           allow_any_instance_of(Repository).to receive(:empty?).and_return(true)

spec/features/projects/snippets/user_updates_snippet_spec.rb

@@ -7,12 +7,9 @@ describe 'Projects > Snippets > User updates a snippet', :js do
   let_it_be(:project) { create(:project, namespace: user.namespace) }
   let_it_be(:snippet, reload: true) { create(:project_snippet, :repository, project: project, author: user) }
 
-  let(:version_snippet_enabled) { true }
-
   before do
     stub_feature_flags(snippets_vue: false)
     stub_feature_flags(snippets_edit_vue: false)
-    stub_feature_flags(version_snippets: version_snippet_enabled)
 
     project.add_maintainer(user)
     sign_in(user)
@@ -35,18 +32,6 @@ describe 'Projects > Snippets > User updates a snippet', :js do
     end
   end
 
-  context 'when feature flag :version_snippets is disabled' do
-    let(:version_snippet_enabled) { false }
-
-    it 'displays the snippet file_name and content' do
-      aggregate_failures do
-        expect(page.find_field('project_snippet_file_name').value).to eq snippet.file_name
-        expect(page.find('.file-content')).to have_content(snippet.content)
-        expect(page.find('.snippet-file-content', visible: false).value).to eq snippet.content
-      end
-    end
-  end
-
   it 'updates a snippet' do
     fill_in('project_snippet_title', with: 'Snippet new title')
     click_button('Save')

spec/features/snippets/user_edits_snippet_spec.rb

@@ -10,12 +10,9 @@ describe 'User edits snippet', :js do
   let_it_be(:user) { create(:user) }
   let_it_be(:snippet, reload: true) { create(:personal_snippet, :repository, :public, file_name: file_name, content: content, author: user) }
 
-  let(:version_snippet_enabled) { true }
-
   before do
     stub_feature_flags(snippets_vue: false)
     stub_feature_flags(snippets_edit_vue: false)
-    stub_feature_flags(version_snippets: version_snippet_enabled)
 
     sign_in(user)
 
@@ -33,18 +30,6 @@ describe 'User edits snippet', :js do
     end
   end
 
-  context 'when feature flag :version_snippets is disabled' do
-    let(:version_snippet_enabled) { false }
-
-    it 'displays the snippet file_name and content' do
-      aggregate_failures do
-        expect(page.find_field('personal_snippet_file_name').value).to eq file_name
-        expect(page.find('.file-content')).to have_content(content)
-        expect(page.find('.snippet-file-content', visible: false).value).to eq content
-      end
-    end
-  end
-
   it 'updates the snippet' do
     fill_in 'personal_snippet_title', with: 'New Snippet Title'
 

spec/graphql/types/snippet_type_spec.rb

@@ -35,14 +35,6 @@ describe GitlabSchema.types['Snippet'] do
         expect(response['sshUrlToRepo']).to eq(snippet.ssh_url_to_repo)
         expect(response['httpUrlToRepo']).to eq(snippet.http_url_to_repo)
       end
-
-      context 'when version_snippets feature is disabled' do
-        before do
-          stub_feature_flags(version_snippets: false)
-        end
-
-        it_behaves_like 'response without repository URLs'
-      end
     end
   end
 

spec/lib/api/entities/snippet_spec.rb

@@ -26,14 +26,6 @@ describe ::API::Entities::Snippet do
         expect(subject[:file_name]).to eq snippet.blobs.first.path
       end
 
-      context 'when feature flag :version_snippets is disabled' do
-        it 'returns attribute from db' do
-          stub_feature_flags(version_snippets: false)
-
-          expect(subject[:file_name]).to eq snippet.file_name
-        end
-      end
-
       context 'when repository is empty' do
         it 'returns attribute from db' do
           allow(snippet.repository).to receive(:empty?).and_return(true)
@@ -48,14 +40,6 @@ describe ::API::Entities::Snippet do
         expect(subject[:ssh_url_to_repo]).to eq snippet.ssh_url_to_repo
       end
 
-      context 'when feature flag :version_snippets is disabled' do
-        it 'does not include attribute' do
-          stub_feature_flags(version_snippets: false)
-
-          expect(subject).not_to include(:ssh_url_to_repo)
-        end
-      end
-
       context 'when repository does not exist' do
         it 'does not include attribute' do
           allow(snippet).to receive(:repository_exists?).and_return(false)
@@ -70,14 +54,6 @@ describe ::API::Entities::Snippet do
         expect(subject[:http_url_to_repo]).to eq snippet.http_url_to_repo
       end
 
-      context 'when feature flag :version_snippets is disabled' do
-        it 'does not include attribute' do
-          stub_feature_flags(version_snippets: false)
-
-          expect(subject).not_to include(:http_url_to_repo)
-        end
-      end
-
       context 'when repository does not exist' do
         it 'does not include attribute' do
           allow(snippet).to receive(:repository_exists?).and_return(false)

spec/lib/gitlab/git_access_snippet_spec.rb

@@ -31,19 +31,6 @@ describe Gitlab::GitAccessSnippet do
     end
   end
 
-  describe 'when feature flag :version_snippets is disabled' do
-    let(:user) { snippet.author }
-
-    before do
-      stub_feature_flags(version_snippets: false)
-    end
-
-    it 'allows push and pull access' do
-      expect { pull_access_check }.not_to raise_error
-      expect { push_access_check }.not_to raise_error
-    end
-  end
-
   describe '#check_snippet_accessibility!' do
     context 'when the snippet exists' do
       it 'allows access' do

spec/models/snippet_spec.rb

@@ -735,31 +735,19 @@ describe Snippet do
     end
   end
 
-  describe '#versioned_enabled_for?' do
-    let_it_be(:user) { create(:user) }
+  describe '#url_to_repo' do
+    subject { snippet.url_to_repo }
 
-    subject { snippet.versioned_enabled_for?(user) }
+    context 'with personal snippet' do
+      let(:snippet) { create(:personal_snippet) }
 
-    context 'with repository and version_snippets enabled' do
-      let!(:snippet) { create(:personal_snippet, :repository, author: user) }
-
-      it { is_expected.to be_truthy }
-    end
-
-    context 'without repository' do
-      let!(:snippet) { create(:personal_snippet, author: user) }
-
-      it { is_expected.to be_falsy }
+      it { is_expected.to eq(Gitlab.config.gitlab_shell.ssh_path_prefix + "snippets/#{snippet.id}.git") }
     end
 
-    context 'without version_snippets feature disabled' do
-      let!(:snippet) { create(:personal_snippet, :repository, author: user) }
-
-      before do
-        stub_feature_flags(version_snippets: false)
-      end
+    context 'with project snippet' do
+      let(:snippet) { create(:project_snippet) }
 
-      it { is_expected.to be_falsy }
+      it { is_expected.to eq(Gitlab.config.gitlab_shell.ssh_path_prefix + "#{snippet.project.full_path}/snippets/#{snippet.id}.git") }
     end
   end
 end

spec/requests/api/internal/base_spec.rb

@@ -323,18 +323,6 @@ describe API::Internal::Base do
         end
       end
 
-      shared_examples 'snippets with disabled feature flag' do
-        context 'when feature flag :version_snippets is disabled' do
-          it 'returns 401' do
-            stub_feature_flags(version_snippets: false)
-
-            subject
-
-            expect(response).to have_gitlab_http_status(:unauthorized)
-          end
-        end
-      end
-
       shared_examples 'snippet success' do
         it 'responds with success' do
           subject
@@ -344,18 +332,6 @@ describe API::Internal::Base do
         end
       end
 
-      shared_examples 'snippets with web protocol' do
-        it_behaves_like 'snippet success'
-
-        context 'with disabled version flag' do
-          before do
-            stub_feature_flags(version_snippets: false)
-          end
-
-          it_behaves_like 'snippet success'
-        end
-      end
-
       context 'git push with personal snippet' do
         subject { push(key, personal_snippet, env: env.to_json, changes: snippet_changes) }
 
@@ -369,12 +345,6 @@ describe API::Internal::Base do
           expect(user.reload.last_activity_on).to be_nil
         end
 
-        it_behaves_like 'snippets with disabled feature flag'
-
-        it_behaves_like 'snippets with web protocol' do
-          subject { push(key, personal_snippet, 'web', env: env.to_json, changes: snippet_changes) }
-        end
-
         it_behaves_like 'sets hook env' do
           let(:gl_repository) { Gitlab::GlRepository::SNIPPET.identifier_for_container(personal_snippet) }
         end
@@ -392,12 +362,6 @@ describe API::Internal::Base do
           expect(json_response["gl_repository"]).to eq("snippet-#{personal_snippet.id}")
           expect(user.reload.last_activity_on).to eql(Date.today)
         end
-
-        it_behaves_like 'snippets with disabled feature flag'
-
-        it_behaves_like 'snippets with web protocol' do
-          subject { pull(key, personal_snippet, 'web') }
-        end
       end
 
       context 'git push with project snippet' do
@@ -413,12 +377,6 @@ describe API::Internal::Base do
           expect(user.reload.last_activity_on).to be_nil
         end
 
-        it_behaves_like 'snippets with disabled feature flag'
-
-        it_behaves_like 'snippets with web protocol' do
-          subject { push(key, project_snippet, 'web', env: env.to_json, changes: snippet_changes) }
-        end
-
         it_behaves_like 'sets hook env' do
           let(:gl_repository) { Gitlab::GlRepository::SNIPPET.identifier_for_container(project_snippet) }
         end
@@ -434,14 +392,6 @@ describe API::Internal::Base do
           expect(json_response["gl_repository"]).to eq("snippet-#{project_snippet.id}")
           expect(user.reload.last_activity_on).to eql(Date.today)
         end
-
-        it_behaves_like 'snippets with disabled feature flag' do
-          subject { pull(key, project_snippet) }
-        end
-
-        it_behaves_like 'snippets with web protocol' do
-          subject { pull(key, project_snippet, 'web') }
-        end
       end
 
       context "git pull" do

spec/requests/api/project_snippets_spec.rb

@@ -99,16 +99,6 @@ describe API::ProjectSnippets do
       expect(json_response['http_url_to_repo']).to eq(snippet.http_url_to_repo)
     end
 
-    context 'when feature flag :version_snippets is disabled' do
-      before do
-        stub_feature_flags(version_snippets: false)
-
-        get api("/projects/#{project.id}/snippets/#{snippet.id}", user)
-      end
-
-      it_behaves_like 'snippet response without repository URLs'
-    end
-
     it 'returns 404 for invalid snippet id' do
       get api("/projects/#{project.id}/snippets/#{non_existing_record_id}", user)
 
@@ -150,18 +140,6 @@ describe API::ProjectSnippets do
 
         expect(blob.data).to eq params[:code]
       end
-
-      context 'when feature flag :version_snippets is disabled' do
-        it 'does not create snippet repository' do
-          stub_feature_flags(version_snippets: false)
-
-          expect do
-            subject
-          end.to change { ProjectSnippet.count }.by(1)
-
-          expect(snippet.repository_exists?).to be_falsey
-        end
-      end
     end
 
     context 'with an external user' do

spec/requests/api/snippets_spec.rb

@@ -166,16 +166,6 @@ describe API::Snippets do
       expect(json_response['http_url_to_repo']).to eq(private_snippet.http_url_to_repo)
     end
 
-    context 'when feature flag :version_snippets is disabled' do
-      before do
-        stub_feature_flags(version_snippets: false)
-
-        get api("/snippets/#{private_snippet.id}", author)
-      end
-
-      it_behaves_like 'snippet response without repository URLs'
-    end
-
     it 'shows private snippets to an admin' do
       get api("/snippets/#{private_snippet.id}", admin)
 
@@ -245,18 +235,6 @@ describe API::Snippets do
 
         expect(blob.data).to eq params[:content]
       end
-
-      context 'when feature flag :version_snippets is disabled' do
-        it 'does not create snippet repository' do
-          stub_feature_flags(version_snippets: false)
-
-          expect do
-            subject
-          end.to change { PersonalSnippet.count }.by(1)
-
-          expect(snippet.repository_exists?).to be_falsey
-        end
-      end
     end
 
     context 'with restricted visibility settings' do

spec/services/snippets/create_service_spec.rb

@@ -228,28 +228,6 @@ describe Snippets::CreateService do
           expect(snippet.repository_exists?).to be_falsey
         end
       end
-
-      context 'when feature flag :version_snippets is disabled' do
-        before do
-          stub_feature_flags(version_snippets: false)
-        end
-
-        it 'does not create snippet repository' do
-          expect do
-            subject
-          end.to change(Snippet, :count).by(1)
-
-          expect(snippet.repository_exists?).to be_falsey
-        end
-
-        it 'does not try to commit files' do
-          expect_next_instance_of(described_class) do |instance|
-            expect(instance).not_to receive(:create_commit)
-          end
-
-          subject
-        end
-      end
     end
 
     shared_examples 'after_save callback to store_mentions' do |mentionable_class|

spec/services/snippets/update_service_spec.rb

@@ -112,25 +112,16 @@ describe Snippets::UpdateService do
         expect(blob.data).to eq options[:content]
       end
 
-      context 'when the repository does not exist' do
-        it 'does not try to commit file' do
-          allow(snippet).to receive(:repository_exists?).and_return(false)
-
-          expect(service).not_to receive(:create_commit)
-
-          subject
-        end
-      end
-
-      context 'when feature flag is disabled' do
+      context 'when the repository creation fails' do
         before do
-          stub_feature_flags(version_snippets: false)
+          allow(snippet).to receive(:repository_exists?).and_return(false)
         end
 
-        it 'does not create repository' do
-          subject
+        it 'raise an error' do
+          response = subject
 
-          expect(snippet.repository).not_to exist
+          expect(response).to be_error
+          expect(response.payload[:snippet].errors[:repository].to_sentence).to eq 'Error updating the snippet'
         end
 
         it 'does not try to commit file' do

spec/support/shared_examples/requests/snippet_shared_examples.rb

@@ -66,16 +66,6 @@ RSpec.shared_examples 'snippet blob content' do
     expect(response.body).to eq(snippet.blobs.first.data)
   end
 
-  context 'when feature flag :version_snippets is disabled' do
-    it 'returns content from database' do
-      stub_feature_flags(version_snippets: false)
-
-      subject
-
-      expect(response.body).to eq(snippet.content)
-    end
-  end
-
   context 'when snippet repository is empty' do
     let(:snippet) { snippet_with_empty_repo }