Remove deprecated upvotes and downvotes from the notes API

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

+---
+title: 'API: Remove deprecated fields Notes#upvotes and Notes#downvotes'
+merge_request: 1275
+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

@@ -408,8 +408,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

@@ -814,8 +814,6 @@ Example response:
       },
       "created_at": "2015-12-04T10:33:56.698Z",
       "system": false,
-      "upvote": false,
-      "downvote": false,
       "noteable_id": 377,
       "noteable_type": "Issue"
     },
@@ -843,13 +841,13 @@ The activities that update the timestamp are:
 
   - Git HTTP/SSH activities (such as clone, push)
   - User logging in into GitLab
-  
+
 The data is stored in Redis and it depends on it for being recorded and displayed
 over time. This means that we will lose the data if Redis gets flushed, or a custom
 TTL is reached.
 
 By default, it shows the activity for all users in the last 6 months, but this can be
-amended by using the `from` parameter. 
+amended by using the `from` parameter.
 
 This function takes pagination parameters `page` and `per_page` to restrict the list of users.
 

doc/api/v3_to_v4.md

@@ -30,3 +30,4 @@ changes are in V4:
 - Moved `PUT /users/:id/(block|unblock)` to `POST /users/:id/(block|unblock)`
 - Make subscription API more RESTful. Use `post ":id/#{type}/:subscribable_id/subscribe"` to subscribe and `post ":id/#{type}/:subscribable_id/unsubscribe"` to unsubscribe from a resource.
 - Labels filter on `projects/:id/issues` and `/issues` now matches only issues containing all labels (i.e.: Logical AND, not OR)
+- Notes do not return deprecated field `upvote` and `downvote` [!1275](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/1275)

lib/api/api.rb

@@ -13,6 +13,7 @@ module 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

@@ -388,9 +388,6 @@ module API
       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 @@ module API
           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

@@ -236,13 +236,13 @@ module API
         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 @@ module 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/users_spec.rb

@@ -186,4 +186,81 @@ describe API::V3::Users, api: true  do
       expect(response).to have_http_status(404)
     end
   end
+
+  describe 'GET /users/:id/events' do
+    let(:user) { create(:user) }
+    let(:project) { create(:empty_project) }
+    let(:note) { create(:note_on_issue, note: 'What an awesome day!', project: project) }
+
+    before do
+      project.add_user(user, :developer)
+      EventCreateService.new.leave_note(note, user)
+    end
+
+    context "as a user than cannot see the event's project" do
+      it 'returns no events' do
+        other_user = create(:user)
+
+        get api("/users/#{user.id}/events", other_user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_empty
+      end
+    end
+
+    context "as a user than can see the event's project" do
+      context 'joined event' do
+        it 'returns the "joined" event' do
+          get v3_api("/users/#{user.id}/events", user)
+
+          expect(response).to have_http_status(200)
+          expect(response).to include_pagination_headers
+          expect(json_response).to be_an Array
+
+          comment_event = json_response.find { |e| e['action_name'] == 'commented on' }
+
+          expect(comment_event['project_id'].to_i).to eq(project.id)
+          expect(comment_event['author_username']).to eq(user.username)
+          expect(comment_event['note']['id']).to eq(note.id)
+          expect(comment_event['note']['body']).to eq('What an awesome day!')
+
+          joined_event = json_response.find { |e| e['action_name'] == 'joined' }
+
+          expect(joined_event['project_id'].to_i).to eq(project.id)
+          expect(joined_event['author_username']).to eq(user.username)
+          expect(joined_event['author']['name']).to eq(user.name)
+        end
+      end
+
+      context 'when there are multiple events from different projects' do
+        let(:second_note) { create(:note_on_issue, project: create(:empty_project)) }
+        let(:third_note) { create(:note_on_issue, project: project) }
+
+        before do
+          second_note.project.add_user(user, :developer)
+
+          [second_note, third_note].each do |note|
+            EventCreateService.new.leave_note(note, user)
+          end
+        end
+
+        it 'returns events in the correct order (from newest to oldest)' do
+          get v3_api("/users/#{user.id}/events", user)
+
+          comment_events = json_response.select { |e| e['action_name'] == 'commented on' }
+
+          expect(comment_events[0]['target_id']).to eq(third_note.id)
+          expect(comment_events[1]['target_id']).to eq(second_note.id)
+          expect(comment_events[2]['target_id']).to eq(note.id)
+        end
+      end
+    end
+
+    it 'returns a 404 error if not found' do
+      get v3_api('/users/42/events', user)
+
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 User Not Found')
+    end
+  end
 end