Commit 77760455 authored by Sean McGivern's avatar Sean McGivern

Merge branch '23061-consolidate-project-lists' into 'master'

API: consolidate project lists

Closes #23061

See merge request !8962
parents 7b67d3d3 4e9e29d2
---
title: 'API: Consolidate /projects endpoint'
merge_request: 8962
author:
...@@ -73,6 +73,8 @@ Parameters: ...@@ -73,6 +73,8 @@ Parameters:
| `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` | | `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` |
| `search` | string | no | Return list of authorized projects matching the search criteria | | `search` | string | no | Return list of authorized projects matching the search criteria |
| `simple` | boolean | no | Return only the ID, URL, name, and path of each project | | `simple` | boolean | no | Return only the ID, URL, name, and path of each project |
| `owned` | boolean | no | Limit by projects owned by the current user |
| `starred` | boolean | no | Limit by projects starred by the current user |
Example response: Example response:
......
...@@ -36,6 +36,8 @@ Parameters: ...@@ -36,6 +36,8 @@ Parameters:
| `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` | | `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` |
| `search` | string | no | Return list of authorized projects matching the search criteria | | `search` | string | no | Return list of authorized projects matching the search criteria |
| `simple` | boolean | no | Return only the ID, URL, name, and path of each project | | `simple` | boolean | no | Return only the ID, URL, name, and path of each project |
| `owned` | boolean | no | Limit by projects owned by the current user |
| `starred` | boolean | no | Limit by projects starred by the current user |
```json ```json
[ [
...@@ -152,190 +154,6 @@ Parameters: ...@@ -152,190 +154,6 @@ Parameters:
] ]
``` ```
Get a list of projects which the authenticated user can see.
```
GET /projects/visible
```
Parameters:
| Attribute | Type | Required | Description |
| --------- | ---- | -------- | ----------- |
| `archived` | boolean | no | Limit by archived status |
| `visibility` | string | no | Limit by visibility `public`, `internal`, or `private` |
| `order_by` | string | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. Default is `created_at` |
| `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` |
| `search` | string | no | Return list of authorized projects matching the search criteria |
| `simple` | boolean | no | Return only the ID, URL, name, and path of each project |
```json
[
{
"id": 4,
"description": null,
"default_branch": "master",
"public": false,
"visibility_level": 0,
"ssh_url_to_repo": "git@example.com:diaspora/diaspora-client.git",
"http_url_to_repo": "http://example.com/diaspora/diaspora-client.git",
"web_url": "http://example.com/diaspora/diaspora-client",
"tag_list": [
"example",
"disapora client"
],
"owner": {
"id": 3,
"name": "Diaspora",
"created_at": "2013-09-30T13:46:02Z"
},
"name": "Diaspora Client",
"name_with_namespace": "Diaspora / Diaspora Client",
"path": "diaspora-client",
"path_with_namespace": "diaspora/diaspora-client",
"issues_enabled": true,
"open_issues_count": 1,
"merge_requests_enabled": true,
"builds_enabled": true,
"wiki_enabled": true,
"snippets_enabled": false,
"container_registry_enabled": false,
"created_at": "2013-09-30T13:46:02Z",
"last_activity_at": "2013-09-30T13:46:02Z",
"creator_id": 3,
"namespace": {
"id": 3,
"name": "Diaspora",
"path": "diaspora",
"kind": "group"
},
"archived": false,
"avatar_url": "http://example.com/uploads/project/avatar/4/uploads/avatar.png",
"shared_runners_enabled": true,
"forks_count": 0,
"star_count": 0,
"runners_token": "b8547b1dc37721d05889db52fa2f02",
"public_builds": true,
"shared_with_groups": []
},
{
"id": 6,
"description": null,
"default_branch": "master",
"public": false,
"visibility_level": 0,
"ssh_url_to_repo": "git@example.com:brightbox/puppet.git",
"http_url_to_repo": "http://example.com/brightbox/puppet.git",
"web_url": "http://example.com/brightbox/puppet",
"tag_list": [
"example",
"puppet"
],
"owner": {
"id": 4,
"name": "Brightbox",
"created_at": "2013-09-30T13:46:02Z"
},
"name": "Puppet",
"name_with_namespace": "Brightbox / Puppet",
"path": "puppet",
"path_with_namespace": "brightbox/puppet",
"issues_enabled": true,
"open_issues_count": 1,
"merge_requests_enabled": true,
"builds_enabled": true,
"wiki_enabled": true,
"snippets_enabled": false,
"container_registry_enabled": false,
"created_at": "2013-09-30T13:46:02Z",
"last_activity_at": "2013-09-30T13:46:02Z",
"creator_id": 3,
"namespace": {
"id": 4,
"name": "Brightbox",
"path": "brightbox",
"kind": "group"
},
"permissions": {
"project_access": {
"access_level": 10,
"notification_level": 3
},
"group_access": {
"access_level": 50,
"notification_level": 3
}
},
"archived": false,
"avatar_url": null,
"shared_runners_enabled": true,
"forks_count": 0,
"star_count": 0,
"runners_token": "b8547b1dc37721d05889db52fa2f02",
"public_builds": true,
"shared_with_groups": []
}
]
```
### List owned projects
Get a list of projects which are owned by the authenticated user.
```
GET /projects/owned
```
Parameters:
| Attribute | Type | Required | Description |
| --------- | ---- | -------- | ----------- |
| `archived` | boolean | no | Limit by archived status |
| `visibility` | string | no | Limit by visibility `public`, `internal`, or `private` |
| `order_by` | string | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. Default is `created_at` |
| `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` |
| `search` | string | no | Return list of authorized projects matching the search criteria |
| `simple` | boolean | no | Return only the ID, URL, name, and path of each project |
| `statistics` | boolean | no | Include project statistics |
### List starred projects
Get a list of projects which are starred by the authenticated user.
```
GET /projects/starred
```
Parameters:
| Attribute | Type | Required | Description |
| --------- | ---- | -------- | ----------- |
| `archived` | boolean | no | Limit by archived status |
| `visibility` | string | no | Limit by visibility `public`, `internal`, or `private` |
| `order_by` | string | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. Default is `created_at` |
| `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` |
| `search` | string | no | Return list of authorized projects matching the search criteria |
| `simple` | boolean | no | Return only the ID, URL, name, and path of each project |
### List ALL projects
Get a list of all GitLab projects (admin only).
```
GET /projects/all
```
Parameters:
| Attribute | Type | Required | Description |
| --------- | ---- | -------- | ----------- |
| `archived` | boolean | no | Limit by archived status |
| `visibility` | string | no | Limit by visibility `public`, `internal`, or `private` |
| `order_by` | string | no | Return projects ordered by `id`, `name`, `path`, `created_at`, `updated_at`, or `last_activity_at` fields. Default is `created_at` |
| `sort` | string | no | Return projects sorted in `asc` or `desc` order. Default is `desc` |
| `search` | string | no | Return list of authorized projects matching the search criteria |
| `statistics` | boolean | no | Include project statistics |
### Get single project ### Get single project
Get a specific project, identified by project ID or NAMESPACE/PROJECT_NAME, which is owned by the authenticated user. Get a specific project, identified by project ID or NAMESPACE/PROJECT_NAME, which is owned by the authenticated user.
......
...@@ -23,4 +23,4 @@ changes are in V4: ...@@ -23,4 +23,4 @@ changes are in V4:
- `/gitlab_ci_ymls/:key` - `/gitlab_ci_ymls/:key`
- `/dockerfiles/:key` - `/dockerfiles/:key`
- Moved `/projects/fork/:id` to `/projects/:id/fork` - Moved `/projects/fork/:id` to `/projects/:id/fork`
- Endpoints `/projects/owned`, `/projects/visible`, `/projects/starred` & `/projects/all` are consolidated into `/projects` using query parameters
...@@ -143,6 +143,9 @@ module API ...@@ -143,6 +143,9 @@ module API
desc: 'Return projects sorted in ascending and descending order' desc: 'Return projects sorted in ascending and descending order'
optional :simple, type: Boolean, default: false, optional :simple, type: Boolean, default: false,
desc: 'Return only the ID, URL, name, and path of each project' desc: 'Return only the ID, URL, name, and path of each project'
optional :owned, type: Boolean, default: false, desc: 'Limit by owned by authenticated user'
optional :starred, type: Boolean, default: false, desc: 'Limit by starred status'
use :pagination use :pagination
end end
get ":id/projects" do get ":id/projects" do
......
...@@ -256,6 +256,14 @@ module API ...@@ -256,6 +256,14 @@ module API
# project helpers # project helpers
def filter_projects(projects) def filter_projects(projects)
if params[:owned]
projects = projects.merge(current_user.owned_projects)
end
if params[:starred]
projects = projects.merge(current_user.starred_projects)
end
if params[:search].present? if params[:search].present?
projects = projects.search(params[:search]) projects = projects.search(params[:search])
end end
......
...@@ -50,6 +50,8 @@ module API ...@@ -50,6 +50,8 @@ module API
optional :visibility, type: String, values: %w[public internal private], optional :visibility, type: String, values: %w[public internal private],
desc: 'Limit by visibility' desc: 'Limit by visibility'
optional :search, type: String, desc: 'Return list of authorized projects matching the search criteria' optional :search, type: String, desc: 'Return list of authorized projects matching the search criteria'
optional :owned, type: Boolean, default: false, desc: 'Limit by owned by authenticated user'
optional :starred, type: Boolean, default: false, desc: 'Limit by starred status'
end end
params :statistics_params do params :statistics_params do
...@@ -82,62 +84,9 @@ module API ...@@ -82,62 +84,9 @@ module API
params do params do
use :collection_params use :collection_params
end end
get '/visible' do
entity = current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails
present_projects ProjectsFinder.new.execute(current_user), with: entity
end
desc 'Get a projects list for authenticated user' do
success Entities::BasicProjectDetails
end
params do
use :collection_params
end
get do get do
authenticate! entity = current_user ? Entities::ProjectWithAccess : Entities::BasicProjectDetails
present_projects ProjectsFinder.new.execute(current_user), with: entity, statistics: params[:statistics]
present_projects current_user.authorized_projects,
with: Entities::ProjectWithAccess
end
desc 'Get an owned projects list for authenticated user' do
success Entities::BasicProjectDetails
end
params do
use :collection_params
use :statistics_params
end
get '/owned' do
authenticate!
present_projects current_user.owned_projects,
with: Entities::ProjectWithAccess,
statistics: params[:statistics]
end
desc 'Gets starred project for the authenticated user' do
success Entities::BasicProjectDetails
end
params do
use :collection_params
end
get '/starred' do
authenticate!
present_projects current_user.viewable_starred_projects
end
desc 'Get all projects for admin user' do
success Entities::BasicProjectDetails
end
params do
use :collection_params
use :statistics_params
end
get '/all' do
authenticated_as_admin!
present_projects Project.all, with: Entities::ProjectWithAccess, statistics: params[:statistics]
end end
desc 'Create new project' do desc 'Create new project' do
......
...@@ -338,6 +338,26 @@ describe API::Groups, api: true do ...@@ -338,6 +338,26 @@ describe API::Groups, api: true do
expect(json_response.length).to eq(1) expect(json_response.length).to eq(1)
expect(json_response.first['name']).to eq(project3.name) expect(json_response.first['name']).to eq(project3.name)
end end
it 'only returns the projects owned by user' do
project2.group.add_owner(user3)
get api("/groups/#{project2.group.id}/projects", user3), owned: true
expect(response).to have_http_status(200)
expect(json_response.length).to eq(1)
expect(json_response.first['name']).to eq(project2.name)
end
it 'only returns the projects starred by user' do
user1.starred_projects = [project1]
get api("/groups/#{group1.id}/projects", user1), starred: true
expect(response).to have_http_status(200)
expect(json_response.length).to eq(1)
expect(json_response.first['name']).to eq(project1.name)
end
end end
context "when authenticated as admin" do context "when authenticated as admin" do
......
...@@ -41,26 +41,40 @@ describe API::Projects, api: true do ...@@ -41,26 +41,40 @@ describe API::Projects, api: true do
end end
describe 'GET /projects' do describe 'GET /projects' do
before { project } shared_examples_for 'projects response' do
it 'returns an array of projects' do
get api('/projects', current_user)
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response.map { |p| p['id'] }).to contain_exactly(*projects.map(&:id))
end
end
let!(:public_project) { create(:empty_project, :public, name: 'public_project') }
before do
project
project2
project3
project4
end
context 'when unauthenticated' do context 'when unauthenticated' do
it 'returns authentication error' do it_behaves_like 'projects response' do
get api('/projects') let(:current_user) { nil }
expect(response).to have_http_status(401) let(:projects) { [public_project] }
end end
end end
context 'when authenticated as regular user' do context 'when authenticated as regular user' do
it 'returns an array of projects' do it_behaves_like 'projects response' do
get api('/projects', user) let(:current_user) { user }
expect(response).to have_http_status(200) let(:projects) { [public_project, project, project2, project3] }
expect(json_response).to be_an Array
expect(json_response.first['name']).to eq(project.name)
expect(json_response.first['owner']['username']).to eq(user.username)
end end
it 'includes the project labels as the tag_list' do it 'includes the project labels as the tag_list' do
get api('/projects', user) get api('/projects', user)
expect(response.status).to eq 200 expect(response.status).to eq 200
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first.keys).to include('tag_list') expect(json_response.first.keys).to include('tag_list')
...@@ -68,21 +82,39 @@ describe API::Projects, api: true do ...@@ -68,21 +82,39 @@ describe API::Projects, api: true do
it 'includes open_issues_count' do it 'includes open_issues_count' do
get api('/projects', user) get api('/projects', user)
expect(response.status).to eq 200 expect(response.status).to eq 200
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first.keys).to include('open_issues_count') expect(json_response.first.keys).to include('open_issues_count')
end end
it 'does not include open_issues_count' do it 'does not include open_issues_count if issues are disabled' do
project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED) project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED)
get api('/projects', user) get api('/projects', user)
expect(response.status).to eq 200 expect(response.status).to eq 200
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first.keys).not_to include('open_issues_count') expect(json_response.find { |hash| hash['id'] == project.id }.keys).not_to include('open_issues_count')
end
it "does not include statistics by default" do
get api('/projects', user)
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response.first).not_to include('statistics')
end end
context 'GET /projects?simple=true' do it "includes statistics if requested" do
get api('/projects', user), statistics: true
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response.first).to include 'statistics'
end
context 'and with simple=true' do
it 'returns a simplified version of all the projects' do it 'returns a simplified version of all the projects' do
expected_keys = ["id", "http_url_to_repo", "web_url", "name", "name_with_namespace", "path", "path_with_namespace"] expected_keys = ["id", "http_url_to_repo", "web_url", "name", "name_with_namespace", "path", "path_with_namespace"]
...@@ -97,6 +129,7 @@ describe API::Projects, api: true do ...@@ -97,6 +129,7 @@ describe API::Projects, api: true do
context 'and using search' do context 'and using search' do
it 'returns searched project' do it 'returns searched project' do
get api('/projects', user), { search: project.name } get api('/projects', user), { search: project.name }
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.length).to eq(1) expect(json_response.length).to eq(1)
...@@ -106,196 +139,109 @@ describe API::Projects, api: true do ...@@ -106,196 +139,109 @@ describe API::Projects, api: true do
context 'and using the visibility filter' do context 'and using the visibility filter' do
it 'filters based on private visibility param' do it 'filters based on private visibility param' do
get api('/projects', user), { visibility: 'private' } get api('/projects', user), { visibility: 'private' }
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.length).to eq(user.namespace.projects.where(visibility_level: Gitlab::VisibilityLevel::PRIVATE).count) expect(json_response.map { |p| p['id'] }).to contain_exactly(project.id, project2.id, project3.id)
end end
it 'filters based on internal visibility param' do it 'filters based on internal visibility param' do
project2.update_attribute(:visibility_level, Gitlab::VisibilityLevel::INTERNAL)
get api('/projects', user), { visibility: 'internal' } get api('/projects', user), { visibility: 'internal' }
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.length).to eq(user.namespace.projects.where(visibility_level: Gitlab::VisibilityLevel::INTERNAL).count) expect(json_response.map { |p| p['id'] }).to contain_exactly(project2.id)
end end
it 'filters based on public visibility param' do it 'filters based on public visibility param' do
get api('/projects', user), { visibility: 'public' } get api('/projects', user), { visibility: 'public' }
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.length).to eq(user.namespace.projects.where(visibility_level: Gitlab::VisibilityLevel::PUBLIC).count) expect(json_response.map { |p| p['id'] }).to contain_exactly(public_project.id)
end end
end end
context 'and using sorting' do context 'and using sorting' do
before do
project2
project3
end
it 'returns the correct order when sorted by id' do it 'returns the correct order when sorted by id' do
get api('/projects', user), { order_by: 'id', sort: 'desc' } get api('/projects', user), { order_by: 'id', sort: 'desc' }
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first['id']).to eq(project3.id) expect(json_response.first['id']).to eq(project3.id)
end end
end end
end
end
describe 'GET /projects/all' do context 'and with owned=true' do
before { project } it 'returns an array of projects the user owns' do
get api('/projects', user4), owned: true
context 'when unauthenticated' do expect(response).to have_http_status(200)
it 'returns authentication error' do expect(json_response).to be_an Array
get api('/projects/all') expect(json_response.first['name']).to eq(project4.name)
expect(response).to have_http_status(401) expect(json_response.first['owner']['username']).to eq(user4.username)
end
end
context 'when authenticated as regular user' do
it 'returns authentication error' do
get api('/projects/all', user)
expect(response).to have_http_status(403)
end
end
context 'when authenticated as admin' do
it 'returns an array of all projects' do
get api('/projects/all', admin)
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response).to satisfy do |response|
response.one? do |entry|
entry.has_key?('permissions') &&
entry['name'] == project.name &&
entry['owner']['username'] == user.username
end
end end
end end
it "does not include statistics by default" do context 'and with starred=true' do
get api('/projects/all', admin) let(:public_project) { create(:empty_project, :public) }
expect(response).to have_http_status(200) before do
expect(json_response).to be_an Array project_member2
expect(json_response.first).not_to include('statistics') user3.update_attributes(starred_projects: [project, project2, project3, public_project])
end end
it "includes statistics if requested" do
get api('/projects/all', admin), statistics: true
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response.first).to include 'statistics'
end
end
end
describe 'GET /projects/owned' do
before do
project3
project4
end
context 'when unauthenticated' do
it 'returns authentication error' do
get api('/projects/owned')
expect(response).to have_http_status(401)
end
end
context 'when authenticated as project owner' do
it 'returns an array of projects the user owns' do
get api('/projects/owned', user4)
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response.first['name']).to eq(project4.name)
expect(json_response.first['owner']['username']).to eq(user4.username)
end
it "does not include statistics by default" do
get api('/projects/owned', user4)
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response.first).not_to include('statistics')
end
it "includes statistics if requested" do
attributes = {
commit_count: 23,
storage_size: 702,
repository_size: 123,
lfs_objects_size: 234,
build_artifacts_size: 345,
}
project4.statistics.update!(attributes)
get api('/projects/owned', user4), statistics: true it 'returns the starred projects viewable by the user' do
get api('/projects', user3), starred: true
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first['statistics']).to eq attributes.stringify_keys expect(json_response.map { |project| project['id'] }).to contain_exactly(project.id, public_project.id)
end
end end
end
end
describe 'GET /projects/visible' do context 'and with all query parameters' do
shared_examples_for 'visible projects response' do # | | project5 | project6 | project7 | project8 | project9 |
it 'returns the visible projects' do # |---------+----------+----------+----------+----------+----------|
get api('/projects/visible', current_user) # | search | x | | x | x | x |
# | starred | x | x | | x | x |
# | public | x | x | x | | x |
# | owned | x | x | x | x | |
let!(:project5) { create(:empty_project, :public, path: 'gitlab5', namespace: user.namespace) }
let!(:project6) { create(:empty_project, :public, path: 'project6', namespace: user.namespace) }
let!(:project7) { create(:empty_project, :public, path: 'gitlab7', namespace: user.namespace) }
let!(:project8) { create(:empty_project, path: 'gitlab8', namespace: user.namespace) }
let!(:project9) { create(:empty_project, :public, path: 'gitlab9') }
expect(response).to have_http_status(200) before do
expect(json_response).to be_an Array user.update_attributes(starred_projects: [project5, project6, project8, project9])
expect(json_response.map { |p| p['id'] }).to contain_exactly(*projects.map(&:id)) end
end
end
let!(:public_project) { create(:empty_project, :public) } it 'returns only projects that satify all query parameters' do
before do get api('/projects', user), { visibility: 'public', owned: true, starred: true, search: 'gitlab' }
project
project2
project3
project4
end
context 'when unauthenticated' do expect(response).to have_http_status(200)
it_behaves_like 'visible projects response' do expect(json_response).to be_an Array
let(:current_user) { nil } expect(json_response.size).to eq(1)
let(:projects) { [public_project] } expect(json_response.first['id']).to eq(project5.id)
end end
end
context 'when authenticated' do
it_behaves_like 'visible projects response' do
let(:current_user) { user }
let(:projects) { [public_project, project, project2, project3] }
end end
end end
context 'when authenticated as a different user' do context 'when authenticated as a different user' do
it_behaves_like 'visible projects response' do it_behaves_like 'projects response' do
let(:current_user) { user2 } let(:current_user) { user2 }
let(:projects) { [public_project] } let(:projects) { [public_project] }
end end
end end
end
describe 'GET /projects/starred' do
let(:public_project) { create(:empty_project, :public) }
before do
project_member2
user3.update_attributes(starred_projects: [project, project2, project3, public_project])
end
it 'returns the starred projects viewable by the user' do context 'when authenticated as admin' do
get api('/projects/starred', user3) it_behaves_like 'projects response' do
expect(response).to have_http_status(200) let(:current_user) { admin }
expect(json_response).to be_an Array let(:projects) { Project.all }
expect(json_response.map { |project| project['id'] }).to contain_exactly(project.id, public_project.id) end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment