From b9287208523e1a5c05939fe0db038df51a9082fc Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Wed, 30 Aug 2017 16:57:50 +0200 Subject: [PATCH 001/136] Support discussion locking in the backend --- app/controllers/projects/issues_controller.rb | 1 + .../merge_requests/application_controller.rb | 1 + app/controllers/projects/notes_controller.rb | 14 +++++ app/helpers/notes_helper.rb | 4 ++ app/models/system_note_metadata.rb | 2 +- app/policies/issuable_policy.rb | 5 ++ app/policies/note_policy.rb | 10 ++++ app/services/issuable_base_service.rb | 1 + app/services/issues/update_service.rb | 8 +++ app/services/system_note_service.rb | 12 +++++ ...221154_add_dicussion_locked_to_issuable.rb | 16 ++++++ db/schema.rb | 2 + .../projects/notes_controller_spec.rb | 41 ++++++++++++++ .../import_export/safe_model_attributes.yml | 2 + spec/policies/issuable_policy_spec.rb | 28 ++++++++++ spec/policies/note_policy_spec.rb | 54 +++++++++++++++++++ spec/services/issues/update_service_spec.rb | 5 +- .../merge_requests/update_service_spec.rb | 4 +- 18 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb create mode 100644 spec/policies/issuable_policy_spec.rb create mode 100644 spec/policies/note_policy_spec.rb diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 8990c919ca0..ab75a68e56a 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -289,6 +289,7 @@ class Projects::IssuesController < Projects::ApplicationController state_event task_num lock_version + discussion_locked ] + [{ label_ids: [], assignee_ids: [] }] end diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index 6602b204fcb..eb7d7bf374c 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -34,6 +34,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont :target_project_id, :task_num, :title, + :discussion_locked, label_ids: [] ] diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 41a13f6f577..dd3dc71c004 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -66,7 +66,21 @@ class Projects::NotesController < Projects::ApplicationController params.merge(last_fetched_at: last_fetched_at) end + def authorize_admin_note! + return access_denied! unless can?(current_user, :admin_note, note) + end + def authorize_resolve_note! return access_denied! unless can?(current_user, :resolve_note, note) end + + def authorize_create_note! + noteable_type = note_params[:noteable_type] + + return unless ['MergeRequest', 'Issue'].include?(noteable_type) + return access_denied! unless can?(current_user, :create_note, project) + + noteable = noteable_type.constantize.find(note_params[:noteable_id]) + access_denied! unless can?(current_user, :create_note, noteable) + end end diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index ce028195e51..c219aa3d6a9 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -130,8 +130,12 @@ module NotesHelper end def can_create_note? + issuable = @issue || @merge_request + if @snippet.is_a?(PersonalSnippet) can?(current_user, :comment_personal_snippet, @snippet) + elsif issuable + can?(current_user, :create_note, issuable) else can?(current_user, :create_note, @project) end diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 0b33e45473b..1f9f8d7286b 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -2,7 +2,7 @@ class SystemNoteMetadata < ActiveRecord::Base ICON_TYPES = %w[ commit description merge confidential visible label assignee cross_reference title time_tracking branch milestone discussion task moved - opened closed merged duplicate + opened closed merged duplicate locked unlocked outdated ].freeze diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index daf6fa9e18a..212f4989557 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -1,6 +1,9 @@ class IssuablePolicy < BasePolicy delegate { @subject.project } + condition(:locked) { @subject.discussion_locked? } + condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) } + desc "User is the assignee or author" condition(:assignee_or_author) do @user && @subject.assignee_or_author?(@user) @@ -12,4 +15,6 @@ class IssuablePolicy < BasePolicy enable :read_merge_request enable :update_merge_request end + + rule { locked & ~is_project_member }.prevent :create_note end diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index 20cd51cfb99..5d51fbf4f4a 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -2,14 +2,18 @@ class NotePolicy < BasePolicy delegate { @subject.project } condition(:is_author) { @user && @subject.author == @user } + condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) } condition(:for_merge_request, scope: :subject) { @subject.for_merge_request? } condition(:is_noteable_author) { @user && @subject.noteable.author_id == @user.id } condition(:editable, scope: :subject) { @subject.editable? } + condition(:locked) { @subject.noteable.discussion_locked? } rule { ~editable | anonymous }.prevent :edit_note + rule { is_author | admin }.enable :edit_note rule { can?(:master_access) }.enable :edit_note + rule { locked & ~is_author & ~is_project_member }.prevent :edit_note rule { is_author }.policy do enable :read_note @@ -21,4 +25,10 @@ class NotePolicy < BasePolicy rule { for_merge_request & is_noteable_author }.policy do enable :resolve_note end + + rule { locked & ~is_project_member }.policy do + prevent :update_note + prevent :admin_note + prevent :resolve_note + end end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 8b967b78052..40793201664 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -57,6 +57,7 @@ class IssuableBaseService < BaseService params.delete(:due_date) params.delete(:canonical_issue_id) params.delete(:project) + params.delete(:discussion_locked) end filter_assignee(issuable) diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index b4ca3966505..2a24ee85c45 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -41,6 +41,10 @@ module Issues create_confidentiality_note(issue) end + if issue.previous_changes.include?('discussion_locked') + create_discussion_lock_note(issue) + end + added_labels = issue.labels - old_labels if added_labels.present? @@ -95,5 +99,9 @@ module Issues def create_confidentiality_note(issue) SystemNoteService.change_issue_confidentiality(issue, issue.project, current_user) end + + def create_discussion_lock_note(issue) + SystemNoteService.discussion_lock(issue, current_user) + end end end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 1f66a2668f9..cec0a1b6efa 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -591,6 +591,18 @@ module SystemNoteService create_note(NoteSummary.new(noteable, project, author, body, action: 'duplicate')) end + def discussion_lock(issuable, author) + if issuable.discussion_locked + body = 'locked this issue' + action = 'locked' + else + body = 'unlocked this issue' + action = 'unlocked' + end + + create_note(NoteSummary.new(issuable, issuable.project, author, body, action: action)) + end + private def notes_for_mentioner(mentioner, noteable, notes) diff --git a/db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb b/db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb new file mode 100644 index 00000000000..bb60ac2a410 --- /dev/null +++ b/db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb @@ -0,0 +1,16 @@ +class AddDicussionLockedToIssuable < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + disable_ddl_transaction! + + def up + add_column(:merge_requests, :discussion_locked, :boolean) + add_column(:issues, :discussion_locked, :boolean) + end + + def down + remove_column(:merge_requests, :discussion_locked) + remove_column(:issues, :discussion_locked) + end +end diff --git a/db/schema.rb b/db/schema.rb index 2149f5ad23d..16f38f7b60b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -660,6 +660,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do t.integer "cached_markdown_version" t.datetime "last_edited_at" t.integer "last_edited_by_id" + t.boolean "discussion_locked", default: false, null: false end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -882,6 +883,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do t.integer "head_pipeline_id" t.boolean "ref_fetched" t.string "merge_jid" + t.boolean "discussion_locked", default: false, null: false end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 6ffe41b8608..26429b57bd5 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -232,6 +232,47 @@ describe Projects::NotesController do end end end + + context 'when the merge request discussion is locked' do + before do + project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) + merge_request.update_attribute(:discussion_locked, true) + end + + context 'when a user is a team member' do + it 'returns 302 status for html' do + post :create, request_params + + expect(response).to have_http_status(302) + end + + it 'returns 200 status for json' do + post :create, request_params.merge(format: :json) + + expect(response).to have_http_status(200) + end + + it 'creates a new note' do + expect{ post :create, request_params }.to change { Note.count }.by(1) + end + end + + context 'when a user is not a team member' do + before do + project.project_member(user).destroy + end + + it 'returns 404 status' do + post :create, request_params + + expect(response).to have_http_status(404) + end + + it 'does not create a new note' do + expect{ post :create, request_params }.not_to change { Note.count } + end + end + end end describe 'DELETE destroy' do diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 899d17d97c2..763ab029051 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -25,6 +25,7 @@ Issue: - relative_position - last_edited_at - last_edited_by_id +- discussion_locked Event: - id - target_type @@ -168,6 +169,7 @@ MergeRequest: - last_edited_at - last_edited_by_id - head_pipeline_id +- discussion_locked MergeRequestDiff: - id - state diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb new file mode 100644 index 00000000000..9b399d764ea --- /dev/null +++ b/spec/policies/issuable_policy_spec.rb @@ -0,0 +1,28 @@ +require 'spec_helper' + +describe IssuablePolicy, models: true do + describe '#rules' do + context 'when discussion is locked for the issuable' do + let(:user) { create(:user) } + let(:project) { create(:project, :public) } + let(:issue) { create(:issue, project: project, discussion_locked: true) } + let(:policies) { described_class.new(user, issue) } + + context 'when the user is not a project member' do + it 'can not create a note' do + expect(policies).to be_disallowed(:create_note) + end + end + + context 'when the user is a project member' do + before do + project.team << [user, :guest] + end + + it 'can create a note' do + expect(policies).to be_allowed(:create_note) + end + end + end + end +end diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb new file mode 100644 index 00000000000..70a99ed0198 --- /dev/null +++ b/spec/policies/note_policy_spec.rb @@ -0,0 +1,54 @@ +require 'spec_helper' + +describe NotePolicy, mdoels: true do + describe '#rules' do + let(:user) { create(:user) } + let(:project) { create(:project, :public) } + let(:issue) { create(:issue, project: project) } + let(:note) { create(:note, noteable: issue, author: user, project: project) } + + let(:policies) { described_class.new(user, note) } + + context 'when the project is public' do + context 'when the note author is not a project member' do + it 'can edit a note' do + expect(policies).to be_allowed(:update_note) + expect(policies).to be_allowed(:admin_note) + expect(policies).to be_allowed(:resolve_note) + expect(policies).to be_allowed(:read_note) + end + end + + context 'when a discussion is locked' do + before do + issue.update_attribute(:discussion_locked, true) + end + + context 'when the note author is a project member' do + before do + project.add_developer(user) + end + + it 'can eddit a note' do + expect(policies).to be_allowed(:update_note) + expect(policies).to be_allowed(:admin_note) + expect(policies).to be_allowed(:resolve_note) + expect(policies).to be_allowed(:read_note) + end + end + + context 'when the note author is not a project member' do + it 'can not edit a note' do + expect(policies).to be_disallowed(:update_note) + expect(policies).to be_disallowed(:admin_note) + expect(policies).to be_disallowed(:resolve_note) + end + + it 'can read a note' do + expect(policies).to be_allowed(:read_note) + end + end + end + end + end +end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 15a50b85f19..0ed4c2152bb 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -48,7 +48,8 @@ describe Issues::UpdateService, :mailer do assignee_ids: [user2.id], state_event: 'close', label_ids: [label.id], - due_date: Date.tomorrow + due_date: Date.tomorrow, + discussion_locked: true } end @@ -62,6 +63,7 @@ describe Issues::UpdateService, :mailer do expect(issue).to be_closed expect(issue.labels).to match_array [label] expect(issue.due_date).to eq Date.tomorrow + expect(issue.discussion_locked).to be_truthy end it 'updates open issue counter for assignees when issue is reassigned' do @@ -103,6 +105,7 @@ describe Issues::UpdateService, :mailer do expect(issue.labels).to be_empty expect(issue.milestone).to be_nil expect(issue.due_date).to be_nil + expect(issue.discussion_locked).to be_falsey end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 681feee61d1..a07b7ac2218 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -49,7 +49,8 @@ describe MergeRequests::UpdateService, :mailer do state_event: 'close', label_ids: [label.id], target_branch: 'target', - force_remove_source_branch: '1' + force_remove_source_branch: '1', + discussion_locked: true } end @@ -73,6 +74,7 @@ describe MergeRequests::UpdateService, :mailer do expect(@merge_request.labels.first.title).to eq(label.name) expect(@merge_request.target_branch).to eq('target') expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') + expect(@merge_request.discussion_locked).to be_truthy end it 'executes hooks with update action' do From 073ba05d315881730de3995042cc4256c116e2c4 Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Thu, 31 Aug 2017 12:38:32 +0200 Subject: [PATCH 002/136] Support discussion lock in the API --- doc/api/issues.md | 57 +++++++++++++++++-- doc/api/merge_requests.md | 11 +++- lib/api/entities.rb | 2 + lib/api/issues.rb | 3 +- lib/api/merge_requests.rb | 4 +- lib/api/notes.rb | 7 +++ .../api/schemas/public_api/v4/issues.json | 1 + .../schemas/public_api/v4/merge_requests.json | 1 + 8 files changed, 79 insertions(+), 7 deletions(-) diff --git a/doc/api/issues.md b/doc/api/issues.md index 8ca66049d31..61e42345153 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -109,7 +109,8 @@ Example response: "human_time_estimate": null, "human_total_time_spent": null }, - "confidential": false + "confidential": false, + "discussion_locked": false } ] ``` @@ -214,7 +215,8 @@ Example response: "human_time_estimate": null, "human_total_time_spent": null }, - "confidential": false + "confidential": false, + "discussion_locked": false } ] ``` @@ -320,7 +322,8 @@ Example response: "human_time_estimate": null, "human_total_time_spent": null }, - "confidential": false + "confidential": false, + "discussion_locked": false } ] ``` @@ -403,6 +406,7 @@ Example response: "human_total_time_spent": null }, "confidential": false, + "discussion_locked": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", "notes": "http://example.com/api/v4/projects/1/issues/2/notes", @@ -477,6 +481,7 @@ Example response: "human_total_time_spent": null }, "confidential": false, + "discussion_locked": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", "notes": "http://example.com/api/v4/projects/1/issues/2/notes", @@ -510,6 +515,8 @@ PUT /projects/:id/issues/:issue_iid | `state_event` | string | no | The state event of an issue. Set `close` to close the issue and `reopen` to reopen it | | `updated_at` | string | no | Date time string, ISO 8601 formatted, e.g. `2016-03-11T03:45:40Z` (requires admin or project owner rights) | | `due_date` | string | no | Date time string in the format YEAR-MONTH-DAY, e.g. `2016-03-11` | +| `discussion_locked` | boolean | no | Updates an issue to lock or unlock its discussion | + ```bash curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/4/issues/85?state_event=close @@ -552,6 +559,7 @@ Example response: "human_total_time_spent": null }, "confidential": false, + "discussion_locked": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", "notes": "http://example.com/api/v4/projects/1/issues/2/notes", @@ -650,6 +658,7 @@ Example response: "human_total_time_spent": null }, "confidential": false, + "discussion_locked": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", "notes": "http://example.com/api/v4/projects/1/issues/2/notes", @@ -727,6 +736,7 @@ Example response: "human_total_time_spent": null }, "confidential": false, + "discussion_locked": false, "_links": { "self": "http://example.com/api/v4/projects/1/issues/2", "notes": "http://example.com/api/v4/projects/1/issues/2/notes", @@ -757,6 +767,44 @@ POST /projects/:id/issues/:issue_iid/unsubscribe curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/projects/5/issues/93/unsubscribe ``` +Example response: + +```json +{ + "id": 93, + "iid": 12, + "project_id": 5, + "title": "Incidunt et rerum ea expedita iure quibusdam.", + "description": "Et cumque architecto sed aut ipsam.", + "state": "opened", + "created_at": "2016-04-05T21:41:45.217Z", + "updated_at": "2016-04-07T13:02:37.905Z", + "labels": [], + "milestone": null, + "assignee": { + "name": "Edwardo Grady", + "username": "keyon", + "id": 21, + "state": "active", + "avatar_url": "http://www.gravatar.com/avatar/3e6f06a86cf27fa8b56f3f74f7615987?s=80&d=identicon", + "web_url": "https://gitlab.example.com/keyon" + }, + "author": { + "name": "Vivian Hermann", + "username": "orville", + "id": 11, + "state": "active", + "avatar_url": "http://www.gravatar.com/avatar/5224fd70153710e92fb8bcf79ac29d67?s=80&d=identicon", + "web_url": "https://gitlab.example.com/orville" + }, + "subscribed": false, + "due_date": null, + "web_url": "http://example.com/example/example/issues/12", + "confidential": false, + "discussion_locked": false +} +``` + ## Create a todo Manually creates a todo for the current user on an issue. If @@ -849,7 +897,8 @@ Example response: "downvotes": 0, "due_date": null, "web_url": "http://example.com/example/example/issues/110", - "confidential": false + "confidential": false, + "discussion_locked": false }, "target_url": "https://gitlab.example.com/gitlab-org/gitlab-ci/issues/10", "body": "Vel voluptas atque dicta mollitia adipisci qui at.", diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index bff8a2d3e4d..e5d1ebb9cfb 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -192,6 +192,7 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -267,6 +268,7 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -378,6 +380,7 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -471,6 +474,7 @@ POST /projects/:id/merge_requests "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -500,6 +504,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid | `labels` | string | no | Labels for MR as a comma-separated list | | `milestone_id` | integer | no | The ID of a milestone | | `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging | +| `discussion_locked` | boolean | no | Flag indicating if the merge request's discussion is locked | Must include at least one non-required attribute from above. @@ -554,6 +559,7 @@ Must include at least one non-required attribute from above. "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -658,6 +664,7 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -734,6 +741,7 @@ Parameters: "should_remove_source_branch": true, "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", + "discussion_locked": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -1028,7 +1036,8 @@ Example response: "id": 14, "state": "active", "avatar_url": "http://www.gravatar.com/avatar/a7fa515d53450023c83d62986d0658a8?s=80&d=identicon", - "web_url": "https://gitlab.example.com/francisca" + "web_url": "https://gitlab.example.com/francisca", + "discussion_locked": false }, "assignee": { "name": "Dr. Gabrielle Strosin", diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 52c49e5caa9..4b2ac1cce95 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -362,6 +362,7 @@ module API end expose :due_date expose :confidential + expose :discussion_locked expose :web_url do |issue, options| Gitlab::UrlBuilder.build(issue) @@ -458,6 +459,7 @@ module API expose :diff_head_sha, as: :sha expose :merge_commit_sha expose :user_notes_count + expose :discussion_locked expose :should_remove_source_branch?, as: :should_remove_source_branch expose :force_remove_source_branch?, as: :force_remove_source_branch diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 1729df2aad0..88b592083db 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -48,6 +48,7 @@ module API optional :labels, type: String, desc: 'Comma-separated list of label names' optional :due_date, type: String, desc: 'Date string in the format YEAR-MONTH-DAY' optional :confidential, type: Boolean, desc: 'Boolean parameter if the issue should be confidential' + optional :discussion_locked, type: Boolean, desc: "Boolean parameter if the issue's discussion should be locked" end params :issue_params do @@ -193,7 +194,7 @@ module API desc: 'Date time when the issue was updated. Available only for admins and project owners.' optional :state_event, type: String, values: %w[reopen close], desc: 'State of the issue' use :issue_params - at_least_one_of :title, :description, :assignee_ids, :assignee_id, :milestone_id, + at_least_one_of :title, :description, :assignee_ids, :assignee_id, :milestone_id, :discussion_locked, :labels, :created_at, :due_date, :confidential, :state_event end put ':id/issues/:issue_iid' do diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 56d72d511da..35395647fac 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -213,12 +213,14 @@ module API :remove_source_branch, :state_event, :target_branch, - :title + :title, + :discussion_locked ] optional :title, type: String, allow_blank: false, desc: 'The title of the merge request' optional :target_branch, type: String, allow_blank: false, desc: 'The target branch' optional :state_event, type: String, values: %w[close reopen], desc: 'Status of the merge request' + optional :discussion_locked, type: Boolean, desc: 'Whether the MR discussion is locked' use :optional_params at_least_one_of(*at_least_one_of_ce) diff --git a/lib/api/notes.rb b/lib/api/notes.rb index d6e7203adaf..b3db366d875 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -71,6 +71,8 @@ module API post ":id/#{noteables_str}/:noteable_id/notes" do noteable = find_project_noteable(noteables_str, params[:noteable_id]) + authorize! :create_note, user_project + opts = { note: params[:body], noteable_type: noteables_str.classify, @@ -82,6 +84,11 @@ module API opts[:created_at] = params[:created_at] end + noteable_type = opts[:noteable_type].to_s + noteable = Issue.find(opts[:noteable_id]) if noteable_type == 'Issue' + noteable = MergeRequest.find(opts[:noteable_id]) if noteable_type == 'MergeRequest' + authorize! :create_note, noteable if noteable + note = ::Notes::CreateService.new(user_project, current_user, opts).execute if note.valid? diff --git a/spec/fixtures/api/schemas/public_api/v4/issues.json b/spec/fixtures/api/schemas/public_api/v4/issues.json index 8acd9488215..8c854a43fc6 100644 --- a/spec/fixtures/api/schemas/public_api/v4/issues.json +++ b/spec/fixtures/api/schemas/public_api/v4/issues.json @@ -9,6 +9,7 @@ "title": { "type": "string" }, "description": { "type": ["string", "null"] }, "state": { "type": "string" }, + "discussion_locked": { "type": "boolean" }, "created_at": { "type": "date" }, "updated_at": { "type": "date" }, "labels": { diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json index 31b3f4ba946..3b42333bb10 100644 --- a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json +++ b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json @@ -72,6 +72,7 @@ "user_notes_count": { "type": "integer" }, "should_remove_source_branch": { "type": ["boolean", "null"] }, "force_remove_source_branch": { "type": ["boolean", "null"] }, + "discussion_locked": { "type": "boolean" }, "web_url": { "type": "uri" }, "time_stats": { "time_estimate": { "type": "integer" }, From 3d2917bf2e4799a7ba9bcb518c39605eca0a4b1d Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Thu, 31 Aug 2017 12:38:43 +0200 Subject: [PATCH 003/136] Add the possibility to lock issuables from the frontend --- app/assets/javascripts/discussion_lock.js | 46 +++++++++++++++++++ .../javascripts/init_issuable_sidebar.js | 1 + app/assets/javascripts/main.js | 1 + app/assets/stylesheets/pages/notes.scss | 6 +++ app/views/shared/issuable/_sidebar.html.haml | 11 +++++ .../shared/notes/_notes_with_form.html.haml | 10 +++- 6 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 app/assets/javascripts/discussion_lock.js diff --git a/app/assets/javascripts/discussion_lock.js b/app/assets/javascripts/discussion_lock.js new file mode 100644 index 00000000000..7bf83e9b8a0 --- /dev/null +++ b/app/assets/javascripts/discussion_lock.js @@ -0,0 +1,46 @@ +class DiscussionLock { + constructor(containerElm) { + this.containerElm = containerElm; + + const lockButton = containerElm.querySelector('.js-discussion-lock-button'); + console.log(lockButton); + if (lockButton) { + // remove class so we don't bind twice + lockButton.classList.remove('js-discussion-lock-button'); + console.log(lockButton); + lockButton.addEventListener('click', this.toggleDiscussionLock.bind(this)); + } + } + + toggleDiscussionLock(event) { + const button = event.currentTarget; + const buttonSpan = button.querySelector('span'); + if (!buttonSpan || button.classList.contains('disabled')) { + return; + } + button.classList.add('disabled'); + + const url = this.containerElm.dataset.url; + const lock = this.containerElm.dataset.lock; + const issuableType = this.containerElm.dataset.issuableType; + + const data = {} + data[issuableType] = {} + data[issuableType].discussion_locked = lock + + $.ajax({ + url, + data: data, + type: 'PUT' + }).done((data) => { + button.classList.remove('disabled'); + }); + } + + static bindAll(selector) { + [].forEach.call(document.querySelectorAll(selector), elm => new DiscussionLock(elm)); + } +} + +window.gl = window.gl || {}; +window.gl.DiscussionLock = DiscussionLock; diff --git a/app/assets/javascripts/init_issuable_sidebar.js b/app/assets/javascripts/init_issuable_sidebar.js index 29e3d2ea94e..8857601f530 100644 --- a/app/assets/javascripts/init_issuable_sidebar.js +++ b/app/assets/javascripts/init_issuable_sidebar.js @@ -13,6 +13,7 @@ export default () => { new LabelsSelect(); new IssuableContext(sidebarOptions.currentUser); gl.Subscription.bindAll('.subscription'); + gl.DiscussionLock.bindAll('.discussion-lock'); new gl.DueDateSelectors(); window.sidebar = new Sidebar(); }; diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index 0bc31a56684..ea1d50de965 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -80,6 +80,7 @@ import './copy_as_gfm'; import './copy_to_clipboard'; import './create_label'; import './diff'; +import './discussion_lock'; import './dropzone_input'; import './due_date_select'; import './files_comment_button'; diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index e437bad4912..94439ed94a6 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -703,6 +703,12 @@ ul.notes { color: $note-disabled-comment-color; padding: 90px 0; + &.discussion-locked { + border: none; + background-color: $white-light; + } + + a { color: $gl-link-color; } diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 9cae3f51825..38a54c232d0 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -131,6 +131,17 @@ %button.btn.btn-default.pull-right.js-subscribe-button.issuable-subscribe-button.hide-collapsed{ type: "button" } %span= subscribed ? 'Unsubscribe' : 'Subscribe' + - if can_edit_issuable + - locked = issuable.discussion_locked? + .block.light.discussion-lock{ data: { lock: (!locked).to_s, url: "#{issuable_json_path(issuable)}?basic=true", issuable_type: issuable.class.to_s.underscore } } + .sidebar-collapsed-icon + = icon('rss', 'aria-hidden': 'true') + %span.issuable-header-text.hide-collapsed.pull-left + Discussion Lock + - subscribtion_status = locked ? 'locked' : 'not locked' + %button.btn.btn-default.pull-right.js-discussion-lock-button.issuable-discussion-lock-button.hide-collapsed{ type: "button" } + %span= locked ? 'Unlock' : 'Lock' + - project_ref = cross_project_reference(@project, issuable) .block.project-reference .sidebar-collapsed-icon.dont-change-state diff --git a/app/views/shared/notes/_notes_with_form.html.haml b/app/views/shared/notes/_notes_with_form.html.haml index e3e86709b8f..371a9523de9 100644 --- a/app/views/shared/notes/_notes_with_form.html.haml +++ b/app/views/shared/notes/_notes_with_form.html.haml @@ -1,3 +1,6 @@ +- issuable = @issue || @merge_request +- discussion_locked = issuable&.discussion_locked? + %ul#notes-list.notes.main-notes-list.timeline = render "shared/notes/notes" @@ -21,5 +24,10 @@ or = link_to "sign in", new_session_path(:user, redirect_to_referer: 'yes'), class: 'js-sign-in-link' to comment - +- elsif discussion_locked + .discussion_locked + %span + This + = issuable.class.to_s + has been locked. Posting comments has been restricted to project members. %script.js-notes-data{ type: "application/json" }= initial_notes_data(autocomplete).to_json.html_safe From 2b82f907abf2074ac332531d6142893d081f44b9 Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Thu, 31 Aug 2017 14:31:14 +0200 Subject: [PATCH 004/136] Check the discussion lock only for issuables & clean style --- app/controllers/projects/notes_controller.rb | 2 +- app/policies/note_policy.rb | 2 +- spec/controllers/projects/notes_controller_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index dd3dc71c004..bb0c1869955 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -77,7 +77,7 @@ class Projects::NotesController < Projects::ApplicationController def authorize_create_note! noteable_type = note_params[:noteable_type] - return unless ['MergeRequest', 'Issue'].include?(noteable_type) + return unless %w[MergeRequest Issue].include?(noteable_type) return access_denied! unless can?(current_user, :create_note, project) noteable = noteable_type.constantize.find(note_params[:noteable_id]) diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index 5d51fbf4f4a..307c514a74b 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -7,7 +7,7 @@ class NotePolicy < BasePolicy condition(:is_noteable_author) { @user && @subject.noteable.author_id == @user.id } condition(:editable, scope: :subject) { @subject.editable? } - condition(:locked) { @subject.noteable.discussion_locked? } + condition(:locked) { [MergeRequest, Issue].include?(@subject.noteable.class) && @subject.noteable.discussion_locked? } rule { ~editable | anonymous }.prevent :edit_note diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 26429b57bd5..10edad462c1 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -253,7 +253,7 @@ describe Projects::NotesController do end it 'creates a new note' do - expect{ post :create, request_params }.to change { Note.count }.by(1) + expect { post :create, request_params }.to change { Note.count }.by(1) end end @@ -269,7 +269,7 @@ describe Projects::NotesController do end it 'does not create a new note' do - expect{ post :create, request_params }.not_to change { Note.count } + expect { post :create, request_params }.not_to change { Note.count } end end end From 994e7d135947ca162c147c5e0992a0190de22808 Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Fri, 1 Sep 2017 14:03:57 +0200 Subject: [PATCH 005/136] Create system notes for MR too, improve doc + clean up code --- app/controllers/projects/notes_controller.rb | 7 +--- app/models/concerns/noteable.rb | 4 +++ app/policies/issuable_policy.rb | 11 ++++-- app/policies/note_policy.rb | 10 +----- app/services/issuable_base_service.rb | 11 ++++++ app/services/issues/update_service.rb | 8 ----- app/services/system_note_service.rb | 9 ++--- changelogs/unreleased/18608-lock-issues.yml | 5 +++ ...1154_add_discussion_locked_to_issuable.rb} | 5 +-- db/schema.rb | 4 +-- doc/api/issues.md | 2 +- doc/api/merge_requests.md | 2 +- lib/api/issues.rb | 2 +- lib/api/notes.rb | 9 ++--- .../projects/notes_controller_spec.rb | 9 +++++ .../api/schemas/public_api/v4/issues.json | 2 +- .../schemas/public_api/v4/merge_requests.json | 2 +- spec/policies/issuable_policy_spec.rb | 2 +- spec/policies/note_policy_spec.rb | 23 +++++++++++-- spec/requests/api/notes_spec.rb | 34 +++++++++++++++++++ spec/services/issues/update_service_spec.rb | 7 ++++ .../merge_requests/update_service_spec.rb | 7 ++++ 22 files changed, 121 insertions(+), 54 deletions(-) create mode 100644 changelogs/unreleased/18608-lock-issues.yml rename db/migrate/{20161207221154_add_dicussion_locked_to_issuable.rb => 20170815221154_add_discussion_locked_to_issuable.rb} (67%) diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index bb0c1869955..ef7d047b1ad 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -75,12 +75,7 @@ class Projects::NotesController < Projects::ApplicationController end def authorize_create_note! - noteable_type = note_params[:noteable_type] - - return unless %w[MergeRequest Issue].include?(noteable_type) - return access_denied! unless can?(current_user, :create_note, project) - - noteable = noteable_type.constantize.find(note_params[:noteable_id]) + return unless noteable.lockable? access_denied! unless can?(current_user, :create_note, noteable) end end diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index 1c4ddabcad5..5d75b2aa6a3 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -74,4 +74,8 @@ module Noteable def discussions_can_be_resolved_by?(user) discussions_to_be_resolved.all? { |discussion| discussion.can_resolve?(user) } end + + def lockable? + [MergeRequest, Issue].include?(self.class) + end end diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index 212f4989557..f0aa16d2ecf 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -1,7 +1,8 @@ class IssuablePolicy < BasePolicy delegate { @subject.project } - condition(:locked) { @subject.discussion_locked? } + condition(:locked, scope: :subject, score: 0) { @subject.discussion_locked? } + condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) } desc "User is the assignee or author" @@ -16,5 +17,11 @@ class IssuablePolicy < BasePolicy enable :update_merge_request end - rule { locked & ~is_project_member }.prevent :create_note + rule { locked & ~is_project_member }.policy do + prevent :create_note + prevent :update_note + prevent :admin_note + prevent :resolve_note + prevent :edit_note + end end diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index 307c514a74b..d4cb5a77e63 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -1,19 +1,17 @@ class NotePolicy < BasePolicy delegate { @subject.project } + delegate { @subject.noteable if @subject.noteable.lockable? } condition(:is_author) { @user && @subject.author == @user } - condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) } condition(:for_merge_request, scope: :subject) { @subject.for_merge_request? } condition(:is_noteable_author) { @user && @subject.noteable.author_id == @user.id } condition(:editable, scope: :subject) { @subject.editable? } - condition(:locked) { [MergeRequest, Issue].include?(@subject.noteable.class) && @subject.noteable.discussion_locked? } rule { ~editable | anonymous }.prevent :edit_note rule { is_author | admin }.enable :edit_note rule { can?(:master_access) }.enable :edit_note - rule { locked & ~is_author & ~is_project_member }.prevent :edit_note rule { is_author }.policy do enable :read_note @@ -25,10 +23,4 @@ class NotePolicy < BasePolicy rule { for_merge_request & is_noteable_author }.policy do enable :resolve_note end - - rule { locked & ~is_project_member }.policy do - prevent :update_note - prevent :admin_note - prevent :resolve_note - end end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 40793201664..157539ee05b 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -43,6 +43,10 @@ class IssuableBaseService < BaseService SystemNoteService.change_time_spent(issuable, issuable.project, current_user) end + def create_discussion_lock_note(issuable) + SystemNoteService.discussion_lock(issuable, current_user) + end + def filter_params(issuable) ability_name = :"admin_#{issuable.to_ability_name}" @@ -236,6 +240,7 @@ class IssuableBaseService < BaseService handle_common_system_notes(issuable, old_labels: old_labels) end + change_discussion_lock(issuable) handle_changes( issuable, old_labels: old_labels, @@ -292,6 +297,12 @@ class IssuableBaseService < BaseService end end + def change_discussion_lock(issuable) + if issuable.previous_changes.include?('discussion_locked') + create_discussion_lock_note(issuable) + end + end + def toggle_award(issuable) award = params.delete(:emoji_award) if award diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 2a24ee85c45..b4ca3966505 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -41,10 +41,6 @@ module Issues create_confidentiality_note(issue) end - if issue.previous_changes.include?('discussion_locked') - create_discussion_lock_note(issue) - end - added_labels = issue.labels - old_labels if added_labels.present? @@ -99,9 +95,5 @@ module Issues def create_confidentiality_note(issue) SystemNoteService.change_issue_confidentiality(issue, issue.project, current_user) end - - def create_discussion_lock_note(issue) - SystemNoteService.discussion_lock(issue, current_user) - end end end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index cec0a1b6efa..7b32e215c7f 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -592,13 +592,8 @@ module SystemNoteService end def discussion_lock(issuable, author) - if issuable.discussion_locked - body = 'locked this issue' - action = 'locked' - else - body = 'unlocked this issue' - action = 'unlocked' - end + action = issuable.discussion_locked? ? 'locked' : 'unlocked' + body = "#{action} this issue" create_note(NoteSummary.new(issuable, issuable.project, author, body, action: action)) end diff --git a/changelogs/unreleased/18608-lock-issues.yml b/changelogs/unreleased/18608-lock-issues.yml new file mode 100644 index 00000000000..6c0ae7cebf5 --- /dev/null +++ b/changelogs/unreleased/18608-lock-issues.yml @@ -0,0 +1,5 @@ +--- +title: Create system notes for MR too, improve doc + clean up code +merge_request: +author: +type: added diff --git a/db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb b/db/migrate/20170815221154_add_discussion_locked_to_issuable.rb similarity index 67% rename from db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb rename to db/migrate/20170815221154_add_discussion_locked_to_issuable.rb index bb60ac2a410..5bd777c53a0 100644 --- a/db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb +++ b/db/migrate/20170815221154_add_discussion_locked_to_issuable.rb @@ -1,8 +1,5 @@ -class AddDicussionLockedToIssuable < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - +class AddDiscussionLockedToIssuable < ActiveRecord::Migration DOWNTIME = false - disable_ddl_transaction! def up add_column(:merge_requests, :discussion_locked, :boolean) diff --git a/db/schema.rb b/db/schema.rb index 16f38f7b60b..6cdf929b1b6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -660,7 +660,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do t.integer "cached_markdown_version" t.datetime "last_edited_at" t.integer "last_edited_by_id" - t.boolean "discussion_locked", default: false, null: false + t.boolean "discussion_locked" end add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree @@ -883,7 +883,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do t.integer "head_pipeline_id" t.boolean "ref_fetched" t.string "merge_jid" - t.boolean "discussion_locked", default: false, null: false + t.boolean "discussion_locked" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/doc/api/issues.md b/doc/api/issues.md index 61e42345153..479f8754bcc 100644 --- a/doc/api/issues.md +++ b/doc/api/issues.md @@ -515,7 +515,7 @@ PUT /projects/:id/issues/:issue_iid | `state_event` | string | no | The state event of an issue. Set `close` to close the issue and `reopen` to reopen it | | `updated_at` | string | no | Date time string, ISO 8601 formatted, e.g. `2016-03-11T03:45:40Z` (requires admin or project owner rights) | | `due_date` | string | no | Date time string in the format YEAR-MONTH-DAY, e.g. `2016-03-11` | -| `discussion_locked` | boolean | no | Updates an issue to lock or unlock its discussion | +| `discussion_locked` | boolean | no | Flag indicating if the issue's discussion is locked. If the discussion is locked only project members can add or edit comments. | ```bash diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index e5d1ebb9cfb..64daed7c326 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -504,7 +504,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid | `labels` | string | no | Labels for MR as a comma-separated list | | `milestone_id` | integer | no | The ID of a milestone | | `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging | -| `discussion_locked` | boolean | no | Flag indicating if the merge request's discussion is locked | +| `discussion_locked` | boolean | no | Flag indicating if the merge request's discussion is locked. If the discussion is locked only project members can add, edit or resolve comments. | Must include at least one non-required attribute from above. diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 88b592083db..0df41dcc903 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -48,7 +48,7 @@ module API optional :labels, type: String, desc: 'Comma-separated list of label names' optional :due_date, type: String, desc: 'Date string in the format YEAR-MONTH-DAY' optional :confidential, type: Boolean, desc: 'Boolean parameter if the issue should be confidential' - optional :discussion_locked, type: Boolean, desc: "Boolean parameter if the issue's discussion should be locked" + optional :discussion_locked, type: Boolean, desc: " Boolean parameter indicating if the issue's discussion is locked" end params :issue_params do diff --git a/lib/api/notes.rb b/lib/api/notes.rb index b3db366d875..0b9ab4eeb05 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -71,8 +71,6 @@ module API post ":id/#{noteables_str}/:noteable_id/notes" do noteable = find_project_noteable(noteables_str, params[:noteable_id]) - authorize! :create_note, user_project - opts = { note: params[:body], noteable_type: noteables_str.classify, @@ -80,15 +78,12 @@ module API } if can?(current_user, noteable_read_ability_name(noteable), noteable) + authorize! :create_note, noteable + if params[:created_at] && (current_user.admin? || user_project.owner == current_user) opts[:created_at] = params[:created_at] end - noteable_type = opts[:noteable_type].to_s - noteable = Issue.find(opts[:noteable_id]) if noteable_type == 'Issue' - noteable = MergeRequest.find(opts[:noteable_id]) if noteable_type == 'MergeRequest' - authorize! :create_note, noteable if noteable - note = ::Notes::CreateService.new(user_project, current_user, opts).execute if note.valid? diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 10edad462c1..6a6430dfc13 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -239,6 +239,15 @@ describe Projects::NotesController do merge_request.update_attribute(:discussion_locked, true) end + context 'when a noteable is not found' do + it 'returns 404 status' do + request_params[:note][:noteable_id] = 9999 + post :create, request_params.merge(format: :json) + + expect(response).to have_http_status(404) + end + end + context 'when a user is a team member' do it 'returns 302 status for html' do post :create, request_params diff --git a/spec/fixtures/api/schemas/public_api/v4/issues.json b/spec/fixtures/api/schemas/public_api/v4/issues.json index 8c854a43fc6..b7d6dbff031 100644 --- a/spec/fixtures/api/schemas/public_api/v4/issues.json +++ b/spec/fixtures/api/schemas/public_api/v4/issues.json @@ -9,7 +9,7 @@ "title": { "type": "string" }, "description": { "type": ["string", "null"] }, "state": { "type": "string" }, - "discussion_locked": { "type": "boolean" }, + "discussion_locked": { "type": ["boolean", "null"] }, "created_at": { "type": "date" }, "updated_at": { "type": "date" }, "labels": { diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json index 3b42333bb10..5828be5255b 100644 --- a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json +++ b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json @@ -72,7 +72,7 @@ "user_notes_count": { "type": "integer" }, "should_remove_source_branch": { "type": ["boolean", "null"] }, "force_remove_source_branch": { "type": ["boolean", "null"] }, - "discussion_locked": { "type": "boolean" }, + "discussion_locked": { "type": ["boolean", "null"] }, "web_url": { "type": "uri" }, "time_stats": { "time_estimate": { "type": "integer" }, diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb index 9b399d764ea..2cf669e8191 100644 --- a/spec/policies/issuable_policy_spec.rb +++ b/spec/policies/issuable_policy_spec.rb @@ -16,7 +16,7 @@ describe IssuablePolicy, models: true do context 'when the user is a project member' do before do - project.team << [user, :guest] + project.add_guest(user) end it 'can create a note' do diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb index 70a99ed0198..58d36a2c84e 100644 --- a/spec/policies/note_policy_spec.rb +++ b/spec/policies/note_policy_spec.rb @@ -5,9 +5,15 @@ describe NotePolicy, mdoels: true do let(:user) { create(:user) } let(:project) { create(:project, :public) } let(:issue) { create(:issue, project: project) } - let(:note) { create(:note, noteable: issue, author: user, project: project) } - let(:policies) { described_class.new(user, note) } + def policies(noteable = nil) + return @policies if @policies + + noteable ||= issue + note = create(:note, noteable: noteable, author: user, project: project) + + @policies = described_class.new(user, note) + end context 'when the project is public' do context 'when the note author is not a project member' do @@ -19,6 +25,17 @@ describe NotePolicy, mdoels: true do end end + context 'when the noteable is a snippet' do + it 'can edit note' do + policies = policies(create(:project_snippet, project: project)) + + expect(policies).to be_allowed(:update_note) + expect(policies).to be_allowed(:admin_note) + expect(policies).to be_allowed(:resolve_note) + expect(policies).to be_allowed(:read_note) + end + end + context 'when a discussion is locked' do before do issue.update_attribute(:discussion_locked, true) @@ -29,7 +46,7 @@ describe NotePolicy, mdoels: true do project.add_developer(user) end - it 'can eddit a note' do + it 'can edit a note' do expect(policies).to be_allowed(:update_note) expect(policies).to be_allowed(:admin_note) expect(policies).to be_allowed(:resolve_note) diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index f5882c0c74a..fb440fa551c 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -302,6 +302,40 @@ describe API::Notes do expect(private_issue.notes.reload).to be_empty end end + + context 'when the merge request discussion is locked' do + before do + merge_request.update_attribute(:discussion_locked, true) + end + + context 'when a user is a team member' do + subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes", user), body: 'Hi!' } + + it 'returns 200 status' do + subject + + expect(response).to have_http_status(201) + end + + it 'creates a new note' do + expect { subject }.to change { Note.count }.by(1) + end + end + + context 'when a user is not a team member' do + subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes", private_user), body: 'Hi!' } + + it 'returns 403 status' do + subject + + expect(response).to have_http_status(403) + end + + it 'does not create a new note' do + expect { subject }.not_to change { Note.count } + end + end + end end describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 0ed4c2152bb..bcce827fe71 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -144,6 +144,13 @@ describe Issues::UpdateService, :mailer do expect(note).not_to be_nil expect(note.note).to eq 'changed title from **{-Old-} title** to **{+New+} title**' end + + it 'creates system note about discussion lock' do + note = find_note('locked this issue') + + expect(note).not_to be_nil + expect(note.note).to eq 'locked this issue' + end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index a07b7ac2218..b11a1b31f32 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -125,6 +125,13 @@ describe MergeRequests::UpdateService, :mailer do expect(note.note).to eq 'changed target branch from `master` to `target`' end + it 'creates system note about discussion lock' do + note = find_note('locked this issue') + + expect(note).not_to be_nil + expect(note.note).to eq 'locked this issue' + end + context 'when not including source branch removal options' do before do opts.delete(:force_remove_source_branch) From a319418d9c050097a797fbf4f890cebd5256ed43 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Thu, 14 Sep 2017 12:01:07 +0100 Subject: [PATCH 006/136] Merge FE --- .../notes/components/issue_comment_form.vue | 27 +++-- .../notes/components/issue_note_form.vue | 20 +++- .../notes/mixins/issuable_state.js | 15 +++ .../confidential_issue_sidebar.vue | 14 +-- .../components/confidential/edit_form.vue | 9 +- .../confidential/edit_form_buttons.vue | 6 +- .../sidebar/components/lock/edit_form.vue | 62 ++++++++++ .../components/lock/edit_form_buttons.vue | 50 ++++++++ .../components/lock/lock_issue_sidebar.vue | 108 ++++++++++++++++++ .../javascripts/sidebar/sidebar_bundle.js | 70 ++++++++---- .../sidebar/stores/sidebar_store.js | 1 + .../issue/confidential_issue_warning.vue | 16 --- .../components/issue/issue_warning.vue | 53 +++++++++ .../javascripts/vue_shared/mixins/issuable.js | 7 ++ app/assets/stylesheets/framework/buttons.scss | 4 + .../stylesheets/framework/variables.scss | 5 + app/assets/stylesheets/pages/issuable.scss | 27 ++--- app/assets/stylesheets/pages/note_form.scss | 16 ++- app/views/projects/_md_preview.html.haml | 7 ++ app/views/projects/issues/show.html.haml | 5 +- .../merge_requests/_mr_title.html.haml | 2 + app/views/shared/issuable/_sidebar.html.haml | 4 + spec/features/issues_spec.rb | 12 +- .../sidebar/lock/edit_form_buttons_spec.js | 36 ++++++ .../sidebar/lock/edit_form_spec.js | 41 +++++++ .../sidebar/lock/lock_issue_sidebar_spec.js | 73 ++++++++++++ .../issue/confidential_issue_warning_spec.js | 20 ---- .../components/issue/issue_warning_spec.js | 45 ++++++++ 28 files changed, 644 insertions(+), 111 deletions(-) create mode 100644 app/assets/javascripts/notes/mixins/issuable_state.js create mode 100644 app/assets/javascripts/sidebar/components/lock/edit_form.vue create mode 100644 app/assets/javascripts/sidebar/components/lock/edit_form_buttons.vue create mode 100644 app/assets/javascripts/sidebar/components/lock/lock_issue_sidebar.vue delete mode 100644 app/assets/javascripts/vue_shared/components/issue/confidential_issue_warning.vue create mode 100644 app/assets/javascripts/vue_shared/components/issue/issue_warning.vue create mode 100644 app/assets/javascripts/vue_shared/mixins/issuable.js create mode 100644 spec/javascripts/sidebar/lock/edit_form_buttons_spec.js create mode 100644 spec/javascripts/sidebar/lock/edit_form_spec.js create mode 100644 spec/javascripts/sidebar/lock/lock_issue_sidebar_spec.js delete mode 100644 spec/javascripts/vue_shared/components/issue/confidential_issue_warning_spec.js create mode 100644 spec/javascripts/vue_shared/components/issue/issue_warning_spec.js diff --git a/app/assets/javascripts/notes/components/issue_comment_form.vue b/app/assets/javascripts/notes/components/issue_comment_form.vue index 16f4e22aa9b..391a1960eae 100644 --- a/app/assets/javascripts/notes/components/issue_comment_form.vue +++ b/app/assets/javascripts/notes/components/issue_comment_form.vue @@ -6,10 +6,11 @@ import TaskList from '../../task_list'; import * as constants from '../constants'; import eventHub from '../event_hub'; - import confidentialIssue from '../../vue_shared/components/issue/confidential_issue_warning.vue'; + import issueWarning from '../../vue_shared/components/issue/issue_warning.vue'; import issueNoteSignedOutWidget from './issue_note_signed_out_widget.vue'; import markdownField from '../../vue_shared/components/markdown/field.vue'; import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue'; + import issuableStateMixin from '../mixins/issuable_state'; export default { name: 'issueCommentForm', @@ -25,7 +26,7 @@ }; }, components: { - confidentialIssue, + issueWarning, issueNoteSignedOutWidget, markdownField, userAvatarLink, @@ -54,6 +55,9 @@ isIssueOpen() { return this.issueState === constants.OPENED || this.issueState === constants.REOPENED; }, + canCreate() { + return this.getIssueData.current_user.can_create_note; + }, issueActionButtonTitle() { if (this.note.length) { const actionText = this.isIssueOpen ? 'close' : 'reopen'; @@ -89,9 +93,6 @@ endpoint() { return this.getIssueData.create_note_path; }, - isConfidentialIssue() { - return this.getIssueData.confidential; - }, }, methods: { ...mapActions([ @@ -206,6 +207,9 @@ }); }, }, + mixins: [ + issuableStateMixin, + ], mounted() { // jQuery is needed here because it is a custom event being dispatched with jQuery. $(document).on('issuable:change', (e, isClosed) => { @@ -239,15 +243,22 @@
- + class="new-note js-quick-submit common-note-form gfm-form js-main-target-form" + > + + +
+ :is-confidential-issue="isIssueConfidential(getIssueData)">