Remove deprecated `upvotes` and `downvotes` from the notes API

changelogs/unreleased/api-notes-entity-fields.yml

+---
+title: 'API: Remove deprecated fields Notes#upvotes and Notes#downvotes'
+merge_request: 9384
+author: Robert Schilling

doc/api/notes.md

@@ -34,8 +34,6 @@ Parameters:
     "created_at": "2013-10-02T09:22:45Z",
     "updated_at": "2013-10-02T10:22:45Z",
     "system": true,
-    "upvote": false,
-    "downvote": false,
     "noteable_id": 377,
     "noteable_type": "Issue"
   },
@@ -54,8 +52,6 @@ Parameters:
     "created_at": "2013-10-02T09:56:03Z",
     "updated_at": "2013-10-02T09:56:03Z",
     "system": true,
-    "upvote": false,
-    "downvote": false,
     "noteable_id": 121,
     "noteable_type": "Issue"
   }
@@ -147,9 +143,7 @@ Example Response:
   "created_at": "2016-04-05T22:10:44.164Z",
   "system": false,
   "noteable_id": 11,
-  "noteable_type": "Issue",
-  "upvote": false,
-  "downvote": false
+  "noteable_type": "Issue"
 }
 ```
 
@@ -271,9 +265,7 @@ Example Response:
   "created_at": "2016-04-06T16:51:53.239Z",
   "system": false,
   "noteable_id": 52,
-  "noteable_type": "Snippet",
-  "upvote": false,
-  "downvote": false
+  "noteable_type": "Snippet"
 }
 ```
 
@@ -322,8 +314,6 @@ Parameters:
   "created_at": "2013-10-02T08:57:14Z",
   "updated_at": "2013-10-02T08:57:14Z",
   "system": false,
-  "upvote": false,
-  "downvote": false,
   "noteable_id": 2,
   "noteable_type": "MergeRequest"
 }
@@ -400,8 +390,6 @@ Example Response:
   "created_at": "2016-04-05T22:11:59.923Z",
   "system": false,
   "noteable_id": 7,
