summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJarka Kadlecova <jarka@gitlab.com>2017-09-01 14:03:57 +0200
committerJarka Kadlecova <jarka@gitlab.com>2017-09-14 14:50:32 +0200
commit994e7d135947ca162c147c5e0992a0190de22808 (patch)
treecd9ea4d93269c8597541f8c59e89a83ca2b56d2b
parent2b82f907abf2074ac332531d6142893d081f44b9 (diff)
downloadgitlab-ce-994e7d135947ca162c147c5e0992a0190de22808.tar.gz
Create system notes for MR too, improve doc + clean up code
-rw-r--r--app/controllers/projects/notes_controller.rb7
-rw-r--r--app/models/concerns/noteable.rb4
-rw-r--r--app/policies/issuable_policy.rb11
-rw-r--r--app/policies/note_policy.rb10
-rw-r--r--app/services/issuable_base_service.rb11
-rw-r--r--app/services/issues/update_service.rb8
-rw-r--r--app/services/system_note_service.rb9
-rw-r--r--changelogs/unreleased/18608-lock-issues.yml5
-rw-r--r--db/migrate/20170815221154_add_discussion_locked_to_issuable.rb (renamed from db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb)5
-rw-r--r--db/schema.rb4
-rw-r--r--doc/api/issues.md2
-rw-r--r--doc/api/merge_requests.md2
-rw-r--r--lib/api/issues.rb2
-rw-r--r--lib/api/notes.rb9
-rw-r--r--spec/controllers/projects/notes_controller_spec.rb9
-rw-r--r--spec/fixtures/api/schemas/public_api/v4/issues.json2
-rw-r--r--spec/fixtures/api/schemas/public_api/v4/merge_requests.json2
-rw-r--r--spec/policies/issuable_policy_spec.rb2
-rw-r--r--spec/policies/note_policy_spec.rb23
-rw-r--r--spec/requests/api/notes_spec.rb34
-rw-r--r--spec/services/issues/update_service_spec.rb7
-rw-r--r--spec/services/merge_requests/update_service_spec.rb7
22 files changed, 121 insertions, 54 deletions
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
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)