Commit b9287208 authored by Jarka Kadlecova's avatar Jarka Kadlecova

Support discussion locking in the backend

parent 1140fcce
...@@ -289,6 +289,7 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -289,6 +289,7 @@ class Projects::IssuesController < Projects::ApplicationController
state_event state_event
task_num task_num
lock_version lock_version
discussion_locked
] + [{ label_ids: [], assignee_ids: [] }] ] + [{ label_ids: [], assignee_ids: [] }]
end end
......
...@@ -34,6 +34,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont ...@@ -34,6 +34,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
:target_project_id, :target_project_id,
:task_num, :task_num,
:title, :title,
:discussion_locked,
label_ids: [] label_ids: []
] ]
......
...@@ -66,7 +66,21 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -66,7 +66,21 @@ class Projects::NotesController < Projects::ApplicationController
params.merge(last_fetched_at: last_fetched_at) params.merge(last_fetched_at: last_fetched_at)
end end
def authorize_admin_note!
return access_denied! unless can?(current_user, :admin_note, note)
end
def authorize_resolve_note! def authorize_resolve_note!
return access_denied! unless can?(current_user, :resolve_note, note) return access_denied! unless can?(current_user, :resolve_note, note)
end 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 end
...@@ -130,8 +130,12 @@ module NotesHelper ...@@ -130,8 +130,12 @@ module NotesHelper
end end
def can_create_note? def can_create_note?
issuable = @issue || @merge_request
if @snippet.is_a?(PersonalSnippet) if @snippet.is_a?(PersonalSnippet)
can?(current_user, :comment_personal_snippet, @snippet) can?(current_user, :comment_personal_snippet, @snippet)
elsif issuable
can?(current_user, :create_note, issuable)
else else
can?(current_user, :create_note, @project) can?(current_user, :create_note, @project)
end end
......
...@@ -2,7 +2,7 @@ class SystemNoteMetadata < ActiveRecord::Base ...@@ -2,7 +2,7 @@ class SystemNoteMetadata < ActiveRecord::Base
ICON_TYPES = %w[ ICON_TYPES = %w[
commit description merge confidential visible label assignee cross_reference commit description merge confidential visible label assignee cross_reference
title time_tracking branch milestone discussion task moved title time_tracking branch milestone discussion task moved
opened closed merged duplicate opened closed merged duplicate locked unlocked
outdated outdated
].freeze ].freeze
......
class IssuablePolicy < BasePolicy class IssuablePolicy < BasePolicy
delegate { @subject.project } 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" desc "User is the assignee or author"
condition(:assignee_or_author) do condition(:assignee_or_author) do
@user && @subject.assignee_or_author?(@user) @user && @subject.assignee_or_author?(@user)
...@@ -12,4 +15,6 @@ class IssuablePolicy < BasePolicy ...@@ -12,4 +15,6 @@ class IssuablePolicy < BasePolicy
enable :read_merge_request enable :read_merge_request
enable :update_merge_request enable :update_merge_request
end end
rule { locked & ~is_project_member }.prevent :create_note
end end
...@@ -2,14 +2,18 @@ class NotePolicy < BasePolicy ...@@ -2,14 +2,18 @@ class NotePolicy < BasePolicy
delegate { @subject.project } delegate { @subject.project }
condition(:is_author) { @user && @subject.author == @user } 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(:for_merge_request, scope: :subject) { @subject.for_merge_request? }
condition(:is_noteable_author) { @user && @subject.noteable.author_id == @user.id } condition(:is_noteable_author) { @user && @subject.noteable.author_id == @user.id }
condition(:editable, scope: :subject) { @subject.editable? } condition(:editable, scope: :subject) { @subject.editable? }
condition(:locked) { @subject.noteable.discussion_locked? }
rule { ~editable | anonymous }.prevent :edit_note rule { ~editable | anonymous }.prevent :edit_note
rule { is_author | admin }.enable :edit_note rule { is_author | admin }.enable :edit_note
rule { can?(:master_access) }.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 rule { is_author }.policy do
enable :read_note enable :read_note
...@@ -21,4 +25,10 @@ class NotePolicy < BasePolicy ...@@ -21,4 +25,10 @@ class NotePolicy < BasePolicy
rule { for_merge_request & is_noteable_author }.policy do rule { for_merge_request & is_noteable_author }.policy do
enable :resolve_note enable :resolve_note
end end
rule { locked & ~is_project_member }.policy do
prevent :update_note
prevent :admin_note
prevent :resolve_note
end
end end
...@@ -57,6 +57,7 @@ class IssuableBaseService < BaseService ...@@ -57,6 +57,7 @@ class IssuableBaseService < BaseService
params.delete(:due_date) params.delete(:due_date)
params.delete(:canonical_issue_id) params.delete(:canonical_issue_id)
params.delete(:project) params.delete(:project)
params.delete(:discussion_locked)
end end
filter_assignee(issuable) filter_assignee(issuable)
......
...@@ -41,6 +41,10 @@ module Issues ...@@ -41,6 +41,10 @@ module Issues
create_confidentiality_note(issue) create_confidentiality_note(issue)
end end
if issue.previous_changes.include?('discussion_locked')
create_discussion_lock_note(issue)
end
added_labels = issue.labels - old_labels added_labels = issue.labels - old_labels
if added_labels.present? if added_labels.present?
...@@ -95,5 +99,9 @@ module Issues ...@@ -95,5 +99,9 @@ module Issues
def create_confidentiality_note(issue) def create_confidentiality_note(issue)
SystemNoteService.change_issue_confidentiality(issue, issue.project, current_user) SystemNoteService.change_issue_confidentiality(issue, issue.project, current_user)
end end
def create_discussion_lock_note(issue)
SystemNoteService.discussion_lock(issue, current_user)
end
end end
end end
...@@ -591,6 +591,18 @@ module SystemNoteService ...@@ -591,6 +591,18 @@ module SystemNoteService
create_note(NoteSummary.new(noteable, project, author, body, action: 'duplicate')) create_note(NoteSummary.new(noteable, project, author, body, action: 'duplicate'))
end 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 private
def notes_for_mentioner(mentioner, noteable, notes) def notes_for_mentioner(mentioner, noteable, notes)
......
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
...@@ -660,6 +660,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do ...@@ -660,6 +660,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do
t.integer "cached_markdown_version" t.integer "cached_markdown_version"
t.datetime "last_edited_at" t.datetime "last_edited_at"
t.integer "last_edited_by_id" t.integer "last_edited_by_id"
t.boolean "discussion_locked", default: false, null: false
end end
add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
...@@ -882,6 +883,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do ...@@ -882,6 +883,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do
t.integer "head_pipeline_id" t.integer "head_pipeline_id"
t.boolean "ref_fetched" t.boolean "ref_fetched"
t.string "merge_jid" t.string "merge_jid"
t.boolean "discussion_locked", default: false, null: false
end end
add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
......
...@@ -232,6 +232,47 @@ describe Projects::NotesController do ...@@ -232,6 +232,47 @@ describe Projects::NotesController do
end end
end 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 end
describe 'DELETE destroy' do describe 'DELETE destroy' do
......
...@@ -25,6 +25,7 @@ Issue: ...@@ -25,6 +25,7 @@ Issue:
- relative_position - relative_position
- last_edited_at - last_edited_at
- last_edited_by_id - last_edited_by_id
- discussion_locked
Event: Event:
- id - id
- target_type - target_type
...@@ -168,6 +169,7 @@ MergeRequest: ...@@ -168,6 +169,7 @@ MergeRequest:
- last_edited_at - last_edited_at
- last_edited_by_id - last_edited_by_id
- head_pipeline_id - head_pipeline_id
- discussion_locked
MergeRequestDiff: MergeRequestDiff:
- id - id
- state - state
......
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
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
...@@ -48,7 +48,8 @@ describe Issues::UpdateService, :mailer do ...@@ -48,7 +48,8 @@ describe Issues::UpdateService, :mailer do
assignee_ids: [user2.id], assignee_ids: [user2.id],
state_event: 'close', state_event: 'close',
label_ids: [label.id], label_ids: [label.id],
due_date: Date.tomorrow due_date: Date.tomorrow,
discussion_locked: true
} }
end end
...@@ -62,6 +63,7 @@ describe Issues::UpdateService, :mailer do ...@@ -62,6 +63,7 @@ describe Issues::UpdateService, :mailer do
expect(issue).to be_closed expect(issue).to be_closed
expect(issue.labels).to match_array [label] expect(issue.labels).to match_array [label]
expect(issue.due_date).to eq Date.tomorrow expect(issue.due_date).to eq Date.tomorrow
expect(issue.discussion_locked).to be_truthy
end end
it 'updates open issue counter for assignees when issue is reassigned' do it 'updates open issue counter for assignees when issue is reassigned' do
...@@ -103,6 +105,7 @@ describe Issues::UpdateService, :mailer do ...@@ -103,6 +105,7 @@ describe Issues::UpdateService, :mailer do
expect(issue.labels).to be_empty expect(issue.labels).to be_empty
expect(issue.milestone).to be_nil expect(issue.milestone).to be_nil
expect(issue.due_date).to be_nil expect(issue.due_date).to be_nil
expect(issue.discussion_locked).to be_falsey
end end
end end
......
...@@ -49,7 +49,8 @@ describe MergeRequests::UpdateService, :mailer do ...@@ -49,7 +49,8 @@ describe MergeRequests::UpdateService, :mailer do
state_event: 'close', state_event: 'close',
label_ids: [label.id], label_ids: [label.id],
target_branch: 'target', target_branch: 'target',
force_remove_source_branch: '1' force_remove_source_branch: '1',
discussion_locked: true
} }
end end
...@@ -73,6 +74,7 @@ describe MergeRequests::UpdateService, :mailer do ...@@ -73,6 +74,7 @@ describe MergeRequests::UpdateService, :mailer do
expect(@merge_request.labels.first.title).to eq(label.name) expect(@merge_request.labels.first.title).to eq(label.name)
expect(@merge_request.target_branch).to eq('target') expect(@merge_request.target_branch).to eq('target')
expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1') expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1')
expect(@merge_request.discussion_locked).to be_truthy
end end
it 'executes hooks with update action' do it 'executes hooks with update action' do
......
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