Merge branch 'vij-project-snippet-rest-update' into 'master'

ProjectSnippets update with files

See merge request gitlab-org/gitlab!41107

lib/api/project_snippets.rb

@@ -84,14 +84,17 @@ module API
       end
       params do
         requires :snippet_id, type: Integer, desc: 'The ID of a project snippet'
-        optional :title, type: String, allow_blank: false, desc: 'The title of the snippet'
-        optional :file_name, type: String, desc: 'The file name of the snippet'
         optional :content, type: String, allow_blank: false, desc: 'The content of the snippet'
         optional :description, type: String, desc: 'The description of a snippet'
+        optional :file_name, type: String, desc: 'The file name of the snippet'
+        optional :title, type: String, allow_blank: false, desc: 'The title of the snippet'
         optional :visibility, type: String,
                               values: Gitlab::VisibilityLevel.string_values,
                               desc: 'The visibility of the snippet'
-        at_least_one_of :title, :file_name, :content, :visibility
+
+        use :update_file_params
+
+        at_least_one_of :title, :file_name, :content, :files, :visibility
       end
       # rubocop: disable CodeReuse/ActiveRecord
       put ":id/snippets/:snippet_id" do
@@ -100,8 +103,9 @@ module API
 
         authorize! :update_snippet, snippet
 
-        snippet_params = declared_params(include_missing: false)
-          .merge(request: request, api: true)
+        validate_params_for_multiple_files(snippet)
+
+        snippet_params = process_update_params(declared_params(include_missing: false))
 
         service_response = ::Snippets::UpdateService.new(user_project, current_user, snippet_params).execute(snippet)
         snippet = service_response.payload[:snippet]

spec/requests/api/project_snippets_spec.rb

@@ -304,30 +304,9 @@ RSpec.describe API::ProjectSnippets do
     let(:visibility_level) { Snippet::PUBLIC }
     let(:snippet) { create(:project_snippet, :repository, author: admin, visibility_level: visibility_level, project: project) }
 
-    it 'updates snippet' do
-      new_content = 'New content'
-      new_description = 'New description'
-
-      update_snippet(params: { content: new_content, description: new_description, visibility: 'private' })
-
-      expect(response).to have_gitlab_http_status(:ok)
-      snippet.reload
-      expect(snippet.content).to eq(new_content)
-      expect(snippet.description).to eq(new_description)
-      expect(snippet.visibility).to eq('private')
-    end
-
-    it 'updates snippet with content parameter' do
-      new_content = 'New content'
-      new_description = 'New description'
-
-      update_snippet(params: { content: new_content, description: new_description })
-
-      expect(response).to have_gitlab_http_status(:ok)
-      snippet.reload
-      expect(snippet.content).to eq(new_content)
-      expect(snippet.description).to eq(new_description)
-    end
+    it_behaves_like 'snippet file updates'
+    it_behaves_like 'snippet non-file updates'
+    it_behaves_like 'invalid snippet updates'
 
     it 'updates snippet with visibility parameter' do
       expect { update_snippet(params: { visibility: 'private' }) }
@@ -336,33 +315,6 @@ RSpec.describe API::ProjectSnippets do
       expect(snippet.visibility).to eq('private')
     end
 
-    it 'returns 404 for invalid snippet id' do
-      update_snippet(snippet_id: non_existing_record_id, params: { title: 'foo' })
-
-      expect(response).to have_gitlab_http_status(:not_found)
-      expect(json_response['message']).to eq('404 Snippet Not Found')
-    end
-
-    it 'returns 400 for missing parameters' do
-      update_snippet
-
-      expect(response).to have_gitlab_http_status(:bad_request)
-      expect(json_response['error']).to eq 'title, file_name, content, visibility are missing, at least one parameter must be provided'
-    end
-
-    it 'returns 400 if content is blank' do
-      update_snippet(params: { content: '' })
-
-      expect(response).to have_gitlab_http_status(:bad_request)
-    end
-
-    it 'returns 400 if title is blank' do
-      update_snippet(params: { title: '' })
-
-      expect(response).to have_gitlab_http_status(:bad_request)
-      expect(json_response['error']).to eq 'title is empty'
-    end
-
     it_behaves_like 'update with repository actions' do
       let(:snippet_without_repo) { create(:project_snippet, author: admin, project: project, visibility_level: visibility_level) }
     end

spec/requests/api/snippets_spec.rb

@@ -368,7 +368,7 @@ RSpec.describe API::Snippets do
       context 'when the snippet is public' do
         let(:extra_params) { { visibility: 'public' } }
 
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { subject }.not_to change { Snippet.count }
 
           expect(response).to have_gitlab_http_status(:bad_request)
@@ -391,98 +391,17 @@ RSpec.describe API::Snippets do
       create(:personal_snippet, :repository, author: user, visibility_level: visibility_level)
     end
 
-    let(:create_action)     { { action: 'create', file_path: 'foo.txt', content: 'bar' } }
-    let(:update_action)     { { action: 'update', file_path: 'CHANGELOG', content: 'bar' } }
-    let(:move_action)       { { action: 'move',   file_path: '.old-gitattributes', previous_path: '.gitattributes' } }
-    let(:delete_action)     { { action: 'delete', file_path: 'CONTRIBUTING.md' } }
-    let(:bad_file_path)     { { action: 'create', file_path: '../../etc/passwd', content: 'bar' } }
-    let(:bad_previous_path) { { action: 'create', previous_path: '../../etc/passwd', file_path: 'CHANGELOG', content: 'bar' } }
-    let(:invalid_move)      { { action: 'move',   file_path: 'missing_previous_path.txt' } }
-
-    context 'with snippet file changes' do
-      using RSpec::Parameterized::TableSyntax
-
-      where(:is_multi_file, :file_name, :content, :files, :status) do
-        true  | nil       | nil   | [create_action]                | :success
-        true  | nil       | nil   | [update_action]                | :success
-        true  | nil       | nil   | [move_action]                  | :success
-        true  | nil       | nil   | [delete_action]                | :success
-        true  | nil       | nil   | [create_action, update_action] | :success
-        true  | 'foo.txt' | 'bar' | [create_action]                | :bad_request
-        true  | 'foo.txt' | 'bar' | nil                            | :bad_request
-        true  | nil       | nil   | nil                            | :bad_request
-        true  | 'foo.txt' | nil   | [create_action]                | :bad_request
-        true  | nil       | 'bar' | [create_action]                | :bad_request
-        true  | ''        | nil   | [create_action]                | :bad_request
-        true  | nil       | ''    | [create_action]                | :bad_request
-        true  | nil       | nil   | [bad_file_path]                | :bad_request
-        true  | nil       | nil   | [bad_previous_path]            | :bad_request
-        true  | nil       | nil   | [invalid_move]                 | :unprocessable_entity
-
-        false | 'foo.txt' | 'bar' | nil                            | :success
-        false | 'foo.txt' | nil   | nil                            | :success
-        false | nil       | 'bar' | nil                            | :success
-        false | 'foo.txt' | 'bar' | [create_action]                | :bad_request
-        false | nil       | nil   | nil                            | :bad_request
-        false | nil       | ''    | nil                            | :bad_request
-        false | nil       | nil   | [bad_file_path]                | :bad_request
-        false | nil       | nil   | [bad_previous_path]            | :bad_request
-      end
-
-      with_them do
-        before do
-          allow_any_instance_of(Snippet).to receive(:multiple_files?).and_return(is_multi_file)
-        end
-
-        it 'has the correct response' do
-          update_params = {}.tap do |params|
-            params[:files]     = files     if files
-            params[:file_name] = file_name if file_name
-            params[:content]   = content   if content
-          end
-
-          update_snippet(params: update_params)
-
-          expect(response).to have_gitlab_http_status(status)
-        end
-      end
-
-      context 'when save fails due to a repository commit error' do
-        before do
-          allow_next_instance_of(Repository) do |instance|
-            allow(instance).to receive(:multi_action).and_raise(Gitlab::Git::CommitError)
-          end
-
-          update_snippet(params: { files: [create_action] })
-        end
-
-        it 'returns a bad request response' do
-          expect(response).to have_gitlab_http_status(:bad_request)
-        end
-      end
-    end
-
-    shared_examples 'snippet non-file updates' do
-      it 'updates a snippet non-file attributes' do
-        new_description = 'New description'
-        new_title       = 'New title'
-        new_visibility  = 'internal'
-
-        update_snippet(params: { title: new_title, description: new_description, visibility: new_visibility })
+    it_behaves_like 'snippet file updates'
+    it_behaves_like 'snippet non-file updates'
+    it_behaves_like 'invalid snippet updates'
 
