Merge branch 'visual-review-api' into 'master'

Visual Review API

See merge request gitlab-org/gitlab!18710

changelogs/unreleased/visual-review-api.yml

+---
+title: New API endpoint for creating anonymous merge request discussions from Visual Review Tools
+merge_request: 18710
+author:
+type: added

doc/api/api_resources.md

@@ -24,7 +24,7 @@ The following API resources are available in the project context:
 | [Commits](commits.md)                                               | `/projects/:id/repository/commits`, `/projects/:id/statuses`                                                                                                                                          |
 | [Container Registry](container_registry.md)                         | `/projects/:id/registry/repositories`                                                                                                                                                                 |
 | [Custom attributes](custom_attributes.md)                           | `/projects/:id/custom_attributes` (also available for groups and users)                                                                                                                               |
-| [Dependencies](dependencies.md) **(ULTIMATE)**                      | `/projects/:id/dependencies`
+| [Dependencies](dependencies.md) **(ULTIMATE)**                      | `/projects/:id/dependencies`                                                                                                                                                                          |
 | [Deploy keys](deploy_keys.md)                                       | `/projects/:id/deploy_keys` (also available standalone)                                                                                                                                               |
 | [Deployments](deployments.md)                                       | `/projects/:id/deployments`                                                                                                                                                                           |
 | [Discussions](discussions.md) (threaded comments)                   | `/projects/:id/issues/.../discussions`, `/projects/:id/snippets/.../discussions`, `/projects/:id/merge_requests/.../discussions`, `/projects/:id/commits/.../discussions` (also available for groups) |
@@ -67,7 +67,8 @@ The following API resources are available in the project context:
 | [Search](search.md)                                                 | `/projects/:id/search` (also available for groups and standalone)                                                                                                                                     |
 | [Services](services.md)                                             | `/projects/:id/services`                                                                                                                                                                              |
 | [Tags](tags.md)                                                     | `/projects/:id/repository/tags`                                                                                                                                                                       |
-| [Vulnerabilities](vulnerabilities.md) **(ULTIMATE)**                | `/projects/:id/vulnerabilities`
+| [Vulnerabilities](vulnerabilities.md) **(ULTIMATE)**                | `/projects/:id/vulnerabilities`                                                                                                                                                                       |
+| [Visual Review discussions](visual_review_discussions.md) **(STARTER**) | `/projects/:id/merge_requests/:merge_request_id/visual_review_discussions`                                                                                                                            |
 | [Wikis](wikis.md)                                                   | `/projects/:id/wikis`                                                                                                                                                                                 |
 
 ## Group resources

doc/api/visual_review_discussions.md

