kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Merge request and commit discussions API

Browse source
This commit is contained in:
Jan Provaznik 2018-05-01 12:39:44 +00:00 committed by Douwe Maan
parent 3fcb9c115d
commit 7a76caa5a8
17 changed files with 955 additions and 67 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
app/controllers/projects/notes_controller.rb
View file

@ -33,9 +33,7 @@ class Projects::NotesController < Projects::ApplicationController
def resolve
return render_404 unless note.resolvable?
note.resolve!(current_user)
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
Notes::ResolveService.new(project, current_user).execute(note)
discussion = note.discussion

4
app/models/commit.rb
View file

@ -105,6 +105,10 @@ class Commit
end
end
end
def parent_class
::Project
end
end
attr_accessor :raw

9
app/services/notes/resolve_service.rb Normal file
View file

@ -0,0 +1,9 @@
module Notes
class ResolveService < ::BaseService
def execute(note)
note.resolve!(current_user)
::MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
end
end
end

5
changelogs/unreleased/jprovazn-commit-notes-api.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Add discussion API for merge requests and commits
merge_request:
author:
type: added

585
doc/api/discussions.md
View file

@ -1,6 +1,6 @@
# Discussions API
Discussions are set of related notes on snippets or issues.
Discussions are set of related notes on snippets, issues, merge requests or commits.
## Issues
@ -61,7 +61,8 @@ GET /projects/:id/issues/:issue_iid/discussions
"system": false,
"noteable_id": 3,
"noteable_type": "Issue",
"noteable_iid": null
"noteable_iid": null,
"resolvable": false
}
]
},
@ -87,7 +88,8 @@ GET /projects/:id/issues/:issue_iid/discussions
"system": false,
"noteable_id": 3,
"noteable_type": "Issue",
"noteable_iid": null
"noteable_iid": null,
"resolvable": false
}
]
}
@ -265,7 +267,8 @@ GET /projects/:id/snippets/:snippet_id/discussions
"system": false,
"noteable_id": 3,
"noteable_type": "Snippet",
"noteable_id": null
"noteable_id": null,
"resolvable": false
}
]
},
@ -291,7 +294,8 @@ GET /projects/:id/snippets/:snippet_id/discussions
"system": false,
"noteable_id": 3,
"noteable_type": "Snippet",
"noteable_id": null
"noteable_id": null,
"resolvable": false
}
]
}
@ -409,3 +413,574 @@ Parameters:
```bash
curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/snippets/11/discussions/636
```
## Merge requests
### List project merge request discussions
Gets a list of all discussions for a single merge request.
```
GET /projects/:id/merge_requests/:merge_request_iid/discussions
```
| 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 |
```json
[
{
"id": "6a9c1750b37d513a43987b574953fceb50b03ce7",
"individual_note": false,
"notes": [
{
"id": 1126,
"type": "DiscussionNote",
"body": "discussion text",
"attachment": null,
"author": {
"id": 1,
"name": "root",
"username": "root",
"state": "active",
"avatar_url": "https://www.gravatar.com/avatar/00afb8fb6ab07c3ee3e9c1f38777e2f4?s=80&d=identicon",
"web_url": "http://localhost:3000/root"
},
"created_at": "2018-03-03T21:54:39.668Z",
"updated_at": "2018-03-03T21:54:39.668Z",
"system": false,
"noteable_id": 3,
"noteable_type": "Merge request",
"noteable_iid": null,
"resolved": false,
"resolvable": true,
"resolved_by": null
},
{
"id": 1129,
"type": "DiscussionNote",
"body": "reply to the discussion",
"attachment": null,
"author": {
"id": 1,
"name": "root",
"username": "root",
"state": "active",
"avatar_url": "https://www.gravatar.com/avatar/00afb8fb6ab07c3ee3e9c1f38777e2f4?s=80&d=identicon",
"web_url": "http://localhost:3000/root"
},
"created_at": "2018-03-04T13:38:02.127Z",
"updated_at": "2018-03-04T13:38:02.127Z",
"system": false,
"noteable_id": 3,
"noteable_type": "Merge request",
"noteable_iid": null,
"resolved": false,
"resolvable": true,
"resolved_by": null
}
]
},
{
"id": "87805b7c09016a7058e91bdbe7b29d1f284a39e6",
"individual_note": true,
"notes": [
{
"id": 1128,
"type": null,
"body": "a single comment",
"attachment": null,
"author": {
"id": 1,
"name": "root",
"username": "root",
"state": "active",
"avatar_url": "https://www.gravatar.com/avatar/00afb8fb6ab07c3ee3e9c1f38777e2f4?s=80&d=identicon",
"web_url": "http://localhost:3000/root"
},
"created_at": "2018-03-04T09:17:22.520Z",
"updated_at": "2018-03-04T09:17:22.520Z",
"system": false,
"noteable_id": 3,
"noteable_type": "Merge request",
"noteable_iid": null,
"resolved": false,
"resolvable": true,
"resolved_by": null
}
]
}
]
```
Diff comments contain also position:
```json
[
{
"id": "87805b7c09016a7058e91bdbe7b29d1f284a39e6",
"individual_note": false,
"notes": [
{
"id": 1128,
"type": DiffNote,
"body": "diff comment",
"attachment": null,
"author": {
"id": 1,
"name": "root",
"username": "root",
"state": "active",
"avatar_url": "https://www.gravatar.com/avatar/00afb8fb6ab07c3ee3e9c1f38777e2f4?s=80&d=identicon",
"web_url": "http://localhost:3000/root"
},
"created_at": "2018-03-04T09:17:22.520Z",
"updated_at": "2018-03-04T09:17:22.520Z",
"system": false,
"noteable_id": 3,
"noteable_type": "Merge request",
"noteable_iid": null,
"position": {
"base_sha": "b5d6e7b1613fca24d250fa8e5bc7bcc3dd6002ef",
"start_sha": "7c9c2ead8a320fb7ba0b4e234bd9529a2614e306",
"head_sha": "4803c71e6b1833ca72b8b26ef2ecd5adc8a38031",
"old_path": "package.json",
"new_path": "package.json",
"position_type": "text",
"old_line": 27,
"new_line": 27
},
"resolved": false,
"resolvable": true,
"resolved_by": null
}
]
}
]
```
```bash
curl --request GET --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions
```
### Get single merge request discussion
Returns a single discussion for a specific project merge request
```
GET /projects/:id/merge_requests/:merge_request_iid/discussions/:discussion_id
```
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 |
| `discussion_id` | integer | yes | The ID of a discussion |
```bash
curl --request GET --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7
```
### Create new merge request discussion
Creates a new discussion to a single project merge request. This is similar to creating
a note but but another comments (replies) can be added to it later.
```
POST /projects/:id/merge_requests/:merge_request_iid/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 a discussion |
| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z |
| `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', allowed values: 'text' or 'image' |
| `position[new_path]` | string | no | File path after change |
| `position[new_line]` | integer | no | Line number after change (for 'text' diff notes) |
| `position[old_path]` | string | no | File path before change |
| `position[old_line]` | integer | no | Line number before change (for 'text' diff notes) |
| `position[width]` | integer | no | Width of the image (for 'image' diff notes) |
| `position[height]` | integer | no | Height of the image (for 'image' diff notes) |
| `position[x]` | integer | no | X coordinate (for 'image' diff notes) |
| `position[y]` | integer | no | Y coordinate (for 'image' diff notes) |
```bash
curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions?body=comment
```
### Resolve a merge request discussion
Resolve/unresolve whole discussion of a merge request.
```
PUT /projects/:id/merge_requests/:merge_request_iid/discussions/:discussion_id
```
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 |
| `discussion_id` | integer | yes | The ID of a discussion |
| `resolved` | boolean | yes | Resolve/unresolve the discussion |
```bash
curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7?resolved=true
```
### Add note to existing merge request discussion
Adds a new note to the discussion.
```
POST /projects/:id/merge_requests/:merge_request_iid/discussions/:discussion_id/notes
```
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 |
| `discussion_id` | integer | yes | The ID of a discussion |
| `note_id` | integer | yes | The ID of a discussion note |
| `body` | string | yes | The content of a discussion |
| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z |
```bash
curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7/notes?body=comment
```
### Modify an existing merge request discussion note
Modify or resolve an existing discussion note of a merge request.
```
PUT /projects/:id/merge_requests/:merge_request_iid/discussions/:discussion_id/notes/:note_id
```
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 |
| `discussion_id` | integer | yes | The ID of a discussion |
| `note_id` | integer | yes | The ID of a discussion note |
| `body` | string | no | The content of a discussion (exactly one of `body` or `resolved` must be set |
| `resolved` | boolean | no | Resolve/unresolve the note (exactly one of `body` or `resolved` must be set |
```bash
curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7/notes/1108?body=comment
```
Resolving a note:
```bash
curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7/notes/1108?resolved=true
```
### Delete a merge request discussion note
Deletes an existing discussion note of a merge request.
```
DELETE /projects/:id/merge_requests/:merge_request_iid/discussions/:discussion_id/notes/:note_id
```
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 |
| `discussion_id` | integer | yes | The ID of a discussion |
| `note_id` | integer | yes | The ID of a discussion note |
```bash
curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/discussions/636
```
## Commits
### List project commit discussions
Gets a list of all discussions for a single commit.
```
GET /projects/:id/commits/:commit_id/discussions
```
| Attribute | Type | Required | Description |
| ------------------- | ---------------- | ---------- | ------------ |
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
| `commit_id` | integer | yes | The ID of a commit |
```json
[
{
"id": "6a9c1750b37d513a43987b574953fceb50b03ce7",
"individual_note": false,
"notes": [
{
"id": 1126,
"type": "DiscussionNote",
"body": "discussion text",
"attachment": null,
"author": {
"id": 1,
"name": "root",
"username": "root",
"state": "active",
"avatar_url": "https://www.gravatar.com/avatar/00afb8fb6ab07c3ee3e9c1f38777e2f4?s=80&d=identicon",
"web_url": "http://localhost:3000/root"
},
"created_at": "2018-03-03T21:54:39.668Z",
"updated_at": "2018-03-03T21:54:39.668Z",
"system": false,
"noteable_id": 3,
"noteable_type": "Commit",
"noteable_iid": null,
"resolvable": false
},
{
"id": 1129,
"type": "DiscussionNote",
"body": "reply to the discussion",
"attachment": null,
"author": {
"id": 1,
"name": "root",
"username": "root",
"state": "active",
"avatar_url": "https://www.gravatar.com/avatar/00afb8fb6ab07c3ee3e9c1f38777e2f4?s=80&d=identicon",
"web_url": "http://localhost:3000/root"
},
"created_at": "2018-03-04T13:38:02.127Z",
"updated_at": "2018-03-04T13:38:02.127Z",
"system": false,
"noteable_id": 3,
"noteable_type": "Commit",
"noteable_iid": null,
"resolvable": false
}
]
},
{
"id": "87805b7c09016a7058e91bdbe7b29d1f284a39e6",
"individual_note": true,
"notes": [
{
"id": 1128,
"type": null,
"body": "a single comment",
"attachment": null,
"author": {
"id": 1,
"name": "root",
"username": "root",
"state": "active",
"avatar_url": "https://www.gravatar.com/avatar/00afb8fb6ab07c3ee3e9c1f38777e2f4?s=80&d=identicon",
"web_url": "http://localhost:3000/root"
},
"created_at": "2018-03-04T09:17:22.520Z",
"updated_at": "2018-03-04T09:17:22.520Z",
"system": false,
"noteable_id": 3,
"noteable_type": "Commit",
"noteable_iid": null,
"resolvable": false
}
]
}
]
```
Diff comments contain also position:
```json
[
{
"id": "87805b7c09016a7058e91bdbe7b29d1f284a39e6",
"individual_note": false,
"notes": [
{
"id": 1128,
"type": DiffNote,
"body": "diff comment",
"attachment": null,
"author": {
"id": 1,
"name": "root",
"username": "root",
"state": "active",
"avatar_url": "https://www.gravatar.com/avatar/00afb8fb6ab07c3ee3e9c1f38777e2f4?s=80&d=identicon",
"web_url": "http://localhost:3000/root"
},
"created_at": "2018-03-04T09:17:22.520Z",
"updated_at": "2018-03-04T09:17:22.520Z",
"system": false,
"noteable_id": 3,
"noteable_type": "Commit",
"noteable_iid": null,
"position": {
"base_sha": "b5d6e7b1613fca24d250fa8e5bc7bcc3dd6002ef",
"start_sha": "7c9c2ead8a320fb7ba0b4e234bd9529a2614e306",
"head_sha": "4803c71e6b1833ca72b8b26ef2ecd5adc8a38031",
"old_path": "package.json",
"new_path": "package.json",
"position_type": "text",
"old_line": 27,
"new_line": 27
},
"resolvable": false
}
]
}
]
```
```bash
curl --request GET --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/commits/11/discussions
```
### Get single commit discussion
Returns a single discussion for a specific project commit
```
GET /projects/:id/commits/:commit_id/discussions/:discussion_id
```
Parameters:
| Attribute | Type | Required | Description |
| ------------------- | -------------- | -------- | ----------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
| `commit_id` | integer | yes | The ID of a commit |
| `discussion_id` | integer | yes | The ID of a discussion |
```bash
curl --request GET --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/commits/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7
```
### Create new commit discussion
Creates a new discussion to a single project commit. This is similar to creating
a note but but another comments (replies) can be added to it later.
```
POST /projects/:id/commits/:commit_id/discussions
```
Parameters:
| Attribute | Type | Required | Description |
| ------------------------- | -------------- | -------- | ----------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
| `commit_id` | integer | yes | The ID of a commit |
| `body` | string | yes | The content of a discussion |
| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z |
| `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 commit |
| `position[position_type]` | string | yes | Type of the position reference', allowed values: 'text' or 'image' |
| `position[new_path]` | string | no | File path after change |
| `position[new_line]` | integer | no | Line number after change |
| `position[old_path]` | string | no | File path before change |
| `position[old_line]` | integer | no | Line number before change |
| `position[width]` | integer | no | Width of the image (for 'image' diff notes) |
| `position[height]` | integer | no | Height of the image (for 'image' diff notes) |
| `position[x]` | integer | no | X coordinate (for 'image' diff notes) |
| `position[y]` | integer | no | Y coordinate (for 'image' diff notes) |
```bash
curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/commits/11/discussions?body=comment
```
### Add note to existing commit discussion
Adds a new note to the discussion.
```
POST /projects/:id/commits/:commit_id/discussions/:discussion_id/notes
```
Parameters:
| Attribute | Type | Required | Description |
| ------------------- | -------------- | -------- | ----------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
| `commit_id` | integer | yes | The ID of a commit |
| `discussion_id` | integer | yes | The ID of a discussion |
| `note_id` | integer | yes | The ID of a discussion note |
| `body` | string | yes | The content of a discussion |
| `created_at` | string | no | Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z |
```bash
curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/commits/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7/notes?body=comment
```
### Modify an existing commit discussion note
Modify or resolve an existing discussion note of a commit.
```
PUT /projects/:id/commits/:commit_id/discussions/:discussion_id/notes/:note_id
```
Parameters:
| Attribute | Type | Required | Description |
| ------------------- | -------------- | -------- | ----------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
| `commit_id` | integer | yes | The ID of a commit |
| `discussion_id` | integer | yes | The ID of a discussion |
| `note_id` | integer | yes | The ID of a discussion note |
| `body` | string | no | The content of a note |
```bash
curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/commits/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7/notes/1108?body=comment
```
Resolving a note:
```bash
curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/commits/11/discussions/6a9c1750b37d513a43987b574953fceb50b03ce7/notes/1108?resolved=true
```
### Delete a commit discussion note
Deletes an existing discussion note of a commit.
```
DELETE /projects/:id/commits/:commit_id/discussions/:discussion_id/notes/:note_id
```
Parameters:
| Attribute | Type | Required | Description |
| ------------------- | -------------- | -------- | ----------- |
| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
| `commit_id` | integer | yes | The ID of a commit |
| `discussion_id` | integer | yes | The ID of a discussion |
| `note_id` | integer | yes | The ID of a discussion note |
```bash
curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/commits/11/discussions/636
```

9
doc/api/notes.md
View file

@ -39,7 +39,8 @@ GET /projects/:id/issues/:issue_iid/notes?sort=asc&order_by=updated_at
"system": true,
"noteable_id": 377,
"noteable_type": "Issue",
"noteable_iid": 377
"noteable_iid": 377,
"resolvable": false
},
{
"id": 305,
@ -58,7 +59,8 @@ GET /projects/:id/issues/:issue_iid/notes?sort=asc&order_by=updated_at
"system": true,
"noteable_id": 121,
"noteable_type": "Issue",
"noteable_iid": 121
"noteable_iid": 121,
"resolvable": false
}
]
```
@ -314,7 +316,8 @@ Parameters:
"system": false,
"noteable_id": 2,
"noteable_type": "MergeRequest",
"noteable_iid": 2
"noteable_iid": 2,
"resolvable": false
}
```

88
lib/api/discussions.rb
View file

@ -5,11 +5,12 @@ module API
before { authenticate! }
NOTEABLE_TYPES = [Issue, Snippet].freeze
NOTEABLE_TYPES = [Issue, Snippet, MergeRequest, Commit].freeze
NOTEABLE_TYPES.each do |noteable_type|
parent_type = noteable_type.parent_class.to_s.underscore
noteables_str = noteable_type.to_s.underscore.pluralize
noteables_path = noteable_type == Commit ? "repository/#{noteables_str}" : noteables_str
params do
requires :id, type: String, desc: "The ID of a #{parent_type}"
@ -19,14 +20,12 @@ module API
success Entities::Discussion
end
params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
use :pagination
end
get ":id/#{noteables_str}/:noteable_id/discussions" do
get ":id/#{noteables_path}/:noteable_id/discussions" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
break not_found!("Discussions") unless can?(current_user, noteable_read_ability_name(noteable), noteable)
notes = noteable.notes
.inc_relations_for_view
.includes(:noteable)
@ -43,13 +42,13 @@ module API
end
params do
requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
end
get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id" do
get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
notes = readable_discussion_notes(noteable, params[:discussion_id])
if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable)
if notes.empty?
break not_found!("Discussion")
end
@ -62,19 +61,36 @@ module API
success Entities::Discussion
end
params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
requires :noteable_id, types: [Integer, String], 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'
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/#{noteables_str}/:noteable_id/discussions" do
post ":id/#{noteables_path}/:noteable_id/discussions" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
type = params[:position] ? 'DiffNote' : 'DiscussionNote'
id_key = noteable.is_a?(Commit) ? :commit_id : :noteable_id
opts = {
note: params[:body],
created_at: params[:created_at],
type: 'DiscussionNote',
type: type,
noteable_type: noteables_str.classify,
noteable_id: noteable.id
position: params[:position],
id_key => noteable.id
}
note = create_note(noteable, opts)
@ -91,13 +107,13 @@ module API
end
params do
requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
end
get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes" do
get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
notes = readable_discussion_notes(noteable, params[:discussion_id])
if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable)
if notes.empty?
break not_found!("Notes")
end
@ -108,12 +124,12 @@ module API
success Entities::Note
end
params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :discussion_id, type: String, desc: 'The ID of a discussion'
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/discussions/:discussion_id/notes" do
post ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
notes = readable_discussion_notes(noteable, params[:discussion_id])
@ -139,11 +155,11 @@ module API
success Entities::Note
end
params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :note_id, type: Integer, desc: 'The ID of a note'
end
get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
get_note(noteable, params[:note_id])
@ -153,30 +169,52 @@ module API
success Entities::Note
end
params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :note_id, type: Integer, desc: 'The ID of a note'
requires :body, type: String, desc: 'The content of a note'
optional :body, type: String, desc: 'The content of a note'
optional :resolved, type: Boolean, desc: 'Mark note resolved/unresolved'
exactly_one_of :body, :resolved
end
put ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
update_note(noteable, params[:note_id])
if params[:resolved].nil?
update_note(noteable, params[:note_id])
else
resolve_note(noteable, params[:note_id], params[:resolved])
end
end
desc "Delete a comment in a #{noteable_type.to_s.downcase} discussion" do
success Entities::Note
end
params do
requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :note_id, type: Integer, desc: 'The ID of a note'
end
delete ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
delete ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
delete_note(noteable, params[:note_id])
end
if Noteable::RESOLVABLE_TYPES.include?(noteable_type.to_s)
desc "Resolve/unresolve an existing #{noteable_type.to_s.downcase} discussion" do
success Entities::Discussion
end
params do
requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
requires :discussion_id, type: String, desc: 'The ID of a discussion'
requires :resolved, type: Boolean, desc: 'Mark discussion resolved/unresolved'
end
put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do
noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
resolve_discussion(noteable, params[:discussion_id], params[:resolved])
end
end
end
end

19
lib/api/entities.rb
View file

@ -286,6 +286,10 @@ module API
end
end
class DiffRefs < Grape::Entity
expose :base_sha, :head_sha, :start_sha
end
class Commit < Grape::Entity
expose :id, :short_id, :title, :created_at
expose :parent_ids
@ -601,6 +605,8 @@ module API
merge_request.metrics&.pipeline
end
expose :diff_refs, using: Entities::DiffRefs
def build_available?(options)
options[:project]&.feature_available?(:builds, options[:current_user])
end
@ -642,6 +648,11 @@ module API
expose :id, :key, :created_at
end
class DiffPosition < Grape::Entity
expose :base_sha, :start_sha, :head_sha, :old_path, :new_path,
:position_type
end
class Note < Grape::Entity
# Only Issue and MergeRequest have iid
NOTEABLE_TYPES_WITH_IID = %w(Issue MergeRequest).freeze
@ -655,6 +666,14 @@ module API
expose :system?, as: :system
expose :noteable_id, :noteable_type
expose :position, if: ->(note, options) { note.diff_note? } do |note|
note.position.to_h
end
expose :resolvable?, as: :resolvable
expose :resolved?, as: :resolved, if: ->(note, options) { note.resolvable? }
expose :resolved_by, using: Entities::UserBasic, if: ->(note, options) { note.resolvable? }
# Avoid N+1 queries as much as possible
expose(:noteable_iid) { |note| note.noteable.iid if NOTEABLE_TYPES_WITH_IID.include?(note.noteable_type) }
end

4
lib/api/helpers.rb
View file

@ -171,6 +171,10 @@ module API
MergeRequestsFinder.new(current_user, project_id: user_project.id).find_by!(iid: iid)
end
def find_project_commit(id)
user_project.commit_by(oid: id)
end
def find_project_snippet(id)
finder_params = { project: user_project }
SnippetsFinder.new(current_user, finder_params).find(id)

56
lib/api/helpers/notes_helpers.rb
View file

@ -21,6 +21,23 @@ module API
end
end
def resolve_note(noteable, note_id, resolved)
note = noteable.notes.find(note_id)
authorize! :resolve_note, note
bad_request!("Note is not resolvable") unless note.resolvable?
if resolved
parent = noteable_parent(noteable)
::Notes::ResolveService.new(parent, current_user).execute(note)
else
note.unresolve!
end
present note, with: Entities::Note
end
def delete_note(noteable, note_id)
note = noteable.notes.find(note_id)
@ -35,7 +52,7 @@ module API
def get_note(noteable, note_id)
note = noteable.notes.with_metadata.find(params[:note_id])
can_read_note = can?(current_user, noteable_read_ability_name(noteable), noteable) && !note.cross_reference_not_visible_for?(current_user)
can_read_note = !note.cross_reference_not_visible_for?(current_user)
if can_read_note
present note, with: Entities::Note
@ -49,7 +66,20 @@ module API
end
def find_noteable(parent, noteables_str, noteable_id)
public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
readable =
if noteable.is_a?(Commit)
# for commits there is not :read_commit policy, check if user
# has :read_note permission on the commit's project
can?(current_user, :read_note, user_project)
else
can?(current_user, noteable_read_ability_name(noteable), noteable)
end
return not_found!(noteables_str) unless readable
noteable
end
def noteable_parent(noteable)
@ -57,11 +87,8 @@ module API
end
def create_note(noteable, opts)
noteables_str = noteable.model_name.to_s.underscore.pluralize
return not_found!(noteables_str) unless can?(current_user, noteable_read_ability_name(noteable), noteable)
authorize! :create_note, noteable
policy_object = noteable.is_a?(Commit) ? user_project : noteable
authorize!(:create_note, policy_object)
parent = noteable_parent(noteable)
@ -73,6 +100,21 @@ module API
project = parent if parent.is_a?(Project)
::Notes::CreateService.new(project, current_user, opts).execute
end
def resolve_discussion(noteable, discussion_id, resolved)
discussion = noteable.find_discussion(discussion_id)
forbidden! unless discussion.can_resolve?(current_user)
if resolved
parent = noteable_parent(noteable)
::Discussions::ResolveService.new(parent, current_user, merge_request: noteable).execute(discussion)
else
discussion.unresolve!
end
present discussion, with: Entities::Discussion
end
end
end
end

30
lib/api/notes.rb
View file

@ -31,23 +31,19 @@ module API
get ":id/#{noteables_str}/:noteable_id/notes" do
noteable = find_noteable(parent_type, noteables_str, 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.
raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
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(raw_notes)
.reject { |n| n.cross_reference_not_visible_for?(current_user) }
present notes, with: Entities::Note
else
not_found!("Notes")
end
# 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.
raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
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(raw_notes)
.reject { |n| n.cross_reference_not_visible_for?(current_user) }
present notes, with: Entities::Note
end
desc "Get a single #{noteable_type.to_s.downcase} note" do

4
lib/gitlab/diff/position.rb
View file

@ -12,6 +12,10 @@ module Gitlab
:head_sha,
:old_line,
:new_line,
:width,
:height,
:x,
:y,
:position_type, to: :formatter
# A position can belong to a text line or to an image coordinate

5
spec/fixtures/api/schemas/public_api/v4/notes.json vendored
View file

@ -24,7 +24,10 @@
"system": { "type": "boolean" },
"noteable_id": { "type": "integer" },
"noteable_iid": { "type": "integer" },
"noteable_type": { "type": "string" }
"noteable_type": { "type": "string" },
"resolved": { "type": "boolean" },
"resolvable": { "type": "boolean" },
"resolved_by": { "type": ["string", "null"] }
},
"required": [
"id", "body", "attachment", "author", "created_at", "updated_at",

33
spec/requests/api/discussions_spec.rb
View file

@ -2,32 +2,53 @@ require 'spec_helper'
describe API::Discussions do
let(:user) { create(:user) }
let!(:project) { create(:project, :public, namespace: user.namespace) }
let!(:project) { create(:project, :public, :repository, namespace: user.namespace) }
let(:private_user) { create(:user) }
before do
project.add_reporter(user)
project.add_developer(user)
end
context "when noteable is an Issue" do
context 'when noteable is an Issue' do
let!(:issue) { create(:issue, project: project, author: user) }
let!(:issue_note) { create(:discussion_note_on_issue, noteable: issue, project: project, author: user) }
it_behaves_like "discussions API", 'projects', 'issues', 'iid' do
it_behaves_like 'discussions API', 'projects', 'issues', 'iid' do
let(:parent) { project }
let(:noteable) { issue }
let(:note) { issue_note }
end
end
context "when noteable is a Snippet" do
context 'when noteable is a Snippet' do
let!(:snippet) { create(:project_snippet, project: project, author: user) }
let!(:snippet_note) { create(:discussion_note_on_snippet, noteable: snippet, project: project, author: user) }
it_behaves_like "discussions API", 'projects', 'snippets', 'id' do
it_behaves_like 'discussions API', 'projects', 'snippets', 'id' do
let(:parent) { project }
let(:noteable) { snippet }
let(:note) { snippet_note }
end
end
context 'when noteable is a Merge Request' do
let!(:noteable) { create(:merge_request_with_diffs, source_project: project, target_project: project, author: user) }
let!(:note) { create(:discussion_note_on_merge_request, noteable: noteable, project: project, author: user) }
let!(:diff_note) { create(:diff_note_on_merge_request, noteable: noteable, project: project, author: user) }
let(:parent) { project }
it_behaves_like 'discussions API', 'projects', 'merge_requests', 'iid'
it_behaves_like 'diff discussions API', 'projects', 'merge_requests', 'iid'
it_behaves_like 'resolvable discussions API', 'projects', 'merge_requests', 'iid'
end
context 'when noteable is a Commit' do
let!(:noteable) { create(:commit, project: project, author: user) }
let!(:note) { create(:discussion_note_on_commit, commit_id: noteable.id, project: project, author: user) }
let!(:diff_note) { create(:diff_note_on_commit, commit_id: noteable.id, project: project, author: user) }
let(:parent) { project }
it_behaves_like 'discussions API', 'projects', 'repository/commits', 'id'
it_behaves_like 'diff discussions API', 'projects', 'repository/commits', 'id'
end
end

23
spec/services/notes/resolve_service_spec.rb Normal file
View file

@ -0,0 +1,23 @@
require 'spec_helper'
describe Notes::ResolveService do
let(:merge_request) { create(:merge_request) }
let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.project) }
let(:user) { merge_request.author }
describe '#execute' do
it "resolves the note" do
described_class.new(merge_request.project, user).execute(note)
note.reload
expect(note.resolved?).to be true
expect(note.resolved_by).to eq(user)
end
it "sends notifications if all discussions are resolved" do
expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
described_class.new(merge_request.project, user).execute(note)
end
end
end

57
spec/support/shared_examples/requests/api/diff_discussions.rb Normal file
View file

@ -0,0 +1,57 @@
shared_examples 'diff discussions API' do |parent_type, noteable_type, id_name|
describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do
it "includes diff discussions" do
get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user)
discussion = json_response.find { |record| record['id'] == diff_note.discussion_id }
expect(response).to have_gitlab_http_status(200)
expect(discussion).not_to be_nil
expect(discussion['individual_note']).to eq(false)
expect(discussion['notes'].first['body']).to eq(diff_note.note)
end
end
describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do
it "returns a discussion by id" do
get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions/#{diff_note.discussion_id}", user)
expect(response).to have_gitlab_http_status(200)
expect(json_response['id']).to eq(diff_note.discussion_id)
expect(json_response['notes'].first['body']).to eq(diff_note.note)
expect(json_response['notes'].first['position']).to eq(diff_note.position.to_h.stringify_keys)
end
end
describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do
it "creates a new diff note" do
position = diff_note.position.to_h
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), body: 'hi!', position: position
expect(response).to have_gitlab_http_status(201)
expect(json_response['notes'].first['body']).to eq('hi!')
expect(json_response['notes'].first['type']).to eq('DiffNote')
expect(json_response['notes'].first['position']).to eq(position.stringify_keys)
end
it "returns a 400 bad request error when position is invalid" do
position = diff_note.position.to_h.merge(new_line: '100000')
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), body: 'hi!', position: position
expect(response).to have_gitlab_http_status(400)
end
end
describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes" do
it 'adds a new note to the diff discussion' do
post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{diff_note.discussion_id}/notes", user), body: 'hi!'
expect(response).to have_gitlab_http_status(201)
expect(json_response['body']).to eq('hi!')
expect(json_response['type']).to eq('DiffNote')
end
end
end

87
spec/support/shared_examples/requests/api/resolvable_discussions.rb Normal file
View file

@ -0,0 +1,87 @@
shared_examples 'resolvable discussions API' do |parent_type, noteable_type, id_name|
describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do
it "resolves discussion if resolved is true" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}", user), resolved: true
expect(response).to have_gitlab_http_status(200)
expect(json_response['notes'].size).to eq(1)
expect(json_response['notes'][0]['resolved']).to eq(true)
end
it "unresolves discussion if resolved is false" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}", user), resolved: false
expect(response).to have_gitlab_http_status(200)
expect(json_response['notes'].size).to eq(1)
expect(json_response['notes'][0]['resolved']).to eq(false)
end
it "returns a 400 bad request error if resolved parameter is not passed" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}", user)
expect(response).to have_gitlab_http_status(400)
end
it "returns a 401 unauthorized error if user is not authenticated" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}"), resolved: true
expect(response).to have_gitlab_http_status(401)
end
it "returns a 403 error if user resolves discussion of someone else" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}", private_user), resolved: true
expect(response).to have_gitlab_http_status(403)
end
context 'when user does not have access to read the discussion' do
before do
parent.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
end
it 'responds with 404' do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}", private_user), resolved: true
expect(response).to have_gitlab_http_status(404)
end
end
end
describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
it 'returns resolved note when resolved parameter is true' do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}/notes/#{note.id}", user), resolved: true
expect(response).to have_gitlab_http_status(200)
expect(json_response['resolved']).to eq(true)
end
it 'returns a 404 error when note id not found' do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}/notes/12345", user),
body: 'Hello!'
expect(response).to have_gitlab_http_status(404)
end
it 'returns a 400 bad request error if neither body nor resolved parameter is given' do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}/notes/#{note.id}", user)
expect(response).to have_gitlab_http_status(400)
end
it "returns a 403 error if user resolves note of someone else" do
put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
"discussions/#{note.discussion_id}/notes/#{note.id}", private_user), resolved: true
expect(response).to have_gitlab_http_status(403)
end
end
end