-        snippet.reload
+    it "returns 404 for another user's snippet" do
+      update_snippet(requester: other_user, params: { title: 'foobar' })
 
-        aggregate_failures do
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(snippet.description).to eq(new_description)
-          expect(snippet.visibility).to eq(new_visibility)
-          expect(snippet.title).to eq(new_title)
-        end
-      end
+      expect(response).to have_gitlab_http_status(:not_found)
+      expect(json_response['message']).to eq('404 Snippet Not Found')
     end
 
-    it_behaves_like 'snippet non-file updates'
-
     context 'with restricted visibility settings' do
       before do
         stub_application_setting(restricted_visibility_levels:
@@ -493,33 +412,6 @@ RSpec.describe API::Snippets do
       it_behaves_like 'snippet non-file updates'
     end
 
-    it 'returns 404 for invalid snippet id' do
-      update_snippet(snippet_id: non_existing_record_id, params: { title: 'Foo' })
-
-      expect(response).to have_gitlab_http_status(:not_found)
-      expect(json_response['message']).to eq('404 Snippet Not Found')
-    end
-
-    it "returns 404 for another user's snippet" do
-      update_snippet(requester: other_user, params: { title: 'foobar' })
-
-      expect(response).to have_gitlab_http_status(:not_found)
-      expect(json_response['message']).to eq('404 Snippet Not Found')
-    end
-
-    it 'returns 400 for missing parameters' do
-      update_snippet
-
-      expect(response).to have_gitlab_http_status(:bad_request)
-    end
-
-    it 'returns 400 if title is blank' do
-      update_snippet(params: { title: '' })
-
-      expect(response).to have_gitlab_http_status(:bad_request)
-      expect(json_response['error']).to eq 'title is empty'
-    end
-
     it_behaves_like 'update with repository actions' do
       let(:snippet_without_repo) { create(:personal_snippet, author: user, visibility_level: visibility_level) }
     end
@@ -543,7 +435,7 @@ RSpec.describe API::Snippets do
       context 'when the snippet is public' do
         let(:visibility_level) { Snippet::PUBLIC }
 
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { update_snippet(params: { title: 'Foo' }) }
             .not_to change { snippet.reload.title }
 

spec/support/shared_examples/requests/api/snippets_shared_examples.rb

@@ -77,3 +77,123 @@ RSpec.shared_examples 'raw snippet files' do
     end
   end
 end
+
+RSpec.shared_examples 'snippet file updates' do
+  let(:create_action)     { { action: 'create', file_path: 'foo.txt', content: 'bar' } }
+  let(:update_action)     { { action: 'update', file_path: 'CHANGELOG', content: 'bar' } }
+  let(:move_action)       { { action: 'move',   file_path: '.old-gitattributes', previous_path: '.gitattributes' } }
+  let(:delete_action)     { { action: 'delete', file_path: 'CONTRIBUTING.md' } }
+  let(:bad_file_path)     { { action: 'create', file_path: '../../etc/passwd', content: 'bar' } }
+  let(:bad_previous_path) { { action: 'create', previous_path: '../../etc/passwd', file_path: 'CHANGELOG', content: 'bar' } }
+  let(:invalid_move)      { { action: 'move',   file_path: 'missing_previous_path.txt' } }
+
+  context 'with various snippet file changes' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:is_multi_file, :file_name, :content, :files, :status) do
+      true  | nil       | nil   | [create_action]                | :success
+      true  | nil       | nil   | [update_action]                | :success
+      true  | nil       | nil   | [move_action]                  | :success
+      true  | nil       | nil   | [delete_action]                | :success
+      true  | nil       | nil   | [create_action, update_action] | :success
+      true  | 'foo.txt' | 'bar' | [create_action]                | :bad_request
+      true  | 'foo.txt' | 'bar' | nil                            | :bad_request
+      true  | nil       | nil   | nil                            | :bad_request
+      true  | 'foo.txt' | nil   | [create_action]                | :bad_request
+      true  | nil       | 'bar' | [create_action]                | :bad_request
+      true  | ''        | nil   | [create_action]                | :bad_request
+      true  | nil       | ''    | [create_action]                | :bad_request
+      true  | nil       | nil   | [bad_file_path]                | :bad_request
+      true  | nil       | nil   | [bad_previous_path]            | :bad_request
+      true  | nil       | nil   | [invalid_move]                 | :unprocessable_entity
+
+      false | 'foo.txt' | 'bar' | nil                            | :success
+      false | 'foo.txt' | nil   | nil                            | :success
+      false | nil       | 'bar' | nil                            | :success
+      false | 'foo.txt' | 'bar' | [create_action]                | :bad_request
+      false | nil       | nil   | nil                            | :bad_request
+      false | nil       | ''    | nil                            | :bad_request
+      false | nil       | nil   | [bad_file_path]                | :bad_request
+      false | nil       | nil   | [bad_previous_path]            | :bad_request
+    end
+
+    with_them do
+      before do
+        allow_any_instance_of(Snippet).to receive(:multiple_files?).and_return(is_multi_file)
+      end
+
+      it 'has the correct response' do
+        update_params = {}.tap do |params|
+          params[:files]     = files     if files
+          params[:file_name] = file_name if file_name
+          params[:content]   = content   if content
+        end
+
+        update_snippet(params: update_params)
+
+        expect(response).to have_gitlab_http_status(status)
+      end
+    end
+
+    context 'when save fails due to a repository commit error' do
+      before do
+        allow_next_instance_of(Repository) do |instance|
+          allow(instance).to receive(:multi_action).and_raise(Gitlab::Git::CommitError)
+        end
+
+        update_snippet(params: { files: [create_action] })
+      end
+
+      it 'returns a bad request response' do
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+    end
+  end
+end
+
+RSpec.shared_examples 'snippet non-file updates' do
+  it 'updates a snippet non-file attributes' do
+    new_description = 'New description'
+    new_title       = 'New title'
+    new_visibility  = 'internal'
+
+    update_snippet(params: { title: new_title, description: new_description, visibility: new_visibility })
+
+    snippet.reload
+
+    aggregate_failures do
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(snippet.description).to eq(new_description)
+      expect(snippet.visibility).to eq(new_visibility)
+      expect(snippet.title).to eq(new_title)
+    end
+  end
+end
+
+RSpec.shared_examples 'invalid snippet updates' do
+  it 'returns 404 for invalid snippet id' do
+    update_snippet(snippet_id: non_existing_record_id, params: { title: 'foo' })
+
+    expect(response).to have_gitlab_http_status(:not_found)
+    expect(json_response['message']).to eq('404 Snippet Not Found')
+  end
+
+  it 'returns 400 for missing parameters' do
+    update_snippet
+
+    expect(response).to have_gitlab_http_status(:bad_request)
+  end
+
+  it 'returns 400 if content is blank' do
+    update_snippet(params: { content: '' })
+
+    expect(response).to have_gitlab_http_status(:bad_request)
+  end
+
+  it 'returns 400 if title is blank' do
+    update_snippet(params: { title: '' })
+
+    expect(response).to have_gitlab_http_status(:bad_request)
+    expect(json_response['error']).to eq 'title is empty'
+  end
+end