summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/projects/issues_controller.rb1
-rw-r--r--app/controllers/projects/merge_requests/application_controller.rb1
-rw-r--r--app/controllers/projects/notes_controller.rb14
-rw-r--r--app/helpers/notes_helper.rb4
-rw-r--r--app/models/system_note_metadata.rb2
-rw-r--r--app/policies/issuable_policy.rb5
-rw-r--r--app/policies/note_policy.rb10
-rw-r--r--app/services/issuable_base_service.rb1
-rw-r--r--app/services/issues/update_service.rb8
-rw-r--r--app/services/system_note_service.rb12
-rw-r--r--db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb16
-rw-r--r--db/schema.rb2
-rw-r--r--spec/controllers/projects/notes_controller_spec.rb41
-rw-r--r--spec/lib/gitlab/import_export/safe_model_attributes.yml2
-rw-r--r--spec/policies/issuable_policy_spec.rb28
-rw-r--r--spec/policies/note_policy_spec.rb54
-rw-r--r--spec/services/issues/update_service_spec.rb5
-rw-r--r--spec/services/merge_requests/update_service_spec.rb4
18 files changed, 207 insertions, 3 deletions
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
View Lorry Controller Status