-  "noteable_type": "MergeRequest",
-  "upvote": false,
-  "downvote": false
+  "noteable_type": "MergeRequest"
 }
 ```

doc/api/projects.md

@@ -407,8 +407,6 @@ Parameters:
       },
       "created_at": "2015-12-04T10:33:56.698Z",
       "system": false,
-      "upvote": false,
-      "downvote": false,
       "noteable_id": 377,
       "noteable_type": "Issue"
     },

doc/api/users.md

@@ -812,8 +812,6 @@ Example response:
       },
       "created_at": "2015-12-04T10:33:56.698Z",
       "system": false,
-      "upvote": false,
-      "downvote": false,
       "noteable_id": 377,
       "noteable_type": "Issue"
     },

doc/api/v3_to_v4.md

@@ -36,4 +36,4 @@ changes are in V4:
   - POST `:id/repository/commits`
   - POST/PUT/DELETE `:id/repository/files`
 - Renamed `branch_name` to `branch` on DELETE `id/repository/branches/:branch` response
-
+- Notes do not return deprecated field `upvote` and `downvote` [!9384](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9384)

lib/api/api.rb

@@ -15,6 +15,7 @@ class API < Grape::API
       mount ::API::V3::Members
       mount ::API::V3::MergeRequestDiffs
       mount ::API::V3::MergeRequests
+      mount ::API::V3::Notes
       mount ::API::V3::ProjectHooks
       mount ::API::V3::Projects
       mount ::API::V3::ProjectSnippets

lib/api/entities.rb

@@ -339,9 +339,6 @@ class Note < Grape::Entity
       expose :created_at, :updated_at
       expose :system?, as: :system
       expose :noteable_id, :noteable_type
-      # upvote? and downvote? are deprecated, always return false
-      expose(:upvote?)    { |note| false }
-      expose(:downvote?)  { |note| false }
     end
 
     class AwardEmoji < Grape::Entity

lib/api/v3/entities.rb

@@ -11,6 +11,40 @@ class ProjectSnippet < Grape::Entity
           Gitlab::UrlBuilder.build(snippet)
         end
       end
+
+      class Note < Grape::Entity
+        expose :id
+        expose :note, as: :body
+        expose :attachment_identifier, as: :attachment
+        expose :author, using: ::API::Entities::UserBasic
+        expose :created_at, :updated_at
+        expose :system?, as: :system
+        expose :noteable_id, :noteable_type
+        # upvote? and downvote? are deprecated, always return false
+        expose(:upvote?)    { |note| false }
+        expose(:downvote?)  { |note| false }
+      end
+
+      class Event < Grape::Entity
+        expose :title, :project_id, :action_name
+        expose :target_id, :target_type, :author_id
+        expose :data, :target_title
+        expose :created_at
+        expose :note, using: Entities::Note, if: ->(event, options) { event.note? }
+        expose :author, using: ::API::Entities::UserBasic, if: ->(event, options) { event.author }
+
+        expose :author_username do |event, options|
+          event.author&.username
+        end
+      end
+
+      class AwardEmoji < Grape::Entity
+        expose :id
+        expose :name
+        expose :user, using: ::API::Entities::UserBasic
+        expose :created_at, :updated_at
+        expose :awardable_id, :awardable_type
+      end
     end
   end
 end

lib/api/v3/notes.rb

+module API
+  module V3
+    class Notes < Grape::API
+      include PaginationParams
+
+      before { authenticate! }
+
+      NOTEABLE_TYPES = [Issue, MergeRequest, Snippet]
+
+      params do
+        requires :id, type: String, desc: 'The ID of a project'
+      end
+      resource :projects do
+        NOTEABLE_TYPES.each do |noteable_type|
+          noteables_str = noteable_type.to_s.underscore.pluralize
+
+          desc 'Get a list of project +noteable+ notes' do
+            success ::API::V3::Entities::Note
+          end
+          params do
+            requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+            use :pagination
+          end
+          get ":id/#{noteables_str}/:noteable_id/notes" do
+            noteable = user_project.send(noteables_str.to_sym).find(params[:noteable_id])
+
+            if can?(current_user, noteable_read_ability_name(noteable), noteable)
+              # We exclude notes that are cross-references and that cannot be viewed
+              # by the current user. By doing this exclusion at this level and not
+              # at the DB query level (which we cannot in that case), the current
+              # page can have less elements than :per_page even if
+              # there's more than one page.
+              notes =
+                # paginate() only works with a relation. This could lead to a
+                # mismatch between the pagination headers info and the actual notes
+                # array returned, but this is really a edge-case.
+                paginate(noteable.notes).
+                reject { |n| n.cross_reference_not_visible_for?(current_user) }
+              present notes, with: ::API::V3::Entities::Note
+            else
+              not_found!("Notes")
+            end
+          end
+
+          desc 'Get a single +noteable+ note' do
+            success ::API::V3::Entities::Note
+          end
+          params do
+            requires :note_id, type: Integer, desc: 'The ID of a note'
+            requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          end
+          get ":id/#{noteables_str}/:noteable_id/notes/:note_id" do
+            noteable = user_project.send(noteables_str.to_sym).find(params[:noteable_id])
+            note = noteable.notes.find(params[:note_id])
+            can_read_note = can?(current_user, noteable_read_ability_name(noteable), noteable) && !note.cross_reference_not_visible_for?(current_user)
+
+            if can_read_note
+              present note, with: ::API::V3::Entities::Note
+            else
+              not_found!("Note")
+            end
+          end
+
+          desc 'Create a new +noteable+ note' do
+            success ::API::V3::Entities::Note
+          end
+          params do
+            requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+            requires :body, type: String, desc: 'The content of a note'
+            optional :created_at, type: String, desc: 'The creation date of the note'
+          end
+          post ":id/#{noteables_str}/:noteable_id/notes" do
+            opts = {
+              note: params[:body],
+              noteable_type: noteables_str.classify,
+              noteable_id: params[:noteable_id]
+            }
+
+            noteable = user_project.send(noteables_str.to_sym).find(params[:noteable_id])
+
+            if can?(current_user, noteable_read_ability_name(noteable), noteable)
+              if params[:created_at] && (current_user.is_admin? || user_project.owner == current_user)
+                opts[:created_at] = params[:created_at]
+              end
+
+              note = ::Notes::CreateService.new(user_project, current_user, opts).execute
+              if note.valid?
+                present note, with: ::API::V3::Entities::const_get(note.class.name)
+              else
+                not_found!("Note #{note.errors.messages}")
+              end
+            else
+              not_found!("Note")
+            end
+          end
+
+          desc 'Update an existing +noteable+ note' do
+            success ::API::V3::Entities::Note
+          end
+          params do
+            requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+            requires :note_id, type: Integer, desc: 'The ID of a note'
+            requires :body, type: String, desc: 'The content of a note'
+          end
+          put ":id/#{noteables_str}/:noteable_id/notes/:note_id" do
+            note = user_project.notes.find(params[:note_id])
+
+            authorize! :admin_note, note
+
+            opts = {
+              note: params[:body]
+            }
+
+            note = ::Notes::UpdateService.new(user_project, current_user, opts).execute(note)
+
+            if note.valid?
+              present note, with: ::API::V3::Entities::Note
+            else
+              render_api_error!("Failed to save note #{note.errors.messages}", 400)
+            end
+          end
+
+          desc 'Delete a +noteable+ note' do
+            success ::API::V3::Entities::Note
+          end
+          params do
+            requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+            requires :note_id, type: Integer, desc: 'The ID of a note'
+          end
+          delete ":id/#{noteables_str}/:noteable_id/notes/:note_id" do
+            note = user_project.notes.find(params[:note_id])
+            authorize! :admin_note, note
+
+            ::Notes::DestroyService.new(user_project, current_user).execute(note)
+
+            present note, with: ::API::V3::Entities::Note
+          end
+        end
+      end
+
+      helpers do
+        def noteable_read_ability_name(noteable)
+          "read_#{noteable.class.to_s.underscore}".to_sym
+        end
+      end
+    end
+  end
+end

lib/api/v3/projects.rb

@@ -232,13 +232,13 @@ def present_projects(projects, options = {})
         end
 
         desc 'Get events for a single project' do
-          success ::API::Entities::Event
+          success ::API::V3::Entities::Event
         end
         params do
           use :pagination
         end
         get ":id/events" do
-          present paginate(user_project.events.recent), with: ::API::Entities::Event
+          present paginate(user_project.events.recent), with: ::API::V3::Entities::Event
         end
 
         desc 'Fork new project for the current user or provided namespace.' do

lib/api/v3/users.rb

@@ -71,6 +71,27 @@ class Users < Grape::API
             user.activate
           end
         end
+
+        desc 'Get the contribution events of a specified user' do
+          detail 'This feature was introduced in GitLab 8.13.'
+          success ::API::V3::Entities::Event
+        end
+        params do
+          requires :id, type: Integer, desc: 'The ID of the user'
+          use :pagination
+        end
+        get ':id/events' do
+          user = User.find_by(id: params[:id])
+          not_found!('User') unless user
+
+          events = user.events.
+            merge(ProjectsFinder.new.execute(current_user)).
+            references(:project).
+            with_associations.
+            recent
+
+          present paginate(events), with: ::API::V3::Entities::Event
+        end
       end
 
       resource :user do

spec/requests/api/v3/notes_spec.rb

+require 'spec_helper'
+
+describe API::V3::Notes, api: true  do
+  include ApiHelpers
+  let(:user) { create(:user) }
+  let!(:project) { create(:empty_project, :public, namespace: user.namespace) }
+  let!(:issue) { create(:issue, project: project, author: user) }
+  let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: user) }
+  let!(:snippet) { create(:project_snippet, project: project, author: user) }
+  let!(:issue_note) { create(:note, noteable: issue, project: project, author: user) }
+  let!(:merge_request_note) { create(:note, noteable: merge_request, project: project, author: user) }
+  let!(:snippet_note) { create(:note, noteable: snippet, project: project, author: user) }
+
+  # For testing the cross-reference of a private issue in a public issue
+  let(:private_user)    { create(:user) }
+  let(:private_project) do
+    create(:empty_project, namespace: private_user.namespace).
+    tap { |p| p.team << [private_user, :master] }
+  end
+  let(:private_issue)    { create(:issue, project: private_project) }
+
+  let(:ext_proj)  { create(:empty_project, :public) }
+  let(:ext_issue) { create(:issue, project: ext_proj) }
+
+  let!(:cross_reference_note) do
+    create :note,
+    noteable: ext_issue, project: ext_proj,
+    note: "mentioned in issue #{private_issue.to_reference(ext_proj)}",
+    system: true
+  end
+
+  before { project.team << [user, :reporter] }
+
+  describe "GET /projects/:id/noteable/:noteable_id/notes" do
+    context "when noteable is an Issue" do
+      it "returns an array of issue notes" do
+        get v3_api("/projects/#{project.id}/issues/#{issue.id}/notes", user)
+
+        expect(response).to have_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(json_response.first['body']).to eq(issue_note.note)
+        expect(json_response.first['upvote']).to be_falsey
+        expect(json_response.first['downvote']).to be_falsey
+      end
+
+      it "returns a 404 error when issue id not found" do
+        get v3_api("/projects/#{project.id}/issues/12345/notes", user)
+
+        expect(response).to have_http_status(404)
+      end
+
+      context "and current user cannot view the notes" do
+        it "returns an empty array" do
+          get v3_api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user)
+
+          expect(response).to have_http_status(200)
+          expect(response).to include_pagination_headers
+          expect(json_response).to be_an Array
+          expect(json_response).to be_empty
+        end
+
+        context "and issue is confidential" do
+          before { ext_issue.update_attributes(confidential: true) }
+
+          it "returns 404" do
+            get v3_api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", user)
+
+            expect(response).to have_http_status(404)
+          end
+        end
+
+        context "and current user can view the note" do
+          it "returns an empty array" do
+            get v3_api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes", private_user)
+
+            expect(response).to have_http_status(200)
+            expect(response).to include_pagination_headers
+            expect(json_response).to be_an Array
+            expect(json_response.first['body']).to eq(cross_reference_note.note)
+          end
+        end
+      end
+    end
+
+    context "when noteable is a Snippet" do
+      it "returns an array of snippet notes" do
+        get v3_api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user)
+
+        expect(response).to have_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(json_response.first['body']).to eq(snippet_note.note)
+      end
+
+      it "returns a 404 error when snippet id not found" do
+        get v3_api("/projects/#{project.id}/snippets/42/notes", user)
+
+        expect(response).to have_http_status(404)
+      end
+
+      it "returns 404 when not authorized" do
+        get v3_api("/projects/#{project.id}/snippets/#{snippet.id}/notes", private_user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    context "when noteable is a Merge Request" do
+      it "returns an array of merge_requests notes" do
+        get v3_api("/projects/#{project.id}/merge_requests/#{merge_request.id}/notes", user)
+
+        expect(response).to have_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(json_response.first['body']).to eq(merge_request_note.note)
+      end
+
+      it "returns a 404 error if merge request id not found" do
+        get v3_api("/projects/#{project.id}/merge_requests/4444/notes", user)
+
+        expect(response).to have_http_status(404)
+      end
+
+      it "returns 404 when not authorized" do
+        get v3_api("/projects/#{project.id}/merge_requests/4444/notes", private_user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
+
+  describe "GET /projects/:id/noteable/:noteable_id/notes/:note_id" do
+    context "when noteable is an Issue" do
+      it "returns an issue note by id" do
+        get v3_api("/projects/#{project.id}/issues/#{issue.id}/notes/#{issue_note.id}", user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response['body']).to eq(issue_note.note)
+      end
+
+      it "returns a 404 error if issue note not found" do
+        get v3_api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user)
+
+        expect(response).to have_http_status(404)
+      end
+
+      context "and current user cannot view the note" do
+        it "returns a 404 error" do
+          get v3_api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", user)
+
+          expect(response).to have_http_status(404)
+        end
+
+        context "when issue is confidential" do
+          before { issue.update_attributes(confidential: true) }
+
+          it "returns 404" do
+            get v3_api("/projects/#{project.id}/issues/#{issue.id}/notes/#{issue_note.id}", private_user)
+
+            expect(response).to have_http_status(404)
+          end
+        end
+
+        context "and current user can view the note" do
+          it "returns an issue note by id" do
+            get v3_api("/projects/#{ext_proj.id}/issues/#{ext_issue.id}/notes/#{cross_reference_note.id}", private_user)
+
+            expect(response).to have_http_status(200)
+            expect(json_response['body']).to eq(cross_reference_note.note)
+          end
+        end
+      end
+    end
+
+    context "when noteable is a Snippet" do
+      it "returns a snippet note by id" do
+        get v3_api("/projects/#{project.id}/snippets/#{snippet.id}/notes/#{snippet_note.id}", user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response['body']).to eq(snippet_note.note)
+      end
+
+      it "returns a 404 error if snippet note not found" do
+        get v3_api("/projects/#{project.id}/snippets/#{snippet.id}/notes/12345", user)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
+
+  describe "POST /projects/:id/noteable/:noteable_id/notes" do
+    context "when noteable is an Issue" do
+      it "creates a new issue note" do
+        post v3_api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!'
+
+        expect(response).to have_http_status(201)
+        expect(json_response['body']).to eq('hi!')
+        expect(json_response['author']['username']).to eq(user.username)
+      end
+
+      it "returns a 400 bad request error if body not given" do
+        post v3_api("/projects/#{project.id}/issues/#{issue.id}/notes", user)
+
+        expect(response).to have_http_status(400)
+      end
+
+      it "returns a 401 unauthorized error if user not authenticated" do
+        post v3_api("/projects/#{project.id}/issues/#{issue.id}/notes"), body: 'hi!'
+
+        expect(response).to have_http_status(401)
+      end
+
+      context 'when an admin or owner makes the request' do
+        it 'accepts the creation date to be set' do
+          creation_time = 2.weeks.ago
+          post v3_api("/projects/#{project.id}/issues/#{issue.id}/notes", user),
+            body: 'hi!', created_at: creation_time
+
+          expect(response).to have_http_status(201)
+          expect(json_response['body']).to eq('hi!')
+          expect(json_response['author']['username']).to eq(user.username)
+          expect(Time.parse(json_response['created_at'])).to be_like_time(creation_time)
+        end
+      end
+
+      context 'when the user is posting an award emoji on an issue created by someone else' do
+        let(:issue2) { create(:issue, project: project) }
+
+        it 'returns an award emoji' do
+          post v3_api("/projects/#{project.id}/issues/#{issue2.id}/notes", user), body: ':+1:'
+
+          expect(response).to have_http_status(201)
+          expect(json_response['awardable_id']).to eq issue2.id
+        end
+      end
+
+      context 'when the user is posting an award emoji on his/her own issue' do
+        it 'creates a new issue note' do
+          post v3_api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: ':+1:'
+
+          expect(response).to have_http_status(201)
+          expect(json_response['body']).to eq(':+1:')
+        end
+      end
+    end
+
+    context "when noteable is a Snippet" do
+      it "creates a new snippet note" do
+        post v3_api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user), body: 'hi!'
+
+        expect(response).to have_http_status(201)
+        expect(json_response['body']).to eq('hi!')
+        expect(json_response['author']['username']).to eq(user.username)
+      end
+
+      it "returns a 400 bad request error if body not given" do
+        post v3_api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user)
+
+        expect(response).to have_http_status(400)
+      end
+