+# Visual Review discussions API **(STARTER**)
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/merge_requests/18710) in [GitLab Starter](https://about.gitlab.com/pricing/) 12.5.
+
+Visual Review discussions are notes on Merge Requests sent as
+feedback from [Visual Reviews](../ci/review_apps/index.md#visual-reviews-starter).
+
+## Create new merge request thread
+
+Creates a new thread to a single project merge request. This is similar to creating
+a note but other comments (replies) can be added to it later.
+
+```
+POST /projects/:id/merge_requests/:merge_request_iid/visual_review_discussions
+```
+
+Parameters:
+
+| Attribute                 | Type           | Required | Description |
+| ------------------------- | -------------- | -------- | ----------- |
+| `id`                      | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
+| `merge_request_iid`       | integer        | yes      | The IID of a merge request |
+| `body`                    | string         | yes      | The content of the thread |
+| `position`                | hash           | no       | Position when creating a diff note |
+| `position[base_sha]`      | string         | yes      | Base commit SHA in the source branch |
+| `position[start_sha]`     | string         | yes      | SHA referencing commit in target branch |
+| `position[head_sha]`      | string         | yes      | SHA referencing HEAD of this merge request |
+| `position[position_type]` | string         | yes      | Type of the position reference. Either `text` or `image`. |
+| `position[new_path]`      | string         | no       | File path after change |
+| `position[new_line]`      | integer        | no       | Line number after change (Only stored for `text` diff notes) |
+| `position[old_path]`      | string         | no       | File path before change |
+| `position[old_line]`      | integer        | no       | Line number before change (Only stored for `text` diff notes) |
+| `position[width]`         | integer        | no       | Width of the image (Only stored for `image` diff notes) |
+| `position[height]`        | integer        | no       | Height of the image (Only stored for `image` diff notes) |
+| `position[x]`             | integer        | no       | X coordinate (Only stored for `image` diff notes) |
+| `position[y]`             | integer        | no       | Y coordinate (Only stored for `image` diff notes) |
+
+```bash
+curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/visual_review_discussions?body=comment
+```

ee/lib/api/visual_review_discussions.rb

+# frozen_string_literal: true
+
+module API
+  class VisualReviewDiscussions < Grape::API
+    include PaginationParams
+    helpers ::API::Helpers::NotesHelpers
+    helpers ::RendersNotes
+
+    params do
+      requires :id, type: String, desc: "The ID of a Project"
+    end
+    resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+      desc 'Create a new merge request discussion from visual review without authentication' do
+        success Entities::Discussion
+      end
+      params do
+        requires :merge_request_iid, types: [Integer, String], desc: 'The IID of the noteable'
+        requires :body, type: String, desc: 'The content of a note'
+        optional :position, type: Hash do
+          requires :base_sha, type: String, desc: 'Base commit SHA in the source branch'
+          requires :start_sha, type: String, desc: 'SHA referencing commit in target branch'
+          requires :head_sha, type: String, desc: 'SHA referencing HEAD of this merge request'
+          requires :position_type, type: String, desc: 'Type of the position reference', values: %w(text image)
+          optional :new_path, type: String, desc: 'File path after change'
+          optional :new_line, type: Integer, desc: 'Line number after change'
+          optional :old_path, type: String, desc: 'File path before change'
+          optional :old_line, type: Integer, desc: 'Line number before change'
+          optional :width, type: Integer, desc: 'Width of the image'
+          optional :height, type: Integer, desc: 'Height of the image'
+          optional :x, type: Integer, desc: 'X coordinate in the image'
+          optional :y, type: Integer, desc: 'Y coordinate in the image'
+        end
+      end
+      post ":id/merge_requests/:merge_request_iid/visual_review_discussions" do
+        unless Feature.enabled?(:anonymous_visual_review_feedback)
+          forbidden!('Anonymous visual review feedback is disabled')
+        end
+
+        merge_request = find_merge_request_without_permissions_check(params[:id], params[:merge_request_iid])
+
+        note = create_visual_review_note(merge_request, {
+          note: params[:body],
+          type: 'DiscussionNote',
+          noteable_type: 'MergeRequest',
+          position: params[:position],
+          noteable_id: merge_request.id
+        })
+
+        if note.valid?
+          present note.discussion, with: Entities::Discussion
+        else
+          bad_request!("Note #{note.errors.messages}")
+        end
+      end
+    end
+  end
+end

ee/lib/ee/api/api.rb

@@ -42,6 +42,7 @@ module EE
         mount ::API::MergeRequestApprovalRules
         mount ::API::ProjectAliases
         mount ::API::Dependencies
+        mount ::API::VisualReviewDiscussions
 
         version 'v3', using: :path do
           # Although the following endpoints are kept behind V3 namespace,

ee/lib/ee/api/helpers/notes_helpers.rb

@@ -22,6 +22,24 @@ module EE
             super
           end
         end
+
+        # Used only for anonymous Visual Review Tools feedback
+        def find_merge_request_without_permissions_check(parent_id, noteable_id)
+          params = finder_params_by_noteable_type_and_id(::MergeRequest, noteable_id, parent_id)
+
+          ::NotesFinder.new(current_user, params).target || not_found!(noteable_type)
+        end
+
+        def create_visual_review_note(noteable, opts)
+          unless ::Feature.enabled?(:anonymous_visual_review_feedback)
+            forbidden!('Anonymous visual review feedback is disabled')
+          end
+
+          parent  = noteable_parent(noteable)
+          project = parent if parent.is_a?(Project)
+
+          ::Notes::CreateService.new(project, ::User.visual_review_bot, opts).execute
+        end
       end
     end
   end

ee/spec/requests/api/visual_review_discussions_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::VisualReviewDiscussions do
+  let(:user)     { create(:user) }
+  let!(:project) { create(:project, :public, :repository, namespace: user.namespace) }
+
+  context 'when sending merge request feedback from a visual review app without authentication' do
+    let!(:merge_request) do
+      create(:merge_request_with_diffs, source_project: project, target_project: project, author: user)
+    end
+
+    let(:request) do
+      post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/visual_review_discussions"), params: note_params
+    end
+
+    let(:note_params)   { { body: 'hi!' } }
+    let(:response_note) { json_response['notes'].first }
+
+    it 'creates a new note' do
+      expect { request }.to change(merge_request.notes, :count).by(1)
+    end
+
+    describe 'the API response' do
+      before do
+        request
+      end
+
+      it 'responds with a status 201 Created' do
+        expect(response).to have_gitlab_http_status(201)
+      end
+
+      it 'returns the persisted note body' do
+        expect(response_note['body']).to eq('hi!')
+      end
+
+      it 'returns the name of the Visual Review Bot assigned as the author' do
+        expect(response_note['author']['username']).to eq(User.visual_review_bot.username)
+      end
+
+      it 'returns the id of the merge request as the parent noteable_id' do
+        expect(response_note['noteable_id']).to eq(merge_request.id)
+      end
+    end
+
+    context 'with no message body' do
+      let(:note_params) { { some: 'thing' } }
+
+      it 'returns a 400 bad request error if body not given' do
+        expect { request }.not_to change(merge_request.notes, :count)
+
+        expect(response).to have_gitlab_http_status(400)
+      end
+    end
+
+    context 'with an invalid merge request IID' do
+      let(:merge_request) { double(iid: 546574823564) }
+
+      it 'creates a new note' do
+        expect { request }.not_to change(Note, :count)
+      end
+
+      describe 'the API response' do
+        it 'responds with a status 404' do
+          request
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+    end
+
+    context 'when anonymous_visual_review_feedback feature flag is disabled' do
+      before do
+        stub_feature_flags(anonymous_visual_review_feedback: false)
+      end
+
+      it 'does not create a new note' do
+        expect { request }.not_to change(merge_request.notes, :count)
+      end
+
+      describe 'the API response' do
+        before do
+          request
+        end
+
+        it 'responds 403' do
+          expect(response).to have_gitlab_http_status(403)
+        end
+
+        it 'returns error messaging specifying that the feature is disabled' do
+          expect(json_response['message']).to eq('403 Forbidden  - Anonymous visual review feedback is disabled')
+        end
+      end
+    end
+
+    context 'when an admin or owner makes an authenticated request' do
+      let(:request) do
+        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/visual_review_discussions", project.owner), params: note_params
+      end
+
+      let(:note_params) { { body: 'hi!', created_at: 2.weeks.ago } }
+
+      it 'creates a new note' do
+        expect { request }.to change(merge_request.notes, :count).by(1)
+      end
+
+      describe 'the API response' do
+        before do
+          request
+        end
+
+        it 'responds with a status 201 Created' do
+          expect(response).to have_gitlab_http_status(201)
+        end
+
+        it 'returns the persisted note body' do
+          expect(response_note['body']).to eq('hi!')
+        end
+
+        it 'returns the name of the Visual Review Bot assigned as the author' do
+          expect(response_note['author']['username']).to eq(User.visual_review_bot.username)
+        end
+
+        it 'returns the id of the merge request as the parent noteable_id' do
+          expect(response_note['noteable_id']).to eq(merge_request.id)
+        end
+
+        it 'returns a current time stamp instead of the provided one' do
+          expect(Time.parse(response_note['created_at']) > 1.day.ago).to eq(true)
+        end
+      end
+    end
+  end
+end