diff options
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) |